feat(starknet_l1_provider): add L1 gas price scraper

[guy-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2025-02-06T08:41:19Z
• Started at 2025-02-06T12:05:09Z
• Started at 2025-02-06T12:10:06Z
• Started at 2025-02-06T12:23:51Z
• Started at 2025-02-06T14:13:54Z
• Started at 2025-02-09T09:02:11Z
• Started at 2025-02-09T13:46:59Z
• Started at 2025-02-09T13:59:22Z
• Started at 2025-02-09T14:24:01Z
• Started at 2025-02-09T15:28:51Z
• Started at 2025-02-10T09:58:15Z
• Started at 2025-02-10T10:32:44Z
• Started at 2025-02-11T11:33:55Z
• Started at 2025-02-11T13:05:09Z
• Started at 2025-02-11T13:18:41Z
• Started at 2025-02-12T14:38:29Z
• Started at 2025-02-13T12:22:39Z

[guy-starkware]

• feat(starknet_l1_provider): integrate gas price components in the node #4081
• chore(starknet_l1_gas_price): add new crate starknet_l1_gas_price_types #4124
• feat(starknet_l1_provider): add L1 gas price scraper #4000 👈 (View in Graphite)
• feat(starknet_l1_provider): add the L1GasPriceProvider implementation #3794
• feat(starknet_l1_provider): add the trait interface for L1 gas price provider #3792
• feat(starknet_l1_provider): add interface to query ethereum gas price #3679
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.792 ms 34.819 ms 34.852 ms]
change: [-4.1032% -2.6019% -1.3067%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) high mild
2 (2.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.893 ms 34.953 ms 35.019 ms]
change: [-6.9114% -4.6523% -2.6302%] (p = 0.00 < 0.05)
Performance has improved.
Found 16 outliers among 100 measurements (16.00%)
11 (11.00%) high mild
5 (5.00%) high severe

[asmaastarkware]

Reviewed 1 of 8 files at r1, 2 of 8 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 6 of 9 files reviewed, 6 unresolved discussions

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 90 at r3 (raw file):

=

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper_test.rs line 56 at r3 (raw file):

#[async_trait]
impl BaseLayerContract for FakeBaseLayerContract {

why didn't u use automock?

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 85 at r3 (raw file):

        let latest_l1_block_number = self
            .base_layer
            .latest_l1_block_number(self.config.finality)

@matan-starkware what does it mean finality here (Ethereum)?
finality in consensus means after the commit stage?

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 89 at r3 (raw file):

            .map_err(L1GasPriceScraperError::BaseLayerError)?;

        if let Some(latest_l1_block_number) = latest_l1_block_number {

use let else

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 113 at r3 (raw file):

                        .map_err(L1GasPriceScraperError::GasPriceProviderError)?;

                    self.next_block_number_to_fetch = latest_l1_block_number + 1;

good catch to update it here (in case of failure we know where to resume)
u mean block_number?

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 140 at r3 (raw file):

    // Leaky abstraction, these errors should not propagate here.
    #[error(transparent)]
    NetworkError(ClientError),

if it's unused, add it when needed

[guy-starkware]

Reviewable status: 6 of 9 files reviewed, 6 unresolved discussions (waiting on @asmaastarkware)

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 89 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

use let else

Done.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 90 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

=

I think this does not include =

If next_block_number_to_fetch is 10 and latest_l1_block_number is also 10, that means we need to fetch that block. If we use >= instead of > we will not get that.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 113 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

good catch to update it here (in case of failure we know where to resume)
u mean block_number?

Yes, thank you!

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 140 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

if it's unused, add it when needed

Yes, I'll move it to the next PR.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper_test.rs line 56 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

why didn't u use automock?

I tried, it was more complicated for some reason.

[matan-starkware]

Reviewed 2 of 4 files at r5, all commit messages.
Reviewable status: 4 of 9 files reviewed, 10 unresolved discussions (waiting on @asmaastarkware)

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 85 at r3 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

@matan-starkware what does it mean finality here (Ethereum)?
finality in consensus means after the commit stage?

Finality is an ambiguous term that refers to when we are confident that a block will not be reverted.

In the strict sense it means that over 2/3 of validators would need to fork the system (in most PoS systems). In Tendermint this happens immediately (e.g. as soon as H is decided). For Ethereum this means basically that 2 epochs (64 blocks) have passed. This though takes 12m and we don't want to wait that long due to stale data.

In practice people use much smaller numbers of blocks for finality; an acceptable probability that the block will not reorg. If I remember correctly, Ethereum switch to PoS about 2.5 years ago and has never had a reorg longer than 2 blocks. So we use 3 blocks as our finality, as this is only 30s of lag from the tip of Ethereum and has good enough likelihood to be stable. It is though configurable.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper_test.rs line 56 at r3 (raw file):

Previously, guy-starkware wrote…

I tried, it was more complicated for some reason.

It is complicated for 2 reasons:

1. Associated types - can get this to work by following the automock documentation. I made a new error type for the example below
2. Parameters with lifetimes - this one seems to be a known issue which unfortunately requires changing the function definition, but luckily seems to be just a boilerplate change, nothing too complex to implement. Can't mock trait containing function that takes an argument of type Option<&T> asomers/mockall#61

I lean towards this being worthwhile, but let's check with Gilad what he thinks about making this change.

#[derive(thiserror::Error, Debug)]
pub enum MockError {}

/// Interface for getting data from the Starknet base contract.
#[cfg_attr(test, automock(type Error = MockError;))]
#[async_trait]
pub trait BaseLayerContract {
    type Error: Error + Display + Debug;

    /// Get specific events from the Starknet base contract between two L1 block numbers.
    async fn events<'a>(
        &'a self,
        block_range: RangeInclusive<L1BlockNumber>,
        event_identifiers: &'a [&'a str],
    ) -> Result<Vec<L1Event>, Self::Error>;

}

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 23 at r5 (raw file):

type L1GasPriceScraperResult<T, B> = Result<T, L1GasPriceScraperError<B>>;
pub type SharedL1GasPriceProvider = Arc<dyn L1GasPriceProviderClient>;

This should get moved into the infra "types" crate when we make that

Code quote:

pub type SharedL1GasPriceProvider = Arc<dyn L1GasPriceProviderClient>;

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 27 at r5 (raw file):

// TODO(guyn): find a way to synchronize the value of number_of_blocks_for_mean
// with the one in L1GasPriceProviderConfig. In the end they should not be config
// items but values drawn from VersionedConstants.

Just to be clear, from the perspective of the scraper these will remain in the config, it has no idea about versioned constants. From the global perspective, yes the source of these values is versioned constants.

Code quote:

// TODO(guyn): find a way to synchronize the value of number_of_blocks_for_mean
// with the one in L1GasPriceProviderConfig. In the end they should not be config
// items but values drawn from VersionedConstants.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 46 at r5 (raw file):

            finality: 0,
            polling_interval: Duration::from_secs(1),
            number_of_blocks_for_mean: 300,

Why not copy the defaults from Gilad's L1ScraperConfig?

Code quote:

            l1_block_to_start_scraping_from: 0,
            chain_id: ChainId::Mainnet,
            finality: 0,
            polling_interval: Duration::from_secs(1),
            number_of_blocks_for_mean: 300,

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 132 at r5 (raw file):

    #[error(transparent)]
    NetworkError(ClientError),
}

Move this above the impls (I recommend putting this above the structs which have impls also)

https://github.com/rust-lang/rust-analyzer/blob/master/docs/dev/style.md#order-of-items

Code quote:

#[derive(Error, Debug)]
pub enum L1GasPriceScraperError<T: BaseLayerContract + Send + Sync> {
    #[error("Base layer error: {0}")]
    BaseLayerError(T::Error),
    #[error("Could not update gas price provider: {0}")]
    GasPriceProviderError(L1GasPriceProviderError),
    // Leaky abstraction, these errors should not propagate here.
    #[error(transparent)]
    NetworkError(ClientError),
}

[guy-starkware]

Reviewable status: 4 of 9 files reviewed, 10 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 140 at r3 (raw file):

Previously, guy-starkware wrote…

Yes, I'll move it to the next PR.

I actually do need the error type.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 23 at r5 (raw file):

Previously, matan-starkware wrote…

This should get moved into the infra "types" crate when we make that

Should I do it in the next PR or here?

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 27 at r5 (raw file):

Previously, matan-starkware wrote…

Just to be clear, from the perspective of the scraper these will remain in the config, it has no idea about versioned constants. From the global perspective, yes the source of these values is versioned constants.

got it.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 46 at r5 (raw file):

Previously, matan-starkware wrote…

Why not copy the defaults from Gilad's L1ScraperConfig?

Good question. I'll copy them.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 132 at r5 (raw file):

Previously, matan-starkware wrote…

Move this above the impls (I recommend putting this above the structs which have impls also)

https://github.com/rust-lang/rust-analyzer/blob/master/docs/dev/style.md#order-of-items

Done.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper_test.rs line 56 at r3 (raw file):

Previously, matan-starkware wrote…

It is complicated for 2 reasons:

1. Associated types - can get this to work by following the automock documentation. I made a new error type for the example below
2. Parameters with lifetimes - this one seems to be a known issue which unfortunately requires changing the function definition, but luckily seems to be just a boilerplate change, nothing too complex to implement. Can't mock trait containing function that takes an argument of type Option<&T> asomers/mockall#61

I lean towards this being worthwhile, but let's check with Gilad what he thinks about making this change.

#[derive(thiserror::Error, Debug)]
pub enum MockError {}

/// Interface for getting data from the Starknet base contract.
#[cfg_attr(test, automock(type Error = MockError;))]
#[async_trait]
pub trait BaseLayerContract {
    type Error: Error + Display + Debug;

    /// Get specific events from the Starknet base contract between two L1 block numbers.
    async fn events<'a>(
        &'a self,
        block_range: RangeInclusive<L1BlockNumber>,
        event_identifiers: &'a [&'a str],
    ) -> Result<Vec<L1Event>, Self::Error>;

}

I'll give it another try. The mock error certainly helps.

[matan-starkware]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 5 of 9 files reviewed, 9 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 23 at r5 (raw file):

Previously, guy-starkware wrote…

Should I do it in the next PR or here?

If we're doing this it's a separate PR. What did infra team say about this?
If they don't want a separate crate I want the files to be separate for this stuff.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper_test.rs line 56 at r3 (raw file):

Previously, guy-starkware wrote…

I'll give it another try. The mock error certainly helps.

Ok I would rather see the tests using the mock, so can you make this change in a separate PR, send it to me and Asmaa and then rebase this PR on top of that one?

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 53 at r6 (raw file):

    fn default() -> Self {
        Self {
            l1_block_to_start_scraping_from: 0,

I'm ok with this in the config so that we can pass an override in from the command line, but I don't want scraper reading this field. The scraper should have next_block_number_to_fetch configured when built.

Please add a comment about this.

Code quote:

            l1_block_to_start_scraping_from: 0,

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 130 at r6 (raw file):

        }

        Ok(())

Remove this. Just scrape from self.next_height until the provider doesn't know the block (returns None)

Suggestion:

        loop {
            let price_sample = self
                .base_layer
                .get_price_sample(self.next_block_number_to_fetch)
                .await
                .map_err(L1GasPriceScraperError::BaseLayerError)?;
            match price_sample {
                None => break,
                Some(sample) => {
                    self.l1_gas_price_provider
                        .add_price_info(BlockNumber(block_number), sample)
                        .map_err(L1GasPriceScraperError::GasPriceProviderError)?;
                    self.next_block_number_to_fetch += 1;
                }
            }
        }

        Ok(())

[guy-starkware]

Reviewable status: 5 of 9 files reviewed, 9 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 85 at r3 (raw file):

Previously, matan-starkware wrote…

Finality is an ambiguous term that refers to when we are confident that a block will not be reverted.

In the strict sense it means that over 2/3 of validators would need to fork the system (in most PoS systems). In Tendermint this happens immediately (e.g. as soon as H is decided). For Ethereum this means basically that 2 epochs (64 blocks) have passed. This though takes 12m and we don't want to wait that long due to stale data.

In practice people use much smaller numbers of blocks for finality; an acceptable probability that the block will not reorg. If I remember correctly, Ethereum switch to PoS about 2.5 years ago and has never had a reorg longer than 2 blocks. So we use 3 blocks as our finality, as this is only 30s of lag from the tip of Ethereum and has good enough likelihood to be stable. It is though configurable.

👍

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 23 at r5 (raw file):

Previously, matan-starkware wrote…

If we're doing this it's a separate PR. What did infra team say about this?
If they don't want a separate crate I want the files to be separate for this stuff.

I haven't asked them yet, but from looking at so many examples in other crates I think this is what is expected. I'm adding anothe PR.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 53 at r6 (raw file):

Previously, matan-starkware wrote…

I'm ok with this in the config so that we can pass an override in from the command line, but I don't want scraper reading this field. The scraper should have next_block_number_to_fetch configured when built.

Please add a comment about this.

Done.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper.rs line 130 at r6 (raw file):

Previously, matan-starkware wrote…

Remove this. Just scrape from self.next_height until the provider doesn't know the block (returns None)

Done.

---

crates/starknet_l1_gas_price/src/l1_gas_price_scraper_test.rs line 56 at r3 (raw file):

Previously, matan-starkware wrote…

Ok I would rather see the tests using the mock, so can you make this change in a separate PR, send it to me and Asmaa and then rebase this PR on top of that one?

I think I can make it work.