From 1798be0ba9f9ec659512bd343e109aef0fcbd6b5 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Thu, 17 Apr 2025 17:36:29 -0400 Subject: [PATCH] Ensure that an account is not deleted with an XRP balance - Updates the AccountRootsDeletedClean invariant --- src/test/ledger/Invariants_test.cpp | 42 +++++++++++++++++----- src/xrpld/app/tx/detail/InvariantCheck.cpp | 24 +++++++++---- src/xrpld/app/tx/detail/InvariantCheck.h | 9 ++++- 3 files changed, 59 insertions(+), 16 deletions(-) diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 2e25c63ce7..3898bb8a4a 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -159,9 +159,10 @@ class Invariants_test : public beast::unit_test::suite {{"an account root was deleted"}}, [](Account const& A1, Account const&, ApplyContext& ac) { // remove an account from the view - auto const sle = ac.view().peek(keylet::account(A1.id())); + auto sle = ac.view().peek(keylet::account(A1.id())); if (!sle) return false; + sle->at(sfBalance) = beast::zero; ac.view().erase(sle); return true; }); @@ -185,10 +186,12 @@ class Invariants_test : public beast::unit_test::suite {{"account deletion succeeded but deleted multiple accounts"}}, [](Account const& A1, Account const& A2, ApplyContext& ac) { // remove two accounts from the view - auto const sleA1 = ac.view().peek(keylet::account(A1.id())); - auto const sleA2 = ac.view().peek(keylet::account(A2.id())); + auto sleA1 = ac.view().peek(keylet::account(A1.id())); + auto sleA2 = ac.view().peek(keylet::account(A2.id())); if (!sleA1 || !sleA2) return false; + sleA1->at(sfBalance) = beast::zero; + sleA2->at(sfBalance) = beast::zero; ac.view().erase(sleA1); ac.view().erase(sleA2); return true; @@ -203,6 +206,23 @@ class Invariants_test : public beast::unit_test::suite using namespace test::jtx; testcase << "account root deletion left artifact"; + doInvariantCheck( + {{"account deletion left behind a non-zero balance"}}, + [&](Account const& A1, Account const& A2, ApplyContext& ac) { + // Add an object to the ledger for account A1, then delete + // A1 + auto const a1 = A1.id(); + auto const sleA1 = ac.view().peek(keylet::account(a1)); + if (!sleA1) + return false; + + ac.view().erase(sleA1); + + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); + for (auto const& keyletInfo : directAccountKeylets) { // TODO: Use structured binding once LLVM 16 is the minimum @@ -223,29 +243,31 @@ class Invariants_test : public beast::unit_test::suite // Add an object to the ledger for account A1, then delete // A1 auto const a1 = A1.id(); - auto const sleA1 = ac.view().peek(keylet::account(a1)); + auto sleA1 = ac.view().peek(keylet::account(a1)); if (!sleA1) return false; auto const key = std::invoke(keyletfunc, a1); auto const newSLE = std::make_shared(key); ac.view().insert(newSLE); + sleA1->at(sfBalance) = beast::zero; ac.view().erase(sleA1); return true; }, XRPAmount{}, STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); - }; + } // NFT special case doInvariantCheck( {{"account deletion left behind a NFTokenPage object"}}, [&](Account const& A1, Account const&, ApplyContext& ac) { // remove an account from the view - auto const sle = ac.view().peek(keylet::account(A1.id())); + auto sle = ac.view().peek(keylet::account(A1.id())); if (!sle) return false; + sle->at(sfBalance) = beast::zero; ac.view().erase(sle); return true; }, @@ -269,13 +291,14 @@ class Invariants_test : public beast::unit_test::suite [&](Account const& A1, Account const& A2, ApplyContext& ac) { // Delete the AMM account without cleaning up the directory or // deleting the AMM object - auto const sle = ac.view().peek(keylet::account(ammAcctID)); + auto sle = ac.view().peek(keylet::account(ammAcctID)); if (!sle) return false; BEAST_EXPECT(sle->at(~sfAMMID)); BEAST_EXPECT(sle->at(~sfAMMID) == ammKey); + sle->at(sfBalance) = beast::zero; ac.view().erase(sle); return true; @@ -298,7 +321,7 @@ class Invariants_test : public beast::unit_test::suite // Delete all the AMM's trust lines, remove the AMM from the AMM // account's directory (this deletes the directory), and delete // the AMM account. Do not delete the AMM object. - auto const sle = ac.view().peek(keylet::account(ammAcctID)); + auto sle = ac.view().peek(keylet::account(ammAcctID)); if (!sle) return false; @@ -338,6 +361,7 @@ class Invariants_test : public beast::unit_test::suite !ac.view().exists(ownerDirKeylet) || ac.view().emptyDirDelete(ownerDirKeylet)); + sle->at(sfBalance) = beast::zero; ac.view().erase(sle); return true; @@ -1628,7 +1652,7 @@ class Invariants_test : public beast::unit_test::suite doInvariantCheck( {{"Loan Broker with zero OwnerCount has multiple directory " "pages"}}, - [&loanBrokerKeylet, &setupTest, this]( + [&setupTest, this]( Account const& A1, Account const& A2, ApplyContext& ac) { auto test = setupTest(A1, A2, ac); if (!test || !test->first || !test->second) diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 5a187f16d2..7e6925745a 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -415,10 +415,10 @@ void AccountRootsDeletedClean::visitEntry( bool isDelete, std::shared_ptr const& before, - std::shared_ptr const&) + std::shared_ptr const& after) { if (isDelete && before && before->getType() == ltACCOUNT_ROOT) - accountsDeleted_.emplace_back(before); + accountsDeleted_.emplace_back(before, after); } bool @@ -462,9 +462,21 @@ AccountRootsDeletedClean::finalize( return false; }; - for (auto const& accountSLE : accountsDeleted_) + for (auto const& [before, after] : accountsDeleted_) { - auto const accountID = accountSLE->getAccountID(sfAccount); + auto const accountID = before->getAccountID(sfAccount); + // An account should not be deleted with a balance + if (after->at(sfBalance) != beast::zero) + { + JLOG(j.fatal()) << "Invariant failed: account deletion left " + "behind a non-zero balance"; + XRPL_ASSERT( + enforce, + "ripple::AccountRootsDeletedClean::finalize : " + "account deletion left behind a non-zero balance"); + if (enforce) + return false; + } // Simple types for (auto const& [keyletfunc, _, __] : directAccountKeylets) { @@ -490,9 +502,9 @@ AccountRootsDeletedClean::finalize( // Keys directly stored in the AccountRoot object for (auto const& field : getPseudoAccountFields()) { - if (accountSLE->isFieldPresent(*field)) + if (before->isFieldPresent(*field)) { - auto const key = accountSLE->getFieldH256(*field); + auto const key = before->getFieldH256(*field); if (objectExists(keylet::unchecked(key)) && enforce) return false; } diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index 7be27d8a6c..0d98e4e2c7 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/InvariantCheck.h @@ -174,7 +174,14 @@ public: */ class AccountRootsDeletedClean { - std::vector> accountsDeleted_; + // Pair is . Before is used for most of the checks, so that + // if, for example, and object ID field is cleared, but the object is not + // deleted, it can still be found. After is used specifically for any checks + // that are expected as part of the deletion, such as zeroing out the + // balance. + std::vector< + std::pair, std::shared_ptr>> + accountsDeleted_; public: void