From da3692c701e8a7669c09030f658ea2ac66205f4f Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 14 Aug 2023 21:04:21 -0400 Subject: [PATCH] Test the new invariant * Resolves #4638 --- Builds/CMake/RippledCore.cmake | 1 + src/ripple/app/tx/impl/InvariantCheck.cpp | 2 +- src/test/ledger/Invariants_test.cpp | 112 +++++++++++++++++++++- 3 files changed, 111 insertions(+), 4 deletions(-) diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 52c78c580d1..a5d468a788a 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -1117,5 +1117,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 6bb6930f52b..07d2043d02b 100644 --- a/src/ripple/app/tx/impl/InvariantCheck.cpp +++ b/src/ripple/app/tx/impl/InvariantCheck.cpp @@ -385,7 +385,7 @@ AccountRootsDeletedClean::finalize( &keylet::nftpage_min, &keylet::nftpage_max}; - static auto const noObject = [&](auto const& keylet) { + 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). diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 898376bdaf7..cd9c5581af1 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include namespace ripple { @@ -38,15 +39,23 @@ class Invariants_test : public beast::unit_test::suite test::jtx::Account const& b, ApplyContext& ac)>; + // this is an uncommon setup 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. + using Preclose = std::function; + void doInvariantCheck( std::vector const& expect_logs, 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 +63,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 +173,100 @@ 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}, + {"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().erase(sleA1); + ac.view().insert(newSLE); + + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}); + }; + + AccountID ammAcctID; + uint256 ammKey; + Issue ammIssue; + doInvariantCheck( + {{"account deletion left behind a AMM object"}}, + [&](Account const& A1, Account const& A2, ApplyContext& ac) { + // Delete the AMM account 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); + } + } + + 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 +572,7 @@ class Invariants_test : public beast::unit_test::suite { testXRPNotCreated(); testAccountRootsNotRemoved(); + testAccountRootsDeletedClean(); testTypesMatch(); testNoXRPTrustLine(); testXRPBalanceCheck();