fix: Handle rounding just above kMaxRep more accurately#7389
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}
| auto constexpr spread = kMaxRepUp - kMaxRep; | ||
| static_assert(spread < 10 && spread > 0); | ||
| // This should absolutely be impossible | ||
| if constexpr (spread == 0) |
There was a problem hiding this comment.
I am a bit uncertain why this is needed. Both the static_assert and if constexpr would be evaluated at compile time. Am I overlooking a runtime path that could get here?
There was a problem hiding this comment.
I am a bit uncertain why this is needed. Both the
static_assertandif constexprwould be evaluated at compile time. Am I overlooking a runtime path that could get here?
I think I was just being overly paranoid. I'll remove the extra if constexpr.
gregtatcam
left a comment
gregtatcam
left a comment
There was a problem hiding this comment.
pushOverflow() handles the simple integer cases, but it loses information when there are already guard digits. For example, kMaxRep + 1.6 should round up because it is closer to kMaxRepUp than to kMaxRep. Today the code sees the integer part, kMaxRep + 1, turns that into a fake guard digit 3, and pushes it in front of the real guard digit 6. That makes the value look like .36 of the way through the gap instead of 1.6 / 3, so ToNearest rounds down incorrectly. I think the fix is to make the cusp rounding decision use the integer offset and the existing guard digits together, instead of adding a fake digit on top of the current guard state. Here is the unit test, which demonstrates the issue:
NumberRoundModeGuard const roundGuard{Number::RoundingMode::ToNearest};
Number const below{static_cast<std::int64_t>(Number::kMaxRep), 0};
Number const above{false, Number::kMaxRepUp, 0, Number::Normalized{}};
// Exact value is kMaxRep + 1.6. Since the gap from kMaxRep to kMaxRepUp is 3,
// this is closer to kMaxRepUp than kMaxRep.
Number const actual = below + Number{16, -1};
std::stringstream ss;
ss << "kMaxRep + 1.6 rounded to " << actual << ". Expected: " << above;
BEAST_EXPECTS(actual == above, ss.str()); // fails
Oh, daaaaaaannnnnngggggg... Good catch. |
… into ximinez/number-round-maxrep * XRPLF/ximinez/number-round-maxrep-down: clang-tidy: Remove unused header
- Remove unnecessary if constexpr check - Update scaling static_assert - Remove unnecessary rounding logic from Number::Guard::doRound() - Handle fractional rounding between kMaxRep and kMaxRepUp
- Show that old behavior is not affected, and that the new tests fail without them.
This reverts commit 8250aa2.
High Level Overview of Change
kMaxRep(2^63-1) andkMaxRepUp(((kMaxRep / 10) + 1) * 10, which is the next multiple of 10 abovekMaxRep) as ifthose values were sequential, and values in between were "fractional".
kMaxRepUp,and below the midpoint to
kMaxRepwhen rounding to nearest. Otherrounding modes act along the same lines.
Context of Change