Allow DA recorded blocks to come out-of-order #2415

MitchTurner · 2024-10-30T15:35:45Z

Linked Issues/PRs

Closes: #2414

Eliminates the need for: #2397

Description

It sounds like it is likely we will occasionally receive recorded block information from the committer out of order. There was no real reason to expect them in order so I've generalized the algorithm to accept whatever order.

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

crates/fuel-gas-price-algorithm/src/v1.rs

…-of-order

netrome

Looks good overall to me. A few suggestions, nits and questions.

crates/fuel-gas-price-algorithm/gas-price-analysis/src/charts.rs

crates/fuel-gas-price-algorithm/src/v1.rs

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

netrome · 2024-10-30T19:32:48Z

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

@@ -270,3 +228,132 @@ fn update_da_record_data__da_block_updates_projected_total_cost_with_known_and_g
    let expected = new_known_total_cost + guessed_part;
    assert_eq!(actual, expected as u128);
 }
+
+#[test]
+fn update_da_record_data__updates_known_total_cost_if_blocks_are_out_of_order() {


Maybe it's because it's late, but I don't see where we provide blocks out of order in this test case. Is it because we skip 1 in the recorded_range?

Could we be even more ezplicit and set recorded_range to [3, 2] instead?.

That's a good idea. I've switched it. In reality, they'll probably be in order inside the vector, but they don't need to be. The point of this PR is that some heights can be skipped and returned to later. These tests skip 1 and do 2 and 3.

Yes, I also spent a couple of minutes looking at this. Maybe if you rename recorded_range to recorded_range_with_skipped_height_1 will make it easier to follow the logic. Or indeed just change to 3, 2.

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

rafal-ch

LGTM 👍
with just a few nits and minor comments

rafal-ch · 2024-10-31T09:36:34Z

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

@@ -270,3 +228,132 @@ fn update_da_record_data__da_block_updates_projected_total_cost_with_known_and_g
    let expected = new_known_total_cost + guessed_part;
    assert_eq!(actual, expected as u128);
 }
+
+#[test]
+fn update_da_record_data__updates_known_total_cost_if_blocks_are_out_of_order() {


Yes, I also spent a couple of minutes looking at this. Maybe if you rename recorded_range to recorded_range_with_skipped_height_1 will make it easier to follow the logic. Or indeed just change to 3, 2.

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

rafal-ch · 2024-10-31T09:41:32Z

crates/fuel-gas-price-algorithm/src/v1.rs

-            self.latest_da_cost_per_byte = new_cost_per_byte;
-            Ok(())
-        }
+        let recorded_bytes = self.drain_l2_block_bytes_for_range(heights)?;


Maybe just:

Suggested change

let recorded_bytes = self.drain_l2_block_bytes_for_range(heights)?;

let recorded_bytes = self.drain_l2_block_bytes(heights)?;

It'll save us from renaming the function when we change the datatype again. However, the intention here could be to not relate to the Range<T>, but to the "range" as arbitrary "noun".

Either way, just a small detail.

rafal-ch · 2024-10-31T09:44:44Z

crates/fuel-gas-price-algorithm/src/v1.rs

    #[error("Could not calculate cost per byte: {bytes:?} bytes, {cost:?} cost")]
    CouldNotCalculateCostPerByte { bytes: u128, cost: u128 },
    #[error("Failed to include L2 block data: {0}")]
    FailedTooIncludeL2BlockData(String),
-    #[error("L2 block expected but not found in unrecorded blocks: {0}")]
-    L2BlockExpectedNotFound(u32),
+    #[error("L2 block expected but not found in unrecorded blocks: {height:?}")]


Unrelated, but this could be just {height}, not {height:?}. But also I see that we use debug for other messages, so maybe there's a reason.

crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs

netrome

Thanks for updating! 🙏

…data_tests.rs Co-authored-by: Rafał Chabowski <[email protected]>

…-of-order

xgreenx · 2024-10-31T11:46:54Z

crates/fuel-gas-price-algorithm/src/v1.rs

+            let bytes = self.unrecorded_blocks.remove(expected_height).ok_or(
+                Error::L2BlockExpectedNotFound {
+                    height: *expected_height,
+                },
+            )?;


Do we really need an error here? We could log error here and use 0=)

It's up to us. If this errors we have a serious problem with our code.

If the committer has problems and submits several bundles per same height(they can in theory because they do re-bundle), it will break the node.

That can't happen if it's finalized before they report to us.

xgreenx · 2024-10-31T11:50:19Z

crates/fuel-gas-price-algorithm/src/v1.rs

        let projection_portion: u128 = self
            .unrecorded_blocks
            .iter()
-            .map(|(_, &bytes)| (bytes as u128))
+            .map(|(_, &bytes)| u128::from(bytes))
            .fold(0_u128, |acc, n| acc.saturating_add(n))


Do we really need to iterate each time here? Just curios why not to use structure that does accounting internally and updates values once per add/remove events.

This gets called semi-infrequently (only when we get a batch back from the committer). It could become more frequent I guess.

Yes. An alternative would be having the "total unrecorded bytes" tracked and when we remove blocks we subtract their bytes from that total. Then this would just be a multiplication of that. I can add that if you prefer.

…-of-order

crates/fuel-gas-price-algorithm/src/v1.rs

acerone85 · 2024-11-01T12:10:19Z

had a look, only one question from my side. otherwise LGTM

…-of-order

Remove height parameter, change tests

448d8c2

MitchTurner changed the title ~~Remove height parameter, change tests~~ Allow DA recorded blocks to come out-of-order Oct 30, 2024

MitchTurner added 2 commits October 30, 2024 17:50

Modify to take vec instead of range

c5866a3

Add tests to make sure it is sane

c54f098

MitchTurner added the no changelog Skip the CI check of the changelog modification label Oct 30, 2024

MitchTurner added 3 commits October 30, 2024 18:36

Rename variable

fcf4477

Fix analyzer

159d5b4

Rename some params

ba080ff

MitchTurner marked this pull request as ready for review October 30, 2024 16:41

MitchTurner requested review from rafal-ch, xgreenx and Dentosal as code owners October 30, 2024 16:41

rymnc reviewed Oct 30, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/src/v1.rs Outdated Show resolved Hide resolved

MitchTurner added 2 commits October 30, 2024 19:31

Use slice instead of vec

d2b3c28

Merge branch 'master' into feature/allow-DA-blocks-to-be-received-out…

128a9ba

…-of-order

netrome previously approved these changes Oct 30, 2024

View reviewed changes

Cleanup code a bit

5f19932

MitchTurner dismissed netrome’s stale review via 5f19932 October 31, 2024 09:39

rafal-ch previously approved these changes Oct 31, 2024

View reviewed changes

netrome previously approved these changes Oct 31, 2024

View reviewed changes

MitchTurner dismissed stale reviews from netrome and rafal-ch via 922f3e0 October 31, 2024 10:31

MitchTurner and others added 4 commits October 31, 2024 12:31

Update crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_…

922f3e0

…data_tests.rs Co-authored-by: Rafał Chabowski <[email protected]>

Pick nit

d7e9bdd

Merge branch 'master' into feature/allow-DA-blocks-to-be-received-out…

12c517e

…-of-order

Merge branch 'master' into feature/allow-DA-blocks-to-be-received-out…

f8dca06

…-of-order

xgreenx reviewed Oct 31, 2024

View reviewed changes

MitchTurner added 3 commits November 1, 2024 10:24

Optimize projected calc by caching running total

2bb86fe

Merge branch 'master' into feature/allow-DA-blocks-to-be-received-out…

60771e9

…-of-order

Fix analyzer

03da963

Simplify metadata

ea14e6f

acerone85 reviewed Nov 1, 2024

View reviewed changes

crates/fuel-gas-price-algorithm/src/v1.rs Show resolved Hide resolved

acerone85 approved these changes Nov 1, 2024

View reviewed changes

MitchTurner requested a review from xgreenx November 1, 2024 12:18

MitchTurner self-assigned this Nov 9, 2024

MitchTurner mentioned this pull request Nov 10, 2024

feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416

Merged

9 tasks

Merge branch 'master' into feature/allow-DA-blocks-to-be-received-out…

b615b33

…-of-order

rafal-ch approved these changes Nov 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow DA recorded blocks to come out-of-order #2415

Allow DA recorded blocks to come out-of-order #2415

MitchTurner commented Oct 30, 2024 •

edited

Loading

netrome left a comment

netrome Oct 30, 2024

netrome Oct 30, 2024

MitchTurner Oct 31, 2024

rafal-ch Oct 31, 2024

rafal-ch left a comment

rafal-ch Oct 31, 2024

rafal-ch Oct 31, 2024

rafal-ch Oct 31, 2024

netrome left a comment

xgreenx Oct 31, 2024

MitchTurner Oct 31, 2024

xgreenx Oct 31, 2024

MitchTurner Oct 31, 2024

xgreenx Oct 31, 2024

MitchTurner Oct 31, 2024

MitchTurner Nov 1, 2024

acerone85 commented Nov 1, 2024

	let recorded_bytes = self.drain_l2_block_bytes_for_range(heights)?;
	let recorded_bytes = self.drain_l2_block_bytes(heights)?;

Allow DA recorded blocks to come out-of-order #2415

Are you sure you want to change the base?

Allow DA recorded blocks to come out-of-order #2415

Conversation

MitchTurner commented Oct 30, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

netrome 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

rafal-ch 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

netrome 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

acerone85 commented Nov 1, 2024

MitchTurner commented Oct 30, 2024 •

edited

Loading