fix: Handle rounding just above kMaxRep more accurately

include/xrpl/basics/Number.h

@@ -334,6 +334,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

@@ -287,6 +287,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 <UnsignedMantissa T>
+    void
+    pushOverflow(T mantissa);
+
     enum class Round {
         // The result is exact. No rounding is needed. Only used if cuspRoundingFix is Enabled330 or
         // higher.
@@ -299,31 +318,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;
 
@@ -406,10 +410,78 @@ Number::Guard::doDropDigit<uint128_t>(uint128_t& mantissa, int& exponent) noexce
     ++exponent;
 }
 
+template <UnsignedMantissa T>
+void
+Number::Guard::pushOverflow(T 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 in the range [kMaxRep, kMaxRepUp).
+
+        auto constexpr spread = kMaxRepUp - kMaxRep;
+        static_assert(spread == 3);
+
+        // Round in two steps.
+
+        // The first step uses the digits _already_ in the Guard to round the
+        // intermediate mantissa, using only the last digit. Then update the mantissa for the
+        // second step. Ultimately, the purpose of this step is to capture rounding where the stored
+        // digits would change the decision without those digits. (e.g. From just _below_ the
+        // midpoint to just _above_ the midpoint for ToNearest, or from kMaxRep into the in-between
+        // for Upward.
+        // Make an exception if the final digit is 9, because it can only get larger, and we want to
+        // stay in single digits.
+        if (auto finalDigit = mantissa % 10; finalDigit < 9)
+        {
+            // Intentionally use integer math to get the largest value under the midpoint.
+            auto constexpr kMidpoint = kMaxRep + (spread / 2);
+            static_assert(kMidpoint == kMaxRep + 1);
+            auto const r = round();
+            if (r == Round::Up || (r == Round::Even && mantissa == kMidpoint))
+            {
+                ++mantissa;
+            }
+        }
+
+        if (mantissa == kMaxRep)
+        {
+            // If the mantissa ends up exactly kMaxRep, there's nothing more to do.
+            return;
+        }
+
+        // The second step scales the final digit of the update mantissa proportionally from kMaxRep
+        // and kMaxRepUp to 1 to 9. It then pushes that scaled digit onto the guard as if it was a
+        // digit that got removed, but don't actually remove it. This method should be is
+        // future-proof in case the number of mantissa bits ever changes. (Though for integer values
+        // that are a power of two themselves, the spread will always be the same.) Effects:
+        // * For round to nearest
+        //      * if the updated mantissa is below the midpoint, it'll round "down" to kMaxRep
+        //      * if above the midpoint, it'll round "up" to kMaxRepUp
+        //      * it can never be exactly at the midpoint, because kMaxRepUp is always even, and
+        //        kMaxRep is always odd, so don't worry about that case.
+        // * 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 const diff = mantissa - kMaxRep;
+        auto digit = (diff * 10) / spread;
+        XRPL_ASSERT(
+            digit > 0 && digit < 10 && digit != 5,
+            "xrpl::Number::Guard::pushOverflow : valid overflow digit");
+
+        // Don't remove the digit from the mantissa, but add it to the guard as if it was.
+        push(digit);
+    }
+}
+
 // 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
 {
@@ -458,13 +530,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{};
 
@@ -478,7 +553,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) {
@@ -495,18 +572,27 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string
             }
             else
             {
-                // Incrementing the mantissa will require dividing, which will require rounding. So
-                // _don't_ increment the mantissa. Instead, divide and round recursively. It should
-                // be impossible to recurse more than once, because once the mantissa is divided by
-                // 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1 will have no
-                // chance of bringing it back over.
-                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
+                {
+                    // Incrementing the mantissa will require dividing, which will require rounding.
+                    // So _don't_ increment the mantissa. Instead, divide and round recursively. It
+                    // should be impossible to recurse more than once, because once the mantissa is
+                    // divided by 10, it will be _well_ under maxMantissa and kMaxRep, so adding 1
+                    // will have no chance of bringing it back over.
+                    doDropDigit(mantissa, exponent);
+                    XRPL_ASSERT_PARTS(
+                        safeToIncrement(mantissa),
+                        "xrpl::Number::Guard::doRoundUp",
+                        "can't recurse more than once");
+                    doRoundUp(negative, mantissa, exponent, location);
+                    return;
+                }
             }
         }
         else
@@ -524,6 +610,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));
@@ -533,6 +625,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)
     {
@@ -565,7 +659,7 @@ 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)
 {
     auto r = round();
     if (r == Round::Up || (r == Round::Even && (drops & 1) == 1))
@@ -583,6 +677,8 @@ Number::Guard::doRound(rep& drops, std::string location) const
         }
         ++drops;
     }
+    XRPL_ASSERT(drops >= 0, "xrpl::Number::Guard::doRound : positive magnitude");
+
     if (isNegative())
         drops = -drops;
 }
@@ -632,7 +728,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;
 
@@ -682,17 +780,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");
@@ -824,6 +922,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.
 
@@ -908,7 +1009,7 @@ Number::operator+=(Number const& y)
         }
         else
         {
-            if (xm > maxMantissa || xm > kMaxRep)
+            if (xm > maxMantissa || xm > repLimit)
             {
                 g.doDropDigit(xm, xe);
             }
@@ -952,7 +1053,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();
@@ -1026,8 +1127,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));
+            }
         }
 
         {