Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions crates/fuel-gas-price-algorithm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,24 @@

pub mod v0;
pub mod v1;

#[allow(clippy::cast_possible_truncation)]
pub(crate) fn cumulative_percentage(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should name this "cumulative_percentage_change" or something like that instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in adffd99

new_exec_price: u64,
for_height: u32,
percentage: u64,
height: u32,
) -> u64 {
let blocks = height.saturating_sub(for_height) as f64;
let percentage_as_decimal = percentage as f64 / 100.0;
let multiple = (1.0f64 + percentage_as_decimal).powf(blocks);
let mut approx = new_exec_price as f64 * multiple;
// account for rounding errors and take a slightly higher value
const ARB_CUTOFF: f64 = 16948547188989277.0;
if approx > ARB_CUTOFF {
const ARB_ADDITION: f64 = 2000.0;
approx += ARB_ADDITION;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what happens here. What's the meaning of ARB_CUTOFF?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a hack for dealing with really large floats. The rounding error will be on the order of 2000 and based on the prop tests will always result in a value that is larger than the actual worst case value.

So ARB_CUTOFF is where we start seeing the error and ARB_ADDITION is the the amount we add to account for that error. This is the cost of using floats to represent these values, maybe there is a cleaner way to represent it :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't sound arbitrary then. Can we rename it to something like this?

Suggested change
// account for rounding errors and take a slightly higher value
const ARB_CUTOFF: f64 = 16948547188989277.0;
if approx > ARB_CUTOFF {
const ARB_ADDITION: f64 = 2000.0;
approx += ARB_ADDITION;
}
// 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;
approx += ROUNDING_ERROR_COMPENSATION;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Background:

#2025 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't sound arbitrary then. Can we rename it to something like this?

There isn't a "correct" value to choose here, that's why I called it ARB, but you're right that the reason we chose it isn't completely arbitrary and your suggestion is much clearer and helpful anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable names updated in 4337b14

// `f64` over `u64::MAX` are cast to `u64::MAX`
approx.ceil() as u64
}
23 changes: 8 additions & 15 deletions crates/fuel-gas-price-algorithm/src/v0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ use std::{
num::NonZeroU64,
};

use crate::cumulative_percentage;

#[cfg(test)]
mod tests;

Expand All @@ -27,22 +29,13 @@ impl AlgorithmV0 {
self.new_exec_price
}

#[allow(clippy::cast_possible_truncation)]
pub fn worst_case(&self, height: u32) -> u64 {
let price = self.new_exec_price as f64;
let blocks = height.saturating_sub(self.for_height) as f64;
let percentage = self.percentage as f64;
let percentage_as_decimal = percentage / 100.0;
let multiple = (1.0f64 + percentage_as_decimal).powf(blocks);
let mut approx = price * multiple;
// account for rounding errors and take a slightly higher value
const ARB_CUTOFF: f64 = 16948547188989277.0;
if approx > ARB_CUTOFF {
const ARB_ADDITION: f64 = 2000.0;
approx += ARB_ADDITION;
}
// `f64` over `u64::MAX` are cast to `u64::MAX`
approx.ceil() as u64
cumulative_percentage(
self.new_exec_price,
self.for_height,
self.percentage,
height,
)
}
}

Expand Down
43 changes: 36 additions & 7 deletions crates/fuel-gas-price-algorithm/src/v1.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use std::{
ops::Div,
};

use crate::cumulative_percentage;

#[cfg(test)]
mod tests;

Expand All @@ -21,13 +23,37 @@ pub enum Error {

#[derive(Debug, Clone, PartialEq)]
pub struct AlgorithmV1 {
/// the combined Execution and DA gas prices
combined_gas_price: u64,
/// The gas price for to cover the execution of the next block
new_exec_price: u64,
/// The change percentage per block
exec_price_percentage: u64,
/// The gas price for to cover DA commitment
new_da_gas_price: u64,
/// The change percentage per block
da_gas_price_percentage: u64,
/// The block height of the next L2 block
for_height: u32,
}

impl AlgorithmV1 {
pub fn calculate(&self) -> u64 {
self.combined_gas_price
self.new_exec_price.saturating_add(self.new_da_gas_price)
}

pub fn worst_case(&self, height: u32) -> u64 {
let exec = cumulative_percentage(
self.new_exec_price,
self.for_height,
self.exec_price_percentage,
height,
);
let da = cumulative_percentage(
self.new_da_gas_price,
self.for_height,
self.da_gas_price_percentage,
height,
);
exec.saturating_add(da)
}
}

Expand Down Expand Up @@ -370,10 +396,13 @@ impl AlgorithmUpdaterV1 {
}

pub fn algorithm(&self) -> AlgorithmV1 {
let combined_gas_price = self
.descaled_exec_price()
.saturating_add(self.descaled_da_price());
AlgorithmV1 { combined_gas_price }
AlgorithmV1 {
new_exec_price: self.descaled_exec_price(),
exec_price_percentage: self.exec_gas_price_change_percent as u64,
new_da_gas_price: self.descaled_da_price(),
da_gas_price_percentage: self.max_da_gas_price_change_percent as u64,
for_height: self.l2_block_height,
}
}

// We only need to track the difference between the rewards and costs after we have true DA data
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
use crate::v1::tests::UpdaterBuilder;
use crate::v1::{
tests::UpdaterBuilder,
AlgorithmV1,
};
use proptest::prelude::*;

#[test]
fn calculate__returns_sum_of_da_and_exec_gas_prices() {
Expand Down Expand Up @@ -28,3 +32,60 @@ fn calculate__returns_sum_of_da_and_exec_gas_prices() {
let expected = starting_exec_gas_price + starting_da_gas_price;
assert_eq!(expected, actual);
}

fn _worst_case__correctly_calculates_value(
new_exec_price: u64,
new_da_gas_price: u64,
for_height: u32,
block_horizon: u32,
exec_price_percentage: u64,
da_gas_price_percentage: u64,
) {
// given
let algorithm = AlgorithmV1 {
new_exec_price,
exec_price_percentage,
new_da_gas_price,
da_gas_price_percentage,
for_height,
};

// when
let target_height = for_height.saturating_add(block_horizon);
let actual = algorithm.worst_case(target_height);

// then
let mut expected_exec_price = new_exec_price;
let mut expected_da_gas_price = new_da_gas_price;

for _ in 0..block_horizon {
let change_amount = expected_exec_price
.saturating_mul(exec_price_percentage)
.saturating_div(100);
expected_exec_price = expected_exec_price.saturating_add(change_amount);

let change_amount = expected_da_gas_price
.saturating_mul(da_gas_price_percentage)
.saturating_div(100);
expected_da_gas_price = expected_da_gas_price.saturating_add(change_amount);
}

let expected = expected_exec_price.saturating_add(expected_da_gas_price);

dbg!(actual, expected);
assert!(actual >= expected);
}

proptest! {
#[test]
fn worst_case__correctly_calculates_value(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add another test to check for overflows. See the V0 test and this convo:
#2025 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added in 6cb3161

exec_price: u64,
da_price: u64,
starting_height: u32,
block_horizon in 0..10_000u32,
exec_percentage: u64,
da_percentage: u64,
) {
_worst_case__correctly_calculates_value(exec_price, da_price, starting_height, block_horizon, exec_percentage, da_percentage);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,7 @@ impl GasPriceAlgorithm for Algorithm {
fn worst_case_gas_price(&self, height: BlockHeight) -> u64 {
match self {
Algorithm::V0(v0) => v0.worst_case(height.into()),
// TODO: Add worst_case calculation https://github.com/FuelLabs/fuel-core/issues/2203
Algorithm::V1(v1) => v1.calculate(),
Algorithm::V1(v1) => v1.worst_case(height.into()),
}
}
}