From 4d19c260807d57c9dbcdbaa7d6b652e2469a8c82 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 10 Jul 2026 14:14:35 -0400 Subject: [PATCH] Remove premature, incorrect optimization - pushOverflow had a shortcut that if the value rounded down to kMaxRep, then the work was done, but neglected to consider what would happen back in doRoundUp if round() returned Even. Even rounded up on 1.5 in pushOverflow, but on 0.5 in doRoundUp, so you'd get an incorrect result. - Updated tests to reflect the behavior for older modes.behavior for older modes.behavior for older modes.behavior for older modes. --- src/libxrpl/basics/Number.cpp | 8 +------- src/tests/libxrpl/basics/Number.cpp | 14 ++++++++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index e9b03b5ea2..91fe41d2f2 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -431,12 +431,6 @@ Number::Guard::pushOverflow(T mantissa) } } - if (mantissa == kMaxRep) - { - // If the mantissa ends up exactly kMaxRep, there's nothing more to do here. - return; - } - // The second step scales the final digit of the updated mantissa proportionally, converting // from (kMaxRep, kMaxRepUp) to (0 to 9]. It then pushes that scaled digit onto the guard as // if it was a digit that got removed, but doesn't actually remove it. This method should be @@ -455,7 +449,7 @@ Number::Guard::pushOverflow(T mantissa) auto const diff = mantissa - kMaxRep; auto const digit = (diff * 10) / spread; XRPL_ASSERT( - digit > 0 && digit < 10 && digit != 5, + digit >= 0 && digit < 10 && digit != 5, "xrpl::Number::Guard::pushOverflow : valid overflow digit"); // Don't remove the digit from the mantissa, but add it to the guard as if it was. diff --git a/src/tests/libxrpl/basics/Number.cpp b/src/tests/libxrpl/basics/Number.cpp index 0e22e9b8a7..36e1b4a700 100644 --- a/src/tests/libxrpl/basics/Number.cpp +++ b/src/tests/libxrpl/basics/Number.cpp @@ -2730,25 +2730,30 @@ TEST(NumberTest, number_cusp_rounding_with_fractional_parts) auto header = [&] { std::ostringstream log; - log << "Below: " << below << ", Above: " << above << "\n"; + log << "Scale: " << to_string(mantissaScale) << ", Below: " << below + << ", Above: " << above << "\n"; return log.str(); }; auto const zeroPointFour = Number(4, -1); + auto const zeroPointFive = Number(5, -1); auto const zeroPointSix = Number(6, -1); auto const onePointFour = Number(14, -1); auto const onePointFive = Number(15, -1); auto const onePointSix = Number(16, -1); auto const twoPointFour = Number(24, -1); + auto const twoPointFive = Number(25, -1); auto const twoPointSix = Number(26, -1); auto const operands = std::to_array({ zeroPointFour, + zeroPointFive, zeroPointSix, onePointFour, onePointFive, onePointSix, twoPointFour, + twoPointFive, twoPointSix, }); @@ -2767,6 +2772,7 @@ TEST(NumberTest, number_cusp_rounding_with_fractional_parts) NumberRoundModeGuard const rg{mode}; auto const expectedValue = [&]() { + // Returns "above" by default. The checks here are for exceptions. if (scale >= MantissaRange::MantissaScale::Large330) { if (mode == Number::RoundingMode::ToNearest && operand < onePointFive) @@ -2779,7 +2785,7 @@ TEST(NumberTest, number_cusp_rounding_with_fractional_parts) { if (mode == Number::RoundingMode::ToNearest) { - if (operand < zeroPointSix) + if (operand < zeroPointFive) return below; } if (mode == Number::RoundingMode::TowardsZero || @@ -2794,9 +2800,9 @@ TEST(NumberTest, number_cusp_rounding_with_fractional_parts) { if (mode == Number::RoundingMode::ToNearest) { - if (operand < zeroPointSix) + if (operand < zeroPointFive) return below; - if (operand == zeroPointSix) + if (operand <= zeroPointSix) return below - 7; } if (mode == Number::RoundingMode::TowardsZero ||