Use gas prices from actual blocks to calculate estimate gas prices #2501

MitchTurner · 2024-12-16T03:52:53Z

Linked Issues/PRs

Description

Because some nodes won't be running the gas price algorithm, or might not be synced properly, we are going to calculate the price based on the latest block gas price. That is a reliable source of truth.

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

left a few questions :)

rymnc · 2024-12-16T04:41:45Z

crates/fuel-core/src/service/adapters.rs

+}
+
+#[async_trait::async_trait]
+impl GasPriceEstimate for ArcGasPriceEstimate<u32, u64> {


does this have to be async?

rymnc · 2024-12-16T04:44:40Z

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

+    pub(crate) fn update_latest_gas_price(&self, block_info: &BlockInfo) {
+        match block_info {
+            BlockInfo::GenesisBlock => {
+                // do nothing


should we have a test to see what values are stored in the block after genesis? afaiu we test only post genesis transitions

rymnc · 2024-12-16T05:07:25Z

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

@@ -481,10 +506,12 @@ mod tests {
            ),
            None,
        );
+        let latest_gas_price = Arc::new(parking_lot::RwLock::new((0, 0)));


curious why we need this when perhaps the algo could contain this internally?

Yeah... That works too. It's kinda a separate value so I thought to keep it separate but that would work.

Yeah, maybe we oversplitting here. Algorithm could answer this, if we store the last used gas price there. But I think Mitch wants to reuse the code for v0. But maybe it can live in common and used in both algorithm.

acerone85 · 2024-12-16T13:52:46Z

crates/fuel-core/src/service/adapters.rs

+    use super::*;
+    use proptest::proptest;
+
+    async fn _worst_case__correctly_calculates_value(


nit: this function is called later on

Suggested change

async fn _worst_case__correctly_calculates_value(

async fn worst_case__correctly_calculates_value(

Yes. It's the actual test code, but I don't like writing it inside the prop_test macro because the IDE has a hard time parsing it:

proptest! { #[test] fn worst_case__correctly_calculates_value( price: u64, starting_height: u32, block_horizon in 0..10_000u32, percentage: u64 ) { _worst_case__correctly_calculates_value(price, starting_height, block_horizon, percentage); } }

So this is my naming convention for that case.

acerone85 · 2024-12-16T14:03:33Z

crates/fuel-core/src/service/adapters.rs

+}
+
+#[allow(dead_code)]
+pub struct ArcGasPriceEstimate<Height, GasPrice> {


would it be possible to have a bit of documentation here?
In particular, I'd like to understand what the "height" and "price" refer to here, since later they are named as "best_height" and "best_price"

acerone85 · 2024-12-16T14:12:04Z

crates/fuel-core/src/service/adapters.rs

+
+#[async_trait::async_trait]
+impl GasPriceEstimate for ArcGasPriceEstimate<u32, u64> {
+    async fn worst_case_gas_price(&self, height: BlockHeight) -> Option<u64> {


so what I get here is that we assume that the price is going to raise by the specified percentage every block until height is reached, is that correct?

acerone85 · 2024-12-16T14:14:33Z

crates/fuel-core/src/service/adapters.rs

+    best_gas_price: u64,
+    best_height: u32,
+    percentage: u64,
+    target_height: u32,


nit: you can pass directly the difference of target_height and best_height to this function and get rid of one parameter, but I don't have a strong opinion here

I'm fine with this for now.

acerone85 · 2024-12-16T14:14:50Z

crates/fuel-core/src/service/adapters.rs

+
+#[allow(clippy::cast_possible_truncation)]
+pub(crate) fn cumulative_percentage_change(
+    best_gas_price: u64,


nit: maybe start_gas_price is better?

acerone85 · 2024-12-16T14:15:34Z

crates/fuel-core/src/service/adapters.rs

+    let multiple = (1.0f64 + percentage_as_decimal).powf(blocks);
+    let mut approx = best_gas_price as f64 * multiple;
+    // account for rounding errors and take a slightly higher value
+    const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0;


can you put a comment about how this value has been obtained?

acerone85 · 2024-12-16T14:16:03Z

crates/fuel-core/src/service/adapters.rs

+    // account for rounding errors and take a slightly higher value
+    const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0;
+    if approx > ROUNDING_ERROR_CUTOFF {
+        const ROUNDING_ERROR_COMPENSATION: f64 = 2000.0;


same as above https://github.com/FuelLabs/fuel-core/pull/2501/files#r1886892073

acerone85 · 2024-12-16T14:26:29Z

crates/fuel-core/src/service/adapters.rs

+            expected = expected.saturating_add(change_amount);
+        }
+
+        assert!(actual >= expected);


I think we can be more precise here, and place an upper bound on actual, e.g.

Suggested change

assert!(actual >= expected);

assert!(actual >= expected);

assert!(actual <= expected.saturating_add(2000.0))

Sure. But we don't actually care about the lower bound of this. In reality, the gas price given by the block producer will be much lower than what we estimate 99% of the time. So we only really care that the block producer never gives a value higher than what we estimate.

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

acerone85 · 2024-12-16T16:39:59Z

I'm not sure I understand what the role of ArcGasPriceEstimate is in this PR, since it seems to be used only in tests

xgreenx · 2024-12-31T00:03:06Z

crates/fuel-core/src/service/adapters.rs

+        let best_gas_price = self.get_gas_price();
+        let best_height = self.get_height();


You can have a race condition here, because between self.get_gas_price and self.get_height the value can change. You can lock it, read both values, and unlock it again to avoid this issue.

xgreenx · 2024-12-31T00:05:22Z

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

@@ -105,6 +126,7 @@ where
    pub fn new(
        l2_block_source: L2,
        shared_algo: SharedV1Algorithm,
+        latest_gas_price: Arc<parking_lot::RwLock<(u32, u64)>>,


It would be nice to have a type that hides this lock(Arc<parking_lot::RwLock<(u32, u64)>>) inside. Later we can easier refactor it, plus you will not expose direct access to the lock.

xgreenx · 2024-12-31T00:06:24Z

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

@@ -481,10 +506,12 @@ mod tests {
            ),
            None,
        );
+        let latest_gas_price = Arc::new(parking_lot::RwLock::new((0, 0)));


Yeah, maybe we oversplitting here. Algorithm could answer this, if we store the last used gas price there. But I think Mitch wants to reuse the code for v0. But maybe it can live in common and used in both algorithm.

MitchTurner · 2024-12-31T19:55:01Z

I'm not sure I understand what the role of ArcGasPriceEstimate is in this PR, since it seems to be used only in tests
Yeah. It will be used by the GraphQL service. Just showing that it works here.

MitchTurner added 2 commits December 15, 2024 20:52

Add new adapter with tests

181430e

Add updating code to Gas Price Service

f3eba7b

MitchTurner marked this pull request as ready for review December 16, 2024 04:33

MitchTurner requested review from xgreenx and Dentosal as code owners December 16, 2024 04:33

rymnc reviewed Dec 16, 2024

View reviewed changes

acerone85 reviewed Dec 16, 2024

View reviewed changes

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

netrome assigned MitchTurner Dec 16, 2024

netrome mentioned this pull request Dec 17, 2024

Chore/non breaking cli arguments with sensible defaults #2507

Merged

Merge branch 'master' into use-real-gas-price-for-estimate

54447de

xgreenx reviewed Dec 31, 2024

View reviewed changes

MitchTurner added 3 commits December 31, 2024 12:26

Give clarity with comments, better names

00d7c9a

Fix dead lock race conditino

1ad2f60

Fix typo

f5f80e3

Add type for latest gas price to hide lock

9a35d5d

xgreenx previously approved these changes Jan 2, 2025

View reviewed changes

Merge branch 'master' into use-real-gas-price-for-estimate

b5eccc1

xgreenx dismissed their stale review via b5eccc1 January 2, 2025 23:48

xgreenx approved these changes Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gas prices from actual blocks to calculate estimate gas prices #2501

Use gas prices from actual blocks to calculate estimate gas prices #2501

MitchTurner commented Dec 16, 2024 •

edited

Loading

rymnc left a comment

rymnc Dec 16, 2024

rymnc Dec 16, 2024

rymnc Dec 16, 2024

MitchTurner Dec 16, 2024

xgreenx Dec 31, 2024

acerone85 Dec 16, 2024

MitchTurner Dec 31, 2024

acerone85 Dec 16, 2024 •

edited

Loading

acerone85 Dec 16, 2024 •

edited

Loading

MitchTurner Dec 31, 2024

acerone85 Dec 16, 2024

MitchTurner Dec 31, 2024

acerone85 Dec 16, 2024

acerone85 Dec 16, 2024

acerone85 Dec 16, 2024

acerone85 Dec 16, 2024

MitchTurner Dec 31, 2024

acerone85 commented Dec 16, 2024

xgreenx Dec 31, 2024

xgreenx Dec 31, 2024

xgreenx Dec 31, 2024

MitchTurner commented Dec 31, 2024

	async fn _worst_case__correctly_calculates_value(
	async fn worst_case__correctly_calculates_value(

	assert!(actual >= expected);
	assert!(actual >= expected);
	assert!(actual <= expected.saturating_add(2000.0))

		let best_gas_price = self.get_gas_price();
		let best_height = self.get_height();

Use gas prices from actual blocks to calculate estimate gas prices #2501

Are you sure you want to change the base?

Use gas prices from actual blocks to calculate estimate gas prices #2501

Conversation

MitchTurner commented Dec 16, 2024 • edited Loading

Linked Issues/PRs

Description

Checklist

Before requesting review

After merging, notify other teams

rymnc 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 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

acerone85 Dec 16, 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

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 Dec 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner commented Dec 31, 2024

MitchTurner commented Dec 16, 2024 •

edited

Loading

acerone85 Dec 16, 2024 •

edited

Loading

acerone85 Dec 16, 2024 •

edited

Loading