Skip to content

Commit

Permalink
Invariant: prevent a deleted account from leaving (most) artifacts on…
Browse files Browse the repository at this point in the history 
… the ledger. (XRPLF#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 XRPLF#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 <[email protected]>
  • Loading branch information
@ximinez
ximinez and Pretty Printer authored Jul 5, 2024
1 parent 7a1b238 commit a17ccca
Show file tree
Hide file tree
Showing 9 changed files with 350 additions and 17 deletions.
1 change: 1 addition & 0 deletions cmake/RippledCore.cmake
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
3 changes: 2 additions & 1 deletion include/xrpl/protocol/Feature.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
21 changes: 21 additions & 0 deletions include/xrpl/protocol/Indexes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
3 changes: 3 additions & 0 deletions src/libxrpl/protocol/Feature.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
216 changes: 203 additions & 13 deletions src/test/ledger/Invariants_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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.
Expand All @@ -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 = {
tecINVARIANT_FAILED,
tefINVARIANT_FAILED})
std::initializer_list<TER> 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()};
Expand Down Expand Up @@ -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()
{
Expand All @@ -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;
});
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -467,6 +656,7 @@ class Invariants_test : public beast::unit_test::suite
{
testXRPNotCreated();
testAccountRootsNotRemoved();
testAccountRootsDeletedClean();
testTypesMatch();
testNoXRPTrustLine();
testXRPBalanceCheck();
Expand Down
6 changes: 3 additions & 3 deletions src/xrpld/app/misc/FeeVoteImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading

0 comments on commit a17ccca

Please sign in to comment.