From b604857da6d8cbd26b36d2aea23ff4df8f4eca53 Mon Sep 17 00:00:00 2001 From: Timur Ialymov Date: Mon, 10 Aug 2026 13:16:28 +0100 Subject: [PATCH] fix: Remove duplicate pseudo-account rejection from CredentialCreate CredentialCreate::preclaim already rejects a pseudo-account Subject via a separate amendment merged into develop while this branch was in review. Drop the duplicate check added here and keep only the credential cleanup this branch is actually for. Adjust the Vault, LoanBroker, and AMM tests to build the pre-existing pin with that other amendment disabled instead of this one, and drop the now-redundant assertions that a new pin gets rejected -- that is covered elsewhere now. Add an overflow test for LoanBroker mirroring the existing Vault one, exercising the tecINCOMPLETE branch a reviewer flagged as uncovered. Fix an unrelated SeqProxy/namespace-rename mismatch in LoanBroker_test.cpp introduced by the same develop merge, and rewrite comments across the changed files to describe the rule rather than name the amendment or ticket. --- .cspell.config.yaml | 1 + .../xrpl/ledger/helpers/CredentialHelpers.h | 8 +- src/libxrpl/ledger/helpers/AMMHelpers.cpp | 15 +-- src/libxrpl/tx/Transactor.cpp | 2 +- src/libxrpl/tx/invariants/MPTInvariant.cpp | 6 +- .../credentials/CredentialCreate.cpp | 10 -- .../transactors/lending/LoanBrokerDelete.cpp | 2 +- .../tx/transactors/vault/VaultDelete.cpp | 2 +- src/test/app/AMM_test.cpp | 18 ++-- src/test/app/Vault_test.cpp | 29 +++--- src/test/app/lending/LoanBroker_test.cpp | 91 +++++++++++++++---- 11 files changed, 110 insertions(+), 74 deletions(-) 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();