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

@@ -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

src/ripple/app/tx/impl/InvariantCheck.cpp

@@ -358,6 +358,115 @@ 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)
+{
+    // 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<Keylet(AccountID const& id)>;
+    static std::array<fType, 4> 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,

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<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 @@ class ValidClawback
 using InvariantChecks = std::tuple<
     TransactionFeeCheck,
     AccountRootsNotDeleted,
+    AccountRootsDeletedClean,
     LedgerEntryTypesMatch,
     XRPBalanceChecks,
     XRPNotCreated,

src/ripple/app/tx/impl/details/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());
 

src/test/ledger/Invariants_test.cpp

@@ -24,12 +24,23 @@
 #include <ripple/protocol/STLedgerEntry.h>
 #include <boost/algorithm/string/predicate.hpp>
 #include <test/jtx.h>
+#include <test/jtx/AMM.h>
 #include <test/jtx/Env.h>
 
 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<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.
@@ -44,16 +55,18 @@ class Invariants_test : public beast::unit_test::suite
         Precheck const& precheck,
         XRPAmount fee = XRPAmount{},
         STTx tx = STTx{ttACCOUNT_SET, [](STObject&) {}},
-        std::initializer_list<TER> ters = {
-            tecINVARIANT_FAILED,
-            tefINVARIANT_FAILED})
+        std::initializer_list<TER> ters =
+            {tecINVARIANT_FAILED, tefINVARIANT_FAILED},
+        Preclose const& preclose = {})
     {
         using namespace test::jtx;
         Env env{*this};
 
         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<Keylet(AccountID const& id)>;
+        static std::map<std::string, fType> 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<SLE>(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();