feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416

rymnc · 2024-10-31T05:04:48Z

Note

Some values for the tests need expert opinion from @MitchTurner. A follow up PR will be created to define the UninitializedTask that wraps over this task.

Linked Issues/PRs

part of #2140, but doesn't close it yet.

Description

We define a new RunnableTask, GasPriceServiceV1, which uses the da block cost source in tandem with the l2 block source
Tests for the same
Casts to and from the v1 algorithm updater
we take a direct dependency on v0's metadata so that we can migrate between the two.

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?

rymnc · 2024-10-31T09:16:10Z

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

+    }
+
+    #[tokio::test]
+    async fn run__updates_gas_price_with_da_block_cost_source() {


cc: @MitchTurner this test seems to be failing with the following error -

called `Result::unwrap()` on an `Err` value: L2 block expected but not found in unrecorded blocks: 1

which i narrowed down to

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

Lines 546 to 562 in a14922e

fn drain_l2_block_bytes_for_range(

&mut self,

height_range: Range<u32>,

) -> Result<u128, Error> {

let mut total: u128 = 0;

for expected_height in height_range {

let (actual_height, bytes) = self

.unrecorded_blocks

.pop_first()

.ok_or(Error::L2BlockExpectedNotFound(expected_height))?;

if actual_height != expected_height {

return Err(Error::L2BlockExpectedNotFound(expected_height));

}

total = total.saturating_add(bytes as u128);

}

Ok(total)

}

.

This is what happens when the da block cost that is received refers to an L2 block that hasn't been processed by the algorithm yet. that is fine, seems like defined behaviour.

Below, on line 432-434, I define the cost and bytes. Something seems to be wrong in the drain_l2_block_bytes_for_range function

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

Line 546 in a14922e

fn drain_l2_block_bytes_for_range(

when I call it with just one item in the range. because the range_bytes it yields after being called in

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

Line 529 in a14922e

let range_bytes = self.drain_l2_block_bytes_for_range(height_range)?;

is 0, which doesn't seem too correct.

wondering if you have any thoughts about the params I'm using.

I think we discussed this offline someplace, but I'll record it here.

This is what happens when the da block cost that is received refers to an L2 block that hasn't been processed by the algorithm yet. that is fine, seems like defined behaviour.

We should always receive DA recorded blocks after L2 blocks. This is because we can't commit a block to DA until after it is produced on L2.

when I call it with just one item in the range. because the range_bytes it yields after being called in

Good catch. I've changed the behavior anyway in this pr: #2415

Should solve it.

wondering if you have any thoughts about the params I'm using.

With these tests, the values don't exactly matter. We use an assert_ne! and as long as that's true we're not concerned with the details. The problem with this approach is the test writer/reader still needs to know what values would result in a different gas price. This is part of the reason I originally had an abstraction of gas price updater (which we've (you've) removed)

MitchTurner · 2024-11-09T05:35:46Z

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

    /// The range of blocks that the costs apply to
-    pub blocks_range: core::ops::Range<u64>,
+    pub blocks_range: core::ops::Range<u32>,


We'll need to change this to just be a vec with these changes:
#2415

MitchTurner · 2024-11-09T05:44:44Z

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

            // arbitrary logic to generate a new value
            let mut value = self.value.clone();
            if let Some(value) = &mut value {
                value.sequence_number = seq_no;
                value.blocks_range =
                    value.blocks_range.end * seq_no..value.blocks_range.end * seq_no + 10;
-                value.da_block_height = value.da_block_height + (seq_no + 1).into();
+                value.da_block_height =
+                    value.da_block_height + ((seq_no + 1) as u64).into();
                value.total_cost += 1;
                value.total_size_bytes += 1;
            }


There is a lot of unchecked arithmetic and casting going on here. How do you feel about switching to checked and into/from here?

MitchTurner · 2024-11-10T04:34:23Z

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

+    }
+
+    #[tokio::test]
+    async fn run__updates_gas_price_with_da_block_cost_source() {


I think we discussed this offline someplace, but I'll record it here.

This is what happens when the da block cost that is received refers to an L2 block that hasn't been processed by the algorithm yet. that is fine, seems like defined behaviour.

We should always receive DA recorded blocks after L2 blocks. This is because we can't commit a block to DA until after it is produced on L2.

when I call it with just one item in the range. because the range_bytes it yields after being called in

Good catch. I've changed the behavior anyway in this pr: #2415

Should solve it.

MitchTurner · 2024-11-10T06:01:35Z

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

+    }
+
+    #[tokio::test]
+    async fn run__updates_gas_price_with_da_block_cost_source() {


wondering if you have any thoughts about the params I'm using.

With these tests, the values don't exactly matter. We use an assert_ne! and as long as that's true we're not concerned with the details. The problem with this approach is the test writer/reader still needs to know what values would result in a different gas price. This is part of the reason I originally had an abstraction of gas price updater (which we've (you've) removed)

MitchTurner · 2024-11-10T06:20:44Z

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

+
+        // when
+        service.run(&mut watcher).await.unwrap();
+        l2_block_sender.send(l2_block).await.unwrap();


Is the bug you're seeing the reason we aren't submitting a new DA block here?

not seeing the bug anymore :)

I think this test should be "sending" a block to the DA record service, not the l2_block_sender.

I still need to trigger update_da_gas_price(), so I've added it in the update_da_record_data fn of the AlgorithmV1 impl in c516ea9

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

MitchTurner · 2024-11-13T08:46:06Z

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

@@ -103,12 +104,14 @@ where
    async fn run(&mut self, state_watcher: &mut StateWatcher) -> Result<bool> {
        let continue_running;

+        let tick: BoxFuture<tokio::time::Instant> = Box::pin(self.poll_interval.tick());


Why do we need the BoxFuture? I ran it on my machine without and it compiled.

you're right, I had it that way when i was testing something. removed in 53291a5

MitchTurner · 2024-11-13T08:55:17Z

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

@@ -313,6 +313,7 @@ impl AlgorithmUpdaterV1 {
        if !height_range.is_empty() {
            self.da_block_update(height_range, range_cost)?;
            self.recalculate_projected_cost();
+            self.update_da_gas_price();


Ah. Haha. I get it now... I think it makes sense either way but this probably better.

Probably should have some tests in the algo for this.

addressed in 8ae983f

MitchTurner · 2024-11-13T09:00:50Z

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

+    pub capped_range_size: u16,
+    pub decrease_range_size: u16,
+    pub block_activity_threshold: u8,
+    pub unrecorded_blocks: Vec<(u32, u64)>,


Hmm. I guess it doesn't hurt. I don't know why we'd need this if it wasn't already in the metadata.

need this to bootstrap the AlgorithmV1 with a state that we can test only DA data being sent :)

MitchTurner · 2024-11-13T09:19:28Z

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

+        if let Ok(v1_metadata) = V1Metadata::try_from(updater_metadata.clone()) {
+            algorithm_updater = v1_algorithm_from_metadata(v1_metadata, config);
+        } else {
+            let v0_metadata = V0Metadata::try_from(updater_metadata).map_err(|_| {
+                crate::common::utils::Error::CouldNotInitUpdater(anyhow::anyhow!(
+                    "Could not convert metadata to V0Metadata"
+                ))
+            })?;
+            let v1_metadata = V1Metadata::construct_from_v0_metadata(v0_metadata, config)
+                .map_err(|err| {
+                    crate::common::utils::Error::CouldNotInitUpdater(anyhow::anyhow!(err))
+                })?;
+            algorithm_updater = v1_algorithm_from_metadata(v1_metadata, config);
+        }


nit: I think we should just unify this branch and add a method fn best_v1_metadata(metadata: &Metadata, config: &V1AlgorithmConfig). In both branches we end up calling v1_algorithm_from_metadata, so something like:

Suggested change

if let Ok(v1_metadata) = V1Metadata::try_from(updater_metadata.clone()) {

algorithm_updater = v1_algorithm_from_metadata(v1_metadata, config);

} else {

let v0_metadata = V0Metadata::try_from(updater_metadata).map_err(|_| {

crate::common::utils::Error::CouldNotInitUpdater(anyhow::anyhow!(

"Could not convert metadata to V0Metadata"

))

})?;

let v1_metadata = V1Metadata::construct_from_v0_metadata(v0_metadata, config)

.map_err(|err| {

crate::common::utils::Error::CouldNotInitUpdater(anyhow::anyhow!(err))

})?;

algorithm_updater = v1_algorithm_from_metadata(v1_metadata, config);

}

let v1_metadata = best_v1_metadata(updater_metadata, config)?;

algorithm_updater = v1_algorithm_from_metadata(v1_metadata, config);

addressed in 2291207

rymnc added 2 commits October 31, 2024 06:02

feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1

6dc5ed7

chore: changelog

b6149d1

rymnc self-assigned this Oct 31, 2024

rymnc added 2 commits October 31, 2024 10:04

test: this test is broken

8a912c4

test: different args

c9d7ed7

rymnc commented Oct 31, 2024

View reviewed changes

rymnc added 5 commits October 31, 2024 16:27

fix: broken test

7a65574

Merge branch 'master' into feat/gas-price-service-v1-impl

d576665

chore: process block size and fees

b4d8855

chore: clean setting of metadata etc

0a358f6

fix: fmt

49a309a

MitchTurner reviewed Nov 10, 2024

View reviewed changes

chore: remove l2 block being sent

c516ea9

MitchTurner reviewed Nov 13, 2024

View reviewed changes

rymnc added 3 commits November 13, 2024 19:14

fix: remove pinning

53291a5

fix: refactor init algo fn

2291207

chore: add test for AlgorithmV1 changes

8ae983f

rymnc marked this pull request as ready for review November 13, 2024 15:09

rymnc requested review from rafal-ch, xgreenx and Dentosal as code owners November 13, 2024 15:09

rymnc requested a review from a team November 13, 2024 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416

feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416

rymnc commented Oct 31, 2024 •

edited

Loading

rymnc Oct 31, 2024

rymnc Oct 31, 2024

MitchTurner Nov 10, 2024

MitchTurner Nov 10, 2024

MitchTurner Nov 9, 2024

MitchTurner Nov 9, 2024

MitchTurner Nov 10, 2024

MitchTurner Nov 10, 2024

MitchTurner Nov 10, 2024

rymnc Nov 10, 2024

MitchTurner Nov 12, 2024

rymnc Nov 12, 2024

MitchTurner Nov 13, 2024

rymnc Nov 13, 2024

MitchTurner Nov 13, 2024

MitchTurner Nov 13, 2024

rymnc Nov 13, 2024

MitchTurner Nov 13, 2024

rymnc Nov 13, 2024

MitchTurner Nov 13, 2024 •

edited

Loading

rymnc Nov 13, 2024

	fn drain_l2_block_bytes_for_range(
	&mut self,
	height_range: Range<u32>,
	) -> Result<u128, Error> {
	let mut total: u128 = 0;
	for expected_height in height_range {
	let (actual_height, bytes) = self
	.unrecorded_blocks
	.pop_first()
	.ok_or(Error::L2BlockExpectedNotFound(expected_height))?;
	if actual_height != expected_height {
	return Err(Error::L2BlockExpectedNotFound(expected_height));
	}
	total = total.saturating_add(bytes as u128);
	}
	Ok(total)
	}

feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416

Are you sure you want to change the base?

feat(gas_price_service_v1): define RunnableTask for GasPriceServiceV1 #2416

Conversation

rymnc commented Oct 31, 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

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

MitchTurner Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymnc commented Oct 31, 2024 •

edited

Loading

MitchTurner Nov 13, 2024 •

edited

Loading