Create uninitialized task for v1 gas price service #2442
Conversation
…data_tests.rs Co-authored-by: Rafał Chabowski <[email protected]>
…-be-received-out-of-order
MitchTurner
changed the base branch from
master
to
feature/allow-DA-blocks-to-be-received-out-of-order
…to feature/init-task-for-v1-gas-price-service
| let start = read_algo.next_gas_price(); | ||
| let mut watcher = StateWatcher::started(); |
There was a problem hiding this comment.
is there a reason this isn't StateWatcher::default()?
There was a problem hiding this comment.
There were some places that process_l2_block_res wasn't getting called because of get_l2_block yielding a result, but because the service was shutting down (and we always check if there is an l2 block available before shutting down).
I added the started in a few places that it was shutting down early.
That being said, this isn't doing what the test says it's doing next__new_l2_block_saves_old_metadata. It's checking the gas price:
// then
let new = read_algo.next_gas_price();
assert_ne!(start, new);So I'll fix that.
| @@ -141,13 +141,15 @@ where | |||
| Metadata: MetadataStorage, | |||
| { | |||
| async fn run(&mut self, watcher: &mut StateWatcher) -> TaskNextAction { | |||
| tracing::debug!("Starting gas price service"); | |||
There was a problem hiding this comment.
We might not want to log a "sunny day" events on the debug! level. Please consider using trace!.
Reference: this comment (and a couple of other comments in the linked PR).
MitchTurner
dismissed stale reviews from rymnc and netrome
The base branch was changed.
| @@ -92,7 +92,9 @@ impl From<&V1AlgorithmConfig> for AlgorithmUpdaterV1 { | |||
| .map(|size| u128::from(*size)) | |||
| .sum(); | |||
| Self { | |||
| new_scaled_exec_price: value.new_exec_gas_price, | |||
| new_scaled_exec_price: value | |||
There was a problem hiding this comment.
Why do we multiple by the gas_price_factor here? And what is gas_price_factor? I don't see where this value comes from and it is not clear what is the purpose of it.
There was a problem hiding this comment.
The gas_price_factor is just a scaling factor to make sure we can track the change between small values. Like we do for gas costs. If the gas price is 1, it is stored as 100 as the scaled value, then we can increase it by 10% or 5% under the hood but it will still be returned as 1 until it crosses 150 or whatever.
There was a problem hiding this comment.
The gas price factor can be different on different heights. Do we take it into account?
I though we work with Wei and because of that we don't need to scale gas price.
There was a problem hiding this comment.
What if the price is 1 Wei, which it often is? Possibly less so in the future, but still.
There was a problem hiding this comment.
I added a TODO to remove this with this issue:
#2481
There was a problem hiding this comment.
It would be better if you added todo for the field of the config instead of here=)
There was a problem hiding this comment.
I'll move it in my next pr. Gonna merge.
| pub block_stream: BoxStream<SharedImportResult>, | ||
| pub(crate) shared_algo: SharedV1Algorithm, | ||
| pub(crate) algo_updater: AlgorithmUpdaterV1, | ||
| pub(crate) metadata_storage: Metadata, |
There was a problem hiding this comment.
Why is metadata generic? We agreed in the past that the V1 service will work with V1 metadata. In this case, we don't need to over-abstract things and can work directly with the right type that belongs to the service.
There was a problem hiding this comment.
Or is it MetadataStorage? Then why do we have MetadataStorage and gas_price_db? Maybe gas_price_db could be transactional there associated type Transaction implements MetadataStorage trait?
There was a problem hiding this comment.
Working on this in the next PR for the unrecorded blocks stuff.
| config: V1AlgorithmConfig, | ||
| genesis_block_height: BlockHeight, | ||
| settings: SettingsProvider, | ||
| block_stream: BoxStream<SharedImportResult>, |
There was a problem hiding this comment.
It is better to have stream here, because we plan to work with the stream. We could use generic for the stream, but it means one more generic in the already complicated definition.
Boxed stream doesn't hurt the performance here, because we receive event about block each second.
| Metadata: MetadataStorage, | ||
| DA: DaBlockCostsSource, | ||
| { | ||
| let v1_config = config.v1().ok_or(anyhow::anyhow!("Expected V1 config"))?; |
There was a problem hiding this comment.
fuel-core should pass GasPriceV1Config based on the variant of the GasPriceConfig enum
There was a problem hiding this comment.
Are you saying that we should pass GasPriceV0Config in some cases?
There was a problem hiding this comment.
I'm saying the new_gas_price_service_v1 should expect V1AlgorithmConfig to avoid config.v1().ok_or(anyhow::anyhow!("Expected V1 config"))? conversion.
The fuel-core should decided which gas price to use(0 or 1) based on the enum variant for the GasPriceServiceConfig.
Co-authored-by: Green Baneling <[email protected]>
Linked Issues/PRs
part of #2140
Description
Adding the
UninitializedTaskwith minimum test coverage.Because of how we are organizing #2415, I don't believe we need to do any syncing of the DA Cost service before startup. If it's "behind" somehow, then the old values it provides will just be ignored. It might result in a little error in the profit value early on, but it will get washed out pretty quickly.
Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]