Add unrecorded blocks abstraction to gas price algo #2468

MitchTurner · 2024-11-29T20:25:39Z

Linked Issues/PRs

Description

This PR:

Adds a trait to the V1 algorithm for storing unrecorded blocks
Adds TransactionableStorage trait to gas price service to allow abstraction for atomic commits to storage (once per L2 block
Separates Get and Set traits for service to specify which need a transaction to execute
Add a GetSequenceNumber and SetSequence number concept for tracking the DA sequence number the DA service should start on at startup (I can't remember why this got included in this PR, but it felt logical at the time...)

We only need a key-value lookup for the unrecorded blocks. Instead of storing all the data in the metadata, we just moved it to its own abstraction that can be accessed directly from the algorithm. This will also help with historical view, since historical view doesn't have a way to iterate over all values for that height, just do KV lookups. We were iterating before this change.

Also added the unrecorded_block_bytes field to metadata so we can still track that in the case of historical view.

This exposed a problem with the design, which is that we need changes to the algorithm to be atomic per l2 block, which is how our Fuel Storage is designed. This led us to introducing a TransactionableStorage trait for the service, which can be handed to the algorithm updater before being "commit"ed. The same applies to the MetadataStorage, so the service was redesigned slightly to accomodate this change.

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

…-of-order

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

…-of-order

…-be-received-out-of-order

…-of-order

…to feature/init-task-for-v1-gas-price-service

…-of-order

AurelienFT · 2024-12-05T12:00:39Z

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

+    fn set_metadata(&mut self, metadata: &UpdaterMetadata) -> GasPriceResult<()> {
+        let block_height = metadata.l2_block_height();
+        let mut tx = self.write_transaction();
+        tx.storage_as_mut::<GasPriceMetadata>()


Why are we creating a tx for 1 insert ?

I feel like that's what we do most places.

If we constrained with KeyValueMutate we could use put directly, but that's not what we have access to.

AurelienFT · 2024-12-05T12:02:11Z

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

+
+impl Mappable for SequenceNumberTable {
+    type Key = Self::OwnedKey;
+    type OwnedKey = u32;


Can we use BlockHeight type here to be more explicit ?

AurelienFT · 2024-12-05T12:08:25Z

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

-        )?;
-
-        self.set_metadata().await?;
+        let metadata = self.algorithm_updater.clone().into();


AlgorithmUpdater have algorithm() method to get an algorithm. I find it strange to not have a metadata() but a into() to get metadata

Metadata doesn't exist as a concept from the algorithm updater's perspective. We have the metadata at the service level to be able to reproduce the updater in the case the service stops.

rafal-ch

So far looks good, but I didn't finish the review yet, sorry. I'll do another pass ASAP. Currently leaving just a few minor comments.

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

rafal-ch · 2024-12-05T13:14:51Z

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

@@ -34,4 +34,8 @@ impl DaBlockCostsSource for DummyDaBlockCosts {
            }
        }
    }
+
+    async fn set_last_value(&mut self, sequence_number: u32) -> DaBlockCostsResult<()> {


Can DummyDaBlockCosts be behind the test flag?

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

rafal-ch · 2024-12-05T13:26:04Z

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

-
-    async fn handle_normal_block(
-        &mut self,
+    async fn handle_normal_block<'a>(


What does "normal" mean in this context? Maybe "non_genesis" would be more clear?

As opposed to genesis block. I know it's not a great name, but it's true that normally it will be this block. Haha. I'd be open to other names. "non-genesis" seems strange.

Maybe just "handle_block()"?

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

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

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

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

…add-unrecorded-blocks-trait

…s-trait

rafal-ch · 2024-12-09T13:15:43Z

Generic comment, probably not related to this PR.

It seems like some functions do not need to be async, for example: SharedGasPriceAlgo::update(). As a consequence, we might also remove async from some of the callers. We could revisit that, unless there is a plan for async processing in follow-up PRs.

#2483) Suggestions for the #2468 PR. --------- Co-authored-by: Mitchell Turner <[email protected]>

xgreenx

Almost done=) Some questions and suggestions

xgreenx · 2024-12-12T04:27:05Z

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

+        if let Some(sequence_number) = self
+            .gas_price_db
+            .get_sequence_number(&metadata_height.into())?
+        {
+            self.da_source.set_last_value(sequence_number).await?;
+        }


I don't understand how this part should work. You only set the sequencer number when you receive the bundle from the DA.

Let's say you received the bundle with sequencer number 10 at L2 block height X. After that, you processed the L2 block at height X + 1. After that, the node was restarted.

During start up the metadata_height will be X + 1, but the get_sequence_number will return None in this case. So you will not continue from the sequencer number 10.

Ah. I see. So we need to set the most recent value again if it hasn't changed.

Or look for the most recent value... I think setting it every time would be better performance.

xgreenx · 2024-12-12T04:29:45Z

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

 pub mod dummy_costs;
 pub mod service;

 #[derive(Debug, Default, Clone, Eq, Hash, PartialEq)]
 pub struct DaBlockCosts {
+    pub sequence_number: u32,


It would be better if it was bundle_id or if we somehow connected it to the block committer(comment can help)=)

xgreenx · 2024-12-12T04:36:46Z

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

+                tx.set_metadata(&metadata).map_err(|err| anyhow!(err))?;
+                AtomicStorage::commit_transaction(tx)?;
+                let new_algo = self.algorithm_updater.algorithm();
+                // self.update(new_algo).await;


Suggested change

// self.update(new_algo).await;

xgreenx · 2024-12-12T04:40:06Z

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

+        let capacity = Self::validate_block_gas_capacity(block_gas_capacity)?;
+        let mut storage_tx = self.storage_tx_provider.begin_transaction()?;
+
+        for da_block_costs in self.da_block_costs_buffer.drain(..) {


In the case of an error, you will lose da_block_costs_buffer data. Maybe it is better just to iterate here and clear it later after you committed changes to the database.

MitchTurner and others added 30 commits October 30, 2024 17:26

Remove height parameter, change tests

448d8c2

Modify to take vec instead of range

c5866a3

Add tests to make sure it is sane

c54f098

Rename variable

fcf4477

Fix analyzer

159d5b4

Rename some params

ba080ff

Use slice instead of vec

d2b3c28

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

128a9ba

…-of-order

Cleanup code a bit

5f19932

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

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

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

b615b33

…-of-order

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

cfd1a71

…-be-received-out-of-order

Resolve logical conflicts from merge

d983891

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

4c2b844

…-of-order

WIP

1836656

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

48fe978

…-of-order

Merge branch 'feature/allow-DA-blocks-to-be-received-out-of-order' in…

18fefb7

…to feature/init-task-for-v1-gas-price-service

Get compiling

b15a93f

Start adding tests

279191c

Get existing tests passing

63b0266

WIP

477c731

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

MitchTurner added 2 commits December 4, 2024 16:36

Separate traits out into get and set traits

a587b8a

Fix broken test

37235ae

AurelienFT reviewed Dec 5, 2024

View reviewed changes

rafal-ch reviewed Dec 5, 2024

View reviewed changes

MitchTurner added 8 commits December 5, 2024 12:29

Cleanup extra code, remove unused imports, hide imlementations.

96f6b9d

Merge branch 'master' into feature/init-task-for-v1-gas-price-service

f43f00d

Merge branch 'feature/init-task-for-v1-gas-price-service' into chore/…

e80edd5

…add-unrecorded-blocks-trait

Correct typo

51d41c7

Merge branch 'feature/init-task-for-v1-gas-price-service' into chore/…

17a56d3

…add-unrecorded-blocks-trait

Use relevant config for v1 constructor

a642cb7

Merge branch 'feature/init-task-for-v1-gas-price-service' into chore/…

8b027b9

…add-unrecorded-blocks-trait

Add todo

abf0cca

Base automatically changed from feature/init-task-for-v1-gas-price-service to master December 6, 2024 22:56

Merge remote-tracking branch 'origin' into chore/add-unrecorded-block…

2469bf8

…s-trait

xgreenx mentioned this pull request Dec 7, 2024

Suggestions to simplify the V1 storage requirements and initialization #2483

Merged

Add missed things

288f51f

MitchTurner and others added 4 commits December 9, 2024 08:10

Merge branch 'master' into chore/add-unrecorded-blocks-trait

00daced

Remove extra code from merge

9fe913c

Merge branch 'master' into chore/add-unrecorded-blocks-trait

009a847

Suggestions to simplify the V1 storage requirements and initialization (

2eba245

#2483) Suggestions for the #2468 PR. --------- Co-authored-by: Mitchell Turner <[email protected]>

MitchTurner requested a review from a team December 9, 2024 23:33

Small simplificaitons

1b74ff5

xgreenx reviewed Dec 12, 2024

View reviewed changes

MitchTurner added 4 commits December 11, 2024 22:37

Fix the setting of the sequence number

986c6c6

Format

ffce782

Remove comment, move mutation, rename field

4fc7184

Fix compilation

ee1c886

MitchTurner requested a review from xgreenx December 12, 2024 05:45

Appease Clippy-sama

43e19f2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unrecorded blocks abstraction to gas price algo #2468

Add unrecorded blocks abstraction to gas price algo #2468

MitchTurner commented Nov 29, 2024 •

edited

Loading

AurelienFT Dec 5, 2024

MitchTurner Dec 9, 2024

MitchTurner Dec 9, 2024

AurelienFT Dec 5, 2024

AurelienFT Dec 5, 2024

MitchTurner Dec 9, 2024

rafal-ch left a comment

rafal-ch Dec 5, 2024

rafal-ch Dec 5, 2024

MitchTurner Dec 5, 2024

rafal-ch Dec 9, 2024

rafal-ch commented Dec 9, 2024

xgreenx left a comment

xgreenx Dec 12, 2024

MitchTurner Dec 12, 2024 •

edited

Loading

xgreenx Dec 12, 2024

xgreenx Dec 12, 2024

xgreenx Dec 12, 2024

Add unrecorded blocks abstraction to gas price algo #2468

Are you sure you want to change the base?

Add unrecorded blocks abstraction to gas price algo #2468

Conversation

MitchTurner commented Nov 29, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

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

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

Choose a reason for hiding this comment

rafal-ch commented Dec 9, 2024

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner Dec 12, 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

MitchTurner commented Nov 29, 2024 •

edited

Loading

MitchTurner Dec 12, 2024 •

edited

Loading