From dcd2ff0b5f2019c40c3f38640e751426df97d3c8 Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Sat, 23 May 2026 02:40:26 -0400 Subject: [PATCH 01/10] fix: Fix non-canonical MPT amount (#7117) 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> --- include/xrpl/protocol/STAmount.h | 17 +- include/xrpl/tx/Transactor.h | 12 + include/xrpl/tx/invariants/InvariantCheck.h | 16 + .../xrpl/tx/transactors/escrow/EscrowCreate.h | 3 + src/libxrpl/protocol/STAmount.cpp | 46 + src/libxrpl/tx/Transactor.cpp | 10 + src/libxrpl/tx/invariants/InvariantCheck.cpp | 31 + .../tx/transactors/check/CheckCash.cpp | 5 + .../tx/transactors/escrow/EscrowCreate.cpp | 12 +- src/test/app/Invariants_test.cpp | 57 ++ src/test/app/MPToken_test.cpp | 867 +++++++++++++++++- src/test/app/OfferMPT_test.cpp | 2 +- 12 files changed, 1074 insertions(+), 4 deletions(-) diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index c576c0da31..bf3e25eedb 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -3,11 +3,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -593,12 +595,25 @@ STAmount::value() const noexcept return *this; } -inline bool +[[nodiscard]] inline bool isLegalNet(STAmount const& value) { return !value.native() || (value.mantissa() <= STAmount::kMaxNativeN); } +[[nodiscard]] inline bool +isLegalMPT(STAmount const& value) +{ + return !value.holds() || + (!value.negative() && value.exponent() == 0 && value.mantissa() <= kMaxMpTokenAmount); +} + +/* Check recursively if an object has invalid MPTAmount or XRPAmount in STAmount field. + * Calls isLegalNet() and isLegalMPT(). + */ +[[nodiscard]] bool +hasInvalidAmount(STBase const& field, beast::Journal j); + //------------------------------------------------------------------------------ // // Operators diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 61d943c4d5..1440a5097f 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -398,6 +398,15 @@ private: static NotTEC preflight2(PreflightContext const& ctx); + /** Universal validations + - Valid MPTAmount and XRPAmount + + Do not try to call preflightUniversal from preflight() in derived classes. See + the description of invokePreflight for details. + */ + static NotTEC + preflightUniversal(PreflightContext const& ctx); + /** Check transaction-specific invariants only. * * Walks every modified ledger entry via visitInvariantEntry, then @@ -463,6 +472,9 @@ Transactor::invokePreflight(PreflightContext const& ctx) if (auto const ret = preflight1(ctx, T::getFlagsMask(ctx))) return ret; + if (auto const ret = preflightUniversal(ctx)) + return ret; + if (auto const ret = T::preflight(ctx)) return ret; diff --git a/include/xrpl/tx/invariants/InvariantCheck.h b/include/xrpl/tx/invariants/InvariantCheck.h index 5996039bf0..d4c0154269 100644 --- a/include/xrpl/tx/invariants/InvariantCheck.h +++ b/include/xrpl/tx/invariants/InvariantCheck.h @@ -373,6 +373,21 @@ public: finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); }; +/** Verify that MPT/XRP STAmounts are canonical in any ledger entries left after the + * transaction applies. + */ +class ValidAmounts +{ + std::vector> afterEntries_; + +public: + void + visitEntry(bool, std::shared_ptr const&, std::shared_ptr const&); + + [[nodiscard]] bool + finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; +}; + // additional invariant checks can be declared above and then added to this // tuple using InvariantChecks = std::tuple< @@ -402,6 +417,7 @@ using InvariantChecks = std::tuple< ValidLoan, ValidVault, ValidMPTPayment, + ValidAmounts, ValidMPTTransfer>; /** diff --git a/include/xrpl/tx/transactors/escrow/EscrowCreate.h b/include/xrpl/tx/transactors/escrow/EscrowCreate.h index 8682ed7369..2e9da89896 100644 --- a/include/xrpl/tx/transactors/escrow/EscrowCreate.h +++ b/include/xrpl/tx/transactors/escrow/EscrowCreate.h @@ -16,6 +16,9 @@ public: static TxConsequences makeTxConsequences(PreflightContext const& ctx); + static bool + checkExtraFeatures(PreflightContext const& ctx); + static NotTEC preflight(PreflightContext const& ctx); diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index c40beabf12..1ba9cd042f 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -19,8 +19,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -1222,6 +1224,50 @@ operator-(STAmount const& value) STAmount::Unchecked{}); } +static bool +hasInvalidAmount(STBase const& field, int depth, beast::Journal j); + +static bool +hasInvalidAmount(STObject const& object, int depth, beast::Journal j) +{ + return std::ranges::any_of( + object, [&](STBase const& field) { return hasInvalidAmount(field, depth, j); }); +} + +static bool +hasInvalidAmount(STArray const& array, int depth, beast::Journal j) +{ + return std::ranges::any_of( + array, [&](STObject const& object) { return hasInvalidAmount(object, depth, j); }); +} + +static bool +hasInvalidAmount(STBase const& field, int depth, beast::Journal j) +{ + if (depth > 10) + { + JLOG(j.error()) << "hasInvalidAmount: depth exceeds 10"; + return true; + } + + if (auto const amount = dynamic_cast(&field)) + return !isLegalMPT(*amount) || !isLegalNet(*amount); + + if (auto const object = dynamic_cast(&field)) + return hasInvalidAmount(*object, depth + 1, j); + + if (auto const array = dynamic_cast(&field)) + return hasInvalidAmount(*array, depth + 1, j); + + return false; +} + +bool +hasInvalidAmount(STBase const& field, beast::Journal j) +{ + return hasInvalidAmount(field, 0, j); +} + //------------------------------------------------------------------------------ // // Arithmetic diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 97f2cabff2..28fa059902 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include // IWYU pragma: keep @@ -256,6 +257,15 @@ Transactor::preflight2(PreflightContext const& ctx) return tesSUCCESS; } +NotTEC +Transactor::preflightUniversal(PreflightContext const& ctx) +{ + if (ctx.rules.enabled(fixCleanup3_2_0) && hasInvalidAmount(ctx.tx, ctx.j)) + return temBAD_AMOUNT; + + return tesSUCCESS; +} + //------------------------------------------------------------------------------ Transactor::Transactor(ApplyContext& ctx) diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 3e51b6f877..0154dca747 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -1080,4 +1080,35 @@ NoModifiedUnmodifiableFields::finalize( return true; } +void +ValidAmounts::visitEntry( + bool isDelete, + std::shared_ptr const&, + std::shared_ptr const& after) +{ + if (!isDelete && after) + afterEntries_.push_back(after); +} + +bool +ValidAmounts::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) const +{ + bool const badLedgerEntry = std::ranges::any_of( + afterEntries_, [&](auto const& sle) { return hasInvalidAmount(*sle, j); }); + + if (badLedgerEntry) + { + JLOG(j.fatal()) + << "Invariant failed: ledger entry contains non-canonical MPT or XRP amount"; + return !view.rules().enabled(fixCleanup3_2_0); + } + + return true; +} + } // namespace xrpl diff --git a/src/libxrpl/tx/transactors/check/CheckCash.cpp b/src/libxrpl/tx/transactors/check/CheckCash.cpp index cfc133b501..af903ff177 100644 --- a/src/libxrpl/tx/transactors/check/CheckCash.cpp +++ b/src/libxrpl/tx/transactors/check/CheckCash.cpp @@ -139,6 +139,11 @@ CheckCash::preclaim(PreclaimContext const& ctx) }(ctx.tx)}; STAmount const sendMax = sleCheck->at(sfSendMax); + // A legacy Check may contain a non-canonical MPT sfSendMax. Universal + // preflight only validates the CheckCash transaction, not the stored Check. + if (ctx.view.rules().enabled(fixCleanup3_2_0) && !isLegalMPT(sendMax)) + return tefBAD_LEDGER; + if (!equalTokens(value.asset(), sendMax.asset())) { JLOG(ctx.j.warn()) << "Check cash does not match check currency."; diff --git a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp index 0b1db125f6..4de302db3e 100644 --- a/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp +++ b/src/libxrpl/tx/transactors/escrow/EscrowCreate.cpp @@ -81,6 +81,16 @@ EscrowCreate::makeTxConsequences(PreflightContext const& ctx) return TxConsequences{ctx.tx, isXRP(amount) ? amount.xrp() : beast::kZero}; } +bool +EscrowCreate::checkExtraFeatures(PreflightContext const& ctx) +{ + // Only require featureMPTokensV1 when the escrow amount is an MPT and + // fixCleanup3_2_0 is active; XRP/IOU escrows are unaffected by this gate. + if (ctx.rules.enabled(fixCleanup3_2_0) && ctx.tx[sfAmount].holds()) + return ctx.rules.enabled(featureMPTokensV1); + return true; +} + template static NotTEC escrowCreatePreflightHelper(PreflightContext const& ctx); @@ -103,7 +113,7 @@ template <> NotTEC escrowCreatePreflightHelper(PreflightContext const& ctx) { - if (!ctx.rules.enabled(featureMPTokensV1)) + if (!ctx.rules.enabled(fixCleanup3_2_0) && !ctx.rules.enabled(featureMPTokensV1)) return temDISABLED; auto const amount = ctx.tx[sfAmount]; diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 6fd1904992..cc16948553 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4065,6 +4065,63 @@ class Invariants_test : public beast::unit_test::Suite using namespace test::jtx; testcase << "MPT"; + MPTIssue const nonCanonicalMPTIssue{makeMptID(1, AccountID(0x4985601))}; + auto const nonCanonicalMPTAmount = [&](SField const& field) { + return STAmount{ + field, + nonCanonicalMPTIssue, + kMaxMpTokenAmount + std::uint64_t{1}, + 0, + false, + STAmount::Unchecked{}}; + }; + auto const negativeMPTAmount = [&](SField const& field) { + return STAmount{field, nonCanonicalMPTIssue, 2, 0, true, STAmount::Unchecked{}}; + }; + auto const nonCanonicalMPTPayment = [&]() { + return STTx{ttPAYMENT, [&](STObject& tx) { + tx.setFieldAmount(sfAmount, nonCanonicalMPTAmount(sfAmount)); + }}; + }; + + doInvariantCheck( + Env{*this, defaultAmendments() - fixCleanup3_2_0}, + {}, + [](Account const&, Account const&, ApplyContext&) { return true; }, + XRPAmount{}, + nonCanonicalMPTPayment(), + {tesSUCCESS, tesSUCCESS}); + + doInvariantCheck( + {{"ledger entry contains non-canonical MPT or XRP amount"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + auto const sle = ac.view().peek(keylet::account(a1.id())); + if (!sle) + return false; + + auto sleNew = std::make_shared(keylet::check(a1.id(), (*sle)[sfSequence])); + sleNew->setAccountID(sfAccount, a1.id()); + sleNew->setAccountID(sfDestination, a2.id()); + sleNew->setFieldAmount(sfSendMax, nonCanonicalMPTAmount(sfSendMax)); + ac.view().insert(sleNew); + return true; + }); + + doInvariantCheck( + {{"ledger entry contains non-canonical MPT or XRP amount"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + auto const sle = ac.view().peek(keylet::account(a1.id())); + if (!sle) + return false; + + auto sleNew = std::make_shared(keylet::check(a1.id(), (*sle)[sfSequence])); + sleNew->setAccountID(sfAccount, a1.id()); + sleNew->setAccountID(sfDestination, a2.id()); + sleNew->setFieldAmount(sfSendMax, negativeMPTAmount(sfSendMax)); + ac.view().insert(sleNew); + return true; + }); + // MPT OutstandingAmount > MaximumAmount doInvariantCheck( {{"OutstandingAmount overflow"}}, diff --git a/src/test/app/MPToken_test.cpp b/src/test/app/MPToken_test.cpp index 6906c4aba0..3d6cff0885 100644 --- a/src/test/app/MPToken_test.cpp +++ b/src/test/app/MPToken_test.cpp @@ -25,11 +25,13 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -49,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -57,13 +60,17 @@ #include #include +#include #include #include #include +#include +#include #include #include #include #include +#include #include #include #include @@ -2115,6 +2122,864 @@ class MPToken_test : public beast::unit_test::Suite BEAST_EXPECT(txWithAmounts.empty()); } + void + testNonCanonicalMPTAmountCleanup(FeatureBitset features) + { + using namespace test::jtx; + using namespace std::literals; + FeatureBitset const withoutFix = features - fixCleanup3_2_0; + FeatureBitset const withFix = features | fixCleanup3_2_0; + FeatureBitset const withoutFixAndV2 = withoutFix - featureMPTokensV2; + FeatureBitset const withFixAndWithoutV2 = withFix - featureMPTokensV2; + + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const gw{"gw"}; + + using MPTValue = MPTAmount::value_type; + MPTValue const mptMin = std::numeric_limits::min(); + MPTValue const mptMax = std::numeric_limits::max(); + std::uint64_t const u64Max = std::numeric_limits::max(); + std::uint64_t const firstInvalidMPTMantissa = static_cast(mptMax) + 1; + MPTValue const alice0 = 10'000; + MPTValue const gw0 = -20'000; + TER const success = tesSUCCESS; + TER const invariantFailed = tecINVARIANT_FAILED; + TER const pathPartial = tecPATH_PARTIAL; + TER const badAmountTer = temBAD_AMOUNT; + + struct BadMPTAmount + { + std::string_view name; + std::uint64_t mantissa; + bool negative; + MPTValue mptValue; + TER issuerToHolderPreFixTer; + TER holderSourcePreFixTer; + MPTValue issuerToHolderAliceAfterPreFix; + MPTValue issuerToHolderIssuerAfterPreFix; + MPTValue issuerToHolderAliceAfterPostFix; + MPTValue issuerToHolderIssuerAfterPostFix; + }; + // clang-format off + std::array const badMPTAmounts = {{ + { .name="INT64_MAX + 1", .mantissa=firstInvalidMPTMantissa, .negative=false, .mptValue=mptMin, .issuerToHolderPreFixTer=invariantFailed, .holderSourcePreFixTer=pathPartial, .issuerToHolderAliceAfterPreFix=alice0, .issuerToHolderIssuerAfterPreFix=gw0, .issuerToHolderAliceAfterPostFix=alice0 - 1, .issuerToHolderIssuerAfterPostFix=gw0 + 1}, + { .name="INT64_MAX + 10", .mantissa=firstInvalidMPTMantissa + 9, .negative=false, .mptValue=mptMin + 9, .issuerToHolderPreFixTer=invariantFailed, .holderSourcePreFixTer=pathPartial, .issuerToHolderAliceAfterPreFix=alice0, .issuerToHolderIssuerAfterPreFix=gw0, .issuerToHolderAliceAfterPostFix=alice0 - 1, .issuerToHolderIssuerAfterPostFix=gw0 + 1}, + { .name="UINT64_MAX - 9998", .mantissa=u64Max - 9'998, .negative=false, .mptValue=MPTValue{-9'999}, .issuerToHolderPreFixTer=success, .holderSourcePreFixTer=pathPartial, .issuerToHolderAliceAfterPreFix=alice0 - 9'999, .issuerToHolderIssuerAfterPreFix=gw0 + 9'999, .issuerToHolderAliceAfterPostFix=alice0 - 10'000, .issuerToHolderIssuerAfterPostFix=gw0 + 10'000}, + { .name="UINT64_MAX - 9", .mantissa=u64Max - 9, .negative=false, .mptValue=MPTValue{-10}, .issuerToHolderPreFixTer=success, .holderSourcePreFixTer=pathPartial, .issuerToHolderAliceAfterPreFix=alice0 - 10, .issuerToHolderIssuerAfterPreFix=gw0 + 10, .issuerToHolderAliceAfterPostFix=alice0 - 11, .issuerToHolderIssuerAfterPostFix=gw0 + 11}, + { .name="UINT64_MAX - 1", .mantissa=u64Max - 1, .negative=false, .mptValue=MPTValue{-2}, .issuerToHolderPreFixTer=success, .holderSourcePreFixTer=pathPartial, .issuerToHolderAliceAfterPreFix=alice0 - 2, .issuerToHolderIssuerAfterPreFix=gw0 + 2, .issuerToHolderAliceAfterPostFix=alice0 - 3, .issuerToHolderIssuerAfterPostFix=gw0 + 3}, + { .name="UINT64_MAX", .mantissa=u64Max, .negative=false, .mptValue=MPTValue{-1}, .issuerToHolderPreFixTer=success, .holderSourcePreFixTer=pathPartial, .issuerToHolderAliceAfterPreFix=alice0 - 1, .issuerToHolderIssuerAfterPreFix=gw0 + 1, .issuerToHolderAliceAfterPostFix=alice0 - 2, .issuerToHolderIssuerAfterPostFix=gw0 + 2}, + { .name="-2", .mantissa=std::uint64_t{2}, .negative=true, .mptValue=MPTValue{-2}, .issuerToHolderPreFixTer=badAmountTer, .holderSourcePreFixTer=badAmountTer, .issuerToHolderAliceAfterPreFix=alice0, .issuerToHolderIssuerAfterPreFix=gw0, .issuerToHolderAliceAfterPostFix=alice0 - 1, .issuerToHolderIssuerAfterPostFix=gw0 + 1} + }}; + // clang-format on + auto const badMPTAmount = [&](MPTIssue const& issue, BadMPTAmount const& bad) { + return STAmount{issue, bad.mantissa, 0, bad.negative, STAmount::Unchecked{}}; + }; + auto const makeIssue = [&](Env& env) { + MPTTester const mpt{ + {.env = env, + .issuer = gw, + .holders = {alice, bob}, + .pay = 10'000, + .flags = tfMPTCanTransfer | tfMPTCanTrade | tfMPTCanEscrow | tfMPTCanClawback}}; + return MPTIssue{mpt.issuanceID()}; + }; + auto const withNonCanonicalMPTAmount = + [](JTx jt, SField const& field, STAmount const& amount, Account const& signer) { + STTx tx{*jt.stx}; + tx.setFieldAmount(field, amount); + tx.sign(signer.pk(), signer.sk()); + jt.stx = std::make_shared(tx); + return jt; + }; + auto const roundTrip = [](STTx const& tx) { + Serializer s; + tx.add(s); + SerialIter sit{s.slice()}; + return STTx{sit}; + }; + auto const expectRoundTripBadMPT = + [&](JTx const& jt, SField const& field, BadMPTAmount const& bad) { + auto const roundTripped = roundTrip(*jt.stx); + auto const persisted = roundTripped.getFieldAmount(field); + BEAST_EXPECT(persisted.holds()); + BEAST_EXPECT(persisted.mantissa() == bad.mantissa); + BEAST_EXPECT(persisted.exponent() == 0); + BEAST_EXPECT(persisted.negative() == bad.negative); + BEAST_EXPECT(persisted.mpt().value() == bad.mptValue); + if (!bad.negative) + BEAST_EXPECT(persisted.mantissa() > kMaxMpTokenAmount); + }; + + for (auto const& bad : badMPTAmounts) + { + testcase("fixCleanup3_2_0 rejects non-canonical MPT Payment amounts"); + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto malformedHolderToHolder = withNonCanonicalMPTAmount( + env.jt(pay(alice, bob, STAmount{issue, std::uint64_t{1}})), + sfAmount, + badAmount, + alice); + expectRoundTripBadMPT(malformedHolderToHolder, sfAmount, bad); + malformedHolderToHolder.ter = bad.holderSourcePreFixTer; + env.submit(malformedHolderToHolder); + env.close(); + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{-20'000}, issue})); + + env.enableFeature(fixCleanup3_2_0); + env.close(); + env(env.jt(pay(bob, alice, STAmount{issue, std::uint64_t{1}})), Ter{tesSUCCESS}); + env.close(); + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'001}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{9'999}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{-20'000}, issue})); + } + { + Env env{*this, envconfig(), withoutFixAndV2, nullptr, beast::Severity::Disabled}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto malformedIssuerToHolder = withNonCanonicalMPTAmount( + env.jt(pay(gw, alice, STAmount{issue, std::uint64_t{1}})), + sfAmount, + badAmount, + gw); + expectRoundTripBadMPT(malformedIssuerToHolder, sfAmount, bad); + malformedIssuerToHolder.ter = bad.issuerToHolderPreFixTer; + env.submit(malformedIssuerToHolder); + env.close(); + BEAST_EXPECT( + (env.balance(alice, issue).value() == + STAmount{MPTAmount{bad.issuerToHolderAliceAfterPreFix}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == + STAmount{MPTAmount{bad.issuerToHolderIssuerAfterPreFix}, issue})); + + env.enableFeature(fixCleanup3_2_0); + env.close(); + env(env.jt(pay(alice, gw, STAmount{issue, std::uint64_t{1}})), Ter{tesSUCCESS}); + env.close(); + BEAST_EXPECT( + (env.balance(alice, issue).value() == + STAmount{MPTAmount{bad.issuerToHolderAliceAfterPostFix}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == + STAmount{MPTAmount{bad.issuerToHolderIssuerAfterPostFix}, issue})); + } + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto malformedHolderToIssuer = withNonCanonicalMPTAmount( + env.jt(pay(alice, gw, STAmount{issue, std::uint64_t{1}})), + sfAmount, + badAmount, + alice); + expectRoundTripBadMPT(malformedHolderToIssuer, sfAmount, bad); + malformedHolderToIssuer.ter = bad.holderSourcePreFixTer; + env.submit(malformedHolderToIssuer); + env.close(); + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{-20'000}, issue})); + + env.enableFeature(fixCleanup3_2_0); + env.close(); + env(env.jt(pay(gw, alice, STAmount{issue, std::uint64_t{1}})), Ter{tesSUCCESS}); + env.close(); + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'001}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{-20'001}, issue})); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(pay(alice, bob, STAmount{issue, std::uint64_t{1}})), + sfAmount, + badAmount, + alice); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(pay(gw, alice, STAmount{issue, std::uint64_t{1}})), + sfAmount, + badAmount, + gw); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(pay(alice, gw, STAmount{issue, std::uint64_t{1}})), + sfAmount, + badAmount, + alice); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("fixCleanup3_2_0 rejects non-canonical MPT Check amounts"); + { + Env env{*this, envconfig(), withoutFix, nullptr, beast::Severity::Disabled}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badSendMax = badMPTAmount(issue, bad); + auto const checkSeq = env.seq(alice); + auto tx = withNonCanonicalMPTAmount( + env.jt(check::create(alice, bob, STAmount{issue, std::uint64_t{10}})), + sfSendMax, + badSendMax, + alice); + tx.ter = bad.negative ? TER{temBAD_AMOUNT} : TER{tesSUCCESS}; + env.submit(tx); + env.close(); + + auto const checkKeylet = keylet::check(alice.id(), checkSeq); + auto const sleCheck = env.le(checkKeylet); + BEAST_EXPECT((sleCheck != nullptr) == !bad.negative); + if (sleCheck && !bad.negative) + { + auto const persisted = sleCheck->getFieldAmount(sfSendMax); + BEAST_EXPECT(persisted.holds()); + BEAST_EXPECT(persisted.mantissa() == bad.mantissa); + BEAST_EXPECT(persisted.negative() == bad.negative); + } + } + { + Env env{*this, envconfig(), withoutFix, nullptr, beast::Severity::Disabled}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badSendMax = badMPTAmount(issue, bad); + auto const checkSeq = env.seq(alice); + auto tx = withNonCanonicalMPTAmount( + env.jt(check::create(alice, bob, STAmount{issue, std::uint64_t{10}})), + sfSendMax, + badSendMax, + alice); + tx.ter = bad.negative ? TER{temBAD_AMOUNT} : TER{tesSUCCESS}; + env.submit(tx); + env.close(); + + auto const checkKeylet = keylet::check(alice.id(), checkSeq); + BEAST_EXPECT((env.le(checkKeylet) != nullptr) == !bad.negative); + if (!bad.negative) + { + // CheckCancel has no amount fields, but it must be able to + // remove a malformed legacy Check while the fix is disabled. + env(env.jt(check::cancel(alice, checkKeylet.key)), Ter{tesSUCCESS}); + env.close(); + BEAST_EXPECT(env.le(checkKeylet) == nullptr); + } + } + { + Env env{*this, envconfig(), withoutFix, nullptr, beast::Severity::Disabled}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badSendMax = badMPTAmount(issue, bad); + auto const checkSeq = env.seq(alice); + auto tx = withNonCanonicalMPTAmount( + env.jt(check::create(alice, bob, STAmount{issue, std::uint64_t{10}})), + sfSendMax, + badSendMax, + alice); + tx.ter = bad.negative ? TER{temBAD_AMOUNT} : TER{tesSUCCESS}; + env.submit(tx); + env.close(); + + auto const checkKeylet = keylet::check(alice.id(), checkSeq); + BEAST_EXPECT((env.le(checkKeylet) != nullptr) == !bad.negative); + if (!bad.negative) + { + env.enableFeature(fixCleanup3_2_0); + env.close(); + + // Once the fix is enabled, CheckCancel should still remove + // a legacy Check because it does not consume the bad amount. + env(env.jt(check::cancel(alice, checkKeylet.key)), Ter{tesSUCCESS}); + env.close(); + BEAST_EXPECT(env.le(checkKeylet) == nullptr); + } + } + { + Env env{*this, envconfig(), withoutFix, nullptr, beast::Severity::Disabled}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badSendMax = badMPTAmount(issue, bad); + auto const checkSeq = env.seq(alice); + auto tx = withNonCanonicalMPTAmount( + env.jt(check::create(alice, bob, STAmount{issue, std::uint64_t{10}})), + sfSendMax, + badSendMax, + alice); + tx.ter = bad.negative ? TER{temBAD_AMOUNT} : TER{tesSUCCESS}; + env.submit(tx); + env.close(); + + auto const checkKeylet = keylet::check(alice.id(), checkSeq); + BEAST_EXPECT((env.le(checkKeylet) != nullptr) == !bad.negative); + if (!bad.negative) + { + env.enableFeature(fixCleanup3_2_0); + env.close(); + + auto const cashAmount = STAmount{sfAmount, issue, std::uint64_t{1}, 0, false}; + env(env.jt(check::cash(bob, checkKeylet.key, cashAmount)), Ter{tefBAD_LEDGER}); + env.close(); + BEAST_EXPECT(env.le(checkKeylet) != nullptr); + } + } + { + Env env{*this, withoutFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const sendMax = STAmount{sfSendMax, issue, std::uint64_t{10}, 0, false}; + auto const checkSeq = env.seq(alice); + env(env.jt(check::create(alice, bob, sendMax)), Ter{tesSUCCESS}); + env.close(); + + auto const badCashAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + check::cash( + bob, + keylet::check(alice.id(), checkSeq).key, + STAmount{issue, std::uint64_t{1}})), + sfAmount, + badCashAmount, + bob); + expectRoundTripBadMPT(tx, sfAmount, bad); + tx.ter = bad.holderSourcePreFixTer; + env.submit(tx); + env.close(); + BEAST_EXPECT(env.le(keylet::check(alice.id(), checkSeq)) != nullptr); + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{-20'000}, issue})); + } + { + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const sendMax = STAmount{sfSendMax, issue, std::uint64_t{10}, 0, false}; + auto const checkSeq = env.seq(alice); + env(env.jt(check::create(alice, bob, sendMax)), Ter{tesSUCCESS}); + env.close(); + + auto const badCashAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + check::cash( + bob, + keylet::check(alice.id(), checkSeq).key, + STAmount{issue, std::uint64_t{1}})), + sfAmount, + badCashAmount, + bob); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("fixCleanup3_2_0 rejects non-canonical MPT Escrow amounts"); + { + Env env{*this, withoutFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const escrowSeq = env.seq(alice); + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + escrow::create(alice, bob, STAmount{issue, std::uint64_t{1}}), + escrow::kFinishTime(env.now() + 1s)), + sfAmount, + badAmount, + alice); + tx.ter = bad.negative ? TER{temBAD_AMOUNT} : TER{tecINSUFFICIENT_FUNDS}; + env.submit(tx); + env.close(); + BEAST_EXPECT(env.le(keylet::escrow(alice.id(), escrowSeq)) == nullptr); + } + { + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + escrow::create(alice, bob, STAmount{issue, std::uint64_t{1}}), + escrow::kFinishTime(env.now() + 1s)), + sfAmount, + badAmount, + alice); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("fixCleanup3_2_0 rejects non-canonical MPT Clawback amounts"); + { + Env env{*this, withoutFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(claw(gw, STAmount{issue, std::uint64_t{1}}, bob)), + sfAmount, + badAmount, + gw); + expectRoundTripBadMPT(tx, sfAmount, bad); + tx.ter = bad.negative ? TER{temBAD_AMOUNT} : TER{tesSUCCESS}; + env.submit(tx); + env.close(); + + MPTValue const bobAfter = bad.negative ? MPTValue{10'000} : MPTValue{0}; + MPTValue const gwAfter = bad.negative ? MPTValue{-20'000} : MPTValue{-10'000}; + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{bobAfter}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{gwAfter}, issue})); + } + { + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(claw(gw, STAmount{issue, std::uint64_t{1}}, bob)), + sfAmount, + badAmount, + gw); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + env.close(); + + BEAST_EXPECT( + (env.balance(alice, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(bob, issue).value() == STAmount{MPTAmount{10'000}, issue})); + BEAST_EXPECT( + (env.balance(gw, issue).value() == STAmount{MPTAmount{-20'000}, issue})); + } + + testcase("featureMPTokensV2 disabled rejects MPT OfferCreate amounts"); + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badTakerPays = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(offer(alice, STAmount{issue, std::uint64_t{1}}, XRP(10))), + sfTakerPays, + badTakerPays, + alice); + expectRoundTripBadMPT(tx, sfTakerPays, bad); + tx.ter = temDISABLED; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badTakerPays = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(offer(alice, STAmount{issue, std::uint64_t{1}}, XRP(10))), + sfTakerPays, + badTakerPays, + alice); + tx.ter = temDISABLED; + env.submit(tx); + } + { + // sfTakerPays is MPT: both amendments active. Negative offers + // fail in OfferCreate::preflight() before the universal check; + // positive non-canonical amounts fail in the universal check. + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badTakerPays = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(offer(alice, STAmount{issue, std::uint64_t{1}}, XRP(10))), + sfTakerPays, + badTakerPays, + alice); + tx.ter = TER{temBAD_AMOUNT}; + env.submit(tx); + } + { + // sfTakerGets is MPT: both amendments active. Negative offers + // fail in OfferCreate::preflight() before the universal check; + // positive non-canonical amounts fail in the universal check. + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badTakerGets = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt(offer(alice, XRP(10), STAmount{issue, std::uint64_t{1}})), + sfTakerGets, + badTakerGets, + alice); + tx.ter = TER{temBAD_AMOUNT}; + env.submit(tx); + } + + testcase("featureMPTokensV2 disabled rejects MPT AMMCreate amounts"); + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::createJv(alice.id(), STAmount{issue, std::uint64_t{1}}, XRP(1), 0), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + expectRoundTripBadMPT(tx, sfAmount, bad); + tx.ter = temDISABLED; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::createJv(alice.id(), STAmount{issue, std::uint64_t{1}}, XRP(1), 0), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + tx.ter = temDISABLED; + env.submit(tx); + } + { + // sfAmount is MPT: both amendments active, expect temBAD_AMOUNT + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::createJv(alice.id(), STAmount{issue, std::uint64_t{1}}, XRP(1), 0), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("featureMPTokensV2 disabled rejects MPT AMMDeposit amounts"); + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::depositJv( + {.account = alice, + .asset1In = STAmount{issue, std::uint64_t{1}}, + .assets = std::make_pair(Asset{issue}, Asset{xrpIssue()})}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + expectRoundTripBadMPT(tx, sfAmount, bad); + tx.ter = temDISABLED; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::depositJv( + {.account = alice, + .asset1In = STAmount{issue, std::uint64_t{1}}, + .assets = std::make_pair(Asset{issue}, Asset{xrpIssue()})}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + tx.ter = temDISABLED; + env.submit(tx); + } + { + // sfAmount is MPT: both amendments active, expect temBAD_AMOUNT + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::depositJv( + {.account = alice, + .asset1In = STAmount{issue, std::uint64_t{1}}, + .assets = std::make_pair(Asset{issue}, Asset{xrpIssue()})}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("featureMPTokensV2 disabled rejects MPT AMMWithdraw amounts"); + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::withdrawJv( + {.account = alice, + .asset1Out = STAmount{issue, std::uint64_t{1}}, + .assets = std::make_pair(Asset{issue}, Asset{xrpIssue()})}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + expectRoundTripBadMPT(tx, sfAmount, bad); + tx.ter = temDISABLED; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::withdrawJv( + {.account = alice, + .asset1Out = STAmount{issue, std::uint64_t{1}}, + .assets = std::make_pair(Asset{issue}, Asset{xrpIssue()})}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + tx.ter = temDISABLED; + env.submit(tx); + } + { + // sfAmount is MPT: both amendments active, expect temBAD_AMOUNT + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + AMM::withdrawJv( + {.account = alice, + .asset1Out = STAmount{issue, std::uint64_t{1}}, + .assets = std::make_pair(Asset{issue}, Asset{xrpIssue()})}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + alice); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("featureMPTokensV2 disabled rejects MPT AMMClawback amounts"); + { + Env env{*this, withoutFixAndV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + amm::ammClawback( + gw, + alice, + Asset{issue}, + Asset{xrpIssue()}, + std::make_optional(STAmount{issue, std::uint64_t{1}})), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + gw); + expectRoundTripBadMPT(tx, sfAmount, bad); + tx.ter = temDISABLED; + env.submit(tx); + } + { + Env env{*this, withFixAndWithoutV2}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + amm::ammClawback( + gw, + alice, + Asset{issue}, + Asset{xrpIssue()}, + std::make_optional(STAmount{issue, std::uint64_t{1}})), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + gw); + tx.ter = temDISABLED; + env.submit(tx); + } + { + // sfAmount is MPT: both amendments active, expect temBAD_AMOUNT + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + auto tx = withNonCanonicalMPTAmount( + env.jt( + amm::ammClawback( + gw, + alice, + Asset{issue}, + Asset{xrpIssue()}, + std::make_optional(STAmount{issue, std::uint64_t{1}})), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + gw); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + + testcase("fixCleanup3_2_0 rejects non-canonical MPT VaultClawback amounts"); + { + Env env{*this, withFix}; + env.fund(XRP(100'000), alice, bob, gw); + env.close(); + auto const issue = makeIssue(env); + + auto const badAmount = badMPTAmount(issue, bad); + uint256 const fakeVaultId = keylet::vault(gw.id(), 1).key; + auto tx = withNonCanonicalMPTAmount( + env.jt( + Vault::clawback( + {.issuer = gw, + .id = fakeVaultId, + .holder = alice, + .amount = STAmount{issue, std::uint64_t{1}}}), + Fee(static_cast(env.current()->fees().increment.drops()))), + sfAmount, + badAmount, + gw); + tx.ter = temBAD_AMOUNT; + env.submit(tx); + } + } + } + void testTxJsonMetaFields(FeatureBitset features) { @@ -6947,7 +7812,7 @@ public: // Test MPT Amount is invalid in Tx, which don't support MPT testMPTInvalidInTx(all); - + testNonCanonicalMPTAmountCleanup(all); // Test parsed MPTokenIssuanceID in API response metadata testTxJsonMetaFields(all); diff --git a/src/test/app/OfferMPT_test.cpp b/src/test/app/OfferMPT_test.cpp index 3f88e57bc7..e9366f7c32 100644 --- a/src/test/app/OfferMPT_test.cpp +++ b/src/test/app/OfferMPT_test.cpp @@ -973,7 +973,7 @@ public: // Offers with negative amounts { - env(offer(alice, -usd(1'000), XRP(1'000)), Ter(temBAD_OFFER)); + env(offer(alice, -usd(1'000), XRP(1'000)), Ter(temBAD_AMOUNT)); env.require(Owners(alice, 1), offers(alice, 0)); } From 30de556224bdcdedb1732a8865fcce3b40ab2531 Mon Sep 17 00:00:00 2001 From: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> Date: Sat, 23 May 2026 15:48:48 +0100 Subject: [PATCH 02/10] fix: Address review feedback on FD/handle guarding (#5823 follow-up) (#7310) --- include/xrpl/server/detail/Door.h | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/include/xrpl/server/detail/Door.h b/include/xrpl/server/detail/Door.h index b72bc9bdea..79d36cae0c 100644 --- a/include/xrpl/server/detail/Door.h +++ b/include/xrpl/server/detail/Door.h @@ -23,7 +23,6 @@ #include #include -#include #endif #include @@ -99,7 +98,10 @@ private: static constexpr std::chrono::milliseconds kMaxAcceptDelay{2000}; std::chrono::milliseconds acceptDelay_{kInitialAcceptDelay}; boost::asio::steady_timer backoffTimer_; - static constexpr double kFreeFdThreshold = 0.70; + static constexpr std::uint64_t kMaxUsedFdPercent = 70; + static constexpr std::chrono::milliseconds kFdSampleInterval{250}; + clock_type::time_point fdSampleAt_; + bool cachedThrottle_{false}; struct FDStats { @@ -280,6 +282,7 @@ Door::Door( , acceptor_(ioContext) , strand_(boost::asio::make_strand(ioContext)) , backoffTimer_(ioContext) + , fdSampleAt_(clock_type::now() - kFdSampleInterval) { reOpen(); } @@ -338,11 +341,11 @@ Door::doAccept(boost::asio::yield_context doYield) { if (shouldThrottleForFds()) { + JLOG(j_.warn()) << "Throttling do_accept for " << acceptDelay_.count() << "ms."; backoffTimer_.expires_after(acceptDelay_); boost::system::error_code tec; backoffTimer_.async_wait(doYield[tec]); acceptDelay_ = std::min(acceptDelay_ * 2, kMaxAcceptDelay); - JLOG(j_.warn()) << "Throttling do_accept for " << acceptDelay_.count() << "ms."; continue; } @@ -359,8 +362,11 @@ Door::doAccept(boost::asio::yield_context doYield) if (ec == boost::asio::error::no_descriptors || ec == boost::asio::error::no_buffer_space) { - JLOG(j_.warn()) << "accept: Too many open files. Pausing for " - << acceptDelay_.count() << "ms."; + char const* const cause = (ec == boost::asio::error::no_descriptors) + ? "too many open files" + : "kernel buffer space exhausted"; + JLOG(j_.warn()) << "accept: " << cause << ". Pausing for " << acceptDelay_.count() + << "ms."; backoffTimer_.expires_after(acceptDelay_); boost::system::error_code tec; @@ -428,14 +434,15 @@ Door::shouldThrottleForFds() #if BOOST_OS_WINDOWS return false; #else - auto const stats = queryFdStats(); - if (!stats || stats->limit == 0) - return false; + auto const now = clock_type::now(); + if (now - fdSampleAt_ < kFdSampleInterval) + return cachedThrottle_; - auto const& s = *stats; - auto const free = (s.limit > s.used) ? (s.limit - s.used) : 0ull; - double const freeRatio = static_cast(free) / static_cast(s.limit); - return freeRatio < kFreeFdThreshold; + fdSampleAt_ = now; + auto const stats = queryFdStats(); + cachedThrottle_ = + stats && stats->limit > 0 && stats->used * 100 > stats->limit * kMaxUsedFdPercent; + return cachedThrottle_; #endif } From e34c2667d7b29cc38d83e8ec7190cd7769ec4e9f Mon Sep 17 00:00:00 2001 From: Peter Chen <34582813+PeterChen13579@users.noreply.github.com> Date: Sun, 24 May 2026 16:37:16 -0400 Subject: [PATCH 03/10] fix: Skip deleted book directories and non-root modifications in `ValidBookDirectory` invariant (#7312) --- .../tx/invariants/DirectoryInvariant.cpp | 18 ++++-- src/test/app/Invariants_test.cpp | 64 +++++++++++++++++++ 2 files changed, 77 insertions(+), 5 deletions(-) diff --git a/src/libxrpl/tx/invariants/DirectoryInvariant.cpp b/src/libxrpl/tx/invariants/DirectoryInvariant.cpp index 2a4fac07d0..1624a19830 100644 --- a/src/libxrpl/tx/invariants/DirectoryInvariant.cpp +++ b/src/libxrpl/tx/invariants/DirectoryInvariant.cpp @@ -40,19 +40,27 @@ badExchangeRate(SLE const& dir) void ValidBookDirectory::visitEntry( - bool, + bool isDelete, std::shared_ptr const& before, std::shared_ptr const& after) { // New root directories must have matching exchange-rate metadata. New - // child directories must point to an existing root. + // child directories, and modified directories that change sfRootIndex, must + // point to an existing root. - // Only validate newly-created directories; LedgerStateFix handles legacy - // bad exchange-rate metadata. - if (badBookDirectory_ || before || !after || after->getType() != ltDIR_NODE) + // Only validate newly-created directories and sfRootIndex changes; + // LedgerStateFix handles legacy bad exchange-rate metadata. Skip deletions + // because `after` is not guaranteed to be null. + if (badBookDirectory_ || isDelete || !after || after->getType() != ltDIR_NODE) return; auto const rootIndex = after->getFieldH256(sfRootIndex); + // Ignore ordinary modifications that do not change which root this + // directory belongs to. That tolerates legacy bad exchange-rate metadata + // during normal operation while still checking sfRootIndex changes. + if (before && before->getFieldH256(sfRootIndex) == rootIndex) + return; + if (after->key() == rootIndex && !badBookDirectory_) { badBookDirectory_ = badBookDirectory_ || badExchangeRate(*after); diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index cc16948553..c8a6e813de 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2137,6 +2137,70 @@ class Invariants_test : public beast::unit_test::Suite BEAST_EXPECT( invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); } + + // A bad root is rejected when added, ignored when a legacy bad root is + // modified without changing sfRootIndex or deleted, and checked when a + // modified directory changes sfRootIndex. + { + Env env{*this, defaultAmendments()}; + Account const a1{"A1"}; + env.fund(XRP(1000), a1); + env.close(); + + OpenView view{*env.current()}; + auto const directoryQuality = STAmount::kURateOne; + auto const rootDir = getBookRootKey(a1, directoryQuality); + auto const missingRootDir = getBookRootKey(a1, directoryQuality + 1); + auto const badRoot = makeRootPage(rootDir, directoryQuality + 1); + view.rawInsert(badRoot); + + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + + { + // add + ValidBookDirectory invariant; + invariant.visitEntry(false, nullptr, badRoot); + + BEAST_EXPECT( + !invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); + } + { + // modify (without changing the sfRootIndex) + ValidBookDirectory invariant; + invariant.visitEntry(false, badRoot, badRoot); + + BEAST_EXPECT( + invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); + } + { + // modify (changing sfRootIndex to a missing root) + auto const childBefore = makeChildPage(rootDir); + auto const childAfter = std::make_shared(*childBefore, childBefore->key()); + childAfter->setFieldH256(sfRootIndex, missingRootDir.key); + + ValidBookDirectory invariant; + invariant.visitEntry(false, childBefore, childAfter); + + test::StreamSink missingRootSink{beast::Severity::Warning}; + beast::Journal const missingRootJlog{missingRootSink}; + BEAST_EXPECT(!invariant.finalize( + makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, missingRootJlog)); + BEAST_EXPECT( + missingRootSink.messages().str().find("book directory root missing") != + std::string::npos); + } + { + // delete + view.rawErase(badRoot); + BEAST_EXPECT(!view.exists(rootDir)); + + ValidBookDirectory invariant; + invariant.visitEntry(true, badRoot, badRoot); + BEAST_EXPECT( + invariant.finalize(makeOfferCreateTx(), tesSUCCESS, XRPAmount{}, view, jlog)); + } + } } Keylet From a911f9089e7c2f67b564dd888715fbf71e1197fd Mon Sep 17 00:00:00 2001 From: Jingchen Date: Sun, 24 May 2026 21:44:29 +0100 Subject: [PATCH 04/10] fix: Use consistent scale for `debtTotal` (#7093) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- include/xrpl/ledger/helpers/LendingHelpers.h | 15 + .../lending/LoanBrokerCoverClawback.cpp | 29 +- .../lending/LoanBrokerCoverWithdraw.cpp | 6 + .../tx/transactors/lending/LoanPay.cpp | 30 +- .../tx/transactors/lending/LoanSet.cpp | 18 +- src/test/app/Loan_test.cpp | 409 ++++++++++++++++++ 6 files changed, 482 insertions(+), 25 deletions(-) diff --git a/include/xrpl/ledger/helpers/LendingHelpers.h b/include/xrpl/ledger/helpers/LendingHelpers.h index cce41a38c5..b7a1ed6bf2 100644 --- a/include/xrpl/ledger/helpers/LendingHelpers.h +++ b/include/xrpl/ledger/helpers/LendingHelpers.h @@ -203,6 +203,21 @@ getAssetsTotalScale(SLE::const_ref vaultSle) return scale(vaultSle->at(sfAssetsTotal), vaultSle->at(sfAsset)); } +// Compute the minimum required broker cover, rounded consistently. +// DebtTotal is a broker-level aggregate maintained at vault scale, so the +// rounding must also use vault scale — never an individual loan's scale. +inline Number +minimumBrokerCover(Number const& debtTotal, TenthBips32 coverRateMinimum, SLE::const_ref vaultSle) +{ + XRPL_ASSERT( + vaultSle && vaultSle->getType() == ltVAULT, "xrpl::minimumBrokerCover : valid Vault sle"); + NumberRoundModeGuard const mg(Number::RoundingMode::Upward); + return roundToAsset( + vaultSle->at(sfAsset), + tenthBipsOfValue(debtTotal, coverRateMinimum), + getAssetsTotalScale(vaultSle)); +} + TER checkLoanGuards( Asset const& vaultAsset, diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp index 48cb6b90aa..11095fdebe 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -160,15 +161,25 @@ Expected determineClawAmount( SLE const& sleBroker, Asset const& vaultAsset, - std::optional const& amount) + std::optional const& amount, + SLE::const_ref vaultSle, + Rules const& rules) { auto const maxClawAmount = [&]() { - // Always round the minimum required up - NumberRoundModeGuard const mg1(Number::RoundingMode::Upward); - auto const minRequiredCover = - tenthBipsOfValue(sleBroker[sfDebtTotal], TenthBips32(sleBroker[sfCoverRateMinimum])); + auto const minRequiredCover = [&]() { + if (rules.enabled(fixCleanup3_2_0)) + { + return minimumBrokerCover( + sleBroker[sfDebtTotal], TenthBips32(sleBroker[sfCoverRateMinimum]), vaultSle); + } + + // Always round the minimum required up + NumberRoundModeGuard const mg(Number::RoundingMode::Upward); + return tenthBipsOfValue( + sleBroker[sfDebtTotal], TenthBips32(sleBroker[sfCoverRateMinimum])); + }(); // The subtraction probably won't round, but round down if it does. - NumberRoundModeGuard const mg2(Number::RoundingMode::Downward); + NumberRoundModeGuard const mg(Number::RoundingMode::Downward); return sleBroker[sfCoverAvailable] - minRequiredCover; }(); if (maxClawAmount <= beast::kZero) @@ -283,7 +294,8 @@ LoanBrokerCoverClawback::preclaim(PreclaimContext const& ctx) } } - auto const findClawAmount = determineClawAmount(*sleBroker, vaultAsset, amount); + auto const findClawAmount = + determineClawAmount(*sleBroker, vaultAsset, amount, vault, ctx.view.rules()); if (!findClawAmount) { JLOG(ctx.j.warn()) << "LoanBroker cover is already at minimum."; @@ -345,7 +357,8 @@ LoanBrokerCoverClawback::doApply() auto const vaultAsset = vault->at(sfAsset); - auto const findClawAmount = determineClawAmount(*sleBroker, vaultAsset, amount); + auto const findClawAmount = + determineClawAmount(*sleBroker, vaultAsset, amount, vault, view().rules()); if (!findClawAmount) return tecINTERNAL; // LCOV_EXCL_LINE STAmount const& clawAmount = *findClawAmount; diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index dbe2de2100..f4c95541f6 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp @@ -142,6 +142,12 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) // Cover Rate is in 1/10 bips units auto const currentDebtTotal = sleBroker->at(sfDebtTotal); auto const minimumCover = [&]() { + if (ctx.view.rules().enabled(fixCleanup3_2_0)) + { + return minimumBrokerCover( + currentDebtTotal, TenthBips32{sleBroker->at(sfCoverRateMinimum)}, vault); + } + // Always round the minimum required up. // Applies to `tenthBipsOfValue` as well as `roundToAsset`. NumberRoundModeGuard const mg(Number::RoundingMode::Upward); diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index b8b940f3e3..65220573de 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -311,6 +311,8 @@ LoanPay::doApply() TenthBips32 const coverRateMinimum{brokerSle->at(sfCoverRateMinimum)}; auto debtTotalProxy = brokerSle->at(sfDebtTotal); + auto const vaultScale = getAssetsTotalScale(vaultSle); + // Send the broker fee to the owner if they have sufficient cover available, // _and_ if the owner can receive funds // _and_ if the broker is authorized to hold funds. If not, so as not to @@ -320,14 +322,22 @@ LoanPay::doApply() // Normally freeze status is checked in preclaim, but we do it here to // avoid duplicating the check. It'll claim a fee either way. bool const sendBrokerFeeToOwner = [&]() { - // Round the minimum required cover up to be conservative. This ensures - // CoverAvailable never drops below the theoretical minimum, protecting - // the broker's solvency. - NumberRoundModeGuard const mg(Number::RoundingMode::Upward); - return coverAvailableProxy >= - roundToAsset( - asset, tenthBipsOfValue(debtTotalProxy.value(), coverRateMinimum), loanScale) && - !isDeepFrozen(view, brokerOwner, asset) && + // In the fixCleanup3_2_0 path, vault-related values (for example, + // DebtTotal) use vaultScale. The legacy path below intentionally retains + // its pre-amendment loanScale behavior. + auto const minCover = [&]() { + if (view.rules().enabled(fixCleanup3_2_0)) + { + return minimumBrokerCover(debtTotalProxy.value(), coverRateMinimum, vaultSle); + } + // Round the minimum required cover up to be conservative. This ensures + // CoverAvailable never drops below the theoretical minimum, protecting + // the broker's solvency. + NumberRoundModeGuard const mg(Number::RoundingMode::Upward); + return roundToAsset( + asset, tenthBipsOfValue(debtTotalProxy.value(), coverRateMinimum), loanScale); + }(); + return coverAvailableProxy >= minCover && !isDeepFrozen(view, brokerOwner, asset) && !requireAuth(view, asset, brokerOwner, AuthType::StrongAuth); }(); @@ -423,10 +433,6 @@ LoanPay::doApply() auto assetsAvailableProxy = vaultSle->at(sfAssetsAvailable); auto assetsTotalProxy = vaultSle->at(sfAssetsTotal); - // The vault may be at a different scale than the loan. Reduce rounding - // errors during the payment by rounding some of the values to that scale. - auto const vaultScale = getAssetsTotalScale(vaultSle); - auto const totalPaidToVaultRaw = paymentParts->principalPaid + paymentParts->interestPaid; auto const totalPaidToVaultRounded = roundToAsset(asset, totalPaidToVaultRaw, vaultScale, Number::RoundingMode::Downward); diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index a3b71fd20d..561ab6b2aa 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -493,11 +493,19 @@ LoanSet::doApply() } TenthBips32 const coverRateMinimum{brokerSle->at(sfCoverRateMinimum)}; { - // Round the minimum required cover up to be conservative. This ensures - // CoverAvailable never drops below the theoretical minimum, protecting - // the broker's solvency. - NumberRoundModeGuard const mg(Number::RoundingMode::Upward); - if (brokerSle->at(sfCoverAvailable) < tenthBipsOfValue(newDebtTotal, coverRateMinimum)) + auto const minCover = [&]() { + if (ctx_.view().rules().enabled(fixCleanup3_2_0)) + { + return minimumBrokerCover(newDebtTotal, coverRateMinimum, vaultSle); + } + + // Round the minimum required cover up to be conservative. This ensures + // CoverAvailable never drops below the theoretical minimum, protecting + // the broker's solvency. + NumberRoundModeGuard const mg(Number::RoundingMode::Upward); + return tenthBipsOfValue(newDebtTotal, coverRateMinimum); + }(); + if (brokerSle->at(sfCoverAvailable) < minCover) { JLOG(j_.warn()) << "Insufficient first-loss capital to cover the loan."; return tecINSUFFICIENT_FUNDS; diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index b5687265ab..5e8e89cefa 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -7858,6 +7858,414 @@ protected: to_string(tolerance)); } + // Verify that LoanPay, LoanBrokerCoverWithdraw, and LoanSet all use the + // same vault-scale minimum cover when fixCleanup3_2_0 is enabled. + // Before the amendment, each transactor computed its minimum cover at a + // different precision (loanScale, debtScale, or the raw unrounded + // tenthBipsOfValue), which could lead to inconsistent decisions for the + // same broker state. After the amendment all three use + // minimumBrokerCover at vaultScale. + void + testMinimumBrokerCoverConsistency(FeatureBitset features) + { + using namespace jtx; + using namespace loan; + using namespace loanBroker; + + bool const withAmendment = features[fixCleanup3_2_0]; + + struct Ctx + { + jtx::Account issuer; + jtx::Account lender; + jtx::Account borrower; + jtx::PrettyAsset iou; + BrokerInfo broker; + BrokerParameters brokerParams; + }; + + // Shared setup, parametrized by vaultDeposit (the only varying setup + // field across the three scenarios). Each call runs in its own Env + // so multiple invocations within one scenario cannot interfere. + // The caller is responsible for invoking testcase(...) before the + // first runTest call of each scenario. + auto runTest = [&](Number vaultDeposit, auto&& body) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1'000'000'000), issuer, lender, borrower); + env.close(); + + // Enable clawback on the issuer *before* any trust lines exist + // (asfAllowTrustLineClawback requires an empty owner directory). + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const iou = issuer[iouCurrency_]; + env(trust(lender, iou(1'000'000'000))); + env(trust(borrower, iou(1'000'000'000))); + env.close(); + env(pay(issuer, lender, iou(100'000'000))); + env(pay(issuer, borrower, iou(100'000'000))); + env.close(); + + // 13.37% — non-round rate produces a messier minimum. + BrokerParameters const brokerParams{ + .vaultDeposit = vaultDeposit, + .debtMax = 0, + .coverRateMin = TenthBips32{13'370}, + .coverDeposit = 5'000, + .managementFeeRate = TenthBips16{500}}; + + BrokerInfo const broker = createVaultAndBroker(env, iou, lender, brokerParams); + + body( + env, + Ctx{.issuer = issuer, + .lender = lender, + .borrower = borrower, + .iou = iou, + .broker = broker, + .brokerParams = brokerParams}); + }; + + // Scenario 1 — LoanPay + // + // Verify that LoanPay's minimum cover check uses vault scale (not + // loan scale). Before the amendment, different loans could produce + // different fee routing decisions for the same broker-level state. + // Small vault deposit => vaultScale = -12. + testcase("LoanPay minimum cover scale consistency"); + { + struct LoanKeylets + { + Keylet tiny; + Keylet big; + }; + + // Create the tiny + big loans and reduce cover via clawback so + // that subsequent LoanPay calls hit the minimum-cover boundary. + // Used by the two pay-and-check sub-tests below so each can run + // in its own Env. + auto setupLoansAndClawback = [&](Env& env, Ctx const& c) -> std::optional { + Asset const asset{c.iou}; + + // Create the TINY loan first (while vaultScale is still + // small). principal 0.01, 0% interest, 1 payment => + // loanScale = vaultScale. + auto const brokerSle1 = env.le(keylet::loanbroker(c.broker.brokerID)); + if (!BEAST_EXPECT(brokerSle1)) + return std::nullopt; + auto const tinyLoanSeq = brokerSle1->at(sfLoanSequence); + auto const tinyLoanKeylet = keylet::loan(c.broker.brokerID, tinyLoanSeq); + + env(set(c.borrower, c.broker.brokerID, Number{1, -2}), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{0}), + kPaymentTotal(1), + kPaymentInterval(86400 * 365), + Fee(XRP(10))); + env.close(); + + // Create the BIG loan second. 100% annual interest over 20 + // payments pushes totalValueOutstanding high enough that + // loanScale > vaultScale. + auto const brokerSle2 = env.le(keylet::loanbroker(c.broker.brokerID)); + if (!BEAST_EXPECT(brokerSle2)) + return std::nullopt; + auto const bigLoanSeq = brokerSle2->at(sfLoanSequence); + auto const bigLoanKeylet = keylet::loan(c.broker.brokerID, bigLoanSeq); + + env(set(c.borrower, c.broker.brokerID, Number{500}), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{100'000}), + kPaymentTotal(20), + kPaymentInterval(86400 * 365), + Fee(XRP(10))); + env.close(); + + // The tiny loan's scale is frozen at the vault's pre-big-loan + // scale, so it is strictly smaller than the big loan's. + // After the big loan is created the vault absorbs its value, + // pushing vaultScale up to match bigLoanScale. + auto const tinyLoanSle = env.le(tinyLoanKeylet); + auto const bigLoanSle = env.le(bigLoanKeylet); + auto const vaultSle = env.le(keylet::vault(c.broker.vaultID)); + if (!BEAST_EXPECT(tinyLoanSle) || !BEAST_EXPECT(bigLoanSle) || + !BEAST_EXPECT(vaultSle)) + return std::nullopt; + if (!BEAST_EXPECT(tinyLoanSle->at(sfLoanScale) == -12) || + !BEAST_EXPECT(bigLoanSle->at(sfLoanScale) == -11) || + !BEAST_EXPECT(getAssetsTotalScale(vaultSle) == -11)) + return std::nullopt; + + // Use issuer clawback to reduce cover to the minimum the + // clawback transactor allows. Compute the amount as + // initialCover - expectedCoverAfter so we exercise the exact + // clawback rather than relying on the transactor to clip + // down. + // + // Before the amendment the clawback minimum is the + // *unrounded* tenthBipsOfValue — strictly less than the + // rounded-at-vaultScale minimum LoanPay uses for the big + // loan. After the amendment both clawback and LoanPay use + // the same rounded minimum (via minimumBrokerCover), so + // cover lands exactly at that threshold. + Number const expectedCoverAfter = withAmendment ? Number{1330651855688460000, -15} + : Number{1330651855688458000, -15}; + Number const clawbackAmount = + Number{c.brokerParams.coverDeposit} - expectedCoverAfter; + + env(coverClawback(c.issuer), + kLoanBrokerId(c.broker.brokerID), + kAmount(STAmount{asset, clawbackAmount})); + env.close(); + + auto const brokerSle = env.le(keylet::loanbroker(c.broker.brokerID)); + if (!BEAST_EXPECT(brokerSle) || + !BEAST_EXPECT(brokerSle->at(sfCoverAvailable) == expectedCoverAfter)) + return std::nullopt; + + return LoanKeylets{.tiny = tinyLoanKeylet, .big = bigLoanKeylet}; + }; + + // Pay one loan and report whether the fee went to the broker's + // pseudo account (the fallback when cover < minimum) rather + // than to the owner. + auto feeGoesToPseudo = [&](Env& env, Ctx const& c, Keylet const& loanKeylet) -> bool { + Asset const asset{c.iou}; + auto const brokerSle = env.le(keylet::loanbroker(c.broker.brokerID)); + if (!BEAST_EXPECT(brokerSle)) + return false; + auto const pseudoAcct = Account("pseudo", brokerSle->at(sfAccount)); + auto const pseudoBefore = env.balance(pseudoAcct, c.iou); + + auto const payLoan = env.le(loanKeylet); + if (!BEAST_EXPECT(payLoan)) + return false; + auto const periodicPayment = payLoan->at(sfPeriodicPayment); + auto const serviceFee = payLoan->at(sfLoanServiceFee); + std::int32_t const loanScale = payLoan->at(sfLoanScale); + + auto const payment = roundPeriodicPayment(asset, periodicPayment, loanScale); + auto const payAmt = STAmount{asset, payment + serviceFee}; + + env(loan::pay(c.borrower, loanKeylet.key, payAmt), Fee(XRP(10))); + env.close(); + + auto const pseudoAfter = env.balance(pseudoAcct, c.iou); + return pseudoAfter.number() > pseudoBefore.number(); + }; + + // Pay the BIG loan in its own Env so its outcome cannot affect + // the TINY-loan check. With the fix, LoanPay and clawback use + // the same vaultScale minimum (cover == minAtVaultScale => + // fee to owner). Without the fix, LoanPay uses bigLoanScale=-11, + // rounds up to a larger minimum than what clawback used => + // cover < min => fee to pseudo. + runTest(/*vaultDeposit=*/1'000, [&](Env& env, Ctx const& c) { + auto const loans = setupLoansAndClawback(env, c); + if (!loans) + return; + BEAST_EXPECT(feeGoesToPseudo(env, c, loans->big) == !withAmendment); + }); + + // Pay the TINY loan in its own Env. Fee goes to the owner + // either way: + // - With the fix: LoanPay uses vaultScale=-11 (same as + // clawback) => owner. + // - Without the fix: LoanPay uses tinyLoanScale=-12, rounds + // up at -12 (a no-op) => min == cover => owner. + runTest(/*vaultDeposit=*/1'000, [&](Env& env, Ctx const& c) { + auto const loans = setupLoansAndClawback(env, c); + if (!loans) + return; + BEAST_EXPECT(!feeGoesToPseudo(env, c, loans->tiny)); + }); + } + + // Scenario 2 — LoanBrokerCoverWithdraw + // + // Verify that CoverWithdraw's minimum cover check uses vault scale + // (not scale(debtTotal, asset)). Before the amendment, CoverWithdraw + // used: + // roundToAsset(asset, tenthBipsOfValue(debt, rate), scale(debt, asset)) + // which could disagree with LoanPay's minimum (which used loanScale). + // + // Use a large vault deposit so that vaultScale (from AssetsTotal) is + // strictly larger than debtScale (from DebtTotal). With + // vaultDeposit = 100,000: after the big loan + // AssetsTotal ≈ 109,500 → vaultScale = -10 + // DebtTotal ≈ 10,000 → debtScale = -11 + // The one-order-of-magnitude gap makes roundToAsset at -10 truncate + // more aggressively than at -11, exposing the bug. + testcase("CoverWithdraw minimum cover scale consistency"); + runTest( + /*vaultDeposit=*/100'000, [&](Env& env, Ctx const& c) { + Asset const asset{c.iou}; + + // Create only the big loan to push DebtTotal up to ~10,000 + // while AssetsTotal stays around 109,500 (dominated by the + // large vault deposit). + env(set(c.borrower, c.broker.brokerID, Number{500}), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{100'000}), + kPaymentTotal(20), + kPaymentInterval(86400 * 365), + Fee(XRP(10))); + env.close(); + + // Read broker state and compute both old and new minimums. + auto const brokerSle = env.le(keylet::loanbroker(c.broker.brokerID)); + auto const vaultSle = env.le(keylet::vault(c.broker.vaultID)); + if (!BEAST_EXPECT(brokerSle) || !BEAST_EXPECT(vaultSle)) + return; + + auto const coverAvail = brokerSle->at(sfCoverAvailable); + auto const debtTotal = brokerSle->at(sfDebtTotal); + auto const vaultScale = getAssetsTotalScale(vaultSle); + auto const debtScale = scale(debtTotal, asset); + + // Sanity: debt scale differs from vault scale for this setup. + BEAST_EXPECT(debtScale < vaultScale); + + auto const oldMin = [&]() { + NumberRoundModeGuard const mg(Number::RoundingMode::Upward); + return roundToAsset( + asset, + tenthBipsOfValue(debtTotal, TenthBips32{c.brokerParams.coverRateMin}), + debtScale); + }(); + auto const newMin = minimumBrokerCover( + debtTotal, TenthBips32{c.brokerParams.coverRateMin}, vaultSle); + + // The new (vaultScale) minimum must be strictly larger than + // the old (debtScale) minimum — that is the gap the amendment + // closes. + Number const expectedNewMin{1330650518688500000, -15}; + Number const expectedOldMin{1330650518688472000, -15}; + BEAST_EXPECT(newMin == expectedNewMin); + BEAST_EXPECT(oldMin == expectedOldMin); + + // Try to withdraw so that remaining cover lands between the + // two minimums: oldMin < target < newMin. + auto const target = oldMin + (newMin - oldMin) / 2; + auto const withdrawAmount = STAmount{asset, coverAvail - target}; + + if (withAmendment) + { + // CoverWithdraw now uses vaultScale: target < newMin + // => FAILS. + env(coverWithdraw(c.lender, c.broker.brokerID, withdrawAmount), + Ter(tecINSUFFICIENT_FUNDS)); + } + else + { + // Old CoverWithdraw uses debtScale: target > oldMin + // => SUCCEEDS. + env(coverWithdraw(c.lender, c.broker.brokerID, withdrawAmount)); + } + env.close(); + }); + + // Scenario 3 — LoanSet + // + // Verify that LoanSet's minimum cover check uses vault scale (not the + // raw unrounded tenthBipsOfValue). Before the amendment, LoanSet + // used tenthBipsOfValue(newDebtTotal, coverRateMinimum) (no + // roundToAsset), while clawback/withdraw used different formulas. + // After the amendment all use minimumBrokerCover at vaultScale, and + // rounding at a coarser scale can absorb a tiny debt increase — + // allowing a loan that would otherwise be rejected. + testcase("LoanSet minimum cover scale consistency"); + runTest( + /*vaultDeposit=*/1'000, [&](Env& env, Ctx const& c) { + // Create the tiny loan (scale -12) AND the big loan (scale + // -11). Both loans are needed so that DebtTotal has a full + // 16-digit mantissa — a "messy" value where roundToAsset at + // vaultScale actually truncates digits and produces a + // different result from the raw tenthBipsOfValue. With only + // the big loan, DebtTotal has ~4 significant digits and + // rounding at scale -11 is a no-op, masking the amendment's + // effect. + env(set(c.borrower, c.broker.brokerID, Number{1, -2}), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{0}), + kPaymentTotal(1), + kPaymentInterval(86400 * 365), + Fee(XRP(10))); + env.close(); + + env(set(c.borrower, c.broker.brokerID, Number{500}), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{100'000}), + kPaymentTotal(20), + kPaymentInterval(86400 * 365), + Fee(XRP(10))); + env.close(); + + // Clawback to reduce cover to the clawback transactor's + // minimum. Pass the exact amount rather than relying on the + // transactor to clip down; the setup matches Scenario 1 so + // the same residual-cover values apply. + Number const expectedCoverAfter = withAmendment ? Number{1330651855688460000, -15} + : Number{1330651855688458000, -15}; + Number const clawbackAmount = + Number{c.brokerParams.coverDeposit} - expectedCoverAfter; + env(coverClawback(c.issuer), + kLoanBrokerId(c.broker.brokerID), + kAmount(c.iou(clawbackAmount))); + env.close(); + + // Verify scales. + auto const vaultSle = env.le(keylet::vault(c.broker.vaultID)); + if (!BEAST_EXPECT(vaultSle)) + return; + auto const vaultScale = getAssetsTotalScale(vaultSle); + BEAST_EXPECT(vaultScale == -11); + + // Now try to create a tiny additional loan. Principal is + // 1e-11 (the smallest value that survives the precision + // check at loanScale = vaultScale = -11), with 0% interest + // and 1 payment. + // + // The tiny debt increase adds ~1.337e-12 to the unrounded + // minimum. + // - Without the amendment: the old LoanSet formula rounds + // up during tenthBipsOfValue (16-digit Number + // normalisation), pushing the minimum past the cover left + // by clawback => tecINSUFFICIENT_FUNDS. + // - With the amendment: minimumBrokerCover rounds at + // vaultScale=-11, which absorbs the tiny increase — the + // rounded minimum stays the same => tesSUCCESS. + auto const tinyPrincipal = Number{1, -11}; + + if (withAmendment) + { + env(set(c.borrower, c.broker.brokerID, tinyPrincipal), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{0}), + kPaymentTotal(1), + kPaymentInterval(86400 * 365), + Fee(XRP(10))); + } + else + { + env(set(c.borrower, c.broker.brokerID, tinyPrincipal), + Sig(sfCounterpartySignature, c.lender), + kInterestRate(TenthBips32{0}), + kPaymentTotal(1), + kPaymentInterval(86400 * 365), + Fee(XRP(10)), + Ter(tecINSUFFICIENT_FUNDS)); + } + env.close(); + }); + } + void runAmendmentIndependent() { @@ -7919,6 +8327,7 @@ protected: testOverpaymentManagementFee(features); testIssuerIsBorrower(features); testIntegerScalePrincipalSticks(features); + testMinimumBrokerCoverConsistency(features); // RIPD regressions testRIPD3831(features); From e9d885bd9b8d7b6f0fb0fbcd3db7ff39df34e68b Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 26 May 2026 14:50:18 +0100 Subject: [PATCH 05/10] fix: Fix clang-tidy pre-commit hook to locate compile_commands.json from repo root (#7325) Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> --- .github/scripts/levelization/generate.py | 0 bin/pre-commit/clang_tidy_check.py | 8 +++++++- 2 files changed, 7 insertions(+), 1 deletion(-) mode change 100644 => 100755 .github/scripts/levelization/generate.py diff --git a/.github/scripts/levelization/generate.py b/.github/scripts/levelization/generate.py old mode 100644 new mode 100755 diff --git a/bin/pre-commit/clang_tidy_check.py b/bin/pre-commit/clang_tidy_check.py index 7fb51d1c46..f134660671 100755 --- a/bin/pre-commit/clang_tidy_check.py +++ b/bin/pre-commit/clang_tidy_check.py @@ -168,7 +168,13 @@ def main(): if not os.environ.get("TIDY"): return 0 - repo_root = Path(__file__).parent.parent + repo_root = Path( + subprocess.check_output( + ["git", "rev-parse", "--show-toplevel"], + cwd=Path(__file__).parent, + text=True, + ).strip() + ) files = staged_files(repo_root) if not files: return 0 From 22a21b175efbf95a3435360ba5d6a85ffbfcbcd2 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 26 May 2026 16:01:52 +0200 Subject: [PATCH 06/10] fix: Include management-fee delta in doOverpayment assertion (#7039) --- src/libxrpl/ledger/helpers/LendingHelpers.cpp | 117 ++++++++++------- src/test/app/Loan_test.cpp | 118 ++++++++++++++---- 2 files changed, 168 insertions(+), 67 deletions(-) diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 1fedbb5f13..9cda5905c9 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -769,9 +769,6 @@ doOverpayment( "xrpl::detail::doOverpayment", "principal change agrees"); - // I'm not 100% sure the following asserts are correct. If in doubt, and - // everything else works, remove any that cause trouble. - JLOG(j.debug()) << "valueChange: " << loanPaymentParts.valueChange << ", totalValue before: " << *totalValueOutstandingProxy << ", totalValue after: " << newRoundedLoanState.valueOutstanding @@ -783,11 +780,28 @@ doOverpayment( << overpaymentComponents.trackedPrincipalDelta - (totalValueOutstandingProxy - newRoundedLoanState.valueOutstanding); + // The valueChange returned by tryOverpayment satisfies + // valueChange = (newInterestDue - oldInterestDue) + untrackedInterest. + // Using the loan-state identity v = p + i + m and the adjacent + // `principal change agrees` assertion (dp = oldP - newP), this + // rearranges into three independently-computable terms: + // + // 1. TVO change beyond what principal repayment alone explains: + // newTVO - (oldTVO - dp) + // 2. Management fee released by re-amortization (positive when + // mfee decreased; zero when managementFeeRate == 0): + // oldMfee - newMfee + // 3. The overpayment's penalty interest part (= untrackedInterest + // for the overpayment path; see computeOverpaymentComponents): + // trackedInterestPart() + [[maybe_unused]] Number const tvoChange = newRoundedLoanState.valueOutstanding - + (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta); + [[maybe_unused]] Number const managementFeeReleased = + managementFeeOutstandingProxy - newRoundedLoanState.managementFeeDue; + [[maybe_unused]] Number const interestPart = overpaymentComponents.trackedInterestPart(); + XRPL_ASSERT_PARTS( - loanPaymentParts.valueChange == - newRoundedLoanState.valueOutstanding - - (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta) + - overpaymentComponents.trackedInterestPart(), + loanPaymentParts.valueChange == tvoChange + managementFeeReleased + interestPart, "xrpl::detail::doOverpayment", "interest paid agrees"); @@ -2027,51 +2041,62 @@ loanMakePayment( // It shouldn't be possible for the overpayment to be greater than // totalValueOutstanding, because that would have been processed as // another normal payment. But cap it just in case. - Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy); + Number const overpaymentRaw = + std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy); - detail::ExtendedPaymentComponents const overpaymentComponents = - detail::computeOverpaymentComponents( - asset, - loanScale, - overpayment, - overpaymentInterestRate, - overpaymentFeeRate, - managementFeeRate); + bool const fixEnabled = view.rules().enabled(fixCleanup3_2_0); + Number const overpayment = fixEnabled + ? roundToAsset(asset, overpaymentRaw, loanScale, Number::RoundingMode::Downward) + : overpaymentRaw; - // Don't process an overpayment if the whole amount (or more!) - // gets eaten by fees and interest. - if (overpaymentComponents.trackedPrincipalDelta > 0) + // Post-amendment, the rounded overpayment can be zero; pre-amendment + // it's always positive given the surrounding guards. + if (!fixEnabled || overpayment > 0) { - XRPL_ASSERT_PARTS( - overpaymentComponents.untrackedInterest >= beast::kZero, - "xrpl::loanMakePayment", - "overpayment penalty did not reduce value of loan"); - // Can't just use `periodicPayment` here, because it might - // change - auto periodicPaymentProxy = loan->at(sfPeriodicPayment); - if (auto const overResult = detail::doOverpayment( - view.rules(), + detail::ExtendedPaymentComponents const overpaymentComponents = + detail::computeOverpaymentComponents( asset, loanScale, - overpaymentComponents, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - periodicPaymentProxy, - periodicRate, - paymentRemainingProxy, - managementFeeRate, - j)) + overpayment, + overpaymentInterestRate, + overpaymentFeeRate, + managementFeeRate); + + // Don't process an overpayment if the whole amount (or more!) + // gets eaten by fees and interest. + if (overpaymentComponents.trackedPrincipalDelta > 0) { - totalParts += *overResult; - } - else if (overResult.error()) - { - // error() will be the TER returned if a payment is not - // made. It will only evaluate to true if it's unsuccessful. - // Otherwise, tesSUCCESS means nothing was done, so - // continue. - return Unexpected(overResult.error()); + XRPL_ASSERT_PARTS( + overpaymentComponents.untrackedInterest >= beast::kZero, + "xrpl::loanMakePayment", + "overpayment penalty did not reduce value of loan"); + // Can't just use `periodicPayment` here, because it might + // change + auto periodicPaymentProxy = loan->at(sfPeriodicPayment); + if (auto const overResult = detail::doOverpayment( + view.rules(), + asset, + loanScale, + overpaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + periodicPaymentProxy, + periodicRate, + paymentRemainingProxy, + managementFeeRate, + j)) + { + totalParts += *overResult; + } + else if (overResult.error()) + { + // error() will be the TER returned if a payment is not + // made. It will only evaluate to true if it's unsuccessful. + // Otherwise, tesSUCCESS means nothing was done, so + // continue. + return Unexpected(overResult.error()); + } } } } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 5e8e89cefa..c380655563 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -169,6 +169,10 @@ protected: TenthBips32 coverRateLiquidation = percentageToTenthBips(25); std::string data = {}; // NOLINT(readability-redundant-member-init) std::uint32_t flags = 0; + // If set, the vault is created with this sfScale value. Useful for + // tests that need finer loanScale to exercise rounding edge cases. + std::optional vaultScale = + std::nullopt; // NOLINT(readability-redundant-member-init) [[nodiscard]] Number maxCoveredLoanValue(Number const& currentDebt) const @@ -522,6 +526,8 @@ protected: auto const coverRateMinValue = params.coverRateMin; auto [tx, vaultKeylet] = vault.create({.owner = lender, .asset = asset}); + if (params.vaultScale) + tx[sfScale] = *params.vaultScale; env(tx); env.close(); BEAST_EXPECT(env.le(vaultKeylet)); @@ -2157,21 +2163,23 @@ protected: // If the loan does not allow overpayments, send a payment that // tries to make an overpayment. Do not include `txFlags`, so we // don't end up duplicating the next test transaction. - env(pay(borrower, - loanKeylet.key, - STAmount{broker.asset, state.periodicPayment * Number{15, -1}}, - tfLoanOverpayment), - Fee(XRPAmount{baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}), - Ter(tecNO_PERMISSION)); + // + // fixCleanup3_1_3 gates tfLoanOverpayment as a valid flag: + // with fix on → preflight passes, apply returns tecNO_PERMISSION; + // with fix off → preflight rejects the flag, returns temINVALID_FLAG. + bool const hasFix313 = env.current()->rules().enabled(fixCleanup3_1_3); + STAmount const overpayAmount{broker.asset, state.periodicPayment * Number{15, -1}}; + XRPAmount const overpayFee{ + baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}; + env(pay(borrower, loanKeylet.key, overpayAmount, tfLoanOverpayment), + Fee(overpayFee), + Ter(hasFix313 ? TER{tecNO_PERMISSION} : TER{temINVALID_FLAG})); + if (hasFix313) { env.disableFeature(fixCleanup3_1_3); - env(pay(borrower, - loanKeylet.key, - STAmount{broker.asset, state.periodicPayment * Number{15, -1}}, - tfLoanOverpayment), - Fee(XRPAmount{ - baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}), + env(pay(borrower, loanKeylet.key, overpayAmount, tfLoanOverpayment), + Fee(overpayFee), Ter(temINVALID_FLAG)); env.enableFeature(fixCleanup3_1_3); } @@ -7027,7 +7035,7 @@ protected: auto credType = "credential1"; - pdomain::Credentials const credentials1{{.issuer = issuer, .credType = credType}}; + pdomain::Credentials const credentials1 = {{.issuer = issuer, .credType = credType}}; env(pdomain::setTx(issuer, credentials1)); env.close(); @@ -7572,6 +7580,74 @@ protected: attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS); } + // An overpayment whose residual amount has more precision than loanScale + // fires the isRounded(asset, overpayment, loanScale) assertion in + // computeOverpaymentComponents (and a downstream "interest paid agrees" + // assertion in doOverpayment). fixCleanup3_2_0 rounds the residual down + // to loanScale before passing it in. The pre-amendment path can't be + // tested here because the assertion fires in Debug builds and aborts + // the test process — see the PR description for context. + void + testBugOverpayUnroundedAmount() + { + testcase("bug: computeOverpaymentComponents isRounded assertion"); + + using namespace jtx; + using namespace loan; + Env env(*this, all_); + + Account const issuer{"issuer"}; + Account const lender{"vaultOwner"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1'000'000), issuer, lender, borrower); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const iouAsset = issuer["USD"]; + STAmount const iouLimit{iouAsset.raw(), Number{9'999'999'999'999'999LL}}; + env(trust(lender, iouLimit)); + env(trust(borrower, iouLimit)); + env(pay(issuer, lender, iouAsset(1'000'000))); + env(pay(issuer, borrower, iouAsset(1'000'000))); + env.close(); + + auto const broker = createVaultAndBroker( + env, + iouAsset, + lender, + {.vaultDeposit = 100'000, + .debtMax = 5000, + .managementFeeRate = TenthBips16{1000}, + .vaultScale = 1}); + + auto const sleBroker = env.le(broker.brokerKeylet()); + if (!BEAST_EXPECT(sleBroker)) + return; + auto const loanSequence = sleBroker->at(sfLoanSequence); + auto const loanKeylet = keylet::loan(broker.brokerID, loanSequence); + + using namespace loan; + env(set(borrower, broker.brokerID, Number{1000}, tfLoanOverpayment), + Sig(sfCounterpartySignature, lender), + kInterestRate(TenthBips32{10000}), + kPaymentTotal(12), + kPaymentInterval(60), + kGracePeriod(60), + kOverpaymentFee(TenthBips32{1000}), + kOverpaymentInterestRate(TenthBips32{1000}), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + // periodic * 1.5 at 15-sig-digit precision: 125.000154585042. This + // has too many digits to round cleanly to loanScale=-10, so the + // overpayment residual fails the isRounded check. + STAmount const payAmount{iouAsset.raw(), Number{125'000'154'585'042LL, -12}}; + env(pay(borrower, loanKeylet.key, payAmount), Txflags(tfLoanOverpayment), Ter(tesSUCCESS)); + env.close(); + } + // Regression for the dual-rounding fix at coarse (integer-MPT) scale. // // Loan: P=1, r=50% (50000 tenth-bips), n=3, yearly interval. The @@ -8280,6 +8356,9 @@ protected: testRIPD3901(); testBorrowerIsBroker(); testLimitExceeded(); + testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared(); + testLendingCanTradeClearedNoImpact(); + testBugOverpayUnroundedAmount(); for (auto const flags : {0u, tfLoanOverpayment}) testYieldTheftRounding(flags); @@ -8295,11 +8374,11 @@ protected: testLoanPayLateFullPaymentBypassesPenalties(features); testLoanCoverMinimumRoundingExploit(features); #endif - // Lifecycle - testSelfLoan(features); - testLoanSet(features); testLifecycle(features); + testLoanSet(features); + testDosLoanPay(features); + testSelfLoan(features); // Payment paths testWithdrawReflectsUnrealizedLoss(features); @@ -8346,11 +8425,8 @@ public: run() override { runAmendmentIndependent(); - testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared(); - testLendingCanTradeClearedNoImpact(); - testDosLoanPay(all_ | fixCleanup3_1_3); - testDosLoanPay(all_ - fixCleanup3_1_3); - for (auto const& features : amendmentCombinations({fixCleanup3_2_0, featureMPTokensV2})) + for (auto const& features : + amendmentCombinations({fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2})) runAmendmentSensitive(features); } }; From 49cb3f45a42d1f050212e1653a663789fb592d2a Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 26 May 2026 16:45:33 +0100 Subject: [PATCH 07/10] ci: Add clang to nix images (#7308) Co-authored-by: semgrep-companion-app[bot] <218312740+semgrep-companion-app[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .github/workflows/build-nix-image.yml | 114 ++++++++++-------- .../workflows/reusable-build-docker-image.yml | 89 ++++++++++++++ cspell.config.yaml | 3 + docker/check-sanitizers.sh | 42 +++++++ docker/cpp_files/asan.cpp | 28 +++++ docker/cpp_files/tsan.cpp | 26 ++++ docker/cpp_files/ubsan.cpp | 13 ++ docker/nix.Dockerfile | 29 +++++ flake.lock | 4 +- flake.nix | 6 +- nix/ci-env.nix | 108 +++++++++++++---- nix/packages.nix | 1 + nix/utils.nix | 8 +- 13 files changed, 387 insertions(+), 84 deletions(-) create mode 100644 .github/workflows/reusable-build-docker-image.yml create mode 100755 docker/check-sanitizers.sh create mode 100644 docker/cpp_files/asan.cpp create mode 100644 docker/cpp_files/tsan.cpp create mode 100644 docker/cpp_files/ubsan.cpp diff --git a/.github/workflows/build-nix-image.yml b/.github/workflows/build-nix-image.yml index 6554cf6c08..a8fb6eec86 100644 --- a/.github/workflows/build-nix-image.yml +++ b/.github/workflows/build-nix-image.yml @@ -6,14 +6,16 @@ on: - develop paths: - ".github/workflows/build-nix-image.yml" - - "docker/nix.Dockerfile" + - ".github/workflows/reusable-build-docker-image.yml" + - "docker/**" - "flake.nix" - "flake.lock" - "nix/**" pull_request: paths: - ".github/workflows/build-nix-image.yml" - - "docker/nix.Dockerfile" + - ".github/workflows/reusable-build-docker-image.yml" + - "docker/**" - "flake.nix" - "flake.lock" - "nix/**" @@ -27,75 +29,81 @@ defaults: run: shell: bash -env: - UBUNTU_VERSION: "20.04" - RHEL_VERSION: "9" - DEBIAN_VERSION: "bookworm" - jobs: build: - name: Build and push Nix image (${{ matrix.distro }}) + name: Build ${{ matrix.distro.name }} (${{ matrix.target.platform }}) + permissions: + contents: read + packages: write + strategy: + fail-fast: false + matrix: + # The base images are the oldest supported version of each distro + # that we want to build images for. + distro: + - name: nixos + base_image: nixos/nix:latest + - name: ubuntu + base_image: ubuntu:20.04 + - name: rhel + base_image: registry.access.redhat.com/ubi9/ubi:latest + - name: debian + base_image: debian:bookworm + target: + - platform: linux/amd64 + runner: ubuntu-latest + - platform: linux/arm64 + runner: ubuntu-24.04-arm + uses: ./.github/workflows/reusable-build-docker-image.yml + with: + image_name: ghcr.io/xrplf/xrpld/nix-${{ matrix.distro.name }} + dockerfile: docker/nix.Dockerfile + base_image: ${{ matrix.distro.base_image }} + platform: ${{ matrix.target.platform }} + runner: ${{ matrix.target.runner }} + push: ${{ github.event_name == 'push' }} + + merge: + name: Merge ${{ matrix.distro }} manifest + needs: build + if: github.event_name == 'push' runs-on: ubuntu-latest permissions: contents: read packages: write strategy: + fail-fast: false matrix: - include: - - distro: nixos - - distro: ubuntu - - distro: rhel - - distro: debian + distro: [nixos, ubuntu, rhel, debian] + env: + IMAGE_NAME: ghcr.io/xrplf/xrpld/nix-${{ matrix.distro }} steps: - - name: Checkout repository - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - - name: Determine base image - id: vars - run: | - case "${{ matrix.distro }}" in - nixos) - echo "base_image=nixos/nix:latest" >> $GITHUB_OUTPUT - ;; - ubuntu) - echo "base_image=ubuntu:${UBUNTU_VERSION}" >> $GITHUB_OUTPUT - ;; - rhel) - echo "base_image=registry.access.redhat.com/ubi${RHEL_VERSION}/ubi:latest" >> $GITHUB_OUTPUT - ;; - debian) - echo "base_image=debian:${DEBIAN_VERSION}" >> $GITHUB_OUTPUT - ;; - esac - - name: Set up Docker Buildx uses: docker/setup-buildx-action@4d04d5d9486b7bd6fa91e7baf45bbb4f8b9deedd # v4.0.0 + - name: Docker metadata + id: meta + uses: docker/metadata-action@030e881283bb7a6894de51c315a6bfe6a94e05cf # v6.0.0 + with: + images: ${{ env.IMAGE_NAME }} + tags: | + type=sha,prefix=sha-,format=short + type=raw,value=latest + - name: Login to GitHub Container Registry - if: github.event_name == 'push' uses: docker/login-action@4907a6ddec9925e35a0a9e82d7399ccc52663121 # v4.1.0 with: registry: ghcr.io username: ${{ github.repository_owner }} password: ${{ secrets.GITHUB_TOKEN }} - - name: Docker metadata - id: meta - uses: docker/metadata-action@030e881283bb7a6894de51c315a6bfe6a94e05cf # v6.0.0 - with: - images: ghcr.io/xrplf/ci/nix-${{ matrix.distro }} - tags: | - type=sha,prefix=sha-,format=short - type=raw,value=latest + - name: Create multi-arch manifests + run: | + for tag in $(jq -cr '.tags[]' <<< "$DOCKER_METADATA_OUTPUT_JSON"); do + docker buildx imagetools create -t "$tag" "${tag}-amd64" "${tag}-arm64" + done - - name: Build and push - uses: docker/build-push-action@bcafcacb16a39f128d818304e6c9c0c18556b85f # v7.1.0 - with: - context: . - file: docker/nix.Dockerfile - platforms: linux/amd64 - push: ${{ github.event_name == 'push' }} - tags: ${{ steps.meta.outputs.tags }} - labels: ${{ steps.meta.outputs.labels }} - build-args: BASE_IMAGE=${{ steps.vars.outputs.base_image }} + - name: Inspect image + run: | + docker buildx imagetools inspect "${IMAGE_NAME}:${{ steps.meta.outputs.version }}" diff --git a/.github/workflows/reusable-build-docker-image.yml b/.github/workflows/reusable-build-docker-image.yml new file mode 100644 index 0000000000..e631e02368 --- /dev/null +++ b/.github/workflows/reusable-build-docker-image.yml @@ -0,0 +1,89 @@ +# Build a single-platform Docker image. On push, the image is pushed to +# GHCR with arch-suffixed tags (e.g. `:latest-amd64`, `:sha-abc-amd64`) +# so the calling workflow can stitch per-arch builds into a multi-arch +# manifest without needing to pass digests around. +name: Reusable build Docker image (single platform) + +on: + workflow_call: + inputs: + image_name: + description: "Full image name without tag (e.g. 'ghcr.io/xrplf/xrpld/nix-ubuntu')" + required: true + type: string + dockerfile: + description: "Path to the Dockerfile, relative to the repository root" + required: true + type: string + base_image: + description: "Value passed to the Dockerfile as the BASE_IMAGE build arg" + required: true + type: string + platform: + description: "Docker platform string, e.g. linux/amd64" + required: true + type: string + runner: + description: "GitHub Actions runner label to build on" + required: true + type: string + push: + description: "Whether to push the image to GHCR" + required: true + type: boolean + +defaults: + run: + shell: bash + +jobs: + build: + name: Build (${{ inputs.platform }}) + runs-on: ${{ inputs.runner }} + permissions: + contents: read + packages: write + + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Determine arch + id: vars + env: + PLATFORM: ${{ inputs.platform }} + run: | + echo "arch=${PLATFORM##*/}" >> $GITHUB_OUTPUT + + - name: Set up Docker Buildx + uses: docker/setup-buildx-action@4d04d5d9486b7bd6fa91e7baf45bbb4f8b9deedd # v4.0.0 + + - name: Login to GitHub Container Registry + if: inputs.push + uses: docker/login-action@4907a6ddec9925e35a0a9e82d7399ccc52663121 # v4.1.0 + with: + registry: ghcr.io + username: ${{ github.repository_owner }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Docker metadata + id: meta + uses: docker/metadata-action@030e881283bb7a6894de51c315a6bfe6a94e05cf # v6.0.0 + with: + images: ${{ inputs.image_name }} + tags: | + type=sha,prefix=sha-,format=short + type=raw,value=latest + flavor: | + suffix=-${{ steps.vars.outputs.arch }},onlatest=true + + - name: Build and push + uses: docker/build-push-action@bcafcacb16a39f128d818304e6c9c0c18556b85f # v7.1.0 + with: + context: . + file: ${{ inputs.dockerfile }} + platforms: ${{ inputs.platform }} + push: ${{ inputs.push }} + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + build-args: BASE_IMAGE=${{ inputs.base_image }} diff --git a/cspell.config.yaml b/cspell.config.yaml index 275df41f58..82a6a20158 100644 --- a/cspell.config.yaml +++ b/cspell.config.yaml @@ -199,11 +199,13 @@ words: - nonxrp - noreplace - noripple + - nostdinc - notifempty - nudb - nullptr - nunl - Nyffenegger + - onlatest - ostr - pargs - partitioner @@ -298,6 +300,7 @@ words: - unauthorizing - unergonomic - unfetched + - unfindable - unflatten - unfund - unimpair diff --git a/docker/check-sanitizers.sh b/docker/check-sanitizers.sh new file mode 100755 index 0000000000..db51f11851 --- /dev/null +++ b/docker/check-sanitizers.sh @@ -0,0 +1,42 @@ +#!/bin/bash + +# Sanity-check that the sanitizer runtimes shipped with g++/clang++ work +# end-to-end against the system loader: compile each example with both +# compilers, run it, and confirm the expected diagnostic is emitted. + +set -eo pipefail + +cpp_files_dir="${1:?usage: $0 }" + +case "$(uname -m)" in + x86_64) loader=/lib64/ld-linux-x86-64.so.2 ;; + aarch64) loader=/lib/ld-linux-aarch64.so.1 ;; + *) echo "Unsupported arch: $(uname -m)" >&2; exit 1 ;; +esac + +declare -A sanitize=( + [asan]="-fsanitize=address" + [tsan]="-fsanitize=thread" + [ubsan]="-fsanitize=undefined" +) +declare -A expect=( + [asan]="heap-use-after-free" + [tsan]="data race" + [ubsan]="signed integer overflow" +) + +for compiler in g++ clang++; do + for name in asan tsan ubsan; do + bin="/tmp/${name}-${compiler}" + echo "=== Build ${name} with ${compiler} ===" + "$compiler" -std=c++20 -O1 -g ${sanitize[$name]} \ + -Wl,--dynamic-linker=$loader \ + "${cpp_files_dir}/${name}.cpp" -o "$bin" + echo "=== Run ${name}-${compiler} ===" + output=$("$bin" 2>&1) || true + echo "$output" + echo "$output" | grep -q "${expect[$name]}" \ + || { echo "expected '${expect[$name]}' from $bin"; exit 1; } + rm -f "$bin" + done +done diff --git a/docker/cpp_files/asan.cpp b/docker/cpp_files/asan.cpp new file mode 100644 index 0000000000..8347f58d37 --- /dev/null +++ b/docker/cpp_files/asan.cpp @@ -0,0 +1,28 @@ +#include +#include +#include + +#if defined(__clang__) || defined(__GNUC__) +__attribute__((noinline)) +#elif defined(_MSC_VER) +__declspec(noinline) +#endif +int +read_after_free(volatile int* array, std::size_t index) +{ + std::atomic_signal_fence(std::memory_order_seq_cst); + int value = array[index]; + std::atomic_signal_fence(std::memory_order_seq_cst); + return value; +} + +int +main() +{ + int* array = new int[5]{10, 20, 30, 40, 50}; + delete[] array; + + std::cout << "Value at index 2: " << read_after_free(array, 2) << std::endl; + + return 0; +} diff --git a/docker/cpp_files/tsan.cpp b/docker/cpp_files/tsan.cpp new file mode 100644 index 0000000000..34b0990a6d --- /dev/null +++ b/docker/cpp_files/tsan.cpp @@ -0,0 +1,26 @@ +#include +#include + +static int kCounter = 0; + +void +increment() +{ + for (int i = 0; i < 100'000; ++i) + { + ++kCounter; + } +} + +int +main() +{ + std::thread t1(increment); + std::thread t2(increment); + + t1.join(); + t2.join(); + + std::cout << "Final counter value: " << kCounter << std::endl; + return 0; +} diff --git a/docker/cpp_files/ubsan.cpp b/docker/cpp_files/ubsan.cpp new file mode 100644 index 0000000000..db86119070 --- /dev/null +++ b/docker/cpp_files/ubsan.cpp @@ -0,0 +1,13 @@ +#include +#include + +int +main() +{ + int maxInt = std::numeric_limits::max(); + int volatile one = 1; + std::cout << "Current max: " << maxInt << std::endl; + int overflowed = maxInt + one; + std::cout << "Overflowed result: " << overflowed << std::endl; + return 0; +} diff --git a/docker/nix.Dockerfile b/docker/nix.Dockerfile index 52faa8b8dc..690f0b76bd 100644 --- a/docker/nix.Dockerfile +++ b/docker/nix.Dockerfile @@ -45,8 +45,30 @@ COPY --from=builder /tmp/build/result /nix/ci-env ENV PATH="/nix/ci-env/bin:$PATH" +# Externally-built dynamically-linked ELF binaries hard-code the loader path +# (e.g. /lib64/ld-linux-x86-64.so.2) in their PT_INTERP header. Copy the +# loader from the Nix store to that path when the base image doesn't already +# provide one (i.e. on nixos/nix). +RUN <&2; exit 1 ;; +esac +if [ ! -e "$target" ]; then + # Use the loader from the same glibc that gcc links libc against, so + # ld-linux and libc/libpthread share GLIBC_PRIVATE symbols at runtime. + src="$(dirname "$(gcc -print-file-name=libc.so.6)")/$(basename "$target")" + [ -e "$src" ] || { echo "ld-linux not found at $src" >&2; exit 1; } + mkdir -p "$(dirname "$target")" + cp "$src" "$target" +fi +EOF + RUN </dev/null && /tmp/check-sanitizers.sh /tmp/cpp_files || true diff --git a/flake.lock b/flake.lock index 5ec053975d..3149f3feed 100644 --- a/flake.lock +++ b/flake.lock @@ -15,7 +15,7 @@ "type": "indirect" } }, - "nixpkgs-glibc231": { + "nixpkgs-custom-glibc": { "flake": false, "locked": { "lastModified": 1593520194, @@ -35,7 +35,7 @@ "root": { "inputs": { "nixpkgs": "nixpkgs", - "nixpkgs-glibc231": "nixpkgs-glibc231" + "nixpkgs-custom-glibc": "nixpkgs-custom-glibc" } } }, diff --git a/flake.nix b/flake.nix index 18671bdf31..3b3ec7ea08 100644 --- a/flake.nix +++ b/flake.nix @@ -6,16 +6,16 @@ # version — matches the system libc on Ubuntu 20.04 LTS. Imported # manually (flake = false) because this revision predates nixpkgs' # own flake.nix. - nixpkgs-glibc231 = { + nixpkgs-custom-glibc = { url = "github:NixOS/nixpkgs/9cd98386a38891d1074fc18036b842dc4416f562"; flake = false; }; }; outputs = - { nixpkgs, nixpkgs-glibc231, ... }: + { nixpkgs, nixpkgs-custom-glibc, ... }: let - forEachSystem = import ./nix/utils.nix { inherit nixpkgs nixpkgs-glibc231; }; + forEachSystem = import ./nix/utils.nix { inherit nixpkgs nixpkgs-custom-glibc; }; in { devShells = forEachSystem (import ./nix/devshell.nix); diff --git a/nix/ci-env.nix b/nix/ci-env.nix index d8021fe0bd..0d617913d9 100644 --- a/nix/ci-env.nix +++ b/nix/ci-env.nix @@ -1,39 +1,102 @@ { pkgs, - glibc231, + customGlibc, ... }: let inherit (import ./packages.nix { inherit pkgs; }) commonPackages; + inherit (pkgs) lib; - # binutils wrapped to emit binaries that reference glibc 2.31 (dynamic - # linker path, library search path, RPATH). - binutils231 = pkgs.wrapBintoolsWith { + # Underlying compiler toolchains to wrap. Bump these in one place to + # roll the whole environment forward. + customGccPackage = pkgs.gcc15; + customLlvmPackages = pkgs.llvmPackages_22; + customClangMajor = lib.versions.major (lib.getVersion customLlvmPackages.clang-unwrapped); + + # binutils wrapped to emit binaries that reference the custom glibc + # (dynamic linker path, library search path, RPATH). + customBinutils = pkgs.wrapBintoolsWith { bintools = pkgs.binutils-unwrapped; - libc = glibc231; + libc = customGlibc; }; - # Rebuild gcc 15 (specifically libstdc++ / libgcc_s) against glibc 2.31. - # The override swaps gcc15.cc's bootstrap stdenv for one that uses the - # existing gcc 15 binary but links against glibc 2.31, so the resulting - # compiler ships runtime libraries that only reference symbols available - # in glibc 2.31. - gcc15CcWithGlibc231 = pkgs.gcc15.cc.override { + # Rebuild gcc (specifically libstdc++ / libgcc_s) against the custom + # glibc. The override swaps gcc.cc's bootstrap stdenv for one that uses + # the existing gcc binary but links against the custom glibc, so the + # resulting compiler ships runtime libraries that only reference symbols + # available in that glibc. + customGccCc = customGccPackage.cc.override { stdenv = pkgs.stdenvAdapters.overrideCC pkgs.stdenv ( pkgs.wrapCCWith { - cc = pkgs.gcc15.cc; - libc = glibc231; - bintools = binutils231; + cc = customGccPackage.cc; + libc = customGlibc; + bintools = customBinutils; } ); }; - # cc-wrapper around the rebuilt compiler, pointing at glibc 2.31 headers - # and libraries. This is what we actually expose to users. - gcc15WithGlibc231 = pkgs.wrapCCWith { - cc = gcc15CcWithGlibc231; - libc = glibc231; - bintools = binutils231; + # cc-wrapper around the rebuilt compiler, pointing at the custom glibc + # headers and libraries. This is what we actually expose to users. + customGcc = pkgs.wrapCCWith { + cc = customGccCc; + libc = customGlibc; + bintools = customBinutils; + }; + + # stdenv built around the rebuilt gcc / custom glibc. Used to rebuild + # compiler-rt below so its sanitizer runtimes see the custom glibc + # headers. + customStdenv = pkgs.stdenvAdapters.overrideCC pkgs.stdenv customGcc; + + # Rebuild compiler-rt against the custom glibc so the sanitizer runtimes + # don't use glibc symbols (or sysconf constants like _SC_SIGSTKSZ) that + # only exist in newer glibc versions. scudo is dropped because its CMake + # includes CheckAtomic with -nostdinc++ in CMAKE_REQUIRED_FLAGS, which + # makes std::atomic unfindable in our stdenv; we don't use scudo (only + # asan/ubsan/tsan etc.). + customCompilerRt = + (customLlvmPackages.compiler-rt.override { + stdenv = customStdenv; + }).overrideAttrs + (old: { + postPatch = (old.postPatch or "") + '' + substituteInPlace lib/CMakeLists.txt \ + --replace-quiet 'add_subdirectory(scudo/standalone)' \ + '# scudo/standalone disabled in xrpld ci-env' + ''; + }); + + # cc-wrapper around clang, pointing at the custom glibc headers and + # libraries. Reuses the rebuilt gcc for libstdc++ / libgcc_s so that + # C++ binaries produced by clang also only reference symbols available + # in the custom glibc. compiler-rt is wired into a resource-root so + # sanitizer runtimes (libclang_rt.*.a) are found at link time; this + # mirrors what nixpkgs does internally when building llvmPackages.clang. + customClang = pkgs.wrapCCWith { + cc = customLlvmPackages.clang-unwrapped; + libc = customGlibc; + bintools = customBinutils; + gccForLibs = customGccCc; + extraPackages = [ customCompilerRt ]; + extraBuildCommands = '' + rsrc="$out/resource-root" + mkdir "$rsrc" + ln -s "${customLlvmPackages.clang-unwrapped.lib}/lib/clang/${customClangMajor}/include" "$rsrc/include" + ln -s "${customCompilerRt.out}/lib" "$rsrc/lib" + ln -s "${customCompilerRt.out}/share" "$rsrc/share" || true + echo "-resource-dir=$rsrc" >> $out/nix-support/cc-cflags + ''; + }; + + # Strip the generic cc/c++/cpp symlinks from the clang wrapper so it can + # coexist with the gcc wrapper in buildEnv. gcc remains the default + # compiler (cc/c++/cpp); clang is invoked explicitly as clang/clang++. + customClangForCiEnv = pkgs.symlinkJoin { + name = "clang-wrapper-custom-for-ci-env"; + paths = [ customClang ]; + postBuild = '' + rm -f $out/bin/cc $out/bin/c++ $out/bin/cpp + ''; }; in @@ -41,8 +104,9 @@ in default = pkgs.buildEnv { name = "xrpld-ci-env"; paths = commonPackages ++ [ - gcc15WithGlibc231 - binutils231 + customGcc + customClangForCiEnv + customBinutils ]; pathsToLink = [ "/bin" diff --git a/nix/packages.nix b/nix/packages.nix index edfe302ec9..d209620a68 100644 --- a/nix/packages.nix +++ b/nix/packages.nix @@ -17,6 +17,7 @@ in llvmPackages_22.clang-tools mold ninja + patchelf perl # needed for openssl pkg-config pre-commit diff --git a/nix/utils.nix b/nix/utils.nix index 07ff169c44..d83e612c16 100644 --- a/nix/utils.nix +++ b/nix/utils.nix @@ -1,4 +1,4 @@ -{ nixpkgs, nixpkgs-glibc231 }: +{ nixpkgs, nixpkgs-custom-glibc }: function: nixpkgs.lib.genAttrs [ @@ -12,10 +12,10 @@ nixpkgs.lib.genAttrs function { pkgs = import nixpkgs { inherit system; }; # glibc 2.31 — matches the system libc on Ubuntu 20.04 LTS. Sourced - # from the nixpkgs snapshot pinned via the `nixpkgs-glibc231` flake - # input, so the build uses the compiler from that snapshot + # from the nixpkgs snapshot pinned via the `nixpkgs-custom-glibc` + # flake input, so the build uses the compiler from that snapshot # (gcc 9.3.0) along with the matching patches, configure flags, and # hardening defaults. - glibc231 = (import nixpkgs-glibc231 { inherit system; }).glibc; + customGlibc = (import nixpkgs-custom-glibc { inherit system; }).glibc; } ) From 633ef4706ffdd0bed445032d2de92e86e59985d6 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 26 May 2026 18:32:44 +0200 Subject: [PATCH 08/10] fix: Fix `VaultInvariant` and `VaultDeposit` precision bugs at IOU scale boundaries (#7272) Co-authored-by: Bart --- include/xrpl/protocol/STAmount.h | 1 - include/xrpl/tx/invariants/VaultInvariant.h | 77 ++- src/libxrpl/tx/invariants/VaultInvariant.cpp | 345 ++++++------ .../tx/transactors/vault/VaultDeposit.cpp | 87 +++- src/test/app/Vault_test.cpp | 489 ++++++++++++++++++ src/test/protocol/STAmount_test.cpp | 2 - 6 files changed, 820 insertions(+), 181 deletions(-) diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index bf3e25eedb..a4fffad40c 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -189,7 +189,6 @@ public: /** * Checks if this amount evaluates to zero when constrained to a specific * accounting scale. - * * For XRP and MPT `roundToScale` is a no-op, returns true only when the amount itself is zero. * The `scale` argument is ignored in that case. * For IOU, the amount is rounded to the given scale using Number::RoundingMode::ToNearest mode diff --git a/include/xrpl/tx/invariants/VaultInvariant.h b/include/xrpl/tx/invariants/VaultInvariant.h index ab55cd086a..abc256c880 100644 --- a/include/xrpl/tx/invariants/VaultInvariant.h +++ b/include/xrpl/tx/invariants/VaultInvariant.h @@ -4,9 +4,11 @@ #include #include #include +#include #include #include #include +#include #include #include @@ -79,16 +81,83 @@ private: std::vector beforeMPTs_; std::unordered_map deltas_; + /** + * @brief Compute the minimum STAmount scale for rounding invariant + * calculations. + * + * Post-amendment (@c fixCleanup3_2_0) this is simply the posterior + * @c assetsTotal scale. Pre-amendment it is the coarsest scale across + * @p vaultDelta and both asset-field deltas. + * + * @param vaultDelta Delta of the vault's asset balance for this transaction. + * @param rules Active ledger rules (used to check the amendment). + * @returns The minimum scale to apply when rounding vault-related amounts. + */ + [[nodiscard]] std::int32_t + computeVaultMinScale(DeltaInfo const& vaultDelta, Rules const& rules) const; + + /** + * @brief Return the vault-asset balance-change delta for an account. + * + * Looks up the ledger-entry delta recorded during @c visitEntry for the + * account entry (XRP), trust line (IOU), or MPToken (MPT) that corresponds + * to the vault asset held by @p id. + * + * @param id Account whose asset delta is requested. + * @returns The delta, or @c std::nullopt if the entry was not touched. + */ + [[nodiscard]] std::optional + deltaAssets(AccountID const& id) const; + + /** + * @brief Return the vault-asset delta for the transaction's sending + * account, adjusted for the fee. + * + * Calls @c deltaAssets for @c tx[sfAccount] and, for non-delegated XRP + * transactions, adds the consumed fee back so the invariant sees the net + * asset movement rather than the fee-reduced balance change. + * + * @param tx The transaction being applied. + * @param fee Fee charged by this transaction. + * @returns The fee-adjusted delta, or @c std::nullopt if the net delta is + * zero or the account entry was not touched. + */ + [[nodiscard]] std::optional + deltaAssetsTxAccount(STTx const& tx, XRPAmount fee) const; + + /** + * @brief Return the vault-share balance-change delta for an account. + * + * For the vault's pseudo-account the @c MPTokenIssuance outstanding-amount + * delta is returned; for all other accounts the @c MPToken delta is + * returned. + * + * @param id Account whose share delta is requested. + * @returns The delta, or @c std::nullopt if the entry was not touched. + */ + [[nodiscard]] std::optional + deltaShares(AccountID const& id) const; + + /** + * @brief Check whether a vault holds no assets. + * + * @param vault Snapshot of the vault to test. + * @returns @c true when both @c assetsAvailable and @c assetsTotal are + * zero. + */ + [[nodiscard]] static bool + isVaultEmpty(Vault const& vault); + public: + // Compute the coarsest scale required to represent all numbers + [[nodiscard]] static std::int32_t + computeCoarsestScale(std::vector const& numbers); + void visitEntry(bool, std::shared_ptr const&, std::shared_ptr const&); bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&); - - // Compute the coarsest scale required to represent all numbers - [[nodiscard]] static std::int32_t - computeCoarsestScale(std::vector const& numbers); }; } // namespace xrpl diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index 2947be3bd4..4aa79279a1 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -186,6 +186,101 @@ ValidVault::visitEntry( } } +std::optional +ValidVault::deltaAssets(AccountID const& id) const +{ + auto const& vaultAsset = afterVault_[0].asset; + auto const lookup = [&](uint256 const& key) -> std::optional { + auto const it = deltas_.find(key); + if (it == deltas_.end()) + return std::nullopt; + return it->second; + }; + + return std::visit( + [&](TIss const& issue) -> std::optional { + if constexpr (std::is_same_v) + { + if (isXRP(issue)) + return lookup(keylet::account(id).key); + auto result = lookup(keylet::line(id, issue).key); + // Trust-line balance is stored from the low-account's perspective; + // negate if id is the high account so the delta is in id's terms. + if (result && id > issue.getIssuer()) + result->delta = -result->delta; + return result; + } + else if constexpr (std::is_same_v) + { + return lookup(keylet::mptoken(issue.getMptID(), id).key); + } + }, + vaultAsset.value()); +} + +std::optional +ValidVault::deltaAssetsTxAccount(STTx const& tx, XRPAmount fee) const +{ + auto const& vaultAsset = afterVault_[0].asset; + auto ret = deltaAssets(tx[sfAccount]); + if (!ret.has_value() || !vaultAsset.native()) + return ret; + + if (auto const delegate = tx[~sfDelegate]; delegate.has_value() && *delegate != tx[sfAccount]) + return ret; + + ret->delta += fee.drops(); + if (ret->delta == kZero) + return std::nullopt; + + return ret; +} + +std::optional +ValidVault::deltaShares(AccountID const& id) const +{ + auto const& afterVault = afterVault_[0]; + auto const it = [&]() { + if (id == afterVault.pseudoId) + return deltas_.find(keylet::mptIssuance(afterVault.shareMPTID).key); + return deltas_.find(keylet::mptoken(afterVault.shareMPTID, id).key); + }(); + + return it != deltas_.end() ? std::optional(it->second) : std::nullopt; +} + +bool +ValidVault::isVaultEmpty(Vault const& vault) +{ + return vault.assetsAvailable == 0 && vault.assetsTotal == 0; +} + +std::int32_t +ValidVault::computeVaultMinScale(DeltaInfo const& vaultDelta, Rules const& rules) const +{ + // Returns the posterior `assetsTotal` scale. + // + // 1. Because STAmounts are normalized, `assetsTotal` (being >= `assetsAvailable`) + // safely represents the coarsest exponent needed for both fields. + // + // 2. The scale may decrease (withdraw/clawback) or increase (deposit). In both cases + // we ensure the vault is in a legitimate state in the post-transaction scale. + auto const& afterVault = afterVault_[0]; + auto const& vaultAsset = afterVault.asset; + if (rules.enabled(fixCleanup3_2_0)) + { + NumberRoundModeGuard const roundGuard(Number::RoundingMode::ToNearest); + return scale(afterVault.assetsTotal, vaultAsset); + } + + auto const& beforeVault = beforeVault_[0]; + auto const totalDelta = + DeltaInfo::makeDelta(beforeVault.assetsTotal, afterVault.assetsTotal, vaultAsset); + auto const availableDelta = + DeltaInfo::makeDelta(beforeVault.assetsAvailable, afterVault.assetsAvailable, vaultAsset); + return computeCoarsestScale({vaultDelta, totalDelta, availableDelta}); +} + bool ValidVault::finalize( STTx const& tx, @@ -445,61 +540,6 @@ ValidVault::finalize( } auto const& vaultAsset = afterVault.asset; - auto const deltaAssets = [&](AccountID const& id) -> std::optional { - auto const get = // - [&](auto const& it, std::int8_t sign = 1) -> std::optional { - if (it == deltas_.end()) - return std::nullopt; - - return DeltaInfo{it->second.delta * sign, it->second.scale}; - }; - - return std::visit( - [&](TIss const& issue) { - if constexpr (std::is_same_v) - { - if (isXRP(issue)) - return get(deltas_.find(keylet::account(id).key)); - return get( - deltas_.find(keylet::line(id, issue).key), id > issue.getIssuer() ? -1 : 1); - } - else if constexpr (std::is_same_v) - { - return get(deltas_.find(keylet::mptoken(issue.getMptID(), id).key)); - } - }, - vaultAsset.value()); - }; - auto const deltaAssetsTxAccount = [&]() -> std::optional { - auto ret = deltaAssets(tx[sfAccount]); - // Nothing returned or not XRP transaction - if (!ret.has_value() || !vaultAsset.native()) - return ret; - - // Delegated transaction; no need to compensate for fees - if (auto const delegate = tx[~sfDelegate]; - delegate.has_value() && *delegate != tx[sfAccount]) - return ret; - - ret->delta += fee.drops(); - if (ret->delta == kZero) - return std::nullopt; - - return ret; - }; - auto const deltaShares = [&](AccountID const& id) -> std::optional { - auto const it = [&]() { - if (id == afterVault.pseudoId) - return deltas_.find(keylet::mptIssuance(afterVault.shareMPTID).key); - return deltas_.find(keylet::mptoken(afterVault.shareMPTID, id).key); - }(); - - return it != deltas_.end() ? std::optional(it->second) : std::nullopt; - }; - - auto const vaultHoldsNoAssets = [&](Vault const& vault) { - return vault.assetsAvailable == 0 && vault.assetsTotal == 0; - }; // Technically this does not need to be a lambda, but it's more // convenient thanks to early "return false"; the not-so-nice @@ -629,16 +669,8 @@ ValidVault::finalize( return false; // That's all we can do } - // Get the coarsest scale to round calculations to - auto const totalDelta = DeltaInfo::makeDelta( - beforeVault.assetsTotal, afterVault.assetsTotal, vaultAsset); - auto const availableDelta = DeltaInfo::makeDelta( - beforeVault.assetsAvailable, afterVault.assetsAvailable, vaultAsset); - auto const minScale = computeCoarsestScale({ - *maybeVaultDeltaAssets, - totalDelta, - availableDelta, - }); + // Get the posterior scale to round calculations to + auto const minScale = computeVaultMinScale(*maybeVaultDeltaAssets, view.rules()); auto const vaultDeltaAssets = roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); @@ -669,12 +701,11 @@ ValidVault::finalize( if (!issuerDeposit) { - auto const maybeAccDeltaAssets = deltaAssetsTxAccount(); + auto const maybeAccDeltaAssets = deltaAssetsTxAccount(tx, fee); if (!maybeAccDeltaAssets) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must change depositor " - "balance"; + JLOG(j.fatal()) + << "Invariant failed: deposit must change depositor balance"; return false; } auto const localMinScale = @@ -685,19 +716,20 @@ ValidVault::finalize( auto const localVaultDeltaAssets = roundToAsset(vaultAsset, vaultDeltaAssets, localMinScale); + // For IOUs, if the deposit amount is not-representable at depositor trustline + // scale deposit amount could round to zero, giving depositor shares for no + // assets. Unlike withdrawal, we do not allow that. if (accountDeltaAssets >= kZero) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must decrease depositor " - "balance"; + JLOG(j.fatal()) + << "Invariant failed: deposit must decrease depositor balance"; result = false; } if (localVaultDeltaAssets * -1 != accountDeltaAssets) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must change vault and " - "depositor balance by equal amount"; + JLOG(j.fatal()) << "Invariant failed: " << // + "deposit must change vault and depositor balance by equal amount"; result = false; } } @@ -705,45 +737,38 @@ ValidVault::finalize( if (afterVault.assetsMaximum > kZero && afterVault.assetsTotal > afterVault.assetsMaximum) { - JLOG(j.fatal()) << // - "Invariant failed: deposit assets outstanding must not " - "exceed assets maximum"; + JLOG(j.fatal()) << "Invariant failed: " << // + "deposit assets outstanding must not exceed assets maximum"; result = false; } auto const maybeAccDeltaShares = deltaShares(tx[sfAccount]); if (!maybeAccDeltaShares) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must change depositor " - "shares"; + JLOG(j.fatal()) << "Invariant failed: deposit must change depositor shares"; return false; // That's all we can do } - // We don't need to round shares, they are integral MPT + // We don't round shares, they are integral MPT auto const& accountDeltaShares = *maybeAccDeltaShares; if (accountDeltaShares.delta <= kZero) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must increase depositor " - "shares"; + JLOG(j.fatal()) << "Invariant failed: deposit must increase depositor shares"; result = false; } auto const maybeVaultDeltaShares = deltaShares(afterVault.pseudoId); if (!maybeVaultDeltaShares || maybeVaultDeltaShares->delta == kZero) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must change vault shares"; + JLOG(j.fatal()) << "Invariant failed: deposit must change vault shares"; return false; // That's all we can do } - // We don't need to round shares, they are integral MPT + // We don't round shares, they are integral MPT auto const& vaultDeltaShares = *maybeVaultDeltaShares; if (vaultDeltaShares.delta * -1 != accountDeltaShares.delta) { - JLOG(j.fatal()) << // - "Invariant failed: deposit must change depositor and " - "vault shares by equal amount"; + JLOG(j.fatal()) << "Invariant failed: " << // + "deposit must change depositor and vault shares by equal amount"; result = false; } @@ -751,8 +776,8 @@ ValidVault::finalize( vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); if (assetTotalDelta != vaultDeltaAssets) { - JLOG(j.fatal()) << "Invariant failed: deposit and assets " - "outstanding must add up"; + JLOG(j.fatal()) + << "Invariant failed: deposit and assets outstanding must add up"; result = false; } @@ -760,8 +785,7 @@ ValidVault::finalize( vaultAsset, afterVault.assetsAvailable - beforeVault.assetsAvailable, minScale); if (assetAvailableDelta != vaultDeltaAssets) { - JLOG(j.fatal()) << "Invariant failed: deposit and assets " - "available must add up"; + JLOG(j.fatal()) << "Invariant failed: deposit and assets available must add up"; result = false; } @@ -772,34 +796,25 @@ ValidVault::finalize( XRPL_ASSERT( !beforeVault_.empty(), - "xrpl::ValidVault::finalize : withdrawal updated a " - "vault"); + "xrpl::ValidVault::finalize : withdrawal updated a vault"); auto const& beforeVault = beforeVault_[0]; auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); - if (!maybeVaultDeltaAssets) { - JLOG(j.fatal()) << "Invariant failed: withdrawal must " - "change vault balance"; + JLOG(j.fatal()) << "Invariant failed: withdrawal must change vault balance"; return false; // That's all we can do } - // Get the most coarse scale to round calculations to - auto const totalDelta = DeltaInfo::makeDelta( - beforeVault.assetsTotal, afterVault.assetsTotal, vaultAsset); - auto const availableDelta = DeltaInfo::makeDelta( - beforeVault.assetsAvailable, afterVault.assetsAvailable, vaultAsset); - auto const minScale = - computeCoarsestScale({*maybeVaultDeltaAssets, totalDelta, availableDelta}); + // Get the posterior scale to round calculations to + auto const minScale = computeVaultMinScale(*maybeVaultDeltaAssets, view.rules()); auto const vaultPseudoDeltaAssets = roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); if (vaultPseudoDeltaAssets >= kZero) { - JLOG(j.fatal()) << "Invariant failed: withdrawal must " - "decrease vault balance"; + JLOG(j.fatal()) << "Invariant failed: withdrawal must decrease vault balance"; result = false; } @@ -814,7 +829,7 @@ ValidVault::finalize( if (!issuerWithdrawal) { - auto const maybeAccDelta = deltaAssetsTxAccount(); + auto const maybeAccDelta = deltaAssetsTxAccount(tx, fee); auto const maybeOtherAccDelta = [&]() -> std::optional { if (auto const destination = tx[~sfDestination]; destination && *destination != tx[sfAccount]) @@ -825,8 +840,7 @@ ValidVault::finalize( if (maybeAccDelta.has_value() == maybeOtherAccDelta.has_value()) { JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change one " - "destination balance"; + "Invariant failed: withdrawal must change one destination balance"; return false; } @@ -835,63 +849,83 @@ ValidVault::finalize( // the scale of destinationDelta can be coarser than // minScale, so we take that into account when rounding - auto const localMinScale = - std::max(minScale, computeCoarsestScale({destinationDelta})); + auto const destinationScale = computeCoarsestScale({destinationDelta}); + auto const localMinScale = std::max(minScale, destinationScale); auto const roundedDestinationDelta = roundToAsset(vaultAsset, destinationDelta.delta, localMinScale); - if (roundedDestinationDelta <= kZero) + // Post-fixCleanup3_2_0: Tolerate zero-rounded destination deltas for IOUs only. + // If the receiver's trust line sits at a coarser scale, the inflow may + // safely round down to zero. + // + // XRP and MPT remain strict. Because they are integer-exact, a zero + // destination delta indicates a true accounting bug, not a rounding artifact. + bool const tolerateZeroDelta = + view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); + auto const invalidBalanceChange = tolerateZeroDelta + ? roundedDestinationDelta < kZero + : roundedDestinationDelta <= kZero; + if (invalidBalanceChange) { JLOG(j.fatal()) << // - "Invariant failed: withdrawal must increase " - "destination balance"; + "Invariant failed: withdrawal must increase destination balance"; result = false; } auto const localPseudoDeltaAssets = roundToAsset(vaultAsset, vaultPseudoDeltaAssets, localMinScale); - if (localPseudoDeltaAssets * -1 != roundedDestinationDelta) + // For IOU assets near a precision boundary the destination's STAmount + // exponent can shift, making part of the sent value unrepresentable at the + // receiver's new scale — that portion is irreversibly absorbed by the IOU + // rail. Tolerate the mismatch only when the destroyed amount (vault outflow + // minus destination inflow, in Number space) is itself sub-ULP at the + // destination's scale. Floor rounding is used so that values exactly at the + // step boundary are not mistakenly dismissed. Any representable discrepancy + // indicates a real accounting bug and must be caught. + auto const destroyedIsSubUlp = tolerateZeroDelta && + roundToAsset( + vaultAsset, + maybeVaultDeltaAssets->delta * -1 - destinationDelta.delta, + destinationScale, + Number::RoundingMode::Downward) == kZero; + if (!destroyedIsSubUlp && + localPseudoDeltaAssets * -1 != roundedDestinationDelta) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change vault " - "and destination balance by equal amount"; + JLOG(j.fatal()) << "Invariant failed: " << // + "withdrawal must change vault and destination balance by equal " + "amount"; result = false; } } - // We don't need to round shares, they are integral MPT + // We don't round shares, they are integral MPT auto const accountDeltaShares = deltaShares(tx[sfAccount]); if (!accountDeltaShares) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change depositor " - "shares"; + JLOG(j.fatal()) << "Invariant failed: withdrawal must change depositor shares"; return false; } if (accountDeltaShares->delta >= kZero) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must decrease depositor " - "shares"; + JLOG(j.fatal()) + << "Invariant failed: withdrawal must decrease depositor shares"; result = false; } - // We don't need to round shares, they are integral MPT + // We don't round shares, they are integral MPT auto const vaultDeltaShares = deltaShares(afterVault.pseudoId); if (!vaultDeltaShares || vaultDeltaShares->delta == kZero) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change vault shares"; + JLOG(j.fatal()) << "Invariant failed: withdrawal must change vault shares"; return false; // That's all we can do } if (vaultDeltaShares->delta * -1 != accountDeltaShares->delta) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change depositor " - "and vault shares by equal amount"; + JLOG(j.fatal()) << "Invariant failed: " << // + "withdrawal must change depositor and vault shares by equal amount"; result = false; } @@ -900,8 +934,8 @@ ValidVault::finalize( // Note, vaultBalance is negative (see check above) if (assetTotalDelta != vaultPseudoDeltaAssets) { - JLOG(j.fatal()) << "Invariant failed: withdrawal and " - "assets outstanding must add up"; + JLOG(j.fatal()) + << "Invariant failed: withdrawal and assets outstanding must add up"; result = false; } @@ -910,8 +944,8 @@ ValidVault::finalize( if (assetAvailableDelta != vaultPseudoDeltaAssets) { - JLOG(j.fatal()) << "Invariant failed: withdrawal and " - "assets available must add up"; + JLOG(j.fatal()) + << "Invariant failed: withdrawal and assets available must add up"; result = false; } @@ -929,12 +963,11 @@ ValidVault::finalize( // The owner can use clawback to force-burn shares when the // vault is empty but there are outstanding shares if (!(beforeShares && beforeShares->sharesTotal > 0 && - vaultHoldsNoAssets(beforeVault) && beforeVault.owner == tx[sfAccount])) + isVaultEmpty(beforeVault) && beforeVault.owner == tx[sfAccount])) { - JLOG(j.fatal()) << // - "Invariant failed: clawback may only be performed " - "by the asset issuer, or by the vault owner of an " - "empty vault"; + JLOG(j.fatal()) << "Invariant failed: " << // + "clawback may only be performed by the asset issuer, or by the vault " + "owner of an empty vault"; return false; // That's all we can do } } @@ -942,19 +975,13 @@ ValidVault::finalize( auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); if (maybeVaultDeltaAssets) { - auto const totalDelta = DeltaInfo::makeDelta( - beforeVault.assetsTotal, afterVault.assetsTotal, vaultAsset); - auto const availableDelta = DeltaInfo::makeDelta( - beforeVault.assetsAvailable, afterVault.assetsAvailable, vaultAsset); auto const minScale = - computeCoarsestScale({*maybeVaultDeltaAssets, totalDelta, availableDelta}); + computeVaultMinScale(*maybeVaultDeltaAssets, view.rules()); auto const vaultDeltaAssets = roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); if (vaultDeltaAssets >= kZero) { - JLOG(j.fatal()) << // - "Invariant failed: clawback must decrease vault " - "balance"; + JLOG(j.fatal()) << "Invariant failed: clawback must decrease vault balance"; result = false; } @@ -963,8 +990,7 @@ ValidVault::finalize( if (assetsTotalDelta != vaultDeltaAssets) { JLOG(j.fatal()) << // - "Invariant failed: clawback and assets outstanding " - "must add up"; + "Invariant failed: clawback and assets outstanding must add up"; result = false; } @@ -975,12 +1001,11 @@ ValidVault::finalize( if (assetAvailableDelta != vaultDeltaAssets) { JLOG(j.fatal()) << // - "Invariant failed: clawback and assets available " - "must add up"; + "Invariant failed: clawback and assets available must add up"; result = false; } } - else if (!vaultHoldsNoAssets(beforeVault)) + else if (!isVaultEmpty(beforeVault)) { JLOG(j.fatal()) << // "Invariant failed: clawback must change vault balance"; @@ -998,8 +1023,7 @@ ValidVault::finalize( if (maybeAccountDeltaShares->delta >= kZero) { JLOG(j.fatal()) << // - "Invariant failed: clawback must decrease holder " - "shares"; + "Invariant failed: clawback must decrease holder shares"; result = false; } @@ -1014,9 +1038,8 @@ ValidVault::finalize( if (vaultDeltaShares->delta * -1 != maybeAccountDeltaShares->delta) { - JLOG(j.fatal()) << // - "Invariant failed: clawback must change holder and " - "vault shares by equal amount"; + JLOG(j.fatal()) << "Invariant failed: " << // + "clawback must change holder and vault shares by equal amount"; result = false; } diff --git a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp index aaaf3cced8..50d165a2ba 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -13,6 +14,7 @@ #include #include #include +#include #include #include // IWYU pragma: keep #include @@ -26,6 +28,24 @@ namespace xrpl { +[[nodiscard]] +static STAmount +roundToVaultScale(STAmount const& amount, SLE::const_ref vault) +{ + XRPL_ASSERT(vault && vault->getType() == ltVAULT, "xrpl::roundToVaultScale : valid vault sle"); + XRPL_ASSERT( + amount.asset() == vault->at(sfAsset), "xrpl::roundToVaultScale : valid vault asset"); + + if (amount.integral()) + return amount; + + int const postScale = [&]() { + NumberRoundModeGuard const rg(Number::RoundingMode::ToNearest); + return scale(vault->at(sfAssetsTotal) + amount, vault->at(sfAsset)); + }(); + return roundToScale(amount, postScale, Number::RoundingMode::Downward); +} + NotTEC VaultDeposit::preflight(PreflightContext const& ctx) { @@ -49,9 +69,9 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) return tecNO_ENTRY; auto const& account = ctx.tx[sfAccount]; - auto const assets = ctx.tx[sfAmount]; + auto const amount = ctx.tx[sfAmount]; auto const vaultAsset = vault->at(sfAsset); - if (assets.asset() != vaultAsset) + if (amount.asset() != vaultAsset) return tecWRONG_ASSET; auto const& vaultAccount = vault->at(sfAccount); @@ -63,7 +83,7 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) auto const mptIssuanceID = vault->at(sfShareMPTID); auto const vaultShare = MPTIssue(mptIssuanceID); - if (vaultShare == assets.asset()) + if (vaultShare == amount.asset()) { // LCOV_EXCL_START JLOG(ctx.j.error()) << "VaultDeposit: vault shares and assets cannot be same."; @@ -122,28 +142,69 @@ VaultDeposit::preclaim(PreclaimContext const& ctx) if (auto const ter = requireAuth(ctx.view, vaultAsset, account); !isTesSuccess(ter)) return ter; - if (accountHolds( - ctx.view, - account, - vaultAsset, - FreezeHandling::ZeroIfFrozen, - AuthHandling::ZeroIfUnauthorized, - ctx.j, - SpendableHandling::FullBalance) < assets) + bool const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); + auto const roundedAmount = fix320Enabled ? roundToVaultScale(amount, vault) : amount; + + if (fix320Enabled && roundedAmount == beast::kZero) + { + JLOG(ctx.j.warn()) << "VaultDeposit: deposit amount: " << ctx.tx[sfAmount] + << " is zero at vault scale"; + return tecPRECISION_LOSS; + } + + auto const accountBalance = accountHolds( + ctx.view, + account, + vaultAsset, + FreezeHandling::ZeroIfFrozen, + AuthHandling::ZeroIfUnauthorized, + ctx.j, + SpendableHandling::FullBalance); + + if (accountBalance < roundedAmount) return tecINSUFFICIENT_FUNDS; + // IOU precision checks + if (fix320Enabled && !roundedAmount.integral()) + { + // reject deposits that would canonicalize to a no-op at the depositor's trustline scale. + // Skipped for issuer-as-depositor: accountHolds returns (kMaxValue @ kMaxOffset) which + // would always trip the predicate. + if (account != amount.getIssuer() && + amount.isZeroAtScale(scale(accountBalance, vaultAsset))) + { + JLOG(ctx.j.warn()) << "VaultDeposit: amount " << amount.getFullText() + << " rounds to zero at counterparty trust-line scale"; + return tecPRECISION_LOSS; + } + } + return tesSUCCESS; } TER VaultDeposit::doApply() { + bool const fix320Enabled = view().rules().enabled(fixCleanup3_2_0); auto const vault = view().peek(keylet::vault(ctx_.tx[sfVaultID])); if (!vault) return tefINTERNAL; // LCOV_EXCL_LINE auto const vaultAsset = vault->at(sfAsset); - auto const amount = ctx_.tx[sfAmount]; + // Post-amendment IOU only: round Downward to the AssetsTotal precision so + // a sub-ULP tail can't be silently absorbed by one rail and not the other. + auto const amount = + fix320Enabled ? roundToVaultScale(ctx_.tx[sfAmount], vault) : ctx_.tx[sfAmount]; + + // We validated zero-amount in preclaim, if we ended up with zero now, fail hard. + if (amount == beast::kZero) + { + // LCOV_EXCL_START + JLOG(j_.error()) << "VaultDeposit: deposit amount: " << ctx_.tx[sfAmount] << " is zero"; + return tecINTERNAL; + // LCOV_EXCL_STOP + } + // Make sure the depositor can hold shares. auto const mptIssuanceID = (*vault)[sfShareMPTID]; auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID)); @@ -259,7 +320,7 @@ VaultDeposit::doApply() // trust line into debt the exact case preclaim authorizes via SpendableHandling::FullBalance. // The check thus converts a preclaim- authorized deposit into tefINTERNAL after the asset // transfer. - if (!view().rules().enabled(fixCleanup3_2_0)) + if (!fix320Enabled) { // Sanity check if (accountHolds( diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index c6a9a54a53..d8527876f1 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -6457,6 +6457,489 @@ class Vault_test : public beast::unit_test::Suite runTest(amendments); } + // Bug: DeltaInfo::makeDelta uses max(scale(after), scale(before)) for the + // sfAssetsTotal and sfAssetsAvailable deltas, and visitEntry applies the + // same max() for the vault pseudo-account RippleState. When + // sfAssetsTotal sits exactly at 1e16 (IOU exponent 1, ULP = 10) and a + // withdrawal of 5 USD brings it to 9.999...995e15 (IOU exponent 0, + // ULP = 1), all three computations pick the anterior coarser scale 1. + // roundToAsset(-5, scale=1) collapses to 0, so the invariant check + // vaultPseudoDeltaAssets >= kZero fires even though the state change is + // valid and fully consistent at IOU precision. + // + // Fix (fixCleanup3_2_0): finalize compares the vault pseudo-account and + // sfAssetsTotal/Available deltas directly in Number space, bypassing + // scale-coarsened rounding. + void + testBugMakeDeltaAnteriorScale() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + // Trust limit of 2e16, fund exactly 1e16 so deposit lands at the + // IOU scale-1 boundary (exponent 1, ULP = 10). + STAmount const fundAndDeposit{usd.raw(), Number{1, 16}}; + + env(trust(alice, STAmount{usd.raw(), 2, 16})); + env.close(); + env(pay(issuer, alice, fundAndDeposit)); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + + // sfAssetsTotal = sfAssetsAvailable = 1e16 (exponent 1, ULP = 10). + env(vault.deposit( + {.depositor = alice, .id = vaultKeylet.key, .amount = fundAndDeposit})); + env.close(); + + // Withdraw 5 USD: -5 is sub-ULP at the anterior scale (ULP = 10) + // but exact at the posterior scale (ULP = 1). The state change is + // consistent; only the invariant's scale selection is wrong. + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(5)}), + Ter(expected)); + env.close(); + }; + + { + testcase( + "bug: VaultWithdraw across IOU scale boundary fires invariant " + "(pre-fixCleanup3_2_0)"); + runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultWithdraw across IOU scale boundary succeeds " + "(post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), tesSUCCESS); + } + } + + // Bug: DeltaInfo::makeDelta uses max(scale(after), scale(before)) for + // sfAssetsTotal/Available deltas. This is symmetric to + // testBugMakeDeltaAnteriorScale but in the opposite direction: a deposit + // pushes assetsTotal from just below 1e16 (IOU exponent 0, ULP = 1) to just + // above it (exponent 1, ULP = 10). makeDelta picks the coarser *posterior* + // scale 1. The trust line balance rounds from atEdge + 2 = 10,000,000,000,000,001 + // → 1e16, so the pseudo-account delta is only +1 in IOU space. + // roundToAsset(+1, scale=1) = 0 fires "deposit must increase vault balance" + // even though the state change is consistent at every precision boundary. + // + // Fix (fixCleanup3_2_0): computeVaultMinScale uses the posterior Number-space + // scale of sfAssetsTotal (which retains the full value 10,000,000,000,000,001, + // exponent 0), giving minScale = 0. roundToAsset(+1, scale=0) = 1 > 0 and + // the invariant passes. However the transactor's own precision guard fires + // first (bob pays 2 USD, vault receives only 1 due to IOU rounding), so the + // post-amendment result is tecPRECISION_LOSS rather than tesSUCCESS — + // the depositor is protected from silently losing 1 USD to rounding. + void + testBugMakeDeltaPosteriorScale() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + env.fund(XRP(100'000), issuer, alice, bob); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + // atEdge is the largest IOU value with exponent 0 (ULP = 1). + // A deposit of 2 USD brings assetsTotal to 10,000,000,000,000,001 + // in Number space, crossing the 1e16 boundary in IOU space. + STAmount const atEdge{usd.raw(), Number{9'999'999'999'999'999LL}}; + + env(trust(alice, STAmount{usd.raw(), 2, 16})); + env(trust(bob, usd(100))); + env.close(); + env(pay(issuer, alice, atEdge)); + env(pay(issuer, bob, usd(2))); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + + // sfAssetsTotal = sfAssetsAvailable = atEdge (exponent 0, ULP = 1) + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = atEdge})); + env.close(); + + // Deposit 2 USD: +2 is sub-ULP at the posterior IOU scale (ULP = 10) + // but exact at the Number scale retained by sfAssetsTotal. + env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = usd(2)}), + Ter(expected)); + env.close(); + }; + + { + testcase( + "bug: VaultDeposit across IOU scale boundary fires invariant " + "(pre-fixCleanup3_2_0)"); + runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultDeposit across IOU scale boundary succeeds " + "(post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), tecPRECISION_LOSS); + } + } + + // Bug: ValidVault::visitEntry computes destinationDelta.scale as + // max(before_exponent, after_exponent) for RippleState entries. When a + // withdrawal credits a destination whose IOU balance sits just below a + // power-of-10 boundary (atEdge = 9'999'999'999'999'999), the post-credit + // STAmount rounds up one exponent (exponent 0 → 1), making + // destinationDelta.scale = 1. The invariant then calls + // roundToAsset(+2 USD, scale=1) = 0 and incorrectly fires + // "withdrawal must increase destination balance". + // + // Fix (fixCleanup3_2_0): finalize compares destination delta directly in + // Number space, bypassing scale-coarsened rounding. The transaction + // itself succeeds because the effective IOU credit is non-trivial at + // Number precision even though the STAmount exponent shifted. + void + testVaultWithdrawCanonicalizeToZero() + { + using namespace test::jtx; + + enum class DestKind : bool { ThirdParty = false, Self = true }; + + auto runScenario = [this](FeatureBitset features, DestKind destKind, TER expected) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + env.fund(XRP(100'000), issuer, alice, bob); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + STAmount const aliceLimit{usd.raw(), 2, 16}; + STAmount const bobLimit{usd.raw(), 2, 16}; + STAmount const atEdge{usd.raw(), Number{9'999'999'999'999'999LL}}; + + env(trust(alice, aliceLimit)); + if (destKind == DestKind::ThirdParty) + env(trust(bob, bobLimit)); + env.close(); + + env(pay(issuer, alice, usd(1'000))); + if (destKind == DestKind::ThirdParty) + env(pay(issuer, bob, atEdge)); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(1'000)})); + env.close(); + + // For the self-destination case, push alice's own trust line to + // the IOU edge so the next withdraw inflow crosses the boundary. + if (destKind == DestKind::Self) + { + env(pay(issuer, alice, atEdge)); + env.close(); + } + + auto tx = vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(2)}); + if (destKind == DestKind::ThirdParty) + tx[sfDestination] = bob.human(); + env(tx, Ter(expected)); + env.close(); + }; + + { + testcase( + "bug: VaultWithdraw to third-party at IOU edge fires invariant " + "(pre-fixCleanup3_2_0)"); + runScenario( + testableAmendments() - fixCleanup3_2_0, DestKind::ThirdParty, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultWithdraw to third-party at IOU edge succeeds " + "(post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), DestKind::ThirdParty, tesSUCCESS); + } + { + testcase( + "bug: VaultWithdraw to self at IOU edge fires invariant " + "(pre-fixCleanup3_2_0)"); + runScenario( + testableAmendments() - fixCleanup3_2_0, DestKind::Self, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultWithdraw to self at IOU edge succeeds " + "(post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), DestKind::Self, tesSUCCESS); + } + } + + // Bug: the equality check (vault outflow == destination inflow) was + // skipped whenever the destination delta rounded to zero at localMinScale, + // including cases where the vault outflow rounded to a non-zero value and + // a representable amount of value was genuinely destroyed. + // + // Scenario: Bob's IOU balance sits 5 units below the 10^16 STAmount + // precision boundary (atEdge2 = 9,999,999,999,999,995). A withdrawal of + // 6 USD shifts his balance across that boundary: the exponent increments + // (0 → 1), so his effective inflow in Number space is only +5 — 1 USD is + // consumed by the precision-boundary rounding and cannot be credited. + // + // The destroyed amount (1 USD) is sub-ULP at destinationScale=1 (step=10), + // so the check treats it as an unavoidable IOU-precision artefact and + // lets the transaction succeed. + // + // Contrast: if 15 USD were destroyed at the same scale (destroyed ≥ step), + // floor(15/10)=1 ≠ 0 and the invariant would fire — that discrepancy IS + // representable and indicates a real accounting bug. + // + // Pre-fixCleanup3_2_0: the "must increase destination balance" check fires + // because roundedDestinationDelta = 0 ≤ 0. + void + testVaultWithdrawEqualityEnforced() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + env.fund(XRP(100'000), issuer, alice, bob); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + STAmount const aliceLimit{usd.raw(), 2, 16}; + STAmount const bobLimit{usd.raw(), 2, 16}; + // Bob's balance sits 5 units below the 10^16 STAmount precision + // boundary. Receiving 6 USD shifts his exponent 0 → 1; the + // STAmount records +5, not +6 (1 USD is lost to rounding). + STAmount const atEdge2{usd.raw(), Number{9'999'999'999'999'995LL}}; + + env(trust(alice, aliceLimit)); + env(trust(bob, bobLimit)); + env.close(); + + env(pay(issuer, alice, usd(1'000))); + env(pay(issuer, bob, atEdge2)); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(1'000)})); + env.close(); + + // Withdraw 6 USD to Bob: vault loses 6, Bob gains only 5. + // Destroyed amount = 1 USD, which is sub-ULP at destinationScale=1. + auto tx = vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(6)}); + tx[sfDestination] = bob.human(); + env(tx, Ter(expected)); + env.close(); + }; + + { + testcase( + "bug: VaultWithdraw to destination at IOU precision boundary fires " + "invariant (pre-fixCleanup3_2_0)"); + runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultWithdraw to destination at IOU precision boundary succeeds " + "when destroyed amount is sub-ULP (post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), tesSUCCESS); + } + } + + // Bug: when a depositor's IOU trustline balance is very large (e.g. + // ~1e17), adding a small deposit (e.g. 1 USD) leaves sfAssetsTotal + // unchanged at IOU precision because the increment is sub-ULP at the + // vault's current asset scale. The vault records the deposit, mints + // shares, and decrements the depositor's trustline, but sfAssetsTotal + // does not change — the conservation invariant fires because the rail + // delta is zero. + // + // Two sub-cases are exercised: + // 1. First-ever deposit into an empty vault: the depositor's own + // trustline has a large balance so 1 USD canonicalizes to zero + // when written back through the IOU rail. + // 2. Subsequent deposit after the vault already holds a large + // sfAssetsTotal: a different depositor (bob, with a small balance) + // sends 1 USD, which again rounds to zero at the vault's coarse + // asset scale. + // + // Fix (fixCleanup3_2_0): the deposit transactor checks whether + // roundToAsset(amount, vault_scale) == 0 and rejects early with + // tecPRECISION_LOSS before any state is modified. + void + testVaultDepositCanonicalizeToZero() + { + using namespace test::jtx; + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + + env.fund(XRP(100'000), issuer, alice, bob); + env.close(); + + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + + STAmount const trustLimit{usd.raw(), Number{99'999'999'999'999'999LL}}; + STAmount const aliceFund{usd.raw(), Number{99'999'999'999'999'999LL}}; + + env(trust(alice, trustLimit)); + env(trust(bob, trustLimit)); + env.close(); + + env(pay(issuer, alice, aliceFund)); + env(pay(issuer, bob, usd(1000))); + env.close(); + + Vault const vault{env}; + + // Scale=0 so sfAssetsTotal stores whole USD + auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + + // Alice's deposit canonicalizes to zero at her own trustline scale + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(1)}), + Ter(expected)); + + // Increase vault-scale + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = aliceFund})); + env.close(); + + env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = usd(1)}), + Ter(expected)); + env.close(); + }; + + { + testcase( + "bug: VaultDeposit below Vault precision canonicalized to zero " + "(pre-fixCleanup3_2_0)"); + runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultDeposit below Vault precision canonicalized to zero " + "(post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), tecPRECISION_LOSS); + } + } + + // VaultDeposit by issuer with the vault parked at the IOU 16-digit + // edge (9.999e15). Issuer mints 2 more USD; the vault trust line + // goes 9.999e15 → 10^16, gaining 1 unit instead of 2 (canonicalization). + // + // Pre-fixCleanup3_2_0: the proactive check is absent; the deposit + // applies, then VaultInvariant's "deposit must increase vault + // balance" assertion fires at finalize time on the rounded vault + // delta of zero, returning tecINVARIANT_FAILED. + // Post-amendment: reject deposit that is not representable at Vault scale. + void + testBugIssuerVaultDepositAtEdge() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + + env.fund(XRP(100'000), issuer, owner); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + STAmount const trustLimit{usd.raw(), 2, 16}; + STAmount const ownerFund{usd.raw(), Number{9'999'999'999'999'999LL}}; + + env(trust(owner, trustLimit)); + env.close(); + env(pay(issuer, owner, ownerFund)); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + env(vault.deposit({.depositor = owner, .id = vaultKeylet.key, .amount = ownerFund})); + env.close(); + + // Vault pseudo-account is now at 9.999e15. Issuer mints 2 + // more USD. Pre: tecINVARIANT_FAILED at finalize. Post: + // tecPRECISION_LOSS proactively. Either way, no value moves. + env(vault.deposit({.depositor = issuer, .id = vaultKeylet.key, .amount = usd(2)}), + Ter(expected)); + env.close(); + }; + + { + testcase( + "bug: VaultDeposit by issuer at IOU edge fires " + "tecINVARIANT_FAILED at finalize (pre-fixCleanup3_2_0)"); + runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED); + } + { + testcase( + "bug: VaultDeposit by issuer at IOU edge rejects with " + "tecPRECISION_LOSS proactively (post-fixCleanup3_2_0)"); + runScenario(testableAmendments(), tecPRECISION_LOSS); + } + } + void testReferenceHolding() { @@ -6940,6 +7423,12 @@ public: void run() override { + testVaultWithdrawEqualityEnforced(); + testBugIssuerVaultDepositAtEdge(); + testBugMakeDeltaPosteriorScale(); + testBugMakeDeltaAnteriorScale(); + testVaultDepositCanonicalizeToZero(); + testVaultWithdrawCanonicalizeToZero(); testVaultDepositNegativeBalanceFromOppositeLimit(); testSequences(); testPreflight(); diff --git a/src/test/protocol/STAmount_test.cpp b/src/test/protocol/STAmount_test.cpp index c7207589ff..720fafa8f2 100644 --- a/src/test/protocol/STAmount_test.cpp +++ b/src/test/protocol/STAmount_test.cpp @@ -1203,8 +1203,6 @@ public: } } - //-------------------------------------------------------------------------- - void testIsZeroAtScale() { From 84ca271d95dfaa82fa6dfcc11647de532132765f Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 26 May 2026 13:45:55 -0400 Subject: [PATCH 09/10] Address some AI review feedback: predeclare, include, format, comment - Predeclare type reference in Rules.h - Remove an unneeded include in EscrowToken_test - Number_test will format negative BigInts correctly (unused) - Remove an inaccurate comment --- include/xrpl/protocol/Rules.h | 1 + src/test/app/EscrowToken_test.cpp | 1 - src/test/basics/Number_test.cpp | 3 +-- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/include/xrpl/protocol/Rules.h b/include/xrpl/protocol/Rules.h index b34e7995d3..9c17ff2391 100644 --- a/include/xrpl/protocol/Rules.h +++ b/include/xrpl/protocol/Rules.h @@ -123,6 +123,7 @@ private: }; class NumberSO; +class NumberMantissaScaleGuard; bool useRulesGuards(Rules const& rules); diff --git a/src/test/app/EscrowToken_test.cpp b/src/test/app/EscrowToken_test.cpp index bac2f798ad..5bb1303dba 100644 --- a/src/test/app/EscrowToken_test.cpp +++ b/src/test/app/EscrowToken_test.cpp @@ -28,7 +28,6 @@ #include #include #include -#include #include #include #include diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 52af19ed36..243bdb017b 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -31,7 +31,7 @@ class Number_test : public beast::unit_test::Suite int count = 0; for (auto it = s.rbegin(); it != s.rend(); ++it) { - if (count != 0 && count % 3 == 0) + if (count != 0 && count % 3 == 0 && isdigit(*it)) out.insert(out.begin(), '_'); out.insert(out.begin(), *it); ++count; @@ -1596,7 +1596,6 @@ public: constexpr std::int64_t kAValue = 1'000'000'000'000'049'863LL; constexpr std::int64_t kBValue = 9'223'372'036'854'315'903LL; - // Public conversion operator: STAmount::operator Number() const. Number const a = kAValue; Number const b = kBValue; Number const product = a * b; From fbee0349f53f76464e97e44f659007a5fa23e0cf Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 26 May 2026 15:21:42 -0400 Subject: [PATCH 10/10] clang-tidy: implicit bool conversion --- src/test/basics/Number_test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/basics/Number_test.cpp b/src/test/basics/Number_test.cpp index 243bdb017b..7d04022a19 100644 --- a/src/test/basics/Number_test.cpp +++ b/src/test/basics/Number_test.cpp @@ -31,7 +31,7 @@ class Number_test : public beast::unit_test::Suite int count = 0; for (auto it = s.rbegin(); it != s.rend(); ++it) { - if (count != 0 && count % 3 == 0 && isdigit(*it)) + if (count != 0 && count % 3 == 0 && (isdigit(*it) != 0)) out.insert(out.begin(), '_'); out.insert(out.begin(), *it); ++count;