From ea13be81b7037ce0a03c706e5b916b0beb581496 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:21:23 +0200 Subject: [PATCH] feat: Add an invariant to ensure object deletion also deletes its pseudo-account (#7445) Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> --- include/xrpl/tx/invariants/InvariantCheck.h | 26 +++- src/libxrpl/tx/invariants/InvariantCheck.cpp | 95 +++++++++++-- src/test/app/Invariants_test.cpp | 142 +++++++++++++++++-- 3 files changed, 242 insertions(+), 21 deletions(-) diff --git a/include/xrpl/tx/invariants/InvariantCheck.h b/include/xrpl/tx/invariants/InvariantCheck.h index 8931d189fd..ba91cba064 100644 --- a/include/xrpl/tx/invariants/InvariantCheck.h +++ b/include/xrpl/tx/invariants/InvariantCheck.h @@ -375,16 +375,35 @@ public: */ class ValidAmounts { - std::vector> afterEntries_; + std::vector afterEntries_; public: void - visitEntry(bool, std::shared_ptr const&, std::shared_ptr const&); + visitEntry(bool, SLE::const_ref, SLE::const_ref); [[nodiscard]] bool finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; }; +/* + * Verify that when an object with an associated pseudo-account is deleted, + * its pseudo-account is also deleted. + * + * The reverse (pseudo-account deleted → object deleted) is enforced by + * AccountRootsDeletedClean via getPseudoAccountFields(). + */ +class ObjectHasPseudoAccount +{ +public: + void + visitEntry(bool, SLE::const_ref, SLE::const_ref); + + [[nodiscard]] bool + finalize(STTx const&, TER const, XRPAmount const, ReadView const&, beast::Journal const&) const; + +private: + std::vector deletedObjSles_; +}; // additional invariant checks can be declared above and then added to this // tuple using InvariantChecks = std::tuple< @@ -416,7 +435,8 @@ using InvariantChecks = std::tuple< ValidConfidentialMPToken, ValidMPTPayment, ValidAmounts, - ValidMPTTransfer>; + ValidMPTTransfer, + ObjectHasPseudoAccount>; /** * @brief get a tuple of all invariant checks diff --git a/src/libxrpl/tx/invariants/InvariantCheck.cpp b/src/libxrpl/tx/invariants/InvariantCheck.cpp index 308342da74..32df44a96b 100644 --- a/src/libxrpl/tx/invariants/InvariantCheck.cpp +++ b/src/libxrpl/tx/invariants/InvariantCheck.cpp @@ -35,6 +35,7 @@ #include #include #include +#include #include namespace xrpl { @@ -63,6 +64,23 @@ hasPrivilege(STTx const& tx, Privilege priv) #undef TRANSACTION #pragma pop_macro("TRANSACTION") +// Returns the human-readable name of a ledger entry's type, falling back to +// the numeric type if the format is somehow unknown. +static std::string +ledgerEntryTypeName(SLE const& sle) +{ + auto const item = LedgerFormats::getInstance().findByType(sle.getType()); + + if (item == nullptr) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::ledgerEntryTypeName : ledger entry has no known ledger format"); + return std::to_string(sle.getType()); + // LCOV_EXCL_STOP + } + return item->getName(); +} + void TransactionFeeCheck::visitEntry(bool, SLE::const_ref, SLE::const_ref) { @@ -457,16 +475,8 @@ AccountRootsDeletedClean::finalize( if (auto const sle = view.read(keylet)) { // Finding the object is bad - auto const typeName = [&sle]() { - auto item = LedgerFormats::getInstance().findByType(sle->getType()); - - if (item != nullptr) - return item->getName(); - return std::to_string(sle->getType()); - }(); - - JLOG(j.fatal()) << "Invariant failed: account deletion left behind a " << typeName - << " object"; + JLOG(j.fatal()) << "Invariant failed: account deletion left behind a " + << ledgerEntryTypeName(*sle) << " object"; // The comment above starting with "assert(enforce)" explains this // assert. XRPL_ASSERT( @@ -1070,4 +1080,69 @@ ValidAmounts::finalize( return true; } +void +ObjectHasPseudoAccount::visitEntry(bool isDelete, SLE::const_ref before, SLE::const_ref after) +{ + if (!isDelete) + return; + + // Before should never be null when isDelete = true + if (!before) + { + // LCOV_EXCL_START + UNREACHABLE( + "xrpl::ObjectHasPseudoAccount::visitEntry : deleted ledger entry missing before state"); + return; + // LCOV_EXCL_STOP + } + + switch (before->getType()) + { + case ltAMM: + case ltVAULT: + case ltLOAN_BROKER: + deletedObjSles_.push_back(before); + break; + default: + return; + } +} + +[[nodiscard]] bool +ObjectHasPseudoAccount::finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) const +{ + if (!view.rules().enabled(fixCleanup3_3_0)) + return true; + + if (deletedObjSles_.empty()) + return true; + + bool failed = false; + for (auto const& sle : deletedObjSles_) + { + if (!sle->isFieldPresent(sfAccount)) + { + JLOG(j.fatal()) << "Invariant failed: deleted " << ledgerEntryTypeName(*sle) + << " is missing pseudo-account field"; + failed = true; + continue; + } + + // The pseudo-account must NOT exist on the ledger after the object is deleted. + if (view.exists(keylet::account(sle->getAccountID(sfAccount)))) + { + JLOG(j.fatal()) << "Invariant failed: deleted " << ledgerEntryTypeName(*sle) + << " without deleting its pseudo-account"; + failed = true; + } + } + + return !failed; +} + } // namespace xrpl diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 9fde52ecb2..079ff9df88 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2829,7 +2829,8 @@ class Invariants_test : public beast::unit_test::Suite }); doInvariantCheck( - {"vault updated by a wrong transaction type"}, + {"vault updated by a wrong transaction type", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -2840,7 +2841,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttPAYMENT, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; auto [tx, _] = vault.create({.owner = a1, .asset = xrpIssue()}); @@ -2877,6 +2878,7 @@ class Invariants_test : public beast::unit_test::Suite auto const vaultPage = ac.view().dirInsert( keylet::ownerDir(a1.id()), sleVault->key(), describeOwnerDir(a1.id())); sleVault->setFieldU64(sfOwnerNode, *vaultPage); + sleVault->setAccountID(sfAccount, a1.id()); ac.view().insert(sleVault); return true; }, @@ -2885,7 +2887,8 @@ class Invariants_test : public beast::unit_test::Suite {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); doInvariantCheck( - {"vault deleted by a wrong transaction type"}, + {"vault deleted by a wrong transaction type", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -2896,7 +2899,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_SET, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; auto [tx, _] = vault.create({.owner = a1, .asset = xrpIssue()}); @@ -2905,7 +2908,8 @@ class Invariants_test : public beast::unit_test::Suite }); doInvariantCheck( - {"vault operation updated more than single vault"}, + {"vault operation updated more than single vault", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); @@ -2925,7 +2929,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_DELETE, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; { @@ -2949,6 +2953,7 @@ class Invariants_test : public beast::unit_test::Suite auto const vaultPage = ac.view().dirInsert( keylet::ownerDir(a.id()), sleVault->key(), describeOwnerDir(a.id())); sleVault->setFieldU64(sfOwnerNode, *vaultPage); + sleVault->setAccountID(sfAccount, a.id()); ac.view().insert(sleVault); }; insertVault(a1); @@ -2960,7 +2965,8 @@ class Invariants_test : public beast::unit_test::Suite {tecINVARIANT_FAILED, tecINVARIANT_FAILED}); doInvariantCheck( - {"deleted vault must also delete shares"}, + {"deleted vault must also delete shares", + "deleted Vault without deleting its pseudo-account"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { auto const keylet = keylet::vault(a1.id(), ac.view().seq()); auto sleVault = ac.view().peek(keylet); @@ -2971,7 +2977,7 @@ class Invariants_test : public beast::unit_test::Suite }, XRPAmount{}, STTx{ttVAULT_DELETE, [](STObject&) {}}, - {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, [&](Account const& a1, Account const& a2, Env& env) { Vault const vault{env}; auto [tx, _] = vault.create({.owner = a1, .asset = xrpIssue()}); @@ -5176,6 +5182,125 @@ class Invariants_test : public beast::unit_test::Suite } } + void + testObjectHasPseudoAccount() + { + testcase << "object has pseudo-account"; + using namespace jtx; + + auto const amendments = defaultAmendments() | fixCleanup3_3_0; + + // Vault: object deleted without its pseudo-account + { + Keylet vaultKeylet = keylet::amendments(); + doInvariantCheck( + Env{*this, amendments}, + {{"deleted Vault without deleting its pseudo-account"}}, + [&vaultKeylet](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(vaultKeylet); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttVAULT_DELETE, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&vaultKeylet](Account const& a1, Account const&, Env& env) { + Vault const vault{env}; + auto [tx, keylet] = vault.create({.owner = a1, .asset = xrpIssue()}); + env(tx); + vaultKeylet = keylet; + return true; + }); + } + + // AMM: object deleted without its pseudo-account + { + uint256 ammID{}; + Account const gw{"gw"}; + doInvariantCheck( + Env{*this, amendments}, + {{"deleted AMM without deleting its pseudo-account"}}, + [&ammID](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(keylet::amm(ammID)); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttAMM_DELETE, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&ammID, &gw](Account const&, Account const&, Env& env) { + env.fund(XRP(1'000), gw); + AMM const amm(env, gw, XRP(100), gw["USD"](100)); + ammID = amm.ammID(); + return true; + }); + } + + // LoanBroker: object deleted without its pseudo-account + { + Keylet loanBrokerKeylet = keylet::amendments(); + doInvariantCheck( + Env{*this, amendments}, + {{"deleted LoanBroker without deleting its pseudo-account"}}, + [&loanBrokerKeylet](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(loanBrokerKeylet); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttLOAN_BROKER_DELETE, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&loanBrokerKeylet, this](Account const& a1, Account const&, Env& env) { + PrettyAsset const xrpAsset{xrpIssue(), 1'000'000}; + loanBrokerKeylet = this->createLoanBroker(a1, env, xrpAsset); + return BEAST_EXPECT(env.le(loanBrokerKeylet)); + }); + } + + // Deleted object missing sfAccount field (defensive check). + // Manually construct the view to place a vault SLE without + // sfAccount into the base ledger, then erase it. + { + Env env{*this, amendments}; + Account const a1{"A1"}; + Account const a2{"A2"}; + env.fund(XRP(1000), a1, a2); + env.close(); + + OpenView ov{*env.current()}; + + auto const vaultKeylet = keylet::vault(a1.id(), ov.seq()); + auto sleVault = std::make_shared(vaultKeylet); + sleVault->makeFieldAbsent(sfAccount); + ov.rawInsert(sleVault); + + STTx const tx{ttVAULT_DELETE, [](STObject&) {}}; + test::StreamSink sink{beast::Severity::Warning}; + beast::Journal const jlog{sink}; + ApplyContext ac{ + env.app(), ov, tx, tesSUCCESS, env.current()->fees().base, TapNone, jlog}; + CurrentTransactionRulesGuard const rulesGuard(ov.rules()); + + auto sle = ac.view().peek(vaultKeylet); + if (!BEAST_EXPECT(sle)) + return; + ac.view().erase(sle); + + auto transactor = makeTransactor(ac); + if (!BEAST_EXPECT(transactor)) + return; + TER const result = transactor->checkInvariants(tesSUCCESS, XRPAmount{}); + BEAST_EXPECT(result == tecINVARIANT_FAILED); + BEAST_EXPECT(sink.messages().str().contains("is missing pseudo-account field")); + } + } + void testConfidentialMPTTransfer() { @@ -5458,6 +5583,7 @@ public: testInvariantOverwrite(defaultAmendments() - fixCleanup3_1_3); testVaultComputeCoarsestScale(); testAMM(); + testObjectHasPseudoAccount(); } };