Invariant: prevent a deleted account from leaving (most) artifacts on the ledger. (#4663)

* Add feature / amendment "InvariantsV1_1"

* 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

* [FOLD] Review feedback from @gregtatcam:

* Fix unused variable warning
* Improve Invariant test const correctness

* [FOLD] Review feedback from @mvadari:

* Centralize the account keylet function list, and some optimization

* [FOLD] Some structured binding doesn't work in clang

* [FOLD] Review feedback 2 from @mvadari:

* Clean up and clarify some comments.

* [FOLD] Change InvariantsV1_1 to unsupported

* Will allow multiple PRs to be merged over time using the same amendment.

* fixup! [FOLD] Change InvariantsV1_1 to unsupported

* [FOLD] Update and clarify some comments. No code changes.

* Move CMake directory

* Rearrange sources

* Rewrite includes

* Recompute loops

* Fix merge issue and formatting

---------

Co-authored-by: Pretty Printer <cpp@ripple.com>

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()

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
 

include/xrpl/protocol/Indexes.h

@@ -29,6 +29,7 @@
 #include <xrpl/protocol/STXChainBridge.h>
 #include <xrpl/protocol/Serializer.h>
 #include <xrpl/protocol/UintTypes.h>
+#include <xrpl/protocol/jss.h>
 #include <cstdint>
 
 namespace ripple {
@@ -306,6 +307,26 @@ getTicketIndex(AccountID const& account, std::uint32_t uSequence);
 uint256
 getTicketIndex(AccountID const& account, SeqProxy ticketSeq);
 
+template <class... keyletParams>
+struct keyletDesc
+{
+    std::function<Keylet(keyletParams...)> function;
+    Json::StaticString expectedLEName;
+    bool includeInTests;
+};
+
+// This list should include all of the keylet functions that take a single
+// AccountID parameter.
+std::array<keyletDesc<AccountID const&>, 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

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.

src/test/ledger/Invariants_test.cpp

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include <test/jtx.h>
+#include <test/jtx/AMM.h>
 #include <test/jtx/Env.h>
 #include <xrpld/app/tx/apply.h>
 #include <xrpld/app/tx/detail/ApplyContext.h>
@@ -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<bool(
+        test::jtx::Account const& a,
+        test::jtx::Account const& b,
+        test::jtx::Env& env)>;
+
     // 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<std::string> const& expect_logs,
         Precheck const& precheck,
         XRPAmount fee = XRPAmount{},
         STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}},
-        std::initializer_list<TER> ters = {
+        std::initializer_list<TER> ters =
-            tecINVARIANT_FAILED,
+            {tecINVARIANT_FAILED, tefINVARIANT_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 const A1{"A1"};
-        Account A2{"A2"};
+        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<SLE>(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<SLE>(ltTICKET, sle->key());
+                auto const sleNew = std::make_shared<SLE>(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<SLE>(
+                auto const sleNew = std::make_shared<SLE>(
                     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<SLE>(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<SLE>(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<SLE>(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<SLE>(acctKeylet);
                 sleNew->setFieldU32(sfSequence, ac.view().seq() + 1);
@@ -467,6 +656,7 @@ public:
     {
         testXRPNotCreated();
         testAccountRootsNotRemoved();
+        testAccountRootsDeletedClean();
         testTypesMatch();
         testNoXRPTrustLine();
         testXRPBalanceCheck();

src/xrpld/app/misc/FeeVoteImpl.cpp

@@ -277,9 +277,9 @@ FeeVoteImpl::doVoting(
     }
 
     // choose our positions
-    // TODO: Use structured binding once LLVM issue
+    // TODO: Use structured binding once LLVM 16 is the minimum supported
-    // https://github.com/llvm/llvm-project/issues/48582
+    // version. See also: https://github.com/llvm/llvm-project/issues/48582
-    // is fixed.
+    // https://github.com/llvm/llvm-project/commit/127bf44385424891eb04cff8e52d3f157fc2cb7c
     auto const baseFee = baseFeeVote.getVotes();
     auto const baseReserve = baseReserveVote.getVotes();
     auto const incReserve = incReserveVote.getVotes();

src/xrpld/app/tx/detail/InvariantCheck.cpp

@@ -358,6 +358,91 @@ AccountRootsNotDeleted::finalize(
 
 //------------------------------------------------------------------------------
 
+void
+AccountRootsDeletedClean::visitEntry(
+    bool isDelete,
+    std::shared_ptr<SLE const> const& before,
+    std::shared_ptr<SLE const> 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<uint256> 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,

src/xrpld/app/tx/detail/InvariantCheck.h

@@ -164,6 +164,36 @@ public:
         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<std::shared_ptr<SLE const>> accountsDeleted_;
+
+public:
+    void
+    visitEntry(
+        bool,
+        std::shared_ptr<SLE const> const&,
+        std::shared_ptr<SLE const> 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 @@ public:
 using InvariantChecks = std::tuple<
     TransactionFeeCheck,
     AccountRootsNotDeleted,
+    AccountRootsDeletedClean,
     LedgerEntryTypesMatch,
     XRPBalanceChecks,
     XRPNotCreated,

src/xrpld/app/tx/detail/NFTokenUtils.cpp

@@ -191,6 +191,7 @@ getPageForToken(
         : carr[0].getFieldH256(sfNFTokenID);
 
     auto np = std::make_shared<SLE>(keylet::nftpage(base, tokenIDForNewPage));
+    assert(np->key() > base.key);
     np->setFieldArray(sfNFTokens, narr);
     np->setFieldH256(sfNextPageMin, cp->key());