ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-20 05:40:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

Catch MPToken deletion in the on-failure transfer invariant

Browse Source 
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.
This commit is contained in:
Gregory Tsipenyuk
2026-08-18 10:20:55 -04:00
parent 2060dbba9d
commit de90cfca8e
2 changed files with 69 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
src/libxrpl/tx/invariants/MPTInvariant.cpp
View File

@@ -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;

47
src/test/app/Invariants_test.cpp
View File

@@ -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.