ARCHIVE/xahaud
3
0
Fork 0
mirror of https://github.com/Xahau/xahaud.git synced 2025-11-18 17:45:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
* 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>
This commit is contained in:
Ed Hennis
2024-07-05 13:27:15 -04:00
committed by tequ
parent 3e95f07a25
commit 1dee7d6c4d
9 changed files with 350 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
cmake/RippledCore.cmake
View File

@@ -153,6 +153,7 @@ 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
include/xrpl/protocol/Feature.h
View File

@@ -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 = 100;
static constexpr std::size_t numFeatures = 101;
/** Amendments that this server supports and the default voting behavior.
Whether they are enabled depends on the Rules defined in the validated
@@ -394,6 +394,7 @@ extern uint256 const featureNFTokenMintOffer;
extern uint256 const fixReducedOffersV2;
extern uint256 const fixEnforceNFTokenTrustline;
extern uint256 const fixInnerObjTemplate2;
extern uint256 const featureInvariantsV1_1;
} // namespace ripple

21
include/xrpl/protocol/Indexes.h
View File

@@ -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 {
@@ -340,6 +341,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
src/libxrpl/protocol/Feature.cpp
View File

@@ -520,6 +520,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.

216
src/test/ledger/Invariants_test.cpp
View File

@@ -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 = {
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()};
@@ -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;
@@ -405,7 +594,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);
@@ -417,13 +606,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);
@@ -435,7 +624,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);
@@ -452,6 +641,7 @@ public:
{
testXRPNotCreated();
testAccountRootsNotRemoved();
testAccountRootsDeletedClean();
testTypesMatch();
testNoXRPTrustLine();
testXRPBalanceCheck();

6
src/xrpld/app/misc/FeeVoteImpl.cpp
View File

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

85
src/xrpld/app/tx/detail/InvariantCheck.cpp
View File

@@ -471,6 +471,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,

31
src/xrpld/app/tx/detail/InvariantCheck.h
View File

@@ -165,6 +165,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.
@@ -424,6 +454,7 @@ public:
using InvariantChecks = std::tuple<
TransactionFeeCheck,
AccountRootsNotDeleted,
AccountRootsDeletedClean,
LedgerEntryTypesMatch,
XRPBalanceChecks,
XRPNotCreated,

1
src/xrpld/app/tx/detail/NFTokenUtils.cpp
View File

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