fix: Handle rounding just above kMaxRep more accurately

include/xrpl/basics/Number.h

@@ -337,6 +337,8 @@ class Number final
     static constexpr internalrep kMaxRep = std::numeric_limits<rep>::max();
     static_assert(kMaxRep == 9'223'372'036'854'775'807);
     static_assert(-kMaxRep == std::numeric_limits<rep>::min() + 1);
+    static constexpr internalrep kMaxRepUp = ((kMaxRep / 10) + 1) * 10;
+    static_assert(kMaxRepUp == 9'223'372'036'854'775'810ULL);
 
     // May need to make unchecked private
     struct Unchecked

src/libxrpl/basics/Number.cpp

@@ -295,6 +295,25 @@ class Number::Guard
     void
     doDropDigit(T& mantissa, int& exponent) noexcept;
 
+    // Modify the result to the correctly rounded value
+    template <UnsignedMantissa T>
+    void
+    doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location);
+
+    // Modify the result to the correctly rounded value
+    template <UnsignedMantissa T>
+    void
+    doRoundDown(bool& negative, T& mantissa, int& exponent);
+
+    // Modify the result to the correctly rounded value
+    void
+    doRound(rep& drops, std::string location);
+
+private:
+    template <class T>
+    void
+    pushOverflow(T const& mantissa);
+
     enum class Round {
         // The result is exact. No rounding is needed. Only used if cuspRoundingFix is Enabled330 or
         // higher.
@@ -307,31 +326,16 @@ class Number::Guard
         // 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)
+        // Enabled330)
         Up = 1,
     };
 
-    // Indicate round direction:  1 is up, -1 is down, 0 is even
+    // Indicate round direction. See Round enum above.
     // This enables the client to round towards nearest, and on
     // tie, round towards even.
     [[nodiscard]] Round
     round() const noexcept;
 
-    // Modify the result to the correctly rounded value
-    template <UnsignedMantissa T>
-    void
-    doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location);
-
-    // Modify the result to the correctly rounded value
-    template <UnsignedMantissa T>
-    void
-    doRoundDown(bool& negative, T& mantissa, int& exponent);
-
-    // Modify the result to the correctly rounded value
-    void
-    doRound(rep& drops, std::string location) const;
-
-private:
     void
     doPush(unsigned d) noexcept;
 
@@ -414,10 +418,48 @@ Number::Guard::doDropDigit<uint128_t>(uint128_t& mantissa, int& exponent) noexce
     ++exponent;
 }
 
+template <class T>
+void
+Number::Guard::pushOverflow(T const& mantissa)
+{
+    XRPL_ASSERT(mantissa <= kMaxRepUp, "xrpl::Number::Guard::pushOverflow : valid mantissa");
+    if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa > kMaxRep &&
+        mantissa < kMaxRepUp)
+    {
+        // Special case rounding rules for the values between kMaxRep and kMaxRepUp.
+        // Scale the spread between kMaxRep and kMaxRepUp from 1 to 9, and push it onto the guard as
+        // if it was a digit that got removed, but don't remove it. This method is future-proof in
+        // case the number of mantissa bits ever changes. Effects:
+        // * For round to nearest
+        //      * if the mantissa is below the midpoint, it'll round "down" to kMaxRep
+        //      * if above the midpoint, it'll round "up" to kMaxRepUp
+        //      * it can never be exactly at the midpoint, because kMaxRepUp is always even, and
+        //        kMaxRep is always odd, so don't worry about that case.
+        // * For round upward, will round up to kMaxRepUp for positive values, down to kMaxRep for
+        //   negative.
+        // * For round downward, does the opposite of upward.
+        // * For round toward zero, always rounds down to kMaxRep.
+        auto constexpr spread = kMaxRepUp - kMaxRep;
+        static_assert(spread < 10 && spread > 0);
+        // This should absolutely be impossible
+        if constexpr (spread == 0)
+            return;  // LCOV_EXCL_LINE
+
+        auto const diff = mantissa - kMaxRep;
+        auto const digit = (diff * 10) / spread;
+        XRPL_ASSERT(
+            digit > 0 && digit < 10, "xrpl::Number::Guard::pushOverflow : valid overflow digit");
+
+        // Don't remove the digit from the mantissa, but add it to the guard as if it was.
+        push(digit);
+    }
+}
+
 // Returns:
-//     -1 if Guard is less than half
-//      0 if Guard is exactly half
-//      1 if Guard is greater than half
+//     Exact if Guard is _zero_, and appropriate amendments are enabled
+//     Down  if Guard is less than half
+//     Even  if Guard is exactly half
+//     Up    if Guard is greater than half
 Number::Guard::Round
 Number::Guard::round() const noexcept
 {
@@ -461,13 +503,16 @@ void
 Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent)
 {
     // Bring mantissa back into the minMantissa / maxMantissa range AFTER
-    // rounding
+    // rounding. Mantissa should never be 0.
+    XRPL_ASSERT(mantissa != 0, "xrpl::Number::Guard::bringIntoRange : valid mantissa");
     if (mantissa < minMantissa)
     {
         mantissa *= 10;
         --exponent;
     }
-    if (exponent < kMinExponent)
+    // mantissa should never be 0, but if it _is_ make the result kZero.
+    if (exponent < kMinExponent ||
+        (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa == 0))
     {
         static constexpr Number kZero = Number{};
 
@@ -481,7 +526,9 @@ template <UnsignedMantissa T>
 void
 Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string location)
 {
-    auto r = round();
+    pushOverflow(mantissa);
+
+    auto const r = round();
     if (r == Round::Up || (r == Round::Even && (mantissa & 1) == 1))
     {
         auto const safeToIncrement = [this](auto const& mantissa) {
@@ -503,13 +550,21 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string
                 // be impossible to recurse more than once, because once the mantissa is divided by
                 // 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1 will have no
                 // chance of bringing it back over.
-                doDropDigit(mantissa, exponent);
-                XRPL_ASSERT_PARTS(
-                    safeToIncrement(mantissa),
-                    "xrpl::Number::Guard::doRoundUp",
-                    "can't recurse more than once");
-                doRoundUp(negative, mantissa, exponent, location);
-                return;
+                if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 &&
+                    mantissa > kMaxRep && mantissa < kMaxRepUp)
+                {
+                    mantissa = kMaxRepUp;
+                }
+                else
+                {
+                    doDropDigit(mantissa, exponent);
+                    XRPL_ASSERT_PARTS(
+                        safeToIncrement(mantissa),
+                        "xrpl::Number::Guard::doRoundUp",
+                        "can't recurse more than once");
+                    doRoundUp(negative, mantissa, exponent, location);
+                    return;
+                }
             }
         }
         else
@@ -527,6 +582,12 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string
             }
         }
     }
+    else if (
+        cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa > kMaxRep &&
+        mantissa < kMaxRepUp)
+    {
+        mantissa = kMaxRep;
+    }
     bringIntoRange(negative, mantissa, exponent);
     if (exponent > kMaxExponent)
         Throw<std::overflow_error>(std::string(location));
@@ -536,6 +597,8 @@ template <UnsignedMantissa T>
 void
 Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent)
 {
+    // Do not pushOverflow here.
+
     auto r = round();
     if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330)
     {
@@ -568,8 +631,10 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent)
 
 // Modify the result to the correctly rounded value
 void
-Number::Guard::doRound(rep& drops, std::string location) const
+Number::Guard::doRound(rep& drops, std::string location)
 {
+    pushOverflow(drops);
+
     auto r = round();
     if (r == Round::Up || (r == Round::Even && (drops & 1) == 1))
     {
@@ -586,6 +651,14 @@ Number::Guard::doRound(rep& drops, std::string location) const
         }
         ++drops;
     }
+    else if (
+        cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && drops > kMaxRep &&
+        drops < kMaxRepUp)
+    {
+        // This will probably be impossible because this function is not called by mutating
+        // functions, so the Number will already be normalized.
+        drops = kMaxRep;
+    }
     if (isNegative())
         drops = -drops;
 }
@@ -635,7 +708,9 @@ doNormalize(
 {
     static constexpr auto kMinExponent = Number::kMinExponent;
     static constexpr auto kMaxExponent = Number::kMaxExponent;
-    static constexpr auto kMaxRep = Number::kMaxRep;
+    auto const repLimit = cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330
+        ? Number::kMaxRepUp
+        : Number::kMaxRep;
 
     using Guard = Number::Guard;
 
@@ -685,17 +760,17 @@ doNormalize(
     //      9,900,000,000,000,123,450 or 9,900,000,000,000,123,460.
     // mantissa() will return mantissa / 10, and exponent() will return
     // exponent + 1.
-    if (m > kMaxRep)
+    if (m > repLimit)
     {
         if (exponent >= kMaxExponent)
             throw std::overflow_error("Number::normalize 1.5");
         g.doDropDigit(m, exponent);
     }
     // Before modification, m should be within the min/max range. After
-    // modification, it must be less than kMaxRep. In other words, the original
-    // value should have been no more than kMaxRep * 10.
-    // (kMaxRep * 10 > maxMantissa)
-    XRPL_ASSERT_PARTS(m <= kMaxRep, "xrpl::doNormalize", "intermediate mantissa fits in int64");
+    // modification, it must be less than repLimit. In other words, the original
+    // value should have been no more than repLimit * 10.
+    // (repLimit * 10 > maxMantissa)
+    XRPL_ASSERT_PARTS(m <= repLimit, "xrpl::doNormalize", "intermediate mantissa fits in limit");
     mantissa = m;
 
     g.doRoundUp(negative, mantissa, exponent, "Number::normalize 2");
@@ -827,6 +902,9 @@ Number::operator+=(Number const& y)
     auto const& maxMantissa = g.maxMantissa;
     auto const cuspRoundingFix = g.cuspRoundingFix;
 
+    auto const repLimit =
+        cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 ? kMaxRepUp : kMaxRep;
+
     // Bring the exponents of both values into agreement, so the mantissas are on the same scale
     //   and can be added directly together.
 
@@ -911,7 +989,7 @@ Number::operator+=(Number const& y)
         }
         else
         {
-            if (xm > maxMantissa || xm > kMaxRep)
+            if (xm > maxMantissa || xm > repLimit)
             {
                 g.doDropDigit(xm, xe);
             }
@@ -955,7 +1033,7 @@ Number::operator+=(Number const& y)
         {
             // 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)
+            while (xm < minMantissa && xm * 10 <= repLimit)
             {
                 xm *= 10;
                 xm -= g.pop();
@@ -1029,8 +1107,10 @@ Number::operator*=(Number const& y)
         g.setNegative();
 
     auto const& maxMantissa = g.maxMantissa;
+    auto const repLimit =
+        g.cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 ? kMaxRepUp : kMaxRep;
 
-    while (zm > maxMantissa || zm > kMaxRep)
+    while (zm > maxMantissa || zm > repLimit)
     {
         g.doDropDigit(zm, ze);
     }

src/test/app/LoanBroker_test.cpp

@@ -1452,18 +1452,54 @@ class LoanBroker_test : public beast::unit_test::Suite
             env(tx2, Ter(temINVALID));
         }
 
+        if (Number::getMantissaScale() == MantissaRange::MantissaScale::Large330)
         {
-            auto const dm = power(2, 63) - 1;
-            BEAST_EXPECTS(dm > kMaxMpTokenAmount, to_string(dm));
-            tx2[sfDebtMaximum] = dm;
-            env(tx2, Ter(temINVALID));
-        }
+            // For the Large330 scale, 2^63 rounds _down_ to Number::kMaxRep
+            {
+                auto const dm = power(2, 63);
+                BEAST_EXPECTS(dm == kMaxMpTokenAmount, to_string(dm));
+                tx2[sfDebtMaximum] = dm;
+                env(tx2, Ter(tesSUCCESS));
+            }
+
+            {
+                auto const dm = power(2, 63) - 1;
+                BEAST_EXPECTS(dm < kMaxMpTokenAmount, to_string(dm));
+                tx2[sfDebtMaximum] = dm;
+                env(tx2, Ter(tesSUCCESS));
+            }
+
+            {
+                auto const dm = power(2, 63) - 3;
+                BEAST_EXPECTS(dm < kMaxMpTokenAmount, to_string(dm));
+                tx2[sfDebtMaximum] = dm;
+                env(tx2, Ter(tesSUCCESS));
+            }
 
+            {
+                auto const dm = power(2, 63) + 3;
+                BEAST_EXPECTS(dm > kMaxMpTokenAmount, to_string(dm));
+                tx2[sfDebtMaximum] = dm;
+                env(tx2, Ter(temINVALID));
+            }
+        }
+        else
         {
-            auto const dm = power(2, 63) - 3;
-            BEAST_EXPECTS(dm == kMaxMpTokenAmount, to_string(dm));
-            tx2[sfDebtMaximum] = dm;
-            env(tx2, Ter(tesSUCCESS));
+            // For other scales, 2^63 rounds _up_ to Number::kMaxRepUp. Subtracting 1 rounds up
+            // again.
+            {
+                auto const dm = power(2, 63) - 1;
+                BEAST_EXPECTS(dm > kMaxMpTokenAmount, to_string(dm));
+                tx2[sfDebtMaximum] = dm;
+                env(tx2, Ter(temINVALID));
+            }
+
+            {
+                auto const dm = power(2, 63) - 3;
+                BEAST_EXPECTS(dm == kMaxMpTokenAmount, to_string(dm));
+                tx2[sfDebtMaximum] = dm;
+                env(tx2, Ter(tesSUCCESS));
+            }
         }
 
         {