fix: Improve upward rounding edge cases for Number::operator/= - #7328
Merged
Conversation
- Add helper function, doDropDigit, to wrap the common pattern:
push(mantissa % 10);
mantissa /= 10;
++exponent;
- Might have been helpful to catch this issue when developing.
- Refactor the Guard decision in withTxnType into createGuards, which lives in Rules.cpp. It is physically located near setCurrentTransactionRules, and documented to explain that changes need to be synchronized.
- Some EscrowToken tests used a hard-coded list of amendments to determine whether to expect large mantissa logic. That ignored the effects of fixCleanup3_2_0, especially as applied to the previous fix affecting preflight, preclaim, etc. - Add a helper function, useRulesGuards, which will return the same decision as createGuards and setCurrentRulesImpl. Use that helper function in the relevant tests. - Also remove an #include that clang-tidy was complaining about.
…maxrepcusp * XRPLF/develop: release: Set version to 3.3.0-b0 (7280) refactor: Rename static constants (7120) refactor: Use `isFlag` where possible instead of bitwise math (7278) ci: Update XRPLF/actions (7281)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Tapanito
reviewed
May 28, 2026
- Add missing headers. - Improve code coverage exclusions. - Clean up several variable names. - Improve explanatory comments. - Remove the switch statement from MantissaRange::getMin. Change it to a straight power of ten lookup.
- Demonstrates the incorrect "before" behavior
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- Downward with a negative result, and ToNearest with a remainder slightly larger than 0.5.
Collaborator
|
@ximinez Ready to merge? |
ximinez
added
the
Ready to merge
bthomee
reviewed
May 31, 2026
Comment on lines
+625
to
+626
| // Not used by every compiler version, and thus not necessarily | ||
| // counted by coverage build |
Collaborator
There was a problem hiding this comment.
FYI: here and elsewhere, please consider writing new/updated comments such that they span as close as possible to the 100 chars we decided our line lengths would be.
Kassaking7
pushed a commit
to Kassaking7/rippled
that referenced
this pull request
Jun 2, 2026
…#7328) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
Prevents extreme dust rounding from getting lost, especially when rounding away from zero. (Upward for positive, downward for negative.)
Note: This commit was originally pushed to #7051, and resulted in some significant feedback. That feedback has not been addressed, but will be ASAP.
Also Note: I'd like to try to get this into 3.2 if only to avoid needing to create another amendment gate the change. However, 3.3 is fine if necessary.
Context of Change
If there is still a remainder after the
correctionFactorused inNumberdivision is processed, pass a flag todoNormalizewhich will set the "x-bit", indicating that non-zero digits have been dropped off the end of the number, which will round the unsigned mantissa up if:UpwardDownward0.5+epslionscenario, so the nearest value will be the next one up.The remainder is always rounded away, since large-mantissa
Numberis limited to 19 significant digits (~2^64), but in extreme cases, the rounding can go in the wrong direction.Also refactors the computation so that the factors used are consistent across the various mantissa scales, but the math is otherwise exactly the same.
Before / After
Example from the unit test:
Note that for this test,
Numberis rounding upward. The first 19-digits of the result are1999999999999999986. The remaining digits are000000000000000098.Before:
The correction computation only finds
000which didn't meaningfully modify the original result, such that even though rounded "up", the resulting mantissa remained1999999999999999986.After:
The correction computation finds still finds
000, but determines there is still some remainder left after accounting for that. Thus after rounding upward the resulting mantissa is the correct1999999999999999987value.