chore(gas_price_service): define port for L2 data #2224

rymnc · 2024-09-19T16:55:50Z

Note

This is PR 1/n in cleaning up the gas price service. This PR is first of few that moves the algorithm_updater from the sub_services module of fuel-core to fuel-core-gas-price-service.

To do so, we have to cut dependencies on fuel-core, which means we have to remove the direct dependency on Database, ConsensusParametersProvider etc.

Linked Issues/PRs

Description

We define a new port in the fuel-core-gas-price-service crate that is meant to cut the dependency of algorithm_updater from fuel-core.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests (existing tests already work)
The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

I have reviewed the code myself
I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

Rust SDK
Sway compiler
Platform documentation (for out-of-organization contributors, the person merging the PR will do this)
Someone else?

MitchTurner

LGTM

xgreenx · 2024-09-20T10:09:43Z

crates/fuel-core/src/service/sub_services/algorithm_updater.rs

    let first = metadata_height.saturating_add(1);
    let view = on_chain_db.latest_view()?;
    for height in first..=latest_block_height {
        let block = view
-            .storage::<FuelBlocks>()
-            .get(&height.into())?
+            .get_block(&height.into())?
            .ok_or(anyhow::anyhow!("Expected block to exist"))?;
        let last_tx_id = block.transactions().last().ok_or(anyhow::anyhow!(
            "Expected block to have at least one transaction"
        ))?;
        let param_version = block.header().consensus_parameters_version;
        let params = settings.settings(&param_version)?;
        let mint = view


Hmm, the logic below let block_gas_used = mint.mint_amount(); looks incorrect. In another places we calculate this value from several paramterers.

I think we don't need ability to fetch the last transaction. I think we need to process the whole block as we do in the service itself. Sofrom port perspective we need only latest_height and get_block where get_block return whole block with all transactions.

It is an expensive operation than just fetching last transaction, but it will avoid potential issue and code will be easier to read, since ew use the same update logic during initialization, during run function and during shutdown.

addressed in d48e374, qq: (maybe @MitchTurner) what behaviour should we have for this line? -

fuel-core/crates/fuel-core/src/service/sub_services/algorithm_updater.rs

Line 332 in d48e374

Err(anyhow::anyhow!("should not be genesis block"))?

I marked it as an error, since we have the saturating_add above.

Yeah. That seems fine to me.

xgreenx · 2024-09-20T10:49:55Z

crates/fuel-core/src/service/adapters/gas_price_adapters.rs

+    fn get_block(&self, height: &BlockHeight) -> StorageResult<Block<Transaction>> {
+        let block = self
+            .storage::<FuelBlocks>()
+            .get(height)
+            .map(|block| block.map(|block| block.as_ref().clone()))?
+            .ok_or(not_found!("FuelBlock"))?;
+
+        let mut transactions = vec![];
+        for tx_id in block.transactions() {
+            let tx = self
+                .storage::<Transactions>()
+                .get(tx_id)
+                .map(|tx| tx.map(|tx| tx.as_ref().clone()))?
+                .ok_or(not_found!("Transaction"))?;
+            transactions.push(tx);
+        }
+        Ok(block.uncompress(transactions))
+    }


We have get_full_block function that you can reuse

addressed in abbf480

xgreenx · 2024-09-20T11:00:35Z

crates/fuel-core/src/service/sub_services/algorithm_updater.rs

-            "Expected block to have at least one transaction"
-        ))?;
+            .get_block(&height.into())?
+            .ok_or(not_found!("FullBlock"))?;
        let param_version = block.header().consensus_parameters_version;


When InitializeTask will be moved into gas-price-service, instead of duplicating logic we can call GasPriceTaskV0::process_l2_block(block) inside of InitializeTask::into_task. Just a reminder to not forget

yes, that's on the roadmap :)

> [!NOTE] > This is PR 1/n in cleaning up the gas price service. This PR is first of few that moves the `algorithm_updater` from the `sub_services` module of `fuel-core` to `fuel-core-gas-price-service`. > > To do so, we have to cut dependencies on `fuel-core`, which means we have to remove the direct dependency on `Database`, `ConsensusParametersProvider` etc. ## Linked Issues/PRs  ## Description  We define a new port in the `fuel-core-gas-price-service` crate that is meant to cut the dependency of `algorithm_updater` from `fuel-core`. ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests (existing tests already work) - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? --------- Co-authored-by: Green Baneling <XgreenX9999@gmail.com>

…eUpdater type into GasPriceService (#2256) > [!WARNING] > 🚧🚧 This is PR 6/n in refactoring the gas price service. Now that the `algorithm_updater` is a part of `fuel-core-gas-price-service` we have squashed it into the `GasPriceService` using the `UninitializedTask` struct. We don't implement `RunnableService` *or* `RunnableTask` for the `UninitializedTask` struct, merely use it as a wrapper to generate the `GasPriceServiceV0` 🚧🚧 ## Linked Issues/PRs  - #2246 - #2245 - #2230 - #2226 - #2224 - #2192 ## Description  - created common module containing storage adapter, l2 block source and some updater metadata stuff (linked to storage) - created v0 module containing impl of GasPriceAlgorithm for AlgorithmV0, and V0metadata stuff - created v1 module containing impl of GasPriceAlgorithm for AlgorithmV1 & da block costs source (V1Metadata didn’t exist before so i didn’t create it) - fuel-gas-price-updater will be nuked, an UninitializedGasPriceServiceV(x) for each version that takes in all the args needed ```mermaid 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 end ``` ## Checklist - [x] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [x] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else? ---------

@rymnc

## 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

rymnc added the no changelog Skip the CI check of the changelog modification label Sep 19, 2024

rymnc self-assigned this Sep 19, 2024

rymnc force-pushed the chore/gas-price-service-ports branch 2 times, most recently from a389d2b to 352cd0b Compare September 19, 2024 18:21

rymnc mentioned this pull request Sep 20, 2024

chore(gas_price_service): define port for GasPriceDatabase #2226

Closed

9 tasks

rymnc force-pushed the chore/gas-price-service-ports branch from 352cd0b to 100fee8 Compare September 20, 2024 08:03

rymnc marked this pull request as ready for review September 20, 2024 08:03

rymnc requested review from xgreenx, Dentosal and MitchTurner as code owners September 20, 2024 08:03

chore(gas_price_service): define port for L2 data

d45a323

rymnc force-pushed the chore/gas-price-service-ports branch from 100fee8 to d45a323 Compare September 20, 2024 08:10

MitchTurner previously approved these changes Sep 20, 2024

View reviewed changes

xgreenx reviewed Sep 20, 2024

View reviewed changes

fix: reuse get_block_info fn

d48e374

rymnc dismissed MitchTurner’s stale review via d48e374 September 20, 2024 10:27

fix: fmt

44378c6

rymnc requested review from xgreenx and MitchTurner September 20, 2024 10:30

xgreenx reviewed Sep 20, 2024

View reviewed changes

fix: use get_full_block

abbf480

MitchTurner approved these changes Sep 20, 2024

View reviewed changes

Merge branch 'master' into chore/gas-price-service-ports

8801f27

xgreenx approved these changes Sep 20, 2024

View reviewed changes

rymnc enabled auto-merge (squash) September 20, 2024 11:02

rymnc merged commit 6720e35 into master Sep 20, 2024
32 checks passed

rymnc deleted the chore/gas-price-service-ports branch September 20, 2024 11:17

rymnc mentioned this pull request Sep 27, 2024

chore(gas_price_service): split into v0 and v1 and squash FuelGasPriceUpdater type into GasPriceService #2256

Merged

9 tasks

xgreenx mentioned this pull request Oct 5, 2024

Release v0.37.0 #2302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gas_price_service): define port for L2 data #2224

chore(gas_price_service): define port for L2 data #2224

rymnc commented Sep 19, 2024 •

edited

Loading

MitchTurner left a comment

xgreenx Sep 20, 2024

rymnc Sep 20, 2024

MitchTurner Sep 20, 2024

xgreenx Sep 20, 2024

rymnc Sep 20, 2024

xgreenx Sep 20, 2024

rymnc Sep 20, 2024

chore(gas_price_service): define port for L2 data #2224

chore(gas_price_service): define port for L2 data #2224

Conversation

rymnc commented Sep 19, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

MitchTurner left a comment

Choose a reason for hiding this comment

xgreenx Sep 20, 2024

Choose a reason for hiding this comment

rymnc Sep 20, 2024

Choose a reason for hiding this comment

MitchTurner Sep 20, 2024

Choose a reason for hiding this comment

xgreenx Sep 20, 2024

Choose a reason for hiding this comment

rymnc Sep 20, 2024

Choose a reason for hiding this comment

xgreenx Sep 20, 2024

Choose a reason for hiding this comment

rymnc Sep 20, 2024

Choose a reason for hiding this comment

rymnc commented Sep 19, 2024 •

edited

Loading