diff --git a/cmake/RippledCore.cmake b/cmake/RippledCore.cmake index 9af3303f662..cc483034714 100644 --- a/cmake/RippledCore.cmake +++ b/cmake/RippledCore.cmake @@ -142,5 +142,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() diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 49f94b88d86..7eec46e89eb 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -80,7 +80,7 @@ namespace detail { // Feature.cpp. Because it's only used to reserve storage, and determine how // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than // the actual number of amendments. A LogicError on startup will verify this. -static constexpr std::size_t numFeatures = 77; +static constexpr std::size_t numFeatures = 78; /** Amendments that this server supports and the default voting behavior. Whether they are enabled depends on the Rules defined in the validated @@ -370,6 +370,7 @@ extern uint256 const featureNFTokenMintOffer; extern uint256 const fixReducedOffersV2; extern uint256 const fixEnforceNFTokenTrustline; extern uint256 const fixInnerObjTemplate2; +extern uint256 const featureInvariantsV1_1; } // namespace ripple diff --git a/include/xrpl/protocol/Indexes.h b/include/xrpl/protocol/Indexes.h index d57525121de..f179bbacfab 100644 --- a/include/xrpl/protocol/Indexes.h +++ b/include/xrpl/protocol/Indexes.h @@ -29,6 +29,7 @@ #include #include #include +#include #include namespace ripple { @@ -306,6 +307,26 @@ getTicketIndex(AccountID const& account, std::uint32_t uSequence); uint256 getTicketIndex(AccountID const& account, SeqProxy ticketSeq); +template +struct keyletDesc +{ + std::function function; + Json::StaticString expectedLEName; + bool includeInTests; +}; + +// This list should include all of the keylet functions that take a single +// AccountID parameter. +std::array, 6> const directAccountKeylets{ + {{&keylet::account, jss::AccountRoot, false}, + {&keylet::ownerDir, jss::DirectoryNode, true}, + {&keylet::signers, jss::SignerList, true}, + // It's normally impossible to create an item at nftpage_min, but + // test it anyway, since the invariant checks for it. + {&keylet::nftpage_min, jss::NFTokenPage, true}, + {&keylet::nftpage_max, jss::NFTokenPage, true}, + {&keylet::did, jss::DID, true}}}; + } // namespace ripple #endif diff --git a/src/libxrpl/protocol/Feature.cpp b/src/libxrpl/protocol/Feature.cpp index 8a978955086..87395b7e189 100644 --- a/src/libxrpl/protocol/Feature.cpp +++ b/src/libxrpl/protocol/Feature.cpp @@ -497,6 +497,9 @@ REGISTER_FEATURE(NFTokenMintOffer, Supported::yes, VoteBehavior::De REGISTER_FIX (fixReducedOffersV2, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixEnforceNFTokenTrustline, Supported::yes, VoteBehavior::DefaultNo); REGISTER_FIX (fixInnerObjTemplate2, Supported::yes, VoteBehavior::DefaultNo); +// InvariantsV1_1 will be changes to Supported::yes when all the +// invariants expected to be included under it are complete. +REGISTER_FEATURE(InvariantsV1_1, Supported::no, VoteBehavior::DefaultNo); // The following amendments are obsolete, but must remain supported // because they could potentially get enabled. diff --git a/src/test/ledger/Invariants_test.cpp b/src/test/ledger/Invariants_test.cpp index 1fd1543f18b..66523700a88 100644 --- a/src/test/ledger/Invariants_test.cpp +++ b/src/test/ledger/Invariants_test.cpp @@ -18,6 +18,7 @@ //============================================================================== #include +#include #include #include #include @@ -30,6 +31,15 @@ namespace ripple { class Invariants_test : public beast::unit_test::suite { + // The optional Preclose function is used to process additional transactions + // on the ledger after creating two accounts, but before closing it, and + // before the Precheck function. These should only be valid functions, and + // not direct manipulations. Preclose 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. @@ -38,22 +48,42 @@ class Invariants_test : public beast::unit_test::suite test::jtx::Account const& b, ApplyContext& ac)>; + /** Run a specific test case to put the ledger into a state that will be + * detected by an invariant. Simulates the actions of a transaction that + * would violate an invariant. + * + * @param expect_logs One or more messages related to the failing invariant + * that should be in the log output + * @precheck See "Precheck" above + * @fee If provided, the fee amount paid by the simulated transaction. + * @tx A mock transaction that took the actions to trigger the invariant. In + * most cases, only the type matters. + * @ters The TER results expected on the two passes of the invariant + * checker. + * @preclose See "Preclose" above. Note that @preclose runs *before* + * @precheck, but is the last parameter for historical reasons + * + */ 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}; + FeatureBitset amendments = + supported_amendments() | featureInvariantsV1_1; + Env env{*this, amendments}; - Account A1{"A1"}; - Account A2{"A2"}; + Account const A1{"A1"}; + Account const A2{"A2"}; env.fund(XRP(1000), A1, A2); + if (preclose) + BEAST_EXPECT(preclose(A1, A2, env)); env.close(); OpenView ov{*env.current()}; @@ -162,6 +192,165 @@ 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"; + + for (auto const& keyletInfo : directAccountKeylets) + { + // TODO: Use structured binding once LLVM 16 is the minimum + // supported version. See also: + // https://github.com/llvm/llvm-project/issues/48582 + // https://github.com/llvm/llvm-project/commit/127bf44385424891eb04cff8e52d3f157fc2cb7c + if (!keyletInfo.includeInTests) + continue; + auto const& keyletfunc = keyletInfo.function; + auto const& type = keyletInfo.expectedLEName; + + using namespace std::string_literals; + + doInvariantCheck( + {{"account deletion left behind a "s + type.c_str() + + " 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) {}}); + }; + + // 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())); + if (!sle) + return false; + ac.view().erase(sle); + return true; + }, + XRPAmount{}, + STTx{ttACCOUNT_DELETE, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + [&](Account const& A1, Account const&, Env& env) { + // Preclose callback to mint the NFT which will be deleted in + // the Precheck callback above. + env(token::mint(A1)); + + return true; + }); + + // 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) { + // Preclose callback to create the AMM which will be partially + // deleted in the Precheck callback above. + AMM const 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 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)); + 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 const lowLimit = line->at(sfLowLimit); + STAmount const 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) { + // Preclose callback to create the AMM which will be partially + // deleted in the Precheck callback above. + AMM const amm(env, A1, XRP(100), A1["USD"](50)); + ammAcctID = amm.ammAccount(); + ammKey = amm.ammID(); + ammIssue = amm.lptIssue(); + return true; + }); + } + void testTypesMatch() { @@ -175,7 +364,7 @@ class Invariants_test : public beast::unit_test::suite auto const sle = ac.view().peek(keylet::account(A1.id())); if (!sle) return false; - auto sleNew = std::make_shared(ltTICKET, sle->key()); + auto const sleNew = std::make_shared(ltTICKET, sle->key()); ac.rawView().rawReplace(sleNew); return true; }); @@ -191,7 +380,7 @@ class Invariants_test : public beast::unit_test::suite // make a dummy escrow ledger entry, then change the type to an // unsupported value so that the valid type invariant check // will fail. - auto sleNew = std::make_shared( + auto const sleNew = std::make_shared( keylet::escrow(A1, (*sle)[sfSequence] + 2)); // We don't use ltNICKNAME directly since it's marked deprecated @@ -231,7 +420,7 @@ class Invariants_test : public beast::unit_test::suite auto const sle = ac.view().peek(keylet::account(A1.id())); if (!sle) return false; - STAmount nonNative(A2["USD"](51)); + STAmount const nonNative(A2["USD"](51)); sle->setFieldAmount(sfBalance, nonNative); ac.view().update(sle); return true; @@ -420,7 +609,7 @@ class Invariants_test : public beast::unit_test::suite [](Account const&, Account const&, ApplyContext& ac) { // Insert a new account root created by a non-payment into // the view. - const Account A3{"A3"}; + Account const A3{"A3"}; Keylet const acctKeylet = keylet::account(A3); auto const sleNew = std::make_shared(acctKeylet); ac.view().insert(sleNew); @@ -432,13 +621,13 @@ class Invariants_test : public beast::unit_test::suite [](Account const&, Account const&, ApplyContext& ac) { // Insert two new account roots into the view. { - const Account A3{"A3"}; + Account const A3{"A3"}; Keylet const acctKeylet = keylet::account(A3); auto const sleA3 = std::make_shared(acctKeylet); ac.view().insert(sleA3); } { - const Account A4{"A4"}; + Account const A4{"A4"}; Keylet const acctKeylet = keylet::account(A4); auto const sleA4 = std::make_shared(acctKeylet); ac.view().insert(sleA4); @@ -450,7 +639,7 @@ class Invariants_test : public beast::unit_test::suite {{"account created with wrong starting sequence number"}}, [](Account const&, Account const&, ApplyContext& ac) { // Insert a new account root with the wrong starting sequence. - const Account A3{"A3"}; + Account const A3{"A3"}; Keylet const acctKeylet = keylet::account(A3); auto const sleNew = std::make_shared(acctKeylet); sleNew->setFieldU32(sfSequence, ac.view().seq() + 1); @@ -467,6 +656,7 @@ class Invariants_test : public beast::unit_test::suite { testXRPNotCreated(); testAccountRootsNotRemoved(); + testAccountRootsDeletedClean(); testTypesMatch(); testNoXRPTrustLine(); testXRPBalanceCheck(); diff --git a/src/xrpld/app/misc/FeeVoteImpl.cpp b/src/xrpld/app/misc/FeeVoteImpl.cpp index af57314ef6d..cb4e57b0f73 100644 --- a/src/xrpld/app/misc/FeeVoteImpl.cpp +++ b/src/xrpld/app/misc/FeeVoteImpl.cpp @@ -277,9 +277,9 @@ FeeVoteImpl::doVoting( } // choose our positions - // TODO: Use structured binding once LLVM issue - // https://github.com/llvm/llvm-project/issues/48582 - // is fixed. + // TODO: Use structured binding once LLVM 16 is the minimum supported + // version. See also: https://github.com/llvm/llvm-project/issues/48582 + // https://github.com/llvm/llvm-project/commit/127bf44385424891eb04cff8e52d3f157fc2cb7c auto const baseFee = baseFeeVote.getVotes(); auto const baseReserve = baseReserveVote.getVotes(); auto const incReserve = incReserveVote.getVotes(); diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index d1d5890bec3..70210b90d75 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -358,6 +358,91 @@ 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) +{ + // 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 objectExists = [&view, enforce, &j](auto const& keylet) { + 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"; + (void)enforce; + assert(enforce); + return true; + } + return false; + }; + + for (auto const& accountSLE : accountsDeleted_) + { + auto const accountID = accountSLE->getAccountID(sfAccount); + // Simple types + for (auto const& [keyletfunc, _, __] : directAccountKeylets) + { + if (objectExists(std::invoke(keyletfunc, accountID)) && enforce) + return false; + } + + { + // NFT pages. ntfpage_min and nftpage_max were already explicitly + // checked above as entries in directAccountKeylets. This uses + // view.succ() to check for any NFT pages in between the two + // endpoints. + Keylet const first = keylet::nftpage_min(accountID); + Keylet const last = keylet::nftpage_max(accountID); + + std::optional key = view.succ(first.key, last.key.next()); + + // current page + if (key && objectExists(Keylet{ltNFTOKEN_PAGE, *key}) && enforce) + return false; + } + + // Keys directly stored in the AccountRoot object + if (auto const ammKey = accountSLE->at(~sfAMMID)) + { + if (objectExists(keylet::amm(*ammKey)) && enforce) + return false; + } + } + + return true; +} + +//------------------------------------------------------------------------------ + void LedgerEntryTypesMatch::visitEntry( bool, diff --git a/src/xrpld/app/tx/detail/InvariantCheck.h b/src/xrpld/app/tx/detail/InvariantCheck.h index ef89569cb7c..6a83f5c9b7b 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.h +++ b/src/xrpld/app/tx/detail/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/xrpld/app/tx/detail/NFTokenUtils.cpp b/src/xrpld/app/tx/detail/NFTokenUtils.cpp index b2ed29714df..342b3805a0e 100644 --- a/src/xrpld/app/tx/detail/NFTokenUtils.cpp +++ b/src/xrpld/app/tx/detail/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());