From 9b3dd7002dfd3d0a80b66539edb0b5cbf7c6e403 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 17 Jun 2026 13:06:40 -0400 Subject: [PATCH 01/12] fix: Allocate TaggedCache::getKeys() memory outside of lock - Uses a loop in case the size grows while the lock is free. Guarantees the result vector will not need to allocate under lock. --- include/xrpl/basics/TaggedCache.ipp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index cee02749c6..ef9942ab86 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -2,6 +2,7 @@ #include #include +#include namespace xrpl { @@ -536,8 +537,15 @@ TaggedCache v; { - std::scoped_lock const lock(mutex_); - v.reserve(cache_.size()); + std::unique_lock lock(mutex_); + for (int size = cache_.size(); v.capacity() < size; size = cache_.size()) + { + ScopeUnlock const unlock(lock); + v.reserve(size); + } + XRPL_ASSERT(lock.owns_lock(), "xrpl::TaggedCache::getKeys(): owns lock"); + XRPL_ASSERT( + v.capacity() >= cache_.size(), "xrpl::TaggedCache::getKeys(): sufficient capacity"); for (auto const& _ : cache_) v.push_back(_.first); } From 054284701e88e134ebb6481109eab9eb6eb56530 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 17 Jun 2026 15:20:48 -0400 Subject: [PATCH 02/12] Apply suggestion from @xrplf-ai-reviewer[bot] Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> --- include/xrpl/basics/TaggedCache.ipp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index ef9942ab86..8e235dbb23 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -538,7 +538,7 @@ TaggedCache Date: Tue, 30 Jun 2026 16:13:11 -0400 Subject: [PATCH 03/12] Make the getKeys() allocation more robust - Make it very unlikely that the allocation loop will ever need to run more than once. Recover gracefully if it does. - Prevent livelock by limiting the total possible number of iterations. - If this ever happens, throw our metaphorical hands in the air, and allocate under lock. - Pad the size before allocating to give the cache a little room to grow while not holding the lock. - Pad by less on each iteration. - Assert that no more than two iterations occur. Even two is probably overkill, but this allows for rare edge case scenarios. e.g. The cache is very small, the allocation takes a long time (which is contradictory) and a handful of items get added, overcoming the padding. --- include/xrpl/basics/TaggedCache.ipp | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 4b7e1fe5fb..8ba50c8f87 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -596,11 +596,33 @@ TaggedCache v; { + // Keep track of how many iterations are needed. Exit the loop if the number of retries gets + // absurd. (Note that if this somehow ever happens, one more allocation will be done under + // lock, which is undesirable, but really should be almost impossible. Also, assert that + // there were fewer than 3 needed after the loop, because in a normal operating environment, + // even 2 is going to be unusual, and 3 shouldn't be needed. + std::size_t allocationIterations = 0; std::unique_lock lock(mutex_); - for (auto size = cache_.size(); v.capacity() < size; size = cache_.size()) + for (auto size = cache_.size(); v.capacity() < size && allocationIterations < 20; + size = cache_.size()) { ScopeUnlock const unlock(lock); + // Allocate the current size plus a little extra, in case the cache grows while + // allocating. Each time another allocation is needed, the extra also gets bigger until + // it ultimately doubles the size + 1. + size += (size >> (4 - std::min(allocationIterations, 4ul))) + 1; v.reserve(size); + ++allocationIterations; + } + XRPL_ASSERT( + allocationIterations < 3, + "xrpl::TaggedCache::getKeys(): limited allocation iterations"); + if (v.capacity() < cache_.size()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::TaggedCache::getKeys(): failed to allocate sufficient capacity"); + v.reserve(cache_.size()); + // LCOV_EXCL_STOP } XRPL_ASSERT(lock.owns_lock(), "xrpl::TaggedCache::getKeys(): owns lock"); XRPL_ASSERT( From c2e54d12e9a8c1a49c10e4d6042df7a6d12f4270 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 30 Jun 2026 16:25:18 -0400 Subject: [PATCH 04/12] Use the right type in include/xrpl/basics/TaggedCache.ipp Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> --- include/xrpl/basics/TaggedCache.ipp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 8ba50c8f87..507f3dc5b3 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -610,7 +610,7 @@ TaggedCache> (4 - std::min(allocationIterations, 4ul))) + 1; + size += (size >> (4 - std::min(allocationIterations, std::size_t{4}))) + 1; v.reserve(size); ++allocationIterations; } From 1a3d460046aa75075aeafa28626f2d12db1f77e6 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 30 Jun 2026 16:26:43 -0400 Subject: [PATCH 05/12] Add missed header --- include/xrpl/basics/TaggedCache.ipp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 507f3dc5b3..0d523feffe 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -4,6 +4,8 @@ #include #include +#include + namespace xrpl { namespace detail { From 5d2bc88a2e6b66978982d47cb72a35a95bb8959d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 1 Jul 2026 19:05:01 -0400 Subject: [PATCH 06/12] Document the reasons for the post-allocation assert - Hopefully the AI bots will stop telling me to change it now. --- include/xrpl/basics/TaggedCache.ipp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 0d523feffe..7d7b9724a8 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -600,9 +600,7 @@ TaggedCache Date: Tue, 14 Jul 2026 14:16:46 -0400 Subject: [PATCH 07/12] fix: Document and assert "after" is never null in invariants (#7354) Co-authored-by: Bart --- include/xrpl/tx/invariants/InvariantCheck.h | 15 ++++-- src/libxrpl/tx/invariants/InvariantCheck.cpp | 56 +++++++++++--------- 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/include/xrpl/tx/invariants/InvariantCheck.h b/include/xrpl/tx/invariants/InvariantCheck.h index 150c0ed510..1239305e79 100644 --- a/include/xrpl/tx/invariants/InvariantCheck.h +++ b/include/xrpl/tx/invariants/InvariantCheck.h @@ -71,9 +71,18 @@ public: /** * @brief called for each ledger entry in the current transaction. * - * @param isDelete true if the SLE is being deleted - * @param before ledger entry before modification by the transaction - * @param after ledger entry after modification by the transaction + * @param isDelete true if the SLE is being deleted. + * @param before ledger entry before modification by the transaction. `before` will be null if + * the entry is new. + * @param after ledger entry after modification by the transaction. Always non-null. When + * deleting, `after` may differ from `before`. Whether that is important is up to the + * individual invariant check. + * + * @note `after` IS NEVER NULL. `isDelete` is the only correct way to check for deletions. + * Do not make logic or branching decisions on whether on `after` is set, because it will + * always be set. Treat a null `after` as a programming error (with XRPL_ASSERT). An + * invariant MAY check for null defensively, if it makes more sense, but an assertion is + * preferred for new invariants. */ void visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after); diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 1032644132..3615594d19 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -162,33 +162,37 @@ XRPNotCreated::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref a } } - if (after) + if (!after) { - switch (after->getType()) - { - case ltACCOUNT_ROOT: - drops_ += (*after)[sfBalance].xrp().drops(); - break; - case ltPAYCHAN: - if (!isDelete) - drops_ += ((*after)[sfAmount] - (*after)[sfBalance]).xrp().drops(); - break; - case ltESCROW: - if (!isDelete && isXRP((*after)[sfAmount])) - drops_ += (*after)[sfAmount].xrp().drops(); - break; - case ltSPONSORSHIP: - if (!isDelete && after->isFieldPresent(sfFeeAmount)) - { - XRPL_ASSERT( - isXRP((*after)[sfFeeAmount]), - "XRPNotCreated::visitEntry : Sponsorship.FeeAmount is XRP"); - drops_ += (*after)[sfFeeAmount].xrp().drops(); - } - break; - default: - break; - } + // LCOV_EXCL_START + UNREACHABLE("xrpl::XRPNotCreated::visitEntry : after can't be null"); + return; + // LCOV_EXCL_STOP + } + switch (after->getType()) + { + case ltACCOUNT_ROOT: + drops_ += (*after)[sfBalance].xrp().drops(); + break; + case ltPAYCHAN: + if (!isDelete) + drops_ += ((*after)[sfAmount] - (*after)[sfBalance]).xrp().drops(); + break; + case ltESCROW: + if (!isDelete && isXRP((*after)[sfAmount])) + drops_ += (*after)[sfAmount].xrp().drops(); + break; + case ltSPONSORSHIP: + if (!isDelete && after->isFieldPresent(sfFeeAmount)) + { + XRPL_ASSERT( + isXRP((*after)[sfFeeAmount]), + "XRPNotCreated::visitEntry : Sponsorship.FeeAmount is XRP"); + drops_ += (*after)[sfFeeAmount].xrp().drops(); + } + break; + default: + break; } } From f10dd7b450b574109f70f651b7035e736cad7df1 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 14 Jul 2026 14:47:41 -0400 Subject: [PATCH 08/12] fix: Handle rounding just above kMaxRep more accurately (#7389) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> --- include/xrpl/basics/Number.h | 16 +- src/libxrpl/basics/Number.cpp | 207 +++++++--- src/libxrpl/protocol/Rules.cpp | 2 +- src/libxrpl/protocol/STNumber.cpp | 43 +- src/test/app/LoanBroker_test.cpp | 76 +++- src/test/app/Vault_test.cpp | 166 +++++--- src/test/protocol/STNumber_test.cpp | 54 ++- src/tests/libxrpl/basics/Number.cpp | 585 ++++++++++++++++++++++++---- 8 files changed, 946 insertions(+), 203 deletions(-) diff --git a/include/xrpl/basics/Number.h b/include/xrpl/basics/Number.h index 0026e7f006..f90800c715 100644 --- a/include/xrpl/basics/Number.h +++ b/include/xrpl/basics/Number.h @@ -364,6 +364,8 @@ public: static constexpr internalrep kMaxRep = std::numeric_limits::max(); static_assert(kMaxRep == 9'223'372'036'854'775'807); static_assert(-kMaxRep == std::numeric_limits::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 @@ -591,6 +593,13 @@ public: std::pair normalizeToRange() const; + // Safely convert rep (int64) mantissa to internalrep (uint64). If the rep + // is negative, returns the positive value. This takes a little extra work + // because converting std::numeric_limits::min() flirts with + // UB, and can vary across compilers. + static internalrep + externalToInternal(rep mantissa); + private: static thread_local RoundingMode mode; // The available ranges for mantissa @@ -645,13 +654,6 @@ private: // exponent could go out of range, so it will be checked. [[nodiscard]] Number shiftExponent(int exponentDelta) const; - - // Safely convert rep (int64) mantissa to internalrep (uint64). If the rep - // is negative, returns the positive value. This takes a little extra work - // because converting std::numeric_limits::min() flirts with - // UB, and can vary across compilers. - static internalrep - externalToInternal(rep mantissa); }; 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 d08fd23016..1f2c41809a 100644 --- a/src/libxrpl/basics/Number.cpp +++ b/src/libxrpl/basics/Number.cpp @@ -277,6 +277,25 @@ public: void doDropDigit(T& mantissa, int& exponent) noexcept; + // Modify the result to the correctly rounded value + template + void + 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) const; + + // Modify the result to the correctly rounded value + void + doRound(rep& drops, std::string location) const; + +private: + template + void + pushOverflow(T mantissa); + enum class Round { // The result is exact. No rounding is needed. Only used if cuspRoundingFix is Enabled330 or // higher. @@ -289,37 +308,22 @@ 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 - // 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 - void - 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); - - // Modify the result to the correctly rounded value - void - doRound(rep& drops, std::string location) const; - -private: void doPush(unsigned d) noexcept; template void - bringIntoRange(bool& negative, T& mantissa, int& exponent); + bringIntoRange(bool& negative, T& mantissa, int& exponent) const; }; inline void @@ -349,6 +353,7 @@ Number::Guard::isNegative() const noexcept inline void Number::Guard::doPush(unsigned d) noexcept { + XRPL_ASSERT(d < 10, "xrpl::Number::Guard::doPush : valid digit"); xbit_ = xbit_ || ((digits_ & 0x0000'0000'0000'000F) != 0); digits_ >>= 4; digits_ |= (d & 0x0000'0000'0000'000FULL) << 60; @@ -396,10 +401,69 @@ Number::Guard::doDropDigit(uint128_t& mantissa, int& exponent) noexce ++exponent; } +template +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 possibly round the mantissa up. + // 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 don't want + // to bump up to kMaxRepUp. + if (mantissa % 10 < 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; + } + } + + // The second step scales the final digit of the updated mantissa proportionally, converting + // from (kMaxRep, kMaxRepUp) to (0 to 9]. It then pushes that scaled digit onto the guard as + // if it was a digit that got removed, but doesn't actually remove it. This method should be + // future-proof in case the number of mantissa bits ever changes. (Though for integer values + // of the form 2^(2^x-1), 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 const digit = static_cast((diff * 10) / spread); + XRPL_ASSERT( + digit < 10u && 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 { @@ -445,17 +509,23 @@ Number::Guard::round() const noexcept template void -Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) +Number::Guard::bringIntoRange(bool& negative, T& mantissa, int& exponent) const { // Bring mantissa back into the minMantissa / maxMantissa range AFTER - // rounding - if (mantissa < minMantissa) + // rounding. + if (mantissa < minMantissa && + (cuspRoundingFix < MantissaRange::CuspRoundingFix::Enabled330 || mantissa != 0)) { mantissa *= 10; --exponent; } - if (exponent < kMinExponent) + // mantissa should never be 0, but if it _is_ assert, but fall back to making the result kZero. + if (exponent < kMinExponent || + (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa == 0)) { + // Engineers: If you hit this assert, you probably did something wrong in the operation + // leading up to the rounding work. + XRPL_ASSERT(mantissa != 0, "xrpl::Number::Guard::bringIntoRange : valid mantissa"); static constexpr Number kZero = Number{}; negative = kZero.negative_; @@ -468,7 +538,9 @@ template 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) { @@ -485,18 +557,29 @@ 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) + { + // When rounding up a value in between kMaxRep, and kMaxRepUp, round to + // kMaxRepUp. Note that the decision for this rounding is dominated by the + // results of pushOverflow. + 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 @@ -514,6 +597,14 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string } } } + else if ( + cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330 && mantissa > kMaxRep && + mantissa < kMaxRepUp) + { + // When rounding down a value in between kMaxRep, and kMaxRepUp, round to kMaxRep. + // Note that the decision for this rounding is dominated by the results of pushOverflow. + mantissa = kMaxRep; + } bringIntoRange(negative, mantissa, exponent); if (exponent > kMaxExponent) Throw(std::string(location)); @@ -521,8 +612,10 @@ Number::Guard::doRoundUp(bool& negative, T& mantissa, int& exponent, std::string template void -Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) +Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) const { + // Do not pushOverflow here. + auto r = round(); if (cuspRoundingFix >= MantissaRange::CuspRoundingFix::Enabled330) { @@ -557,6 +650,8 @@ Number::Guard::doRoundDown(bool& negative, T& mantissa, int& exponent) void Number::Guard::doRound(rep& drops, std::string location) const { + // Do not pushOverflow here. + auto r = round(); if (r == Round::Up || (r == Round::Even && (drops & 1) == 1)) { @@ -573,6 +668,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; } @@ -622,7 +719,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; @@ -672,17 +771,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"); @@ -814,6 +913,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. @@ -898,7 +1000,7 @@ Number::operator+=(Number const& y) } else { - if (xm > maxMantissa || xm > kMaxRep) + if (xm > maxMantissa || xm > repLimit) { g.doDropDigit(xm, xe); } @@ -942,7 +1044,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(); @@ -1016,8 +1118,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); } @@ -1282,8 +1386,11 @@ to_string(Number const& amount) } std::string ret = negative ? "-" : ""; ret.append(std::to_string(mantissa)); - ret.append(1, 'e'); - ret.append(std::to_string(exponent)); + if (exponent != 0) + { + ret.append(1, 'e'); + ret.append(std::to_string(exponent)); + } return ret; } diff --git a/src/libxrpl/protocol/Rules.cpp b/src/libxrpl/protocol/Rules.cpp index d71bb77f66..197139027a 100644 --- a/src/libxrpl/protocol/Rules.cpp +++ b/src/libxrpl/protocol/Rules.cpp @@ -45,7 +45,7 @@ setCurrentTransactionRules(std::optional r) // amendments must also be added to useRulesGuards. bool const enableLargeNumbers = !r || (r->enabled(featureSingleAssetVault) || r->enabled(featureLendingProtocol)); - // If enableLargeNumbers is true, then useRulesGuard must also return true. + // If enableLargeNumbers is true, then useRulesGuards must also return true. // However, the reverse is not true. Other amendments can cause the rules guard to be used, // even though large numbers are _not_ used. XRPL_ASSERT( diff --git a/src/libxrpl/protocol/STNumber.cpp b/src/libxrpl/protocol/STNumber.cpp index bd7f67649b..7bf98f270c 100644 --- a/src/libxrpl/protocol/STNumber.cpp +++ b/src/libxrpl/protocol/STNumber.cpp @@ -255,8 +255,47 @@ numberFromJson(SField const& field, json::Value const& value) Throw("not a number"); } - return STNumber{ - field, Number{parts.negative, parts.mantissa, parts.exponent, Number::Normalized{}}}; + Number const num{parts.negative, parts.mantissa, parts.exponent, Number::Normalized{}}; + + // Canonicalize "parts" and "num" with each other by getting rid of trailing 0s until either the + // exponents match, or there are no more 0s. If the two results don't match exactly, then the + // value has been rounded one way or another, and should not be used, because it may lead to an + // unexpected result. canonicalizeParts is not to be confused with Number::canonicalize, because + // they have completely different goals. + auto canonicalizeParts = [](NumberParts p, int otherExponent) { + if (p.mantissa == 0) + return NumberParts{}; + + while (p.exponent < otherExponent && p.mantissa % 10 == 0) + { + p.mantissa /= 10; + ++p.exponent; + } + + return p; + }; + + auto const numberMantissa = num.mantissa(); + auto const numberExponent = num.exponent(); + + auto const canonicalParts = canonicalizeParts(parts, numberExponent); + + auto const canonicalNum = canonicalizeParts( + NumberParts{ + .mantissa = Number::externalToInternal(numberMantissa), + .exponent = numberExponent, + .negative = numberMantissa < 0, + }, + canonicalParts.exponent); + + if (canonicalParts.mantissa != canonicalNum.mantissa || + canonicalParts.exponent != canonicalNum.exponent || + canonicalParts.negative != canonicalNum.negative) + { + Throw("number cannot be represented"); + } + + return STNumber{field, num}; } } // namespace xrpl diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index b0b0924c6e..f6f85a0cca 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -53,6 +53,7 @@ #include #include +#include #include #include #include @@ -1437,18 +1438,77 @@ class LoanBroker_test : public beast::unit_test::Suite env(tx2, Ter(temINVALID)); } + env.setParseFailureExpected(true); + try { - auto const dm = power(2, 63) - 1; - BEAST_EXPECTS(dm > kMaxMpTokenAmount, to_string(dm)); - tx2[sfDebtMaximum] = dm; + tx2[sfDebtMaximum] = "9223372036854775808"; env(tx2, Ter(temINVALID)); + // should throw in parser + fail(); } - + catch (std::exception const& e) { - auto const dm = power(2, 63) - 3; - BEAST_EXPECTS(dm == kMaxMpTokenAmount, to_string(dm)); - tx2[sfDebtMaximum] = dm; - env(tx2, Ter(tesSUCCESS)); + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.DebtMaximum' has invalid data."); + } + env.setParseFailureExpected(false); + + if (Number::getMantissaScale() >= MantissaRange::MantissaScale::Large330) + { + // 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) + Number{1, -1}; + 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 + { + // 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)); + } } { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 4de8c9c616..617820c89c 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -61,6 +61,7 @@ #include #include +#include #include #include #include @@ -5246,11 +5247,15 @@ class Vault_test : public beast::unit_test::Suite auto const maxInt64 = std::to_string(std::numeric_limits::max()); BEAST_EXPECT(maxInt64 == "9223372036854775807"); - // Naming things is hard auto const maxInt64Plus1 = std::to_string( static_cast(std::numeric_limits::max()) + 1); BEAST_EXPECT(maxInt64Plus1 == "9223372036854775808"); + // Naming things is hard + auto const maxInt64Plus2 = std::to_string( + static_cast(std::numeric_limits::max()) + 2); + BEAST_EXPECT(maxInt64Plus2 == "9223372036854775809"); + auto const initialXRP = to_string(kInitialXrp); BEAST_EXPECT(initialXRP == "100000000000000000"); @@ -5277,25 +5282,58 @@ class Vault_test : public beast::unit_test::Suite env(tx); env.close(); - tx[sfAssetsMaximum] = maxInt64Plus1; - env(tx, Ter(tefEXCEPTION)); - env.close(); + // There are several parse failures expected in this function, so just disable it once. + env.setParseFailureExpected(true); + try + { + tx[sfAssetsMaximum] = maxInt64Plus1; + env(tx, Ter(tefEXCEPTION)); + env.close(); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } + + try + { + tx[sfAssetsMaximum] = maxInt64Plus2; + env(tx, Ter(tefEXCEPTION)); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } - // This value will be rounded - auto const insertAt = maxInt64Plus1.size() - 3; - auto const decimalTest = maxInt64Plus1.substr(0, insertAt) + "." + - maxInt64Plus1.substr(insertAt); // (max int64+1) / 1000 - BEAST_EXPECT(decimalTest == "9223372036854775.808"); - tx[sfAssetsMaximum] = decimalTest; auto const newKeylet = keylet::vault(owner.id(), env.seq(owner)); - env(tx); - env.close(); + try + { + auto const insertAt = maxInt64Plus2.size() - 3; + auto const decimalTest = maxInt64Plus2.substr(0, insertAt) + "." + + maxInt64Plus2.substr(insertAt); // (max int64+2) / 1000 + BEAST_EXPECT(decimalTest == "9223372036854775.809"); + tx[sfAssetsMaximum] = decimalTest; + env(tx); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } auto const vaultSle = env.le(newKeylet); - if (!BEAST_EXPECT(vaultSle)) - return; - - BEAST_EXPECT(vaultSle->at(sfAssetsMaximum) == 9223372036854776); + BEAST_EXPECT(!vaultSle); } { @@ -5329,25 +5367,41 @@ class Vault_test : public beast::unit_test::Suite env(tx); env.close(); - tx[sfAssetsMaximum] = maxInt64Plus1; - env(tx, Ter(tefEXCEPTION)); - env.close(); + try + { + tx[sfAssetsMaximum] = maxInt64Plus2; + env(tx, Ter(tefEXCEPTION)); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } - // This value will be rounded - auto const insertAt = maxInt64Plus1.size() - 1; - auto const decimalTest = maxInt64Plus1.substr(0, insertAt) + "." + - maxInt64Plus1.substr(insertAt); // (max int64+1) / 10 - BEAST_EXPECT(decimalTest == "922337203685477580.8"); - tx[sfAssetsMaximum] = decimalTest; auto const newKeylet = keylet::vault(owner.id(), env.seq(owner)); - env(tx); - env.close(); + try + { + auto const insertAt = maxInt64Plus2.size() - 1; + auto const decimalTest = maxInt64Plus2.substr(0, insertAt) + "." + + maxInt64Plus2.substr(insertAt); // (max int64+2) / 10 + BEAST_EXPECT(decimalTest == "922337203685477580.9"); + tx[sfAssetsMaximum] = decimalTest; + env(tx); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } auto const vaultSle = env.le(newKeylet); - if (!BEAST_EXPECT(vaultSle)) - return; - - BEAST_EXPECT(vaultSle->at(sfAssetsMaximum) == 922337203685477581); + BEAST_EXPECT(!vaultSle); } { @@ -5374,9 +5428,22 @@ class Vault_test : public beast::unit_test::Suite env(tx); env.close(); - tx[sfAssetsMaximum] = maxInt64Plus1; - env(tx); - env.close(); + // Since several tests are expected to have parser failures, leave this flag set for the + // remainder of this function. + env.setParseFailureExpected(true); + try + { + tx[sfAssetsMaximum] = maxInt64Plus2; + env(tx); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } tx[sfAssetsMaximum] = "1000000000000000e80"; env.close(); @@ -5386,22 +5453,27 @@ class Vault_test : public beast::unit_test::Suite // These values will be rounded to 15 significant digits { - auto const insertAt = maxInt64Plus1.size() - 1; - auto const decimalTest = maxInt64Plus1.substr(0, insertAt) + "." + - maxInt64Plus1.substr(insertAt); // (max int64+1) / 10 - BEAST_EXPECT(decimalTest == "922337203685477580.8"); - tx[sfAssetsMaximum] = decimalTest; auto const newKeylet = keylet::vault(owner.id(), env.seq(owner)); - env(tx); - env.close(); + try + { + auto const insertAt = maxInt64Plus2.size() - 1; + auto const decimalTest = maxInt64Plus2.substr(0, insertAt) + "." + + maxInt64Plus2.substr(insertAt); // (max int64+2) / 10 + BEAST_EXPECT(decimalTest == "922337203685477580.9"); + tx[sfAssetsMaximum] = decimalTest; + env(tx); + // should throw in parser + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT( + std::string(e.what()) == + "invalidParamsField 'tx_json.AssetsMaximum' has invalid data."); + } auto const vaultSle = env.le(newKeylet); - if (!BEAST_EXPECT(vaultSle)) - return; - - BEAST_EXPECT( - (vaultSle->at(sfAssetsMaximum) == - Number{9223372036854776, 2, Number::Normalized{}})); + BEAST_EXPECT(!vaultSle); } { tx[sfAssetsMaximum] = "9223372036854775807e40"; // max int64 * 10^40 diff --git a/src/test/protocol/STNumber_test.cpp b/src/test/protocol/STNumber_test.cpp index 5c9c3fd83c..74792e0a70 100644 --- a/src/test/protocol/STNumber_test.cpp +++ b/src/test/protocol/STNumber_test.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -101,6 +102,39 @@ struct STNumber_test : public beast::unit_test::Suite BEAST_EXPECT(numberFromJson(sfNumber, "-0.000e6") == STNumber(sfNumber, 0)); { + auto const parseNumber = [](std::string const& boundary) { + return numberFromJson(sfNumber, boundary); + }; + auto const expectParseThrows = [this, &parseNumber](std::string const& boundary) { + try + { + parseNumber(boundary); + fail(); + } + catch (std::exception const& e) + { + BEAST_EXPECT(std::string(e.what()) == "number cannot be represented"); + } + }; + + // Small rejects this; large scales parse it as 9223372036854775800e-1. + auto constexpr positiveBoundary = "922337203685477580"; + auto constexpr negativeBoundary = "-922337203685477580"; + if (Number::getMantissaScale() == MantissaRange::MantissaScale::Small) + { + expectParseThrows(positiveBoundary); + expectParseThrows(negativeBoundary); + } + else + { + BEAST_EXPECT( + parseNumber(positiveBoundary) == + STNumber(sfNumber, Number{922'337'203'685'477'580, 0})); + BEAST_EXPECT( + parseNumber(negativeBoundary) == + STNumber(sfNumber, Number{-922'337'203'685'477'580, 0})); + } + NumberRoundModeGuard const mg(Number::RoundingMode::TowardsZero); // maxint64 9,223,372,036,854,775,807 auto const maxInt = std::to_string(std::numeric_limits::max()); @@ -108,23 +142,19 @@ struct STNumber_test : public beast::unit_test::Suite auto const minInt = std::to_string(std::numeric_limits::min()); if (Number::getMantissaScale() == MantissaRange::MantissaScale::Small) { - BEAST_EXPECT( - numberFromJson(sfNumber, maxInt) == - STNumber(sfNumber, Number{9'223'372'036'854'775, 3})); - BEAST_EXPECT( - numberFromJson(sfNumber, minInt) == - STNumber(sfNumber, Number{-9'223'372'036'854'775, 3})); + // min/maxInt can't be exactly represented with the small mantissa, so they + // don't parse, and are expected to throw. + expectParseThrows(maxInt); + expectParseThrows(minInt); } else { + // with large mantissas, maxint is fine BEAST_EXPECT( - numberFromJson(sfNumber, maxInt) == + parseNumber(maxInt) == STNumber(sfNumber, Number{9'223'372'036'854'775'807, 0})); - BEAST_EXPECT( - numberFromJson(sfNumber, minInt) == - STNumber( - sfNumber, - Number{true, 9'223'372'036'854'775'808ULL, 0, Number::Normalized{}})); + // but minint's mantissa is > kMaxRep, and so rounds, and thus can't be parsed + expectParseThrows(minInt); } } diff --git a/src/tests/libxrpl/basics/Number.cpp b/src/tests/libxrpl/basics/Number.cpp index 70d6be2da5..36e1b4a700 100644 --- a/src/tests/libxrpl/basics/Number.cpp +++ b/src/tests/libxrpl/basics/Number.cpp @@ -182,6 +182,35 @@ TEST(NumberTest, limits) caught = true; } EXPECT_TRUE(caught); + + if (scale == MantissaRange::MantissaScale::Large330) + { + // Normalization with the other scales, including the older large mantissa scales, will + // overflow. + Number const bigNum{Number::kMaxRepUp, Number::kMaxExponent, Number::Normalized{}}; + // The display of large exponents won't go above kMaxExponent + EXPECT_EQ(to_string(bigNum), "9223372036854775810e32768") << bigNum; + // Perhaps surprisingly, this is ok, because the exponent range is related to when the + // number is _normalized_, and for mantissas > kMaxRep, the accessors return values that + // are not normalized. + EXPECT_EQ(bigNum.mantissa(), 922337203685477581ULL) << bigNum.mantissa(); + EXPECT_EQ(bigNum.exponent(), 32769) << bigNum.exponent(); + } + else + { + try + { + Number{Number::kMaxRepUp, Number::kMaxExponent, Number::Normalized{}}; + ADD_FAILURE(); + } + catch (std::overflow_error const& e) + { + std::string const expected = + (scale == MantissaRange::MantissaScale::Small ? "Number::normalize 1" + : "Number::normalize 1.5"); + EXPECT_EQ(e.what(), expected) << e.what(); + } + } } } @@ -193,7 +222,11 @@ TEST(NumberTest, add) auto const scale = Number::getMantissaScale(); + EXPECT_EQ(Number::getround(), Number::RoundingMode::ToNearest) + << to_string(Number::getround()); + using Case = std::tuple; + // TODO: Move these to the blocks where they're used auto const cSmall = std::to_array({ {Number{1'000'000'000'000'000, -15}, Number{6'555'555'555'555'555, -29}, @@ -304,12 +337,15 @@ TEST(NumberTest, add) auto const cLargeLegacy = std::to_array({ {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep / 10, 1}, __LINE__}, }); - auto const cLargeCorrected = std::to_array({ + auto const cLarge320 = std::to_array({ {Number{Number::kMaxRep}, Number{6, -1}, Number{(Number::kMaxRep / 10) + 1, 1}, __LINE__}, }); + auto const cLargeCorrected = std::to_array({ + {Number{Number::kMaxRep}, Number{6, -1}, Number{Number::kMaxRep}, __LINE__}, + }); auto test = [](auto const& c) { for (auto const& [x, y, z, line] : c) { @@ -330,9 +366,28 @@ TEST(NumberTest, add) { test(cLargeLegacy); } + else if (scale == MantissaRange::MantissaScale::Large320) + { + test(cLarge320); + } else { test(cLargeCorrected); + + // This has to be created in this block, because normalization with the other + // scales, including the older large mantissa scales, will overflow. + Number const bigResult{ + Number::kMaxRepUp, Number::kMaxExponent, Number::Normalized{}}; + auto const cBigNums = std::to_array({ + { + // Add 3 to the mantissa to avoid rounding + Number::max(), + Number{3, Number::kMaxExponent}, + bigResult, + __LINE__, + }, + }); + test(cBigNums); } } { @@ -381,7 +436,7 @@ TEST(NumberTest, sub) Number{1'000'000'000'000'000, -15}, Number{1'000'000'000'000'000, -30}, __LINE__}}); - auto const cLarge = std::to_array( + auto const cLargeAll = std::to_array( // Note that items with extremely large mantissas need to be // calculated, because otherwise they overflow uint64. Items from C // with larger mantissa @@ -428,16 +483,55 @@ TEST(NumberTest, sub) 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}, - __LINE__}, - {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, - Number{3, 0}, - Number{Number::kMaxRep}, - __LINE__}, - {power(2, 63), Number{3, 0}, Number{Number::kMaxRep}, __LINE__}, }); + // Note that items with extremely large mantissas need to be + // calculated, because otherwise they overflow uint64. Items from C + // with larger mantissa + auto const cLarge = std::to_array({ + // Anything larger than kMaxRep rounds up + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{1, 0}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep}, + __LINE__}, + {Number{false, Number::kMaxRep + 2, 0, Number::Normalized{}}, + Number{1, 0}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, + {Number{false, Number::kMaxRep + 2, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep}, + __LINE__}, + {power(2, 63), Number{3, 0}, Number{Number::kMaxRep}, __LINE__}, + }); + auto const cLarge330 = std::to_array({ + // kMaxRep + 1 is below the half-way point, so it rounds down to kMaxRep when the Number + // is created. + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{1, 0}, + Number{Number::kMaxRep - 1}, + __LINE__}, + {Number{false, Number::kMaxRep + 1, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep - 3}, + __LINE__}, + // kMaxRepUp -1 is above the half-way point, so it rounds up to kMaxRepUp when the + // Number is created. Subtracting 1 from that rounds up again. A little non-intuitive. + {Number{false, Number::kMaxRepUp - 1, 0, Number::Normalized{}}, + Number{1, 0}, + Number{(Number::kMaxRep / 10) + 1, 1}, + __LINE__}, + // Subtracting 3 gets back down to kMaxRep + {Number{false, Number::kMaxRepUp - 1, 0, Number::Normalized{}}, + Number{3, 0}, + Number{Number::kMaxRep}, + __LINE__}, + // 2^63 is the same as kMaxRep+1 + {power(2, 63), Number{3, 0}, Number{Number::kMaxRep - 3}, __LINE__}, + }); auto test = [](auto const& c) { for (auto const& [x, y, z, line] : c) { @@ -447,13 +541,23 @@ TEST(NumberTest, sub) EXPECT_EQ(result, z) << ss.str() << " Line: " << line; } }; - if (scale == MantissaRange::MantissaScale::Small) + switch (scale) { - test(cSmall); - } - else - { - test(cLarge); + case MantissaRange::MantissaScale::Small: + test(cSmall); + break; + case MantissaRange::MantissaScale::LargeLegacy: + case MantissaRange::MantissaScale::Large320: + test(cLargeAll); + test(cLarge); + break; + case MantissaRange::MantissaScale::Large330: + test(cLargeAll); + test(cLarge330); + break; + default: + ADD_FAILURE(); + break; } } } @@ -1370,38 +1474,39 @@ TEST(NumberTest, to_string) auto const scale = Number::getMantissaScale(); - auto test = [](Number const& n, std::string const& expected) { + auto test = [](Number const& n, std::string const& expected, int line) { auto const result = to_string(n); std::stringstream ss; ss << "to_string(" << result << "). Expected: " << expected; - EXPECT_EQ(result, expected) << ss.str(); + EXPECT_EQ(result, expected) << ss.str() << " Line: " << line; }; - test(Number(-2, 0), "-2"); - test(Number(0, 0), "0"); - test(Number(2, 0), "2"); - test(Number(25, -3), "0.025"); - test(Number(-25, -3), "-0.025"); - test(Number(25, 1), "250"); - test(Number(-25, 1), "-250"); - test(Number(2, 20), "2e20"); - test(Number(-2, -20), "-2e-20"); + test(Number(-2, 0), "-2", __LINE__); + test(Number(0, 0), "0", __LINE__); + test(Number(2, 0), "2", __LINE__); + test(Number(25, -3), "0.025", __LINE__); + test(Number(-25, -3), "-0.025", __LINE__); + test(Number(25, 1), "250", __LINE__); + test(Number(-25, 1), "-250", __LINE__); + test(Number(2, 20), "2e20", __LINE__); + test(Number(-2, -20), "-2e-20", __LINE__); // Test the edges // ((exponent < -(25)) || (exponent > -(5))))) // or ((exponent < -(28)) || (exponent > -(8))))) - test(Number(2, -10), "0.0000000002"); - test(Number(2, -11), "2e-11"); + test(Number(2, -10), "0.0000000002", __LINE__); + test(Number(2, -11), "2e-11", __LINE__); - test(Number(-2, 10), "-20000000000"); - test(Number(-2, 11), "-2e11"); + test(Number(-2, 10), "-20000000000", __LINE__); + test(Number(-2, 11), "-2e11", __LINE__); + test(Number(-2, 11) - 1, "-200000000001", __LINE__); switch (scale) { case MantissaRange::MantissaScale::Small: - test(Number::min(), "1e-32753"); - test(Number::max(), "9999999999999999e32768"); - test(Number::lowest(), "-9999999999999999e32768"); + test(Number::min(), "1e-32753", __LINE__); + test(Number::max(), "9999999999999999e32768", __LINE__); + test(Number::lowest(), "-9999999999999999e32768", __LINE__); { NumberRoundModeGuard const mg(Number::RoundingMode::TowardsZero); @@ -1409,61 +1514,131 @@ TEST(NumberTest, to_string) EXPECT_EQ(maxMantissa, (9'999'999'999'999'999)); test( Number{false, (maxMantissa * 1000) + 999, -3, Number::Normalized()}, - "9999999999999999"); + "9999999999999999", + __LINE__); test( Number{true, (maxMantissa * 1000) + 999, -3, Number::Normalized()}, - "-9999999999999999"); + "-9999999999999999", + __LINE__); - test(Number{std::numeric_limits::max(), -3}, "9223372036854775"); + test( + Number{std::numeric_limits::max(), -3}, + "9223372036854775", + __LINE__); test( -(Number{std::numeric_limits::max(), -3}), - "-9223372036854775"); + "-9223372036854775", + __LINE__); test( - Number{std::numeric_limits::min(), 0}, "-9223372036854775e3"); + Number{std::numeric_limits::min(), 0}, + "-9223372036854775e3", + __LINE__); test( -(Number{std::numeric_limits::min(), 0}), - "9223372036854775e3"); + "9223372036854775e3", + __LINE__); } break; default: // Test the edges // ((exponent < -(28)) || (exponent > -(8))))) - test(Number::min(), "1e-32750"); - test(Number::max(), "9223372036854775807e32768"); - test(Number::lowest(), "-9223372036854775807e32768"); + test(Number::min(), "1e-32750", __LINE__); + test(Number::max(), "9223372036854775807e32768", __LINE__); + test(Number::lowest(), "-9223372036854775807e32768", __LINE__); { NumberRoundModeGuard const mg(Number::RoundingMode::TowardsZero); auto const maxMantissa = Number::maxMantissa(); EXPECT_EQ((maxMantissa), (9'999'999'999'999'999'999ULL)); test( - Number{false, maxMantissa, 0, Number::Normalized{}}, "9999999999999999990"); + Number{false, maxMantissa, 0, Number::Normalized{}}, + "9999999999999999990", + __LINE__); test( - Number{true, maxMantissa, 0, Number::Normalized{}}, "-9999999999999999990"); + Number{true, maxMantissa, 0, Number::Normalized{}}, + "-9999999999999999990", + __LINE__); test( - Number{std::numeric_limits::max(), 0}, "9223372036854775807"); + Number{std::numeric_limits::max(), 0}, + "9223372036854775807", + __LINE__); test( -(Number{std::numeric_limits::max(), 0}), - "-9223372036854775807"); + "-9223372036854775807", + __LINE__); - // Because the absolute value of min is larger than max, it - // will be scaled down to fit under max. Since we're - // rounding towards zero, the 8 at the end is dropped. - test( - Number{std::numeric_limits::min(), 0}, - "-9223372036854775800"); - test( - -(Number{std::numeric_limits::min(), 0}), - "9223372036854775800"); + switch (scale) + { + case MantissaRange::MantissaScale::Large330: + // Because the absolute value of min() is larger than max(), it + // will be rounded down toward max() + test( + Number{std::numeric_limits::min(), 0}, + "-9223372036854775807", + __LINE__); + test( + -(Number{std::numeric_limits::min(), 0}), + "9223372036854775807", + __LINE__); + break; + default: + // Because the absolute value of min() is larger than max(), it + // will be scaled down to fit under max(). Since we're + // rounding towards zero, the 8 at the end is dropped. + test( + Number{std::numeric_limits::min(), 0}, + "-9223372036854775800", + __LINE__); + test( + -(Number{std::numeric_limits::min(), 0}), + "9223372036854775800", + __LINE__); + break; + } } + switch (scale) + { + case MantissaRange::MantissaScale::Large330: + // Rounding to nearest, since the mantissa is below the halfway point from + // kMaxRep to kMaxRepUp, it will be rounded down to kMaxRep + test( + Number{std::numeric_limits::max(), 0} + 1, + "9223372036854775807", + __LINE__); + test( + -(Number{std::numeric_limits::max(), 0} + 1), + "-9223372036854775807", + __LINE__); + break; + default: + // Rounding to nearest, since the mantissa is bigger than kMaxRep, the 8 + // will be dropped, and since that is bigger than 5, the result will be + // rounded up from 0 to 1. + test( + Number{std::numeric_limits::max(), 0} + 1, + "9223372036854775810", + __LINE__); + test( + -(Number{std::numeric_limits::max(), 0} + 1), + "-9223372036854775810", + __LINE__); + break; + } + // Rounding to nearest, will be rounded up to kMaxRepUp, but for different reasons + // depending on the scale. If older than "Large", it rounds up for the same reason + // "+1" rounds up. For "Large", since the mantissa is above the halfway point from + // kMaxRep to kMaxRepUp, it will be rounded up to kMaxRepUp. test( - Number{std::numeric_limits::max(), 0} + 1, "9223372036854775810"); + Number{std::numeric_limits::max(), 0} + 2, + "9223372036854775810", + __LINE__); test( - -(Number{std::numeric_limits::max(), 0} + 1), - "-9223372036854775810"); + -(Number{std::numeric_limits::max(), 0} + 2), + "-9223372036854775810", + __LINE__); break; } } @@ -1851,15 +2026,14 @@ TEST(NumberTest, upward_rounding_produces_value_not_below_exact_at_k_max_rep_cus auto const message = [&] { std::ostringstream os; - os << "\n" - << " a = " << fmt(BigInt(kAValue)) << "\n" + os << " a = " << fmt(BigInt(kAValue)) << "\n" << " b = " << fmt(BigInt(kBValue)) << "\n" << " exact a*b = " << fmt(exactProduct) << "\n" << " stored = " << fmt(storedValue) << "\n" << " stored - exact = " << fmt(signedDifference) << "\n" << " upward = " << (signedDifference >= 0 ? "held" : "VIOLATED") << "\n" << " stored.mantissa = " << product.mantissa() << "\n" - << " stored.exponent = " << product.exponent() << "\n"; + << " stored.exponent = " << product.exponent() << "\n\n"; return os.str(); }; @@ -1939,15 +2113,14 @@ TEST(NumberTest, upward_division_returns_value_not_below_exact_on_large_scale) auto const message = [&] { std::ostringstream os; - os << "\n" - << " a = " << kAValue << "\n" + os << " a = " << kAValue << "\n" << " b = " << kBValue << "\n" << " exact a/b = " << fmt(exact) << "\n" << " stored a/b = " << fmt(stored) << "\n" << " stored - exact = " << fmt(diff) << " (negative => Upward gave value BELOW truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" - << " quotient.exponent = " << quotient.exponent() << "\n"; + << " quotient.exponent = " << quotient.exponent() << "\n\n"; return os.str(); }; @@ -1997,15 +2170,14 @@ TEST(NumberTest, downward_division_returns_value_not_above_exact_on_large_scale) auto const message = [&] { std::ostringstream os; - os << "\n" - << " a = " << kAValue << "\n" + os << " a = " << kAValue << "\n" << " b = " << kBValue << "\n" << " exact a/b = " << fmt(exact) << "\n" << " stored a/b = " << fmt(stored) << "\n" << " stored - exact = " << fmt(diff) << " (positive => Downward gave value ABOVE truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" - << " quotient.exponent = " << quotient.exponent() << "\n"; + << " quotient.exponent = " << quotient.exponent() << "\n\n"; return os.str(); }; @@ -2065,15 +2237,14 @@ TEST(NumberTest, to_nearest_division_uses_dropped_digits_on_large_scale) auto const message = [&] { std::ostringstream os; - os << "\n" - << " a = " << kAValue << "\n" + os << " a = " << kAValue << "\n" << " b = " << kBValue << "\n" << " exact a/b = " << fmt(exact) << "\n" << " stored a/b = " << fmt(stored) << "\n" << " stored - exact = " << fmt(diff) << " (negative => ToNearest gave value BELOW truth)\n" << " quotient.mantissa = " << quotient.mantissa() << "\n" - << " quotient.exponent = " << quotient.exponent() << "\n"; + << " quotient.exponent = " << quotient.exponent() << "\n\n"; return os.str(); }; @@ -2169,14 +2340,13 @@ TEST(NumberTest, subtraction_rounding) auto const message = [&](auto const& r, auto const& sum) { std::ostringstream os; - os << "\n a = " << a << " (" << fmt(bigA) - << ")\n b = " << b << " (" << fmt(bigB) - << ")\n exact a + b = " << fmt(exact) << "\n"; + os << " a = " << a << " (" << fmt(bigA) << ")\n b = " << b + << " (" << fmt(bigB) << ")\n exact a + b = " << fmt(exact) << "\n"; auto const diff = sum.first - exact; auto const rLabel = to_string(r); os << std::string(15 - rLabel.length(), ' ') << rLabel << " = " << fmt(sum.first) - << "\n difference = " << fmt(diff) << "\n"; + << "\n difference = " << fmt(diff) << "\n\n"; return os.str(); }; @@ -2228,6 +2398,93 @@ TEST(NumberTest, subtraction_rounding) } } +TEST(NumberTest, normalization_cusp_tonearest_and_downward) +{ + for (auto const mantissaScale : MantissaRange::getAllScales()) + { + NumberMantissaScaleGuard const mg{mantissaScale}; + NumberRoundModeGuard const rg{Number::RoundingMode::ToNearest}; + + auto const scale = Number::getMantissaScale(); + + constexpr auto kMaxRep = Number::kMaxRep; + + // Both ToNearest and Downward should round to `below` + auto constexpr actual = static_cast(kMaxRep) + 1; + Number const below{static_cast(kMaxRep), 0}; + Number const above{false, static_cast(kMaxRep) + 3, 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 const toNearest = construct(Number::RoundingMode::ToNearest); + + Number const downward = construct(Number::RoundingMode::Downward); + + auto message = [&] { + std::ostringstream log; + log << " 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"; + return log.str(); + }; + + switch (scale) + { + case MantissaRange::MantissaScale::Small: + // With the small mantissa, everything but Downward rounds UP, including the + // reference values, "above" and "below" + + EXPECT_EQ(below, above) << message(); + EXPECT_EQ(upward, above) << message(); + EXPECT_EQ(toNearest, above) << message(); + + EXPECT_LT(downward, below) << message(); + + break; + + case MantissaRange::MantissaScale::LargeLegacy: + case MantissaRange::MantissaScale::Large320: + // Upward round UP + EXPECT_EQ(upward, above) << message(); + + // ToNearest rounds UP when the DOWN neighbor is strictly closer + EXPECT_EQ(toNearest, above) << message(); + EXPECT_GT(toNearest, below) << message(); + + // Downward undershoots: it returns a value below `below` + EXPECT_LT(downward, below) << message(); + + // Both should have given the same answer, but they differ + EXPECT_GT(toNearest, downward) << message(); + + break; + default: + // Covers "Large" and any newly added scales + + // Upward round UP + EXPECT_EQ(upward, above) << message(); + + // ToNearest rounds to the strictly closer DOWN neighbor + EXPECT_NE(toNearest, above) << message(); + EXPECT_EQ(toNearest, below) << message(); + + // Downward also rounds to `below` + EXPECT_EQ(downward, below) << message(); + + // ToNearest rounds to downward + EXPECT_EQ(toNearest, downward) << message(); + break; + } + } +} + TEST(NumberTest, number_add_directed_sign_wrong) { for (auto const mantissaScale : MantissaRange::getAllScales()) @@ -2460,4 +2717,180 @@ TEST(NumberTest, number_add_to_nearest_picks_farther) } } +TEST(NumberTest, number_cusp_rounding_with_fractional_parts) +{ + for (auto const mantissaScale : MantissaRange::getAllScales()) + { + NumberMantissaScaleGuard const mg{mantissaScale}; + + auto const scale = Number::getMantissaScale(); + + Number const below{static_cast(Number::kMaxRep), 0}; + Number const above{false, Number::kMaxRepUp, 0, Number::Normalized{}}; + + auto header = [&] { + std::ostringstream log; + log << "Scale: " << to_string(mantissaScale) << ", Below: " << below + << ", Above: " << above << "\n"; + return log.str(); + }; + + auto const zeroPointFour = Number(4, -1); + auto const zeroPointFive = Number(5, -1); + auto const zeroPointSix = Number(6, -1); + auto const onePointFour = Number(14, -1); + auto const onePointFive = Number(15, -1); + auto const onePointSix = Number(16, -1); + auto const twoPointFour = Number(24, -1); + auto const twoPointFive = Number(25, -1); + auto const twoPointSix = Number(26, -1); + + auto const operands = std::to_array({ + zeroPointFour, + zeroPointFive, + zeroPointSix, + onePointFour, + onePointFive, + onePointSix, + twoPointFour, + twoPointFive, + twoPointSix, + }); + + auto const modes = std::to_array({ + Number::RoundingMode::ToNearest, + Number::RoundingMode::TowardsZero, + Number::RoundingMode::Downward, + Number::RoundingMode::Upward, + }); + + // Addition cases test kMaxRep + Operand + for (auto const& mode : modes) + { + for (auto const& operand : operands) + { + NumberRoundModeGuard const rg{mode}; + + auto const expectedValue = [&]() { + // Returns "above" by default. The checks here are for exceptions. + if (scale >= MantissaRange::MantissaScale::Large330) + { + if (mode == Number::RoundingMode::ToNearest && operand < onePointFive) + return below; + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + return below; + } + if (scale == MantissaRange::MantissaScale::Large320) + { + if (mode == Number::RoundingMode::ToNearest) + { + if (operand < zeroPointFive) + return below; + } + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + { + if (operand >= onePointFour) + return below - 7; + return below; + } + } + if (scale == MantissaRange::MantissaScale::LargeLegacy) + { + if (mode == Number::RoundingMode::ToNearest) + { + if (operand < zeroPointFive) + return below; + if (operand <= zeroPointSix) + return below - 7; + } + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + { + if (operand >= onePointFour) + return below - 7; + return below; + } + if (mode == Number::RoundingMode::Upward && operand <= zeroPointSix) + return below - 7; + } + if (scale == MantissaRange::MantissaScale::Small && + mode == Number::RoundingMode::Upward) + return above + 1000; + return above; + }(); + + Number const actual = below + operand; + + auto message = [&] { + std::stringstream ss; + ss << header() << "kMaxRep + " << operand << " rounded " << to_string(mode) + << " to " << actual << ". Expected: " << expectedValue; + return ss.str(); + }; + EXPECT_EQ(actual, expectedValue) << message(); + } + } + + // Subtraction cases test kMaxRepUp - Operand + for (auto const& mode : modes) + { + for (auto const& operand : operands) + { + NumberRoundModeGuard const rg{mode}; + + auto const expectedValue = [&]() { + if (scale >= MantissaRange::MantissaScale::Large330) + { + if (mode == Number::RoundingMode::ToNearest && operand > onePointFive) + return below; + if (mode == Number::RoundingMode::TowardsZero || + mode == Number::RoundingMode::Downward) + return below; + } + if (scale == MantissaRange::MantissaScale::LargeLegacy || + scale == MantissaRange::MantissaScale::Large320) + { + if (mode == Number::RoundingMode::ToNearest) + { + if (operand >= twoPointSix) + return below; + } + if (mode == Number::RoundingMode::TowardsZero) + { + if (operand >= onePointFour) + return below - 7; + } + if (mode == Number::RoundingMode::Downward) + { + if (operand <= onePointSix) + return below - 7; + return below; + } + } + if (scale == MantissaRange::MantissaScale::Small) + { + if (mode == Number::RoundingMode::Downward) + return below - 1000; + if (mode == Number::RoundingMode::Upward) + return below; + } + return above; + }(); + + Number const actual = above - operand; + + auto message = [&] { + std::stringstream ss; + ss << header() << "kMaxRepUp - " << operand << " rounded " << to_string(mode) + << " to " << actual << ". Expected: " << expectedValue; + return ss.str(); + }; + EXPECT_EQ(actual, expectedValue) << message(); + } + } + } +} + } // namespace xrpl From 530e09dbe8982ade14b8d469ab6323d7a65f0594 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 14 Jul 2026 14:48:10 -0400 Subject: [PATCH 09/12] fix: Update base_uint and test changes released in 3.1.3 (#7570) Co-authored-by: Sergey Kuznetsov Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Ayaz Salikhov --- .../scripts/levelization/results/ordering.txt | 1 + include/xrpl/basics/base_uint.h | 10 +- src/test/protocol/Issue_test.cpp | 100 +++++++++++++ src/test/protocol/STIssue_test.cpp | 140 ++++++++++++++++++ src/test/rpc/AccountObjects_test.cpp | 83 +++++++++++ src/test/rpc/Transaction_test.cpp | 122 +++++++++++++++ src/tests/libxrpl/basics/base_uint_test.cpp | 80 ++++++++++ 7 files changed, 534 insertions(+), 2 deletions(-) diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index aee6f4c579..b31e6ae961 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -159,6 +159,7 @@ test.peerfinder > xrpl.protocol test.protocol > test.jtx test.protocol > test.unit_test test.protocol > xrpl.basics +test.protocol > xrpld.core test.protocol > xrpl.json test.protocol > xrpl.protocol test.rpc > test.jtx diff --git a/include/xrpl/basics/base_uint.h b/include/xrpl/basics/base_uint.h index 96cfa343e3..bee8b8b945 100644 --- a/include/xrpl/basics/base_uint.h +++ b/include/xrpl/basics/base_uint.h @@ -308,7 +308,9 @@ public: XRPL_ASSERT( c.size() * sizeof(typename Container::value_type) == size(), "xrpl::BaseUInt::fromRaw(Container auto) : input size match"); - std::memcpy(result.data_.data(), c.data(), size()); + std::size_t const canCopy = + std::min(size(), c.size() * sizeof(typename Container::value_type)); + std::memcpy(result.data_.data(), c.data(), canCopy); return result; } @@ -322,7 +324,11 @@ public: XRPL_ASSERT( c.size() * sizeof(typename Container::value_type) == size(), "xrpl::BaseUInt::operator=(Container auto) : input size match"); - std::memcpy(data_.data(), c.data(), size()); + std::size_t const canCopy = + std::min(size(), c.size() * sizeof(typename Container::value_type)); + if (canCopy < size()) + *this = beast::kZero; + std::memcpy(data_.data(), c.data(), canCopy); return *this; } diff --git a/src/test/protocol/Issue_test.cpp b/src/test/protocol/Issue_test.cpp index 44a36b7dcf..a6a1fdd341 100644 --- a/src/test/protocol/Issue_test.cpp +++ b/src/test/protocol/Issue_test.cpp @@ -1,10 +1,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include @@ -863,6 +865,101 @@ public: //-------------------------------------------------------------------------- + void + testIssueFromJson() + { + testcase("issueFromJson"); + + // Valid XRP — no issuer field + { + json::Value jv; + jv[jss::currency] = "XRP"; + auto const issue = issueFromJson(jv); + BEAST_EXPECT(isXRP(issue)); + } + + // Valid IOU — legitimate issuer + { + json::Value jv; + jv[jss::currency] = "USD"; + jv[jss::issuer] = "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh"; + auto const issue = issueFromJson(jv); + BEAST_EXPECT(!isXRP(issue)); + BEAST_EXPECT(issue.account != noAccount()); + } + + // noAccount() is the MPT sentinel in binary serialization - must be + // rejected + try + { + json::Value jv; + jv[jss::currency] = "USD"; + jv[jss::issuer] = to_string(noAccount()); + issueFromJson(jv); + fail("noAccount() accepted as IOU issuer"); + } + catch (...) + { + pass(); + } + + // xrpAccount() is the XRP sentinel (all zeros) - must be rejected + // as IOU issuer + try + { + json::Value jv; + jv[jss::currency] = "USD"; + jv[jss::issuer] = to_string(xrpAccount()); + issueFromJson(jv); + fail("xrpAccount() accepted as IOU issuer"); + } + catch (...) + { + pass(); + } + + // Invalid base58 — must be rejected + try + { + json::Value jv; + jv[jss::currency] = "USD"; + jv[jss::issuer] = "not_a_valid_address"; + issueFromJson(jv); + fail("invalid base58 accepted as IOU issuer"); + } + catch (...) + { + pass(); + } + + // Non-XRP currency with no issuer field — must be rejected + try + { + json::Value jv; + jv[jss::currency] = "USD"; + issueFromJson(jv); + fail("missing issuer accepted"); + } + catch (...) + { + pass(); + } + + // XRP with an issuer field — must be rejected + try + { + json::Value jv; + jv[jss::currency] = "XRP"; + jv[jss::issuer] = "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh"; + issueFromJson(jv); + fail("XRP with issuer accepted"); + } + catch (...) + { + pass(); + } + } + void run() override { @@ -897,6 +994,9 @@ public: // --- testIssueDomainSets(); testIssueDomainMaps(); + + // --- + testIssueFromJson(); } }; diff --git a/src/test/protocol/STIssue_test.cpp b/src/test/protocol/STIssue_test.cpp index 1d6d750355..b7cc944e6b 100644 --- a/src/test/protocol/STIssue_test.cpp +++ b/src/test/protocol/STIssue_test.cpp @@ -1,15 +1,24 @@ #include +#include #include // IWYU pragma: keep +#include + +#include #include #include +#include +#include #include #include #include #include #include #include +#include + +#include namespace xrpl::test { @@ -137,12 +146,143 @@ public: "000000000000000000000000000000000000000000000002"); } + void + testNoAccountIssuerRpc() + { + testcase("noAccount issuer rejected via RPC sign"); + + using namespace jtx; + Env env{*this, envconfig([](std::unique_ptr cfg) { + cfg->loadFromString("[signing_support]\ntrue"); + return cfg; + })}; + + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env.close(); + + json::Value txJson; + txJson[jss::TransactionType] = "AMMDelete"; + txJson[jss::Account] = alice.human(); + txJson[jss::Asset][jss::currency] = "USD"; + txJson[jss::Asset][jss::issuer] = to_string(noAccount()); + txJson[jss::Asset2][jss::currency] = "XRP"; + + json::Value req; + req[jss::tx_json] = txJson; + req[jss::secret] = alice.name(); + + auto const result = env.rpc("json", "sign", to_string(req))[jss::result]; + + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::error_message] == "Field 'tx_json.Asset' has invalid data."); + } + + void + testNoAccountIssuer() + { + testcase("noAccount issuer rejection"); + + { + json::Value jv; + jv[jss::currency] = "USD"; + jv[jss::issuer] = to_string(noAccount()); + + try + { + issueFromJson(sfAsset, jv); + fail("issueFromJson accepted noAccount() as IOU issuer"); + } + catch (...) + { + pass(); + } + } + + { + Serializer s; + s.addBitString(toCurrency("USD")); + s.addBitString(noAccount()); + SerialIter iter(s.slice()); + + try + { + STIssue const stissue(iter, sfAsset); + fail( + "STIssue deserialization of [USD][noAccount()] should " + "throw"); + } + catch (...) + { + pass(); + } + } + } + + void + testXrpAccountIssuerRpc() + { + testcase("xrpAccount issuer rejected via RPC sign"); + + using namespace jtx; + Env env{*this, envconfig([](std::unique_ptr cfg) { + cfg->loadFromString("[signing_support]\ntrue"); + return cfg; + })}; + + Account const alice{"alice"}; + env.fund(XRP(10000), alice); + env.close(); + + json::Value txJson; + txJson[jss::TransactionType] = "AMMDelete"; + txJson[jss::Account] = alice.human(); + txJson[jss::Asset][jss::currency] = "USD"; + txJson[jss::Asset][jss::issuer] = to_string(xrpAccount()); + txJson[jss::Asset2][jss::currency] = "XRP"; + + json::Value req; + req[jss::tx_json] = txJson; + req[jss::secret] = alice.name(); + + auto const result = env.rpc("json", "sign", to_string(req))[jss::result]; + + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::error_message] == "Field 'tx_json.Asset' has invalid data."); + } + + void + testXrpAccountIssuer() + { + testcase("xrpAccount issuer rejection"); + + { + json::Value jv; + jv[jss::currency] = "USD"; + jv[jss::issuer] = to_string(xrpAccount()); + + try + { + issueFromJson(sfAsset, jv); + fail("issueFromJson accepted xrpAccount() as IOU issuer"); + } + catch (...) + { + pass(); + } + } + } + void run() override { // compliments other unit tests to ensure complete coverage testConstructor(); testCompare(); + testNoAccountIssuerRpc(); + testNoAccountIssuer(); + testXrpAccountIssuerRpc(); + testXrpAccountIssuer(); } }; diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index 6ba6e7de9f..c656c97a4c 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -36,6 +37,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -1616,6 +1618,86 @@ public: } } + void + testAccountObjectDoesntShowCancelledOffers() + { + testcase("AccountObjectDoesntShowCancelledOffers"); + + using namespace jtx; + Env env(*this); + + Account const alice{"alice"}; + Account const bob{"bob"}; + auto const eur = bob["EUR"]; + env.fund(XRP(10000), alice, bob); + env.close(); + + auto const rpcAccountObjects = [&](std::optional limit = std::nullopt) { + json::Value params; + params[jss::account] = alice.human(); + if (limit.has_value()) + { + params[jss::limit] = *limit; + } + return env.rpc("json", "account_objects", to_string(params)); + }; + + auto const numEntries = 33; + std::vector seqs; + seqs.reserve(numEntries); + for ([[maybe_unused]] auto _ : std::ranges::iota_view{0, numEntries}) + { + json::Value params; + params[jss::secret] = toBase58(generateSeed("alice")); + params[jss::tx_json] = offer(alice, eur(1), XRP(2)); + auto const res = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(res[jss::engine_result].asString() == "tesSUCCESS"); + seqs.push_back(env.seq(alice)); + } + + auto res = rpcAccountObjects(); + BEAST_EXPECT(res[jss::result][jss::account_objects].size() == numEntries); + BEAST_EXPECT(not res[jss::result].isMember(jss::limit)); + BEAST_EXPECT(not res[jss::result].isMember(jss::marker)); + + for (auto const s : std::views::all(seqs) | std::views::take(numEntries - 1)) + { + json::Value params; + params[jss::secret] = toBase58(generateSeed("alice")); + params[jss::tx_json] = offerCancel(alice, s - 1); + auto const res = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(res[jss::engine_result].asString() == "tesSUCCESS"); + } + + res = rpcAccountObjects(); + BEAST_EXPECT(res[jss::result][jss::account_objects].size() == 1); + BEAST_EXPECT(not res[jss::result].isMember(jss::limit)); + BEAST_EXPECT(not res[jss::result].isMember(jss::marker)); + + { + json::Value params; + params[jss::secret] = toBase58(generateSeed("alice")); + json::Value txJson; + txJson[jss::TransactionType] = jss::NFTokenMint; + txJson[jss::Account] = to_string(alice.id()); + txJson["NFTokenTaxon"] = 1; + params[jss::tx_json] = txJson; + auto const res = env.rpc("json", "submit", to_string(params))[jss::result]; + BEAST_EXPECT(res[jss::engine_result].asString() == "tesSUCCESS"); + } + env.close(); + + res = rpcAccountObjects(); + BEAST_EXPECT(res[jss::result][jss::account_objects].size() == 2); + BEAST_EXPECT(not res[jss::result].isMember(jss::limit)); + BEAST_EXPECT(not res[jss::result].isMember(jss::marker)); + + res = rpcAccountObjects(1); + BEAST_EXPECT(res[jss::result][jss::account_objects].size() == 1); + BEAST_EXPECT(res[jss::result][jss::limit].asUInt() == 1); + BEAST_EXPECT(res[jss::result].isMember(jss::marker)); + } + void run() override { @@ -1627,6 +1709,7 @@ public: testAccountNFTs(); testAccountObjectMarker(); testSponsoredFilter(); + testAccountObjectDoesntShowCancelledOffers(); } }; diff --git a/src/test/rpc/Transaction_test.cpp b/src/test/rpc/Transaction_test.cpp index c3bf707ec4..4dae475b63 100644 --- a/src/test/rpc/Transaction_test.cpp +++ b/src/test/rpc/Transaction_test.cpp @@ -2,6 +2,7 @@ #include #include #include +#include #include #include #include @@ -880,6 +881,125 @@ class Transaction_test : public beast::unit_test::Suite } } + void + testSignForNetworkIDValidation() + { + testcase("SignFor NetworkID validation"); + using namespace test::jtx; + + Account const owner{"owner"}; + Account const signer{"signer"}; + + auto makeConfig = [](std::uint32_t networkID) { + return envconfig([networkID](std::unique_ptr cfg) { + cfg->networkId = networkID; + return cfg; + }); + }; + + auto setupEnv = [&](Env& env) { + env.fund(XRP(10'000), owner, signer); + env.close(); + env(signers(owner, 1, {{signer, 1}})); + env.close(); + }; + + auto makeTx = [&](Env& env) { + json::Value tx; + tx[jss::TransactionType] = jss::AccountSet; + tx[jss::Account] = owner.human(); + tx[jss::Sequence] = env.seq(owner); + tx[jss::Fee] = "100"; + tx[jss::SigningPubKey] = ""; + return tx; + }; + + auto signFor = [&](Env& env, json::Value const& tx) { + json::Value signReq; + signReq[jss::tx_json] = tx; + signReq[jss::account] = signer.human(); + signReq[jss::secret] = signer.name(); + return env.rpc("json", "sign_for", to_string(signReq))[jss::result]; + }; + + // Test case: NetworkID < 1024 - field is not required + { + Env env{*this, makeConfig(500)}; + setupEnv(env); + + auto tx = makeTx(env); + auto result = signFor(env, tx); + + BEAST_EXPECT(result[jss::status] == "success"); + BEAST_EXPECT(!result[jss::tx_json].isMember(jss::NetworkID)); + } + + // Test case: NetworkID > 1024 - missing NetworkID field + { + Env env{*this, makeConfig(2040)}; + setupEnv(env); + + auto tx = makeTx(env); + auto result = signFor(env, tx); + + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::error_message] == "Missing field 'tx_json.NetworkID'."); + } + + // Test case: NetworkID > 1024 - NetworkID field is not a number + { + Env env{*this, makeConfig(2040)}; + setupEnv(env); + + auto tx = makeTx(env); + tx[jss::NetworkID] = "not_a_number"; + auto result = signFor(env, tx); + + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::error_message] == "Invalid field 'tx_json.NetworkID'."); + } + + // Test case: NetworkID > 1024 - NetworkID field is not integral + { + Env env{*this, makeConfig(2040)}; + setupEnv(env); + + auto tx = makeTx(env); + tx[jss::NetworkID] = 2040.1; + auto result = signFor(env, tx); + + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::error_message] == "Invalid field 'tx_json.NetworkID'."); + } + + // Test case: NetworkID > 1024 - NetworkID field is different from + // actual NetworkID + { + Env env{*this, makeConfig(2040)}; + setupEnv(env); + + auto tx = makeTx(env); + tx[jss::NetworkID] = 9999; + auto result = signFor(env, tx); + + BEAST_EXPECT(result[jss::error] == "invalidParams"); + BEAST_EXPECT(result[jss::error_message] == "Invalid field 'tx_json.NetworkID'."); + } + + // Test case: NetworkID > 1024 - NetworkID field is correct + { + Env env{*this, makeConfig(2040)}; + setupEnv(env); + + auto tx = makeTx(env); + tx[jss::NetworkID] = 2040; + auto result = signFor(env, tx); + + BEAST_EXPECT(result[jss::status] == "success"); + BEAST_EXPECT(result[jss::tx_json][jss::NetworkID].asUInt() == 2040); + } + } + public: void run() override @@ -889,6 +1009,8 @@ public: FeatureBitset const all{testableAmendments()}; testWithFeats(all); + + testSignForNetworkIDValidation(); } void diff --git a/src/tests/libxrpl/basics/base_uint_test.cpp b/src/tests/libxrpl/basics/base_uint_test.cpp index eefbff158d..c9bfc35c94 100644 --- a/src/tests/libxrpl/basics/base_uint_test.cpp +++ b/src/tests/libxrpl/basics/base_uint_test.cpp @@ -122,6 +122,73 @@ struct BaseUintTest : public ::testing::Test } }; +using BaseUintDeathTest = BaseUintTest; + +TEST_F(BaseUintDeathTest, fromRaw_size_mismatch) +{ + // ENABLE_VOIDSTAR is a debug build, but does not crash on failed asserts. Rather than twist + // these tests into knots to make them work, just skip them. +#ifdef ENABLE_VOIDSTAR + GTEST_SKIP() << "ENABLE_VOIDSTAR is a debug build, but does not crash on failed asserts."; +#else + auto smallConstruct = [] { + // Container smaller than the base_uint (8 bytes vs 12 bytes for + // test96). Only the first 8 bytes are copied; the remaining 4 bytes + // stay zero. + Blob const tooSmall{1, 2, 3, 4, 5, 6, 7, 8}; + BaseUInt96 const result = BaseUInt96::fromRaw(tooSmall); + auto const resultText = to_string(result); + EXPECT_EQ(resultText, "010203040506070800000000") << resultText; + }; + EXPECT_DEBUG_DEATH(smallConstruct(), "input size match"); + + auto largeConstruct = [] { + // Container larger than the base_uint (16 bytes vs 12 bytes for + // test96). Only the first 12 bytes are copied; the extra bytes are + // ignored. + Blob const tooBig{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; + BaseUInt96 const result = BaseUInt96::fromRaw(tooBig); + auto const resultText = to_string(result); + EXPECT_EQ(resultText, "0102030405060708090A0B0C") << resultText; + }; + EXPECT_DEBUG_DEATH(largeConstruct(), "input size match"); + + auto smallCopy = [] { + // Container smaller than the base_uint (8 bytes vs 12 bytes for + // test96). Only the first 8 bytes are copied; the remaining 4 bytes + // stay zero. + Blob const tooSmall{1, 2, 3, 4, 5, 6, 7, 8}; + BaseUInt96 result{}; + --result; + { + auto const originalText = to_string(result); + EXPECT_EQ(originalText, "FFFFFFFFFFFFFFFFFFFFFFFF") << originalText; + } + result = tooSmall; + auto const resultText = to_string(result); + EXPECT_EQ(resultText, "010203040506070800000000") << resultText; + }; + EXPECT_DEBUG_DEATH(smallCopy(), "input size match"); + + auto const largeCopy = [] { + // Container larger than the base_uint (16 bytes vs 12 bytes for + // test96). Only the first 12 bytes are copied; the extra bytes are + // ignored. + Blob const tooBig{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}; + BaseUInt96 result{}; + --result; + { + auto const originalText = to_string(result); + EXPECT_EQ(originalText, "FFFFFFFFFFFFFFFFFFFFFFFF") << originalText; + } + result = tooBig; + auto const resultText = to_string(result); + EXPECT_EQ(resultText, "0102030405060708090A0B0C") << resultText; + }; + EXPECT_DEBUG_DEATH(largeCopy(), "input size match"); +#endif +} + TEST_F(BaseUintTest, base_uint) { static_assert(!std::is_constructible_v>); @@ -198,6 +265,19 @@ TEST_F(BaseUintTest, base_uint) EXPECT_EQ(d, 0); } + { + // There are several ways to create a zero. beast::kZero is tested above. Test some + // others. + BaseUInt96 const z1; + EXPECT_EQ(z1, z) << to_string(z1); + + BaseUInt96 const z2{}; + EXPECT_EQ(z2, z) << to_string(z2); + + BaseUInt96 const z3{0u}; + EXPECT_EQ(z3, z) << to_string(z3); + } + BaseUInt96 n{z}; n++; EXPECT_EQ(n, BaseUInt96(1)); From cda63d00a29ef7fd42f88d7296655bf951410ca0 Mon Sep 17 00:00:00 2001 From: Kassaking7 <96991820+Kassaking7@users.noreply.github.com> Date: Tue, 14 Jul 2026 15:41:53 -0400 Subject: [PATCH 10/12] fix: Add amendment sponsor for AccountRootsDeletedClean (#7801) --- src/libxrpl/tx/invariants/InvariantCheck.cpp | 2 +- src/test/app/Invariants_test.cpp | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 3615594d19..9b997e06dd 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -489,7 +489,7 @@ AccountRootsDeletedClean::finalize( // feature is enabled. Enabled, or not, though, a fatal-level message will // be logged [[maybe_unused]] bool const enforce = view.rules().enabled(fixCleanup3_2_0) || - view.rules().enabled(featureSingleAssetVault) || + view.rules().enabled(featureSponsor) || view.rules().enabled(featureSingleAssetVault) || view.rules().enabled(featureLendingProtocol); auto const objectExists = [&view, enforce, &j](auto const& keylet) { diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 9c90ade72f..076e39a42b 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -412,6 +412,23 @@ class Invariants_test : public beast::unit_test::Suite XRPAmount{}, STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); + doInvariantCheck( + Env{*this, FeatureBitset{featureSponsor}}, + {{"account deletion left behind a sponsorship field"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + auto const sleA1 = ac.view().peek(keylet::account(a1.id())); + if (!sleA1) + return false; + sleA1->at(sfBalance) = beast::kZero; + sleA1->setAccountID(sfSponsor, a2.id()); + + ac.view().erase(sleA1); + + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); + for (auto const& keyletInfo : kDirectAccountKeylets) { // TODO: Use structured binding once LLVM 16 is the minimum From e7d335e09e348b8000526ee55990f37a3fc85fc7 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 14 Jul 2026 19:31:22 -0400 Subject: [PATCH 11/12] Clean up the loop to make it more understandable, add logging - Hopefully, it'll make it more understandable. --- include/xrpl/basics/TaggedCache.ipp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 0b12231051..447743a7b7 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -613,20 +613,23 @@ TaggedCache 0) + { + JLOG(journal_.info()) + << "getKeys(): Cache grew beyond allocated capacity after " + << allocationIterations << " prior attempt(s). Have " << v.capacity() + << ", need " << size << ". Retrying allocation"; + } // Allocate the current size plus a little extra, in case the cache grows while // allocating. Each time another allocation is needed, the extra also gets bigger until // it ultimately doubles the size + 1. - size += (size >> (4 - std::min(allocationIterations, std::size_t{4}))) + 1; + constexpr std::size_t baseShift = 5; + auto const bufferOffset = std::min(allocationIterations, std::size_t{baseShift}); + auto const bufferShift = baseShift - bufferOffset; + size += (size >> bufferShift) + 1; v.reserve(size); ++allocationIterations; } - // In a normal operating environment, because of the padding added to size before - // allocating, even 2 iterations is going to be very rare. If 3 or more are ever needed, - // that's unusual enough that I want to know about it. Don't ask me to change it without - // empirical data. - Ed H. - XRPL_ASSERT( - allocationIterations < 3, - "xrpl::TaggedCache::getKeys(): limited allocation iterations"); if (v.capacity() < cache_.size()) { // LCOV_EXCL_START From 50a7a8e3e033b38c1201a87cbebe30d427f97bc6 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 14 Jul 2026 20:21:23 -0400 Subject: [PATCH 12/12] Experiment. Do not push --- include/xrpl/basics/TaggedCache.ipp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 447743a7b7..710d6f6174 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -623,10 +623,11 @@ TaggedCache> bufferShift) + 1; + auto const buffer = (size >> bufferShift) + 1; + size += buffer; v.reserve(size); ++allocationIterations; }