From eaa26166801e956e42b72c4cfc2511412b251cf3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Mon, 23 Sep 2024 15:50:14 +0200 Subject: [PATCH 1/5] Protect against passing `i128::MIN` to `abs()` which causes overflow --- crates/fuel-gas-price-algorithm/src/v1.rs | 6 ++++ .../src/v1/tests/algorithm_v1_tests.rs | 31 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/crates/fuel-gas-price-algorithm/src/v1.rs b/crates/fuel-gas-price-algorithm/src/v1.rs index b79d5968f28..f7a5d3127e0 100644 --- a/crates/fuel-gas-price-algorithm/src/v1.rs +++ b/crates/fuel-gas-price-algorithm/src/v1.rs @@ -325,12 +325,18 @@ impl AlgorithmUpdaterV1 { fn da_change(&self, p: i128, d: i128) -> i128 { let pd_change = p.saturating_add(d); + let pd_change = if pd_change == i128::MIN { + pd_change + 1 + } else { + pd_change + }; let upcast_percent = self.max_da_gas_price_change_percent.into(); let max_change = self .new_scaled_da_gas_price .saturating_mul(upcast_percent) .saturating_div(100) .into(); + debug_assert!(pd_change != i128::MIN); let clamped_change = pd_change.abs().min(max_change); pd_change.signum().saturating_mul(clamped_change) } diff --git a/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs b/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs index 5578af6bdc2..ed8aaa332e2 100644 --- a/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs +++ b/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs @@ -1,6 +1,10 @@ +use std::num::NonZeroU64; + use crate::v1::{ tests::UpdaterBuilder, + AlgorithmUpdaterV1, AlgorithmV1, + ClampedPercentage, }; use proptest::prelude::*; @@ -143,3 +147,30 @@ fn worst_case__same_block_gives_the_same_value_as_calculate() { let expected = algorithm.calculate(); assert_eq!(expected, actual); } + +#[test] +fn da_change_should_not_panic() { + let updater = AlgorithmUpdaterV1 { + new_scaled_exec_price: 0, + min_exec_gas_price: 0, + exec_gas_price_change_percent: 0, + l2_block_height: 0, + l2_block_fullness_threshold_percent: ClampedPercentage::new(0), + new_scaled_da_gas_price: 0, + gas_price_factor: NonZeroU64::new(1).unwrap(), + min_da_gas_price: 0, + max_da_gas_price_change_percent: 0, + total_da_rewards_excess: 0, + da_recorded_block_height: 0, + latest_known_total_da_cost_excess: 0, + projected_total_da_cost: 0, + da_p_component: 0, + da_d_component: 0, + last_profit: 0, + second_to_last_profit: 0, + latest_da_cost_per_byte: 0, + unrecorded_blocks: vec![], + }; + + updater.da_change(i128::MIN / 2, i128::MIN / 2); +} From 5c0f1fd28b7bc6b7c163ab8cb9490438e5237b6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Mon, 23 Sep 2024 17:13:59 +0200 Subject: [PATCH 2/5] Extract `safe_signed_abs()` function --- crates/fuel-gas-price-algorithm/src/utils.rs | 21 ++++++++++++++++++++ crates/fuel-gas-price-algorithm/src/v1.rs | 13 +++++------- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/crates/fuel-gas-price-algorithm/src/utils.rs b/crates/fuel-gas-price-algorithm/src/utils.rs index 7b6f14a24ad..5d54c08cb49 100644 --- a/crates/fuel-gas-price-algorithm/src/utils.rs +++ b/crates/fuel-gas-price-algorithm/src/utils.rs @@ -18,3 +18,24 @@ pub(crate) fn cumulative_percentage_change( // `f64` over `u64::MAX` are cast to `u64::MAX` approx.ceil() as u64 } + +pub(crate) fn safe_signed_abs(n: i128) -> i128 { + let n = if n == i128::MIN { + n.saturating_add(1) + } else { + n + }; + debug_assert!(n != i128::MIN); + n.abs() +} + +#[cfg(test)] +mod tests { + use crate::utils::safe_signed_abs; + + #[test] + fn safe_signed_abs_does_not_overflow_on_min_value() { + let abs = safe_signed_abs(i128::MIN); + assert_eq!(abs, i128::MAX); + } +} diff --git a/crates/fuel-gas-price-algorithm/src/v1.rs b/crates/fuel-gas-price-algorithm/src/v1.rs index f7a5d3127e0..e33a444a11b 100644 --- a/crates/fuel-gas-price-algorithm/src/v1.rs +++ b/crates/fuel-gas-price-algorithm/src/v1.rs @@ -4,7 +4,10 @@ use std::{ ops::Div, }; -use crate::utils::cumulative_percentage_change; +use crate::utils::{ + cumulative_percentage_change, + safe_signed_abs, +}; #[cfg(test)] mod tests; @@ -325,19 +328,13 @@ impl AlgorithmUpdaterV1 { fn da_change(&self, p: i128, d: i128) -> i128 { let pd_change = p.saturating_add(d); - let pd_change = if pd_change == i128::MIN { - pd_change + 1 - } else { - pd_change - }; let upcast_percent = self.max_da_gas_price_change_percent.into(); let max_change = self .new_scaled_da_gas_price .saturating_mul(upcast_percent) .saturating_div(100) .into(); - debug_assert!(pd_change != i128::MIN); - let clamped_change = pd_change.abs().min(max_change); + let clamped_change = safe_signed_abs(pd_change).min(max_change); pd_change.signum().saturating_mul(clamped_change) } From 056ba1e9592955650536c851a68c978cb5b58445 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Mon, 23 Sep 2024 21:35:54 +0200 Subject: [PATCH 3/5] Use `saturating_abs()` to protect against overflow --- crates/fuel-gas-price-algorithm/src/utils.rs | 21 -------------------- crates/fuel-gas-price-algorithm/src/v1.rs | 7 ++----- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/crates/fuel-gas-price-algorithm/src/utils.rs b/crates/fuel-gas-price-algorithm/src/utils.rs index 5d54c08cb49..7b6f14a24ad 100644 --- a/crates/fuel-gas-price-algorithm/src/utils.rs +++ b/crates/fuel-gas-price-algorithm/src/utils.rs @@ -18,24 +18,3 @@ pub(crate) fn cumulative_percentage_change( // `f64` over `u64::MAX` are cast to `u64::MAX` approx.ceil() as u64 } - -pub(crate) fn safe_signed_abs(n: i128) -> i128 { - let n = if n == i128::MIN { - n.saturating_add(1) - } else { - n - }; - debug_assert!(n != i128::MIN); - n.abs() -} - -#[cfg(test)] -mod tests { - use crate::utils::safe_signed_abs; - - #[test] - fn safe_signed_abs_does_not_overflow_on_min_value() { - let abs = safe_signed_abs(i128::MIN); - assert_eq!(abs, i128::MAX); - } -} diff --git a/crates/fuel-gas-price-algorithm/src/v1.rs b/crates/fuel-gas-price-algorithm/src/v1.rs index e33a444a11b..5d111dde64b 100644 --- a/crates/fuel-gas-price-algorithm/src/v1.rs +++ b/crates/fuel-gas-price-algorithm/src/v1.rs @@ -4,10 +4,7 @@ use std::{ ops::Div, }; -use crate::utils::{ - cumulative_percentage_change, - safe_signed_abs, -}; +use crate::utils::cumulative_percentage_change; #[cfg(test)] mod tests; @@ -334,7 +331,7 @@ impl AlgorithmUpdaterV1 { .saturating_mul(upcast_percent) .saturating_div(100) .into(); - let clamped_change = safe_signed_abs(pd_change).min(max_change); + let clamped_change = pd_change.saturating_abs().min(max_change); pd_change.signum().saturating_mul(clamped_change) } From 3047c7e2024081212514b0bd3304ca37986fe732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Tue, 24 Sep 2024 16:50:35 +0200 Subject: [PATCH 4/5] Test overflow fix via `update_l2_block_data()` --- .../src/v1/tests/algorithm_v1_tests.rs | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs b/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs index ed8aaa332e2..37c7ead8006 100644 --- a/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs +++ b/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs @@ -150,27 +150,37 @@ fn worst_case__same_block_gives_the_same_value_as_calculate() { #[test] fn da_change_should_not_panic() { - let updater = AlgorithmUpdaterV1 { + const HEIGHT: u32 = 0; + + // The following values are chosen to make the `p.saturating_add(d)` result in + // i128::MIN, which would cause a panic if the calculation is not using `saturating_abs()`. + const LAST_PROFIT: i128 = i128::MIN / 2; + const TOTAL_DA_REWARDS_EXCESS: i128 = i128::MAX / 2; + const P: i64 = 1; + const D: i64 = 1; + + let mut updater = AlgorithmUpdaterV1 { new_scaled_exec_price: 0, min_exec_gas_price: 0, exec_gas_price_change_percent: 0, - l2_block_height: 0, + l2_block_height: HEIGHT, l2_block_fullness_threshold_percent: ClampedPercentage::new(0), new_scaled_da_gas_price: 0, gas_price_factor: NonZeroU64::new(1).unwrap(), min_da_gas_price: 0, max_da_gas_price_change_percent: 0, - total_da_rewards_excess: 0, + total_da_rewards_excess: TOTAL_DA_REWARDS_EXCESS as u128, da_recorded_block_height: 0, latest_known_total_da_cost_excess: 0, projected_total_da_cost: 0, - da_p_component: 0, - da_d_component: 0, - last_profit: 0, + da_p_component: P, + da_d_component: D, + last_profit: LAST_PROFIT, second_to_last_profit: 0, latest_da_cost_per_byte: 0, unrecorded_blocks: vec![], }; - updater.da_change(i128::MIN / 2, i128::MIN / 2); + let _ = + updater.update_l2_block_data(HEIGHT + 1, 0, NonZeroU64::new(1).unwrap(), 0, 0); } From 7359bd7a850e6a48732c4bc887526dd39093f196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Chabowski?= Date: Wed, 25 Sep 2024 10:06:35 +0200 Subject: [PATCH 5/5] Remove the redundant test as `da_change` no longer panics --- .../src/v1/tests/algorithm_v1_tests.rs | 41 ------------------- 1 file changed, 41 deletions(-) diff --git a/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs b/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs index 37c7ead8006..5578af6bdc2 100644 --- a/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs +++ b/crates/fuel-gas-price-algorithm/src/v1/tests/algorithm_v1_tests.rs @@ -1,10 +1,6 @@ -use std::num::NonZeroU64; - use crate::v1::{ tests::UpdaterBuilder, - AlgorithmUpdaterV1, AlgorithmV1, - ClampedPercentage, }; use proptest::prelude::*; @@ -147,40 +143,3 @@ fn worst_case__same_block_gives_the_same_value_as_calculate() { let expected = algorithm.calculate(); assert_eq!(expected, actual); } - -#[test] -fn da_change_should_not_panic() { - const HEIGHT: u32 = 0; - - // The following values are chosen to make the `p.saturating_add(d)` result in - // i128::MIN, which would cause a panic if the calculation is not using `saturating_abs()`. - const LAST_PROFIT: i128 = i128::MIN / 2; - const TOTAL_DA_REWARDS_EXCESS: i128 = i128::MAX / 2; - const P: i64 = 1; - const D: i64 = 1; - - let mut updater = AlgorithmUpdaterV1 { - new_scaled_exec_price: 0, - min_exec_gas_price: 0, - exec_gas_price_change_percent: 0, - l2_block_height: HEIGHT, - l2_block_fullness_threshold_percent: ClampedPercentage::new(0), - new_scaled_da_gas_price: 0, - gas_price_factor: NonZeroU64::new(1).unwrap(), - min_da_gas_price: 0, - max_da_gas_price_change_percent: 0, - total_da_rewards_excess: TOTAL_DA_REWARDS_EXCESS as u128, - da_recorded_block_height: 0, - latest_known_total_da_cost_excess: 0, - projected_total_da_cost: 0, - da_p_component: P, - da_d_component: D, - last_profit: LAST_PROFIT, - second_to_last_profit: 0, - latest_da_cost_per_byte: 0, - unrecorded_blocks: vec![], - }; - - let _ = - updater.update_l2_block_data(HEIGHT + 1, 0, NonZeroU64::new(1).unwrap(), 0, 0); -}