From de90cfca8e7232ce3d399d97f178f3b35cbcbacb Mon Sep 17 00:00:00 2001 From: Gregory Tsipenyuk Date: Tue, 18 Aug 2026 10:20:55 -0400 Subject: [PATCH] Catch MPToken deletion in the on-failure transfer invariant A deleted holder has no amtAfter, so the sender/receiver classification skipped it and a deletion on a failed transaction went unreported. Add tesSUCCESS baselines so the result guard itself is covered, document what invariantPasses means at each site, and record why a missing issuance cannot be tightened into a failure. --- src/libxrpl/tx/invariants/MPTInvariant.cpp | 26 ++++++++++-- src/test/app/Invariants_test.cpp | 47 ++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 4542475f7e..3a04a5bbf0 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -143,6 +143,10 @@ ValidMPTIssuance::finalize( // must not dangle outside that controlled lifecycle. if (rules.enabled(fixCleanup3_2_0)) { + // Unlike the same-named flags in the finalize() methods below, this is + // a plain accumulator: the checks in this block are enforcing whenever + // fixCleanup3_2_0 is enabled, and it is cleared by any violation so + // that all of them get logged before returning. bool invariantPasses = true; if (referenceHoldingMutated_) { @@ -487,6 +491,10 @@ ValidMPTBalanceChanges::finalize( return true; } + // Value returned when a violation is detected below, so this is the + // advisory (log-only) condition: it holds when NEITHER amendment is + // enabled. Enabling either featureMPTokensV2 or fixCleanup3_4_0 makes + // the checks enforcing. bool const invariantPasses = !view.rules().enabled(featureMPTokensV2) && !fix340Enabled; if (overflow_) { @@ -523,8 +531,7 @@ ValidMPTBalanceChanges::finalize( // credentials. No MPToken or MPTokenIssuance is in that set, and // none of those deletions moves MPT value, so whatever a transactor // wrote before returning a tec cannot reach this check. - bool const failed = !isTesSuccess(result); - if (failed && data.outstanding[kIAfter] != data.outstanding[kIBefore]) + if (!isTesSuccess(result) && data.outstanding[kIAfter] != data.outstanding[kIBefore]) { JLOG(j.fatal()) << "Invariant failed: OutstandingAmount balance changed on failure " << tx.getTxnType() << " " << result; @@ -877,6 +884,10 @@ ValidMPTTransfer::finalize( }(); auto const fix340Enabled = view.rules().enabled(fixCleanup3_4_0); + // Value returned when a violation is detected below, so this is the + // advisory (log-only) condition: it holds when NEITHER amendment is + // enabled. Enabling either featureMPTokensV2 or fixCleanup3_4_0 makes the + // checks enforcing. auto const invariantPasses = !view.rules().enabled(featureMPTokensV2) && !fix340Enabled; for (auto const& [mptID, values] : amount_) @@ -887,6 +898,13 @@ ValidMPTTransfer::finalize( auto const sleIssuance = view.read(keylet::mptokenIssuance(mptID)); if (!sleIssuance) { + // A missing issuance does not imply that this transaction destroyed + // it. MPTokenIssuanceDestroy only requires a zero OutstandingAmount, + // so holders' MPTokens outlive the issuance as orphans, and a later + // transaction of any type may clean one up (see the "Skipping + // Deleted MPTs" case in Invariants_test). There is no issuance left + // to read the transfer rules from, so skip the entry rather than + // trying to infer intent from the transaction type. continue; } @@ -949,8 +967,8 @@ ValidMPTTransfer::finalize( // failure. No result code is exempt — see the matching note in // ValidMPTBalanceChanges::finalize for why a tec cannot carry an MPT // change this far. - bool const failed = !isTesSuccess(result); - if (fix340Enabled && failed && (senders > 0 || receivers > 0)) + if (fix340Enabled && !isTesSuccess(result) && + (senders > 0 || receivers > 0 || !deletedAuthorized_.empty())) { JLOG(j.fatal()) << "Invariant failed: MPToken balance changed on failure " << txnType << " " << result; diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 5e4ad4bd6f..8903f17b34 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -4927,6 +4927,14 @@ class Invariants_test : public beast::unit_test::Suite STTx const payment{ttPAYMENT, [](STObject&) {}}; + // Baseline: both changes are conservation-consistent, so on + // tesSUCCESS nothing fires. Without these the on-failure cases below + // would still pass if the result guard were dropped, since they only + // establish that a violation is caught, not that it is caught solely + // on failure. + doInvariantCheck({}, mint, XRPAmount{}, payment, {tesSUCCESS, tesSUCCESS}, setup); + doInvariantCheck({}, transfer, XRPAmount{}, payment, {tesSUCCESS, tesSUCCESS}, setup); + // tecKILLED and tecINCOMPLETE are not special: an MPT change paired // with either fires the check, exactly as any other failure does. doInvariantCheck( @@ -5009,6 +5017,8 @@ class Invariants_test : public beast::unit_test::Suite ac.view().update(sleTok); return true; }; + // Baseline, as above: a lock is legitimate on tesSUCCESS. + doInvariantCheck({}, lock, XRPAmount{}, payment, {tesSUCCESS, tesSUCCESS}, setup); doInvariantCheck( {{"MPToken balance changed on failure"}}, lock, @@ -5030,6 +5040,43 @@ class Invariants_test : public beast::unit_test::Suite TxAccount::None, std::source_location::current(), tecEXPIRED); + + // Deleting a holder's MPToken moves no value, so the sender / + // receiver classification skips it (a deleted holder has no + // amtAfter). It must still be caught on failure — that is what the + // deletedAuthorized_ term covers. This needs a separate issuance + // whose holders were authorized but never paid, so the MPToken can + // be erased with a zero balance and OutstandingAmount untouched; + // otherwise the holder would register as a sender and the term + // under test would never be the deciding one. + MPTID emptyId; + auto const setupEmpty = [&](Account const& a1, Account const& a2, Env& env) { + Account const gw("gw"); + env.fund(XRP(1'000), gw); + MPTTester const mpt({.env = env, .issuer = gw, .holders = {a1, a2}, .maxAmt = 100}); + emptyId = mpt.issuanceID(); + return true; + }; + Precheck const eraseToken = [&](Account const& a1, Account const&, ApplyContext& ac) { + auto sleTok = ac.view().peek(keylet::mptoken(emptyId, a1.id())); + if (!sleTok || (*sleTok)[sfMPTAmount] != 0) + return false; + ac.view().erase(sleTok); + return true; + }; + // ValidMPTIssuance also reports the deletion, so assert on the + // ValidMPTTransfer message specifically: it is only logged when the + // deletedAuthorized_ term fires. + doInvariantCheck( + {{"MPToken balance changed on failure"}}, + eraseToken, + XRPAmount{}, + payment, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + setupEmpty, + TxAccount::None, + std::source_location::current(), + tecEXPIRED); } // Invalid IOU clawback delta must fail once MPTokensV2 enforces before/after validation.