From f255ef621479030de208008a3779ab5c36e43397 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 29 May 2026 19:03:34 -0400 Subject: [PATCH 01/23] test: Add another rounding unit test --- src/test/basics/Number_test.cpp | 41 +++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index f4bd1c9d66..aabe27f294 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1965,6 +1965,47 @@ public: break; } } + + { + testcase << "normalization cusp: ToNearest and Downward disagree" << to_string(scale); + + constexpr auto kMaxRep = Number::kMaxRep; + + // Both ToNearest and Downward should round to `below` + auto const actual = static_cast(kMaxRep) + 1; + Number const below{static_cast(kMaxRep), 0}; + Number const above{ + false, static_cast(kMaxRep) + 3, 0, Number::Unchecked{}}; + + Number toNearest; + { + NumberRoundModeGuard const roundGuard{Number::RoundingMode::ToNearest}; + toNearest = Number(false, actual, 0, Number::Normalized{}); + } + + Number downward; + { + NumberRoundModeGuard const roundGuard{Number::RoundingMode::Downward}; + downward = Number(false, actual, 0, Number::Normalized{}); + } + + log << "\n" + << " actual = " << actual << " (kMaxRep + 1)\n" + << " below = " << below << " (kMaxRep, distance 1)\n" + << " above = " << above << " (kMaxRep + 3, distance 2)\n" + << " ToNearest = " << toNearest << "\n" + << " Downward = " << downward << "\n\n"; + + // ToNearest rounds UP when the DOWN neighbor is strictly closer + BEAST_EXPECT(toNearest == above); + BEAST_EXPECT(toNearest != below); + + // Downward undershoots: it returns a value below `below` + BEAST_EXPECT(downward < below); + + // Both should have given the same answer, but they differ + BEAST_EXPECT(toNearest != downward); + } } void From b624b2eeaef1e0200d81b8d49d90a8d5b9794c5c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 29 May 2026 19:46:50 -0400 Subject: [PATCH 02/23] Include upward, write tests based on "expected" behavior --- src/test/basics/Number_test.cpp | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index aabe27f294..43c9daa42e 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1967,7 +1967,7 @@ public: } { - testcase << "normalization cusp: ToNearest and Downward disagree" << to_string(scale); + testcase << "normalization cusp: ToNearest and Downward disagree " << to_string(scale); constexpr auto kMaxRep = Number::kMaxRep; @@ -1977,34 +1977,37 @@ public: Number const above{ false, static_cast(kMaxRep) + 3, 0, Number::Unchecked{}}; - Number toNearest; - { - NumberRoundModeGuard const roundGuard{Number::RoundingMode::ToNearest}; - toNearest = Number(false, actual, 0, Number::Normalized{}); - } + auto construct = [](Number::RoundingMode mode) { + NumberRoundModeGuard const roundGuard{mode}; + return Number(false, actual, 0, Number::Normalized{}); + }; + Number const upward = construct(Number::RoundingMode::Upward); - Number downward; - { - NumberRoundModeGuard const roundGuard{Number::RoundingMode::Downward}; - downward = Number(false, actual, 0, Number::Normalized{}); - } + Number const toNearest = construct(Number::RoundingMode::ToNearest); + + Number const downward = construct(Number::RoundingMode::Downward); log << "\n" << " actual = " << actual << " (kMaxRep + 1)\n" << " below = " << below << " (kMaxRep, distance 1)\n" << " above = " << above << " (kMaxRep + 3, distance 2)\n" + << " Upward = " << upward << "\n" << " ToNearest = " << toNearest << "\n" << " Downward = " << downward << "\n\n"; + log.flush(); + + // Upward round UP + BEAST_EXPECT(upward == above); // ToNearest rounds UP when the DOWN neighbor is strictly closer - BEAST_EXPECT(toNearest == above); - BEAST_EXPECT(toNearest != below); + BEAST_EXPECT(toNearest != above); + BEAST_EXPECT(toNearest == below); // Downward undershoots: it returns a value below `below` - BEAST_EXPECT(downward < below); + BEAST_EXPECT(downward == below); // Both should have given the same answer, but they differ - BEAST_EXPECT(toNearest != downward); + BEAST_EXPECT(toNearest == downward); } } From 6d89fbef7a02b1f1daff3c122d97ba23678bded9 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 1 Jun 2026 21:48:57 -0400 Subject: [PATCH 03/23] Experimental: Scale addition operands up to preserve accuracy --- include/xrpl/basics/Number.h | 24 ++-- src/libxrpl/basics/Number.cpp | 79 +++++++++-- src/test/basics/Number_test.cpp | 242 +++++++++++++++++++++++--------- 3 files changed, 262 insertions(+), 83 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index cee0c45355..fae65ecb35 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -51,37 +51,43 @@ namespace detail { * compile time. Doing it at runtime would be pretty wasteful and * inefficient. */ -constexpr std::size_t kInt64Digits = 20; -consteval std::array +constexpr std::size_t kUint64Digits = 20; +constexpr std::size_t kUint128Digits = 39; + +template +consteval std::array buildPowersOfTen() { - std::array result{}; + std::array result{}; - std::uint64_t power = 1; + T power = 1; std::size_t exponent = 0; // end the loop early so it doesn't overflow; for (; exponent < result.size() - 1; ++exponent, power *= 10) { result[exponent] = power; - if (power > std::numeric_limits::max() / 10) + if (power > std::numeric_limits::max() / 10) throw std::logic_error("Power of 10 table is too big"); } result[exponent] = power; - if (power < std::numeric_limits::max() / 10) - throw std::logic_error("Power of 10 table is not big enough for the uint64_t type"); + if (power < std::numeric_limits::max() / 10) + throw std::logic_error("Power of 10 table is not big enough for the given type"); return result; } } // namespace detail -constexpr std::array kPowerOfTen = detail::buildPowersOfTen(); +template +constexpr std::array kPowerOfTenImpl = detail::buildPowersOfTen(); + +constexpr auto kPowerOfTen = kPowerOfTenImpl; static_assert(kPowerOfTen[0] == 1); static_assert(kPowerOfTen[1] == 10); static_assert(kPowerOfTen[10] == 10'000'000'000); static_assert( - isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kInt64Digits - 1); + isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kUint64Digits - 1); /** MantissaRange defines a range for the mantissa of a normalized Number. * diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 23e913bbdc..4b5fe7b728 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -720,26 +720,54 @@ Number::operator+=(Number const& y) uint128_t ym = y.mantissa_; auto ye = y.exponent_; Guard g; + + auto const& range = kRange.get(); + + // Bring the exponents of both values into agreement, so the mantissas are on the same scale + // and can be added directly together + // expandM / expandE: First try to expand the mantissa and bring the exponent down + // shringM / shrinkE: Then shrink the mantissa and bring the exponent up, if necessary + auto const adjust = [&g, &range]( + uint128_t& expandM, int& expandE, uint128_t& shrinkM, int& shrinkE) { + constexpr uint128_t kSafeLimit = kPowerOfTenImpl[37]; + + if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + { + while (shrinkE < expandE && shrinkM % 10 == 0) + { + g.doDropDigit(shrinkM, shrinkE); + } + + // We've got 128 bits of mantissa to work with here. Don't throw away data unless we + // have to + while (shrinkE < expandE && expandE > kMinExponent && expandM < kSafeLimit) + { + expandM *= 10; + --expandE; + } + } + + while (shrinkE < expandE) + { + g.doDropDigit(shrinkM, shrinkE); + } + }; + if (xe < ye) { if (xn) g.setNegative(); - do - { - g.doDropDigit(xm, xe); - } while (xe < ye); + + adjust(ym, ye, xm, xe); } else if (xe > ye) { if (yn) g.setNegative(); - do - { - g.doDropDigit(ym, ye); - } while (xe > ye); + + adjust(xm, xe, ym, ye); } - auto const& range = kRange.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; @@ -747,9 +775,19 @@ Number::operator+=(Number const& y) if (xn == yn) { xm += ym; - if (xm > maxMantissa || xm > kMaxRep) + if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) { - g.doDropDigit(xm, xe); + while (xm > maxMantissa || xm > kMaxRep) + { + g.doDropDigit(xm, xe); + } + } + else + { + if (xm > maxMantissa || xm > kMaxRep) + { + g.doDropDigit(xm, xe); + } } g.doRoundUp( xn, @@ -779,6 +817,25 @@ Number::operator+=(Number const& y) --xe; } g.doRoundDown(xn, xm, xe, minMantissa); + if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + { + // make a new guard + Guard g; + if (xn) + g.setNegative(); + while (xm > maxMantissa || xm > kMaxRep) + { + g.doDropDigit(xm, xe); + } + g.doRoundUp( + xn, + xm, + xe, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + "Number::addition overflow"); + } } negative_ = xn; diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 43c9daa42e..cf77bf3599 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -46,6 +46,15 @@ class Number_test : public beast::unit_test::Suite return out; } + static BigInt + toBigInt(Number const& n) + { + BigInt v = n.mantissa(); + for (int i = 0; i < n.exponent(); ++i) + v *= 10; + return v; + } + using dec = boost::multiprecision::cpp_dec_float_50; template @@ -172,28 +181,34 @@ public: auto const scale = Number::getMantissaScale(); testcase << "test_add " << to_string(scale); - using Case = std::tuple; + using Case = std::tuple; auto const cSmall = std::to_array( {{Number{1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, - Number{1'000'000'000'000'066, -15}}, + Number{1'000'000'000'000'066, -15}, + __LINE__}, {Number{-1'000'000'000'000'000, -15}, Number{-6'555'555'555'555'555, -29}, - Number{-1'000'000'000'000'066, -15}}, + Number{-1'000'000'000'000'066, -15}, + __LINE__}, {Number{-1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, - Number{-9'999'999'999'999'344, -16}}, + Number{-9'999'999'999'999'344, -16}, + __LINE__}, {Number{-6'555'555'555'555'555, -29}, Number{1'000'000'000'000'000, -15}, - Number{9'999'999'999'999'344, -16}}, - {Number{}, Number{5}, Number{5}}, - {Number{5}, Number{}, Number{5}}, + Number{9'999'999'999'999'344, -16}, + __LINE__}, + {Number{}, Number{5}, Number{5}, __LINE__}, + {Number{5}, Number{}, Number{5}, __LINE__}, {Number{5'555'555'555'555'555, -32768}, Number{-5'555'555'555'555'554, -32768}, - Number{0}}, + Number{0}, + __LINE__}, {Number{-9'999'999'999'999'999, -31}, Number{1'000'000'000'000'000, -15}, - Number{9'999'999'999'999'990, -16}}}); + Number{9'999'999'999'999'990, -16}, + __LINE__}}); auto const cLarge = std::to_array( // Note that items with extremely large mantissas need to be // calculated, because otherwise they overflow uint64. Items from C @@ -201,45 +216,57 @@ public: { {Number{1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, - Number{1'000'000'000'000'065'556, -18}}, + Number{1'000'000'000'000'065'556, -18}, + __LINE__}, {Number{-1'000'000'000'000'000, -15}, Number{-6'555'555'555'555'555, -29}, - Number{-1'000'000'000'000'065'556, -18}}, + Number{-1'000'000'000'000'065'556, -18}, + __LINE__}, {Number{-1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, - Number{true, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}}, + Number{true, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}, + __LINE__}, {Number{-6'555'555'555'555'555, -29}, Number{1'000'000'000'000'000, -15}, - Number{false, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}}, - {Number{}, Number{5}, Number{5}}, - {Number{5}, Number{}, Number{5}}, + Number{false, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}, + __LINE__}, + {Number{}, Number{5}, Number{5}, __LINE__}, + {Number{5}, Number{}, Number{5}, __LINE__}, {Number{5'555'555'555'555'555'000, -32768}, Number{-5'555'555'555'555'554'000, -32768}, - Number{0}}, + Number{0}, + __LINE__}, {Number{-9'999'999'999'999'999, -31}, Number{1'000'000'000'000'000, -15}, - Number{9'999'999'999'999'990, -16}}, + Number{9'999'999'999'999'990, -16}, + __LINE__}, // Items from cSmall expanded for the larger mantissa {Number{1'000'000'000'000'000'000, -18}, Number{6'555'555'555'555'555'555, -35}, - Number{1'000'000'000'000'000'066, -18}}, + Number{1'000'000'000'000'000'066, -18}, + __LINE__}, {Number{-1'000'000'000'000'000'000, -18}, Number{-6'555'555'555'555'555'555, -35}, - Number{-1'000'000'000'000'000'066, -18}}, + Number{-1'000'000'000'000'000'066, -18}, + __LINE__}, {Number{-1'000'000'000'000'000'000, -18}, Number{6'555'555'555'555'555'555, -35}, - Number{true, 9'999'999'999'999'999'344ULL, -19, Number::Normalized{}}}, + Number{true, 9'999'999'999'999'999'344ULL, -19, Number::Normalized{}}, + __LINE__}, {Number{-6'555'555'555'555'555'555, -35}, Number{1'000'000'000'000'000'000, -18}, - Number{false, 9'999'999'999'999'999'344ULL, -19, Number::Normalized{}}}, - {Number{}, Number{5}, Number{5}}, + Number{false, 9'999'999'999'999'999'344ULL, -19, Number::Normalized{}}, + __LINE__}, + {Number{}, Number{5}, Number{5}, __LINE__}, {Number{5'555'555'555'555'555'555, -32768}, Number{-5'555'555'555'555'555'554, -32768}, - Number{0}}, + Number{0}, + __LINE__}, {Number{true, 9'999'999'999'999'999'999ULL, -37, Number::Normalized{}}, Number{1'000'000'000'000'000'000, -18}, - Number{false, 9'999'999'999'999'999'990ULL, -19, Number::Normalized{}}}, - {Number{Number::kMaxRep - 1}, Number{1, 0}, Number{Number::kMaxRep}}, + Number{false, 9'999'999'999'999'999'990ULL, -19, Number::Normalized{}}, + __LINE__}, + {Number{Number::kMaxRep - 1}, Number{1, 0}, Number{Number::kMaxRep}, __LINE__}, // Test extremes { // Each Number operand rounds up, so the actual mantissa is @@ -247,6 +274,7 @@ public: Number{false, 9'999'999'999'999'999'999ULL, 0, Number::Normalized{}}, Number{false, 9'999'999'999'999'999'999ULL, 0, Number::Normalized{}}, Number{2, 19}, + __LINE__, }, { // Does not round. Mantissas are going to be > kMaxRep, so if @@ -257,21 +285,25 @@ public: Number{false, 9'999'999'999'999'999'990ULL, 0, Number::Normalized{}}, Number{false, 9'999'999'999'999'999'990ULL, 0, Number::Normalized{}}, Number{false, 1'999'999'999'999'999'998ULL, 1, Number::Normalized{}}, + __LINE__, }, }); auto const cLargeLegacy = std::to_array({ - {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep / 10, 1}}, + {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep / 10, 1}, __LINE__}, }); auto const cLargeCorrected = std::to_array({ - {Number{Number::kMaxRep}, Number{6, -1}, Number{(Number::kMaxRep / 10) + 1, 1}}, + {Number{Number::kMaxRep}, + Number{6, -1}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, }); auto test = [this](auto const& c) { - for (auto const& [x, y, z] : c) + for (auto const& [x, y, z, line] : c) { auto const result = x + y; std::stringstream ss; ss << x << " + " << y << " = " << result << ". Expected: " << z; - BEAST_EXPECTS(result == z, ss.str()); + expect(result == z, ss.str(), __FILE__, line); } }; if (scale == MantissaRange::MantissaScale::Small) @@ -311,21 +343,28 @@ public: auto const scale = Number::getMantissaScale(); testcase << "test_sub " << to_string(scale); - using Case = std::tuple; + using Case = std::tuple; auto const cSmall = std::to_array( {{Number{1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, - Number{9'999'999'999'999'344, -16}}, + Number{9'999'999'999'999'344, -16}, + __LINE__}, {Number{6'555'555'555'555'555, -29}, Number{1'000'000'000'000'000, -15}, - Number{-9'999'999'999'999'344, -16}}, - {Number{1'000'000'000'000'000, -15}, Number{1'000'000'000'000'000, -15}, Number{0}}, + Number{-9'999'999'999'999'344, -16}, + __LINE__}, + {Number{1'000'000'000'000'000, -15}, + Number{1'000'000'000'000'000, -15}, + Number{0}, + __LINE__}, {Number{1'000'000'000'000'000, -15}, Number{1'000'000'000'000'001, -15}, - Number{-1'000'000'000'000'000, -30}}, + Number{-1'000'000'000'000'000, -30}, + __LINE__}, {Number{1'000'000'000'000'001, -15}, Number{1'000'000'000'000'000, -15}, - Number{1'000'000'000'000'000, -30}}}); + Number{1'000'000'000'000'000, -30}, + __LINE__}}); auto const cLarge = std::to_array( // Note that items with extremely large mantissas need to be // calculated, because otherwise they overflow uint64. Items from C @@ -333,49 +372,63 @@ public: { {Number{1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, - Number{false, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}}, + Number{false, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}, + __LINE__}, {Number{6'555'555'555'555'555, -29}, Number{1'000'000'000'000'000, -15}, - Number{true, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}}, - {Number{1'000'000'000'000'000, -15}, Number{1'000'000'000'000'000, -15}, Number{0}}, + Number{true, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}, + __LINE__}, + {Number{1'000'000'000'000'000, -15}, + Number{1'000'000'000'000'000, -15}, + Number{0}, + __LINE__}, {Number{1'000'000'000'000'000, -15}, Number{1'000'000'000'000'001, -15}, - Number{-1'000'000'000'000'000, -30}}, + Number{-1'000'000'000'000'000, -30}, + __LINE__}, {Number{1'000'000'000'000'001, -15}, Number{1'000'000'000'000'000, -15}, - Number{1'000'000'000'000'000, -30}}, + Number{1'000'000'000'000'000, -30}, + __LINE__}, // Items from cSmall expanded for the larger mantissa {Number{1'000'000'000'000'000'000, -18}, Number{6'555'555'555'555'555'555, -32}, - Number{false, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}}, + Number{false, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}, + __LINE__}, {Number{6'555'555'555'555'555'555, -32}, Number{1'000'000'000'000'000'000, -18}, - Number{true, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}}, + Number{true, 9'999'999'999'999'344'444ULL, -19, Number::Normalized{}}, + __LINE__}, {Number{1'000'000'000'000'000'000, -18}, Number{1'000'000'000'000'000'000, -18}, - Number{0}}, + Number{0}, + __LINE__}, {Number{1'000'000'000'000'000'000, -18}, Number{1'000'000'000'000'000'001, -18}, - Number{-1'000'000'000'000'000'000, -36}}, + Number{-1'000'000'000'000'000'000, -36}, + __LINE__}, {Number{1'000'000'000'000'000'001, -18}, Number{1'000'000'000'000'000'000, -18}, - Number{1'000'000'000'000'000'000, -36}}, - {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep - 1}}, + Number{1'000'000'000'000'000'000, -36}, + __LINE__}, + {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep - 1}, __LINE__}, {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, Number{1, 0}, - Number{(Number::kMaxRep / 10) + 1, 1}}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, Number{3, 0}, - Number{Number::kMaxRep}}, - {power(2, 63), Number{3, 0}, Number{Number::kMaxRep}}, + Number{Number::kMaxRep}, + __LINE__}, + {power(2, 63), Number{3, 0}, Number{Number::kMaxRep}, __LINE__}, }); auto test = [this](auto const& c) { - for (auto const& [x, y, z] : c) + for (auto const& [x, y, z, line] : c) { auto const result = x - y; std::stringstream ss; ss << x << " - " << y << " = " << result << ". Expected: " << z; - BEAST_EXPECTS(result == z, ss.str()); + expect(result == z, ss.str(), __FILE__, line); } }; if (scale == MantissaRange::MantissaScale::Small) @@ -1740,9 +1793,7 @@ public: BigInt const exactProduct = BigInt(kAValue) * BigInt(kBValue); // What Number actually stored. - BigInt storedValue = BigInt(product.mantissa()); - for (int i = 0; i < product.exponent(); ++i) - storedValue *= 10; + BigInt storedValue = toBigInt(product); BigInt const signedDifference = storedValue - exactProduct; @@ -1996,18 +2047,83 @@ public: << " Downward = " << downward << "\n\n"; log.flush(); - // Upward round UP - BEAST_EXPECT(upward == above); + switch (scale) + { + case MantissaRange::MantissaScale::Small: + // With the small mantissa, everything rounds up - // ToNearest rounds UP when the DOWN neighbor is strictly closer - BEAST_EXPECT(toNearest != above); - BEAST_EXPECT(toNearest == below); + // Upward round UP + BEAST_EXPECT(upward > above); - // Downward undershoots: it returns a value below `below` - BEAST_EXPECT(downward == below); + // ToNearest rounds UP when the DOWN neighbor is strictly closer + BEAST_EXPECT(toNearest > above); + BEAST_EXPECT(toNearest == below); - // Both should have given the same answer, but they differ - BEAST_EXPECT(toNearest == downward); + // Downward undershoots: it returns a value below `below` + BEAST_EXPECT(downward < below); + + // Both should have given the same answer, but they differ + BEAST_EXPECT(toNearest > downward); + + break; + + case MantissaRange::MantissaScale::LargeLegacy: + // Upward round UP + BEAST_EXPECT(upward == above); + + // ToNearest rounds UP when the DOWN neighbor is strictly closer + BEAST_EXPECT(toNearest == above); + BEAST_EXPECT(toNearest > below); + + // Downward undershoots: it returns a value below `below` + BEAST_EXPECT(downward < below); + + // Both should have given the same answer, but they differ + BEAST_EXPECT(toNearest > downward); + + break; + default: + // Upward round UP + BEAST_EXPECT(upward == above); + + // ToNearest rounds UP when the DOWN neighbor is strictly closer + BEAST_EXPECT(toNearest != above); + BEAST_EXPECT(toNearest == below); + + // Downward undershoots: it returns a value below `below` + BEAST_EXPECT(downward == below); + + // Both should have given the same answer, but they differ + BEAST_EXPECT(toNearest == downward); + } + } + { + testcase << "operator+ TowardsZero rounds away from zero " << to_string(scale); + + Number const a{1LL, 20}; + Number const b{-1'000'000'000'000'000'001LL}; + + BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000000"}); + if (scale != MantissaRange::MantissaScale::Small) + BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000001"}); + else + BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000000"}); + + Number sum; + { + NumberRoundModeGuard const roundGuard{Number::RoundingMode::TowardsZero}; + sum = a + b; + } + + BigInt const exact = toBigInt(a) + toBigInt(b); + BigInt const stored = toBigInt(sum); + + log << "\n exact a + b = " << exact.str() << "\n TowardsZero = " << stored.str() + << "\n"; + log.flush(); + + if (scale != MantissaRange::MantissaScale::LargeLegacy) + BEAST_EXPECT(stored == exact); } } From 48e0ca72b003cd19b67c7fd27cc5561bd12d68f8 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 2 Jun 2026 15:35:42 -0400 Subject: [PATCH 04/23] Improve accuracy of Number::operator+= - Use more of the available range of the uint128 operands. - Also refactor Number::Guard::round() to return an enum. --- src/libxrpl/basics/Number.cpp | 38 ++++++++++------- src/test/basics/Number_test.cpp | 75 ++++++++++++++++++++------------- 2 files changed, 67 insertions(+), 46 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 4b5fe7b728..9f42209d1e 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -206,10 +206,16 @@ public: void doDropDigit(T& mantissa, int& exponent) noexcept; + enum class Round { + Down = -1, + Even = 0, + Up = 1, + }; + // Indicate round direction: 1 is up, -1 is down, 0 is even // This enables the client to round towards nearest, and on // tie, round towards even. - [[nodiscard]] int + [[nodiscard]] Round round() const noexcept; // Modify the result to the correctly rounded value @@ -314,41 +320,41 @@ Number::Guard::doDropDigit(uint128_t& mantissa, int& exponent) noexce // -1 if Guard is less than half // 0 if Guard is exactly half // 1 if Guard is greater than half -int +Number::Guard::Round Number::Guard::round() const noexcept { auto mode = Number::getround(); if (mode == RoundingMode::TowardsZero) - return -1; + return Round::Down; if (mode == RoundingMode::Downward) { if (sbit_) { if (digits_ > 0 || xbit_) - return 1; + return Round::Up; } - return -1; + return Round::Down; } if (mode == RoundingMode::Upward) { if (sbit_) - return -1; + return Round::Down; if (digits_ > 0 || xbit_) - return 1; - return -1; + return Round::Up; + return Round::Down; } // assume round to nearest if mode is not one of the predefined values if (digits_ > 0x5000'0000'0000'0000) - return 1; + return Round::Up; if (digits_ < 0x5000'0000'0000'0000) - return -1; + return Round::Down; if (xbit_) - return 1; - return 0; + return Round::Up; + return Round::Even; } template @@ -388,7 +394,7 @@ Number::Guard::doRoundUp( std::string location) { auto r = round(); - if (r == 1 || (r == 0 && (mantissa & 1) == 1)) + if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { auto const safeToIncrement = [&maxMantissa](auto const& mantissa) { return mantissa < maxMantissa && mantissa < kMaxRep; @@ -454,7 +460,7 @@ Number::Guard::doRoundDown( internalrep const& minMantissa) { auto r = round(); - if (r == 1 || (r == 0 && (mantissa & 1) == 1)) + if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { --mantissa; if (mantissa < minMantissa) @@ -471,7 +477,7 @@ void Number::Guard::doRound(rep& drops, std::string location) const { auto r = round(); - if (r == 1 || (r == 0 && (drops & 1) == 1)) + if (r == Round::Up || (r == Round::Even && (drops & 1) == 1)) { if (drops >= kMaxRep) { @@ -817,7 +823,7 @@ Number::operator+=(Number const& y) --xe; } g.doRoundDown(xn, xm, xe, minMantissa); - if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled && xm != 0) { // make a new guard Guard g; diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index cf77bf3599..722e63bdfb 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -182,33 +182,34 @@ public: testcase << "test_add " << to_string(scale); using Case = std::tuple; - auto const cSmall = std::to_array( - {{Number{1'000'000'000'000'000, -15}, - Number{6'555'555'555'555'555, -29}, - Number{1'000'000'000'000'066, -15}, - __LINE__}, - {Number{-1'000'000'000'000'000, -15}, - Number{-6'555'555'555'555'555, -29}, - Number{-1'000'000'000'000'066, -15}, - __LINE__}, - {Number{-1'000'000'000'000'000, -15}, - Number{6'555'555'555'555'555, -29}, - Number{-9'999'999'999'999'344, -16}, - __LINE__}, - {Number{-6'555'555'555'555'555, -29}, - Number{1'000'000'000'000'000, -15}, - Number{9'999'999'999'999'344, -16}, - __LINE__}, - {Number{}, Number{5}, Number{5}, __LINE__}, - {Number{5}, Number{}, Number{5}, __LINE__}, - {Number{5'555'555'555'555'555, -32768}, - Number{-5'555'555'555'555'554, -32768}, - Number{0}, - __LINE__}, - {Number{-9'999'999'999'999'999, -31}, - Number{1'000'000'000'000'000, -15}, - Number{9'999'999'999'999'990, -16}, - __LINE__}}); + auto const cSmall = std::to_array({ + {Number{1'000'000'000'000'000, -15}, + Number{6'555'555'555'555'555, -29}, + Number{1'000'000'000'000'066, -15}, + __LINE__}, + {Number{-1'000'000'000'000'000, -15}, + Number{-6'555'555'555'555'555, -29}, + Number{-1'000'000'000'000'066, -15}, + __LINE__}, + {Number{-1'000'000'000'000'000, -15}, + Number{6'555'555'555'555'555, -29}, + Number{-9'999'999'999'999'344, -16}, + __LINE__}, + {Number{-6'555'555'555'555'555, -29}, + Number{1'000'000'000'000'000, -15}, + Number{9'999'999'999'999'344, -16}, + __LINE__}, + {Number{}, Number{5}, Number{5}, __LINE__}, + {Number{5}, Number{}, Number{5}, __LINE__}, + {Number{5'555'555'555'555'555, -32768}, + Number{-5'555'555'555'555'554, -32768}, + Number{0}, + __LINE__}, + {Number{-9'999'999'999'999'999, -31}, + Number{1'000'000'000'000'000, -15}, + Number{9'999'999'999'999'990, -16}, + __LINE__}, + }); auto const cLarge = std::to_array( // Note that items with extremely large mantissas need to be // calculated, because otherwise they overflow uint64. Items from C @@ -2117,13 +2118,27 @@ public: BigInt const exact = toBigInt(a) + toBigInt(b); BigInt const stored = toBigInt(sum); + BigInt const diff = stored - exact; - log << "\n exact a + b = " << exact.str() << "\n TowardsZero = " << stored.str() - << "\n"; + log << "\n a = " << a << "\n b = " << b + << "\n exact a + b = " << exact.str() << "\n TowardsZero = " << stored.str() + << "\n difference = " << diff.str() << "\n"; log.flush(); - if (scale != MantissaRange::MantissaScale::LargeLegacy) + if (scale == MantissaRange::MantissaScale::Small) + { BEAST_EXPECT(stored == exact); + } + else if (scale == MantissaRange::MantissaScale::LargeLegacy) + { + BEAST_EXPECT(stored > exact); + } + else + { + BEAST_EXPECT(stored < exact); + BEAST_EXPECT(diff < 0); + BEAST_EXPECT(-diff < pow10(sum.exponent())); + } } } From 73bd964917b099ff78b6f7caa8ba5fdff6c8a3b1 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 2 Jun 2026 15:59:26 -0400 Subject: [PATCH 05/23] Remove the kMaxRep+1 rounding tests --- src/test/basics/Number_test.cpp | 80 --------------------------------- 1 file changed, 80 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 722e63bdfb..0c33ab5aee 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2018,86 +2018,6 @@ public: } } - { - testcase << "normalization cusp: ToNearest and Downward disagree " << to_string(scale); - - constexpr auto kMaxRep = Number::kMaxRep; - - // Both ToNearest and Downward should round to `below` - auto const actual = static_cast(kMaxRep) + 1; - Number const below{static_cast(kMaxRep), 0}; - Number const above{ - false, static_cast(kMaxRep) + 3, 0, Number::Unchecked{}}; - - auto construct = [](Number::RoundingMode mode) { - NumberRoundModeGuard const roundGuard{mode}; - return Number(false, actual, 0, Number::Normalized{}); - }; - Number const upward = construct(Number::RoundingMode::Upward); - - Number const toNearest = construct(Number::RoundingMode::ToNearest); - - Number const downward = construct(Number::RoundingMode::Downward); - - log << "\n" - << " actual = " << actual << " (kMaxRep + 1)\n" - << " below = " << below << " (kMaxRep, distance 1)\n" - << " above = " << above << " (kMaxRep + 3, distance 2)\n" - << " Upward = " << upward << "\n" - << " ToNearest = " << toNearest << "\n" - << " Downward = " << downward << "\n\n"; - log.flush(); - - switch (scale) - { - case MantissaRange::MantissaScale::Small: - // With the small mantissa, everything rounds up - - // Upward round UP - BEAST_EXPECT(upward > above); - - // ToNearest rounds UP when the DOWN neighbor is strictly closer - BEAST_EXPECT(toNearest > above); - BEAST_EXPECT(toNearest == below); - - // Downward undershoots: it returns a value below `below` - BEAST_EXPECT(downward < below); - - // Both should have given the same answer, but they differ - BEAST_EXPECT(toNearest > downward); - - break; - - case MantissaRange::MantissaScale::LargeLegacy: - // Upward round UP - BEAST_EXPECT(upward == above); - - // ToNearest rounds UP when the DOWN neighbor is strictly closer - BEAST_EXPECT(toNearest == above); - BEAST_EXPECT(toNearest > below); - - // Downward undershoots: it returns a value below `below` - BEAST_EXPECT(downward < below); - - // Both should have given the same answer, but they differ - BEAST_EXPECT(toNearest > downward); - - break; - default: - // Upward round UP - BEAST_EXPECT(upward == above); - - // ToNearest rounds UP when the DOWN neighbor is strictly closer - BEAST_EXPECT(toNearest != above); - BEAST_EXPECT(toNearest == below); - - // Downward undershoots: it returns a value below `below` - BEAST_EXPECT(downward == below); - - // Both should have given the same answer, but they differ - BEAST_EXPECT(toNearest == downward); - } - } { testcase << "operator+ TowardsZero rounds away from zero " << to_string(scale); From 0a240237972af2a23e62ff1f0c3d1c28c0325f93 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 4 Jun 2026 20:05:53 -0400 Subject: [PATCH 06/23] clang-tidy: template param names, const correctness, braces --- include/xrpl/basics/Number.h | 10 +++++----- src/test/basics/Number_test.cpp | 6 +++++- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index fae65ecb35..d256bcbb2a 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -54,11 +54,11 @@ namespace detail { constexpr std::size_t kUint64Digits = 20; constexpr std::size_t kUint128Digits = 39; -template -consteval std::array +template +consteval std::array buildPowersOfTen() { - std::array result{}; + std::array result{}; T power = 1; std::size_t exponent = 0; @@ -78,8 +78,8 @@ buildPowersOfTen() } // namespace detail -template -constexpr std::array kPowerOfTenImpl = detail::buildPowersOfTen(); +template +constexpr std::array kPowerOfTenImpl = detail::buildPowersOfTen(); constexpr auto kPowerOfTen = kPowerOfTenImpl; diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 0c33ab5aee..aa1de91b0c 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1794,7 +1794,7 @@ public: BigInt const exactProduct = BigInt(kAValue) * BigInt(kBValue); // What Number actually stored. - BigInt storedValue = toBigInt(product); + BigInt const storedValue = toBigInt(product); BigInt const signedDifference = storedValue - exactProduct; @@ -2026,9 +2026,13 @@ public: BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000000"}); if (scale != MantissaRange::MantissaScale::Small) + { BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000001"}); + } else + { BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000000"}); + } Number sum; { From 8ca90e7d01a31a4f76f8a217ed03a9bb70709983 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 5 Jun 2026 12:06:41 -0400 Subject: [PATCH 07/23] refactor: Construct Number::Guard from MantissaRange or relevant fields - Simplifies the function signatures in Guard, because it doesn't need to have those values passed in constantly. - Also simplifies some of the functions because they don't need to store values just to pass them to Guard functions. --- include/xrpl/basics/Number.h | 14 ++- src/libxrpl/basics/Number.cpp | 194 ++++++++++++++-------------------- 2 files changed, 86 insertions(+), 122 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index d256bcbb2a..49659956cd 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -147,7 +147,7 @@ struct MantissaRange final int const log{getExponent(scale)}; rep const min{getMin(scale, log)}; rep const max{(min * 10) - 1}; - CuspRoundingFix const cuspRoundingFixEnabled{isCuspFixEnabled(scale)}; + CuspRoundingFix const cuspRoundingFix{isCuspFixEnabled(scale)}; static MantissaRange const& getMantissaRange(MantissaScale scale); @@ -556,9 +556,15 @@ private: // changing the values inside the range. static thread_local std::reference_wrapper kRange; + class Guard; + void normalize(MantissaRange const& range); + // Guard has the fields that we need, as well as MantissaRange, so if we have a guard, use that + void + normalize(Guard const& guard); + /** Normalize Number components to an arbitrary range. * * min/maxMantissa are parameters because this function is used by both @@ -573,7 +579,7 @@ private: int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled); + MantissaRange::CuspRoundingFix cuspRoundingFix); template friend void @@ -583,7 +589,7 @@ private: int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + MantissaRange::CuspRoundingFix cuspRoundingFix, bool dropped); [[nodiscard]] bool @@ -601,8 +607,6 @@ private: // UB, and can vary across compilers. static internalrep externalToInternal(rep mantissa); - - class Guard; }; constexpr Number::Number(bool negative, internalrep mantissa, int exponent, Unchecked) noexcept diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 9f42209d1e..7cfc009ad1 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -65,7 +65,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 15); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max < Number::kMaxRep); - static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Disabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Disabled); } { [[maybe_unused]] @@ -76,7 +76,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Disabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Disabled); } { [[maybe_unused]] @@ -87,7 +87,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Enabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled); } return map; }(); @@ -171,7 +171,21 @@ class Number::Guard std::uint8_t sbit_ : 1 {0}; // the sign of the guard digits public: - explicit Guard() = default; + internalrep const minMantissa_; + internalrep const maxMantissa_; + MantissaRange::CuspRoundingFix const cuspRoundingFix_; + + explicit Guard( + internalrep const& minMantissa, + internalrep const& maxMantissa, + MantissaRange::CuspRoundingFix cuspRoundingFix) + : minMantissa_(minMantissa), maxMantissa_(maxMantissa), cuspRoundingFix_(cuspRoundingFix) + { + } + + explicit Guard(MantissaRange const& range) : Guard(range.min, range.max, range.cuspRoundingFix) + { + } // set & test the sign bit void @@ -221,19 +235,12 @@ public: // Modify the result to the correctly rounded value template void - doRoundUp( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa, - internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, - std::string location); + doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location); // Modify the result to the correctly rounded value template void - doRoundDown(bool& negative, T& mantissa, int& exponent, internalrep const& minMantissa); + doRoundDown(bool& negative, T& mantissa, int& exponent); // Modify the result to the correctly rounded value void @@ -245,7 +252,7 @@ private: template void - bringIntoRange(bool& negative, T& mantissa, int& exponent, internalrep const& minMantissa); + bringIntoRange(bool& negative, T& mantissa, int& exponent); }; inline void @@ -359,15 +366,11 @@ Number::Guard::round() const noexcept template void -Number::Guard::bringIntoRange( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa) +Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) { // Bring mantissa back into the minMantissa / maxMantissa range AFTER // rounding - if (mantissa < minMantissa) + if (mantissa < minMantissa_) { mantissa *= 10; --exponent; @@ -384,22 +387,15 @@ Number::Guard::bringIntoRange( template void -Number::Guard::doRoundUp( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa, - internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, - std::string location) +Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location) { auto r = round(); if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { - auto const safeToIncrement = [&maxMantissa](auto const& mantissa) { - return mantissa < maxMantissa && mantissa < kMaxRep; + auto const safeToIncrement = [this](auto const& mantissa) { + return mantissa < maxMantissa_ && mantissa < kMaxRep; }; - if (cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix_ == MantissaRange::CuspRoundingFix::Enabled) { // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". @@ -420,14 +416,7 @@ Number::Guard::doRoundUp( safeToIncrement(mantissa), "xrpl::Number::Guard::doRoundUp", "can't recurse more than once"); - doRoundUp( - negative, - mantissa, - exponent, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - location); + doRoundUp(negative, mantissa, exponent, location); return; } } @@ -438,7 +427,7 @@ Number::Guard::doRoundUp( ++mantissa; // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". - if (mantissa > maxMantissa || mantissa > kMaxRep) + if (mantissa > maxMantissa_ || mantissa > kMaxRep) { // Don't use doDropDigit here mantissa /= 10; @@ -446,30 +435,26 @@ Number::Guard::doRoundUp( } } } - bringIntoRange(negative, mantissa, exponent, minMantissa); + bringIntoRange(negative, mantissa, exponent); if (exponent > kMaxExponent) Throw(std::string(location)); } template void -Number::Guard::doRoundDown( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa) +Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { --mantissa; - if (mantissa < minMantissa) + if (mantissa < minMantissa_) { mantissa *= 10; --exponent; } } - bringIntoRange(negative, mantissa, exponent, minMantissa); + bringIntoRange(negative, mantissa, exponent); } // Modify the result to the correctly rounded value @@ -536,7 +521,7 @@ doNormalize( int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + MantissaRange::CuspRoundingFix cuspRoundingFix, bool dropped) { static constexpr auto kMinExponent = Number::kMinExponent; @@ -559,7 +544,7 @@ doNormalize( m *= 10; --exponent; } - Guard g; + Guard g(minMantissa, maxMantissa, cuspRoundingFix); if (negative) g.setNegative(); if (dropped) @@ -604,14 +589,7 @@ doNormalize( XRPL_ASSERT_PARTS(m <= kMaxRep, "xrpl::doNormalize", "intermediate mantissa fits in int64"); mantissa = m; - g.doRoundUp( - negative, - mantissa, - exponent, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::normalize 2"); + g.doRoundUp(negative, mantissa, exponent, "Number::normalize 2"); XRPL_ASSERT_PARTS( mantissa >= minMantissa && mantissa <= maxMantissa, "xrpl::doNormalize", @@ -626,13 +604,12 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFix) { // Not used by every compiler version, and thus not necessarily // counted by coverage build // LCOV_EXCL_START - doNormalize( - negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); // LCOV_EXCL_STOP } @@ -644,13 +621,12 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFix) { // Not used by every compiler version, and thus not necessarily // counted by coverage build // LCOV_EXCL_START - doNormalize( - negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); // LCOV_EXCL_STOP } @@ -662,16 +638,27 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFix) { - doNormalize( - negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); } void Number::normalize(MantissaRange const& range) { - normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFixEnabled); + normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFix); +} + +void +Number::normalize(Guard const& guard) +{ + normalize( + negative_, + mantissa_, + exponent_, + guard.minMantissa_, + guard.maxMantissa_, + guard.cuspRoundingFix_); } // Copy the number, but set a new exponent. Because the mantissa doesn't change, @@ -725,19 +712,20 @@ Number::operator+=(Number const& y) bool const yn = y.negative_; uint128_t ym = y.mantissa_; auto ye = y.exponent_; - Guard g; + Guard g(kRange); - auto const& range = kRange.get(); + auto const& minMantissa = g.minMantissa_; + auto const& maxMantissa = g.maxMantissa_; + auto const cuspRoundingFix = g.cuspRoundingFix_; // Bring the exponents of both values into agreement, so the mantissas are on the same scale // and can be added directly together // expandM / expandE: First try to expand the mantissa and bring the exponent down // shringM / shrinkE: Then shrink the mantissa and bring the exponent up, if necessary - auto const adjust = [&g, &range]( - uint128_t& expandM, int& expandE, uint128_t& shrinkM, int& shrinkE) { + auto const adjust = [&g](uint128_t& expandM, int& expandE, uint128_t& shrinkM, int& shrinkE) { constexpr uint128_t kSafeLimit = kPowerOfTenImpl[37]; - if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + if (g.cuspRoundingFix_ == MantissaRange::CuspRoundingFix::Enabled) { while (shrinkE < expandE && shrinkM % 10 == 0) { @@ -774,14 +762,10 @@ Number::operator+=(Number const& y) adjust(xm, xe, ym, ye); } - auto const& minMantissa = range.min; - auto const& maxMantissa = range.max; - auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; - if (xn == yn) { xm += ym; - if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) { while (xm > maxMantissa || xm > kMaxRep) { @@ -795,14 +779,7 @@ Number::operator+=(Number const& y) g.doDropDigit(xm, xe); } } - g.doRoundUp( - xn, - xm, - xe, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::addition overflow"); + g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } else { @@ -822,32 +799,25 @@ Number::operator+=(Number const& y) xm -= g.pop(); --xe; } - g.doRoundDown(xn, xm, xe, minMantissa); - if (range.cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled && xm != 0) + g.doRoundDown(xn, xm, xe); + if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled && xm != 0) { - // make a new guard - Guard g; + // this will be going away + Guard g(kRange); if (xn) g.setNegative(); while (xm > maxMantissa || xm > kMaxRep) { g.doDropDigit(xm, xe); } - g.doRoundUp( - xn, - xm, - xe, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::addition overflow"); + g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } } negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe; - normalize(range); + normalize(g); return *this; } @@ -881,14 +851,11 @@ Number::operator*=(Number const& y) auto ze = xe + ye; auto zs = xs * ys; bool zn = (zs == -1); - Guard g; + Guard g(kRange); if (zn) g.setNegative(); - auto const& range = kRange.get(); - auto const& minMantissa = range.min; - auto const& maxMantissa = range.max; - auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; + auto const& maxMantissa = g.maxMantissa_; while (zm > maxMantissa || zm > kMaxRep) { @@ -897,19 +864,12 @@ Number::operator*=(Number const& y) xm = static_cast(zm); xe = ze; - g.doRoundUp( - zn, - xm, - xe, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::multiplication overflow : exponent is " + std::to_string(xe)); + g.doRoundUp(zn, xm, xe, "Number::multiplication overflow : exponent is " + std::to_string(xe)); negative_ = zn; mantissa_ = xm; exponent_ = xe; - normalize(range); + normalize(g); return *this; } @@ -945,7 +905,7 @@ Number::operator/=(Number const& y) auto const& range = kRange.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; - auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; + auto const cuspRoundingFix = range.cuspRoundingFix; // Division operates on two large integers (16-digit for small // mantissas, 19-digit for large) using integer math. If the values @@ -1077,14 +1037,14 @@ Number::operator/=(Number const& y) // rounding fix is enabled, flag if there is still // a remainder from stage 2. bool const useTrailingRemainder = - cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; + cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled; if (useTrailingRemainder) { dropped = partialNumerator % dm != 0; } } } - doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped); + doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFix, dropped); negative_ = zp; mantissa_ = static_cast(zm); exponent_ = ze; @@ -1098,7 +1058,7 @@ operator rep() const { rep drops = mantissa(); int offset = exponent(); - Guard g; + Guard g(kRange); if (drops != 0) { if (negative_) From 64cb53629d876438368c32973807c4e6bf3b4f4b Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 5 Jun 2026 18:02:45 -0400 Subject: [PATCH 08/23] Rework subtraction rounding (again) for more accuracy - Go back to the old method of computing the mantissa, but when post processing, expand the mantissa to slightly larger than maxMantissa, then in doRoundDown, if the result is not exact, subtract one. Finally, let doNormalize figure out the rounding of the result. --- include/xrpl/basics/Number.h | 20 ++++ src/libxrpl/basics/Number.cpp | 133 +++++++++++++--------- src/test/basics/Number_test.cpp | 190 +++++++++++++++++++++++++------- 3 files changed, 254 insertions(+), 89 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 49659956cd..4df4a79835 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -884,6 +884,26 @@ to_string(MantissaRange::MantissaScale const& scale) } } +inline std::string +to_string(Number::RoundingMode const& round) +{ + switch (round) + { + enum class RoundingMode { ToNearest, TowardsZero, Downward, Upward }; + + case Number::RoundingMode::ToNearest: + return "ToNearest"; + case Number::RoundingMode::TowardsZero: + return "TowardsZero"; + case Number::RoundingMode::Downward: + return "Downward"; + case Number::RoundingMode::Upward: + return "Upward"; + default: + throw std::runtime_error("Bad rounding mode"); + } +} + class SaveNumberRoundMode { Number::RoundingMode mode_; diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 7cfc009ad1..9cf820c0c9 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -208,6 +208,10 @@ public: unsigned pop() noexcept; + // if true, there are no more digits to recover with pop() + bool + empty() const noexcept; + /** Drop a digit from the mantissa, and increment the exponent, storing the dropped digit in * this Guard. * @@ -221,8 +225,17 @@ public: doDropDigit(T& mantissa, int& exponent) noexcept; enum class Round { + // The result is exact. No rounding is needed. + Exact = -2, + // Round down. Since we use integer math, that usually means no change is needed. + // Exceptions are for when the result is between kMaxRap and kMaxRepUp (round to kMaxRep), + // or after subtraction where _any_ remainder will modify the result. The latter is what + // distinguishes Exact from Down. Down = -1, + // The result was exactly half-way between two integers. This will round to whichever of + // the two is even. Even = 0, + // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not enabled) Up = 1, }; @@ -302,6 +315,13 @@ Number::Guard::pop() noexcept return d; } +// if true, there are no more digits to recover with pop() +inline bool +Number::Guard::empty() const noexcept +{ + return digits_ == 0 && !xbit_; +} + template void Number::Guard::doDropDigit(T& mantissa, int& exponent) noexcept @@ -332,6 +352,12 @@ Number::Guard::round() const noexcept { auto mode = Number::getround(); + if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled && empty()) + { + // No remainder + return Round::Exact; + } + if (mode == RoundingMode::TowardsZero) return Round::Down; @@ -445,13 +471,31 @@ void Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); - if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) + if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled) { - --mantissa; - if (mantissa < minMantissa_) + // If there was any remainder, subtract 1 from the result, and pad with 9s. + // Example with 4 digit mantissas: + // 1000 - 0.0000000001 = 999.9999999999 + // In operator+=, the result will be: 1000, with Guard holding (0, xbit=true) + // * Rounding away from zero, the result should be 1000 + // * Rounding towards zero, the result should be 999.9 + // * Rounding to nearest, the result should be 999.9 + // The most accurate result is always 999.9 + if (r != Round::Exact) { - mantissa *= 10; - --exponent; + --mantissa; + } + } + else + { + if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) + { + --mantissa; + if (mantissa < minMantissa_) + { + mantissa *= 10; + --exponent; + } } } bringIntoRange(negative, mantissa, exponent); @@ -720,46 +764,24 @@ Number::operator+=(Number const& y) // Bring the exponents of both values into agreement, so the mantissas are on the same scale // and can be added directly together - // expandM / expandE: First try to expand the mantissa and bring the exponent down - // shringM / shrinkE: Then shrink the mantissa and bring the exponent up, if necessary - auto const adjust = [&g](uint128_t& expandM, int& expandE, uint128_t& shrinkM, int& shrinkE) { - constexpr uint128_t kSafeLimit = kPowerOfTenImpl[37]; - - if (g.cuspRoundingFix_ == MantissaRange::CuspRoundingFix::Enabled) - { - while (shrinkE < expandE && shrinkM % 10 == 0) - { - g.doDropDigit(shrinkM, shrinkE); - } - - // We've got 128 bits of mantissa to work with here. Don't throw away data unless we - // have to - while (shrinkE < expandE && expandE > kMinExponent && expandM < kSafeLimit) - { - expandM *= 10; - --expandE; - } - } - - while (shrinkE < expandE) - { - g.doDropDigit(shrinkM, shrinkE); - } - }; - + // Then shrink the mantissa and bring the exponent up of the value with the lower exponent if (xe < ye) { if (xn) g.setNegative(); - - adjust(ym, ye, xm, xe); + do + { + g.doDropDigit(xm, xe); + } while (xe < ye); } else if (xe > ye) { if (yn) g.setNegative(); - - adjust(xm, xe, ym, ye); + do + { + g.doDropDigit(ym, ye); + } while (xe > ye); } if (xn == yn) @@ -793,31 +815,38 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - while (xm < minMantissa && xm * 10 <= kMaxRep) + if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) { - xm *= 10; - xm -= g.pop(); - --xe; - } - g.doRoundDown(xn, xm, xe); - if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled && xm != 0) - { - // this will be going away - Guard g(kRange); - if (xn) - g.setNegative(); - while (xm > maxMantissa || xm > kMaxRep) + // Grow xm/xe and pull digits out of the Guard until it's just past the range, so that + // normalize will have enough information to make an accurate rounding decision, but + // stop if the Guard empties out. Note that if the xbit is set, the Guard will never be + // empty. + while (xm <= maxMantissa && !g.empty()) { - g.doDropDigit(xm, xe); + xm *= 10; + xm -= g.pop(); + --xe; } - g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } + else + { + // Grow xm/xe and pull digits out of the Guard until it's back in range. + while (xm < minMantissa && xm * 10 <= kMaxRep) + { + xm *= 10; + xm -= g.pop(); + --xe; + } + } + // Round down, based on whether there is any data left in the Guard (depending on + // cuspRoundingFix) + g.doRoundDown(xn, xm, xe); } + doNormalize(xn, xm, xe, minMantissa, maxMantissa, cuspRoundingFix, false); negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe; - normalize(g); return *this; } diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index aa1de91b0c..3b8156dea6 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -46,12 +46,17 @@ class Number_test : public beast::unit_test::Suite return out; } - static BigInt + BigInt toBigInt(Number const& n) { BigInt v = n.mantissa(); for (int i = 0; i < n.exponent(); ++i) v *= 10; + for (int i = 0; i > n.exponent(); --i) + { + BEAST_EXPECT(v % 10 == 0); + v /= 10; + } return v; } @@ -2019,49 +2024,160 @@ public: } { - testcase << "operator+ TowardsZero rounds away from zero " << to_string(scale); + testcase << "subtraction rounding " << to_string(scale); - Number const a{1LL, 20}; - Number const b{-1'000'000'000'000'000'001LL}; - - BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000000"}); - if (scale != MantissaRange::MantissaScale::Small) - { - BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000001"}); - } - else - { - BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000000"}); - } - - Number sum; - { - NumberRoundModeGuard const roundGuard{Number::RoundingMode::TowardsZero}; - sum = a + b; - } - - BigInt const exact = toBigInt(a) + toBigInt(b); - BigInt const stored = toBigInt(sum); - BigInt const diff = stored - exact; - - log << "\n a = " << a << "\n b = " << b - << "\n exact a + b = " << exact.str() << "\n TowardsZero = " << stored.str() - << "\n difference = " << diff.str() << "\n"; - log.flush(); + auto const exp = Number::mantissaLog(); + Number const a{1LL, exp + 2}; + Number const b{-(Number{1, exp} + 1)}; if (scale == MantissaRange::MantissaScale::Small) { - BEAST_EXPECT(stored == exact); - } - else if (scale == MantissaRange::MantissaScale::LargeLegacy) - { - BEAST_EXPECT(stored > exact); + BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000"}); + BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000001"}); } else { - BEAST_EXPECT(stored < exact); - BEAST_EXPECT(diff < 0); - BEAST_EXPECT(-diff < pow10(sum.exponent())); + BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000000"}); + BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000001"}); + } + + auto construct = [&a, &b, this](Number::RoundingMode r) { + NumberRoundModeGuard const roundGuard{r}; + auto const sum = a + b; + BigInt const stored = toBigInt(sum); + return std::make_pair(r, std::make_pair(stored, sum)); + }; + + auto const bigA = toBigInt(a); + auto const bigB = toBigInt(b); + BigInt const exact = bigA + bigB; + + auto const sums = [&]() { + std::map> sums; + sums.emplace(construct(Number::RoundingMode::TowardsZero)); + sums.emplace(construct(Number::RoundingMode::Upward)); + sums.emplace(construct(Number::RoundingMode::Downward)); + sums.emplace(construct(Number::RoundingMode::ToNearest)); + return sums; + }(); + + log << "\n a = " << a << " (" << fmt(bigA) << ")\n b = " << b + << " (" << fmt(bigB) << ")\n exact a + b = " << fmt(exact) << "\n"; + for (auto const& [r, sum] : sums) + { + auto const diff = sum.first - exact; + auto const rLabel = to_string(r); + log << std::string(15 - rLabel.length(), ' ') << rLabel << " = " << fmt(sum.first) + << "\n difference = " << fmt(diff) << "\n"; + } + log.flush(); + + switch (scale) + { + case MantissaRange::MantissaScale::Small: + case MantissaRange::MantissaScale::LargeLegacy: { + // Without the fix, all the results but one round up + BEAST_EXPECT(sums.at(Number::RoundingMode::TowardsZero).first > exact); + BEAST_EXPECT(sums.at(Number::RoundingMode::Upward).first > exact); + BEAST_EXPECT(sums.at(Number::RoundingMode::ToNearest).first > exact); + // Downward works because the Guard sign is negative, and Downward returns Up + // instead of Down if negative and there's a remainder, whereas TowardsZero + // always returns Down. + BEAST_EXPECT(sums.at(Number::RoundingMode::Downward).first < exact); + break; + } + default: { + for (auto const& [r, sum] : sums) + { + auto const epsilon = pow10(sum.second.exponent()); + BEAST_EXPECT(epsilon == 100); + auto diff = sum.first - exact; + switch (r) + { + case Number::RoundingMode::Upward: + case Number::RoundingMode::ToNearest: + BEAST_EXPECT(sum.first > exact); + BEAST_EXPECT(diff < epsilon); + break; + default: + BEAST_EXPECT(sum.first < exact); + BEAST_EXPECT(-diff < epsilon); + } + } + } + } + } + { + auto const offset = 30; + testcase << "subtraction rounding offset of " << offset << " " << to_string(scale); + + auto const exp = Number::mantissaLog(); + Number const a{1LL, exp + offset}; + Number const b{-1}; + + auto construct = [&a, &b, this](Number::RoundingMode r) { + NumberRoundModeGuard const roundGuard{r}; + auto const sum = a + b; + BigInt const stored = toBigInt(sum); + return std::make_pair(r, std::make_pair(stored, sum)); + }; + + auto const bigA = toBigInt(a); + auto const bigB = toBigInt(b); + BigInt const exact = bigA + bigB; + + auto const sums = [&]() { + std::map> sums; + sums.emplace(construct(Number::RoundingMode::TowardsZero)); + sums.emplace(construct(Number::RoundingMode::Upward)); + sums.emplace(construct(Number::RoundingMode::Downward)); + sums.emplace(construct(Number::RoundingMode::ToNearest)); + return sums; + }(); + + log << "\n a = " << a << " (" << fmt(bigA) << ")\n b = " << b + << " (" << fmt(bigB) << ")\n exact a + b = " << fmt(exact) << "\n"; + for (auto const& [r, sum] : sums) + { + auto const diff = sum.first - exact; + auto const rLabel = to_string(r); + log << std::string(15 - rLabel.length(), ' ') << rLabel << " = " << fmt(sum.first) + << "\n difference = " << fmt(diff) << "\n"; + } + log.flush(); + + switch (scale) + { + case MantissaRange::MantissaScale::Small: + case MantissaRange::MantissaScale::LargeLegacy: { + // Without the fix, all the results but one round up + BEAST_EXPECT(sums.at(Number::RoundingMode::TowardsZero).first > exact); + BEAST_EXPECT(sums.at(Number::RoundingMode::Upward).first > exact); + BEAST_EXPECT(sums.at(Number::RoundingMode::ToNearest).first > exact); + // Downward works because the Guard sign is negative, and Downward returns Up + // instead of Down if negative and there's a remainder, whereas TowardsZero + // always returns Down. + BEAST_EXPECT(sums.at(Number::RoundingMode::Downward).first < exact); + break; + } + default: { + for (auto const& [r, sum] : sums) + { + auto const epsilon = pow10(sum.second.exponent()); + auto diff = sum.first - exact; + switch (r) + { + case Number::RoundingMode::Upward: + case Number::RoundingMode::ToNearest: + BEAST_EXPECT(sum.first > exact); + BEAST_EXPECT(diff < epsilon); + break; + default: + BEAST_EXPECT(sum.first < exact); + BEAST_EXPECT(-diff < epsilon); + } + } + } } } } From 184f936362ecbba2401a50cf1f29d61171464777 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 6 Jun 2026 13:09:52 -0400 Subject: [PATCH 09/23] Improve comment descriptions --- src/libxrpl/basics/Number.cpp | 49 ++++++++++++----------------------- 1 file changed, 17 insertions(+), 32 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 9cf820c0c9..d43eebc314 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -208,7 +208,7 @@ public: unsigned pop() noexcept; - // if true, there are no more digits to recover with pop() + // if true, there are no digits in the guard, including dropped digits (xbit_) bool empty() const noexcept; @@ -225,15 +225,14 @@ public: doDropDigit(T& mantissa, int& exponent) noexcept; enum class Round { - // The result is exact. No rounding is needed. + // The result is exact. No rounding is needed. Only used if cuspRoundingFix is enabled. Exact = -2, // Round down. Since we use integer math, that usually means no change is needed. // Exceptions are for when the result is between kMaxRap and kMaxRepUp (round to kMaxRep), // or after subtraction where _any_ remainder will modify the result. The latter is what // distinguishes Exact from Down. Down = -1, - // The result was exactly half-way between two integers. This will round to whichever of - // the two is even. + // The result was exactly half-way between two integers. This will round to even. Even = 0, // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not enabled) Up = 1, @@ -315,7 +314,6 @@ Number::Guard::pop() noexcept return d; } -// if true, there are no more digits to recover with pop() inline bool Number::Guard::empty() const noexcept { @@ -473,14 +471,8 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) auto r = round(); if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled) { - // If there was any remainder, subtract 1 from the result, and pad with 9s. - // Example with 4 digit mantissas: - // 1000 - 0.0000000001 = 999.9999999999 - // In operator+=, the result will be: 1000, with Guard holding (0, xbit=true) - // * Rounding away from zero, the result should be 1000 - // * Rounding towards zero, the result should be 999.9 - // * Rounding to nearest, the result should be 999.9 - // The most accurate result is always 999.9 + // If there was any remainder, subtract 1 from the result. This is sufficient to get the + // best rounding. if (r != Round::Exact) { --mantissa; @@ -763,8 +755,9 @@ Number::operator+=(Number const& y) auto const cuspRoundingFix = g.cuspRoundingFix_; // Bring the exponents of both values into agreement, so the mantissas are on the same scale - // and can be added directly together - // Then shrink the mantissa and bring the exponent up of the value with the lower exponent + // and can be added directly together. + // Shrink the mantissa and bring the exponent up of the value with the lower exponent. Store any + // dropped digits in the Guard. if (xe < ye) { if (xn) @@ -787,19 +780,9 @@ Number::operator+=(Number const& y) if (xn == yn) { xm += ym; - if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) + if (xm > maxMantissa || xm > kMaxRep) { - while (xm > maxMantissa || xm > kMaxRep) - { - g.doDropDigit(xm, xe); - } - } - else - { - if (xm > maxMantissa || xm > kMaxRep) - { - g.doDropDigit(xm, xe); - } + g.doDropDigit(xm, xe); } g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } @@ -817,11 +800,13 @@ Number::operator+=(Number const& y) } if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) { - // Grow xm/xe and pull digits out of the Guard until it's just past the range, so that - // normalize will have enough information to make an accurate rounding decision, but - // stop if the Guard empties out. Note that if the xbit is set, the Guard will never be - // empty. - while (xm <= maxMantissa && !g.empty()) + // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than + // maxMantissa, so that normalize will have enough information to make an accurate + // rounding decision, but stop if the Guard empties out, because no rounding will be + // necessary. (Normalize will pad it back into range.) Note that if any digits were lost + // (xbit), the Guard will never be empty, so xm will get big. + auto const upperLimit = static_cast(minMantissa) * 1000; + while (xm < upperLimit && !g.empty()) { xm *= 10; xm -= g.pop(); From e77b154edcc43ecf1a47b0d2ed8c40c9b5028793 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 6 Jun 2026 14:33:31 -0400 Subject: [PATCH 10/23] Include rounding in failed unit tests --- src/test/basics/Number_test.cpp | 53 +++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 3b8156dea6..7a003a372d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2077,13 +2077,17 @@ public: case MantissaRange::MantissaScale::Small: case MantissaRange::MantissaScale::LargeLegacy: { // Without the fix, all the results but one round up - BEAST_EXPECT(sums.at(Number::RoundingMode::TowardsZero).first > exact); - BEAST_EXPECT(sums.at(Number::RoundingMode::Upward).first > exact); - BEAST_EXPECT(sums.at(Number::RoundingMode::ToNearest).first > exact); - // Downward works because the Guard sign is negative, and Downward returns Up - // instead of Down if negative and there's a remainder, whereas TowardsZero - // always returns Down. - BEAST_EXPECT(sums.at(Number::RoundingMode::Downward).first < exact); + for (auto const& [r, sum] : sums) { + if (r == Number::RoundingMode::Downward) { + // Downward works because the Guard sign is negative, and Downward returns Up + // instead of Down if negative and there's a remainder, whereas TowardsZero + // always returns Down. + BEAST_EXPECTS(sums.at(Number::RoundingMode::Downward).first < exact, to_string(r)); + } + else { + BEAST_EXPECTS(sums.at(r).first > exact, to_string(r)); + } + } break; } default: { @@ -2096,12 +2100,12 @@ public: { case Number::RoundingMode::Upward: case Number::RoundingMode::ToNearest: - BEAST_EXPECT(sum.first > exact); - BEAST_EXPECT(diff < epsilon); + BEAST_EXPECTS(sum.first > exact, to_string(r)); + BEAST_EXPECTS(diff < epsilon, to_string(r)); break; default: - BEAST_EXPECT(sum.first < exact); - BEAST_EXPECT(-diff < epsilon); + BEAST_EXPECTS(sum.first < exact, to_string(r)); + BEAST_EXPECTS(-diff < epsilon, to_string(r)); } } } @@ -2150,14 +2154,17 @@ public: { case MantissaRange::MantissaScale::Small: case MantissaRange::MantissaScale::LargeLegacy: { - // Without the fix, all the results but one round up - BEAST_EXPECT(sums.at(Number::RoundingMode::TowardsZero).first > exact); - BEAST_EXPECT(sums.at(Number::RoundingMode::Upward).first > exact); - BEAST_EXPECT(sums.at(Number::RoundingMode::ToNearest).first > exact); - // Downward works because the Guard sign is negative, and Downward returns Up - // instead of Down if negative and there's a remainder, whereas TowardsZero - // always returns Down. - BEAST_EXPECT(sums.at(Number::RoundingMode::Downward).first < exact); + for (auto const& [r, sum] : sums) { + if (r == Number::RoundingMode::Downward) { + // Downward works because the Guard sign is negative, and Downward returns Up + // instead of Down if negative and there's a remainder, whereas TowardsZero + // always returns Down. + BEAST_EXPECTS(sums.at(Number::RoundingMode::Downward).first < exact, to_string(r)); + } + else { + BEAST_EXPECTS(sums.at(r).first > exact, to_string(r)); + } + } break; } default: { @@ -2169,12 +2176,12 @@ public: { case Number::RoundingMode::Upward: case Number::RoundingMode::ToNearest: - BEAST_EXPECT(sum.first > exact); - BEAST_EXPECT(diff < epsilon); + BEAST_EXPECTS(sum.first > exact, to_string(r)); + BEAST_EXPECTS(diff < epsilon, to_string(r)); break; default: - BEAST_EXPECT(sum.first < exact); - BEAST_EXPECT(-diff < epsilon); + BEAST_EXPECTS(sum.first < exact, to_string(r)); + BEAST_EXPECTS(-diff < epsilon, to_string(r)); } } } From 719157449989302857a1aacf24cd1b853f807571 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 6 Jun 2026 14:34:31 -0400 Subject: [PATCH 11/23] Rollback Number class changes; show the fix works without side effects --- include/xrpl/basics/Number.h | 38 ++--- src/libxrpl/basics/Number.cpp | 285 +++++++++++++++------------------- 2 files changed, 138 insertions(+), 185 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 4df4a79835..8e7d945621 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -51,43 +51,37 @@ namespace detail { * compile time. Doing it at runtime would be pretty wasteful and * inefficient. */ -constexpr std::size_t kUint64Digits = 20; -constexpr std::size_t kUint128Digits = 39; - -template -consteval std::array +constexpr std::size_t kInt64Digits = 20; +consteval std::array buildPowersOfTen() { - std::array result{}; + std::array result{}; - T power = 1; + std::uint64_t power = 1; std::size_t exponent = 0; // end the loop early so it doesn't overflow; for (; exponent < result.size() - 1; ++exponent, power *= 10) { result[exponent] = power; - if (power > std::numeric_limits::max() / 10) + if (power > std::numeric_limits::max() / 10) throw std::logic_error("Power of 10 table is too big"); } result[exponent] = power; - if (power < std::numeric_limits::max() / 10) - throw std::logic_error("Power of 10 table is not big enough for the given type"); + if (power < std::numeric_limits::max() / 10) + throw std::logic_error("Power of 10 table is not big enough for the uint64_t type"); return result; } } // namespace detail -template -constexpr std::array kPowerOfTenImpl = detail::buildPowersOfTen(); - -constexpr auto kPowerOfTen = kPowerOfTenImpl; +constexpr std::array kPowerOfTen = detail::buildPowersOfTen(); static_assert(kPowerOfTen[0] == 1); static_assert(kPowerOfTen[1] == 10); static_assert(kPowerOfTen[10] == 10'000'000'000); static_assert( - isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kUint64Digits - 1); + isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kInt64Digits - 1); /** MantissaRange defines a range for the mantissa of a normalized Number. * @@ -147,7 +141,7 @@ struct MantissaRange final int const log{getExponent(scale)}; rep const min{getMin(scale, log)}; rep const max{(min * 10) - 1}; - CuspRoundingFix const cuspRoundingFix{isCuspFixEnabled(scale)}; + CuspRoundingFix const cuspRoundingFixEnabled{isCuspFixEnabled(scale)}; static MantissaRange const& getMantissaRange(MantissaScale scale); @@ -556,15 +550,9 @@ private: // changing the values inside the range. static thread_local std::reference_wrapper kRange; - class Guard; - void normalize(MantissaRange const& range); - // Guard has the fields that we need, as well as MantissaRange, so if we have a guard, use that - void - normalize(Guard const& guard); - /** Normalize Number components to an arbitrary range. * * min/maxMantissa are parameters because this function is used by both @@ -579,7 +567,7 @@ private: int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix); + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled); template friend void @@ -589,7 +577,7 @@ private: int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix, + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, bool dropped); [[nodiscard]] bool @@ -607,6 +595,8 @@ private: // UB, and can vary across compilers. static internalrep externalToInternal(rep mantissa); + + class Guard; }; constexpr Number::Number(bool negative, internalrep mantissa, int exponent, Unchecked) noexcept diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index d43eebc314..23e913bbdc 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -65,7 +65,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 15); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max < Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Disabled); + static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Disabled); } { [[maybe_unused]] @@ -76,7 +76,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Disabled); + static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Disabled); } { [[maybe_unused]] @@ -87,7 +87,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled); + static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Enabled); } return map; }(); @@ -171,21 +171,7 @@ class Number::Guard std::uint8_t sbit_ : 1 {0}; // the sign of the guard digits public: - internalrep const minMantissa_; - internalrep const maxMantissa_; - MantissaRange::CuspRoundingFix const cuspRoundingFix_; - - explicit Guard( - internalrep const& minMantissa, - internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix) - : minMantissa_(minMantissa), maxMantissa_(maxMantissa), cuspRoundingFix_(cuspRoundingFix) - { - } - - explicit Guard(MantissaRange const& range) : Guard(range.min, range.max, range.cuspRoundingFix) - { - } + explicit Guard() = default; // set & test the sign bit void @@ -208,10 +194,6 @@ public: unsigned pop() noexcept; - // if true, there are no digits in the guard, including dropped digits (xbit_) - bool - empty() const noexcept; - /** Drop a digit from the mantissa, and increment the exponent, storing the dropped digit in * this Guard. * @@ -224,35 +206,28 @@ public: void doDropDigit(T& mantissa, int& exponent) noexcept; - enum class Round { - // The result is exact. No rounding is needed. Only used if cuspRoundingFix is enabled. - Exact = -2, - // Round down. Since we use integer math, that usually means no change is needed. - // Exceptions are for when the result is between kMaxRap and kMaxRepUp (round to kMaxRep), - // or after subtraction where _any_ remainder will modify the result. The latter is what - // distinguishes Exact from Down. - Down = -1, - // The result was exactly half-way between two integers. This will round to even. - Even = 0, - // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not enabled) - Up = 1, - }; - // Indicate round direction: 1 is up, -1 is down, 0 is even // This enables the client to round towards nearest, and on // tie, round towards even. - [[nodiscard]] Round + [[nodiscard]] int round() const noexcept; // Modify the result to the correctly rounded value template void - doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location); + doRoundUp( + bool& negative, + T& mantissa, + int& exponent, + internalrep const& minMantissa, + internalrep const& maxMantissa, + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + std::string location); // Modify the result to the correctly rounded value template void - doRoundDown(bool& negative, T& mantissa, int& exponent); + doRoundDown(bool& negative, T& mantissa, int& exponent, internalrep const& minMantissa); // Modify the result to the correctly rounded value void @@ -264,7 +239,7 @@ private: template void - bringIntoRange(bool& negative, T& mantissa, int& exponent); + bringIntoRange(bool& negative, T& mantissa, int& exponent, internalrep const& minMantissa); }; inline void @@ -314,12 +289,6 @@ Number::Guard::pop() noexcept return d; } -inline bool -Number::Guard::empty() const noexcept -{ - return digits_ == 0 && !xbit_; -} - template void Number::Guard::doDropDigit(T& mantissa, int& exponent) noexcept @@ -345,56 +314,54 @@ Number::Guard::doDropDigit(uint128_t& mantissa, int& exponent) noexce // -1 if Guard is less than half // 0 if Guard is exactly half // 1 if Guard is greater than half -Number::Guard::Round +int Number::Guard::round() const noexcept { auto mode = Number::getround(); - if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled && empty()) - { - // No remainder - return Round::Exact; - } - if (mode == RoundingMode::TowardsZero) - return Round::Down; + return -1; if (mode == RoundingMode::Downward) { if (sbit_) { if (digits_ > 0 || xbit_) - return Round::Up; + return 1; } - return Round::Down; + return -1; } if (mode == RoundingMode::Upward) { if (sbit_) - return Round::Down; + return -1; if (digits_ > 0 || xbit_) - return Round::Up; - return Round::Down; + return 1; + return -1; } // assume round to nearest if mode is not one of the predefined values if (digits_ > 0x5000'0000'0000'0000) - return Round::Up; + return 1; if (digits_ < 0x5000'0000'0000'0000) - return Round::Down; + return -1; if (xbit_) - return Round::Up; - return Round::Even; + return 1; + return 0; } template void -Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) +Number::Guard::bringIntoRange( + bool& negative, + T& mantissa, + int& exponent, + internalrep const& minMantissa) { // Bring mantissa back into the minMantissa / maxMantissa range AFTER // rounding - if (mantissa < minMantissa_) + if (mantissa < minMantissa) { mantissa *= 10; --exponent; @@ -411,15 +378,22 @@ Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) template void -Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location) +Number::Guard::doRoundUp( + bool& negative, + T& mantissa, + int& exponent, + internalrep const& minMantissa, + internalrep const& maxMantissa, + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + std::string location) { auto r = round(); - if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) + if (r == 1 || (r == 0 && (mantissa & 1) == 1)) { - auto const safeToIncrement = [this](auto const& mantissa) { - return mantissa < maxMantissa_ && mantissa < kMaxRep; + auto const safeToIncrement = [&maxMantissa](auto const& mantissa) { + return mantissa < maxMantissa && mantissa < kMaxRep; }; - if (cuspRoundingFix_ == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) { // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". @@ -440,7 +414,14 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string safeToIncrement(mantissa), "xrpl::Number::Guard::doRoundUp", "can't recurse more than once"); - doRoundUp(negative, mantissa, exponent, location); + doRoundUp( + negative, + mantissa, + exponent, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + location); return; } } @@ -451,7 +432,7 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string ++mantissa; // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". - if (mantissa > maxMantissa_ || mantissa > kMaxRep) + if (mantissa > maxMantissa || mantissa > kMaxRep) { // Don't use doDropDigit here mantissa /= 10; @@ -459,38 +440,30 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string } } } - bringIntoRange(negative, mantissa, exponent); + bringIntoRange(negative, mantissa, exponent, minMantissa); if (exponent > kMaxExponent) Throw(std::string(location)); } template void -Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) +Number::Guard::doRoundDown( + bool& negative, + T& mantissa, + int& exponent, + internalrep const& minMantissa) { auto r = round(); - if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled) + if (r == 1 || (r == 0 && (mantissa & 1) == 1)) { - // If there was any remainder, subtract 1 from the result. This is sufficient to get the - // best rounding. - if (r != Round::Exact) + --mantissa; + if (mantissa < minMantissa) { - --mantissa; + mantissa *= 10; + --exponent; } } - else - { - if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) - { - --mantissa; - if (mantissa < minMantissa_) - { - mantissa *= 10; - --exponent; - } - } - } - bringIntoRange(negative, mantissa, exponent); + bringIntoRange(negative, mantissa, exponent, minMantissa); } // Modify the result to the correctly rounded value @@ -498,7 +471,7 @@ void Number::Guard::doRound(rep& drops, std::string location) const { auto r = round(); - if (r == Round::Up || (r == Round::Even && (drops & 1) == 1)) + if (r == 1 || (r == 0 && (drops & 1) == 1)) { if (drops >= kMaxRep) { @@ -557,7 +530,7 @@ doNormalize( int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix, + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, bool dropped) { static constexpr auto kMinExponent = Number::kMinExponent; @@ -580,7 +553,7 @@ doNormalize( m *= 10; --exponent; } - Guard g(minMantissa, maxMantissa, cuspRoundingFix); + Guard g; if (negative) g.setNegative(); if (dropped) @@ -625,7 +598,14 @@ doNormalize( XRPL_ASSERT_PARTS(m <= kMaxRep, "xrpl::doNormalize", "intermediate mantissa fits in int64"); mantissa = m; - g.doRoundUp(negative, mantissa, exponent, "Number::normalize 2"); + g.doRoundUp( + negative, + mantissa, + exponent, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + "Number::normalize 2"); XRPL_ASSERT_PARTS( mantissa >= minMantissa && mantissa <= maxMantissa, "xrpl::doNormalize", @@ -640,12 +620,13 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix) + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { // Not used by every compiler version, and thus not necessarily // counted by coverage build // LCOV_EXCL_START - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); + doNormalize( + negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); // LCOV_EXCL_STOP } @@ -657,12 +638,13 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix) + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { // Not used by every compiler version, and thus not necessarily // counted by coverage build // LCOV_EXCL_START - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); + doNormalize( + negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); // LCOV_EXCL_STOP } @@ -674,27 +656,16 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFix) + MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); + doNormalize( + negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); } void Number::normalize(MantissaRange const& range) { - normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFix); -} - -void -Number::normalize(Guard const& guard) -{ - normalize( - negative_, - mantissa_, - exponent_, - guard.minMantissa_, - guard.maxMantissa_, - guard.cuspRoundingFix_); + normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFixEnabled); } // Copy the number, but set a new exponent. Because the mantissa doesn't change, @@ -748,16 +719,7 @@ Number::operator+=(Number const& y) bool const yn = y.negative_; uint128_t ym = y.mantissa_; auto ye = y.exponent_; - Guard g(kRange); - - auto const& minMantissa = g.minMantissa_; - auto const& maxMantissa = g.maxMantissa_; - auto const cuspRoundingFix = g.cuspRoundingFix_; - - // Bring the exponents of both values into agreement, so the mantissas are on the same scale - // and can be added directly together. - // Shrink the mantissa and bring the exponent up of the value with the lower exponent. Store any - // dropped digits in the Guard. + Guard g; if (xe < ye) { if (xn) @@ -777,6 +739,11 @@ Number::operator+=(Number const& y) } while (xe > ye); } + auto const& range = kRange.get(); + auto const& minMantissa = range.min; + auto const& maxMantissa = range.max; + auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; + if (xn == yn) { xm += ym; @@ -784,7 +751,14 @@ Number::operator+=(Number const& y) { g.doDropDigit(xm, xe); } - g.doRoundUp(xn, xm, xe, "Number::addition overflow"); + g.doRoundUp( + xn, + xm, + xe, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + "Number::addition overflow"); } else { @@ -798,40 +772,19 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) + while (xm < minMantissa && xm * 10 <= kMaxRep) { - // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than - // maxMantissa, so that normalize will have enough information to make an accurate - // rounding decision, but stop if the Guard empties out, because no rounding will be - // necessary. (Normalize will pad it back into range.) Note that if any digits were lost - // (xbit), the Guard will never be empty, so xm will get big. - auto const upperLimit = static_cast(minMantissa) * 1000; - while (xm < upperLimit && !g.empty()) - { - xm *= 10; - xm -= g.pop(); - --xe; - } + xm *= 10; + xm -= g.pop(); + --xe; } - else - { - // Grow xm/xe and pull digits out of the Guard until it's back in range. - while (xm < minMantissa && xm * 10 <= kMaxRep) - { - xm *= 10; - xm -= g.pop(); - --xe; - } - } - // Round down, based on whether there is any data left in the Guard (depending on - // cuspRoundingFix) - g.doRoundDown(xn, xm, xe); + g.doRoundDown(xn, xm, xe, minMantissa); } - doNormalize(xn, xm, xe, minMantissa, maxMantissa, cuspRoundingFix, false); negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe; + normalize(range); return *this; } @@ -865,11 +818,14 @@ Number::operator*=(Number const& y) auto ze = xe + ye; auto zs = xs * ys; bool zn = (zs == -1); - Guard g(kRange); + Guard g; if (zn) g.setNegative(); - auto const& maxMantissa = g.maxMantissa_; + auto const& range = kRange.get(); + auto const& minMantissa = range.min; + auto const& maxMantissa = range.max; + auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; while (zm > maxMantissa || zm > kMaxRep) { @@ -878,12 +834,19 @@ Number::operator*=(Number const& y) xm = static_cast(zm); xe = ze; - g.doRoundUp(zn, xm, xe, "Number::multiplication overflow : exponent is " + std::to_string(xe)); + g.doRoundUp( + zn, + xm, + xe, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + "Number::multiplication overflow : exponent is " + std::to_string(xe)); negative_ = zn; mantissa_ = xm; exponent_ = xe; - normalize(g); + normalize(range); return *this; } @@ -919,7 +882,7 @@ Number::operator/=(Number const& y) auto const& range = kRange.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; - auto const cuspRoundingFix = range.cuspRoundingFix; + auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; // Division operates on two large integers (16-digit for small // mantissas, 19-digit for large) using integer math. If the values @@ -1051,14 +1014,14 @@ Number::operator/=(Number const& y) // rounding fix is enabled, flag if there is still // a remainder from stage 2. bool const useTrailingRemainder = - cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled; + cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; if (useTrailingRemainder) { dropped = partialNumerator % dm != 0; } } } - doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFix, dropped); + doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped); negative_ = zp; mantissa_ = static_cast(zm); exponent_ = ze; @@ -1072,7 +1035,7 @@ operator rep() const { rep drops = mantissa(); int offset = exponent(); - Guard g(kRange); + Guard g; if (drops != 0) { if (negative_) From b263f442be72e27959eb31f899492a133e411f78 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 6 Jun 2026 14:35:35 -0400 Subject: [PATCH 12/23] Revert "Rollback Number class changes; show the fix works without side effects" This reverts commit 8743be8eae2a28b05f3683d3f0164dd50737305e. --- include/xrpl/basics/Number.h | 38 +++-- src/libxrpl/basics/Number.cpp | 285 +++++++++++++++++++--------------- 2 files changed, 185 insertions(+), 138 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 8e7d945621..4df4a79835 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -51,37 +51,43 @@ namespace detail { * compile time. Doing it at runtime would be pretty wasteful and * inefficient. */ -constexpr std::size_t kInt64Digits = 20; -consteval std::array +constexpr std::size_t kUint64Digits = 20; +constexpr std::size_t kUint128Digits = 39; + +template +consteval std::array buildPowersOfTen() { - std::array result{}; + std::array result{}; - std::uint64_t power = 1; + T power = 1; std::size_t exponent = 0; // end the loop early so it doesn't overflow; for (; exponent < result.size() - 1; ++exponent, power *= 10) { result[exponent] = power; - if (power > std::numeric_limits::max() / 10) + if (power > std::numeric_limits::max() / 10) throw std::logic_error("Power of 10 table is too big"); } result[exponent] = power; - if (power < std::numeric_limits::max() / 10) - throw std::logic_error("Power of 10 table is not big enough for the uint64_t type"); + if (power < std::numeric_limits::max() / 10) + throw std::logic_error("Power of 10 table is not big enough for the given type"); return result; } } // namespace detail -constexpr std::array kPowerOfTen = detail::buildPowersOfTen(); +template +constexpr std::array kPowerOfTenImpl = detail::buildPowersOfTen(); + +constexpr auto kPowerOfTen = kPowerOfTenImpl; static_assert(kPowerOfTen[0] == 1); static_assert(kPowerOfTen[1] == 10); static_assert(kPowerOfTen[10] == 10'000'000'000); static_assert( - isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kInt64Digits - 1); + isPowerOfTen(kPowerOfTen.back()) && *logTen(kPowerOfTen.back()) == detail::kUint64Digits - 1); /** MantissaRange defines a range for the mantissa of a normalized Number. * @@ -141,7 +147,7 @@ struct MantissaRange final int const log{getExponent(scale)}; rep const min{getMin(scale, log)}; rep const max{(min * 10) - 1}; - CuspRoundingFix const cuspRoundingFixEnabled{isCuspFixEnabled(scale)}; + CuspRoundingFix const cuspRoundingFix{isCuspFixEnabled(scale)}; static MantissaRange const& getMantissaRange(MantissaScale scale); @@ -550,9 +556,15 @@ private: // changing the values inside the range. static thread_local std::reference_wrapper kRange; + class Guard; + void normalize(MantissaRange const& range); + // Guard has the fields that we need, as well as MantissaRange, so if we have a guard, use that + void + normalize(Guard const& guard); + /** Normalize Number components to an arbitrary range. * * min/maxMantissa are parameters because this function is used by both @@ -567,7 +579,7 @@ private: int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled); + MantissaRange::CuspRoundingFix cuspRoundingFix); template friend void @@ -577,7 +589,7 @@ private: int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + MantissaRange::CuspRoundingFix cuspRoundingFix, bool dropped); [[nodiscard]] bool @@ -595,8 +607,6 @@ private: // UB, and can vary across compilers. static internalrep externalToInternal(rep mantissa); - - class Guard; }; constexpr Number::Number(bool negative, internalrep mantissa, int exponent, Unchecked) noexcept diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 23e913bbdc..d43eebc314 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -65,7 +65,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 15); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max < Number::kMaxRep); - static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Disabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Disabled); } { [[maybe_unused]] @@ -76,7 +76,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Disabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Disabled); } { [[maybe_unused]] @@ -87,7 +87,7 @@ MantissaRange::getRanges() static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFixEnabled == CuspRoundingFix::Enabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled); } return map; }(); @@ -171,7 +171,21 @@ class Number::Guard std::uint8_t sbit_ : 1 {0}; // the sign of the guard digits public: - explicit Guard() = default; + internalrep const minMantissa_; + internalrep const maxMantissa_; + MantissaRange::CuspRoundingFix const cuspRoundingFix_; + + explicit Guard( + internalrep const& minMantissa, + internalrep const& maxMantissa, + MantissaRange::CuspRoundingFix cuspRoundingFix) + : minMantissa_(minMantissa), maxMantissa_(maxMantissa), cuspRoundingFix_(cuspRoundingFix) + { + } + + explicit Guard(MantissaRange const& range) : Guard(range.min, range.max, range.cuspRoundingFix) + { + } // set & test the sign bit void @@ -194,6 +208,10 @@ public: unsigned pop() noexcept; + // if true, there are no digits in the guard, including dropped digits (xbit_) + bool + empty() const noexcept; + /** Drop a digit from the mantissa, and increment the exponent, storing the dropped digit in * this Guard. * @@ -206,28 +224,35 @@ public: void doDropDigit(T& mantissa, int& exponent) noexcept; + enum class Round { + // The result is exact. No rounding is needed. Only used if cuspRoundingFix is enabled. + Exact = -2, + // Round down. Since we use integer math, that usually means no change is needed. + // Exceptions are for when the result is between kMaxRap and kMaxRepUp (round to kMaxRep), + // or after subtraction where _any_ remainder will modify the result. The latter is what + // distinguishes Exact from Down. + Down = -1, + // The result was exactly half-way between two integers. This will round to even. + Even = 0, + // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not enabled) + Up = 1, + }; + // Indicate round direction: 1 is up, -1 is down, 0 is even // This enables the client to round towards nearest, and on // tie, round towards even. - [[nodiscard]] int + [[nodiscard]] Round round() const noexcept; // Modify the result to the correctly rounded value template void - doRoundUp( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa, - internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, - std::string location); + doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location); // Modify the result to the correctly rounded value template void - doRoundDown(bool& negative, T& mantissa, int& exponent, internalrep const& minMantissa); + doRoundDown(bool& negative, T& mantissa, int& exponent); // Modify the result to the correctly rounded value void @@ -239,7 +264,7 @@ private: template void - bringIntoRange(bool& negative, T& mantissa, int& exponent, internalrep const& minMantissa); + bringIntoRange(bool& negative, T& mantissa, int& exponent); }; inline void @@ -289,6 +314,12 @@ Number::Guard::pop() noexcept return d; } +inline bool +Number::Guard::empty() const noexcept +{ + return digits_ == 0 && !xbit_; +} + template void Number::Guard::doDropDigit(T& mantissa, int& exponent) noexcept @@ -314,54 +345,56 @@ Number::Guard::doDropDigit(uint128_t& mantissa, int& exponent) noexce // -1 if Guard is less than half // 0 if Guard is exactly half // 1 if Guard is greater than half -int +Number::Guard::Round Number::Guard::round() const noexcept { auto mode = Number::getround(); + if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled && empty()) + { + // No remainder + return Round::Exact; + } + if (mode == RoundingMode::TowardsZero) - return -1; + return Round::Down; if (mode == RoundingMode::Downward) { if (sbit_) { if (digits_ > 0 || xbit_) - return 1; + return Round::Up; } - return -1; + return Round::Down; } if (mode == RoundingMode::Upward) { if (sbit_) - return -1; + return Round::Down; if (digits_ > 0 || xbit_) - return 1; - return -1; + return Round::Up; + return Round::Down; } // assume round to nearest if mode is not one of the predefined values if (digits_ > 0x5000'0000'0000'0000) - return 1; + return Round::Up; if (digits_ < 0x5000'0000'0000'0000) - return -1; + return Round::Down; if (xbit_) - return 1; - return 0; + return Round::Up; + return Round::Even; } template void -Number::Guard::bringIntoRange( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa) +Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) { // Bring mantissa back into the minMantissa / maxMantissa range AFTER // rounding - if (mantissa < minMantissa) + if (mantissa < minMantissa_) { mantissa *= 10; --exponent; @@ -378,22 +411,15 @@ Number::Guard::bringIntoRange( template void -Number::Guard::doRoundUp( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa, - internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, - std::string location) +Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location) { auto r = round(); - if (r == 1 || (r == 0 && (mantissa & 1) == 1)) + if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { - auto const safeToIncrement = [&maxMantissa](auto const& mantissa) { - return mantissa < maxMantissa && mantissa < kMaxRep; + auto const safeToIncrement = [this](auto const& mantissa) { + return mantissa < maxMantissa_ && mantissa < kMaxRep; }; - if (cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix_ == MantissaRange::CuspRoundingFix::Enabled) { // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". @@ -414,14 +440,7 @@ Number::Guard::doRoundUp( safeToIncrement(mantissa), "xrpl::Number::Guard::doRoundUp", "can't recurse more than once"); - doRoundUp( - negative, - mantissa, - exponent, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - location); + doRoundUp(negative, mantissa, exponent, location); return; } } @@ -432,7 +451,7 @@ Number::Guard::doRoundUp( ++mantissa; // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". - if (mantissa > maxMantissa || mantissa > kMaxRep) + if (mantissa > maxMantissa_ || mantissa > kMaxRep) { // Don't use doDropDigit here mantissa /= 10; @@ -440,30 +459,38 @@ Number::Guard::doRoundUp( } } } - bringIntoRange(negative, mantissa, exponent, minMantissa); + bringIntoRange(negative, mantissa, exponent); if (exponent > kMaxExponent) Throw(std::string(location)); } template void -Number::Guard::doRoundDown( - bool& negative, - T& mantissa, - int& exponent, - internalrep const& minMantissa) +Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); - if (r == 1 || (r == 0 && (mantissa & 1) == 1)) + if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled) { - --mantissa; - if (mantissa < minMantissa) + // If there was any remainder, subtract 1 from the result. This is sufficient to get the + // best rounding. + if (r != Round::Exact) { - mantissa *= 10; - --exponent; + --mantissa; } } - bringIntoRange(negative, mantissa, exponent, minMantissa); + else + { + if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) + { + --mantissa; + if (mantissa < minMantissa_) + { + mantissa *= 10; + --exponent; + } + } + } + bringIntoRange(negative, mantissa, exponent); } // Modify the result to the correctly rounded value @@ -471,7 +498,7 @@ void Number::Guard::doRound(rep& drops, std::string location) const { auto r = round(); - if (r == 1 || (r == 0 && (drops & 1) == 1)) + if (r == Round::Up || (r == Round::Even && (drops & 1) == 1)) { if (drops >= kMaxRep) { @@ -530,7 +557,7 @@ doNormalize( int& exponent, MantissaRange::rep const& minMantissa, MantissaRange::rep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled, + MantissaRange::CuspRoundingFix cuspRoundingFix, bool dropped) { static constexpr auto kMinExponent = Number::kMinExponent; @@ -553,7 +580,7 @@ doNormalize( m *= 10; --exponent; } - Guard g; + Guard g(minMantissa, maxMantissa, cuspRoundingFix); if (negative) g.setNegative(); if (dropped) @@ -598,14 +625,7 @@ doNormalize( XRPL_ASSERT_PARTS(m <= kMaxRep, "xrpl::doNormalize", "intermediate mantissa fits in int64"); mantissa = m; - g.doRoundUp( - negative, - mantissa, - exponent, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::normalize 2"); + g.doRoundUp(negative, mantissa, exponent, "Number::normalize 2"); XRPL_ASSERT_PARTS( mantissa >= minMantissa && mantissa <= maxMantissa, "xrpl::doNormalize", @@ -620,13 +640,12 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFix) { // Not used by every compiler version, and thus not necessarily // counted by coverage build // LCOV_EXCL_START - doNormalize( - negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); // LCOV_EXCL_STOP } @@ -638,13 +657,12 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFix) { // Not used by every compiler version, and thus not necessarily // counted by coverage build // LCOV_EXCL_START - doNormalize( - negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); // LCOV_EXCL_STOP } @@ -656,16 +674,27 @@ Number::normalize( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, - MantissaRange::CuspRoundingFix cuspRoundingFixEnabled) + MantissaRange::CuspRoundingFix cuspRoundingFix) { - doNormalize( - negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled, false); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFix, false); } void Number::normalize(MantissaRange const& range) { - normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFixEnabled); + normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFix); +} + +void +Number::normalize(Guard const& guard) +{ + normalize( + negative_, + mantissa_, + exponent_, + guard.minMantissa_, + guard.maxMantissa_, + guard.cuspRoundingFix_); } // Copy the number, but set a new exponent. Because the mantissa doesn't change, @@ -719,7 +748,16 @@ Number::operator+=(Number const& y) bool const yn = y.negative_; uint128_t ym = y.mantissa_; auto ye = y.exponent_; - Guard g; + Guard g(kRange); + + auto const& minMantissa = g.minMantissa_; + auto const& maxMantissa = g.maxMantissa_; + auto const cuspRoundingFix = g.cuspRoundingFix_; + + // Bring the exponents of both values into agreement, so the mantissas are on the same scale + // and can be added directly together. + // Shrink the mantissa and bring the exponent up of the value with the lower exponent. Store any + // dropped digits in the Guard. if (xe < ye) { if (xn) @@ -739,11 +777,6 @@ Number::operator+=(Number const& y) } while (xe > ye); } - auto const& range = kRange.get(); - auto const& minMantissa = range.min; - auto const& maxMantissa = range.max; - auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; - if (xn == yn) { xm += ym; @@ -751,14 +784,7 @@ Number::operator+=(Number const& y) { g.doDropDigit(xm, xe); } - g.doRoundUp( - xn, - xm, - xe, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::addition overflow"); + g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } else { @@ -772,19 +798,40 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - while (xm < minMantissa && xm * 10 <= kMaxRep) + if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) { - xm *= 10; - xm -= g.pop(); - --xe; + // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than + // maxMantissa, so that normalize will have enough information to make an accurate + // rounding decision, but stop if the Guard empties out, because no rounding will be + // necessary. (Normalize will pad it back into range.) Note that if any digits were lost + // (xbit), the Guard will never be empty, so xm will get big. + auto const upperLimit = static_cast(minMantissa) * 1000; + while (xm < upperLimit && !g.empty()) + { + xm *= 10; + xm -= g.pop(); + --xe; + } } - g.doRoundDown(xn, xm, xe, minMantissa); + else + { + // Grow xm/xe and pull digits out of the Guard until it's back in range. + while (xm < minMantissa && xm * 10 <= kMaxRep) + { + xm *= 10; + xm -= g.pop(); + --xe; + } + } + // Round down, based on whether there is any data left in the Guard (depending on + // cuspRoundingFix) + g.doRoundDown(xn, xm, xe); } + doNormalize(xn, xm, xe, minMantissa, maxMantissa, cuspRoundingFix, false); negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe; - normalize(range); return *this; } @@ -818,14 +865,11 @@ Number::operator*=(Number const& y) auto ze = xe + ye; auto zs = xs * ys; bool zn = (zs == -1); - Guard g; + Guard g(kRange); if (zn) g.setNegative(); - auto const& range = kRange.get(); - auto const& minMantissa = range.min; - auto const& maxMantissa = range.max; - auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; + auto const& maxMantissa = g.maxMantissa_; while (zm > maxMantissa || zm > kMaxRep) { @@ -834,19 +878,12 @@ Number::operator*=(Number const& y) xm = static_cast(zm); xe = ze; - g.doRoundUp( - zn, - xm, - xe, - minMantissa, - maxMantissa, - cuspRoundingFixEnabled, - "Number::multiplication overflow : exponent is " + std::to_string(xe)); + g.doRoundUp(zn, xm, xe, "Number::multiplication overflow : exponent is " + std::to_string(xe)); negative_ = zn; mantissa_ = xm; exponent_ = xe; - normalize(range); + normalize(g); return *this; } @@ -882,7 +919,7 @@ Number::operator/=(Number const& y) auto const& range = kRange.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; - auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; + auto const cuspRoundingFix = range.cuspRoundingFix; // Division operates on two large integers (16-digit for small // mantissas, 19-digit for large) using integer math. If the values @@ -1014,14 +1051,14 @@ Number::operator/=(Number const& y) // rounding fix is enabled, flag if there is still // a remainder from stage 2. bool const useTrailingRemainder = - cuspRoundingFixEnabled == MantissaRange::CuspRoundingFix::Enabled; + cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled; if (useTrailingRemainder) { dropped = partialNumerator % dm != 0; } } } - doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled, dropped); + doNormalize(zp, zm, ze, minMantissa, maxMantissa, cuspRoundingFix, dropped); negative_ = zp; mantissa_ = static_cast(zm); exponent_ = ze; @@ -1035,7 +1072,7 @@ operator rep() const { rep drops = mantissa(); int offset = exponent(); - Guard g; + Guard g(kRange); if (drops != 0) { if (negative_) From 6cc45297d7f0caba2060040cb9746e0b951057fe Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 6 Jun 2026 15:37:03 -0400 Subject: [PATCH 13/23] Fix formatting, add an assert --- src/libxrpl/basics/Number.cpp | 3 +++ src/test/basics/Number_test.cpp | 38 +++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index d43eebc314..9fc464f487 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -473,6 +473,9 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { // If there was any remainder, subtract 1 from the result. This is sufficient to get the // best rounding. + XRPL_ASSERT( + empty() || mantissa > maxMantissa_, + "xrpl::Number::Guard::doRoundDown : mantissa is expected size"); if (r != Round::Exact) { --mantissa; diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 7a003a372d..9bb8a3815d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2077,14 +2077,19 @@ public: case MantissaRange::MantissaScale::Small: case MantissaRange::MantissaScale::LargeLegacy: { // Without the fix, all the results but one round up - for (auto const& [r, sum] : sums) { - if (r == Number::RoundingMode::Downward) { - // Downward works because the Guard sign is negative, and Downward returns Up - // instead of Down if negative and there's a remainder, whereas TowardsZero - // always returns Down. - BEAST_EXPECTS(sums.at(Number::RoundingMode::Downward).first < exact, to_string(r)); + for (auto const& [r, sum] : sums) + { + if (r == Number::RoundingMode::Downward) + { + // Downward works because the Guard sign is negative, and Downward + // returns Up instead of Down if negative and there's a remainder, + // whereas TowardsZero always returns Down. + BEAST_EXPECTS( + sums.at(Number::RoundingMode::Downward).first < exact, + to_string(r)); } - else { + else + { BEAST_EXPECTS(sums.at(r).first > exact, to_string(r)); } } @@ -2154,14 +2159,19 @@ public: { case MantissaRange::MantissaScale::Small: case MantissaRange::MantissaScale::LargeLegacy: { - for (auto const& [r, sum] : sums) { - if (r == Number::RoundingMode::Downward) { - // Downward works because the Guard sign is negative, and Downward returns Up - // instead of Down if negative and there's a remainder, whereas TowardsZero - // always returns Down. - BEAST_EXPECTS(sums.at(Number::RoundingMode::Downward).first < exact, to_string(r)); + for (auto const& [r, sum] : sums) + { + if (r == Number::RoundingMode::Downward) + { + // Downward works because the Guard sign is negative, and Downward + // returns Up instead of Down if negative and there's a remainder, + // whereas TowardsZero always returns Down. + BEAST_EXPECTS( + sums.at(Number::RoundingMode::Downward).first < exact, + to_string(r)); } - else { + else + { BEAST_EXPECTS(sums.at(r).first > exact, to_string(r)); } } From 07ae5fa8671536ab16e56647b2eb8f59d7917ec7 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sat, 6 Jun 2026 16:19:25 -0400 Subject: [PATCH 14/23] Reorganize the subtraction tests --- src/test/basics/Number_test.cpp | 40 ++++++++++++++++----------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 9bb8a3815d..ee61087001 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2072,45 +2072,43 @@ public: } log.flush(); - switch (scale) + for (auto const& [r, sum] : sums) { - case MantissaRange::MantissaScale::Small: - case MantissaRange::MantissaScale::LargeLegacy: { - // Without the fix, all the results but one round up - for (auto const& [r, sum] : sums) - { + auto const epsilon = pow10(sum.second.exponent()); + auto diff = sum.first - exact; + auto const rLabel = to_string(r); + switch (scale) + { + case MantissaRange::MantissaScale::Small: + case MantissaRange::MantissaScale::LargeLegacy: { + // Without the fix, all the results but one round up if (r == Number::RoundingMode::Downward) { // Downward works because the Guard sign is negative, and Downward // returns Up instead of Down if negative and there's a remainder, // whereas TowardsZero always returns Down. - BEAST_EXPECTS( - sums.at(Number::RoundingMode::Downward).first < exact, - to_string(r)); + BEAST_EXPECTS(sum.first < exact, rLabel); + BEAST_EXPECTS(diff == -(epsilon - 1), rLabel); } else { - BEAST_EXPECTS(sums.at(r).first > exact, to_string(r)); + BEAST_EXPECTS(sum.first > exact, rLabel); + BEAST_EXPECTS(diff == 1, rLabel); } + break; } - break; - } - default: { - for (auto const& [r, sum] : sums) - { - auto const epsilon = pow10(sum.second.exponent()); + default: { BEAST_EXPECT(epsilon == 100); - auto diff = sum.first - exact; switch (r) { case Number::RoundingMode::Upward: case Number::RoundingMode::ToNearest: - BEAST_EXPECTS(sum.first > exact, to_string(r)); - BEAST_EXPECTS(diff < epsilon, to_string(r)); + BEAST_EXPECTS(sum.first > exact, rLabel); + BEAST_EXPECTS(diff == 1, rLabel); break; default: - BEAST_EXPECTS(sum.first < exact, to_string(r)); - BEAST_EXPECTS(-diff < epsilon, to_string(r)); + BEAST_EXPECTS(sum.first < exact, rLabel); + BEAST_EXPECTS(diff == -(epsilon - 1), rLabel); } } } From 1162ccf7f4b4bf03674e07cbb4904bb8a853b172 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 8 Jun 2026 19:00:24 -0400 Subject: [PATCH 15/23] Clean up tests --- src/libxrpl/basics/Number.cpp | 11 +- src/test/basics/Number_test.cpp | 257 +++++++++++++------------------- 2 files changed, 109 insertions(+), 159 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 9fc464f487..9e61cdc76d 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -474,7 +474,7 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) // If there was any remainder, subtract 1 from the result. This is sufficient to get the // best rounding. XRPL_ASSERT( - empty() || mantissa > maxMantissa_, + r == Round::Exact || mantissa > maxMantissa_, "xrpl::Number::Guard::doRoundDown : mantissa is expected size"); if (r != Round::Exact) { @@ -759,7 +759,7 @@ Number::operator+=(Number const& y) // Bring the exponents of both values into agreement, so the mantissas are on the same scale // and can be added directly together. - // Shrink the mantissa and bring the exponent up of the value with the lower exponent. Store any + // Shrink the mantissa and raise the exponent of the value with the lower exponent. Store any // dropped digits in the Guard. if (xe < ye) { @@ -801,13 +801,13 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled) { // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than // maxMantissa, so that normalize will have enough information to make an accurate // rounding decision, but stop if the Guard empties out, because no rounding will be // necessary. (Normalize will pad it back into range.) Note that if any digits were lost - // (xbit), the Guard will never be empty, so xm will get big. + // (xbit), the Guard will never be empty, so xm will get larger than upperLimit. auto const upperLimit = static_cast(minMantissa) * 1000; while (xm < upperLimit && !g.empty()) { @@ -818,7 +818,8 @@ Number::operator+=(Number const& y) } else { - // Grow xm/xe and pull digits out of the Guard until it's back in range. + // Grow xm/xe and pull digits out of the Guard until it's back in the + // minMantissa/maxMantissa range. while (xm < minMantissa && xm * 10 <= kMaxRep) { xm *= 10; diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index ee61087001..aabef90446 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -2024,172 +2024,121 @@ public: } { - testcase << "subtraction rounding " << to_string(scale); - auto const exp = Number::mantissaLog(); - Number const a{1LL, exp + 2}; - Number const b{-(Number{1, exp} + 1)}; - - if (scale == MantissaRange::MantissaScale::Small) + // SubCase is + // * offset: offset from exp + // * extraB: whether to include 1e"exp" in "b" + // * aString: expected string value for "a" + // * bString: expected string value for "b" + // There aren't too many valid combinations for test cases here. If extraB is true, + // offset can really only be 2, because any larger and the mantissa can't be represented + // without loss. Offset can't be less than 2, or there's no error. + using SubCase = std::tuple; + auto const c = std::to_array({ + {2, + true, + scale == MantissaRange::MantissaScale::Small ? "100000000000000000" + : "100000000000000000000", + scale == MantissaRange::MantissaScale::Small ? "-1000000000000001" + : "-1000000000000000001"}, + {2, + false, + scale == MantissaRange::MantissaScale::Small ? "100000000000000000" + : "100000000000000000000", + "-1"}, + {30, + false, + scale == MantissaRange::MantissaScale::Small + ? "1000000000000000000000000000000000000000000000" + : "1000000000000000000000000000000000000000000000000", + "-1"}, + }); + for (auto const& [offset, extraB, aString, bString] : c) { - BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000"}); - BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000001"}); - } - else - { - BEAST_EXPECT(toBigInt(a) == BigInt{"100000000000000000000"}); - BEAST_EXPECT(toBigInt(b) == BigInt{"-1000000000000000001"}); - } + testcase << "subtraction rounding. offset: " << offset + << ", scale: " << to_string(scale); - auto construct = [&a, &b, this](Number::RoundingMode r) { - NumberRoundModeGuard const roundGuard{r}; - auto const sum = a + b; - BigInt const stored = toBigInt(sum); - return std::make_pair(r, std::make_pair(stored, sum)); - }; + Number const a{1LL, exp + offset}; + Number const b{-((extraB ? Number{1, exp} : kNumZero) + 1)}; - auto const bigA = toBigInt(a); - auto const bigB = toBigInt(b); - BigInt const exact = bigA + bigB; + auto const bigA = toBigInt(a); + auto const bigB = toBigInt(b); - auto const sums = [&]() { - std::map> sums; - sums.emplace(construct(Number::RoundingMode::TowardsZero)); - sums.emplace(construct(Number::RoundingMode::Upward)); - sums.emplace(construct(Number::RoundingMode::Downward)); - sums.emplace(construct(Number::RoundingMode::ToNearest)); - return sums; - }(); + BEAST_EXPECT(bigA == BigInt{aString}); + BEAST_EXPECT(bigB == BigInt{bString}); - log << "\n a = " << a << " (" << fmt(bigA) << ")\n b = " << b - << " (" << fmt(bigB) << ")\n exact a + b = " << fmt(exact) << "\n"; - for (auto const& [r, sum] : sums) - { - auto const diff = sum.first - exact; - auto const rLabel = to_string(r); - log << std::string(15 - rLabel.length(), ' ') << rLabel << " = " << fmt(sum.first) - << "\n difference = " << fmt(diff) << "\n"; - } - log.flush(); + auto construct = [&a, &b, this](Number::RoundingMode r) { + NumberRoundModeGuard const roundGuard{r}; + auto const sum = a + b; + BigInt const stored = toBigInt(sum); + return std::make_pair(r, std::make_pair(stored, sum)); + }; - for (auto const& [r, sum] : sums) - { - auto const epsilon = pow10(sum.second.exponent()); - auto diff = sum.first - exact; - auto const rLabel = to_string(r); - switch (scale) + BigInt const exact = bigA + bigB; + + auto const sums = [&]() { + std::map> sums; + sums.emplace(construct(Number::RoundingMode::TowardsZero)); + sums.emplace(construct(Number::RoundingMode::Upward)); + sums.emplace(construct(Number::RoundingMode::Downward)); + sums.emplace(construct(Number::RoundingMode::ToNearest)); + return sums; + }(); + + log << "\n a = " << a << " (" << fmt(bigA) + << ")\n b = " << b << " (" << fmt(bigB) + << ")\n exact a + b = " << fmt(exact) << "\n"; + for (auto const& [r, sum] : sums) { - case MantissaRange::MantissaScale::Small: - case MantissaRange::MantissaScale::LargeLegacy: { - // Without the fix, all the results but one round up - if (r == Number::RoundingMode::Downward) - { - // Downward works because the Guard sign is negative, and Downward - // returns Up instead of Down if negative and there's a remainder, - // whereas TowardsZero always returns Down. - BEAST_EXPECTS(sum.first < exact, rLabel); - BEAST_EXPECTS(diff == -(epsilon - 1), rLabel); - } - else - { - BEAST_EXPECTS(sum.first > exact, rLabel); - BEAST_EXPECTS(diff == 1, rLabel); - } - break; - } - default: { - BEAST_EXPECT(epsilon == 100); - switch (r) - { - case Number::RoundingMode::Upward: - case Number::RoundingMode::ToNearest: - BEAST_EXPECTS(sum.first > exact, rLabel); - BEAST_EXPECTS(diff == 1, rLabel); - break; - default: + auto const diff = sum.first - exact; + auto const rLabel = to_string(r); + log << std::string(15 - rLabel.length(), ' ') << rLabel << " = " + << fmt(sum.first) << "\n difference = " << fmt(diff) << "\n"; + } + log.flush(); + + auto const expectedExponent = + offset - (scale == MantissaRange::MantissaScale::Small && extraB ? 1 : 0); + auto const epsilon = pow10(expectedExponent); + for (auto const& [r, sum] : sums) + { + auto diff = sum.first - exact; + auto const rLabel = to_string(r); + switch (scale) + { + case MantissaRange::MantissaScale::Small: + case MantissaRange::MantissaScale::LargeLegacy: { + // Without the fix, all the results but one round up + if (r == Number::RoundingMode::Downward) + { + // Downward works because the Guard sign is negative, and Downward + // returns Up instead of Down if negative and there's a remainder, + // whereas TowardsZero always returns Down. BEAST_EXPECTS(sum.first < exact, rLabel); BEAST_EXPECTS(diff == -(epsilon - 1), rLabel); + } + else + { + BEAST_EXPECTS(sum.first > exact, rLabel); + BEAST_EXPECTS(diff == 1, rLabel); + } + break; } - } - } - } - } - { - auto const offset = 30; - testcase << "subtraction rounding offset of " << offset << " " << to_string(scale); - - auto const exp = Number::mantissaLog(); - Number const a{1LL, exp + offset}; - Number const b{-1}; - - auto construct = [&a, &b, this](Number::RoundingMode r) { - NumberRoundModeGuard const roundGuard{r}; - auto const sum = a + b; - BigInt const stored = toBigInt(sum); - return std::make_pair(r, std::make_pair(stored, sum)); - }; - - auto const bigA = toBigInt(a); - auto const bigB = toBigInt(b); - BigInt const exact = bigA + bigB; - - auto const sums = [&]() { - std::map> sums; - sums.emplace(construct(Number::RoundingMode::TowardsZero)); - sums.emplace(construct(Number::RoundingMode::Upward)); - sums.emplace(construct(Number::RoundingMode::Downward)); - sums.emplace(construct(Number::RoundingMode::ToNearest)); - return sums; - }(); - - log << "\n a = " << a << " (" << fmt(bigA) << ")\n b = " << b - << " (" << fmt(bigB) << ")\n exact a + b = " << fmt(exact) << "\n"; - for (auto const& [r, sum] : sums) - { - auto const diff = sum.first - exact; - auto const rLabel = to_string(r); - log << std::string(15 - rLabel.length(), ' ') << rLabel << " = " << fmt(sum.first) - << "\n difference = " << fmt(diff) << "\n"; - } - log.flush(); - - switch (scale) - { - case MantissaRange::MantissaScale::Small: - case MantissaRange::MantissaScale::LargeLegacy: { - for (auto const& [r, sum] : sums) - { - if (r == Number::RoundingMode::Downward) - { - // Downward works because the Guard sign is negative, and Downward - // returns Up instead of Down if negative and there's a remainder, - // whereas TowardsZero always returns Down. + default: { BEAST_EXPECTS( - sums.at(Number::RoundingMode::Downward).first < exact, - to_string(r)); - } - else - { - BEAST_EXPECTS(sums.at(r).first > exact, to_string(r)); - } - } - break; - } - default: { - for (auto const& [r, sum] : sums) - { - auto const epsilon = pow10(sum.second.exponent()); - auto diff = sum.first - exact; - switch (r) - { - case Number::RoundingMode::Upward: - case Number::RoundingMode::ToNearest: - BEAST_EXPECTS(sum.first > exact, to_string(r)); - BEAST_EXPECTS(diff < epsilon, to_string(r)); - break; - default: - BEAST_EXPECTS(sum.first < exact, to_string(r)); - BEAST_EXPECTS(-diff < epsilon, to_string(r)); + sum.second.exponent() <= expectedExponent, + to_string(sum.second.exponent())); + switch (r) + { + case Number::RoundingMode::Upward: + case Number::RoundingMode::ToNearest: + BEAST_EXPECTS(sum.first > exact, rLabel); + BEAST_EXPECTS(diff == 1, rLabel); + break; + default: + BEAST_EXPECTS(sum.first < exact, rLabel); + BEAST_EXPECTS(diff == -(epsilon - 1), rLabel); + } } } } From 693e9015ab3cda4718bd96f27fe88112671ac327 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 8 Jun 2026 19:03:53 -0400 Subject: [PATCH 16/23] clang-tidy: Guard public member variable names; Missing include --- src/libxrpl/basics/Number.cpp | 40 +++++++++++++++++------------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 9e61cdc76d..9386b411d0 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -171,15 +171,15 @@ class Number::Guard std::uint8_t sbit_ : 1 {0}; // the sign of the guard digits public: - internalrep const minMantissa_; - internalrep const maxMantissa_; - MantissaRange::CuspRoundingFix const cuspRoundingFix_; + internalrep const minMantissa; + internalrep const maxMantissa; + MantissaRange::CuspRoundingFix const cuspRoundingFix; explicit Guard( internalrep const& minMantissa, internalrep const& maxMantissa, MantissaRange::CuspRoundingFix cuspRoundingFix) - : minMantissa_(minMantissa), maxMantissa_(maxMantissa), cuspRoundingFix_(cuspRoundingFix) + : minMantissa(minMantissa), maxMantissa(maxMantissa), cuspRoundingFix(cuspRoundingFix) { } @@ -209,7 +209,7 @@ public: pop() noexcept; // if true, there are no digits in the guard, including dropped digits (xbit_) - bool + [[nodiscard]] bool empty() const noexcept; /** Drop a digit from the mantissa, and increment the exponent, storing the dropped digit in @@ -350,7 +350,7 @@ Number::Guard::round() const noexcept { auto mode = Number::getround(); - if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled && empty()) + if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && empty()) { // No remainder return Round::Exact; @@ -394,7 +394,7 @@ Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) { // Bring mantissa back into the minMantissa / maxMantissa range AFTER // rounding - if (mantissa < minMantissa_) + if (mantissa < minMantissa) { mantissa *= 10; --exponent; @@ -417,9 +417,9 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { auto const safeToIncrement = [this](auto const& mantissa) { - return mantissa < maxMantissa_ && mantissa < kMaxRep; + return mantissa < maxMantissa && mantissa < kMaxRep; }; - if (cuspRoundingFix_ == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) { // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". @@ -451,7 +451,7 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string ++mantissa; // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". - if (mantissa > maxMantissa_ || mantissa > kMaxRep) + if (mantissa > maxMantissa || mantissa > kMaxRep) { // Don't use doDropDigit here mantissa /= 10; @@ -469,12 +469,12 @@ void Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); - if (cuspRoundingFix_ != MantissaRange::CuspRoundingFix::Disabled) + if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled) { // If there was any remainder, subtract 1 from the result. This is sufficient to get the // best rounding. XRPL_ASSERT( - r == Round::Exact || mantissa > maxMantissa_, + r == Round::Exact || mantissa > maxMantissa, "xrpl::Number::Guard::doRoundDown : mantissa is expected size"); if (r != Round::Exact) { @@ -486,7 +486,7 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1)) { --mantissa; - if (mantissa < minMantissa_) + if (mantissa < minMantissa) { mantissa *= 10; --exponent; @@ -695,9 +695,9 @@ Number::normalize(Guard const& guard) negative_, mantissa_, exponent_, - guard.minMantissa_, - guard.maxMantissa_, - guard.cuspRoundingFix_); + guard.minMantissa, + guard.maxMantissa, + guard.cuspRoundingFix); } // Copy the number, but set a new exponent. Because the mantissa doesn't change, @@ -753,9 +753,9 @@ Number::operator+=(Number const& y) auto ye = y.exponent_; Guard g(kRange); - auto const& minMantissa = g.minMantissa_; - auto const& maxMantissa = g.maxMantissa_; - auto const cuspRoundingFix = g.cuspRoundingFix_; + auto const& minMantissa = g.minMantissa; + auto const& maxMantissa = g.maxMantissa; + auto const cuspRoundingFix = g.cuspRoundingFix; // Bring the exponents of both values into agreement, so the mantissas are on the same scale // and can be added directly together. @@ -873,7 +873,7 @@ Number::operator*=(Number const& y) if (zn) g.setNegative(); - auto const& maxMantissa = g.maxMantissa_; + auto const& maxMantissa = g.maxMantissa; while (zm > maxMantissa || zm > kMaxRep) { From 2e97056b4073dee6769c314f71ff932bf1e7ff09 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 9 Jun 2026 16:43:24 -0400 Subject: [PATCH 17/23] Update to use a new amendment, since this PR will not be part of 3.2.0 - This requires creating yet another MantissaScale, and CuspRoundingFix option. --- include/xrpl/basics/Number.h | 30 ++++++++++------ include/xrpl/protocol/detail/features.macro | 2 ++ src/libxrpl/basics/Number.cpp | 38 ++++++++++++++------- src/libxrpl/protocol/Rules.cpp | 35 +++++++++++-------- src/libxrpl/protocol/STNumber.cpp | 5 +-- src/test/app/Invariants_test.cpp | 7 ++-- src/test/basics/Number_test.cpp | 21 ++++++------ 7 files changed, 83 insertions(+), 55 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 4df4a79835..fed79b78fe 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -130,13 +130,16 @@ struct MantissaRange final Small, // LargeLegacy can be removed when fixCleanup3_2_0 is retired LargeLegacy, - Large, + // Large3_2_0 can be removed when fixCleanup3_3_0 is retired + Large3_2_0, + LargeNew, // TODO: Convert this back to Large after conversion is done }; // This entire enum can be removed when fixCleanup3_2_0 is retired - enum class CuspRoundingFix : bool { - Disabled = false, - Enabled = true, + enum class CuspRoundingFix : std::uint8_t { + Disabled = 0, + Enabled3_2_0 = 1, + EnabledNew = 2, // TODO: Convert this back to Enabled after conversion is done }; explicit constexpr MantissaRange(MantissaScale sc) : scale(sc) @@ -164,7 +167,8 @@ private: case MantissaScale::Small: return 15; case MantissaScale::LargeLegacy: - case MantissaScale::Large: + case MantissaScale::Large3_2_0: + case MantissaScale::LargeNew: return 18; // LCOV_EXCL_START default: @@ -193,8 +197,10 @@ private: case MantissaScale::Small: case MantissaScale::LargeLegacy: return CuspRoundingFix::Disabled; - case MantissaScale::Large: - return CuspRoundingFix::Enabled; + case MantissaScale::Large3_2_0: + return CuspRoundingFix::Enabled3_2_0; + case MantissaScale::LargeNew: + return CuspRoundingFix::EnabledNew; default: // If called in a constexpr context, this throw assures that the build fails if an // invalid scale is used. @@ -874,11 +880,13 @@ to_string(MantissaRange::MantissaScale const& scale) switch (scale) { case MantissaRange::MantissaScale::Small: - return "small"; + return "Small"; case MantissaRange::MantissaScale::LargeLegacy: - return "largeLegacy"; - case MantissaRange::MantissaScale::Large: - return "large"; + return "LargeLegacy"; + case MantissaRange::MantissaScale::Large3_2_0: + return "Large320"; + case MantissaRange::MantissaScale::LargeNew: + return "Large"; default: throw std::runtime_error("Bad scale"); } diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index c99c1e5ce8..f83442926d 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -15,6 +15,8 @@ // Add new amendments to the top of this list. // Keep it sorted in reverse chronological order. +// The name "NumberStuff" is a placeholder +XRPL_FIX (NumberStuff, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_2_0, Supported::Yes, VoteBehavior::DefaultNo) XRPL_FEATURE(MPTokensV2, Supported::No, VoteBehavior::DefaultNo) XRPL_FIX (Cleanup3_1_3, Supported::Yes, VoteBehavior::DefaultYes) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 9386b411d0..8253e6c6ee 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -31,7 +31,7 @@ namespace xrpl { thread_local Number::RoundingMode Number::mode = Number::RoundingMode::ToNearest; thread_local std::reference_wrapper Number::kRange = - MantissaRange::getMantissaRange(MantissaRange::MantissaScale::Large); + MantissaRange::getMantissaRange(MantissaRange::MantissaScale::LargeNew); std::set const& MantissaRange::getAllScales() @@ -39,7 +39,8 @@ MantissaRange::getAllScales() static std::set const kScales = { MantissaRange::MantissaScale::Small, MantissaRange::MantissaScale::LargeLegacy, - MantissaRange::MantissaScale::Large, + MantissaRange::MantissaScale::Large3_2_0, + MantissaRange::MantissaScale::LargeNew, }; return kScales; } @@ -80,14 +81,25 @@ MantissaRange::getRanges() } { [[maybe_unused]] - constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large}; + constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large3_2_0}; static_assert(isPowerOfTen(kRange.min)); static_assert(kRange.min == 1'000'000'000'000'000'000ULL); static_assert(kRange.max == rep(9'999'999'999'999'999'999ULL)); static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled3_2_0); + } + { + [[maybe_unused]] + constexpr static MantissaRange kRange{MantissaRange::MantissaScale::LargeNew}; + static_assert(isPowerOfTen(kRange.min)); + static_assert(kRange.min == 1'000'000'000'000'000'000ULL); + static_assert(kRange.max == rep(9'999'999'999'999'999'999ULL)); + static_assert(kRange.log == 18); + static_assert(kRange.min < Number::kMaxRep); + static_assert(kRange.max > Number::kMaxRep); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::EnabledNew); } return map; }(); @@ -225,7 +237,7 @@ public: doDropDigit(T& mantissa, int& exponent) noexcept; enum class Round { - // The result is exact. No rounding is needed. Only used if cuspRoundingFix is enabled. + // The result is exact. No rounding is needed. Only used if cuspRoundingFix is EnabledNew. Exact = -2, // Round down. Since we use integer math, that usually means no change is needed. // Exceptions are for when the result is between kMaxRap and kMaxRepUp (round to kMaxRep), @@ -234,7 +246,8 @@ public: Down = -1, // The result was exactly half-way between two integers. This will round to even. Even = 0, - // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not enabled) + // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not + // EnabledNew) Up = 1, }; @@ -350,7 +363,7 @@ Number::Guard::round() const noexcept { auto mode = Number::getround(); - if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled && empty()) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::EnabledNew && empty()) { // No remainder return Round::Exact; @@ -419,7 +432,7 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string auto const safeToIncrement = [this](auto const& mantissa) { return mantissa < maxMantissa && mantissa < kMaxRep; }; - if (cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled) { // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". @@ -469,7 +482,7 @@ void Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); - if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::EnabledNew) { // If there was any remainder, subtract 1 from the result. This is sufficient to get the // best rounding. @@ -801,7 +814,7 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - if (cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::EnabledNew) { // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than // maxMantissa, so that normalize will have enough information to make an accurate @@ -836,6 +849,7 @@ Number::operator+=(Number const& y) negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe; + XRPL_ASSERT(isnormal(), "xrpl::Number::operator+= : result is normal"); return *this; } @@ -971,7 +985,7 @@ Number::operator/=(Number const& y) // This is equivalent to if we had used an initial factor of 10^22, // a couple digits more than we actually need. // - // Stage 3: If there is still a remainder, and the CuspRoundingFix + // Stage 3: If there is still a remainder, and the cuspRoundingFix // is enabled, pass a flag indicating such to doNormalize. The Guard // in doNormalize will treat that flag as if non-zero digits had // been dropped from the mantissa when shrinking it into range. @@ -1055,7 +1069,7 @@ Number::operator/=(Number const& y) // rounding fix is enabled, flag if there is still // a remainder from stage 2. bool const useTrailingRemainder = - cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled; + cuspRoundingFix != MantissaRange::CuspRoundingFix::Disabled; if (useTrailingRemainder) { dropped = partialNumerator % dm != 0; diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index 08a95145eb..06f50423b8 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -40,22 +40,29 @@ setCurrentTransactionRules(std::optional r) // Push the appropriate setting, instead of having the class pull every time // the value is needed. That could get expensive fast. - // If any new conditions with new amendments are added, those amendments must also be added to - // useRulesGuards. - bool const enableVaultNumbers = - !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); - bool const enableCuspRoundingFix = !r || r->enabled(fixCleanup3_2_0); - XRPL_ASSERT( - !r || useRulesGuards(*r) == (enableCuspRoundingFix || enableVaultNumbers), - "setCurrentTransactionRules : rule decisions match"); - // Declare the range this way to keep clang-tidy from complaining - auto const range = [enableCuspRoundingFix, enableVaultNumbers]() { + auto const range = [&r]() { + // If any new conditions with new amendments are added, those amendments must also be added + // to useRulesGuards. + bool const enableVaultNumbers = + !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); + bool const enableCuspRounding3_2_0 = !r || r->enabled(fixCleanup3_2_0); + bool const enableCuspRounding3_3_0 = !r || r->enabled(fixNumberStuff); + XRPL_ASSERT( + !r || + useRulesGuards(*r) == + (enableVaultNumbers || enableCuspRounding3_2_0 || enableCuspRounding3_3_0), + "setCurrentTransactionRules : rule decisions match"); + if (enableVaultNumbers) { - if (enableCuspRoundingFix) + if (enableCuspRounding3_3_0) { - return MantissaRange::MantissaScale::Large; + return MantissaRange::MantissaScale::LargeNew; + } + if (enableCuspRounding3_2_0) + { + return MantissaRange::MantissaScale::Large3_2_0; } return MantissaRange::MantissaScale::LargeLegacy; } @@ -76,8 +83,8 @@ useRulesGuards(Rules const& rules) // As soon as any one of these amendments is retired, this whole function can be removed, along // with createGuards, and any other callers, and the first set of guards can be created directly // at the call site, without using optional. - return rules.enabled(fixCleanup3_2_0) || rules.enabled(featureSingleAssetVault) || - rules.enabled(featureLendingProtocol); + return rules.enabled(featureSingleAssetVault) || rules.enabled(featureLendingProtocol) || + rules.enabled(fixCleanup3_2_0) || rules.enabled(fixNumberStuff); } void diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index 8ef7b9760f..11cde05da2 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -88,7 +88,6 @@ STNumber::add(Serializer& s) const } else { -#if !NDEBUG // There are circumstances where an already-rounded Number is // serialized without being touched by a transactor, and thus // without an asset. We can't know if it's rounded, because it could @@ -96,11 +95,9 @@ STNumber::add(Serializer& s) const // Json. Regardless, the only time we should be serializing an // STNumber is when the scale is large. XRPL_ASSERT_PARTS( - Number::getMantissaScale() == MantissaRange::MantissaScale::LargeLegacy || - Number::getMantissaScale() == MantissaRange::MantissaScale::Large, + Number::getMantissaScale() != MantissaRange::MantissaScale::Small, "xrpl::STNumber::add", "STNumber only used with large mantissa scale"); -#endif } } diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 6d53d25661..46980a74c9 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4766,11 +4766,10 @@ class Invariants_test : public beast::unit_test::Suite std::vector values; }; - for (auto const mantissaScale : { - MantissaRange::MantissaScale::LargeLegacy, - MantissaRange::MantissaScale::Large, - }) + for (auto const mantissaScale : MantissaRange::getAllScales()) { + if (mantissaScale == MantissaRange::MantissaScale::Small) + continue; NumberMantissaScaleGuard const g{mantissaScale}; auto makeDelta = [&vaultAsset](Number const& n) -> ValidVault::DeltaInfo { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index aabef90446..24c515af1c 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1399,8 +1399,7 @@ public: "9223372036854775e3"); } break; - case MantissaRange::MantissaScale::LargeLegacy: - case MantissaRange::MantissaScale::Large: + default: // Test the edges // ((exponent < -(28)) || (exponent > -(8))))) test(Number::min(), "1e-32750"); @@ -1438,9 +1437,6 @@ public: test( -(Number{std::numeric_limits::max(), 0} + 1), "-9223372036854775810"); - break; - default: - BEAST_EXPECT(false); } } @@ -1816,7 +1812,8 @@ public: switch (scale) { - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::LargeNew: BEAST_EXPECT(signedDifference >= 0); BEAST_EXPECT(signedDifference < pow10(product.exponent())); BEAST_EXPECT( @@ -1898,7 +1895,8 @@ public: // Upward invariant: stored >= exact. Bug: stored < exact. switch (scale) { - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::LargeNew: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); break; @@ -1948,7 +1946,8 @@ public: // invariant: stored <= exact. Bug: stored > exact. switch (scale) { - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::LargeNew: BEAST_EXPECT(stored <= exact); BEAST_EXPECT(diff > -pow10(quotient.exponent())); break; @@ -2005,7 +2004,8 @@ public: // invariant: stored >= exact. Bug: stored < exact. switch (scale) { - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::LargeNew: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); break; @@ -2107,7 +2107,8 @@ public: switch (scale) { case MantissaRange::MantissaScale::Small: - case MantissaRange::MantissaScale::LargeLegacy: { + case MantissaRange::MantissaScale::LargeLegacy: + case MantissaRange::MantissaScale::Large3_2_0: { // Without the fix, all the results but one round up if (r == Number::RoundingMode::Downward) { From 182ca1c12f5ac7cc35571ccacddb871408908dfa Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 9 Jun 2026 17:06:27 -0400 Subject: [PATCH 18/23] Clean up the "New" names --- include/xrpl/basics/Number.h | 12 ++++++------ src/libxrpl/basics/Number.cpp | 18 +++++++++--------- src/libxrpl/protocol/Rules.cpp | 2 +- src/test/basics/Number_test.cpp | 8 ++++---- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index fed79b78fe..3ecfa64a02 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -132,14 +132,14 @@ struct MantissaRange final LargeLegacy, // Large3_2_0 can be removed when fixCleanup3_3_0 is retired Large3_2_0, - LargeNew, // TODO: Convert this back to Large after conversion is done + Large, }; // This entire enum can be removed when fixCleanup3_2_0 is retired enum class CuspRoundingFix : std::uint8_t { Disabled = 0, Enabled3_2_0 = 1, - EnabledNew = 2, // TODO: Convert this back to Enabled after conversion is done + Enabled = 2, }; explicit constexpr MantissaRange(MantissaScale sc) : scale(sc) @@ -168,7 +168,7 @@ private: return 15; case MantissaScale::LargeLegacy: case MantissaScale::Large3_2_0: - case MantissaScale::LargeNew: + case MantissaScale::Large: return 18; // LCOV_EXCL_START default: @@ -199,8 +199,8 @@ private: return CuspRoundingFix::Disabled; case MantissaScale::Large3_2_0: return CuspRoundingFix::Enabled3_2_0; - case MantissaScale::LargeNew: - return CuspRoundingFix::EnabledNew; + case MantissaScale::Large: + return CuspRoundingFix::Enabled; default: // If called in a constexpr context, this throw assures that the build fails if an // invalid scale is used. @@ -885,7 +885,7 @@ to_string(MantissaRange::MantissaScale const& scale) return "LargeLegacy"; case MantissaRange::MantissaScale::Large3_2_0: return "Large320"; - case MantissaRange::MantissaScale::LargeNew: + case MantissaRange::MantissaScale::Large: return "Large"; default: throw std::runtime_error("Bad scale"); diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 8253e6c6ee..234ee4e40c 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -31,7 +31,7 @@ namespace xrpl { thread_local Number::RoundingMode Number::mode = Number::RoundingMode::ToNearest; thread_local std::reference_wrapper Number::kRange = - MantissaRange::getMantissaRange(MantissaRange::MantissaScale::LargeNew); + MantissaRange::getMantissaRange(MantissaRange::MantissaScale::Large); std::set const& MantissaRange::getAllScales() @@ -40,7 +40,7 @@ MantissaRange::getAllScales() MantissaRange::MantissaScale::Small, MantissaRange::MantissaScale::LargeLegacy, MantissaRange::MantissaScale::Large3_2_0, - MantissaRange::MantissaScale::LargeNew, + MantissaRange::MantissaScale::Large, }; return kScales; } @@ -92,14 +92,14 @@ MantissaRange::getRanges() } { [[maybe_unused]] - constexpr static MantissaRange kRange{MantissaRange::MantissaScale::LargeNew}; + constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large}; static_assert(isPowerOfTen(kRange.min)); static_assert(kRange.min == 1'000'000'000'000'000'000ULL); static_assert(kRange.max == rep(9'999'999'999'999'999'999ULL)); static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::EnabledNew); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled); } return map; }(); @@ -237,7 +237,7 @@ public: doDropDigit(T& mantissa, int& exponent) noexcept; enum class Round { - // The result is exact. No rounding is needed. Only used if cuspRoundingFix is EnabledNew. + // The result is exact. No rounding is needed. Only used if cuspRoundingFix is Enabled. Exact = -2, // Round down. Since we use integer math, that usually means no change is needed. // Exceptions are for when the result is between kMaxRap and kMaxRepUp (round to kMaxRep), @@ -247,7 +247,7 @@ public: // The result was exactly half-way between two integers. This will round to even. Even = 0, // Round up. Always adds 1 (or subtracts 1 in some cases if cuspRoundingFix is not - // EnabledNew) + // Enabled) Up = 1, }; @@ -363,7 +363,7 @@ Number::Guard::round() const noexcept { auto mode = Number::getround(); - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::EnabledNew && empty()) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled && empty()) { // No remainder return Round::Exact; @@ -482,7 +482,7 @@ void Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::EnabledNew) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled) { // If there was any remainder, subtract 1 from the result. This is sufficient to get the // best rounding. @@ -814,7 +814,7 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::EnabledNew) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled) { // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than // maxMantissa, so that normalize will have enough information to make an accurate diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index 06f50423b8..c7e9431f81 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -58,7 +58,7 @@ setCurrentTransactionRules(std::optional r) { if (enableCuspRounding3_3_0) { - return MantissaRange::MantissaScale::LargeNew; + return MantissaRange::MantissaScale::Large; } if (enableCuspRounding3_2_0) { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 24c515af1c..7121948d69 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1813,7 +1813,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large3_2_0: - case MantissaRange::MantissaScale::LargeNew: + case MantissaRange::MantissaScale::Large: BEAST_EXPECT(signedDifference >= 0); BEAST_EXPECT(signedDifference < pow10(product.exponent())); BEAST_EXPECT( @@ -1896,7 +1896,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large3_2_0: - case MantissaRange::MantissaScale::LargeNew: + case MantissaRange::MantissaScale::Large: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); break; @@ -1947,7 +1947,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large3_2_0: - case MantissaRange::MantissaScale::LargeNew: + case MantissaRange::MantissaScale::Large: BEAST_EXPECT(stored <= exact); BEAST_EXPECT(diff > -pow10(quotient.exponent())); break; @@ -2005,7 +2005,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large3_2_0: - case MantissaRange::MantissaScale::LargeNew: + case MantissaRange::MantissaScale::Large: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); break; From 772e0c30f753d39b63a7b139bb23bb34bb765fa5 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 9 Jun 2026 18:38:03 -0400 Subject: [PATCH 19/23] clang-tidy: rename MantissaScale enums from "3_2_0" to "320" --- include/xrpl/basics/Number.h | 14 +++++++------- src/libxrpl/basics/Number.cpp | 6 +++--- src/libxrpl/protocol/Rules.cpp | 2 +- src/test/basics/Number_test.cpp | 10 +++++----- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 3ecfa64a02..cb04dd9716 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -130,15 +130,15 @@ struct MantissaRange final Small, // LargeLegacy can be removed when fixCleanup3_2_0 is retired LargeLegacy, - // Large3_2_0 can be removed when fixCleanup3_3_0 is retired - Large3_2_0, + // Large320 can be removed when fixCleanup3_3_0 is retired + Large320, Large, }; // This entire enum can be removed when fixCleanup3_2_0 is retired enum class CuspRoundingFix : std::uint8_t { Disabled = 0, - Enabled3_2_0 = 1, + Enabled320 = 1, Enabled = 2, }; @@ -167,7 +167,7 @@ private: case MantissaScale::Small: return 15; case MantissaScale::LargeLegacy: - case MantissaScale::Large3_2_0: + case MantissaScale::Large320: case MantissaScale::Large: return 18; // LCOV_EXCL_START @@ -197,8 +197,8 @@ private: case MantissaScale::Small: case MantissaScale::LargeLegacy: return CuspRoundingFix::Disabled; - case MantissaScale::Large3_2_0: - return CuspRoundingFix::Enabled3_2_0; + case MantissaScale::Large320: + return CuspRoundingFix::Enabled320; case MantissaScale::Large: return CuspRoundingFix::Enabled; default: @@ -883,7 +883,7 @@ to_string(MantissaRange::MantissaScale const& scale) return "Small"; case MantissaRange::MantissaScale::LargeLegacy: return "LargeLegacy"; - case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::Large320: return "Large320"; case MantissaRange::MantissaScale::Large: return "Large"; diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 234ee4e40c..133ec82692 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -39,7 +39,7 @@ MantissaRange::getAllScales() static std::set const kScales = { MantissaRange::MantissaScale::Small, MantissaRange::MantissaScale::LargeLegacy, - MantissaRange::MantissaScale::Large3_2_0, + MantissaRange::MantissaScale::Large320, MantissaRange::MantissaScale::Large, }; return kScales; @@ -81,14 +81,14 @@ MantissaRange::getRanges() } { [[maybe_unused]] - constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large3_2_0}; + constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large320}; static_assert(isPowerOfTen(kRange.min)); static_assert(kRange.min == 1'000'000'000'000'000'000ULL); static_assert(kRange.max == rep(9'999'999'999'999'999'999ULL)); static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled3_2_0); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled320); } { [[maybe_unused]] diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index c7e9431f81..15136115c7 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -62,7 +62,7 @@ setCurrentTransactionRules(std::optional r) } if (enableCuspRounding3_2_0) { - return MantissaRange::MantissaScale::Large3_2_0; + return MantissaRange::MantissaScale::Large320; } return MantissaRange::MantissaScale::LargeLegacy; } diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 7121948d69..266bc9bc1e 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1812,7 +1812,7 @@ public: switch (scale) { - case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::Large320: case MantissaRange::MantissaScale::Large: BEAST_EXPECT(signedDifference >= 0); BEAST_EXPECT(signedDifference < pow10(product.exponent())); @@ -1895,7 +1895,7 @@ public: // Upward invariant: stored >= exact. Bug: stored < exact. switch (scale) { - case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::Large320: case MantissaRange::MantissaScale::Large: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); @@ -1946,7 +1946,7 @@ public: // invariant: stored <= exact. Bug: stored > exact. switch (scale) { - case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::Large320: case MantissaRange::MantissaScale::Large: BEAST_EXPECT(stored <= exact); BEAST_EXPECT(diff > -pow10(quotient.exponent())); @@ -2004,7 +2004,7 @@ public: // invariant: stored >= exact. Bug: stored < exact. switch (scale) { - case MantissaRange::MantissaScale::Large3_2_0: + case MantissaRange::MantissaScale::Large320: case MantissaRange::MantissaScale::Large: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); @@ -2108,7 +2108,7 @@ public: { case MantissaRange::MantissaScale::Small: case MantissaRange::MantissaScale::LargeLegacy: - case MantissaRange::MantissaScale::Large3_2_0: { + case MantissaRange::MantissaScale::Large320: { // Without the fix, all the results but one round up if (r == Number::RoundingMode::Downward) { From 5c62c15ad8e4e7f13833a789446f35cefc0acc75 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 9 Jun 2026 18:44:46 -0400 Subject: [PATCH 20/23] Future proofing: Rename Large and Enabled to Large330 and Enabled330 - If more fixes need to be made in the future, they can be added after, instead of needing to do the "rename dance", I had to do with this PR. --- include/xrpl/basics/Number.h | 50 ++++++------------------------- src/libxrpl/basics/Number.cpp | 52 ++++++++++++++++++++++++++++----- src/libxrpl/protocol/Rules.cpp | 2 +- src/test/basics/Number_test.cpp | 8 ++--- 4 files changed, 59 insertions(+), 53 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index cb04dd9716..ccd448887a 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -132,14 +132,14 @@ struct MantissaRange final LargeLegacy, // Large320 can be removed when fixCleanup3_3_0 is retired Large320, - Large, + Large330, }; // This entire enum can be removed when fixCleanup3_2_0 is retired enum class CuspRoundingFix : std::uint8_t { Disabled = 0, Enabled320 = 1, - Enabled = 2, + Enabled330 = 2, }; explicit constexpr MantissaRange(MantissaScale sc) : scale(sc) @@ -168,7 +168,7 @@ private: return 15; case MantissaScale::LargeLegacy: case MantissaScale::Large320: - case MantissaScale::Large: + case MantissaScale::Large330: return 18; // LCOV_EXCL_START default: @@ -199,8 +199,8 @@ private: return CuspRoundingFix::Disabled; case MantissaScale::Large320: return CuspRoundingFix::Enabled320; - case MantissaScale::Large: - return CuspRoundingFix::Enabled; + case MantissaScale::Large330: + return CuspRoundingFix::Enabled330; default: // If called in a constexpr context, this throw assures that the build fails if an // invalid scale is used. @@ -874,43 +874,11 @@ squelch(Number const& x, Number const& limit) noexcept return x; } -inline std::string -to_string(MantissaRange::MantissaScale const& scale) -{ - switch (scale) - { - case MantissaRange::MantissaScale::Small: - return "Small"; - case MantissaRange::MantissaScale::LargeLegacy: - return "LargeLegacy"; - case MantissaRange::MantissaScale::Large320: - return "Large320"; - case MantissaRange::MantissaScale::Large: - return "Large"; - default: - throw std::runtime_error("Bad scale"); - } -} +std::string +to_string(MantissaRange::MantissaScale const& scale); -inline std::string -to_string(Number::RoundingMode const& round) -{ - switch (round) - { - enum class RoundingMode { ToNearest, TowardsZero, Downward, Upward }; - - case Number::RoundingMode::ToNearest: - return "ToNearest"; - case Number::RoundingMode::TowardsZero: - return "TowardsZero"; - case Number::RoundingMode::Downward: - return "Downward"; - case Number::RoundingMode::Upward: - return "Upward"; - default: - throw std::runtime_error("Bad rounding mode"); - } -} +std::string +to_string(Number::RoundingMode const& round); class SaveNumberRoundMode { diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index 133ec82692..c356c51722 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -31,7 +31,45 @@ namespace xrpl { thread_local Number::RoundingMode Number::mode = Number::RoundingMode::ToNearest; thread_local std::reference_wrapper Number::kRange = - MantissaRange::getMantissaRange(MantissaRange::MantissaScale::Large); + MantissaRange::getMantissaRange(MantissaRange::MantissaScale::Large330); + +std::string +to_string(MantissaRange::MantissaScale const& scale) +{ + switch (scale) + { + case MantissaRange::MantissaScale::Small: + return "Small"; + case MantissaRange::MantissaScale::LargeLegacy: + return "LargeLegacy"; + case MantissaRange::MantissaScale::Large320: + return "Large320"; + case MantissaRange::MantissaScale::Large330: + return "Large330"; + default: + throw std::runtime_error("Bad scale"); + } +} + +std::string +to_string(Number::RoundingMode const& round) +{ + switch (round) + { + enum class RoundingMode { ToNearest, TowardsZero, Downward, Upward }; + + case Number::RoundingMode::ToNearest: + return "ToNearest"; + case Number::RoundingMode::TowardsZero: + return "TowardsZero"; + case Number::RoundingMode::Downward: + return "Downward"; + case Number::RoundingMode::Upward: + return "Upward"; + default: + throw std::runtime_error("Bad rounding mode"); + } +} std::set const& MantissaRange::getAllScales() @@ -40,7 +78,7 @@ MantissaRange::getAllScales() MantissaRange::MantissaScale::Small, MantissaRange::MantissaScale::LargeLegacy, MantissaRange::MantissaScale::Large320, - MantissaRange::MantissaScale::Large, + MantissaRange::MantissaScale::Large330, }; return kScales; } @@ -92,14 +130,14 @@ MantissaRange::getRanges() } { [[maybe_unused]] - constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large}; + constexpr static MantissaRange kRange{MantissaRange::MantissaScale::Large330}; static_assert(isPowerOfTen(kRange.min)); static_assert(kRange.min == 1'000'000'000'000'000'000ULL); static_assert(kRange.max == rep(9'999'999'999'999'999'999ULL)); static_assert(kRange.log == 18); static_assert(kRange.min < Number::kMaxRep); static_assert(kRange.max > Number::kMaxRep); - static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled); + static_assert(kRange.cuspRoundingFix == CuspRoundingFix::Enabled330); } return map; }(); @@ -363,7 +401,7 @@ Number::Guard::round() const noexcept { auto mode = Number::getround(); - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled && empty()) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && empty()) { // No remainder return Round::Exact; @@ -482,7 +520,7 @@ void Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) { auto r = round(); - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330) { // If there was any remainder, subtract 1 from the result. This is sufficient to get the // best rounding. @@ -814,7 +852,7 @@ Number::operator+=(Number const& y) xe = ye; xn = yn; } - if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled) + if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330) { // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than // maxMantissa, so that normalize will have enough information to make an accurate diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index 15136115c7..5c4730e1a6 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -58,7 +58,7 @@ setCurrentTransactionRules(std::optional r) { if (enableCuspRounding3_3_0) { - return MantissaRange::MantissaScale::Large; + return MantissaRange::MantissaScale::Large330; } if (enableCuspRounding3_2_0) { diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 266bc9bc1e..4ebdc6a73d 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1813,7 +1813,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large320: - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large330: BEAST_EXPECT(signedDifference >= 0); BEAST_EXPECT(signedDifference < pow10(product.exponent())); BEAST_EXPECT( @@ -1896,7 +1896,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large320: - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large330: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); break; @@ -1947,7 +1947,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large320: - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large330: BEAST_EXPECT(stored <= exact); BEAST_EXPECT(diff > -pow10(quotient.exponent())); break; @@ -2005,7 +2005,7 @@ public: switch (scale) { case MantissaRange::MantissaScale::Large320: - case MantissaRange::MantissaScale::Large: + case MantissaRange::MantissaScale::Large330: BEAST_EXPECT(stored >= exact); BEAST_EXPECT(diff < pow10(quotient.exponent())); break; From 318c2c2dd3ffdeb5cfbfcca2c48b7d07d9079465 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 9 Jun 2026 19:06:38 -0400 Subject: [PATCH 21/23] Also fix local 3_2_0 variable names --- src/libxrpl/protocol/Rules.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index 5c4730e1a6..2f55d7136b 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -46,21 +46,21 @@ setCurrentTransactionRules(std::optional r) // to useRulesGuards. bool const enableVaultNumbers = !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); - bool const enableCuspRounding3_2_0 = !r || r->enabled(fixCleanup3_2_0); - bool const enableCuspRounding3_3_0 = !r || r->enabled(fixNumberStuff); + bool const enableCuspRounding320 = !r || r->enabled(fixCleanup3_2_0); + bool const enableCuspRounding330 = !r || r->enabled(fixNumberStuff); XRPL_ASSERT( !r || useRulesGuards(*r) == - (enableVaultNumbers || enableCuspRounding3_2_0 || enableCuspRounding3_3_0), + (enableVaultNumbers || enableCuspRounding320 || enableCuspRounding330), "setCurrentTransactionRules : rule decisions match"); if (enableVaultNumbers) { - if (enableCuspRounding3_3_0) + if (enableCuspRounding330) { return MantissaRange::MantissaScale::Large330; } - if (enableCuspRounding3_2_0) + if (enableCuspRounding320) { return MantissaRange::MantissaScale::Large320; } From a1cfa89e1590842d4489c1ada5554a4ac099ddca Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 10 Jun 2026 16:11:43 -0400 Subject: [PATCH 22/23] test: Add more Number edge case tests, showing failures - NumberAddDirectedSignWrong - Addition of two negative numbers with the same exponent rounds ToNearest in the wrong direction. - Also include unit test cases with same exponent, and mixed signs. - No rounding issues in any combination, because the exponent can't change. - NumberAddToNearestPicksFarther - In scenarios where the two operands have different signs, and significantly different exponents, you can end up in a situation where the rounding looks like 0.5, which may round down to even, but is actually 0.5....nnn, which should always round up, you get the wrong result. --- src/test/basics/Number_test.cpp | 220 +++++++++++++++++++++++++++++++- 1 file changed, 218 insertions(+), 2 deletions(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 4ebdc6a73d..1f720f3c11 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -50,9 +50,11 @@ class Number_test : public beast::unit_test::Suite toBigInt(Number const& n) { BigInt v = n.mantissa(); - for (int i = 0; i < n.exponent(); ++i) + auto e = n.exponent(); + + for (; e > 0; --e) v *= 10; - for (int i = 0; i > n.exponent(); --i) + for (; e < 0; ++e) { BEAST_EXPECT(v % 10 == 0); v /= 10; @@ -2147,12 +2149,224 @@ public: } } + void + testNumberAddDirectedSignWrong() + { + auto const scale = Number::getMantissaScale(); + testcase << "operator+ directed rounding wrong for equal-exponent negative sums " + << to_string(scale); + { + // Two negative numbers with the same exponent + Number const a{-6, Number::mantissaLog()}; + Number const b{a - 3}; + BEAST_EXPECT(a.exponent() == b.exponent() && abs(b) > abs(a)); + + BigInt const exact = toBigInt(a) + toBigInt(b); + if (scale == MantissaRange::MantissaScale::Small) + { + BEAST_EXPECT(exact == BigInt{"-12000000000000003"}); + } + else + { + BEAST_EXPECT(exact == BigInt{"-12000000000000000003"}); + } + + Number down, up; + { + NumberRoundModeGuard const g{Number::RoundingMode::Downward}; + down = a + b; + } + { + NumberRoundModeGuard const g{Number::RoundingMode::Upward}; + up = a + b; + } + + auto const valueDown = toBigInt(down); + auto const valueUp = toBigInt(up); + log << " exact = " << fmt(exact) << "\n downward = " << fmt(valueDown) + << " (correct rounding: <= exact)" + << "\n upward = " << fmt(valueUp) << " (correct rounding: >= exact)\n\n"; + log.flush(); + + if (scale == MantissaRange::MantissaScale::Large330) + { + BEAST_EXPECT(valueDown <= exact); // Downward should round away from zero + BEAST_EXPECT(valueUp >= exact); // Upward should round toward 0 + } + else + { + BEAST_EXPECT(valueDown > exact); // Downward rounded toward zero (too high) + BEAST_EXPECT(valueUp < exact); // Upward rounded toward -inf (too low) + } + } + + { + // Positive control: the same magnitudes with a positive result round + Number const pa{6, Number::mantissaLog()}; + Number const pb{pa + 3}; + BEAST_EXPECT(pa.exponent() == pb.exponent() && abs(pb) > abs(pa)); + BigInt const pexact = toBigInt(pa) + toBigInt(pb); // 12'000'000'000'000'000'003 + + Number pdown, pup; + { + NumberRoundModeGuard const g{Number::RoundingMode::Downward}; + pdown = pa + pb; + } + { + NumberRoundModeGuard const g{Number::RoundingMode::Upward}; + pup = pa + pb; + } + auto const valuePDown = toBigInt(pdown); + auto const valuePUp = toBigInt(pup); + log << " exact = " << fmt(pexact) << "\n downward = " << fmt(valuePDown) + << " (correct rounding: <= exact)" + << "\n upward = " << fmt(valuePUp) << " (correct rounding: >= exact)\n\n"; + log.flush(); + + BEAST_EXPECT(valuePDown <= pexact); // correct for positive results + BEAST_EXPECT(valuePUp >= pexact); + } + + { + // Mixed sign numbers with the same exponent: negative second value + Number const a{1, Number::mantissaLog()}; + Number const b{Number{-9, Number::mantissaLog()} - 3}; + BEAST_EXPECT(a.exponent() == b.exponent() && abs(b) > abs(a)); + + BigInt const exact = toBigInt(a) + toBigInt(b); + if (scale == MantissaRange::MantissaScale::Small) + { + BEAST_EXPECT(exact == BigInt{"-8000000000000003"}); + } + else + { + BEAST_EXPECT(exact == BigInt{"-8000000000000000003"}); + } + + Number down, up; + { + NumberRoundModeGuard const g{Number::RoundingMode::Downward}; + down = a + b; + } + { + NumberRoundModeGuard const g{Number::RoundingMode::Upward}; + up = a + b; + } + + auto const valueDown = toBigInt(down); + auto const valueUp = toBigInt(up); + log << " exact = " << fmt(exact) << "\n downward = " << fmt(valueDown) + << " (correct rounding: <= exact)" + << "\n upward = " << fmt(valueUp) << " (correct rounding: >= exact)\n\n"; + log.flush(); + + BEAST_EXPECT(valueDown <= exact); // Downward should round away from zero + BEAST_EXPECT(valueUp >= exact); // Upward should round toward 0 + } + + { + // Mixed sign numbers with the same exponent: negative first value + Number const a{-1, Number::mantissaLog()}; + Number const b{Number{9, Number::mantissaLog()} + 3}; + BEAST_EXPECT(a.exponent() == b.exponent() && abs(b) > abs(a)); + + BigInt const exact = toBigInt(a) + toBigInt(b); + if (scale == MantissaRange::MantissaScale::Small) + { + BEAST_EXPECT(exact == BigInt{"8000000000000003"}); + } + else + { + BEAST_EXPECT(exact == BigInt{"8000000000000000003"}); + } + + Number down, up; + { + NumberRoundModeGuard const g{Number::RoundingMode::Downward}; + down = a + b; + } + { + NumberRoundModeGuard const g{Number::RoundingMode::Upward}; + up = a + b; + } + + auto const valueDown = toBigInt(down); + auto const valueUp = toBigInt(up); + log << " exact = " << fmt(exact) << "\n downward = " << fmt(valueDown) + << " (correct rounding: <= exact)" + << "\n upward = " << fmt(valueUp) << " (correct rounding: >= exact)\n\n"; + log.flush(); + + BEAST_EXPECT(valueDown <= exact); // Downward should round away from zero + BEAST_EXPECT(valueUp >= exact); // Upward should round toward 0 + } + } + + void + testNumberAddToNearestPicksFarther() + { + auto const scale = Number::getMantissaScale(); + + // Case is + using Case = std::pair; + + auto const c = std::to_array({ + {Number{5'175'909'259'972'499'745LL, 22}, -1'074'951'375'311'646'003}, + {Number{1}, -1'074'956'551'220'905'975}, + {Number{1, 10}, -1'074'956'551'220'905'975}, + {Number{1, 20}, -1'074'956'551'220'905'975}, + {Number{1, 27}, -1'074'956'551'220'905'975}, + {Number{1, 28}, -1'074'956'551'220'905'974}, + {Number{1, 31}, -1'074'956'551'220'904'975}, + }); + + testcase << "operator+ ToNearest picks farther representable in cancellation " + << to_string(scale); + + for (auto const& [y, expectedQ] : c) + { + NumberRoundModeGuard const roundGuard{Number::RoundingMode::ToNearest}; + + Number const x{-1'074'956'551'220'905'975LL, 28}; + Number const res = x + y; + + BigInt const exact = toBigInt(x) + toBigInt(y); + BigInt const vres = toBigInt(res); + + BigInt ulp = 1; + for (int i = 0; i < res.exponent(); ++i) + ulp *= 10; + + BigInt const q = (exact - ulp / 2) / ulp; + Number const normalizedExact{static_cast(q), res.exponent()}; + BigInt const norm = toBigInt(normalizedExact); + + log << " x = " << x << "\n y = " << y + << "\n exact = " << fmt(exact) + << "\n result (x + y) = " << fmt(vres) + << "\n normalize(exact) = " << fmt(norm) << "\n\n"; + log.flush(); + + if (scale == MantissaRange::MantissaScale::Small) + { + auto const comp = toBigInt(Number{expectedQ, -3}); + BEAST_EXPECTS(q == comp, fmt(q) + " != " + fmt(comp)); + } + else + { + BEAST_EXPECTS(q == expectedQ, fmt(q) + " != " + fmt(BigInt(expectedQ))); + } + BEAST_EXPECT(normalizedExact == res); + } + } + void run() override { for (auto const scale : MantissaRange::getAllScales()) { NumberMantissaScaleGuard const sg(scale); + testZero(); testLimits(); testToString(); @@ -2176,6 +2390,8 @@ public: testInt64(); testUpwardRoundsDown(); + testNumberAddDirectedSignWrong(); + testNumberAddToNearestPicksFarther(); } } }; From 5703ca527ff292cd5e5332d47cc21d9cef9745fc Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 10 Jun 2026 16:18:38 -0400 Subject: [PATCH 23/23] Number improvements - Expand documentation. - Refactor Number::Guard::round() to simplify. - Set the Guard sign correctly in += for numbers with the same exponent. - Only really relevant if both values are negative. - In +=, when needed, expand one mantissa to a size large enough to have a few extra digits, which can be used to determine rounding. - If the exponents are still different, trim the other mantissa as before until the exponents match. - For subtraction (where the values' signs are different), pop digits out of the Guard as necessary, but go far enough to have a few extra digits again for rounding later. - Finally, don't discard any "leftover" digits in the Guard when normalizing, to avoid the 0.5....nnn problem. --- src/libxrpl/basics/Number.cpp | 175 +++++++++++++++++++++++++++------- 1 file changed, 138 insertions(+), 37 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index c356c51722..222c0b611c 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -205,15 +205,37 @@ divu10(uint128_t& u) return r; } -// Guard - -// The Guard class is used to temporarily add extra digits of -// precision to an operation. This enables the final result -// to be correctly rounded to the internal precision of Number. - template concept UnsignedMantissa = std::is_unsigned_v || std::is_same_v; +/** Guard + + The Guard class is used to temporarily add extra digits of + precision to an operation. This enables the final result + to be correctly rounded to the internal precision of Number. + + At it's core, the Guard really only needs three pieces of information to determine how to round: + 1. The rounding mode + 2. The last digit dropped from the mantissa (i.e. the first digit after the decimal point). + (first byte of digits_) + 3. Whether any other non-zero digits were dropped from the mantissa. (xbit_) + + Upward and Downward rounding modes round the unsigned mantissa toward or away from zero + depending on whether the sign is negative (sbit_). For positive values, Upward is away, and + Downward is toward. For negative values, that's reversed. For simplicity, I'm going to describe + the logic using "TowardZero" and "FromZero". + + * TowardZero is the easiest rounding mode. It always rounds down. digits_ and xbit_ are + irrelevant. + * FromZero is almost as simple. If both "digits_" and "xbit_" are zero (0), it rounds down. + Else it rounds up. + * ToNearest is only a little more complicated. If the last dropped digit is < 5, then round + down. If it is > 5, round up. If it is exactly 5, and there are _any_ other digits (the + remainder of "digits_" or "xbit_"), round up, else round to even. + + The current implementation stores 16 digits in "digits_" so that digits can be "pop"ped back + out if needed during subtraction (negative addition) operations. +*/ class Number::Guard { std::uint64_t digits_{0}; // 16 decimal guard digits @@ -410,23 +432,18 @@ Number::Guard::round() const noexcept if (mode == RoundingMode::TowardsZero) return Round::Down; - if (mode == RoundingMode::Downward) + // Also Towards Zero + if ((mode == RoundingMode::Downward && !sbit_) || (mode == RoundingMode::Upward && sbit_)) { - if (sbit_) - { - if (digits_ > 0 || xbit_) - return Round::Up; - } return Round::Down; } - if (mode == RoundingMode::Upward) + // Away from Zero + if ((mode == RoundingMode::Downward && sbit_) || (mode == RoundingMode::Upward && !sbit_)) { - if (sbit_) + if (empty()) return Round::Down; - if (digits_ > 0 || xbit_) - return Round::Up; - return Round::Down; + return Round::Up; } // assume round to nearest if mode is not one of the predefined values @@ -810,35 +827,94 @@ Number::operator+=(Number const& y) // Bring the exponents of both values into agreement, so the mantissas are on the same scale // and can be added directly together. + + auto const upperLimit = static_cast(g.minMantissa) * 1000; + // For the "adjust" lambda + // expandM / expandE: The values for which the mantissa will be expanded, and the exponent + // decreased to match. Mantissa won't be expanded beyond upperLimit. + // (37e8 == 37000e5 == 37000000e2) + // shrinkM / shrinkE: The values for which the mantissa will be shrunk, and exponent increased + // to match, if necessary. + auto const adjust = [&g, &upperLimit]( + uint128_t& expandM, int& expandE, uint128_t& shrinkM, int& shrinkE) { + // Adjust up and down until the exponents match + if (g.cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled330) + { + // For Enabled330, there are three steps. + // 1. First, shrink the mantissa of shrinkM/shrinkE while shrinkM ends in 0. + while (shrinkE < expandE && shrinkM % 10 == 0) + { + g.doDropDigit(shrinkM, shrinkE); + } + + // 2. Then expand the mantissa of expandM/expandE, with a limit for expandM a few orders + // of magnitude above the MantissaRange. This will leave a few extra digits for rounding + // later, but no excess. + while (shrinkE < expandE && expandE > kMinExponent && expandM < upperLimit) + { + expandM *= 10; + --expandE; + } + } + + // 3. Finally, shrink the mantissa of shrinkM/shrinkE until the exponents match. Any removed + // digits will be put into the Guard. This is the only step for non-Enabled330 modes. + while (shrinkE < expandE) + { + g.doDropDigit(shrinkM, shrinkE); + } + }; + // Shrink the mantissa and raise the exponent of the value with the lower exponent. Store any // dropped digits in the Guard. if (xe < ye) { if (xn) g.setNegative(); - do - { - g.doDropDigit(xm, xe); - } while (xe < ye); + + adjust(ym, ye, xm, xe); } else if (xe > ye) { if (yn) g.setNegative(); - do - { - g.doDropDigit(ym, ye); - } while (xe > ye); + + adjust(xm, xe, ym, ye); + } + else if (g.cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled330) + { + // Both values have the same exponent. + // Set the sign of the Guard based on the sign of the Number with the smallest + // unsigned _mantissa_ + if ((xm < ym && xn) || (ym < xm && yn)) + g.setNegative(); } if (xn == yn) { xm += ym; - if (xm > maxMantissa || xm > kMaxRep) + + if (g.cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330) { - g.doDropDigit(xm, xe); + // Don't do any adjustments for Enabled330. Normalize will take care of it + // Because of "adjust", the only way there can be data in the Guard is if we first grew + // the mantissa past the maxMantissa. Since we added here, it can only get bigger. + // If xm > maxMantissa, then doNormalize has all the data it needs from the last 3-4 + // digits, plus the "dropped" flag that will be passed in. + // If not, then the mantissa will only need to be padded out with 0s and won't need to + // round. + XRPL_ASSERT( + xm > maxMantissa || g.empty(), + "xrpl::Number::operator+ : rounding state expected after add"); + } + else + { + if (xm > maxMantissa || xm > kMaxRep) + { + g.doDropDigit(xm, xe); + } + g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } - g.doRoundUp(xn, xm, xe, "Number::addition overflow"); } else { @@ -854,18 +930,24 @@ Number::operator+=(Number const& y) } if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330) { - // Grow xm/xe and pull digits out of the Guard until it's a little bit larger than - // maxMantissa, so that normalize will have enough information to make an accurate - // rounding decision, but stop if the Guard empties out, because no rounding will be - // necessary. (Normalize will pad it back into range.) Note that if any digits were lost - // (xbit), the Guard will never be empty, so xm will get larger than upperLimit. - auto const upperLimit = static_cast(minMantissa) * 1000; + // Because we subtracted, xm can have any number of digits from 1 up to + // upperLimit * 10, and g can be in any state. (Note that xm can't be zero, because that + // special case was tested earlier.) + + // Grow xm/xe and pull digits out of the Guard until xm reaches upperLimit, but stop if + // the Guard empties out, because no rounding will be necessary. This will ensure that + // normalize will have enough information to make an accurate rounding decision. + // (Normalize will pad a small mantissa back into range.) Note that if any digits were + // lost (xbit_), the Guard will never be empty, so xm will grow larger than upperLimit. while (xm < upperLimit && !g.empty()) { xm *= 10; xm -= g.pop(); --xe; } + XRPL_ASSERT( + xm > maxMantissa || g.empty(), + "xrpl::Number::operator+ : rounding state expected after subtract"); } else { @@ -878,12 +960,31 @@ Number::operator+=(Number const& y) --xe; } } - // Round down, based on whether there is any data left in the Guard (depending on - // cuspRoundingFix) + // Rounding down can result in decrementing xm, based on whether there is any data left in + // the Guard (depending on cuspRoundingFix). Note that if that happens, then the Guard is + // not empty. For Enabled330, that will also result in the "dropped" flag being passed to + // doNormalize, which may result in the mantissa being incremented again. It doesn't matter + // what the dropped digits are, only that they exist. This is because subtracting one + // "overcorrects", so we know there are still trailing digits to be accounted for in the + // rounding. + // + // This works because + // 1. The rounding up will be done _after_ the mantissa is brought into range. It may not + // be in range right now, and + // 2. The "dropped" flag is only ever used as a tie-breaker, specifically when rounding + // away from zero, and the dropped digits are 0, or when rounding to nearest, and + // the dropped digits represent exactly 0.5. g.doRoundDown(xn, xm, xe); } - doNormalize(xn, xm, xe, minMantissa, maxMantissa, cuspRoundingFix, false); + doNormalize( + xn, + xm, + xe, + minMantissa, + maxMantissa, + cuspRoundingFix, + cuspRoundingFix == MantissaRange::CuspRoundingFix::Enabled330 && !g.empty()); negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe;