diff --git a/.cspell.config.yaml b/.cspell.config.yaml index 21b0145f43..cbd77dbf06 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -355,6 +355,7 @@ words: - venv - vfalco - vinnie + - vkeylet - wasmi - wextra - wptr diff --git a/include/xrpl/ledger/helpers/CredentialHelpers.h b/include/xrpl/ledger/helpers/CredentialHelpers.h index bcb4e5d3e2..66be6646da 100644 --- a/include/xrpl/ledger/helpers/CredentialHelpers.h +++ b/include/xrpl/ledger/helpers/CredentialHelpers.h @@ -37,10 +37,10 @@ deleteSLE(ApplyView& view, SLE::ref sleCredential, beast::Journal j); * @brief Remove credentials pinned to a pseudo-account's owner directory. * * Cleans up credentials that were linked to a pseudo-account (Vault, LoanBroker, - * AMM) before fixCleanup3_4_0, which such an account can neither accept nor - * delete. Only credentials are removed, at most @p maxNodesToDelete of them; on - * reaching that bound the result is `tecINCOMPLETE` and the caller must - * propagate it so a later transaction resumes. + * AMM), which such an account can neither accept nor delete. Only credentials + * are removed, at most @p maxNodesToDelete of them; on reaching that bound the + * result is `tecINCOMPLETE` and the caller must propagate it so a later + * transaction resumes. * * @param view Mutable ledger view. * @param pseudoAcct The pseudo-account whose directory is cleaned. diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index b73b2ded45..e8d81f3108 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -690,9 +690,10 @@ deleteAMMTrustLines( return {deleteAMMTrustLine(sb, sleItem, ammAccountID, j), SkipEntry::No}; } - // A credential can be pinned to the AMM pseudo-account before - // fixCleanup3_4_0. Clean it up here, inside the same bounded walk, so - // the pinned AMM can be deleted. See fixCleanup3_4_0. + // A credential naming the pseudo-account as subject can't be + // accepted or deleted by it and would otherwise permanently pin the + // AMM. Clean it up here, inside the same bounded walk, so the + // pinned AMM can still be deleted. if (sb.rules().enabled(fixCleanup3_4_0) && nodeType == ltCREDENTIAL) return {credentials::deleteSLE(sb, sleItem, j), SkipEntry::No}; // LCOV_EXCL_START @@ -773,7 +774,7 @@ deleteAMMAccount(Sandbox& sb, Asset const& asset, Asset const& asset2, beast::Jo } // deleteAMMTrustLines also removes any credentials pinned to the AMM - // pseudo-account before fixCleanup3_4_0, within its bounded walk. + // pseudo-account, within its bounded walk. if (auto const ter = deleteAMMTrustLines(sb, ammAccountID, kMaxDeletableAmmTrustLines, j); !isTesSuccess(ter)) return ter; @@ -915,9 +916,9 @@ isOnlyLiquidityProvider(ReadView const& view, Issue const& ammIssue, AccountID c ++nMPT; continue; } - // A credential can be pinned to the AMM pseudo-account before - // fixCleanup3_4_0. Ignore it here; VaultDelete-style - // cleanup in deleteAMMAccount removes it when the AMM is deleted. + // A credential naming the pseudo-account as subject can be pinned + // to its owner directory. Ignore it here; deleteAMMTrustLines + // removes it when the AMM is deleted. if (view.rules().enabled(fixCleanup3_4_0) && entryType == ltCREDENTIAL) continue; if (entryType != ltRIPPLE_STATE) diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 92fc0894da..c6ca3aa467 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -1448,7 +1448,7 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee) types.insert(ltRIPPLE_STATE); // A bounded pseudo-account credential cleanup (VaultDelete / // LoanBrokerDelete) persists its partial credential deletions so a - // later transaction can resume. See fixCleanup3_4_0. + // later transaction can resume. if (credentialCleanup) types.insert(ltCREDENTIAL); } diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index b27a1f28af..39922f69db 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -232,9 +232,9 @@ ValidMPTIssuance::finalize( if (hasPrivilege(tx, DestroyMptIssuance)) { // A VaultDelete that is still cleaning up credentials pinned to its - // pseudo-account before fixCleanup3_4_0 returns tecINCOMPLETE and has - // not yet reached the share issuance. Don't require the issuance to - // be removed until the deletion completes (a later transaction). + // pseudo-account returns tecINCOMPLETE and has not yet reached the + // share issuance. Don't require the issuance to be removed until + // the deletion completes (a later transaction). if (rules.enabled(fixCleanup3_4_0) && result == tecINCOMPLETE) return mptIssuancesDeleted_ == 0 && mptIssuancesCreated_ == 0; diff --git a/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp b/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp index 3b3cd1c365..5cce1a7de8 100644 --- a/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp +++ b/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp @@ -92,16 +92,6 @@ CredentialCreate::preclaim(PreclaimContext const& ctx) return tecNO_TARGET; } - // A pseudo-account (Vault, LoanBroker, AMM) can't sign, so it can never - // accept or delete a credential issued to it. Such a credential would stay - // pinned in the pseudo-account's owner directory forever and block deletion - // of the owning object (tecHAS_OBLIGATIONS). Reject it up front. - if (ctx.view.rules().enabled(fixCleanup3_4_0) && isPseudoAccount(ctx.view, subject)) - { - JLOG(ctx.j.trace()) << "Subject is a pseudo-account."; - return tecPSEUDO_ACCOUNT; - } - if (ctx.view.exists(keylet::credential(subject, ctx.tx[sfAccount], credType))) { JLOG(ctx.j.trace()) << "Credential already exists."; diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp index 9b862d6b1a..61fe14e8fc 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp @@ -147,7 +147,7 @@ LoanBrokerDelete::doApply() // else. They would otherwise keep its owner directory alive and block // deletion with tecHAS_OBLIGATIONS. Doing it first means a bounded, // tecINCOMPLETE cleanup can be resumed by a later transaction without having - // already torn down the broker. See fixCleanup3_4_0. + // already torn down the broker. if (view().rules().enabled(fixCleanup3_4_0)) { if (auto const ter = credentials::deletePseudoAccountCredentials( diff --git a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp index f579829046..f3a587d5a4 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp @@ -105,7 +105,7 @@ VaultDelete::doApply() // else. They would otherwise keep its owner directory alive and block // deletion with tecHAS_OBLIGATIONS. Doing it first means a bounded, // tecINCOMPLETE cleanup can be resumed by a later transaction without having - // already torn down the vault. See fixCleanup3_4_0. + // already torn down the vault. if (view().rules().enabled(fixCleanup3_4_0)) { if (auto const ter = credentials::deletePseudoAccountCredentials( diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index 1eed7e55df..58b783fd4b 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -5154,15 +5154,15 @@ private: using namespace jtx; FeatureBitset const all{testableAmendments()}; - // FN-36: a credential issued to an AMM pseudo-account can't be accepted - // or deleted by it. Before fixCleanup3_4_0 it stays pinned in - // the pseudo-account's owner directory and makes AMM deletion fail with - // tecINTERNAL (deleteAMMTrustLines rejects the unexpected directory - // entry). + // A credential issued to an AMM pseudo-account can't be accepted or + // deleted by it. A pin created before the cure activates stays pinned + // in the pseudo-account's owner directory and makes AMM deletion fail + // with tecINTERNAL (deleteAMMTrustLines rejects the unexpected + // directory entry). Account const attacker{"attacker"}; char const credType[] = "FN36"; - Env env(*this, all - fixCleanup3_4_0); + Env env(*this, all - fixCleanup3_3_0 - fixCleanup3_4_0); fund(env, gw_, {alice_}, XRP(20'000), {USD(10'000)}); env.fund(XRP(1'000), attacker); env.close(); @@ -5184,11 +5184,7 @@ private: env.enableFeature(fixCleanup3_4_0); env.close(); - // Part 1: a new pin is rejected outright. - env(credentials::create(ammAcct, attacker, "FN36B"), Ter(tecPSEUDO_ACCOUNT)); - env.close(); - - // Part 2: the pre-existing pin is cleaned up and the AMM deletes. + // The pre-existing pin is cleaned up and the AMM deletes. amm.withdrawAll(alice_); BEAST_EXPECT(!amm.ammExists()); BEAST_EXPECT(!env.le(credKey)); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index c7343b50c6..f0d63bdcae 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -7739,17 +7739,16 @@ class Vault_test : public beast::unit_test::Suite { using namespace test::jtx; - // FN-36: a credential issued to a vault pseudo-account can't be - // accepted or deleted by it (pseudo-accounts can't sign), so it stays - // pinned in the pseudo-account's owner directory and blocks VaultDelete - // with tecHAS_OBLIGATIONS. fixCleanup3_4_0 rejects such - // credentials up front and, for pins created before the amendment, - // removes them on VaultDelete. + // A credential issued to a vault pseudo-account can't be accepted or + // deleted by it (pseudo-accounts can't sign), so it stays pinned in the + // pseudo-account's owner directory and blocks VaultDelete with + // tecHAS_OBLIGATIONS. A pin created before the cure activates is removed + // by VaultDelete once it does. Account const owner{"owner"}; Account const attacker{"attacker"}; char const credType[] = "FN36"; - Env env{*this, all_ - fixCleanup3_4_0}; + Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0}; env.fund(XRP(1'000'000), owner, attacker); env.close(); @@ -7768,8 +7767,7 @@ class Vault_test : public beast::unit_test::Suite // its owner count (an unaccepted credential is owned by the issuer). auto const pseudoOwnerCount = ownerCount(env, pseudo); - // Before the amendment: the pin succeeds. - testcase("Credential pins vault pseudo-account (amendment disabled)"); + testcase("Credential pins vault pseudo-account"); env(credentials::create(pseudo, attacker, credType)); env.close(); @@ -7785,14 +7783,9 @@ class Vault_test : public beast::unit_test::Suite env.enableFeature(fixCleanup3_4_0); env.close(); - // Part 1: a new pin is rejected outright. - testcase("Credential on vault pseudo-account rejected (amendment enabled)"); - env(credentials::create(pseudo, attacker, "FN36B"), Ter(tecPSEUDO_ACCOUNT)); - env.close(); - - // Part 2: the pre-existing pin no longer blocks deletion; the credential - // is cleaned up and the issuer's owner count is restored. - testcase("VaultDelete removes pinned credential (amendment enabled)"); + // The pre-existing pin no longer blocks deletion; the credential is + // cleaned up and the issuer's owner count is restored. + testcase("VaultDelete removes pinned credential"); env(vault.del({.owner = owner, .id = keylet.key})); env.close(); @@ -7814,7 +7807,7 @@ class Vault_test : public beast::unit_test::Suite Account const owner{"owner"}; Account const attacker{"attacker"}; - Env env{*this, all_ - fixCleanup3_4_0}; + Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0}; env.fund(XRP(10'000'000), owner, attacker); env.close(); diff --git a/src/test/app/lending/LoanBroker_test.cpp b/src/test/app/lending/LoanBroker_test.cpp index 15680e78df..4f46a47453 100644 --- a/src/test/app/lending/LoanBroker_test.cpp +++ b/src/test/app/lending/LoanBroker_test.cpp @@ -2748,18 +2748,18 @@ class LoanBroker_test : public beast::unit_test::Suite testCredentialPinsPseudoAccount() { using namespace test::jtx; - using namespace loanBroker; + using namespace loan_broker; - // FN-36: a credential issued to a LoanBroker pseudo-account can't be - // accepted or deleted by it, so it stays pinned in the pseudo-account's - // owner directory and blocks LoanBrokerDelete with tecHAS_OBLIGATIONS. - // fixCleanup3_4_0 rejects such credentials up front and, for - // pins created before the amendment, removes them on LoanBrokerDelete. + // A credential issued to a LoanBroker pseudo-account can't be accepted + // or deleted by it, so it stays pinned in the pseudo-account's owner + // directory and blocks LoanBrokerDelete with tecHAS_OBLIGATIONS. A pin + // created before the cure activates is removed by LoanBrokerDelete once + // it does. Account const alice{"alice"}; // vault & broker owner Account const attacker{"attacker"}; char const credType[] = "FN36"; - Env env{*this, all_ - fixCleanup3_4_0}; + Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0}; env.fund(XRP(1'000'000), alice, attacker); env.close(); @@ -2769,7 +2769,8 @@ class LoanBroker_test : public beast::unit_test::Suite env.close(); BEAST_EXPECT(env.le(vkeylet)); - auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); env(set(alice.id(), vkeylet.key)); env.close(); @@ -2778,8 +2779,7 @@ class LoanBroker_test : public beast::unit_test::Suite Account const pseudo{"broker pseudo-account", broker->at(sfAccount)}; env.memoize(pseudo); - // Before the amendment: the pin succeeds and blocks broker deletion. - testcase("Credential pins broker pseudo-account (amendment disabled)"); + testcase("Credential pins broker pseudo-account"); env(credentials::create(pseudo, attacker, credType)); env.close(); @@ -2793,14 +2793,9 @@ class LoanBroker_test : public beast::unit_test::Suite env.enableFeature(fixCleanup3_4_0); env.close(); - // Part 1: a new pin is rejected outright. - testcase("Credential on broker pseudo-account rejected (amendment enabled)"); - env(credentials::create(pseudo, attacker, "FN36B"), Ter(tecPSEUDO_ACCOUNT)); - env.close(); - - // Part 2: the pre-existing pin no longer blocks deletion; the credential - // is cleaned up and the issuer's owner count is restored. - testcase("LoanBrokerDelete removes pinned credential (amendment enabled)"); + // The pre-existing pin no longer blocks deletion; the credential is + // cleaned up and the issuer's owner count is restored. + testcase("LoanBrokerDelete removes pinned credential"); env(del(alice.id(), brokerKeylet.key)); env.close(); @@ -2810,6 +2805,65 @@ class LoanBroker_test : public beast::unit_test::Suite BEAST_EXPECT(ownerCount(env, attacker) == 0); } + void + testCredentialPinOverflow() + { + using namespace test::jtx; + using namespace loan_broker; + testcase("Credential pin cleanup is bounded (tecINCOMPLETE)"); + + // A pseudo-account can be pinned with more credentials than one + // transaction is allowed to clean up. LoanBrokerDelete then removes + // them a bounded batch at a time, returning tecINCOMPLETE until the + // last batch. + Account const alice{"alice"}; + Account const attacker{"attacker"}; + + Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0}; + env.fund(XRP(10'000'000), alice, attacker); + env.close(); + + Vault const vault{env}; + auto [vtx, vkeylet] = vault.create({.owner = alice, .asset = xrpIssue()}); + env(vtx); + env.close(); + BEAST_EXPECT(env.le(vkeylet)); + + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); + env(set(alice.id(), vkeylet.key)); + env.close(); + + auto const broker = env.le(brokerKeylet); + BEAST_EXPECT(broker); + Account const pseudo{"broker pseudo-account", broker->at(sfAccount)}; + env.memoize(pseudo); + + // Pin more than one cleanup batch's worth of credentials. + std::uint16_t const count = kMaxDeletablePseudoAccountCredentials + 3; + for (std::uint16_t i = 0; i < count; ++i) + env(credentials::create(pseudo, attacker, std::to_string(i))); + env.close(); + BEAST_EXPECT(ownerCount(env, attacker) == count); + + env.enableFeature(fixCleanup3_4_0); + env.close(); + + // First delete removes one bounded batch and reports it isn't finished. + env(del(alice.id(), brokerKeylet.key), Ter(tecINCOMPLETE)); + env.close(); + BEAST_EXPECT(env.le(brokerKeylet)); // broker still exists + auto const remaining = ownerCount(env, attacker); + BEAST_EXPECT(remaining > 0 && remaining < count); + + // Second delete finishes the cleanup and removes the broker. + env(del(alice.id(), brokerKeylet.key)); + env.close(); + BEAST_EXPECT(!env.le(brokerKeylet)); + BEAST_EXPECT(!env.le(keylet::account(pseudo.id()))); + BEAST_EXPECT(ownerCount(env, attacker) == 0); + } + public: void run() override @@ -2829,6 +2883,7 @@ public: testDisabled(); testLifecycle(); testCredentialPinsPseudoAccount(); + testCredentialPinOverflow(); testInvalidLoanBrokerDelete(); testInvalidLoanBrokerSet(); testRequireAuth();