Skip to content

Add checked arithmetic methods to U256 and I256#1665

Closed
leighmcculloch wants to merge 1 commit intomainfrom
256checked
Closed

Add checked arithmetic methods to U256 and I256#1665
leighmcculloch wants to merge 1 commit intomainfrom
256checked

Conversation

@leighmcculloch
Copy link
Copy Markdown
Member

What

Add checked variants of arithmetic operations (add, sub, mul, div, rem_euclid, pow, shl) to U256 and I256 types. Add helper methods zero(), min_value(), and max_value() to both types.

Why

Developers building math libraries (e.g., fixed-point arithmetic, WAD equivalents) need checked variants to handle overflow/underflow gracefully with Option returns instead of panics. The underlying host functions trap on errors rather than returning them, so checked operations are implemented in the SDK guest side until such time as the host supports them.

Close #1659

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds checked arithmetic operations to the U256 and I256 types to enable developers building math libraries to handle overflow and underflow gracefully with Option returns instead of panics. It also adds helper methods (zero, min_value, max_value) for creating common constant values. The implementation is done guest-side in the SDK since the underlying host functions trap on errors rather than returning them.

Key changes:

  • Added checked variants of arithmetic operations (add, sub, mul, div, rem_euclid, pow, shl) for both U256 and I256
  • Added helper methods (zero, min_value, max_value) for both types
  • Comprehensive test coverage for all new operations

Comment thread soroban-sdk/src/num.rs
Comment on lines +724 to +740
// For negative result, the limit is abs(min) = max + 1, but we can only represent max
// So we check: self_abs * other_abs <= max (for positive result)
// Or: self_abs * other_abs <= max + 1 (for negative result), which we approximate as <= max
// since if self_abs * other_abs == max + 1, result is exactly MIN which is valid

// Overflow if self_abs > max / other_abs
let max_self = max.div(&other_abs);
if self_abs <= max_self {
Some(self.mul(other))
} else if result_neg && self_abs == max_self.add(&one) {
// Special case: result is exactly MIN
let result = self.mul(other);
if result == min {
Some(result)
} else {
None
}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The special case handling for multiplication resulting in exactly MIN is flawed. The code calls self.mul(other) which will trap on overflow (as noted in the PR description, host functions trap on errors). Even though the mathematical result would be exactly MIN (a representable value), the host's multiplication will detect this as overflow and trap before returning.

This means the special case will never successfully return Some(min) - it will always trap instead. Consider removing this special case branch and returning None, since multiplication that would result in MIN is treated as overflow by the underlying host function.

Suggested change
// For negative result, the limit is abs(min) = max + 1, but we can only represent max
// So we check: self_abs * other_abs <= max (for positive result)
// Or: self_abs * other_abs <= max + 1 (for negative result), which we approximate as <= max
// since if self_abs * other_abs == max + 1, result is exactly MIN which is valid
// Overflow if self_abs > max / other_abs
let max_self = max.div(&other_abs);
if self_abs <= max_self {
Some(self.mul(other))
} else if result_neg && self_abs == max_self.add(&one) {
// Special case: result is exactly MIN
let result = self.mul(other);
if result == min {
Some(result)
} else {
None
}
// For negative result, the limit is abs(min) = max + 1, but we can only represent max.
// We conservatively treat any case where self_abs * other_abs would exceed max as overflow,
// including products that would mathematically be exactly MIN, because the underlying host
// multiplication treats those as overflow and will trap instead of returning a value.
// Overflow if self_abs > max / other_abs
let max_self = max.div(&other_abs);
if self_abs <= max_self {
Some(self.mul(other))

Copilot uses AI. Check for mistakes.
Comment thread soroban-sdk/src/num.rs
Comment on lines +434 to +440
// Check for overflow: value must be recoverable by right shift
let shifted = self.shl(bits);
if shifted.shr(bits) == *self {
Some(shifted)
} else {
None
}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

This checked shift left implementation has a critical flaw: it calls self.shl(bits) unconditionally, which will trap if the shift overflows (per the PR description, host functions trap on errors). The overflow check on the next line will never execute in overflow cases because the trap occurs first.

To fix this, implement the overflow check before performing the shift. For example, you could check if the high bits that would be shifted out are all zero, perhaps by checking if self >> (256 - bits) equals zero (being careful about the edge case where bits=0).

Suggested change
// Check for overflow: value must be recoverable by right shift
let shifted = self.shl(bits);
if shifted.shr(bits) == *self {
Some(shifted)
} else {
None
}
// Shifting by 0 is always safe and is a no-op.
if bits == 0 {
return Some(self.clone());
}
// For 1 <= bits < 256, check that no high bits would be shifted out.
// Overflow occurs if any of the upper `bits` bits are set, which is
// equivalent to `(self >> (256 - bits)) != 0`.
let shift_out = 256u32 - bits;
let high = self.shr(shift_out);
if high != U256::zero(self.env()) {
return None;
}
Some(self.shl(bits))

Copilot uses AI. Check for mistakes.
Comment thread soroban-sdk/src/num.rs
Comment on lines +828 to +834
// Check for overflow: value must be recoverable by right shift
let shifted = self.shl(bits);
if shifted.shr(bits) == *self {
Some(shifted)
} else {
None
}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

This checked shift left implementation has the same critical flaw as U256::checked_shl: it calls self.shl(bits) unconditionally, which will trap if the shift overflows. The overflow check that follows will never execute in overflow cases.

For signed integers, the overflow check is more complex since you need to preserve the sign bit. Consider checking if all bits that would be shifted out match the sign bit before performing the shift.

Suggested change
// Check for overflow: value must be recoverable by right shift
let shifted = self.shl(bits);
if shifted.shr(bits) == *self {
Some(shifted)
} else {
None
}
if bits == 0 {
return Some(self.clone());
}
// Implement shift-left as repeated doubling via checked addition to
// avoid invoking a potentially trapping `shl` operation.
let mut result = self.clone();
for _ in 0..bits {
result = result.checked_add(&result)?;
}
Some(result)

Copilot uses AI. Check for mistakes.
Comment thread soroban-sdk/src/num.rs
Comment on lines +766 to +773
/// returning `None` if `other == 0`.
pub fn checked_rem_euclid(&self, other: &I256) -> Option<I256> {
let zero = I256::zero(self.env());
if *other == zero {
None
} else {
Some(self.rem_euclid(other))
}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The checked_rem_euclid method only checks for division by zero but doesn't handle the overflow case of MIN % -1. While mathematically the remainder is 0, computing it may involve operations that overflow. Rust's standard library i32::checked_rem returns None for i32::MIN % -1 to be safe.

Consider adding the same check as in checked_div for the MIN/-1 case, returning None to avoid potential overflow in the underlying host operation.

Suggested change
/// returning `None` if `other == 0`.
pub fn checked_rem_euclid(&self, other: &I256) -> Option<I256> {
let zero = I256::zero(self.env());
if *other == zero {
None
} else {
Some(self.rem_euclid(other))
}
/// returning `None` if `other == 0` or if `self == MIN && other == -1`
/// (overflow case).
pub fn checked_rem_euclid(&self, other: &I256) -> Option<I256> {
let zero = I256::zero(self.env());
let neg_one = I256::from_i32(self.env(), -1);
let min = I256::min_value(self.env());
if *other == zero {
return None;
}
// Check for MIN % -1 overflow
if *self == min && *other == neg_one {
return None;
}
Some(self.rem_euclid(other))

Copilot uses AI. Check for mistakes.
@leighmcculloch leighmcculloch deleted the 256checked branch January 5, 2026 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

checked variants of I256 arithmetic functions

2 participants