chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2523

rymnc · 2024-12-30T18:15:08Z

Warning

Needs to be merged into master after #2469 is merged

Linked Issues/PRs

Integrate with V1 algo for tests #2469

Description

When starting a node, we request the da source service for the latest costs. This results in large negative profit / loss influencing the da gas price. We want to avoid this behaviour, and therefore, we ensure that the last_recorded_height is set in the block committer.

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?

MitchTurner

When starting a node, we request the da source service for the latest costs. This results in large negative profit / loss influencing the da gas price. We want to avoid this behaviour, and therefore, we ensure that the last_recorded_height is set in the block committer.

Yeah. Nice catch. This isn't how it worked before, as we only added costs for blocks that were included in our unrecorded_blocks, but looks like we just add it all blindly now:

        let new_da_block_cost = self
            .latest_known_total_da_cost_excess
            .saturating_add(recording_cost);
        self.latest_known_total_da_cost_excess = new_da_block_cost;

I've also considered adding a config value to specify which block we want to start from. That would avoid the Error entirely.

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

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

…set, to avoid initial poll of da source

MitchTurner · 2024-12-31T18:45:39Z

If we set the latest_recorded_height to be the same as the l2 block height on startup, then we still have the same problem (if less extreme) of paying for old blocks. i.e. if the committer is working on a bundle of 200..300, and we start on 250, then we will still add the costs of 200..249, although they weren't the V1 service's responsibility.
Unfortunately, I don't know how to avoid this. The problem is that we don't know what proportion of the DA bundle any specific "unrecorded block" would represent. That's why we just take the entire bundle's cost blindly.

rymnc · 2024-12-31T19:52:21Z

If we set the latest_recorded_height to be the same as the l2 block height on startup, then we still have the same problem (if less extreme) of paying for old blocks. i.e. if the committer is working on a bundle of 200..300, and we start on 250, then we will still add the costs of 200..249, although they weren't the V1 service's responsibility.
Unfortunately, I don't know how to avoid this. The problem is that we don't know what proportion of the DA bundle any specific "unrecorded block" would represent. That's why we just take the entire bundle's cost blindly.

there's no way for us to know the next bundle height deterministically so yeah, no way to avoid fully

…e-service-v1-poll-da-source-with-specific-height

…on creation

rymnc · 2025-01-07T18:09:53Z

moving back to draft as there are some things needing discussion

acerone85 · 2025-01-08T11:41:07Z

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

        let mut service = DaSourceService::new_with_sender(
            da_block_costs_source,
            Some(Duration::from_millis(1)),
            latest_l2_height,
-            None,
+            expected_height,


I'm not sure I understand this test.
You start and run DaSourceService with a block_height ofexpected_height, but then after running the service the recorded_height is still the same? This seems to be the opposite of what the test name suggests.

Should it be

Suggested change

expected_height,

BlockHeight::new(2),

instead?

perhaps.. theres a bunch of things that are wrong in this pr now after #2512 was merged.

cool, I didn't see it was back in draft. Will stop reviewing until it's ready for review again

addressed in 73cf35b

acerone85 · 2025-01-08T11:47:23Z

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

+        let recorded_height = match self.gas_price_db.get_recorded_height()? {
+            Some(height) => height,
+            None => BlockHeight::from(latest_block_height),
+        };


nit (see https://www.lurklurk.org/effective-rust/transform.html)

Suggested change

let recorded_height = match self.gas_price_db.get_recorded_height()? {

Some(height) => height,

None => BlockHeight::from(latest_block_height),

};

let recorded_height = self

.gas_price_db

.get_recorded_height()?

.unwrap_or(BlockHeight::from(latest_block_height));

addressed in 4ca7a94

MitchTurner · 2025-01-08T18:01:23Z

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

@@ -178,10 +184,11 @@ mod tests {

        // given
        let l2_height = 10;
-        let recorded_height = 9;
+        let starting_recorded_height = 2;


Shouldn't the starting_recorded_height be 2 - 1? We call succ on the recorded_height to query the DA heights.

addressed in 53a96cf

MitchTurner · 2025-01-08T18:03:00Z

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

@@ -178,10 +184,11 @@ mod tests {

        // given
        let l2_height = 10;
-        let recorded_height = 9;
+        let starting_recorded_height = 2;
+        let expected_recorded_height = 9;
        let unexpected_costs = DaBlockCosts {


nit: I think this is probably misnamed but that would be my fault.

Suggested change

let unexpected_costs = DaBlockCosts {

let costs = DaBlockCosts {

addressed in 53a96cf

rymnc · 2025-01-08T18:23:00Z

@acerone85 this is ready for another review :)

…e-service-v1-poll-da-source-with-specific-height

crates/services/gas_price_service/src/common/fuel_core_storage_adapter.rs

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

crates/services/gas_price_service/src/v1/da_source_service/service.rs

Co-authored-by: Rafał Chabowski <[email protected]>

…e-service-v1-poll-da-source-with-specific-height

The base branch was changed.

rymnc · 2025-01-14T05:39:45Z

github is being weird with the base branch being different. i'm going to merge this into the previous base and reopen the PR with that: https://github.com/orgs/community/discussions/39031

…set, to avoid initial poll of da source (#2556) ## Linked Issues/PRs  - #2523 ## Description  prev pr didn't have a clean history because the prev base branch was squashed and merged instead of a regular merge commit. same changes as before. ## 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 - [x] 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 added the no changelog Skip the CI check of the changelog modification label Dec 30, 2024

rymnc force-pushed the chore/gas-price-service-v1-poll-da-source-with-specific-height branch from 0b912e1 to 1f5fc22 Compare December 30, 2024 18:36

MitchTurner reviewed Dec 30, 2024

View reviewed changes

crates/services/gas_price_service/src/v1/service.rs Outdated Show resolved Hide resolved

crates/services/gas_price_service/src/v1/da_source_service/block_committer_costs.rs Outdated Show resolved Hide resolved

MitchTurner assigned rymnc Dec 30, 2024

chore(gas_price_service_v1): strictly ensure last_recorded_height is …

cbc6dee

…set, to avoid initial poll of da source

rymnc force-pushed the chore/gas-price-service-v1-poll-da-source-with-specific-height branch from 1f5fc22 to cbc6dee Compare December 30, 2024 19:30

rymnc added 2 commits December 31, 2024 01:54

fix

dba77da

chore: add test for gas price service

3b60422

rymnc force-pushed the chore/gas-price-service-v1-poll-da-source-with-specific-height branch from 50e0299 to 3b60422 Compare December 30, 2024 22:11

rymnc marked this pull request as ready for review December 30, 2024 22:50

rymnc requested review from xgreenx and Dentosal as code owners December 30, 2024 22:50

rymnc requested a review from MitchTurner December 30, 2024 22:50

fix: remove unnecessary storage set, add test for uninit task instead

b151207

rymnc requested a review from a team January 6, 2025 06:04

MitchTurner approved these changes Jan 6, 2025

View reviewed changes

rymnc added 2 commits January 7, 2025 23:10

Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…

0a86e08

…e-service-v1-poll-da-source-with-specific-height

chore: force a recorded height to be passed into da source service up…

62d81ed

…on creation

rymnc marked this pull request as draft January 7, 2025 18:10

acerone85 reviewed Jan 8, 2025

View reviewed changes

rymnc added 4 commits January 8, 2025 17:25

fix: recorded height test in da source service

73cf35b

fix: idiomatic set

4ca7a94

fix: remove unnecessary test now that it is enforced

159eb84

fix: integ test

b11e03f

rymnc marked this pull request as ready for review January 8, 2025 14:37

rymnc requested a review from acerone85 January 8, 2025 14:37

rymnc requested a review from MitchTurner January 8, 2025 14:37

fix: remove dbg log

8a544b9

MitchTurner reviewed Jan 8, 2025

View reviewed changes

fix: disambiguate the l2 block range start and recorded height start

53a96cf

MitchTurner previously approved these changes Jan 8, 2025

View reviewed changes

rymnc added 2 commits January 9, 2025 12:13

Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…

64385ad

…e-service-v1-poll-da-source-with-specific-height

Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…

dce9367

…e-service-v1-poll-da-source-with-specific-height

rymnc added the release label Jan 9, 2025

rafal-ch reviewed Jan 10, 2025

View reviewed changes

rymnc and others added 3 commits January 10, 2025 16:05

chore: remove unused trait bound

687a23b

Co-authored-by: Rafał Chabowski <[email protected]>

fix: unused first_run

2df50bf

fix: comments

2fe4efe

rymnc requested a review from rafal-ch January 10, 2025 10:42

Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…

f9bfc8a

…e-service-v1-poll-da-source-with-specific-height

xgreenx approved these changes Jan 14, 2025

View reviewed changes

MitchTurner added 3 commits January 13, 2025 18:02

Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…

8a0764e

…e-service-v1-poll-da-source-with-specific-height

Fix bad merge

efc3676

Merge branch 'chore/add-tests-for-v1-gas-service' into chore/gas-pric…

b12c9e9

…e-service-v1-poll-da-source-with-specific-height

xgreenx previously approved these changes Jan 14, 2025

View reviewed changes

Base automatically changed from chore/add-tests-for-v1-gas-service to master January 14, 2025 05:33

rymnc changed the base branch from master to chore/add-tests-for-v1-gas-service January 14, 2025 05:36

rymnc merged commit 4ac8593 into chore/add-tests-for-v1-gas-service Jan 14, 2025
31 checks passed

rymnc deleted the chore/gas-price-service-v1-poll-da-source-with-specific-height branch January 14, 2025 05:39

rymnc mentioned this pull request Jan 14, 2025

chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2556

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2523

chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2523

rymnc commented Dec 30, 2024 •

edited

Loading

MitchTurner left a comment •

edited

Loading

MitchTurner commented Dec 31, 2024 •

edited

Loading

rymnc commented Dec 31, 2024

rymnc commented Jan 7, 2025

acerone85 Jan 8, 2025

rymnc Jan 8, 2025

acerone85 Jan 8, 2025

rymnc Jan 8, 2025

acerone85 Jan 8, 2025

rymnc Jan 8, 2025

MitchTurner Jan 8, 2025

rymnc Jan 8, 2025

MitchTurner Jan 8, 2025

rymnc Jan 8, 2025

rymnc commented Jan 8, 2025

rymnc commented Jan 14, 2025 •

edited

Loading

	let unexpected_costs = DaBlockCosts {
	let costs = DaBlockCosts {

chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2523

chore(gas_price_service_v1): strictly ensure last_recorded_height is set, to avoid initial poll of da source #2523

Conversation

rymnc commented Dec 30, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

MitchTurner left a comment • edited Loading

Choose a reason for hiding this comment

MitchTurner commented Dec 31, 2024 • edited Loading

rymnc commented Dec 31, 2024

rymnc commented Jan 7, 2025

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

rymnc commented Jan 8, 2025

rymnc commented Jan 14, 2025 • edited Loading

rymnc commented Dec 30, 2024 •

edited

Loading

MitchTurner left a comment •

edited

Loading

MitchTurner commented Dec 31, 2024 •

edited

Loading

rymnc commented Jan 14, 2025 •

edited

Loading