feat(gas_price_service): update block committer da source with established contract #2265

rymnc · 2024-09-27T22:16:36Z

Note

This is PR 7/7 for #2139

Linked Issues/PRs

Create committer DA block provider for V1 gas price updater #2139

Description

Updates the BlockCommitterApi to implement the contract established with us and the block committer team. There is some business logic that is slightly unclear, need @MitchTurner for insights.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests
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?

fix: remove block height from error

rymnc · 2024-09-27T22:20:56Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+            if let Some(last_value) = &self.last_value {
+                res = DaBlockCosts {
+                    l2_block_range: response.blocks_range.clone(),
+                    blob_size_bytes: response
+                        .total_size_bytes
+                        .checked_sub(last_value.total_size_bytes)
+                        .ok_or(anyhow!("Blob size bytes underflow"))?,
+                    blob_cost_wei: response
+                        .total_cost
+                        .checked_sub(last_value.total_cost)
+                        .ok_or(anyhow!("Blob cost wei underflow"))?,
+                };
+            } else {
+                res = DaBlockCosts {
+                    l2_block_range: response.blocks_range.clone(),
+                    blob_size_bytes: response.total_size_bytes,
+                    blob_cost_wei: response.total_cost,
+                };
+            }


@MitchTurner if the block committer da block cost source is starting up for the first time, we will still get the total_size_bytes and total_cost from the block committer. is it reasonable to expect the consumers of this service to persist state and detect the first value that this source spits out? Or should we include the last_state in the new fn that constructs the BlockCommitterDaBlockCosts? I might lean to the latter approach since it is more testable, and allows consumers to actually bring their own persistence method.

The consumers shouldn't need to know that info. They should just get the info for each committed batch. So the second sounds like a promising approach.

MitchTurner

Bunch of little things.

MitchTurner · 2024-09-30T13:34:53Z

crates/services/gas_price_service/src/v1/da_source_adapter.rs

@@ -24,7 +24,7 @@ pub const POLLING_INTERVAL_MS: u64 = 10_000;

 #[derive(Debug, Default, Clone, Eq, Hash, PartialEq)]
 pub struct DaBlockCosts {
-    pub l2_block_range: core::ops::Range<u32>,
+    pub l2_block_range: core::ops::Range<u64>,


that's because the sequence number is u64 as we decided. the type DaBlockHeight is also u64

MitchTurner · 2024-09-30T13:37:15Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+    async fn get_bundles_by_range(
+        &self,
+        range: core::ops::Range<u64>,
+    ) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>>;


Missing a docstring.

But I'm also unsure what the expected behavior is here. Like if a bundle included a range of blocks 50 -> 100, what would you get back if you asked for range 75 -> 100 or 75 -> 125?

its an inclusive range, so I assume we should actually increment by 1. We aren't using this yet, but it will be a part of a more comprehensive solution.

addressed docstring in 5330975

its an inclusive range

I'm not sure if this answered my question. The committer doesn't have data for individual blocks, just bundles.

Are you saying that if I asked for 50..100 and the existing bundles are 25..75 and 75..125, I would get those two bundles back? or nothing, since my requested range didn't encompass any full bundle?

maybe we should rope in @MujkicA here. it shouldn't return anything.

I've updated the issue on the committer side, probably better if we move this discussion there:

FuelLabs/fuel-block-committer#110

MitchTurner · 2024-09-30T14:21:23Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

-    url: Url,
+pub struct BlockCommitterDaBlockCosts<BlockCommitter> {
+    client: BlockCommitter,
+    last_value: Option<RawDaBlockCosts>,


The algorithm currently expects things in order. I'm not suggesting we change anything, I'm just wondering if it would make more sense to have the checks all in one place or the other. We could check that everything is correct here and do no checks in the algo. I'm not sure what makes the most sense, but worth thinking about.

good idea, we should have data validation, but that should be limited to the scope of the block committer data source. it can't know what data is expected on the receiver end, but it can atleast guarantee some things. we can choose to decouple business logic from this service, that will allow some flexibility on consumers of this data.

MitchTurner · 2024-09-30T14:35:18Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+        // assert_eq!(initial.blob_size_bytes, updated.blob_size_bytes);
+        // assert_eq!(initial.blob_cost_wei, updated.blob_cost_wei);


Do we want these?

addressed in b5e7cbd

MitchTurner · 2024-09-30T14:37:53Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+    #[tokio::test]
+    async fn request_da_block_cost__when_response_is_none__then_error() {
+        // given
+        let mock_block_committer = MockBlockCommitterApi::new(None);


These names are confusing. Maybe mock_api to differentiate it from the block_committer.

This applies to all the tests.

addressed in 28ba81f

MitchTurner · 2024-09-30T14:41:00Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+            block_committer_da_block_costs,
+            DaBlockCosts {
+                l2_block_range: da_block_costs.blocks_range.clone(),
+                blob_size_bytes: da_block_costs.total_size_bytes,
+                blob_cost_wei: da_block_costs.total_cost,
+            }


nit: rename block_committer_da_block_costs to actual and the DaBlockCosts to expected.

addressed in 9ea0a9c

MitchTurner · 2024-09-30T14:55:17Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

-    url: Url,
+pub struct BlockCommitterDaBlockCosts<BlockCommitter> {
+    client: BlockCommitter,
+    last_value: Option<RawDaBlockCosts>,


nit: rename this to last_raw_da_block_costs

addressed in 7d18c22

MitchTurner · 2024-09-30T14:55:29Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+    BlockCommitter: BlockCommitterApi,
+{
+    async fn request_da_block_cost(&mut self) -> DaBlockCostsResult<DaBlockCosts> {
+        let response;


nit: rename raw_da_block_costs

addressed in d545341

MitchTurner · 2024-09-30T14:56:34Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+        }
+
+        if let Some(response) = response {
+            let res;


nit: rename da_block_costs

addressed in d545341

MitchTurner · 2024-09-30T14:58:35Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+
+        if let Some(response) = response {
+            let res;
+            if let Some(last_value) = &self.last_value {


nit: I think I prefer to only have one if let Some(last_value) = &self.last_value and have redundant if let Some(response) = response in each branch.

addressed in edfc1ff and 2893288

acerone85 · 2024-09-30T19:39:18Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

-    pub blob_size_bytes: u32,
-    pub blob_cost: u128,
+pub struct RawDaBlockCosts {
+    // Sequence number (Monotonically increasing nonce)


Suggested change

// Sequence number (Monotonically increasing nonce)

/// Sequence number (Monotonically increasing nonce)

Same for other fields?

addressed in 5330975

acerone85 · 2024-09-30T19:55:42Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+        &self,
+        number: u64,
+    ) -> DaBlockCostsResult<Option<RawDaBlockCosts>>;
+    async fn get_bundles_by_range(


I think adding costs somewhere to the name would make it easier to relate this function to the other two.
E.g. get_cost_bundles_by_range

addressed in 552d224

acerone85 · 2024-09-30T20:01:09Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

@@ -1,3 +1,6 @@
+#![allow(non_snake_case)]


you can remove this line and add #[allow(non_snake_case)] just before the mod tests declaration

addressed in d562d72

acerone85 · 2024-09-30T20:44:48Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+            }
+            self.last_value = Some(response.clone());
+            Ok(res)
+        } else {


you can use the let - else syntax to improve code readability:

let Some(response) = response else { return Err(anyhow!("No response from block committer")) }; /* Now you can have the code in the if branch here */

addressed in edfc1ff

acerone85 · 2024-09-30T20:57:48Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+    async fn request_da_block_cost(&mut self) -> DaBlockCostsResult<DaBlockCosts> {
+        let response;
+
+        if let Some(last_value) = &self.last_value {


you can pattern match to improve readability. To avoid moving last_value out of the option, you can either pattern match on self.last_value.as_ref(), or use the ref syntax.

let response = match self.last_value { Some(ref last_value) => self .client .get_costs_by_seqno(last_value.sequence_number + 1), _ => self.client.get_latest_costs(), } .await?;

addressed in 2893288

acerone85 · 2024-09-30T21:01:24Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+
+        if let Some(response) = response {
+            let res;
+            if let Some(last_value) = &self.last_value {


If you iter over the Option, you can use the .fold function to remove some code duplication and make the code a bit more clear.

I played a bit with your code and ended up with the following:

let Some(ref response) = response else { return Err(anyhow!("No response from block committer")) }; let res = self.last_value.iter().fold( Ok(response.into()), |costs: DaBlockCostsResult<DaBlockCosts>, last_value| { let costs = costs.expect("Defined to be OK"); let blob_size_bytes = costs .blob_size_bytes .checked_sub(last_value.total_size_bytes) .ok_or(anyhow!("Blob size bytes underflow"))?; let blob_cost_wei = response .total_cost .checked_sub(last_value.total_cost) .ok_or(anyhow!("Blob cost wei underflow"))?; Ok(DaBlockCosts { blob_size_bytes, blob_cost_wei, ..costs }) }, )?; self.last_value = Some(response.clone()); Ok(res)

This requires to implement From<&RawDaBlockCosts> for DaBlockCosts.

impl From<&RawDaBlockCosts> for DaBlockCosts { fn from(raw_da_block_costs: &RawDaBlockCosts) -> Self { DaBlockCosts { l2_block_range: raw_da_block_costs.blocks_range.clone(), blob_size_bytes: raw_da_block_costs.total_size_bytes, blob_cost_wei: raw_da_block_costs.total_cost, } } }

addressed in edfc1ff

MitchTurner · 2024-10-01T09:12:57Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+        let result = block_committer.request_da_block_cost().await;
+
+        // then
+        assert!(result.is_err());


I was gonna say we should check the specific error variant here but anyhow :(

in my own experience, I tend to prefer thiserror::Error to anyhow by a lot, especially in codebases where each module has its own error type.

MitchTurner · 2024-10-01T09:13:47Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+        let mut block_committer = BlockCommitterDaBlockCosts::new(mock_api, None);
+
+        // when
+        let actual = block_committer.request_da_block_cost().await.unwrap();


Isn't this part of the setup? I don't think we need to do any checks on this value and it can be moved to given.

addressed in 0794465

MitchTurner · 2024-10-01T09:15:43Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+
+        // then
+        assert!(result.is_err());
+        assert!(block_committer.last_raw_da_block_costs.is_none());


I think this check is unnecessary. From the outside observer, we don't care what is being set internally, just the external result of those internal values.

addressed in 931adfe

MitchTurner · 2024-10-01T09:16:13Z

crates/services/gas_price_service/src/v1/da_source_adapter/block_committer_costs.rs

+
+        // then
+        assert_eq!(actual, expected);
+        assert!(block_committer.last_raw_da_block_costs.is_some());


Also unnecessary I think.

addressed in fd44dd4

…ished contract

The base branch was changed.

acerone85

LGTM

@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 2 commits September 27, 2024 22:01

chore(gas_price_service): split into 2 concrete types, v0 and v1

7f895c0

fix: remove block height from error

fix: redundant error

fb926c4

rymnc self-assigned this Sep 27, 2024

rymnc force-pushed the feat/block-committer-api-integ branch from 463604c to b47bf54 Compare September 27, 2024 22:17

rymnc commented Sep 27, 2024

View reviewed changes

rymnc marked this pull request as ready for review September 27, 2024 22:21

rymnc requested review from xgreenx, Dentosal and MitchTurner as code owners September 27, 2024 22:21

rymnc force-pushed the feat/block-committer-api-integ branch 3 times, most recently from b3680c1 to e488068 Compare September 30, 2024 13:46

fix: make async

9d17cfa

MitchTurner reviewed Sep 30, 2024

View reviewed changes

fix: var extract

e9dfe85

acerone85 reviewed Sep 30, 2024

View reviewed changes

rymnc force-pushed the feat/block-committer-api-integ branch from e488068 to 23a41fa Compare September 30, 2024 19:40

acerone85 reviewed Sep 30, 2024

View reviewed changes

rymnc requested review from MitchTurner and acerone85 October 1, 2024 07:42

Merge branch 'master' into refactor/gas-price-service-v0-v1

ca9733c

rymnc force-pushed the feat/block-committer-api-integ branch from 2893288 to e97c3f9 Compare October 1, 2024 08:16

MitchTurner reviewed Oct 1, 2024

View reviewed changes

rymnc requested a review from MitchTurner October 1, 2024 09:35

MitchTurner previously approved these changes Oct 1, 2024

View reviewed changes

fix: remove unused fn

4553a1c

rymnc added 14 commits October 2, 2024 12:25

feat(gas_price_service): update block committer da source with establ…

b8a2f24

…ished contract

fix: redundant checks

722d607

fix: mock_api

e583fee

fix: use actual and expected

cb0a887

fix: rename last_value

d840fad

fix: better naming

ad73a88

fix: comment work

1f7e2fd

fix: rename cost bundles

f0d0ca8

fix: non snake case predicate

8619d35

fix: use refactored version

5fa2490

fix: pattern matching for api call

4fc83cc

fix: no check for underflow

8e46956

fix: remove check

f9b8c28

fix: remove check

3f3c251

rymnc force-pushed the feat/block-committer-api-integ branch from fd44dd4 to 3f3c251 Compare October 2, 2024 06:55

Base automatically changed from refactor/gas-price-service-v0-v1 to master October 2, 2024 08:39

Merge branch 'master' into feat/block-committer-api-integ

f9bc61f

rymnc requested a review from MitchTurner October 2, 2024 08:44

MitchTurner approved these changes Oct 2, 2024

View reviewed changes

acerone85 approved these changes Oct 2, 2024

View reviewed changes

rymnc enabled auto-merge (squash) October 2, 2024 09:09

rymnc merged commit 4ba9da8 into master Oct 2, 2024
30 of 31 checks passed

rymnc deleted the feat/block-committer-api-integ branch October 2, 2024 09:31

rymnc mentioned this pull request Oct 2, 2024

Create committer DA block provider for V1 gas price updater #2139

Closed

xgreenx mentioned this pull request Oct 5, 2024

Release v0.37.0 #2302

Merged

		// assert_eq!(initial.blob_size_bytes, updated.blob_size_bytes);
		// assert_eq!(initial.blob_cost_wei, updated.blob_cost_wei);

	// Sequence number (Monotonically increasing nonce)
	/// Sequence number (Monotonically increasing nonce)

feat(gas_price_service): update block committer da source with established contract #2265

feat(gas_price_service): update block committer da source with established contract #2265

Conversation

rymnc commented Sep 27, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

rymnc Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

acerone85 left a comment

Choose a reason for hiding this comment

rymnc commented Sep 27, 2024 •

edited

Loading

acerone85 Sep 30, 2024 •

edited

Loading

acerone85 Sep 30, 2024 •

edited

Loading

MitchTurner Oct 1, 2024 •

edited

Loading

rymnc Oct 1, 2024 •

edited

Loading

rymnc Oct 1, 2024 •

edited

Loading

rymnc Oct 1, 2024 •

edited

Loading