Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integer overflow and underflow in DeferralKey methods and add corresponding tests #20387

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
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
43 changes: 39 additions & 4 deletions crates/sui-core/src/authority/transaction_deferral.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

If an epoch runs for long enough that consensus_round reaches u64::MAX (or gets this close to it), then I think we want to panic, because there is no safe way for the system to handle that

Keep in mind that at our current rate of 14 rounds/s it would take an epoch of over 4e10 years for us to reach this limit, even if we somehow 1000x this rate (impossible I think) then it would take 10s of millions of years :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, if we did reach this point, doing a saturating add would cause incorrect behavior.

deferred_from_round: 0,
},
)
Expand Down Expand Up @@ -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) {
xiaodi007 marked this conversation as resolved.
Show resolved Hide resolved
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
}

Expand Down Expand Up @@ -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"
);
}
}
Loading