← Watch feed
Merge rust-bitcoin/rust-bitcoin#6922: Use `try_fold` instead of `fold` in `Sum` impl
Public commit record
100/100 · Strong
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6922: Use `try_fold` instead of `fold` in `Sum` impl
126de69daec3ad3d5564c7e6444fb482b9594b3b Use `try_fold` instead of `fold` in `Sum` impl (Martin Habovstiak)
1665f9592766177094434209eb23e7dca45789ef Remove over-restrictive bounds from `NumOpResult` (Martin Habovstiak)
Pull request description:
The implementation of `Sum` for the amount types used the `fold` method
on iterator which would uselessly process more amounts even if the
overflow error was already detected and would keep re-checking the tag
set in previous iteration of the loop wasting even more performance.
This commit modifies it to use `try_fold` instead by converting it to
`Result` and back which makes the code much simpler and more performant.
This also adds a new method `from_result` to `NumOpResult` which is a
natural inverse of `into_result` and documents that the number of items
consumed is unspecified in case of overflows - this allows us to
optimize the performance in the future by e.g. using SIMD.
The PR also contains a small API improvement that I noticed.
I plan to rebase my SIMD PR on top of this, opening this separately because I believe this should be easier to review and uncontroversial.
ACKs for top commit:
apoelstra:
ACK 126de69daec3ad3d5564c7e6444fb482b9594b3b; successfully ran local tests
tcharding:
ACK 126de69daec3ad3d5564c7e6444fb482b9594b3b
Tree-SHA512: 98b6338b0a50edac2279f9057b698f6c25193afb8804f336f7bafabaf14fc61810adb19cc7048ef430b85838118c2f6342ccb734f0e088447f802cc815cb904e
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This is a code-quality and performance improvement, not a security fix. It changes how the library adds up lists of Bitcoin amounts so that it stops early once an overflow is detected, rather than continuing to process the rest of the list. It also documents that if an overflow happens, the code may not look at every item. The change does not alter the final result returned to callers and does not introduce a known vulnerability.
Recommended action
No security action required. Treat as a normal performance and API cleanup patch. Reviewers may want to confirm that the short-circuiting change is acceptable for any callers that previously relied on the entire iterator being consumed even after overflow, though the commit explicitly documents this is now unspecified.
Security signals we found
01
No security-relevant signal in commit message or diff
02
Refactor preserves overflow-checking behavior (short-circuits instead of continuing)
03
New API method `NumOpResult::from_result` is a pure inverse of existing `into_result`
04
Behavioral note added: iterator consumption count on overflow is now unspecified
Evidence from the diff
The commit refactors the Sum implementations for NumOpResult<Amount> and NumOpResult<SignedAmount> to use Iterator::try_fold instead of Iterator::fold. Previously, fold would continue iterating after an overflow error was already recorded, repeatedly matching on the error variant. The new code short-circuits on the first overflow by converting each addition’s NumOpResult to a Result and back via a new from_result helper. A second change removes an unnecessary T: fmt::Debug bound from several NumOpResult methods and moves unwrap_err into a separate impl block that still requires Debug. The PR description explicitly frames this as a performance/cleanup change and notes that the number of iterator items consumed on overflow is now unspecified, enabling future SIMD optimization.
Changed components
units/src/amount/ops.rsunits/src/result.rscore::iter::Sum implementations for NumOpResult<Amount> and NumOpResult<SignedAmount>NumOpResult<T> APIInspect captured patch +60 / −56
### units/src/amount/ops.rs
@@ -288,95 +288,90 @@ impl ops::Neg for NumOpResult<SignedAmount> {
fn neg(self) -> Self::Output { self.map(ops::Neg::neg) }
}
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl core::iter::Sum<Amount> for NumOpResult<Amount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = Amount>,
{
- iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match acc {
- Self::Valid(lhs) => lhs + amount,
- Self::Error(e) => Self::Error(e),
- })
+ let result = iter.try_fold(Amount::ZERO, |acc, amount| (acc + amount).into_result());
+ Self::from_result(result)
}
}
+
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl<'a> core::iter::Sum<&'a Amount> for NumOpResult<Amount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = &'a Amount>,
{
- iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match acc {
- Self::Valid(lhs) => lhs + amount,
- Self::Error(e) => Self::Error(e),
- })
+ let result = iter.try_fold(Amount::ZERO, |acc, amount| (acc + amount).into_result());
+ Self::from_result(result)
}
}
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl core::iter::Sum<Self> for NumOpResult<Amount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = Self>,
{
- iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match (acc, amount) {
- (Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
- })
+ let result = iter.try_fold(Amount::ZERO, |acc, amount| (acc + amount.into_result()?).into_result());
+ Self::from_result(result)
}
}
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl<'a> core::iter::Sum<&'a Self> for NumOpResult<Amount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = &'a Self>,
{
- iter.fold(Self::Valid(Amount::ZERO), |acc, amount| match (acc, *amount) {
- (Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
- })
+ let result = iter.try_fold(Amount::ZERO, |acc, amount| (acc + amount.into_result()?).into_result());
+ Self::from_result(result)
}
}
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl core::iter::Sum<SignedAmount> for NumOpResult<SignedAmount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = SignedAmount>,
{
- iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match acc {
- Self::Valid(lhs) => lhs + amount,
- Self::Error(e) => Self::Error(e),
- })
+ let result = iter.try_fold(SignedAmount::ZERO, |acc, amount| (acc + amount).into_result());
+ Self::from_result(result)
}
}
+
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl<'a> core::iter::Sum<&'a SignedAmount> for NumOpResult<SignedAmount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = &'a SignedAmount>,
{
- iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match acc {
- Self::Valid(lhs) => lhs + amount,
- Self::Error(e) => Self::Error(e),
- })
+ let result = iter.try_fold(SignedAmount::ZERO, |acc, amount| (acc + amount).into_result());
+ Self::from_result(result)
}
}
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl core::iter::Sum<Self> for NumOpResult<SignedAmount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = Self>,
{
- iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match (acc, amount) {
- (Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
- })
+ let result = iter.try_fold(SignedAmount::ZERO, |acc, amount| (acc + amount.into_result()?).into_result());
+ Self::from_result(result)
}
}
+
+/// Note: it's unspecified how many items from the iterator this implementation consumes if overflow occurs.
impl<'a> core::iter::Sum<&'a Self> for NumOpResult<SignedAmount> {
- fn sum<I>(iter: I) -> Self
+ fn sum<I>(mut iter: I) -> Self
where
I: Iterator<Item = &'a Self>,
{
- iter.fold(Self::Valid(SignedAmount::ZERO), |acc, amount| match (acc, *amount) {
- (Self::Valid(lhs), Self::Valid(rhs)) => lhs + rhs,
- (Self::Error(e), _) | (_, Self::Error(e)) => Self::Error(e),
- })
+ let result = iter.try_fold(SignedAmount::ZERO, |acc, amount| (acc + amount.into_result()?).into_result());
+ Self::from_result(result)
}
}
#[cfg(test)]
### units/src/result.rs
@@ -109,9 +109,7 @@ impl<T> NumOpResult<T> {
Self::Error(e) => NumOpResult::Error(e),
}
}
-}
-impl<T: fmt::Debug> NumOpResult<T> {
/// Returns the contained valid numeric type, consuming `self`.
///
/// # Panics
@@ -144,20 +142,6 @@ impl<T: fmt::Debug> NumOpResult<T> {
}
}
- /// Returns the contained error, consuming `self`.
- ///
- /// # Panics
- ///
- /// Panics if the numeric result is valid.
- #[inline]
- #[track_caller]
- pub fn unwrap_err(self) -> NumOpError {
- match self {
- Self::Error(e) => e,
- Self::Valid(a) => panic!("tried to unwrap a valid numeric result: {:?}", a),
- }
- }
-
/// Returns the contained [`Valid`] value or a provided default.
///
/// Arguments passed to [`unwrap_or`] are eagerly evaluated; if you are passing the result of a
@@ -207,6 +191,15 @@ impl<T: fmt::Debug> NumOpResult<T> {
}
}
+ /// Converts a [`Result<T, NumOpError>`] to a [`NumOpResult`].
+ #[inline]
+ pub fn from_result(result: Result<T, NumOpError>) -> Self {
+ match result {
+ Ok(x) => Self::Valid(x),
+ Err(e) => Self::Error(e),
+ }
+ }
+
/// Calls `op` if the numeric result is [`Valid`], otherwise returns the [`Error`] value of
/// `self`.
///
@@ -237,6 +230,22 @@ impl<T: fmt::Debug> NumOpResult<T> {
pub fn is_error(&self) -> bool { !self.is_valid() }
}
+impl<T: fmt::Debug> NumOpResult<T> {
+ /// Returns the contained error, consuming `self`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the numeric result is valid.
+ #[inline]
+ #[track_caller]
+ pub fn unwrap_err(self) -> NumOpError {
+ match self {
+ Self::Error(e) => e,
+ Self::Valid(a) => panic!("tried to unwrap a valid numeric result: {:?}", a),
+ }
+ }
+}
+
// Implement Add/Sub on NumOpResults for all wrapped types that already implement Add/Sub on themselves
crate::internal_macros::impl_op_for_references! {
impl<T> ops::Add<NumOpResult<T>> for NumOpResult<T>Risk score
Our methodology →Why this scored 19/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.