-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(starknet_l1_provider): add the L1GasPriceProvider implementation #3794
Conversation
7c61569
to
f9e47dd
Compare
1b01757
to
76b5421
Compare
f9e47dd
to
740fc7b
Compare
76b5421
to
d32d4f3
Compare
740fc7b
to
4dcedb4
Compare
d32d4f3
to
d3793af
Compare
4dcedb4
to
9803183
Compare
d3793af
to
3bfb770
Compare
9803183
to
6bf02d6
Compare
3bfb770
to
4d73838
Compare
6bf02d6
to
24cada3
Compare
4d73838
to
d4c3ad4
Compare
24cada3
to
ece1129
Compare
0203056
to
02ab392
Compare
852008c
to
450282a
Compare
02ab392
to
e89fe3c
Compare
Benchmark movements: full_committer_flow performance improved 😺 |
450282a
to
73e55cf
Compare
e89fe3c
to
6c8d247
Compare
6c8d247
to
cf06a20
Compare
1337a1b
to
b0aa91a
Compare
b0aa91a
to
28a2f0c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 9 files at r3, all commit messages.
Reviewable status: 4 of 10 files reviewed, 13 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 63 at r2 (raw file):
Previously, guy-starkware wrote…
OK, but I don't really understand why not make it serializable yet.
The why is that I want this PR to focus on creating and testing the provider. Making the config serializable and connecting to the component infrastructure is a distinct PR.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 76 at r5 (raw file):
) -> Result<(), L1GasPriceProviderError> { let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0); if height.0 != last_plus_one {
Suggestion:
if height.0 != self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0) {
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 77 at r5 (raw file):
let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0); if height.0 != last_plus_one { return Err(L1GasPriceProviderError::AddPriceInfoError(format!(
Suggestion:
InvalidHeight(
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 103 at r5 (raw file):
// Could not find a block with the requested timestamp and lag. let Some(last_index_rev) = index_last_timestamp_rev else { return Err(L1GasPriceProviderError::GetPriceInfoError(format!(
Suggestion:
MissingData
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 109 at r5 (raw file):
}; // We need to convert the index to the forward direction. let last_index = self.data.len() - last_index_rev - 1;
Why bother with this? It seems like you keep doing +1 later, so just keep the old value to use
Code quote:
let last_index = self.data.len() - last_index_rev - 1;
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 111 at r5 (raw file):
let last_index = self.data.len() - last_index_rev - 1; let num_blocks = usize::try_from(self.config.number_of_blocks_for_mean).unwrap();
expect
Code quote:
unwrap();
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 113 at r5 (raw file):
let num_blocks = usize::try_from(self.config.number_of_blocks_for_mean).unwrap(); if last_index + 1 < num_blocks { return Err(L1GasPriceProviderError::GetPriceInfoError(format!(
Suggestion:
MissingData
28a2f0c
to
991f91a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 10 files reviewed, 13 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 63 at r2 (raw file):
Previously, matan-starkware wrote…
The why is that I want this PR to focus on creating and testing the provider. Making the config serializable and connecting to the component infrastructure is a distinct PR.
OK. I just thought that was standard boilerplate but I don't mind doing that later.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 109 at r5 (raw file):
Previously, matan-starkware wrote…
Why bother with this? It seems like you keep doing +1 later, so just keep the old value to use
Yes, good idea.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 111 at r5 (raw file):
Previously, matan-starkware wrote…
expect
I highly doubt this will ever trigger, but ok.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 76 at r5 (raw file):
) -> Result<(), L1GasPriceProviderError> { let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0); if height.0 != last_plus_one {
I'm reusing this variable in the error message, and don't want to repeat the whole thing.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 77 at r5 (raw file):
let last_plus_one = self.data.back().map(|(h, _)| h.0 + 1).unwrap_or(0); if height.0 != last_plus_one { return Err(L1GasPriceProviderError::AddPriceInfoError(format!(
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 103 at r5 (raw file):
// Could not find a block with the requested timestamp and lag. let Some(last_index_rev) = index_last_timestamp_rev else { return Err(L1GasPriceProviderError::GetPriceInfoError(format!(
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 113 at r5 (raw file):
let num_blocks = usize::try_from(self.config.number_of_blocks_for_mean).unwrap(); if last_index + 1 < num_blocks { return Err(L1GasPriceProviderError::GetPriceInfoError(format!(
Done.
ea204ab
to
5d496df
Compare
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 9 files at r3, 7 of 7 files at r7, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @guy-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 63 at r2 (raw file):
Previously, guy-starkware wrote…
OK. I just thought that was standard boilerplate but I don't mind doing that later.
It's standard, but it isn't needed to define this struct. It's needed when we connect this to the node's infrastructure.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 111 at r5 (raw file):
Previously, guy-starkware wrote…
I highly doubt this will ever trigger, but ok.
Agreed. I thought we had a linter for this, CI should have failed.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 35 at r6 (raw file):
InvalidHeight(String), #[error("Failed to add price info: {0}")] MissingData(String),
Suggestion:
#[error("Failed to calculate price info: {0}")]
MissingData(String),
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 37 at r6 (raw file):
MissingData(String), #[error("Failed to get price info: {0}")] GetPriceInfoError(String),
Unused, remove?
Code quote:
#[error("Failed to get price info: {0}")]
GetPriceInfoError(String),
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 12 at r6 (raw file):
// of 6 + 60 (which is the lag margin). // Returns the provider, the final timestamp, and the lag margin. fn make_provider_with_few_samples() -> (L1GasPriceProvider, u64, u64) {
Just set up the provider, don't get to prescriptive about how it will be used. We expect anyone reading/writing a test will look at this setup function.
Suggestion:
fn make_provider() -> (L1GasPriceProvider, Vec<PriceSample>) {
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 24 at r6 (raw file):
let sample = PriceSample { timestamp: time, base_fee_per_gas: price, blob_fee: price + 1 }; provider.add_price_info(BlockNumber(block_num), sample).unwrap(); }
The issue with using let price = i.try_into().unwrap();
is that it masks what range you are averaging over. avg(1, 2, 3, 4, 5) == avg(2, 3, 4)
.
I recommend building the vec manually (you explicitly write vec~ with 5 entries):
- prices should be multiples of number_of_blocks_for_mean so tests don't need to worry about integer division issues
- Make sure the prices are "random" (e.g. avoid the issue above).
Explain these goes in a comment above the vec.
Code quote:
for i in 0..5 {
let block_num = i.try_into().unwrap();
let price = i.try_into().unwrap();
let time = (i * 2).try_into().unwrap();
let sample = PriceSample { timestamp: time, base_fee_per_gas: price, blob_fee: price + 1 };
provider.add_price_info(BlockNumber(block_num), sample).unwrap();
}
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 36 at r6 (raw file):
// This calculation will grab 3 samples from the middle of the range. let (gas_price, data_gas_price) = provider.get_price_info(BlockTimestamp(final_timestamp + lag)).unwrap();
If you really want to be pedantic we can return the config from make_provider_with_few_samples
but this saves a bit of code. (do this throughout the tests)
Suggestion:
provider.config.lag
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 38 at r6 (raw file):
provider.get_price_info(BlockTimestamp(final_timestamp + lag)).unwrap(); // The gas prices (set arbitrarily to equal the block number) should go from let gas_price_calculation = (1 + 2 + 3) / 3;
Makes the test a little more flexible; you are committing this test to elements (1, 2, 3) instead of assuming the prices directly.
Suggestion:
let gas_price_calculation = (samples[1].gas_fee + samples[2].gas_fee + samples[3].gas_fee) / 3;
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 40 at r6 (raw file):
let gas_price_calculation = (1 + 2 + 3) / 3; // The data gas is one more than the gas price. let data_gas_calculation = gas_price_calculation + 1;
change this to be like the gas price
Code quote:
// The data gas is one more than the gas price.
let data_gas_calculation = gas_price_calculation + 1;
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 61 at r6 (raw file):
// This should not change the results if we ask for the same timestamp. assert_eq!(gas_price, gas_price_new); assert_eq!(data_gas_price, data_gas_price_new);
Add another and check we get a missing data error
Code quote:
// This should not change the results if we ask for the same timestamp.
assert_eq!(gas_price, gas_price_new);
assert_eq!(data_gas_price, data_gas_price_new);
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 75 at r6 (raw file):
provider.get_price_info(BlockTimestamp(final_timestamp + lag + 2)).unwrap(); assert!(gas_price_new > gas_price); assert!(data_gas_price_new > data_gas_price);
switching to multiplication shows the intent is to increase by another quanta of time, as opposed to adding 2 is a magic number based on what we set the lag to (and would need to be updated if we change the lag).
Suggestion:
// If we take a higher timestamp the gas prices should change.
let (gas_price_new, data_gas_price_new) =
provider.get_price_info(BlockTimestamp(final_timestamp + lag * 2)).unwrap();
assert_ne!(gas_price_new, gas_price);
assert_ne!(data_gas_price_new, data_gas_price);
09bc942
to
c42acce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 10 files reviewed, 11 unresolved discussions (waiting on @matan-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 111 at r5 (raw file):
Previously, matan-starkware wrote…
Agreed. I thought we had a linter for this, CI should have failed.
I have used unwraps in several place before, never saw it show up on CI. I think in cases of trivial conversion (like integer types) this is not really an issue, but of course I'll go with common convensions.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 37 at r6 (raw file):
Previously, matan-starkware wrote…
Unused, remove?
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 24 at r6 (raw file):
Previously, matan-starkware wrote…
The issue with using
let price = i.try_into().unwrap();
is that it masks what range you are averaging over.avg(1, 2, 3, 4, 5) == avg(2, 3, 4)
.I recommend building the vec manually (you explicitly write vec~ with 5 entries):
- prices should be multiples of number_of_blocks_for_mean so tests don't need to worry about integer division issues
- Make sure the prices are "random" (e.g. avoid the issue above).
Explain these goes in a comment above the vec.
I think I can accomplish most of this using (block number)^2. I'm not sure I can avoid integer division in all cases, and I don't think I should... we are explicitly calculating a mean of integers and will round down the results.
We should ask Product if they want us to use median instead, which is much better in this case, but that's a different matter.
WDYT?
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 36 at r6 (raw file):
Previously, matan-starkware wrote…
If you really want to be pedantic we can return the config from
make_provider_with_few_samples
but this saves a bit of code. (do this throughout the tests)
I don't understand what is the problem with the way it is now...
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 38 at r6 (raw file):
Previously, matan-starkware wrote…
Makes the test a little more flexible; you are committing this test to elements (1, 2, 3) instead of assuming the prices directly.
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 40 at r6 (raw file):
Previously, matan-starkware wrote…
change this to be like the gas price
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 61 at r6 (raw file):
Previously, matan-starkware wrote…
Add another and check we get a missing data error
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 75 at r6 (raw file):
Previously, matan-starkware wrote…
switching to multiplication shows the intent is to increase by another quanta of time, as opposed to adding 2 is a magic number based on what we set the lag to (and would need to be updated if we change the lag).
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider.rs
line 35 at r6 (raw file):
InvalidHeight(String), #[error("Failed to add price info: {0}")] MissingData(String),
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 24 at r6 (raw file):
Previously, guy-starkware wrote…
I think I can accomplish most of this using (block number)^2. I'm not sure I can avoid integer division in all cases, and I don't think I should... we are explicitly calculating a mean of integers and will round down the results.
We should ask Product if they want us to use median instead, which is much better in this case, but that's a different matter.
WDYT?
Product has specified mean. The units in practice are so large this won't matter.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 36 at r6 (raw file):
Previously, guy-starkware wrote…
I don't understand what is the problem with the way it is now...
I'm trying to simplify the setup interface to return fewer things. The lag parameter is part of the config, so I'd rather access it as part of the config
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 16 at r8 (raw file):
// of 6 + 60 (which is the lag margin). // Returns the provider, the final timestamp, and the lag margin. fn make_provider_with_few_samples() -> (L1GasPriceProvider, u64, Vec<PriceSample>) {
I don't think we should have all these comments, the tests themselves show how to use this.
I also don't think we should return the final timestamp, we can use
let (gas_price, data_gas_price) =
provider.get_price_info(BlockTimestamp(samples.last().unwrap().timestamp)).unwrap();
(of you can use samples[len-1] + lag
if you really want to).
Suggestion:
// Make a provider with each consecutive sample spaced `config.lag_margin_seconds` apart.
fn setup() -> (L1GasPriceProvider, Vec<PriceSample>) {
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 41 at r8 (raw file):
// This calculation will grab 3 samples from the middle of the range. let (gas_price, data_gas_price) = provider.get_price_info(BlockTimestamp(final_timestamp + lag)).unwrap();
Let's just take the last timestamp instead of returning and adding
Suggestion:
let (gas_price, data_gas_price) =
provider.get_price_info(BlockTimestamp(samples.last().unwrap().timestamp)).unwrap();
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 46 at r8 (raw file):
let gas_price_calculation = (samples[1].base_fee_per_gas + samples[2].base_fee_per_gas + samples[3].base_fee_per_gas) / 3;
and data gas
Suggestion:
config.num_samples;
c42acce
to
4de3321
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @matan-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 12 at r6 (raw file):
Previously, matan-starkware wrote…
Just set up the provider, don't get to prescriptive about how it will be used. We expect anyone reading/writing a test will look at this setup function.
see the comment below.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 24 at r6 (raw file):
Previously, matan-starkware wrote…
Product has specified mean. The units in practice are so large this won't matter.
Oh, I'm not worried about the integer division, I'm saying median will be better because it will not be sensitive to short spikes in the gas price, while still giving a good estimate of the prices. Of course this isn't something we can decide on by ourselves... but maybe worth telling them about it.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 36 at r6 (raw file):
Previously, matan-starkware wrote…
I'm trying to simplify the setup interface to return fewer things. The lag parameter is part of the config, so I'd rather access it as part of the config
OK, got it. I just noticed that I'm making the same let lag = ...
statement every time I call the setup function, so I wanted to include it to save some boilerplate. But I don't really care either way.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 16 at r8 (raw file):
Previously, matan-starkware wrote…
I don't think we should have all these comments, the tests themselves show how to use this.
I also don't think we should return the final timestamp, we can use
let (gas_price, data_gas_price) = provider.get_price_info(BlockTimestamp(samples.last().unwrap().timestamp)).unwrap();
(of you can use
samples[len-1] + lag
if you really want to).
I think there's a bit of confusion here between the lag and the block interval. As this is evidently not clear, I'm leaving the big comment.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 41 at r8 (raw file):
Previously, matan-starkware wrote…
Let's just take the last timestamp instead of returning and adding
The lag is not the same as the interval between samples. I still have to add lag because that tells the provider how far back to go to get the last sample to look at.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 46 at r8 (raw file):
Previously, matan-starkware wrote…
and data gas
Done.
Benchmark movements: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 16 at r8 (raw file):
Previously, guy-starkware wrote…
I think there's a bit of confusion here between the lag and the block interval. As this is evidently not clear, I'm leaving the big comment.
Ah I thought you had changed the lag definition. All right I want us to change the comment though to be a little more general and also shorter. Here's a suggestion:
// Make a provider with five samples. Timestamps are 2 seconds apart, starting from 0.
// To get the prices for a specific set of blocks (according to the final block) use samples[final_index].timestamp + config.lag
.
// Returns the provider and a vector of samples to compare with.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 37 at r9 (raw file):
let (provider, samples) = make_provider(); let lag = provider.config.lag_margin_seconds; let number: u128 = provider.config.number_of_blocks_for_mean.into();
Suggestion:
let num_samples: u128 = provider.config.number_of_blocks_for_mean.into();
4de3321
to
96b7a48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 16 at r8 (raw file):
Previously, matan-starkware wrote…
Ah I thought you had changed the lag definition. All right I want us to change the comment though to be a little more general and also shorter. Here's a suggestion:
// Make a provider with five samples. Timestamps are 2 seconds apart, starting from 0.
// To get the prices for a specific set of blocks (according to the final block) usesamples[final_index].timestamp + config.lag
.
// Returns the provider and a vector of samples to compare with.
Done.
crates/starknet_l1_gas_price/src/l1_gas_price_provider_test.rs
line 37 at r9 (raw file):
let (provider, samples) = make_provider(); let lag = provider.config.lag_margin_seconds; let number: u128 = provider.config.number_of_blocks_for_mean.into();
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)
No description provided.