ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-18 21:10:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

Simplify MPT invariant comments and conditions

Browse Source
This commit is contained in:
Gregory Tsipenyuk
2026-08-18 14:14:33 -04:00
parent de90cfca8e
commit 1a19758f54
2 changed files with 68 additions and 113 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -143,10 +143,8 @@ 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.
// Not an amendment gate like the same-named flags below, just an
// accumulator, so that every violation gets logged before returning.
bool invariantPasses = true;
if (referenceHoldingMutated_)
{
@@ -491,11 +489,9 @@ 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;
// Returned when a violation is found below, so this is the log-only
// condition. Either amendment makes the checks enforcing.
auto const invariantPasses = !(view.rules().enabled(featureMPTokensV2) || fix340Enabled);
if (overflow_)
{
JLOG(j.fatal()) << "Invariant failed: OutstandingAmount overflow";
@@ -520,18 +516,12 @@ ValidMPTBalanceChanges::finalize(
return invariantPasses;
}
// Enforce the invariant even for a failed transaction: a
// transaction that did not succeed must not have moved MPT value,
// so OutstandingAmount must be unchanged. This catches a bug or
// exploit that mutates issuance state on a code path that then
// reports failure. No result code is exempt. Transactor::operator()
// routes every tec through processPersistentChanges, which discards
// the view and re-applies only deletions of the entry types listed
// in typesForResult: offers, trust lines, NFT offers and
// 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.
if (!isTesSuccess(result) && data.outstanding[kIAfter] != data.outstanding[kIBefore])
// A failed transaction must not have moved MPT value; the check
// above ties mptAmount to the OutstandingAmount delta. No result
// code is exempt: on any tec the transactor discards the view and
// re-applies only offer, trust line, NFT offer and credential
// deletions (Transactor::typesForResult), none of which touch MPTs.
if (!isTesSuccess(result) && data.mptAmount != 0)
{
JLOG(j.fatal()) << "Invariant failed: OutstandingAmount balance changed on failure "
<< tx.getTxnType() << " " << result;
@@ -884,11 +874,9 @@ 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;
// Returned when a violation is found below, so this is the log-only
// condition. Either amendment makes the checks enforcing.
auto const invariantPasses = !(view.rules().enabled(featureMPTokensV2) || fix340Enabled);
for (auto const& [mptID, values] : amount_)
{
@@ -898,13 +886,11 @@ 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.
// A missing issuance does not mean this transaction destroyed it.
// MPTokenIssuanceDestroy only requires a zero OutstandingAmount, so
// holders' MPTokens outlive it as orphans that a later transaction
// of any type may clean up. With no issuance there are no transfer
// rules to check, so the transaction type tells us nothing here.
continue;
}
@@ -955,18 +941,11 @@ ValidMPTTransfer::finalize(
return invariantPasses;
}
// Enforce the invariant even for a failed transaction: a transaction
// that did not succeed must not have changed any holder's MPT balance.
// A single holder whose spendable balance moved (a sender OR a
// receiver) is enough — not just a two-sided transfer — so this also
// catches a one-sided change such as a lock/unlock that shifts value
// between a holder's spendable (sfMPTAmount) and locked (sfLockedAmount)
// buckets. (A change touching only sfLockedAmount is caught instead by
// ValidMPTBalanceChanges, which tracks the holder total.) This catches a
// bug or exploit that moves balances on a code path that then reports
// failure. No result code is exempt — see the matching note in
// ValidMPTBalanceChanges::finalize for why a tec cannot carry an MPT
// change this far.
// A failed transaction must not have changed a holder's balance. One
// side is enough, unlike the transfer check above, so this also catches
// a lock/unlock moving value between sfMPTAmount and sfLockedAmount,
// and a deleted MPToken, which the sender/receiver counts skip. See
// ValidMPTBalanceChanges::finalize for why no result code is exempt.
if (fix340Enabled && !isTesSuccess(result) &&
(senders > 0 || receivers > 0 || !deletedAuthorized_.empty()))
{

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

@@ -139,9 +139,9 @@ class Invariants_test : public beast::unit_test::Suite
Preclose const& preclose = {},
TxAccount setTxAccount = TxAccount::None,
std::source_location const& loc = std::source_location::current(),
// Result fed to the invariant checker on the first pass. Defaults to
// tesSUCCESS; set to a specific tec to test result-dependent invariant
// behavior (e.g. the on-failure checks and their exempt codes).
// Result fed to the invariant checker on the first pass. Set it to a
// tec to exercise result-dependent invariants; the harness runs no
// transactor, so one never arises on its own.
TER initialResult = tesSUCCESS)
{
doInvariantCheck(
@@ -218,11 +218,10 @@ class Invariants_test : public beast::unit_test::Suite
return;
// Invoke the check twice to cover the tec and tef cases. Both passes run
// against the same view -- unlike production, nothing is discarded in
// between (Transactor::reset would), so the second pass sees the same
// violation and escalates tec -> tef. A {tec, tef} pair therefore says
// "this invariant is enforced regardless of the incoming result", not
// that the transaction ends in tefINVARIANT_FAILED on ledger.
// against the same view -- production would discard it in between -- so
// the second sees the same violation and escalates tec -> tef. A
// {tec, tef} pair therefore means "enforced whatever the incoming
// result", not that the transaction ends in tef on ledger.
if (!BEAST_EXPECT(ters.size() == 2))
return;
@@ -238,11 +237,9 @@ class Invariants_test : public beast::unit_test::Suite
loc.line());
auto const messages = sink.messages().str();
// checkInvariants returns its input unchanged when nothing fires and
// an escalated failure code when an invariant fires. So a changed
// result means an invariant fired, and a firing invariant must log.
// A result that passes through unchanged (a success) fired nothing
// and needs no message.
// checkInvariants returns its input unchanged unless something
// fires, so a changed result means an invariant fired, and a firing
// invariant must log.
if (terActual != terInput)
{
expect(
@@ -4871,15 +4868,10 @@ class Invariants_test : public beast::unit_test::Suite
});
// The on-failure MPT checks (OutstandingAmount balance / transfer) apply
// to every non-tesSUCCESS result, with no per-result exemption. No
// transactor reaches the invariant check with an MPT change on a tec:
// Transactor::operator() routes every tec through
// processPersistentChanges -> reset() -> ApplyContext::discard(), and
// re-applies only offer / trust line / NFT offer / credential
// deletions. So a change that survives to here on a failure result is a
// bug, whatever the code. The result is supplied via doInvariantCheck's
// initialResult seed — a tec cannot arise naturally here, since the
// harness runs only the invariant check, not doApply.
// to every non-tesSUCCESS result, with no per-result exemption: on a tec
// the transactor discards the view and re-applies only offer, trust
// line, NFT offer and credential deletions, so an MPT change reaching
// the invariant is a bug whatever the code. Seeded via initialResult.
{
MPTID id;
// preclose: gw issues an MPT held by A1 and A2.
@@ -4893,8 +4885,7 @@ class Invariants_test : public beast::unit_test::Suite
};
// Consistent mint: OutstandingAmount and A1's balance both grow by
// 10, so conservation holds but OutstandingAmount changed — only the
// balance-change on-failure check is sensitive to it.
// 10, so conservation holds and only the on-failure check fires.
Precheck const mint = [&](Account const& a1, Account const&, ApplyContext& ac) {
auto sleIss = ac.view().peek(keylet::mptokenIssuance(id));
auto sleTok = ac.view().peek(keylet::mptoken(id, a1.id()));
@@ -4907,9 +4898,9 @@ class Invariants_test : public beast::unit_test::Suite
return true;
};
// Holder-to-holder transfer (A1 -> A2 by 10): OutstandingAmount
// unchanged, so only the transfer on-failure check is sensitive. Set
// CanTransfer so the ordinary transfer check stays quiet.
// Holder-to-holder transfer (A1 -> A2 by 10). OutstandingAmount is
// unchanged, and CanTransfer keeps the ordinary transfer check
// quiet, so only the on-failure check fires.
Precheck const transfer = [&](Account const& a1, Account const& a2, ApplyContext& ac) {
auto sleIss = ac.view().peek(keylet::mptokenIssuance(id));
auto sleA = ac.view().peek(keylet::mptoken(id, a1.id()));
@@ -4927,16 +4918,13 @@ 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.
// Negative controls: nothing fires on tesSUCCESS. Without these, the
// cases below would still pass if the result guard were dropped.
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.
// with either fires, as with any other failure.
doInvariantCheck(
{{"OutstandingAmount balance changed on failure"}},
mint,
@@ -4977,8 +4965,8 @@ class Invariants_test : public beast::unit_test::Suite
TxAccount::None,
std::source_location::current(),
tecINCOMPLETE);
// The same change under another failure result, for symmetry: the
// check keys off "not tesSUCCESS", nothing finer.
// The same change under a third failure result: the check keys off
// "not tesSUCCESS", nothing finer.
doInvariantCheck(
{{"OutstandingAmount balance changed on failure"}},
mint,
@@ -5000,24 +4988,21 @@ class Invariants_test : public beast::unit_test::Suite
std::source_location::current(),
tecEXPIRED);
// A one-sided lock (spendable -> locked within one holder) is not a
// two-sided transfer, yet must still be caught on failure — this is
// the gap the `senders || receivers` condition closes.
// OutstandingAmount and the holder total are unchanged, so only the
// transfer-side on-failure check sees it.
// A lock moves value within one holder, so it is not a two-sided
// transfer and the `senders || receivers` form is what catches it.
// OutstandingAmount and the holder total are unchanged, so the
// balance check stays quiet.
Precheck const lock = [&](Account const& a1, Account const&, ApplyContext& ac) {
auto sleTok = ac.view().peek(keylet::mptoken(id, a1.id()));
if (!sleTok || (*sleTok)[sfMPTAmount] < 10)
return false;
// Move 10 from spendable to locked (a fresh MPToken has no
// locked amount, so set it directly). Holder total and
// OutstandingAmount are unchanged.
// A fresh MPToken has no locked amount, so set it directly.
(*sleTok)[sfMPTAmount] = (*sleTok)[sfMPTAmount] - 10;
sleTok->setFieldU64(sfLockedAmount, 10);
ac.view().update(sleTok);
return true;
};
// Baseline, as above: a lock is legitimate on tesSUCCESS.
// Negative control: a lock is legitimate on tesSUCCESS.
doInvariantCheck({}, lock, XRPAmount{}, payment, {tesSUCCESS, tesSUCCESS}, setup);
doInvariantCheck(
{{"MPToken balance changed on failure"}},
@@ -5029,7 +5014,7 @@ class Invariants_test : public beast::unit_test::Suite
TxAccount::None,
std::source_location::current(),
tecKILLED);
// The one-sided lock is caught under any failure result.
// The lock is caught under any failure result.
doInvariantCheck(
{{"MPToken balance changed on failure"}},
lock,
@@ -5041,14 +5026,11 @@ class Invariants_test : public beast::unit_test::Suite
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.
// A deleted MPToken has no amtAfter, so the sender/receiver counts
// skip it and only the deletedAuthorized_ term can catch it. That
// needs holders 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 instead.
MPTID emptyId;
auto const setupEmpty = [&](Account const& a1, Account const& a2, Env& env) {
Account const gw("gw");
@@ -5064,9 +5046,8 @@ class Invariants_test : public beast::unit_test::Suite
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.
// ValidMPTIssuance also reports the deletion, so assert on
// ValidMPTTransfer's message, which only the new term can produce.
doInvariantCheck(
{{"MPToken balance changed on failure"}},
eraseToken,
@@ -5842,12 +5823,9 @@ class Invariants_test : public beast::unit_test::Suite
MPTTester const usd(
{.env = env, .issuer = gw, .holders = {a1, a2}, .pay = 100});
id = usd.issuanceID();
// Enforcement is gated on featureMPTokensV2 OR
// fixCleanup3_4_0, so the advisory path must
// disable both to stay non-enforcing. Disabling
// happens after the MPT is set up; doInvariantCheck
// closes the ledger next, which is what makes it
// take effect.
// Either gate enforces, so both must be off to stay
// advisory. Disable after setting up the MPT; the
// next env.close() is what makes it take effect.
if (!gates[featureMPTokensV2])
env.disableFeature(featureMPTokensV2);
if (!gates[fixCleanup3_4_0])
@@ -6032,9 +6010,8 @@ class Invariants_test : public beast::unit_test::Suite
for (bool const isMPT : {false, true})
{
// Both IOU and MPT pools now escalate to tefINVARIANT_FAILED on the
// second invariant pass (MPT balance invariants enforce under
// fixCleanup3_4_0), so the AMM pool-change check fails on both.
// Under fixCleanup3_4_0 the MPT balance invariants also fire on the
// second pass, so both IOU and MPT pools now escalate to tef.
auto const error = TER(tefINVARIANT_FAILED);
for (auto txType : {ttAMM_CREATE, ttAMM_DEPOSIT, ttAMM_CLAWBACK, ttAMM_WITHDRAW})
{
@@ -6729,9 +6706,8 @@ class Invariants_test : public beast::unit_test::Suite
},
XRPAmount{},
STTx{ttCONFIDENTIAL_MPT_SEND, [](STObject&) {}},
// Second pass is tef, not tec: the bumped holder MPTAmount also trips
// ValidMPTTransfer's on-failure "balance changed" check (fixCleanup3_4_0),
// which fires on the pass-2 tec input and escalates it to tef.
// Second pass is tef: the bumped MPTAmount also trips
// ValidMPTTransfer's on-failure check, which escalates the tec.
{tecINVARIANT_FAILED, tefINVARIANT_FAILED},
precloseConfidential);