Add an Invariant check that a deleted account has no traceable objects left on the ledger.

[ximinez]

Summary

Add a new class to InvariantCheck.h/cpp that finds any deleted AccountRoots (there should only be one, of course) and attempts to read the directory root, and any other objects that can be accessed directly (e.g. the SignerList, NFT page... that might be all of them).

Motivation

The first version of AMM (#4294) support merged to develop did not delete the AMM AccountRoot correctly. Fortunately, this was discovered and reported, and a fix is in progress (#4626). During the review of the second PR, it occurred to me that this issue would have been avoided if there was an invariant to verify if a deleted account has no "artifacts" left on the ledger. (#4626 (comment))

Solution

Create a new invariant class (possible name CompletelyDeletedAccount). Use the visitEntry function to build a vector of deleted AccountRoots (There should only be one, but that's checked by the existing AccountRootsNotDeleted class). Use the finalize function to attempt to read any directly accessible objects from the view. "Directly accessible" in this case means any object that can be addressed (via a Keylet) using only the sfAccountID. That includes, but is not necessarily limited to:

• Keylet ownerDir(AccountID const& id) noexcept;
• Keylet signers(AccountID const& account) noexcept;
• Keylet nftpage_min(AccountID const& owner);
• Keylet nftpage_max(AccountID const& owner);

Edit: And don't forget to check the for an AMM object if AMMID is populated.

[intelliot]

After the proposed invariant check has been added, what would occur if a transaction attempts to violate the invariant?

[ximinez]

After the proposed invariant check has been added, what would occur if a transaction attempts to violate the invariant?

It would get a tecINVARIANT_FAILED result, doing nothing but claim a fee, which would raise alarm bells all over the place. https://github.com/XRPLF/rippled/blob/develop/src/ripple/app/tx/impl/Transactor.cpp#L923-L949

[intelliot]

tecINVARIANT_FAILED makes sense. Could you be more specific about the alarm bells? I'm not sure where those are implemented, and where exactly they show up. I think we may want to add some alerting, at minimum for us internally, but potentially for the broader community as well.

[ximinez]

Note to self: rewrite the NFT page lookups to use succ. See also: RPC::getAccountObjects.