From f3b12db9396403af8a824ce1aa30f2572a1d2f46 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 11 Aug 2023 16:47:46 -0400 Subject: [PATCH] Adds invariant AccountRootsDeletedClean: * Checks that a deleted account doesn't leave any directly accessible artifacts behind. * Always tests, but only changes the transaction result if featureInvariantsV1_1 is enabled. * Unit tests. * Resolves #4638 --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/tx/impl/InvariantCheck.cpp | 109 ++++++++++++ src/ripple/app/tx/impl/InvariantCheck.h | 31 ++++ .../app/tx/impl/details/NFTokenUtils.cpp | 1 + src/test/ledger/Invariants_test.cpp | 155 +++++++++++++++++- 5 files changed, 294 insertions(+), 3 deletions(-) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 8d2ff6cbaef..3b0be147790 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1125,5 +1125,6 @@ if (tests) # these two seem to produce conflicts in beast teardown template methods src/test/rpc/ValidatorRPC_test.cpp src/test/rpc/ShardArchiveHandler_test.cpp + src/test/ledger/Invariants_test.cpp PROPERTIES SKIP_UNITY_BUILD_INCLUSION TRUE) endif () #tests diff --git a/src/ripple/app/tx/impl/InvariantCheck.cpp b/src/ripple/app/tx/impl/InvariantCheck.cpp index 7b1ac7d08df..1ace02fa5a1 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -358,6 +358,115 @@ AccountRootsNotDeleted::finalize( //------------------------------------------------------------------------------ +void +AccountRootsDeletedClean::visitEntry( + bool isDelete, + std::shared_ptr const& before, + std::shared_ptr const&) +{ + if (isDelete && before && before->getType() == ltACCOUNT_ROOT) + accountsDeleted_.emplace_back(before); +} + +bool +AccountRootsDeletedClean::finalize( + STTx const& tx, + TER const result, + XRPAmount const, + ReadView const& view, + beast::Journal const& j) +{ + // This list should include all of the keylet functions that take a single + // AccountID parameter, except nftpage_max, because it is exercised below + // using the NFT page lookup logic. (nftpage_min is included because it + // should _never_ be present.) + using fType = std::function; + static std::array const keyletfuncs{ + &keylet::account, + &keylet::ownerDir, + &keylet::signers, + &keylet::nftpage_min, + //&keylet::nftpage_max + }; + + // Always check for objects in the ledger, but to prevent differing + // transaction processing results, however unlikely, only fail if the + // feature is enabled. Enabled, or not, though, a fatal-level message will + // be logged + bool const enforce = view.rules().enabled(featureInvariantsV1_1); + + auto const noObject = [&](auto const& keylet) { + // It may be a little unintuitive, but this function follows the same + // return paradigm as the `finalize` function. True if the check passes + // (object is NOT found). False if it fails (object IS found). + if (auto const sle = view.read(keylet)) + { + // Finding the object is bad + auto const typeName = [&]() { + 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"; + return false; + } + return true; + }; + + for (auto const& accountSLE : accountsDeleted_) + { + auto const accountID = accountSLE->getAccountID(sfAccount); + // Simple types + for (auto const& keyletfunc : keyletfuncs) + { + if (!noObject(std::invoke(keyletfunc, accountID))) + { + assert(enforce); + if (enforce) + return false; + } + } + + // NFT pages + { + Keylet const first = keylet::nftpage_min(accountID); + Keylet const last = keylet::nftpage_max(accountID); + + uint256 key = + view.succ(first.key, last.key.next()).value_or(last.key); + + // current page + if (!noObject(Keylet{ltNFTOKEN_PAGE, key})) + { + assert(enforce); + if (enforce) + return false; + } + } + + // Keys directly stored in the AccountRoot object + if (auto const ammKey = accountSLE->at(~sfAMMID)) + { + if (!noObject(keylet::amm(*ammKey))) + { + assert(enforce); + if (enforce) + return false; + } + } + } + + return true; +} + +//------------------------------------------------------------------------------ + void LedgerEntryTypesMatch::visitEntry( bool, diff --git a/src/ripple/app/tx/impl/InvariantCheck.h b/src/ripple/app/tx/impl/InvariantCheck.h index eb606c2ed3b..59876f3b68b 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.h +++ b/src/ripple/app/tx/impl/InvariantCheck.h @@ -164,6 +164,36 @@ class AccountRootsNotDeleted beast::Journal const&); }; +/** + * @brief Invariant: a deleted account must not have any objects left + * + * We iterate all deleted account roots, and ensure that there are no + * objects left that are directly accessible with that account's ID. + * + * There should only be one deleted account, but that's checked by + * AccountRootsNotDeleted. This invariant will handle multiple deleted account + * roots without a problem. + */ +class AccountRootsDeletedClean +{ + std::vector> accountsDeleted_; + +public: + void + visitEntry( + bool, + std::shared_ptr const&, + std::shared_ptr const&); + + bool + finalize( + STTx const&, + TER const, + XRPAmount const, + ReadView const&, + beast::Journal const&); +}; + /** * @brief Invariant: An account XRP balance must be in XRP and take a value * between 0 and INITIAL_XRP drops, inclusive. @@ -423,6 +453,7 @@ class ValidClawback using InvariantChecks = std::tuple< TransactionFeeCheck, AccountRootsNotDeleted, + AccountRootsDeletedClean, LedgerEntryTypesMatch, XRPBalanceChecks, XRPNotCreated, diff --git a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp index 09ff8f13caa..ea5a111372d 100644 --- a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp +++ b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp @@ -191,6 +191,7 @@ getPageForToken( : carr[0].getFieldH256(sfNFTokenID); auto np = std::make_shared(keylet::nftpage(base, tokenIDForNewPage)); + assert(np->key() > base.key); np->setFieldArray(sfNFTokens, narr); np->setFieldH256(sfNextPageMin, cp->key()); diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 898376bdaf7..9d385673945 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -24,12 +24,23 @@ #include #include #include +#include #include namespace ripple { class Invariants_test : public beast::unit_test::suite { + // this type defines a method for additional setup beyond creating two + // accounts. The preclose function is used to run additional transactions on + // the Env needed to set up the test, after the accounts are created, but + // before the ledger is closed, and a Precheck is called. It is not commonly + // used. + using Preclose = std::function; + // this is common setup/method for running a failing invariant check. The // precheck function is used to manipulate the ApplyContext with view // changes that will cause the check to fail. @@ -44,9 +55,9 @@ class Invariants_test : public beast::unit_test::suite Precheck const& precheck, XRPAmount fee = XRPAmount{}, STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}}, - std::initializer_list ters = { - tecINVARIANT_FAILED, - tefINVARIANT_FAILED}) + std::initializer_list ters = + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + Preclose const& preclose = {}) { using namespace test::jtx; Env env{*this}; @@ -54,6 +65,8 @@ class Invariants_test : public beast::unit_test::suite Account A1{"A1"}; Account A2{"A2"}; env.fund(XRP(1000), A1, A2); + if (preclose) + BEAST_EXPECT(preclose(A1, A2, env)); env.close(); OpenView ov{*env.current()}; @@ -162,6 +175,141 @@ class Invariants_test : public beast::unit_test::suite STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); } + void + testAccountRootsDeletedClean() + { + using namespace test::jtx; + testcase << "account root deletion left artifact"; + + // This list should include all of the keylet functions that take a + // single AccountID parameter + using fType = std::function; + static std::map const keyletfuncs{ + // Skip the account root, since that's what's being deleted. + //{"AccountRoot", &keylet::account}, + {"DirectoryNode", &keylet::ownerDir}, + {"SignerList", &keylet::signers}, + // It's normally impossible to create an item at nftpage_min, but + // test it anyway, since the invariant checks for it. + {"NFTokenPage", &keylet::nftpage_min}, + {"NFTokenPage", &keylet::nftpage_max}}; + + for (auto const& item : keyletfuncs) + { + auto const& type = item.first; + fType const& keyletfunc = item.second; + doInvariantCheck( + {{"account deletion left behind a " + type + " object"}}, + [&](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; + + auto const key = std::invoke(keyletfunc, a1); + auto const newSLE = std::make_shared(key); + ac.view().insert(newSLE); + ac.view().erase(sleA1); + + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); + }; + + // AMM special cases + AccountID ammAcctID; + uint256 ammKey; + Issue ammIssue; + doInvariantCheck( + {{"account deletion left behind a DirectoryNode object"}}, + [&](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)); + if (!sle) + return false; + + BEAST_EXPECT(sle->at(~sfAMMID)); + BEAST_EXPECT(sle->at(~sfAMMID) == ammKey); + + ac.view().erase(sle); + + return true; + }, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&](Account const& A1, Account const& A2, Env& env) { + AMM amm(env, A1, XRP(100), A1["USD"](50)); + ammAcctID = amm.ammAccount(); + ammKey = amm.ammID(); + ammIssue = amm.lptIssue(); + return true; + }); + doInvariantCheck( + {{"account deletion left behind a AMM object"}}, + [&](Account const& A1, Account const& A2, ApplyContext& ac) { + // Delete the AMM account, cleaning up the directory, but + // without deleting the AMM object + auto const sle = ac.view().peek(keylet::account(ammAcctID)); + if (!sle) + return false; + + BEAST_EXPECT(sle->at(~sfAMMID)); + BEAST_EXPECT(sle->at(~sfAMMID) == ammKey); + + for (auto const trustKeylet : + {keylet::line(ammAcctID, A1["USD"]), + keylet::line(A1, ammIssue)}) + { + if (auto const line = ac.view().peek(trustKeylet); !line) + { + return false; + } + else + { + STAmount lowLimit = line->at(sfLowLimit); + STAmount highLimit = line->at(sfHighLimit); + BEAST_EXPECT( + trustDelete( + ac.view(), + line, + lowLimit.getIssuer(), + highLimit.getIssuer(), + ac.journal) == tesSUCCESS); + } + } + + auto const ammSle = ac.view().peek(keylet::amm(ammKey)); + if (!BEAST_EXPECT(ammSle)) + return false; + auto const ownerDirKeylet = keylet::ownerDir(ammAcctID); + + BEAST_EXPECT(ac.view().dirRemove( + ownerDirKeylet, ammSle->at(sfOwnerNode), ammKey, false)); + BEAST_EXPECT( + !ac.view().exists(ownerDirKeylet) || + ac.view().emptyDirDelete(ownerDirKeylet)); + + ac.view().erase(sle); + + return true; + }, + XRPAmount{}, + STTx{ttAMM_WITHDRAW, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&](Account const& A1, Account const& A2, Env& env) { + AMM amm(env, A1, XRP(100), A1["USD"](50)); + ammAcctID = amm.ammAccount(); + ammKey = amm.ammID(); + ammIssue = amm.lptIssue(); + return true; + }); + } + void testTypesMatch() { @@ -467,6 +615,7 @@ class Invariants_test : public beast::unit_test::suite { testXRPNotCreated(); testAccountRootsNotRemoved(); + testAccountRootsDeletedClean(); testTypesMatch(); testNoXRPTrustLine(); testXRPBalanceCheck();