diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index bf7ea0951b..89b4e1dede 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -302,9 +302,9 @@ public: doRound(rep& drops, std::string location); private: - template + template void - pushOverflow(T const& mantissa); + pushOverflow(T mantissa); enum class Round { // The result is exact. No rounding is needed. Only used if cuspRoundingFix is Enabled330 or @@ -410,20 +410,54 @@ Number::Guard::doDropDigit(uint128_t& mantissa, int& exponent) noexce ++exponent; } -template +template void -Number::Guard::pushOverflow(T const& mantissa) +Number::Guard::pushOverflow(T mantissa) { XRPL_ASSERT(mantissa <= kMaxRepUp, "xrpl::Number::Guard::pushOverflow : valid mantissa"); - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa > kMaxRep && + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa >= kMaxRep && mantissa < kMaxRepUp) { - // Special case rounding rules for the values between kMaxRep and kMaxRepUp. - // Scale the spread between kMaxRep and kMaxRepUp from 1 to 9, and push it onto the guard as - // if it was a digit that got removed, but don't remove it. This method is future-proof in - // case the number of mantissa bits ever changes. Effects: + // Special case rounding rules for the values in the range [kMaxRep, kMaxRepUp). + + auto constexpr spread = kMaxRepUp - kMaxRep; + static_assert(spread == 3); + + // Round in two steps. + + // The first step uses the digits _already_ in the Guard to round the + // intermediate mantissa, using only the last digit. Then update the mantissa for the + // second step. Ultimately, the purpose of this step is to capture rounding where the stored + // digits would change the decision without those digits. (e.g. From just _below_ the + // midpoint to just _above_ the midpoint for ToNearest, or from kMaxRep into the in-between + // for Upward. + // Make an exception if the final digit is 9, because it can only get larger, and we want to + // stay in single digits. + if (auto finalDigit = mantissa % 10; finalDigit < 9) + { + // Intentionally use integer math to get the largest value under the midpoint. + auto constexpr kMidpoint = kMaxRep + (spread / 2); + static_assert(kMidpoint == kMaxRep + 1); + auto const r = round(); + if (r == Round::Up || (r == Round::Even && mantissa == kMidpoint)) + { + ++mantissa; + } + } + + if (mantissa == kMaxRep) + { + // If the mantissa ends up exactly kMaxRep, there's nothing more to do. + return; + } + + // The second step scales the final digit of the update mantissa proportionally from kMaxRep + // and kMaxRepUp to 1 to 9. It then pushes that scaled digit onto the guard as if it was a + // digit that got removed, but don't actually remove it. This method should be is + // future-proof in case the number of mantissa bits ever changes. (Though for integer values + // that are a power of two themselves, the spread will always be the same.) Effects: // * For round to nearest - // * if the mantissa is below the midpoint, it'll round "down" to kMaxRep + // * if the updated mantissa is below the midpoint, it'll round "down" to kMaxRep // * if above the midpoint, it'll round "up" to kMaxRepUp // * it can never be exactly at the midpoint, because kMaxRepUp is always even, and // kMaxRep is always odd, so don't worry about that case. @@ -431,16 +465,12 @@ Number::Guard::pushOverflow(T const& mantissa) // negative. // * For round downward, does the opposite of upward. // * For round toward zero, always rounds down to kMaxRep. - auto constexpr spread = kMaxRepUp - kMaxRep; - static_assert(spread < 10 && spread > 0); - // This should absolutely be impossible - if constexpr (spread == 0) - return; // LCOV_EXCL_LINE auto const diff = mantissa - kMaxRep; - auto const digit = (diff * 10) / spread; + auto digit = (diff * 10) / spread; XRPL_ASSERT( - digit > 0 && digit < 10, "xrpl::Number::Guard::pushOverflow : valid overflow digit"); + 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. push(digit); @@ -542,18 +572,19 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string } else { - // Incrementing the mantissa will require dividing, which will require rounding. So - // _don't_ increment the mantissa. Instead, divide and round recursively. It should - // be impossible to recurse more than once, because once the mantissa is divided by - // 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1 will have no - // chance of bringing it back over. if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa > kMaxRep && mantissa < kMaxRepUp) { + // mantissa = kMaxRepUp; } else { + // Incrementing the mantissa will require dividing, which will require rounding. + // So _don't_ increment the mantissa. Instead, divide and round recursively. It + // should be impossible to recurse more than once, because once the mantissa is + // divided by 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1 + // will have no chance of bringing it back over. doDropDigit(mantissa, exponent); XRPL_ASSERT_PARTS( safeToIncrement(mantissa), @@ -630,8 +661,6 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) void Number::Guard::doRound(rep& drops, std::string location) { - pushOverflow(drops); - auto r = round(); if (r == Round::Up || (r == Round::Even && (drops & 1) == 1)) { @@ -648,14 +677,8 @@ Number::Guard::doRound(rep& drops, std::string location) } ++drops; } - else if ( - cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && drops > kMaxRep && - drops < kMaxRepUp) - { - // This will probably be impossible because this function is not called by mutating - // functions, so the Number will already be normalized. - drops = kMaxRep; - } + XRPL_ASSERT(drops >= 0, "xrpl::Number::Guard::doRound : positive magnitude"); + if (isNegative()) drops = -drops; } diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 311270a8e6..e9b03574c4 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -301,12 +301,15 @@ public: auto const cLargeLegacy = std::to_array({ {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep / 10, 1}, __LINE__}, }); - auto const cLargeCorrected = std::to_array({ + auto const cLarge320 = std::to_array({ {Number{Number::kMaxRep}, Number{6, -1}, Number{(Number::kMaxRep / 10) + 1, 1}, __LINE__}, }); + auto const cLargeCorrected = std::to_array({ + {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep}, __LINE__}, + }); auto test = [this](auto const& c) { for (auto const& [x, y, z, line] : c) { @@ -327,6 +330,10 @@ public: { test(cLargeLegacy); } + else if (scale == MantissaRange::MantissaScale::Large320) + { + test(cLarge320); + } else { test(cLargeCorrected); @@ -2555,6 +2562,170 @@ public: } } + void + testNumberRoundCuspWithFractionalParts() + { + auto const scale = Number::getMantissaScale(); + + testcase << "normalization cusp: rounding behavior with fractional parts " + << to_string(scale); + NumberRoundModeGuard const roundGuard{Number::RoundingMode::ToNearest}; + + Number const below{static_cast(Number::kMaxRep), 0}; + Number const above{false, Number::kMaxRepUp, 0, Number::Normalized{}}; + + log << "Below: " << below << ", Above: " << above << std::endl; + + auto const zeroPointFour = Number(4, -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 twoPointSix = Number(26, -1); + + auto const operands = std::to_array({ + zeroPointFour, + zeroPointSix, + onePointFour, + onePointFive, + onePointSix, + twoPointFour, + twoPointSix, + }); + + auto const modes = std::to_array({ + Number::RoundingMode::ToNearest, + Number::RoundingMode::TowardsZero, + Number::RoundingMode::Downward, + Number::RoundingMode::Upward, + }); + + // Addition cases test kMaxRep + Operand + for (auto const& mode : modes) + { + for (auto const& operand : operands) + { + NumberRoundModeGuard const rg{mode}; + + auto const expectedValue = [&]() { + if (scale >= MantissaRange::MantissaScale::Large330) + { + if (mode == Number::RoundingMode::ToNearest && operand < onePointFive) + return below; + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + return below; + } + if (scale == MantissaRange::MantissaScale::Large320) + { + if (mode == Number::RoundingMode::ToNearest) + { + if (operand < zeroPointSix) + return below; + } + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + { + if (operand >= onePointFour) + return below - 7; + return below; + } + } + if (scale == MantissaRange::MantissaScale::LargeLegacy) + { + if (mode == Number::RoundingMode::ToNearest) + { + if (operand < zeroPointSix) + return below; + if (operand == zeroPointSix) + return below - 7; + } + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + { + if (operand >= onePointFour) + return below - 7; + return below; + } + if (mode == Number::RoundingMode::Upward && operand <= zeroPointSix) + return below - 7; + } + if (scale == MantissaRange::MantissaScale::Small && + mode == Number::RoundingMode::Upward) + return above + 1000; + return above; + }(); + + Number const actual = below + operand; + + std::stringstream ss; + ss << "kMaxRep + " << operand << " rounded " << to_string(mode) << " to " << actual + << ". Expected: " << expectedValue; + if (BEAST_EXPECTS(actual == expectedValue, ss.str())) + log << "\tSUCCESS: " << to_string(scale) << " " << ss.str() << std::endl; + } + log << std::endl; + } + + // Subtraction cases test kMaxRepUp - Operand + for (auto const& mode : modes) + { + for (auto const& operand : operands) + { + NumberRoundModeGuard const rg{mode}; + + auto const expectedValue = [&]() { + if (scale >= MantissaRange::MantissaScale::Large330) + { + if (mode == Number::RoundingMode::ToNearest && operand > onePointFive) + return below; + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + return below; + } + if (scale == MantissaRange::MantissaScale::LargeLegacy || + scale == MantissaRange::MantissaScale::Large320) + { + if (mode == Number::RoundingMode::ToNearest) + { + if (operand >= twoPointSix) + return below; + } + if (mode == Number::RoundingMode::TowardsZero) + { + if (operand >= onePointFour) + return below - 7; + } + if (mode == Number::RoundingMode::Downward) + { + if (operand <= onePointSix) + return below - 7; + return below; + } + } + if (scale == MantissaRange::MantissaScale::Small) + { + if (mode == Number::RoundingMode::Downward) + return below - 1000; + if (mode == Number::RoundingMode::Upward) + return below; + } + return above; + }(); + + Number const actual = above - operand; + + std::stringstream ss; + ss << "kMaxRepUp - " << operand << " rounded " << to_string(mode) << " to " + << actual << ". Expected: " << expectedValue; + if (BEAST_EXPECTS(actual == expectedValue, ss.str())) + log << "\tSUCCESS: " << to_string(scale) << " " << ss.str() << std::endl; + } + log << std::endl; + } + } + void run() override { @@ -2587,6 +2758,7 @@ public: testEdgeCases(); testNumberAddDirectedSignWrong(); testNumberAddToNearestPicksFarther(); + testNumberRoundCuspWithFractionalParts(); } } };