From de286d0225d879166071a98a4ada1759e4e4aeb4 Mon Sep 17 00:00:00 2001 From: xiaodi <334830452@qq.com> Date: Fri, 22 Nov 2024 16:34:11 +0800 Subject: [PATCH 1/4] Update transaction_deferral Fix integer overflow in range_for_up_to_consensus_round Prevent integer underflow in transaction_deferral_within_limit --- .../src/authority/transaction_deferral.rs | 43 +++++++++++++++++-- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/crates/sui-core/src/authority/transaction_deferral.rs b/crates/sui-core/src/authority/transaction_deferral.rs index 288ce26013edc..1d3a113ab0af4 100644 --- a/crates/sui-core/src/authority/transaction_deferral.rs +++ b/crates/sui-core/src/authority/transaction_deferral.rs @@ -54,7 +54,7 @@ impl DeferralKey { deferred_from_round: 0, }, Self::ConsensusRound { - future_round: consensus_round.checked_add(1).unwrap(), + future_round: consensus_round.saturating_add(1), deferred_from_round: 0, }, ) @@ -90,11 +90,15 @@ pub fn transaction_deferral_within_limit( deferred_from_round, } = deferral_key { - return (future_round - deferred_from_round) <= max_deferral_rounds_for_congestion_control; + if let Some(diff) = future_round.checked_sub(*deferred_from_round) { + return diff <= max_deferral_rounds_for_congestion_control; + } else { + // future_round < deferred_from_round + return false; + } } - // TODO: drop transactions at the end of the queue if the queue is too long. - + true } @@ -209,4 +213,35 @@ mod object_cost_tests { } assert!(result_count > 0); } + #[test] + fn test_range_for_up_to_consensus_round_no_overflow() { + let consensus_round = u64::MAX; + let (_, max_key) = DeferralKey::range_for_up_to_consensus_round(consensus_round); + + match max_key { + DeferralKey::ConsensusRound { future_round, .. } => { + assert_eq!(future_round, u64::MAX); + } + _ => panic!("Expected ConsensusRound variant"), + } + } + #[test] + fn test_transaction_deferral_within_limit_underflow() { + let future_round = 100; + let deferred_from_round = 200; + let max_deferral_rounds_for_congestion_control = 10; + + let deferral_key = DeferralKey::new_for_consensus_round(future_round, deferred_from_round); + let result = transaction_deferral_within_limit( + &deferral_key, + max_deferral_rounds_for_congestion_control, + ); + + // Expected result is false due to future_round < deferred_from_round + assert_eq!( + result, + false, + "Expected false because future_round < deferred_from_round" + ); + } } From cea3d0b561f6943dcf9ab2c6d3b6993f411c3935 Mon Sep 17 00:00:00 2001 From: xiaodi <334830452@qq.com> Date: Sat, 23 Nov 2024 11:42:41 +0800 Subject: [PATCH 2/4] Update transaction_deferral.rs Add a debug_assert! to enforce the invariant during object creation; and adjust future_round to be at least deferred_from_round in release builds to maintain the invariant without panicking in new_for_consensus_round Add debug_assert!(future_round >= deferred_from_round); and use saturating_sub in transaction_deferral_within_limit. --- .../src/authority/transaction_deferral.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/sui-core/src/authority/transaction_deferral.rs b/crates/sui-core/src/authority/transaction_deferral.rs index 1d3a113ab0af4..4a4c0933fc8f3 100644 --- a/crates/sui-core/src/authority/transaction_deferral.rs +++ b/crates/sui-core/src/authority/transaction_deferral.rs @@ -29,6 +29,15 @@ impl DeferralKey { } pub fn new_for_consensus_round(future_round: Round, deferred_from_round: Round) -> Self { + if future_round < deferred_from_round { + debug_fatal!("future_round must be greater than or equal to deferred_from_round "); + + return Self::ConsensusRound { + future_round: deferred_from_round, + deferred_from_round, + }; + } + Self::ConsensusRound { future_round, deferred_from_round, @@ -90,12 +99,10 @@ pub fn transaction_deferral_within_limit( deferred_from_round, } = deferral_key { - if let Some(diff) = future_round.checked_sub(*deferred_from_round) { - return diff <= max_deferral_rounds_for_congestion_control; - } else { - // future_round < deferred_from_round - return false; - } + debug_assert!(future_round >= *deferred_from_round); + + let diff = future_round.saturating_sub(*deferred_from_round); + return diff <= max_deferral_rounds_for_congestion_control; } // TODO: drop transactions at the end of the queue if the queue is too long. From 18532e69e684b13f259d23c7bf49b3458fb9f870 Mon Sep 17 00:00:00 2001 From: xiaodi <334830452@qq.com> Date: Sat, 23 Nov 2024 11:44:38 +0800 Subject: [PATCH 3/4] Update transaction_deferral.rs Revert the change in range_for_up_to_consensus_round to keep checked_add(1).unwrap() --- crates/sui-core/src/authority/transaction_deferral.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/sui-core/src/authority/transaction_deferral.rs b/crates/sui-core/src/authority/transaction_deferral.rs index 4a4c0933fc8f3..3123a7e12871f 100644 --- a/crates/sui-core/src/authority/transaction_deferral.rs +++ b/crates/sui-core/src/authority/transaction_deferral.rs @@ -63,7 +63,7 @@ impl DeferralKey { deferred_from_round: 0, }, Self::ConsensusRound { - future_round: consensus_round.saturating_add(1), + future_round: consensus_round.checked_add(1).unwrap(), deferred_from_round: 0, }, ) From fb426e50c318f70e48bb568f2ba9d0bb3dc0d5fc Mon Sep 17 00:00:00 2001 From: xiaodi <334830452@qq.com> Date: Sat, 23 Nov 2024 11:54:41 +0800 Subject: [PATCH 4/4] Update transaction_deferral.rs import debug_fatal --- .../src/authority/transaction_deferral.rs | 34 ++----------------- 1 file changed, 2 insertions(+), 32 deletions(-) diff --git a/crates/sui-core/src/authority/transaction_deferral.rs b/crates/sui-core/src/authority/transaction_deferral.rs index 3123a7e12871f..9509056fa3906 100644 --- a/crates/sui-core/src/authority/transaction_deferral.rs +++ b/crates/sui-core/src/authority/transaction_deferral.rs @@ -3,6 +3,7 @@ use narwhal_types::Round; use serde::{Deserialize, Serialize}; +use mysten_common::debug_fatal; use sui_types::base_types::ObjectID; #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize)] @@ -99,7 +100,7 @@ pub fn transaction_deferral_within_limit( deferred_from_round, } = deferral_key { - debug_assert!(future_round >= *deferred_from_round); + debug_assert!(future_round >= deferred_from_round); let diff = future_round.saturating_sub(*deferred_from_round); return diff <= max_deferral_rounds_for_congestion_control; @@ -220,35 +221,4 @@ mod object_cost_tests { } assert!(result_count > 0); } - #[test] - fn test_range_for_up_to_consensus_round_no_overflow() { - let consensus_round = u64::MAX; - let (_, max_key) = DeferralKey::range_for_up_to_consensus_round(consensus_round); - - match max_key { - DeferralKey::ConsensusRound { future_round, .. } => { - assert_eq!(future_round, u64::MAX); - } - _ => panic!("Expected ConsensusRound variant"), - } - } - #[test] - fn test_transaction_deferral_within_limit_underflow() { - let future_round = 100; - let deferred_from_round = 200; - let max_deferral_rounds_for_congestion_control = 10; - - let deferral_key = DeferralKey::new_for_consensus_round(future_round, deferred_from_round); - let result = transaction_deferral_within_limit( - &deferral_key, - max_deferral_rounds_for_congestion_control, - ); - - // Expected result is false due to future_round < deferred_from_round - assert_eq!( - result, - false, - "Expected false because future_round < deferred_from_round" - ); - } }