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.

I've been thinking about the (very unlikely) case that our gas price metadata is "ahead" of the chain on restart. This could in theory result in a bunch of the blocks being missing from the unrecorded_blocks. At that point we might have the da_committer reporting those blocks again.

Since the precision of the algorithm isn't that important, I think the simplest solution would be to just ignore re-reported blocks.

In that case we don't want it throwing an error here. So I'm going to change this.

Hmmmmmmmm.... Well it's kinda weird.

We don't get the values for each committed block, we just get a value for the entire bundle.

This means, for example if the unrecorded_blocks are just [(8, 100)] (height, bytes) and the committed blocks come back and they are for heights [1,2,3,4,5,6,7,8] with cost 10_000, then we will remove the 8 and subtract 100 from the total and then add 10_000 to the total! Even though it's likely that the costs for 1-7 are already accounted for.

We can take 1/8 of the 10_000, which would be better than nothing, but definitely inaccurate.

In terms of the algorithm, I think the best approach here is to ignore the batch. The problem is if it ever occurs, we will carry those unrecorded_blocks forever. It's fine from the algorithm's perspective because the error will become negligible pretty quickly.

The problem is if it ever occurs, we will carry those unrecorded_blocks forever.

Nevermind. We can both remove the blocks and ignore. I think that's the best.

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

…-be-received-out-of-order

…-of-order

rafal-ch · 2024-11-19T08:52:22Z

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

@@ -538,16 +543,13 @@ impl AlgorithmUpdaterV1 {
            )?;
            total = total.saturating_add(bytes as u128);
        }
+        self.unrecorded_blocks_bytes = self.unrecorded_blocks_bytes.saturating_sub(total);


I'm thinking about the following scenario, not sure if this is an issue or not:

we have 3 blocks in self.unrecorded_blocks

we remove() 2 of them, but we get an error on 3rd one

now, the unrecorded_blocks_bytes becomes out-of-sync

Yeah. This is the inverse of the problem I mentioned above.

…-of-order

…t estimate

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 previously 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 previously approved these changes Nov 14, 2024

View reviewed changes

Merge remote-tracking branch 'origin' into feature/allow-DA-blocks-to…

cfd1a71

…-be-received-out-of-order

MitchTurner dismissed stale reviews from rafal-ch and acerone85 via cfd1a71 November 17, 2024 08:03

MitchTurner added 3 commits November 17, 2024 15:58

Resolve logical conflicts from merge

d983891

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

4c2b844

…-of-order

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

48fe978

…-of-order

rafal-ch reviewed Nov 19, 2024

View reviewed changes

MitchTurner added 6 commits November 19, 2024 16:35

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

b578a8e

…-of-order

Ignore entire batches with blocks not in the unrecorded_blocks

7ca55c7

Add comments to explain unexpected behavior

6ac41de

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

f2af4aa

…-of-order

Fix tracing messages to be emitted at the right time

cc90cbd

Fix logic to always remove recorded blocks, use predicted cost as bes…

7ac01aa

…t estimate

MitchTurner mentioned this pull request Nov 20, 2024

Create uninitialized task for v1 gas price service #2442

Open

9 tasks

netrome self-requested a review November 20, 2024 09:05

rymnc previously approved these changes Nov 20, 2024

View reviewed changes

netrome previously approved these changes Nov 20, 2024

View reviewed changes

Add tests, fix bad bug

9f78fa6

MitchTurner dismissed stale reviews from netrome and rymnc via 9f78fa6 November 20, 2024 10:59

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

MitchTurner Nov 19, 2024

MitchTurner Nov 19, 2024

MitchTurner Nov 19, 2024

MitchTurner Nov 19, 2024 •

edited

Loading

xgreenx Oct 31, 2024

MitchTurner Oct 31, 2024

MitchTurner Nov 1, 2024

acerone85 commented Nov 1, 2024

rafal-ch Nov 19, 2024

MitchTurner Nov 19, 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

MitchTurner Nov 19, 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

acerone85 commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner commented Oct 30, 2024 •

edited

Loading

MitchTurner Nov 19, 2024 •

edited

Loading