chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService #2256
Conversation
rymnc
changed the title
rymnc
force-pushed
the
refactor/gas-price-service-v0-v1
branch
4 times, most recently
from
21c9ede to
38290ba
Compare
fix: remove block height from error
rymnc
force-pushed
the
refactor/gas-price-service-v0-v1
branch
from
38290ba to
7f895c0
Compare
rymnc
force-pushed
the
refactor/gas-price-service-v0-v1
branch
from
de143d8 to
fb926c4
Compare
There was a problem hiding this comment.
for reviewers: this code was moved as-is. no changes except for the From impl :)
| fn validate_block_gas_capacity( | ||
| &self, | ||
| block_gas_capacity: u64, | ||
| ) -> anyhow::Result<NonZeroU64> { | ||
| NonZeroU64::new(block_gas_capacity) | ||
| .ok_or_else(|| anyhow!("Block gas capacity must be non-zero")) | ||
| } | ||
|
|
||
| async fn set_metadata(&mut self) -> anyhow::Result<()> { | ||
| let metadata: UpdaterMetadata = self.algorithm_updater.clone().into(); | ||
| self.metadata_storage | ||
| .set_metadata(&metadata) | ||
| .map_err(|err| anyhow!(err)) | ||
| } | ||
|
|
||
| async fn handle_normal_block( | ||
| &mut self, | ||
| height: u32, | ||
| gas_used: u64, | ||
| block_gas_capacity: u64, | ||
| ) -> anyhow::Result<()> { | ||
| let capacity = self.validate_block_gas_capacity(block_gas_capacity)?; | ||
|
|
||
| self.algorithm_updater | ||
| .update_l2_block_data(height, gas_used, capacity)?; | ||
|
|
||
| self.set_metadata().await?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn apply_block_info_to_gas_algorithm( | ||
| &mut self, | ||
| l2_block: BlockInfo, | ||
| ) -> anyhow::Result<()> { | ||
| match l2_block { | ||
| BlockInfo::GenesisBlock => { | ||
| self.set_metadata().await?; | ||
| } | ||
| BlockInfo::Block { | ||
| height, | ||
| gas_used, | ||
| block_gas_capacity, | ||
| } => { | ||
| self.handle_normal_block(height, gas_used, block_gas_capacity) | ||
| .await?; | ||
| } | ||
| } | ||
|
|
||
| self.update(self.algorithm_updater.algorithm()).await; |
There was a problem hiding this comment.
for reviewers: mostly copied as-is except for an update on line 155 to update the algorithm.
| } | ||
| } | ||
|
|
||
| #[allow(clippy::arithmetic_side_effects)] |
There was a problem hiding this comment.
for reviewers: these tests are also more or less the same, only thing changed is the signatures.
There was a problem hiding this comment.
for reviewers: this code is copied as-is.
There was a problem hiding this comment.
for reviewers: this code is copied as-is except the change in signature to L2BlockSource
There was a problem hiding this comment.
for reviewers: this code is copied as-is.
There was a problem hiding this comment.
for reviewers: this code is copied as-is.
There was a problem hiding this comment.
for reviewers: this code is copied as-is.
There was a problem hiding this comment.
for reviewers: this code is mostly as-is and is a frankenstein between FuelGasPriceUpdater, InitializeTask and algorithm_updater 😉
There was a problem hiding this comment.
for reviewers: code moved as-is
rymnc
requested review from
xgreenx,
Dentosal and
MitchTurner
as code owners
| gas_price_db, | ||
| on_chain_db, | ||
| )?; | ||
| let gas_price_service_v0 = gas_price_init.init()?; |
There was a problem hiding this comment.
Isn't this blocking now? The purpose of the InitializeTask was to avoid blocking the initialization of all the sub services.
There was a problem hiding this comment.
yup, its blocking now. if we want it to be non-blocking, I can -
a. move some things around and move all the logic in the init fn to the into_task fn of the GasPriceService
or
b. implement RunnableService for UninitializedTask like how it is done for p2p and the task it returns is GasPriceService. this will require initialization of the shared state and have it as an additional parameter to the GasPriceService::new() fn call.
which direction would you prefer?
There was a problem hiding this comment.
also, in favor of the blocking argument: there are other modules which depend on the shared state being initialized, and this behaviour is more deterministic :)
There was a problem hiding this comment.
I think we should go with b. to be consistent. I like the idea of being blocking, just to control the order of setup for the sub services better, but that's not what we're doing elsewhere so let's avoid it for now.
| let metadata_storage = StructuredStorage::new(gas_price_db.clone()); | ||
|
|
||
| let (algo_updater, shared_algo) = initialize_algorithm( | ||
| get_default_metadata(&config, latest_block_height), |
There was a problem hiding this comment.
Can you pull this into a variable?
|
|
||
| pub fn inner(&self) -> Arc<RwLock<A>> { | ||
| self.0.clone() | ||
| } |
There was a problem hiding this comment.
We don't use it=)
| pub fn inner(&self) -> Arc<RwLock<A>> { | |
| self.0.clone() | |
| } |
## Version v0.37.0 ### Added - [1609](#1609): Add DA compression support. Compressed blocks are stored in the offchain database when blocks are produced, and can be fetched using the GraphQL API. - [2290](#2290): Added a new CLI argument `--graphql-max-directives`. The default value is `10`. - [2195](#2195): Added enforcement of the limit on the size of the L2 transactions per block according to the `block_transaction_size_limit` parameter. - [2131](#2131): Add flow in TxPool in order to ask to newly connected peers to share their transaction pool - [2182](#2151): Limit number of transactions that can be fetched via TxSource::next - [2189](#2151): Select next DA height to never include more than u16::MAX -1 transactions from L1. - [2162](#2162): Pool structure with dependencies, etc.. for the next transaction pool module. Also adds insertion/verification process in PoolV2 and tests refactoring - [2265](#2265): Integrate Block Committer API for DA Block Costs. - [2280](#2280): Allow comma separated relayer addresses in cli - [2299](#2299): Support blobs in the predicates. - [2300](#2300): Added new function to `fuel-core-client` for checking whether a blob exists. ### Changed #### Breaking - [2299](#2299): Anyone who wants to participate in the transaction broadcasting via p2p must upgrade to support new predicates on the TxPool level. - [2299](#2299): Upgraded `fuel-vm` to `0.58.0`. More information in the [release](https://github.com/FuelLabs/fuel-vm/releases/tag/v0.58.0). - [2276](#2276): Changed how complexity for blocks is calculated. The default complexity now is 80_000. All queries that somehow touch the block header now are more expensive. - [2290](#2290): Added a new GraphQL limit on number of `directives`. The default value is `10`. - [2206](#2206): Use timestamp of last block when dry running transactions. - [2153](#2153): Updated default gas costs for the local testnet configuration to match `fuel-core 0.35.0`. ## What's Changed * fix: use core-test.fuellabs.net for dnsaddr resolution by @rymnc in #2214 * Removed state transition bytecode from the local testnet by @xgreenx in #2215 * Send whole transaction pool upon subscription to gossip by @AurelienFT in #2131 * Update default gas costs based on 0.35.0 benchmarks by @xgreenx in #2153 * feat: Use timestamp of last block when dry running transactions by @netrome in #2206 * fix(dnsaddr_resolution): use fqdn separator to prevent suffixing by dns resolvers by @rymnc in #2222 * TransactionSource: specify maximum number of transactions to be fetched by @acerone85 in #2182 * Implement worst case scenario for price algorithm v1 by @rafal-ch in #2219 * chore(gas_price_service): define port for L2 data by @rymnc in #2224 * Block producer selects da height to never exceed u64::MAX - 1 transactions from L1 by @acerone85 in #2189 * Weekly `cargo update` by @github-actions in #2236 * Use fees to calculate DA reward and avoid issues with Gwei/Wei conversions by @MitchTurner in #2229 * Protect against passing `i128::MIN` to `abs()` which causes overflow by @rafal-ch in #2241 * Acquire `da_finalization_period` from the command line by @rafal-ch in #2240 * Executor: test Tx_count limit with incorrect tx source by @acerone85 in #2242 * Minor updates to docs + a few typos fixed by @rafal-ch in #2250 * chore(gas_price_service): move algorithm_updater to fuel-core-gas-price-service by @rymnc in #2246 * Use single heavy input in the `transaction_throughput.rs` benchmarks by @xgreenx in #2205 * Enforce the block size limit by @rafal-ch in #2195 * feat: build ARM and AMD in parallel by @mchristopher in #2130 * Weekly `cargo update` by @github-actions in #2268 * chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService by @rymnc in #2256 * feat(gas_price_service): update block committer da source with established contract by @rymnc in #2265 * Use bytes from `unrecorded_blocks` rather from the block from DA by @MitchTurner in #2252 * TxPool v2 General architecture by @AurelienFT in #2162 * Add value delimiter and tests args by @AurelienFT in #2280 * fix(da_block_costs): remove Arc<Mutex<>> on shared_state and expose channel by @rymnc in #2278 * fix(combined_database): syncing auxiliary databases on startup with custom behaviour by @rymnc in #2272 * fix: Manually encode Authorization header for eventsource_client by @Br1ght0ne in #2284 * Address `async-graphql` vulnerability by @MitchTurner in #2290 * Update the WASM compatibility tests for `0.36` release by @rafal-ch in #2271 * DA compression by @Dentosal in #1609 * Use different port for every version compatibility test by @rafal-ch in #2301 * Fix block query complexity by @xgreenx in #2297 * Support blobs in predicates by @Voxelot in #2299 **Full Changelog**: v0.36.0...v0.37.0
Warning
🚧🚧 This is PR 6/n in refactoring the gas price service. Now that the
algorithm_updateris a part offuel-core-gas-price-servicewe have squashed it into theGasPriceServiceusing theUninitializedTaskstruct. We don't implementRunnableServiceorRunnableTaskfor theUninitializedTaskstruct, merely use it as a wrapper to generate theGasPriceServiceV0🚧🚧Linked Issues/PRs
Description
graph TD A["lib.rs (entry point)"] B[common] B1[utils] B2[storage] B3[l2_block_source] C[ports] D[v0] E[v1] F[v0/algorithm] G[v1/algorithm] H[v0/service] I[v1/da_source] J[v0/metadata] K[v1/service] L[v0/uninitialized] A --> B B --> B1 B --> B2 B --> B3 B --> C C --> D C --> E D --> F D --> H D --> J D --> L E --> G E --> I E --> K F --> H G --> I H --> J J --> L L --> M[SharedV0Algorithm] L --> N[GasPriceServiceV0] subgraph Common B B1 B2 B3 end subgraph Ports C end subgraph V0 D F H J L end subgraph V1 E G I K endChecklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]