From 0cd908112b1c07f11a8cf5e7633596046b24f35b Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 28 Apr 2026 22:28:14 -0400 Subject: [PATCH 1/4] Fix a rounding error at the Number::maxRep cusp --- src/libxrpl/basics/Number.cpp | 57 +++++++++++++++++-- src/test/basics/Number_test.cpp | 98 +++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 6 deletions(-) diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index ba444b77e00..d277e021c4e 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -223,9 +223,12 @@ Number::Guard::bringIntoRange( { mantissa *= 10; --exponent; + std::cout << "bringIntoRange. mantissa*=10: " << mantissa << ", exponent: " << exponent + << std::endl; } if (exponent < minExponent) { + std::cout << "bringIntoRange. zero\n"; constexpr Number zero = Number{}; negative = zero.negative_; @@ -247,13 +250,50 @@ Number::Guard::doRoundUp( auto r = round(); if (r == 1 || (r == 0 && (mantissa & 1) == 1)) { - ++mantissa; - // Ensure mantissa after incrementing fits within both the - // min/maxMantissa range and is a valid "rep". - if (mantissa > maxMantissa || mantissa > maxRep) + std::cout << "doRoundUp. r: " << r << std::endl; + if (isFeatureEnabled(fixCleanup3_2_0) || !getCurrentTransactionRules()) { - mantissa /= 10; - ++exponent; + // Ensure mantissa after incrementing fits within both the + // min/maxMantissa range and is a valid "rep". + if (mantissa < maxMantissa && mantissa < maxRep) + { + // Nothing unusual here, just increment the mantissa + ++mantissa; + std::cout << "\tmantissa++: " << mantissa << std::endl; + } + 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 maxRep, so adding 1 will have no + // change of bringing it back over. + push(mantissa % 10); + mantissa /= 10; + ++exponent; + XRPL_ASSERT_PARTS( + mantissa < maxMantissa && mantissa < maxRep, + "xrpl::Number::Guard::doRoundUp", + "can't recurse more than once"); + std::cout << "\tmantissa/=10: " << mantissa << ", exponent: " << exponent + << std::endl; + // Here be dragons + doRoundUp(negative, mantissa, exponent, minMantissa, maxMantissa, location); + return; + } + } + else + { + // Need to preserve the incorrect behavior until the fix amendment can be retired, + // because otherwise would risk an unplanned ledger fork. + ++mantissa; + // Ensure mantissa after incrementing fits within both the + // min/maxMantissa range and is a valid "rep". + if (mantissa > maxMantissa || mantissa > maxRep) + { + mantissa /= 10; + ++exponent; + } } } bringIntoRange(negative, mantissa, exponent, minMantissa); @@ -668,6 +708,8 @@ Number::operator*=(Number const& y) auto const& minMantissa = range.min; auto const& maxMantissa = range.max; + std::cout << "zn: " << zn << ", zm: " << zm << ", ze: " << ze << std::endl; + while (zm > maxMantissa || zm > maxRep) { // The following is optimization for: @@ -676,6 +718,9 @@ Number::operator*=(Number const& y) g.push(divu10(zm)); ++ze; } + + std::cout << "zn: " << zn << ", zm: " << zm << ", ze: " << ze << std::endl; + xm = static_cast(zm); xe = ze; g.doRoundUp( diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 607ae004b12..93e68f67a49 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -1586,3 +1586,101 @@ class Number_test : public beast::unit_test::suite BEAST_DEFINE_TESTSUITE(Number, basics, xrpl); } // namespace xrpl +#include +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +namespace xrpl { + +class NumberUpwardWrongDirection_test : public beast::unit_test::suite +{ + using BigInt = boost::multiprecision::cpp_int; + + static std::string + fmt(BigInt const& value) + { + std::ostringstream os; + os << value; + auto s = os.str(); + std::string out; + int count = 0; + for (auto it = s.rbegin(); it != s.rend(); ++it) + { + if (count && count % 3 == 0) + out.insert(out.begin(), '_'); + out.insert(out.begin(), *it); + ++count; + } + return out; + } + +public: + void + testUpwardRoundsDown() + { + testcase << "upward rounding produces a value below exact at maxRep cusp"; + + auto const origScale = Number::getMantissaScale(); + auto const origRound = Number::setround(Number::upward); + Number::setMantissaScale(MantissaRange::large); + + constexpr std::int64_t aValue = 1'000'000'000'000'049'863LL; + constexpr std::int64_t bValue = 9'223'372'036'854'315'903LL; + + // JSON -> STAmount -> Number + AccountID const dummyIssuer = AccountID{42u}; + MPTIssue const issue(/*sequence=*/1u, dummyIssuer); + + STAmount const amountA{MPTAmount{aValue}, issue}; + STAmount const amountB{MPTAmount{bValue}, issue}; + + // Public conversion operator: STAmount::operator Number() const. + Number const a = amountA; + Number const b = amountB; + Number const product = a * b; + + // Exact reference in BigInt. + BigInt const exactProduct = BigInt(aValue) * BigInt(bValue); + + // What Number actually stored. + BigInt storedValue = BigInt(product.mantissa()); + for (int i = 0; i < product.exponent(); ++i) + storedValue *= 10; + + BigInt const signedDifference = storedValue - exactProduct; + + log << "\n" + << " a = " << fmt(BigInt(aValue)) << "\n" + << " b = " << fmt(BigInt(bValue)) << "\n" + << " exact a*b = " << fmt(exactProduct) << "\n" + << " stored = " << fmt(storedValue) << "\n" + << " stored - exact = " << fmt(signedDifference) << "\n" + << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n"; + + BEAST_EXPECT(signedDifference >= 0); + BEAST_EXPECT(product.mantissa() == (std::numeric_limits::max() / 10) + 1); + BEAST_EXPECT(product.exponent() == 19); + + Number::setround(origRound); + Number::setMantissaScale(origScale); + } + + void + run() override + { + testUpwardRoundsDown(); + } +}; + +BEAST_DEFINE_TESTSUITE(NumberUpwardWrongDirection, basics, ripple); + +} // namespace xrpl From 8473780da73fe3599385939a0c5a179d70d916f6 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sun, 3 May 2026 20:44:15 -0400 Subject: [PATCH 2/4] [WIP] Checkpoint --- include/xrpl/basics/Number.h | 110 +++++++++----- include/xrpl/protocol/STAmount.h | 2 +- src/libxrpl/basics/Number.cpp | 177 +++++++++++++++------- src/libxrpl/protocol/IOUAmount.cpp | 2 +- src/libxrpl/protocol/Rules.cpp | 10 +- src/libxrpl/protocol/STNumber.cpp | 3 +- src/test/app/Invariants_test.cpp | 190 +++++++++++++----------- src/test/basics/IOUAmount_test.cpp | 3 +- src/test/basics/Number_test.cpp | 152 ++++++++----------- src/test/protocol/STNumber_test.cpp | 3 +- src/test/rpc/GetAggregatePrice_test.cpp | 3 +- 11 files changed, 372 insertions(+), 283 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index b82cb628b90..0431cd7b989 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -7,6 +7,7 @@ #include #include #include +#include #include namespace xrpl { @@ -70,18 +71,39 @@ isPowerOfTen(T value) struct MantissaRange { using rep = std::uint64_t; - enum class mantissa_scale { small, large }; + enum class mantissa_scale { + small, + // largeLegacy can be removed when fixCleanup3_2_0 is retired + largeLegacy, + large, + }; + + // This entire enum can be removed when fixCleanup3_2_0 is retired + enum class cusp_rounding_fix : bool { + disabled = false, + enabled = true, + }; explicit constexpr MantissaRange(mantissa_scale scale_) - : min(getMin(scale_)), log(logTen(min).value_or(-1)), scale(scale_) + : min(getMin(scale_)) + , cuspRoundingFixEnabled(isCuspFixEnabled(scale_)) + , log(logTen(min).value_or(-1)) + , scale(scale_) { } rep min; rep max{(min * 10) - 1}; + cusp_rounding_fix cuspRoundingFixEnabled; int log; mantissa_scale scale; + static MantissaRange const& + getMantissaRange(mantissa_scale scale); + + static std::set const& + getAllScales(); + private: static constexpr rep getMin(mantissa_scale scale_) @@ -90,6 +112,7 @@ struct MantissaRange { case mantissa_scale::small: return 1'000'000'000'000'000ULL; + case mantissa_scale::largeLegacy: case mantissa_scale::large: return 1'000'000'000'000'000'000ULL; default: @@ -99,6 +122,27 @@ struct MantissaRange throw std::runtime_error("Unknown mantissa scale"); } } + + static constexpr cusp_rounding_fix + isCuspFixEnabled(mantissa_scale scale_) + { + switch (scale_) + { + case mantissa_scale::small: + case mantissa_scale::largeLegacy: + return cusp_rounding_fix::disabled; + case mantissa_scale::large: + return cusp_rounding_fix::enabled; + default: + // Since this can never be called outside a non-constexpr + // context, this throw assures that the build fails if an + // invalid scale is used. + throw std::runtime_error("Unknown mantissa scale"); + } + } + + static std::unordered_map const& + getRanges(); }; // Like std::integral, but only 64-bit integral types. @@ -422,49 +466,29 @@ class Number return range_.get().log; } - /// oneSmall is needed because the ranges are private - constexpr static Number - oneSmall(); - /// oneLarge is needed because the ranges are private - constexpr static Number - oneLarge(); - - // And one is needed because it needs to choose between oneSmall and - // oneLarge based on the current range static Number one(); - template + template < + auto minMantissa, + auto maxMantissa, + Integral64 T = std::decay_t, + Integral64 TMax = std::decay_t> [[nodiscard]] std::pair - normalizeToRange(T minMantissa, T maxMantissa) const; + normalizeToRange() const; private: static thread_local rounding_mode mode_; // The available ranges for mantissa - constexpr static MantissaRange smallRange{MantissaRange::mantissa_scale::small}; - static_assert(isPowerOfTen(smallRange.min)); - static_assert(smallRange.min == 1'000'000'000'000'000LL); - static_assert(smallRange.max == 9'999'999'999'999'999LL); - static_assert(smallRange.log == 15); - static_assert(smallRange.min < maxRep); - static_assert(smallRange.max < maxRep); - constexpr static MantissaRange largeRange{MantissaRange::mantissa_scale::large}; - static_assert(isPowerOfTen(largeRange.min)); - static_assert(largeRange.min == 1'000'000'000'000'000'000ULL); - static_assert(largeRange.max == internalrep(9'999'999'999'999'999'999ULL)); - static_assert(largeRange.log == 18); - static_assert(largeRange.min < maxRep); - static_assert(largeRange.max > maxRep); - // The range for the mantissa when normalized. // Use reference_wrapper to avoid making copies, and prevent accidentally // changing the values inside the range. static thread_local std::reference_wrapper range_; void - normalize(); + normalize(MantissaRange const& range); /** Normalize Number components to an arbitrary range. * @@ -479,7 +503,8 @@ class Number T& mantissa, int& exponent, internalrep const& minMantissa, - internalrep const& maxMantissa); + internalrep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled); template friend void @@ -488,7 +513,8 @@ class Number T& mantissa_, int& exponent_, MantissaRange::rep const& minMantissa, - MantissaRange::rep const& maxMantissa); + MantissaRange::rep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled); [[nodiscard]] bool isnormal() const noexcept; @@ -524,7 +550,7 @@ constexpr static Number numZero{}; inline Number::Number(bool negative, internalrep mantissa, int exponent, normalized) : Number(negative, mantissa, exponent, unchecked{}) { - normalize(); + normalize(range_); } inline Number::Number(internalrep mantissa, int exponent, normalized) @@ -694,10 +720,19 @@ Number::isnormal() const noexcept minExponent <= exponent_ && exponent_ <= maxExponent); } -template +template std::pair -Number::normalizeToRange(T minMantissa, T maxMantissa) const +Number::normalizeToRange() const { + static_assert(std::is_same_v || std::is_same_v); + static_assert(std::is_same_v); + auto constexpr min = static_cast(minMantissa); + auto constexpr max = static_cast(maxMantissa); + static_assert(min > 0); + static_assert(min % 10 == 0); + static_assert(max % 10 == 9); + static_assert((max + 1) / 10 == min); + bool negative = negative_; internalrep mantissa = mantissa_; int exponent = exponent_; @@ -709,7 +744,10 @@ Number::normalizeToRange(T minMantissa, T maxMantissa) const "xrpl::Number::normalizeToRange", "Number is non-negative for unsigned range."); } - Number::normalize(negative, mantissa, exponent, minMantissa, maxMantissa); + // Don't need to worry about the cuspRounding fix because rounding up will never take the + // mantissa over maxMantissa with a ones digit value other than 0. 0 can safely be truncated. + Number::normalize( + negative, mantissa, exponent, min, max, MantissaRange::cusp_rounding_fix::disabled); auto const sign = negative ? -1 : 1; return std::make_pair(static_cast(sign * mantissa), exponent); @@ -761,6 +799,8 @@ to_string(MantissaRange::mantissa_scale const& scale) { case MantissaRange::mantissa_scale::small: return "small"; + case MantissaRange::mantissa_scale::largeLegacy: + return "largeLegacy"; case MantissaRange::mantissa_scale::large: return "large"; default: diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index f681f811b74..dbff8960565 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -540,7 +540,7 @@ STAmount::fromNumber(A const& a, Number const& number) return STAmount{asset, intValue, 0, negative}; } - auto const [mantissa, exponent] = working.normalizeToRange(cMinValue, cMaxValue); + auto const [mantissa, exponent] = working.normalizeToRange(); return STAmount{asset, mantissa, exponent, negative}; } diff --git a/src/libxrpl/basics/Number.cpp b/src/libxrpl/basics/Number.cpp index d277e021c4e..03f59531b87 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -28,7 +28,76 @@ using int128_t = __int128_t; namespace xrpl { thread_local Number::rounding_mode Number::mode_ = Number::rounding_mode::to_nearest; -thread_local std::reference_wrapper Number::range_ = largeRange; +thread_local std::reference_wrapper Number::range_ = + MantissaRange::getMantissaRange(MantissaRange::mantissa_scale::large); + +std::set const& +MantissaRange::getAllScales() +{ + static std::set const scales = { + MantissaRange::mantissa_scale::small, + MantissaRange::mantissa_scale::largeLegacy, + MantissaRange::mantissa_scale::large, + }; + return scales; +} + +std::unordered_map const& +MantissaRange::getRanges() +{ + static auto const map = []() { + std::unordered_map map; + for (auto const scale : getAllScales()) + { + map.emplace(scale, scale); + } + + // Use these constexpr declarations to do static_asserts to verify the MantissaRanges are + // created correctly, but nothing else. + { + [[maybe_unused]] + constexpr static MantissaRange range{MantissaRange::mantissa_scale::small}; + static_assert(isPowerOfTen(range.min)); + static_assert(range.min == 1'000'000'000'000'000LL); + static_assert(range.max == 9'999'999'999'999'999LL); + static_assert(range.log == 15); + static_assert(range.min < Number::maxRep); + static_assert(range.max < Number::maxRep); + static_assert(range.cuspRoundingFixEnabled == cusp_rounding_fix::disabled); + } + { + [[maybe_unused]] + constexpr static MantissaRange range{MantissaRange::mantissa_scale::largeLegacy}; + static_assert(isPowerOfTen(range.min)); + static_assert(range.min == 1'000'000'000'000'000'000ULL); + static_assert(range.max == rep(9'999'999'999'999'999'999ULL)); + static_assert(range.log == 18); + static_assert(range.min < Number::maxRep); + static_assert(range.max > Number::maxRep); + static_assert(range.cuspRoundingFixEnabled == cusp_rounding_fix::disabled); + } + { + [[maybe_unused]] + constexpr static MantissaRange range{MantissaRange::mantissa_scale::large}; + static_assert(isPowerOfTen(range.min)); + static_assert(range.min == 1'000'000'000'000'000'000ULL); + static_assert(range.max == rep(9'999'999'999'999'999'999ULL)); + static_assert(range.log == 18); + static_assert(range.min < Number::maxRep); + static_assert(range.max > Number::maxRep); + static_assert(range.cuspRoundingFixEnabled == cusp_rounding_fix::enabled); + } + return map; + }(); + + return map; +} + +MantissaRange const& +MantissaRange::getMantissaRange(mantissa_scale scale) +{ + return getRanges().at(scale); +} Number::rounding_mode Number::getround() @@ -51,10 +120,9 @@ Number::getMantissaScale() void Number::setMantissaScale(MantissaRange::mantissa_scale scale) { - if (scale != MantissaRange::mantissa_scale::small && - scale != MantissaRange::mantissa_scale::large) + if (!MantissaRange::getAllScales().contains(scale)) LogicError("Unknown mantissa scale"); - range_ = scale == MantissaRange::mantissa_scale::small ? smallRange : largeRange; + range_ = MantissaRange::getMantissaRange(scale); } // Guard @@ -107,6 +175,7 @@ class Number::Guard int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled, std::string location); // Modify the result to the correctly rounded value @@ -223,12 +292,9 @@ Number::Guard::bringIntoRange( { mantissa *= 10; --exponent; - std::cout << "bringIntoRange. mantissa*=10: " << mantissa << ", exponent: " << exponent - << std::endl; } if (exponent < minExponent) { - std::cout << "bringIntoRange. zero\n"; constexpr Number zero = Number{}; negative = zero.negative_; @@ -245,13 +311,13 @@ Number::Guard::doRoundUp( int& exponent, internalrep const& minMantissa, internalrep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled, std::string location) { auto r = round(); if (r == 1 || (r == 0 && (mantissa & 1) == 1)) { - std::cout << "doRoundUp. r: " << r << std::endl; - if (isFeatureEnabled(fixCleanup3_2_0) || !getCurrentTransactionRules()) + if (cuspRoundingFixEnabled == MantissaRange::cusp_rounding_fix::enabled) { // Ensure mantissa after incrementing fits within both the // min/maxMantissa range and is a valid "rep". @@ -259,7 +325,6 @@ Number::Guard::doRoundUp( { // Nothing unusual here, just increment the mantissa ++mantissa; - std::cout << "\tmantissa++: " << mantissa << std::endl; } else { @@ -275,10 +340,15 @@ Number::Guard::doRoundUp( mantissa < maxMantissa && mantissa < maxRep, "xrpl::Number::Guard::doRoundUp", "can't recurse more than once"); - std::cout << "\tmantissa/=10: " << mantissa << ", exponent: " << exponent - << std::endl; // Here be dragons - doRoundUp(negative, mantissa, exponent, minMantissa, maxMantissa, location); + doRoundUp( + negative, + mantissa, + exponent, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + location); return; } } @@ -371,29 +441,11 @@ Number::externalToInternal(rep mantissa) return static_cast(-temp); } -constexpr Number -Number::oneSmall() -{ - return Number{false, Number::smallRange.min, -Number::smallRange.log, Number::unchecked{}}; -}; - -constexpr Number oneSml = Number::oneSmall(); - -constexpr Number -Number::oneLarge() -{ - return Number{false, Number::largeRange.min, -Number::largeRange.log, Number::unchecked{}}; -}; - -constexpr Number oneLrg = Number::oneLarge(); - Number Number::one() { - if (&range_.get() == &smallRange) - return oneSml; - XRPL_ASSERT(&range_.get() == &largeRange, "Number::one() : valid range_"); - return oneLrg; + auto const& range = range_.get(); + return Number{false, range.min, -range.log, Number::unchecked{}}; } // Use the member names in this static function for now so the diff is cleaner @@ -405,7 +457,8 @@ doNormalize( T& mantissa_, int& exponent_, MantissaRange::rep const& minMantissa, - MantissaRange::rep const& maxMantissa) + MantissaRange::rep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled) { auto constexpr minExponent = Number::minExponent; auto constexpr maxExponent = Number::maxExponent; @@ -474,7 +527,14 @@ doNormalize( XRPL_ASSERT_PARTS(m <= maxRep, "xrpl::doNormalize", "intermediate mantissa fits in int64"); mantissa_ = m; - g.doRoundUp(negative, mantissa_, exponent_, minMantissa, maxMantissa, "Number::normalize 2"); + g.doRoundUp( + negative, + mantissa_, + exponent_, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + "Number::normalize 2"); XRPL_ASSERT_PARTS( mantissa_ >= minMantissa && mantissa_ <= maxMantissa, "xrpl::doNormalize", @@ -488,9 +548,10 @@ Number::normalize( uint128_t& mantissa, int& exponent, internalrep const& minMantissa, - internalrep const& maxMantissa) + internalrep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled); } template <> @@ -500,9 +561,10 @@ Number::normalize( unsigned long long& mantissa, int& exponent, internalrep const& minMantissa, - internalrep const& maxMantissa) + internalrep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled); } template <> @@ -512,16 +574,16 @@ Number::normalize( unsigned long& mantissa, int& exponent, internalrep const& minMantissa, - internalrep const& maxMantissa) + internalrep const& maxMantissa, + MantissaRange::cusp_rounding_fix cuspRoundingFixEnabled) { - doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa); + doNormalize(negative, mantissa, exponent, minMantissa, maxMantissa, cuspRoundingFixEnabled); } void -Number::normalize() +Number::normalize(MantissaRange const& range) { - auto const& range = range_.get(); - normalize(negative_, mantissa_, exponent_, range.min, range.max); + normalize(negative_, mantissa_, exponent_, range.min, range.max, range.cuspRoundingFixEnabled); } // Copy the number, but set a new exponent. Because the mantissa doesn't change, @@ -602,6 +664,7 @@ Number::operator+=(Number const& y) auto const& range = range_.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; + auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; if (xn == yn) { @@ -612,7 +675,14 @@ Number::operator+=(Number const& y) xm /= 10; ++xe; } - g.doRoundUp(xn, xm, xe, minMantissa, maxMantissa, "Number::addition overflow"); + g.doRoundUp( + xn, + xm, + xe, + minMantissa, + maxMantissa, + cuspRoundingFixEnabled, + "Number::addition overflow"); } else { @@ -638,7 +708,7 @@ Number::operator+=(Number const& y) negative_ = xn; mantissa_ = static_cast(xm); exponent_ = xe; - normalize(); + normalize(range); return *this; } @@ -707,8 +777,7 @@ Number::operator*=(Number const& y) auto const& range = range_.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; - - std::cout << "zn: " << zn << ", zm: " << zm << ", ze: " << ze << std::endl; + auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; while (zm > maxMantissa || zm > maxRep) { @@ -719,8 +788,6 @@ Number::operator*=(Number const& y) ++ze; } - std::cout << "zn: " << zn << ", zm: " << zm << ", ze: " << ze << std::endl; - xm = static_cast(zm); xe = ze; g.doRoundUp( @@ -729,12 +796,13 @@ Number::operator*=(Number const& y) xe, minMantissa, maxMantissa, + cuspRoundingFixEnabled, "Number::multiplication overflow : exponent is " + std::to_string(xe)); negative_ = zn; mantissa_ = xm; exponent_ = xe; - normalize(); + normalize(range); return *this; } @@ -766,6 +834,7 @@ Number::operator/=(Number const& y) auto const& range = range_.get(); auto const& minMantissa = range.min; auto const& maxMantissa = range.max; + auto const cuspRoundingFixEnabled = range.cuspRoundingFixEnabled; // Shift by 10^17 gives greatest precision while not overflowing // uint128_t or the cast back to int64_t @@ -773,8 +842,6 @@ Number::operator/=(Number const& y) // log(2^128,10) ~ 38.5 // largeRange.log = 18, fits in 10^19 // f can be up to 10^(38-19) = 10^19 safely - static_assert(smallRange.log == 15); - static_assert(largeRange.log == 18); bool const small = Number::getMantissaScale() == MantissaRange::mantissa_scale::small; uint128_t const f = small ? 100'000'000'000'000'000 : 10'000'000'000'000'000'000ULL; XRPL_ASSERT_PARTS(f >= minMantissa * 10, "Number::operator/=", "factor expected size"); @@ -824,7 +891,7 @@ Number::operator/=(Number const& y) ze -= 3; } } - normalize(zn, zm, ze, minMantissa, maxMantissa); + normalize(zn, zm, ze, minMantissa, maxMantissa, cuspRoundingFixEnabled); negative_ = zn; mantissa_ = static_cast(zm); exponent_ = ze; @@ -876,7 +943,7 @@ Number::truncate() const noexcept } // We are guaranteed that normalize() will never throw an exception // because exponent is either negative or zero at this point. - ret.normalize(); + ret.normalize(range_); return ret; } diff --git a/src/libxrpl/protocol/IOUAmount.cpp b/src/libxrpl/protocol/IOUAmount.cpp index 69b763dbabb..75e83ce8303 100644 --- a/src/libxrpl/protocol/IOUAmount.cpp +++ b/src/libxrpl/protocol/IOUAmount.cpp @@ -56,7 +56,7 @@ IOUAmount::fromNumber(Number const& number) // to normalize, which calls fromNumber IOUAmount result{}; std::tie(result.mantissa_, result.exponent_) = - number.normalizeToRange(minMantissa, maxMantissa); + number.normalizeToRange(); return result; } diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index cf261a631b8..00ef330d835 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -38,11 +38,13 @@ setCurrentTransactionRules(std::optional r) // Make global changes associated with the rules before the value is moved. // Push the appropriate setting, instead of having the class pull every time // the value is needed. That could get expensive fast. - bool const enableLargeNumbers = - !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); + bool const enableCuspRoundingFix = !r || r->enabled(fixCleanup3_2_0); + bool const enableLargeNumbers = enableCuspRoundingFix || + (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); Number::setMantissaScale( - enableLargeNumbers ? MantissaRange::mantissa_scale::large - : MantissaRange::mantissa_scale::small); + enableCuspRoundingFix ? MantissaRange::mantissa_scale::large + : enableLargeNumbers ? MantissaRange::mantissa_scale::largeLegacy + : MantissaRange::mantissa_scale::small); *getCurrentTransactionRulesRef() = std::move(r); } diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index d506751f860..e4cdb5c6ae2 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -95,7 +95,8 @@ 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::mantissa_scale::large, + Number::getMantissaScale() == MantissaRange::mantissa_scale::largeLegacy || + Number::getMantissaScale() == MantissaRange::mantissa_scale::large, "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 36ba3e98ecf..c92a5f9d605 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4253,109 +4253,119 @@ class Invariants_test : public beast::unit_test::suite std::vector values; }; - NumberMantissaScaleGuard const g{MantissaRange::mantissa_scale::large}; + for (auto const mantissaScale : { + MantissaRange::mantissa_scale::largeLegacy, + MantissaRange::mantissa_scale::large, + }) + { + NumberMantissaScaleGuard const g{mantissaScale}; - auto makeDelta = [&vaultAsset](Number const& n) -> ValidVault::DeltaInfo { - return {.delta = n, .scale = scale(n, vaultAsset.raw())}; - }; + auto makeDelta = [&vaultAsset](Number const& n) -> ValidVault::DeltaInfo { + return {.delta = n, .scale = scale(n, vaultAsset.raw())}; + }; - auto const testCases = std::vector{ - { - .name = "No values", - .expectedMinScale = 0, - .values = {}, - }, - { - .name = "Mixed integer and Number values", - .expectedMinScale = -15, - .values = {makeDelta(1), makeDelta(-1), makeDelta(Number{10, -1})}, - }, - { - .name = "Mixed scales", - .expectedMinScale = -17, - .values = - {makeDelta(Number{1, -2}), makeDelta(Number{5, -3}), makeDelta(Number{3, -2})}, - }, - { - .name = "Equal scales", - .expectedMinScale = -16, - .values = - {makeDelta(Number{1, -1}), makeDelta(Number{5, -1}), makeDelta(Number{1, -1})}, - }, - { - .name = "Mixed mantissa sizes", - .expectedMinScale = -12, - .values = - {makeDelta(Number{1}), - makeDelta(Number{1234, -3}), - makeDelta(Number{12345, -6}), - makeDelta(Number{123, 1})}, - }, - }; + auto const testCases = std::vector{ + { + .name = "No values", + .expectedMinScale = 0, + .values = {}, + }, + { + .name = "Mixed integer and Number values", + .expectedMinScale = -15, + .values = {makeDelta(1), makeDelta(-1), makeDelta(Number{10, -1})}, + }, + { + .name = "Mixed scales", + .expectedMinScale = -17, + .values = + {makeDelta(Number{1, -2}), + makeDelta(Number{5, -3}), + makeDelta(Number{3, -2})}, + }, + { + .name = "Equal scales", + .expectedMinScale = -16, + .values = + {makeDelta(Number{1, -1}), + makeDelta(Number{5, -1}), + makeDelta(Number{1, -1})}, + }, + { + .name = "Mixed mantissa sizes", + .expectedMinScale = -12, + .values = + {makeDelta(Number{1}), + makeDelta(Number{1234, -3}), + makeDelta(Number{12345, -6}), + makeDelta(Number{123, 1})}, + }, + }; - for (auto const& tc : testCases) - { - testcase("vault computeCoarsestScale: " + tc.name); + for (auto const& tc : testCases) + { + testcase("vault computeCoarsestScale: " + tc.name); - auto const actualScale = ValidVault::computeCoarsestScale(tc.values); + auto const actualScale = ValidVault::computeCoarsestScale(tc.values); - BEAST_EXPECTS( - actualScale == tc.expectedMinScale, - "expected: " + std::to_string(tc.expectedMinScale) + - ", actual: " + std::to_string(actualScale)); - for (auto const& num : tc.values) - { - // None of these scales are far enough apart that rounding the - // values would lose information, so check that the rounded - // value matches the original. - auto const actualRounded = roundToAsset(vaultAsset, num.delta, actualScale); BEAST_EXPECTS( - actualRounded == num.delta, - "number " + to_string(num.delta) + " rounded to scale " + - std::to_string(actualScale) + " is " + to_string(actualRounded)); + actualScale == tc.expectedMinScale, + "expected: " + std::to_string(tc.expectedMinScale) + + ", actual: " + std::to_string(actualScale)); + for (auto const& num : tc.values) + { + // None of these scales are far enough apart that rounding the + // values would lose information, so check that the rounded + // value matches the original. + auto const actualRounded = roundToAsset(vaultAsset, num.delta, actualScale); + BEAST_EXPECTS( + actualRounded == num.delta, + "number " + to_string(num.delta) + " rounded to scale " + + std::to_string(actualScale) + " is " + to_string(actualRounded)); + } } - } - auto const testCases2 = std::vector{ - { - .name = "False equivalence", - .expectedMinScale = -15, - .values = - { - makeDelta(Number{1234567890123456789, -18}), - makeDelta(Number{12345, -4}), - makeDelta(Number{1}), - }, - }, - }; + auto const testCases2 = std::vector{ + { + .name = "False equivalence", + .expectedMinScale = -15, + .values = + { + makeDelta(Number{1234567890123456789, -18}), + makeDelta(Number{12345, -4}), + makeDelta(Number{1}), + }, + }, + }; - // Unlike the first set of test cases, the values in these test could - // look equivalent if using the wrong scale. - for (auto const& tc : testCases2) - { - testcase("vault computeCoarsestScale: " + tc.name); + // Unlike the first set of test cases, the values in these test could + // look equivalent if using the wrong scale. + for (auto const& tc : testCases2) + { + testcase("vault computeCoarsestScale: " + tc.name); - auto const actualScale = ValidVault::computeCoarsestScale(tc.values); + auto const actualScale = ValidVault::computeCoarsestScale(tc.values); - BEAST_EXPECTS( - actualScale == tc.expectedMinScale, - "expected: " + std::to_string(tc.expectedMinScale) + - ", actual: " + std::to_string(actualScale)); - std::optional first; - Number firstRounded; - for (auto const& num : tc.values) - { - if (!first) + BEAST_EXPECTS( + actualScale == tc.expectedMinScale, + "expected: " + std::to_string(tc.expectedMinScale) + + ", actual: " + std::to_string(actualScale)); + std::optional first; + Number firstRounded; + for (auto const& num : tc.values) { - first = num.delta; - firstRounded = roundToAsset(vaultAsset, num.delta, actualScale); - continue; + if (!first) + { + first = num.delta; + firstRounded = roundToAsset(vaultAsset, num.delta, actualScale); + continue; + } + auto const numRounded = roundToAsset(vaultAsset, num.delta, actualScale); + BEAST_EXPECTS( + numRounded != firstRounded, + "at a scale of " + std::to_string(actualScale) + " " + + to_string(num.delta) + " == " + to_string(*first)); } - auto const numRounded = roundToAsset(vaultAsset, num.delta, actualScale); - BEAST_EXPECTS( - numRounded != firstRounded, - "at a scale of " + std::to_string(actualScale) + " " + to_string(num.delta) + - " == " + to_string(*first)); } } } diff --git a/src/test/basics/IOUAmount_test.cpp b/src/test/basics/IOUAmount_test.cpp index 5c01445e31e..70c9cc6960c 100644 --- a/src/test/basics/IOUAmount_test.cpp +++ b/src/test/basics/IOUAmount_test.cpp @@ -156,8 +156,7 @@ class IOUAmount_test : public beast::unit_test::suite BEAST_EXPECTS(result == expected, ss.str()); }; - for (auto const mantissaSize : - {MantissaRange::mantissa_scale::small, MantissaRange::mantissa_scale::large}) + for (auto const mantissaSize : MantissaRange::getAllScales()) { NumberMantissaScaleGuard const mg(mantissaSize); diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 93e68f67a49..24d3d033f13 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -6,6 +6,8 @@ #include #include +#include + #include #include #include @@ -19,6 +21,24 @@ namespace xrpl { class Number_test : public beast::unit_test::suite { + using BigInt = boost::multiprecision::cpp_int; + + static std::string + fmt(BigInt const& value) + { + auto s = to_string(value); + std::string out; + int count = 0; + for (auto it = s.rbegin(); it != s.rend(); ++it) + { + if (count && count % 3 == 0) + out.insert(out.begin(), '_'); + out.insert(out.begin(), *it); + ++count; + } + return out; + } + public: void testZero() @@ -178,7 +198,6 @@ class Number_test : public beast::unit_test::suite {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::maxRep}, Number{6, -1}, Number{Number::maxRep / 10, 1}}, {Number{Number::maxRep - 1}, Number{1, 0}, Number{Number::maxRep}}, // Test extremes { @@ -199,6 +218,12 @@ class Number_test : public beast::unit_test::suite Number{false, 1'999'999'999'999'999'998ULL, 1, Number::normalized{}}, }, }); + auto const cLargeLegacy = std::to_array({ + {Number{Number::maxRep}, Number{6, -1}, Number{Number::maxRep / 10, 1}}, + }); + auto const cLargeCorrected = std::to_array({ + {Number{Number::maxRep}, Number{6, -1}, Number{Number::maxRep / 10 + 1, 1}}, + }); auto test = [this](auto const& c) { for (auto const& [x, y, z] : c) { @@ -215,6 +240,10 @@ class Number_test : public beast::unit_test::suite else { test(cLarge); + if (scale == MantissaRange::mantissa_scale::largeLegacy) + test(cLargeLegacy); + else + test(cLargeCorrected); } { bool caught = false; @@ -1266,6 +1295,7 @@ class Number_test : public beast::unit_test::suite "9223372036854775e3"); } break; + case MantissaRange::mantissa_scale::largeLegacy: case MantissaRange::mantissa_scale::large: // Test the edges // ((exponent < -(28)) || (exponent > -(8))))) @@ -1551,101 +1581,20 @@ class Number_test : public beast::unit_test::suite } } - void - run() override - { - for (auto const scale : - {MantissaRange::mantissa_scale::small, MantissaRange::mantissa_scale::large}) - { - NumberMantissaScaleGuard const sg(scale); - testZero(); - test_limits(); - testToString(); - test_add(); - test_sub(); - test_mul(); - test_div(); - test_root(); - test_root2(); - test_power1(); - test_power2(); - testConversions(); - test_to_integer(); - test_squelch(); - test_relationals(); - test_stream(); - test_inc_dec(); - test_toSTAmount(); - test_truncate(); - testRounding(); - testInt64(); - } - } -}; - -BEAST_DEFINE_TESTSUITE(Number, basics, xrpl); - -} // namespace xrpl -#include -#include -#include -#include -#include - -#include - -#include -#include -#include -#include - -namespace xrpl { - -class NumberUpwardWrongDirection_test : public beast::unit_test::suite -{ - using BigInt = boost::multiprecision::cpp_int; - - static std::string - fmt(BigInt const& value) - { - std::ostringstream os; - os << value; - auto s = os.str(); - std::string out; - int count = 0; - for (auto it = s.rbegin(); it != s.rend(); ++it) - { - if (count && count % 3 == 0) - out.insert(out.begin(), '_'); - out.insert(out.begin(), *it); - ++count; - } - return out; - } - -public: void testUpwardRoundsDown() { testcase << "upward rounding produces a value below exact at maxRep cusp"; - auto const origScale = Number::getMantissaScale(); - auto const origRound = Number::setround(Number::upward); - Number::setMantissaScale(MantissaRange::large); + NumberMantissaScaleGuard const mg{MantissaRange::mantissa_scale::large}; + NumberRoundModeGuard const rg{Number::rounding_mode::upward}; constexpr std::int64_t aValue = 1'000'000'000'000'049'863LL; constexpr std::int64_t bValue = 9'223'372'036'854'315'903LL; - // JSON -> STAmount -> Number - AccountID const dummyIssuer = AccountID{42u}; - MPTIssue const issue(/*sequence=*/1u, dummyIssuer); - - STAmount const amountA{MPTAmount{aValue}, issue}; - STAmount const amountB{MPTAmount{bValue}, issue}; - // Public conversion operator: STAmount::operator Number() const. - Number const a = amountA; - Number const b = amountB; + Number const a = aValue; + Number const b = bValue; Number const product = a * b; // Exact reference in BigInt. @@ -1669,18 +1618,41 @@ class NumberUpwardWrongDirection_test : public beast::unit_test::suite BEAST_EXPECT(signedDifference >= 0); BEAST_EXPECT(product.mantissa() == (std::numeric_limits::max() / 10) + 1); BEAST_EXPECT(product.exponent() == 19); - - Number::setround(origRound); - Number::setMantissaScale(origScale); } void run() override { + for (auto const scale : MantissaRange::getAllScales()) + { + NumberMantissaScaleGuard const sg(scale); + testZero(); + test_limits(); + testToString(); + test_add(); + test_sub(); + test_mul(); + test_div(); + test_root(); + test_root2(); + test_power1(); + test_power2(); + testConversions(); + test_to_integer(); + test_squelch(); + test_relationals(); + test_stream(); + test_inc_dec(); + test_toSTAmount(); + test_truncate(); + testRounding(); + testInt64(); + } + // This test sets its own number range testUpwardRoundsDown(); } }; -BEAST_DEFINE_TESTSUITE(NumberUpwardWrongDirection, basics, ripple); +BEAST_DEFINE_TESTSUITE(Number, basics, xrpl); } // namespace xrpl diff --git a/src/test/protocol/STNumber_test.cpp b/src/test/protocol/STNumber_test.cpp index e2c1a91e645..6fc17dca9aa 100644 --- a/src/test/protocol/STNumber_test.cpp +++ b/src/test/protocol/STNumber_test.cpp @@ -280,8 +280,7 @@ struct STNumber_test : public beast::unit_test::suite { static_assert(!std::is_convertible_v); - for (auto const scale : - {MantissaRange::mantissa_scale::small, MantissaRange::mantissa_scale::large}) + for (auto const scale : MantissaRange::getAllScales()) { NumberMantissaScaleGuard const sg(scale); testcase << to_string(Number::getMantissaScale()); diff --git a/src/test/rpc/GetAggregatePrice_test.cpp b/src/test/rpc/GetAggregatePrice_test.cpp index ea8c4986194..e926b3b6071 100644 --- a/src/test/rpc/GetAggregatePrice_test.cpp +++ b/src/test/rpc/GetAggregatePrice_test.cpp @@ -177,8 +177,7 @@ class GetAggregatePrice_test : public beast::unit_test::suite auto const all = testable_amendments(); for (auto const& feats : {all - featureSingleAssetVault - featureLendingProtocol, all}) { - for (auto const mantissaSize : - {MantissaRange::mantissa_scale::small, MantissaRange::mantissa_scale::large}) + for (auto const mantissaSize : MantissaRange::getAllScales()) { // Regardless of the features enabled, RPC is controlled by // the global mantissa size. And since it's a thread-local, From 9801e7c4247823e06122be16ed57817991104223 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 4 May 2026 22:34:46 -0400 Subject: [PATCH 3/4] Change the amendment priority for cusp rounding fix - Fix is only enabled if SAV or LP is also enabled - Basically a lazy way to avoid breaking AMM unit tests --- src/libxrpl/protocol/Rules.cpp | 6 +- src/test/app/AMMExtended_test.cpp | 109 ++++++++++++------------------ src/test/app/AMM_test.cpp | 9 --- 3 files changed, 48 insertions(+), 76 deletions(-) diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index 00ef330d835..c638f90d9dd 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -38,9 +38,9 @@ setCurrentTransactionRules(std::optional r) // Make global changes associated with the rules before the value is moved. // Push the appropriate setting, instead of having the class pull every time // the value is needed. That could get expensive fast. - bool const enableCuspRoundingFix = !r || r->enabled(fixCleanup3_2_0); - bool const enableLargeNumbers = enableCuspRoundingFix || - (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); + bool const enableLargeNumbers = + !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); + bool const enableCuspRoundingFix = enableLargeNumbers && (!r || r->enabled(fixCleanup3_2_0)); Number::setMantissaScale( enableCuspRoundingFix ? MantissaRange::mantissa_scale::large : enableLargeNumbers ? MantissaRange::mantissa_scale::largeLegacy diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 9eb7eb16202..4162c451d97 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -70,6 +70,11 @@ struct AMMExtended_test : public jtx::AMMTest // Use small Number mantissas for the life of this test. NumberMantissaScaleGuard const sg_{xrpl::MantissaRange::mantissa_scale::small}; + // For now, just disable SAV entirely, which locks in the small Number + // mantissas + FeatureBitset const all_{ + testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; + private: void testRmFundedOffer(FeatureBitset features) @@ -1348,37 +1353,33 @@ struct AMMExtended_test : public jtx::AMMTest testOffers() { using namespace jtx; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas - FeatureBitset const all{ - testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; - - testRmFundedOffer(all); - testRmFundedOffer(all - fixAMMv1_1 - fixAMMv1_3); - testEnforceNoRipple(all); - testFillModes(all); - testOfferCrossWithXRP(all); - testOfferCrossWithLimitOverride(all); - testCurrencyConversionEntire(all); - testCurrencyConversionInParts(all); - testCrossCurrencyStartXRP(all); - testCrossCurrencyEndXRP(all); - testCrossCurrencyBridged(all); - testOfferFeesConsumeFunds(all); - testOfferCreateThenCross(all); - testSellFlagExceedLimit(all); - testGatewayCrossCurrency(all); - testGatewayCrossCurrency(all - fixAMMv1_1 - fixAMMv1_3); - testBridgedCross(all); - testSellWithFillOrKill(all); - testTransferRateOffer(all); - testSelfIssueOffer(all); - testBadPathAssert(all); - testSellFlagBasic(all); - testDirectToDirectPath(all); - testDirectToDirectPath(all - fixAMMv1_1 - fixAMMv1_3); - testRequireAuth(all); - testMissingAuth(all); + + testRmFundedOffer(all_); + testRmFundedOffer(all_ - fixAMMv1_1 - fixAMMv1_3); + testEnforceNoRipple(all_); + testFillModes(all_); + testOfferCrossWithXRP(all_); + testOfferCrossWithLimitOverride(all_); + testCurrencyConversionEntire(all_); + testCurrencyConversionInParts(all_); + testCrossCurrencyStartXRP(all_); + testCrossCurrencyEndXRP(all_); + testCrossCurrencyBridged(all_); + testOfferFeesConsumeFunds(all_); + testOfferCreateThenCross(all_); + testSellFlagExceedLimit(all_); + testGatewayCrossCurrency(all_); + testGatewayCrossCurrency(all_ - fixAMMv1_1 - fixAMMv1_3); + testBridgedCross(all_); + testSellWithFillOrKill(all_); + testTransferRateOffer(all_); + testSelfIssueOffer(all_); + testBadPathAssert(all_); + testSellFlagBasic(all_); + testDirectToDirectPath(all_); + testDirectToDirectPath(all_ - fixAMMv1_1 - fixAMMv1_3); + testRequireAuth(all_); + testMissingAuth(all_); } void @@ -3491,15 +3492,11 @@ struct AMMExtended_test : public jtx::AMMTest testFlow() { using namespace jtx; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas in the transaction engine - FeatureBitset const all{ - testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; - - testFalseDry(all); - testBookStep(all); - testTransferRateNoOwnerFee(all); - testTransferRateNoOwnerFee(all - fixAMMv1_1 - fixAMMv1_3); + + testFalseDry(all_); + testBookStep(all_); + testTransferRateNoOwnerFee(all_); + testTransferRateNoOwnerFee(all_ - fixAMMv1_1 - fixAMMv1_3); testLimitQuality(); testXRPPathLoop(); } @@ -3508,34 +3505,22 @@ struct AMMExtended_test : public jtx::AMMTest testCrossingLimits() { using namespace jtx; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas in the transaction engine - FeatureBitset const all{ - testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; - testStepLimit(all); - testStepLimit(all - fixAMMv1_1 - fixAMMv1_3); + testStepLimit(all_); + testStepLimit(all_ - fixAMMv1_1 - fixAMMv1_3); } void testDeliverMin() { using namespace jtx; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas in the transaction engine - FeatureBitset const all{ - testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; - test_convert_all_of_an_asset(all); - test_convert_all_of_an_asset(all - fixAMMv1_1 - fixAMMv1_3); + test_convert_all_of_an_asset(all_); + test_convert_all_of_an_asset(all_ - fixAMMv1_1 - fixAMMv1_3); } void testDepositAuth() { - // For now, just disable SAV entirely, which locks in the small Number - // mantissas in the transaction engine - FeatureBitset const all{ - jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; - testPayment(all); + testPayment(all_); testPayIOU(); } @@ -3543,13 +3528,9 @@ struct AMMExtended_test : public jtx::AMMTest testFreeze() { using namespace test::jtx; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas in the transaction engine - FeatureBitset const sa{ - testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; - testRippleState(sa); - testGlobalFreeze(sa); - testOffersWhenFrozen(sa); + testRippleState(all_); + testGlobalFreeze(all_); + testOffersWhenFrozen(all_); } void diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index b25705beccf..c45a5c5ffdd 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -2619,10 +2619,6 @@ struct AMM_test : public jtx::AMMTest using namespace jtx; using namespace std::chrono; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas - features = features - featureSingleAssetVault - featureLendingProtocol; - // Auction slot initially is owned by AMM creator, who pays 0 price. // Bid 110 tokens. Pay bidMin. @@ -3332,11 +3328,6 @@ struct AMM_test : public jtx::AMMTest testcase("Basic Payment"); using namespace jtx; - // For now, just disable SAV entirely, which locks in the small Number - // mantissas - features = - features - featureSingleAssetVault - featureLendingProtocol - featureLendingProtocol; - // Payment 100USD for 100XRP. // Force one path with tfNoRippleDirect. testAMM( From 791661483923514caf1ebb1c39fc2c726848e629 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 4 May 2026 22:58:24 -0400 Subject: [PATCH 4/4] Go back to original priority, and ignore fixCleanup3_2_0 in AMM tests --- src/libxrpl/protocol/Rules.cpp | 6 +++--- src/test/app/AMMClawbackMPT_test.cpp | 2 +- src/test/app/AMMClawback_test.cpp | 4 ++-- src/test/app/AMMExtended_test.cpp | 2 +- src/test/app/AMM_test.cpp | 3 ++- src/test/jtx/AMMTest.h | 6 ++++-- src/test/jtx/impl/AMMTest.cpp | 2 +- src/test/rpc/GetAggregatePrice_test.cpp | 3 ++- 8 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index c638f90d9dd..00ef330d835 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -38,9 +38,9 @@ setCurrentTransactionRules(std::optional r) // Make global changes associated with the rules before the value is moved. // Push the appropriate setting, instead of having the class pull every time // the value is needed. That could get expensive fast. - bool const enableLargeNumbers = - !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); - bool const enableCuspRoundingFix = enableLargeNumbers && (!r || r->enabled(fixCleanup3_2_0)); + bool const enableCuspRoundingFix = !r || r->enabled(fixCleanup3_2_0); + bool const enableLargeNumbers = enableCuspRoundingFix || + (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); Number::setMantissaScale( enableCuspRoundingFix ? MantissaRange::mantissa_scale::large : enableLargeNumbers ? MantissaRange::mantissa_scale::largeLegacy diff --git a/src/test/app/AMMClawbackMPT_test.cpp b/src/test/app/AMMClawbackMPT_test.cpp index 393c0b062cd..f2479d4fdb6 100644 --- a/src/test/app/AMMClawbackMPT_test.cpp +++ b/src/test/app/AMMClawbackMPT_test.cpp @@ -1829,7 +1829,7 @@ class AMMClawbackMPT_test : public beast::unit_test::suite testLastHolderLPTokenBalance(all - fixAMMv1_3 - fixAMMClawbackRounding); testLastHolderLPTokenBalance( all - fixAMMv1_3 - fixAMMClawbackRounding - featureSingleAssetVault - - featureLendingProtocol); + featureLendingProtocol - fixCleanup3_2_0); testLastHolderLPTokenBalance(all - fixAMMClawbackRounding); testClawAssetCheck(all); } diff --git a/src/test/app/AMMClawback_test.cpp b/src/test/app/AMMClawback_test.cpp index bf08b57bf32..e3912490d3f 100644 --- a/src/test/app/AMMClawback_test.cpp +++ b/src/test/app/AMMClawback_test.cpp @@ -2506,8 +2506,8 @@ class AMMClawback_test : public beast::unit_test::suite { // For now, just disable SAV entirely, which locks in the small Number // mantissas - FeatureBitset const all = - jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol; + FeatureBitset const all = jtx::testable_amendments() - featureSingleAssetVault - + featureLendingProtocol - fixCleanup3_2_0; testInvalidRequest(all); testInvalidRequest(all - featureMPTokensV2); diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 4162c451d97..a9aac690a07 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -73,7 +73,7 @@ struct AMMExtended_test : public jtx::AMMTest // For now, just disable SAV entirely, which locks in the small Number // mantissas FeatureBitset const all_{ - testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; + testable_amendments() - featureSingleAssetVault - featureLendingProtocol - fixCleanup3_2_0}; private: void diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index c45a5c5ffdd..3df3a48d032 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -81,7 +81,8 @@ struct AMM_test : public jtx::AMMTest { // For now, just disable SAV entirely, which locks in the small Number // mantissas - return jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol; + return jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol - + fixCleanup3_2_0; } void diff --git a/src/test/jtx/AMMTest.h b/src/test/jtx/AMMTest.h index ffb186e0d63..7ef5db959af 100644 --- a/src/test/jtx/AMMTest.h +++ b/src/test/jtx/AMMTest.h @@ -21,7 +21,8 @@ struct TestAMMArg std::vector features = { // For now, just disable SAV entirely, which locks in the small Number // mantissas - jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol}; + jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol - + fixCleanup3_2_0}; bool noLog = false; }; @@ -87,7 +88,8 @@ class AMMTestBase : public beast::unit_test::suite { // For now, just disable SAV entirely, which locks in the small Number // mantissas - return jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol; + return jtx::testable_amendments() - featureSingleAssetVault - featureLendingProtocol - + fixCleanup3_2_0; } protected: diff --git a/src/test/jtx/impl/AMMTest.cpp b/src/test/jtx/impl/AMMTest.cpp index dbd8b690f79..14808160e59 100644 --- a/src/test/jtx/impl/AMMTest.cpp +++ b/src/test/jtx/impl/AMMTest.cpp @@ -143,7 +143,7 @@ AMMTestBase::testAMM(std::function const& cb, TestAM // mantissas Env env{ *this, - features - featureSingleAssetVault - featureLendingProtocol, + features - featureSingleAssetVault - featureLendingProtocol - fixCleanup3_2_0, arg.noLog ? std::make_unique(&logs) : nullptr}; auto const [asset1, asset2] = arg.pool ? *arg.pool : std::make_pair(XRP(10000), USD(10000)); diff --git a/src/test/rpc/GetAggregatePrice_test.cpp b/src/test/rpc/GetAggregatePrice_test.cpp index e926b3b6071..d4c0f2acea0 100644 --- a/src/test/rpc/GetAggregatePrice_test.cpp +++ b/src/test/rpc/GetAggregatePrice_test.cpp @@ -175,7 +175,8 @@ class GetAggregatePrice_test : public beast::unit_test::suite // or time threshold { auto const all = testable_amendments(); - for (auto const& feats : {all - featureSingleAssetVault - featureLendingProtocol, all}) + for (auto const& feats : + {all - featureSingleAssetVault - featureLendingProtocol - fixCleanup3_2_0, all}) { for (auto const mantissaSize : MantissaRange::getAllScales()) {