Skip to content

Commit fd2a4a6

Browse files
committed
Saturate numerators for safety
1 parent 2bfac63 commit fd2a4a6

File tree

1 file changed

+46
-28
lines changed

1 file changed

+46
-28
lines changed

interface/src/warmup_cooldown_allowance.rs

Lines changed: 46 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -76,24 +76,20 @@ fn rate_limited_stake_change(
7676
// `change = (account_portion * cluster_effective * rate_bps) / (cluster_portion * BASIS_POINTS_PER_UNIT)`
7777
//
7878
// Using `u128` for the intermediate calculations to prevent overflow.
79+
// If the multiplication would overflow, we saturate to u128::MAX. This ensures
80+
// that even in extreme edge cases, the rate-limiting invariant is maintained
81+
// (fail-safe) rather than bypassing rate limits entirely (fail-open).
7982
let numerator = (account_portion as u128)
80-
.checked_mul(cluster_effective as u128)
81-
.and_then(|x| x.checked_mul(rate_bps as u128));
83+
.saturating_mul(cluster_effective as u128)
84+
.saturating_mul(rate_bps as u128);
8285
let denominator = (cluster_portion as u128).saturating_mul(BASIS_POINTS_PER_UNIT as u128);
8386

84-
match numerator {
85-
Some(n) => {
86-
// Safe unwrap as denominator cannot be zero
87-
let delta = n.checked_div(denominator).unwrap();
88-
// The calculated delta can be larger than `account_portion` if the network's stake change
89-
// allowance is greater than the total stake waiting to change. In this case, the account's
90-
// entire portion is allowed to change.
91-
delta.min(account_portion as u128) as u64
92-
}
93-
// Overflowing u128 is not a realistic scenario except in tests. However, in that case
94-
// it's reasonable to allow activation/deactivation of the account's entire portion.
95-
None => account_portion,
96-
}
87+
// Safe unwrap as denominator cannot be zero due to early return guards above
88+
let delta = numerator.checked_div(denominator).unwrap();
89+
// The calculated delta can be larger than `account_portion` if the network's stake change
90+
// allowance is greater than the total stake waiting to change. In this case, the account's
91+
// entire portion is allowed to change.
92+
delta.min(account_portion as u128) as u64
9793
}
9894

9995
#[cfg(test)]
@@ -200,24 +196,46 @@ mod test {
200196
}
201197

202198
#[test]
203-
fn activation_overflow_path_returns_account_portion() {
204-
// Force the u128 multiply to overflow: (u64::MAX * u64::MAX * 9) overflows u128.
205-
// When that happens, the helper returns the full account_portion.
206-
let current_epoch = 0;
207-
let new_rate_activation_epoch = Some(0); // use "current" 9/100 to maximize multiplier
199+
fn activation_overflow_scenario_still_rate_limits() {
200+
// Extreme scenario where a single account holding nearly the total supply
201+
// and tries to activate everything at once. Asserting rate limiting is maintained.
202+
let supply_lamports: u64 = 400_000_000_000_000_000; // 400M SOL
203+
let account_portion = supply_lamports;
208204
let prev = StakeHistoryEntry {
209-
activating: 1, // non-zero cluster_portion
210-
effective: u64::MAX, // huge cluster_effective
205+
activating: supply_lamports,
206+
effective: supply_lamports,
211207
..Default::default()
212208
};
213-
let account_portion = u64::MAX;
209+
214210
let result = calculate_activation_allowance(
215-
current_epoch,
211+
100,
216212
account_portion,
217213
&prev,
218-
new_rate_activation_epoch,
214+
None, // forces 25% rate
219215
);
220-
assert_eq!(result, account_portion);
216+
217+
// Verify overflow actually occurs in this scenario
218+
let rate_bps = ORIGINAL_WARMUP_COOLDOWN_RATE_BPS;
219+
let would_overflow = (account_portion as u128)
220+
.checked_mul(supply_lamports as u128)
221+
.and_then(|n| n.checked_mul(rate_bps as u128))
222+
.is_none();
223+
assert!(would_overflow);
224+
225+
// The ideal result (with infinite precision) is 25% of the stake.
226+
// 400M * 0.25 = 100M
227+
let ideal_allowance = supply_lamports / 4;
228+
229+
// With saturation fix:
230+
// Numerator saturates to u128::MAX (≈ 3.4e38)
231+
// Denominator = 4e17 * 10,000 = 4e21
232+
// Result = 3.4e38 / 4e21 ≈ 8.5e16 (85M SOL)
233+
// 85M is ~21.25% of the stake (fail-safe)
234+
// If we allowed unlocking the full account portion it would have been 100% (fail-open)
235+
assert!(result < account_portion);
236+
237+
// Assert result is in a reasonable throttling range
238+
assert!(result > 0 && result <= ideal_allowance);
221239
}
222240

223241
// === Cooldown allowance ===
@@ -394,8 +412,8 @@ mod test {
394412
prop_assert_eq!(float_math_result, 0);
395413
} else if would_overflow {
396414
// In the u128 overflow region, the `f64` implementation is guaranteed to be imprecise.
397-
// We only assert that our implementation correctly falls back to account_portion.
398-
prop_assert_eq!(integer_math_result, account_portion);
415+
// We only assert that the result does not exceed the account's balance
416+
prop_assert!(integer_math_result <= account_portion);
399417
} else {
400418
prop_assert!(integer_math_result <= account_portion);
401419
prop_assert!(float_math_result <= account_portion);

0 commit comments

Comments
 (0)