From 346ea40f69f9bc316c0aae4ba8b3f20cb3f9f7cc Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:45:52 +0000 Subject: [PATCH 01/62] fix: Allow zero-value MPT vault withdraw when the asset holding is missing (#8153) --- src/libxrpl/ledger/View.cpp | 15 +- src/libxrpl/tx/invariants/VaultInvariant.cpp | 4 +- src/test/app/vault/VaultBugs_test.cpp | 493 +++++++++++++++++++ 3 files changed, 505 insertions(+), 7 deletions(-) diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0cd082ff47..75a49187b4 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -543,12 +543,19 @@ doWithdraw( { auto const dstSle = ctx.view.read(keylet::account(dstAcct)); - // Create trust line or MPToken for the receiving account + // Create a trust line or MPToken for a self-destination only when there + // is a payout to credit. Post-fixCleanup3_4_0, a zero-value withdraw + // (e.g. share redemption from a fully impaired vault) must not insert + // an empty holding: that records a one-sided zero delta and can also + // create+delete MPTokens in the same transaction. if (dstAcct == senderAcct) { - if (auto const ter = addEmptyHolding(ctx, senderAcct, priorBalance, amount.asset(), j); - !isTesSuccess(ter) && ter != tecDUPLICATE) - return ter; + if (amount > beast::kZero || !ctx.view.rules().enabled(fixCleanup3_4_0)) + { + if (auto const ter = addEmptyHolding(ctx, senderAcct, priorBalance, amount.asset(), j); + !isTesSuccess(ter) && ter != tecDUPLICATE) + return ter; + } } else { diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index ff3a20c8ff..69c3ce92e0 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -1129,9 +1129,7 @@ ValidVault::finalize( // only. If the receiver's trust line sits at a coarser scale, the inflow // may safely round down to zero. // - // XRP and MPT remain strict. Because they are integer-exact, a zero - // destination delta indicates a true accounting bug, not a rounding - // artifact. + // XRP and MPT remain strict for rounding artifacts. bool const tolerateZeroDelta = view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); auto const invalidBalanceChange = tolerateZeroDelta diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 5b6e756e59..02949b8619 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -1677,6 +1678,495 @@ private: } } + // Bug: a fully impaired vault may pay zero assets for a share burn. + // Sending zero MPT is a no-op, so the vault pseudo-account's asset + // MPToken is never written and ValidVault, which only records deltas for + // created, modified or deleted entries, sees no vault delta at all. + // + // Pre-fixCleanup3_4_0 that alone makes the withdrawal impossible: + // zeroDeltaIsLegitimate is gated on the amendment, so the absent vault + // delta fails "withdrawal must change vault balance". Every pre-amendment + // arm below dies there, before any destination-side check runs. + // + // The destination side differs per arm, and only the vault-delta return + // hides that pre-amendment. With Alice's asset MPToken already present + // nothing touches it, so she has no delta either. With it missing, + // doWithdraw still called addEmptyHolding for a self-destination on a + // zero payout and created her MPToken at amount 0; a created MPToken is + // recorded even at zero, so she arrives with a present-and-zero delta, + // which for an integral MPT asset the destination check would reject if + // it were reached. + // + // ValidMPTIssuance is a separate checker and still runs. It only trips on + // the one arm that both creates and deletes an MPToken: Alice's last + // share with the asset MPToken missing, where addEmptyHolding creates the + // asset token while her share token is deleted (created + deleted > 1). + // Leftover shares with the token missing is create-only, and a last share + // with the token present is delete-only; neither exceeds one. Bob still + // owns shares throughout, so this is never the vault's final outstanding + // share. + // + // Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero + // payout and zeroDeltaIsLegitimate lets the vault-delta and + // missing-recipient-delta checks accept the transfer. A present + // destination delta of zero is still rejected. + void + testBugMptZeroWithdrawMissingHolding() + { + using namespace test::jtx; + using namespace loan_broker; + using namespace loan; + using namespace std::chrono_literals; + + auto runScenario = [this]( + FeatureBitset features, + bool removeAssetToken, + bool withdrawAllAliceShares, + TER expected) { + testcase( + std::string{"bug: MPT vault zero-value withdraw "} + + (removeAssetToken ? "without asset MPToken" : "with asset MPToken") + + (withdrawAllAliceShares ? ", Alice's last share" : ", Alice has leftover shares") + + (features[fixCleanup3_4_0] ? " (post-fixCleanup3_4_0)" : " (pre-fixCleanup3_4_0)")); + + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const borrower{"borrower"}; + + env.fund(XRP(100'000), issuer, owner, alice, bob, borrower); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanTransfer}); + PrettyAsset const asset = mptt.issuanceID(); + mptt.authorize({.account = owner}); + mptt.authorize({.account = alice}); + mptt.authorize({.account = bob}); + mptt.authorize({.account = borrower}); + env.close(); + + env(pay(issuer, alice, asset(2))); + env(pay(issuer, bob, asset(8))); + env.close(); + + Vault const vault{env}; + auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded( + {.owner = owner, .asset = asset, .subscriptionOffset = 60s}); + env(createTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(2)})); + env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = asset(8)})); + env.close(); + + vault.closePastSubscription(subscriptionDate); + + auto const brokerKeylet = + keylet::loanBroker(owner.id(), SeqProxy::rawSequence(env.seq(owner))); + env(set(owner, vaultKeylet.key)); + env.close(); + + auto const sleBroker = env.le(brokerKeylet); + if (!BEAST_EXPECT(sleBroker)) + return; + auto const loanKeylet = keylet::loan( + brokerKeylet.key, SeqProxy::rawSequence(sleBroker->at(sfLoanSequence))); + + env(set(borrower, brokerKeylet.key, asset(10).value()), + kInterestRate(percentageToTenthBips(0)), + kGracePeriod(60), + kPaymentInterval(120), + kPaymentTotal(10), + Sig(sfCounterpartySignature, owner), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const loanBefore = env.le(loanKeylet); + if (!BEAST_EXPECT(loanBefore)) + return; + std::uint32_t const dueDate = loanBefore->at(sfNextPaymentDueDate); + env.close(NetClock::time_point{NetClock::duration{dueDate}} + 1s); + + env(manage(owner, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const vaultImpaired = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultImpaired)) + return; + BEAST_EXPECT(vaultImpaired->at(sfAssetsAvailable) == asset(0).value()); + BEAST_EXPECT(vaultImpaired->at(sfAssetsTotal) == vaultImpaired->at(sfLossUnrealized)); + Number const totalBefore = vaultImpaired->at(sfAssetsTotal); + Number const lossBefore = vaultImpaired->at(sfLossUnrealized); + + MPTID const shareId = vaultImpaired->at(sfShareMPTID); + auto const issuanceBefore = env.le(keylet::mptokenIssuance(shareId)); + if (!BEAST_EXPECT(issuanceBefore)) + return; + std::uint64_t const outstandingBefore = + issuanceBefore->getFieldU64(sfOutstandingAmount); + + auto const tokenAlice = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(tokenAlice)) + return; + std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount); + BEAST_EXPECT(sharesBefore == 2); + std::uint64_t const sharesToRedeem = withdrawAllAliceShares ? sharesBefore : 1; + STAmount const redeemShares{MPTIssue{shareId}, Number(sharesToRedeem)}; + + auto const assetTokenKeylet = keylet::mptoken(mptt.issuanceID(), alice.id()); + if (removeAssetToken) + { + mptt.authorize({.account = alice, .flags = tfMPTUnauthorize}); + env.close(); + BEAST_EXPECT(!env.le(assetTokenKeylet)); + } + else + { + auto const existing = env.le(assetTokenKeylet); + if (!BEAST_EXPECT(existing)) + return; + BEAST_EXPECT(existing->getFieldU64(sfMPTAmount) == 0); + } + + std::uint32_t const redemptionDate = vaultImpaired->at(sfRedemptionDate); + env.close(NetClock::time_point{NetClock::duration{redemptionDate}} + 1s); + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}), + Ter(expected)); + env.close(); + if (expected != tesSUCCESS) + return; + + if (removeAssetToken) + { + BEAST_EXPECT(!env.le(assetTokenKeylet)); + } + else + { + auto const assetAfter = env.le(assetTokenKeylet); + if (!BEAST_EXPECT(assetAfter)) + return; + BEAST_EXPECT(assetAfter->getFieldU64(sfMPTAmount) == 0); + } + + auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id())); + if (withdrawAllAliceShares) + { + BEAST_EXPECT(!shareAfter); + } + else if (BEAST_EXPECT(shareAfter)) + { + BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - sharesToRedeem); + } + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == totalBefore); + BEAST_EXPECT(vaultAfter->at(sfLossUnrealized) == lossBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == asset(0).value()); + + auto const issuanceAfter = env.le(keylet::mptokenIssuance(shareId)); + if (!BEAST_EXPECT(issuanceAfter)) + return; + BEAST_EXPECT( + issuanceAfter->getFieldU64(sfOutstandingAmount) == + outstandingBefore - sharesToRedeem); + }; + + runScenario( + all_, false /* removeAssetToken */, false /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_, false /* removeAssetToken */, true /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_, true /* removeAssetToken */, false /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_, true /* removeAssetToken */, true /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_ - fixCleanup3_4_0, + false /* removeAssetToken */, + false /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + runScenario( + all_ - fixCleanup3_4_0, + false /* removeAssetToken */, + true /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + runScenario( + all_ - fixCleanup3_4_0, + true /* removeAssetToken */, + false /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + runScenario( + all_ - fixCleanup3_4_0, + true /* removeAssetToken */, + true /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + } + + // IOU analogue of the missing-MPToken case above. Alice removes her + // zero-balance trust line after depositing, then burns one unit from her + // scaled share balance after the vault is fully impaired. Bob's share + // balance keeps this out of the sole-shareholder loss-waiver and + // final-outstanding-share paths. A zero payout must not recreate Alice's + // unsolicited trust line. + void + testBugIouZeroWithdrawMissingTrustLine() + { + using namespace test::jtx; + using namespace loan_broker; + using namespace loan; + using namespace std::chrono_literals; + + Env env(*this, all_); + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const borrower{"borrower"}; + + env.fund(XRP(100'000), issuer, owner, alice, bob, borrower); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const asset = issuer["USD"]; + env.trust(asset(100), owner); + env.trust(asset(100), alice); + env.trust(asset(100), bob); + env.trust(asset(100), borrower); + env.close(); + + env(pay(issuer, alice, asset(2))); + env(pay(issuer, bob, asset(8))); + env.close(); + + Vault const vault{env}; + auto const [createTx, vaultKeylet, subscriptionDate] = + vault.createClosedEnded({.owner = owner, .asset = asset, .subscriptionOffset = 60s}); + env(createTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(2)})); + env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = asset(8)})); + env.close(); + + auto const assetLine = keylet::trustLine(alice, asset.raw().get()); + if (!BEAST_EXPECT(env.le(assetLine))) + return; + env.trust(asset(0), alice); + env.close(); + BEAST_EXPECT(!env.le(assetLine)); + + vault.closePastSubscription(subscriptionDate); + + auto const brokerKeylet = + keylet::loanBroker(owner.id(), SeqProxy::rawSequence(env.seq(owner))); + env(set(owner, vaultKeylet.key)); + env.close(); + + auto const sleBroker = env.le(brokerKeylet); + if (!BEAST_EXPECT(sleBroker)) + return; + auto const loanKeylet = + keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(sleBroker->at(sfLoanSequence))); + + env(set(borrower, brokerKeylet.key, asset(10).value()), + kInterestRate(percentageToTenthBips(0)), + kGracePeriod(60), + kPaymentInterval(120), + kPaymentTotal(10), + Sig(sfCounterpartySignature, owner), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const loanBefore = env.le(loanKeylet); + if (!BEAST_EXPECT(loanBefore)) + return; + std::uint32_t const dueDate = loanBefore->at(sfNextPaymentDueDate); + env.close(NetClock::time_point{NetClock::duration{dueDate}} + 1s); + + env(manage(owner, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const vaultImpaired = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultImpaired)) + return; + BEAST_EXPECT(vaultImpaired->at(sfAssetsAvailable) == asset(0).value()); + BEAST_EXPECT(vaultImpaired->at(sfAssetsTotal) == vaultImpaired->at(sfLossUnrealized)); + Number const totalBefore = vaultImpaired->at(sfAssetsTotal); + Number const lossBefore = vaultImpaired->at(sfLossUnrealized); + + MPTID const shareId = vaultImpaired->at(sfShareMPTID); + auto const tokenAlice = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(tokenAlice)) + return; + std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount); + // Default IOU vault scale is 6, so 2 USD mints 2e6 shares. Redeem one + // leftover share; do not require 1:1 like the MPT case. + BEAST_EXPECT(sharesBefore > 1); + STAmount const redeemShares{MPTIssue{shareId}, Number(1)}; + + std::uint32_t const redemptionDate = vaultImpaired->at(sfRedemptionDate); + env.close(NetClock::time_point{NetClock::duration{redemptionDate}} + 1s); + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}), + Ter(tesSUCCESS)); + env.close(); + + // A regression in the View guard would recreate this line even though + // no asset value was paid. + BEAST_EXPECT(!env.le(assetLine)); + + auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(shareAfter)) + return; + BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - 1); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == totalBefore); + BEAST_EXPECT(vaultAfter->at(sfLossUnrealized) == lossBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == asset(0).value()); + } + + // Same zero-payout withdrawal as testBugMptZeroWithdrawMissingHolding, but + // the vault asset is XRP. addEmptyHolding is a no-op for native assets. + // Sequence processing still touches the sender AccountRoot; a sponsored + // fee leaves that XRP balance economically unchanged. After the + // sponsored-withdraw fee-payer fix, deltaAssetsForParty collapses that + // economically-zero XRP delta to absence, so tesSUCCESS takes the + // missing-recipient-delta arm gated by zeroDeltaIsLegitimate. This test + // covers that live SUCCESS path. Pre-fixCleanup3_4_0 still fails the + // invariant. + void + testBugXrpZeroWithdrawSponsoredFee() + { + using namespace test::jtx; + using namespace loan_broker; + using namespace loan; + using namespace std::chrono_literals; + + auto runScenario = [this](FeatureBitset features, TER expected) { + testcase( + std::string{"bug: XRP vault zero-value withdraw with sponsored fee"} + + (features[fixCleanup3_4_0] ? " (post-fixCleanup3_4_0)" : " (pre-fixCleanup3_4_0)")); + + Env env(*this, features); + + Account const owner{"owner"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const borrower{"borrower"}; + Account const sponsor{"sponsor"}; + + env.fund(XRP(100'000), owner, alice, bob, borrower, sponsor); + env.close(); + + PrettyAsset const asset{xrpIssue()}; + Vault const vault{env}; + auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded( + {.owner = owner, .asset = asset, .subscriptionOffset = 60s}); + env(createTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(2)})); + env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = asset(8)})); + env.close(); + + vault.closePastSubscription(subscriptionDate); + + auto const brokerKeylet = + keylet::loanBroker(owner.id(), SeqProxy::rawSequence(env.seq(owner))); + env(set(owner, vaultKeylet.key)); + env.close(); + + auto const sleBroker = env.le(brokerKeylet); + if (!BEAST_EXPECT(sleBroker)) + return; + auto const loanKeylet = keylet::loan( + brokerKeylet.key, SeqProxy::rawSequence(sleBroker->at(sfLoanSequence))); + + env(set(borrower, brokerKeylet.key, asset(10).value()), + kInterestRate(percentageToTenthBips(0)), + kGracePeriod(60), + kPaymentInterval(120), + kPaymentTotal(10), + Sig(sfCounterpartySignature, owner), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const loanBefore = env.le(loanKeylet); + if (!BEAST_EXPECT(loanBefore)) + return; + std::uint32_t const dueDate = loanBefore->at(sfNextPaymentDueDate); + env.close(NetClock::time_point{NetClock::duration{dueDate}} + 1s); + + env(manage(owner, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const vaultImpaired = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultImpaired)) + return; + BEAST_EXPECT(vaultImpaired->at(sfAssetsAvailable) == asset(0).value()); + BEAST_EXPECT(vaultImpaired->at(sfAssetsTotal) == vaultImpaired->at(sfLossUnrealized)); + Number const totalBefore = vaultImpaired->at(sfAssetsTotal); + Number const lossBefore = vaultImpaired->at(sfLossUnrealized); + + MPTID const shareId = vaultImpaired->at(sfShareMPTID); + auto const tokenAlice = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(tokenAlice)) + return; + std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount); + BEAST_EXPECT(sharesBefore == 2); + STAmount const redeemShares{MPTIssue{shareId}, Number(1)}; + + std::uint32_t const redemptionDate = vaultImpaired->at(sfRedemptionDate); + env.close(NetClock::time_point{NetClock::duration{redemptionDate}} + 1s); + + auto const aliceBalanceBefore = env.balance(alice); + auto const sponsorBalanceBefore = env.balance(sponsor); + auto const fee = env.current()->fees().base; + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}), + Fee(fee), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor), + Ter(expected)); + env.close(); + + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee); + BEAST_EXPECT(env.balance(alice) == aliceBalanceBefore); + + if (expected != tesSUCCESS) + return; + + auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(shareAfter)) + return; + BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - 1); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == totalBefore); + BEAST_EXPECT(vaultAfter->at(sfLossUnrealized) == lossBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == asset(0).value()); + }; + + runScenario(all_, tesSUCCESS); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + } + // addEmptyHolding() used to check isGlobalFrozen(issuer) and // !lsfDefaultRipple before the "line already exists" tecDUPLICATE // short circuit. doWithdraw() calls addEmptyHolding() for a @@ -2383,6 +2873,9 @@ public: testBugClawbackRoundTripOvershoot(); testBugWithdrawRoundTripOvershoot(); testBugClawbackAfterLoanImpair(); + testBugMptZeroWithdrawMissingHolding(); + testBugIouZeroWithdrawMissingTrustLine(); + testBugXrpZeroWithdrawSponsoredFee(); testBugSelfWithdrawAfterIssuerClearsDefaultRipple(); testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient(); testBugSponsorAsDestinationFeeMisappliedToPayout(); From 5d8fd9824e98202ca8504aea9e45f2bafe0419cd Mon Sep 17 00:00:00 2001 From: Timur Yalymov <36795566+tyalymov@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:00:50 +0000 Subject: [PATCH 02/62] fix: Revert credential cleanup for pseudo-accounts (#8161) --- .cspell.config.yaml | 1 - .../xrpl/ledger/helpers/CredentialHelpers.h | 27 ---- include/xrpl/protocol/Protocol.h | 10 -- src/libxrpl/ledger/helpers/AMMHelpers.cpp | 14 -- .../ledger/helpers/CredentialHelpers.cpp | 32 ----- src/libxrpl/tx/Transactor.cpp | 14 +- src/libxrpl/tx/invariants/MPTInvariant.cpp | 8 -- .../transactors/lending/LoanBrokerDelete.cpp | 15 --- .../tx/transactors/vault/VaultDelete.cpp | 14 -- src/test/app/AMM_test.cpp | 47 ------- src/test/app/lending/LoanBroker_test.cpp | 122 ------------------ src/test/app/vault/VaultBugs_test.cpp | 113 ---------------- 12 files changed, 4 insertions(+), 413 deletions(-) diff --git a/.cspell.config.yaml b/.cspell.config.yaml index 8929973e8a..c1af739255 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -366,7 +366,6 @@ 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 6d235b4316..8b1c819bf4 100644 --- a/include/xrpl/ledger/helpers/CredentialHelpers.h +++ b/include/xrpl/ledger/helpers/CredentialHelpers.h @@ -14,7 +14,6 @@ #include #include -#include #include #include #include @@ -34,32 +33,6 @@ checkExpired(SLE const& sleCredential, NetClock::time_point const& closed); [[nodiscard]] TER 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), which such an account can neither accept nor delete. Only credentials - * are removed; every other object is left in place. The walk visits at most - * @p maxNodesToDelete directory entries and charges the ones it leaves alone - * against that budget too, so a directory holding other objects yields fewer - * than @p maxNodesToDelete deletions. On reaching the 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. - * @param maxNodesToDelete Upper bound on directory entries processed in one call. - * @param j Journal for diagnostics. - * @return tesSUCCESS once no credentials remain, tecINCOMPLETE if the bound was - * reached, or a deletion error. - */ -[[nodiscard]] TER -deletePseudoAccountCredentials( - ApplyView& view, - AccountID const& pseudoAcct, - std::uint16_t maxNodesToDelete, - beast::Journal j); - // Amendment and parameters checks for sfCredentialIDs field NotTEC checkFields(STTx const& tx, Rules const& rules, beast::Journal j); diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 8edd4bf4fd..ec9b9ba70a 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -407,16 +407,6 @@ using TxID = uint256; */ constexpr std::uint16_t kMaxDeletableAmmTrustLines = 512; -/** - * The maximum number of owner-directory entries to walk when clearing - * credentials pinned to a pseudo-account, in a single transaction. - * - * The walk stops after this many entries whether or not each one turns out to - * be a credential, so a directory that also holds other objects yields fewer - * deletions per transaction. - */ -constexpr std::uint16_t kMaxDeletablePseudoAccountCredentials = 512; - /** * The maximum length of a URI inside an Oracle */ diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index 20a793e4cb..fcad22d2d5 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include @@ -691,12 +690,6 @@ deleteAMMTrustLines( return {deleteAMMTrustLine(sb, sleItem, ammAccountID, j), SkipEntry::No}; } - // 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 JLOG(j.error()) << "deleteAMMObjects: deleting non-trustline or non-MPT " << nodeType; return {tecINTERNAL, SkipEntry::No}; @@ -774,8 +767,6 @@ deleteAMMAccount(Sandbox& sb, Asset const& asset, Asset const& asset2, beast::Jo // LCOV_EXCL_STOP } - // deleteAMMTrustLines also removes any credentials pinned to the AMM - // pseudo-account, within its bounded walk. if (auto const ter = deleteAMMTrustLines(sb, ammAccountID, kMaxDeletableAmmTrustLines, j); !isTesSuccess(ter)) return ter; @@ -917,11 +908,6 @@ isOnlyLiquidityProvider(ReadView const& view, Issue const& ammIssue, AccountID c ++nMPT; continue; } - // 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) return std::unexpected(tecINTERNAL); // LCOV_EXCL_LINE auto const lowLimit = sle->getFieldAmount(sfLowLimit); diff --git a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp index 9c3ca4ec78..5ba832957d 100644 --- a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp @@ -5,10 +5,8 @@ #include #include #include -#include #include #include -#include #include #include #include @@ -129,36 +127,6 @@ deleteSLE(ApplyView& view, SLE::ref sleCredential, beast::Journal j) return tesSUCCESS; } -TER -deletePseudoAccountCredentials( - ApplyView& view, - AccountID const& pseudoAcct, - std::uint16_t maxNodesToDelete, - beast::Journal j) -{ - XRPL_ASSERT( - isPseudoAccount(view.read(keylet::account(pseudoAcct))), - "xrpl::credentials::deletePseudoAccountCredentials : is a pseudo-account"); - - // Delete the credentials linked into the pseudo-account's owner directory, - // visiting at most maxNodesToDelete entries. Any other object is left in - // place; the caller's own checks decide whether the remaining directory - // blocks deletion. If the bound is reached, cleanupOnAccountDelete returns - // tecINCOMPLETE and the caller propagates it so a later transaction resumes. - return cleanupOnAccountDelete( - view, - keylet::ownerDir(pseudoAcct), - [&view, &j](LedgerEntryType nodeType, uint256 const&, SLE::pointer& sleItem) - -> std::pair { - if (nodeType == ltCREDENTIAL) - return {deleteSLE(view, sleItem, j), SkipEntry::No}; - - return {tesSUCCESS, SkipEntry::Yes}; - }, - j, - maxNodesToDelete); -} - NotTEC checkFields(STTx const& tx, Rules const& rules, beast::Journal j) { diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 63092cc128..6bf99e567d 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/src/libxrpl/tx/Transactor.cpp @@ -1246,7 +1246,7 @@ removeExpiredNFTokenOffers( } static void -removeDeletedCredentials(ApplyView& view, std::vector const& creds, beast::Journal viewJ) +removeExpiredCredentials(ApplyView& view, std::vector const& creds, beast::Journal viewJ) { for (auto const& index : creds) { @@ -1255,7 +1255,7 @@ removeDeletedCredentials(ApplyView& view, std::vector const& creds, bea if (auto const ter = credentials::deleteSLE(view, sle, viewJ); !isTesSuccess(ter)) { JLOG(viewJ.error()) - << "removeDeletedCredentials: failed to delete credential. Err: " + << "removeExpiredCredentials: failed to delete expired credential. Err: " << transToken(ter); } } @@ -1437,8 +1437,7 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee) // should be used, making it possible to do more useful work // when transactions fail with a `tec` code. - auto typesForResult = [credentialCleanup = - view().rules().enabled(fixCleanup3_4_0)](TER const ter) { + auto typesForResult = [](TER const ter) { std::unordered_set types; if ((ter == tecOVERSIZE) || (ter == tecKILLED)) { @@ -1447,11 +1446,6 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee) else if (ter == tecINCOMPLETE) { types.insert(ltRIPPLE_STATE); - // A bounded pseudo-account credential cleanup (VaultDelete / - // LoanBrokerDelete) persists its partial credential deletions so a - // later transaction can resume. - if (credentialCleanup) - types.insert(ltCREDENTIAL); } else if (ter == tecEXPIRED) { @@ -1529,7 +1523,7 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee) removeDeletedTrustLines(view(), ids, viewJ); break; case ltCREDENTIAL: - removeDeletedCredentials(view(), ids, viewJ); + removeExpiredCredentials(view(), ids, viewJ); break; // LCOV_EXCL_START default: diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 09b4308165..e38e8f2b93 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -234,14 +234,6 @@ ValidMPTIssuance::finalize( if (hasPrivilege(tx, Privilege::DestroyMptIssuance)) { - // A VaultDelete that is still cleaning up credentials pinned to its - // 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) && txnType == ttVAULT_DELETE && - result == tecINCOMPLETE) - return mptIssuancesDeleted_ == 0 && mptIssuancesCreated_ == 0; - if (mptIssuancesDeleted_ == 0) { JLOG(j.fatal()) << "Invariant failed: MPT issuance deletion " diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp index 06907ce366..433d77806a 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp @@ -4,13 +4,11 @@ #include #include #include -#include #include #include #include #include #include -#include #include #include #include @@ -142,19 +140,6 @@ LoanBrokerDelete::doApply() auto const brokerPseudoID = broker->at(sfAccount); - // Remove any credentials pinned to the broker pseudo-account before anything - // 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. - if (view().rules().enabled(fixCleanup3_4_0)) - { - if (auto const ter = credentials::deletePseudoAccountCredentials( - view(), brokerPseudoID, kMaxDeletablePseudoAccountCredentials, j_); - !isTesSuccess(ter)) - return ter; - } - if (!view().dirRemove( keylet::ownerDir(accountID_), broker->at(sfOwnerNode), broker->key(), false)) { diff --git a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp index 35bf80c29f..9c6c41654b 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -102,19 +101,6 @@ VaultDelete::doApply() if (!vault) return tefINTERNAL; // LCOV_EXCL_LINE - // Remove any credentials pinned to the vault pseudo-account before anything - // 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. - if (view().rules().enabled(fixCleanup3_4_0)) - { - if (auto const ter = credentials::deletePseudoAccountCredentials( - view(), vault->at(sfAccount), kMaxDeletablePseudoAccountCredentials, j_); - !isTesSuccess(ter)) - return ter; - } - // Destroy the asset holding. auto asset = vault->at(sfAsset); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index a1d5260606..0212035c6e 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4,7 +4,6 @@ #include #include #include -#include #include #include #include @@ -5193,51 +5192,6 @@ private: {features}); } - void - testCredentialPinsPseudoAccount() - { - testcase("Credential pins AMM pseudo-account"); - - using namespace jtx; - FeatureBitset const all{testableAmendments()}; - - // 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_3_0 - fixCleanup3_4_0); - fund(env, gw_, {alice_}, XRP(20'000), {USD(10'000)}); - env.fund(XRP(1'000), attacker); - env.close(); - - AMM amm(env, alice_, XRP(10'000), USD(10'000)); - Account const ammAcct{"amm pseudo-account", amm.ammAccount()}; - env.memoize(ammAcct); - - env(credentials::create(ammAcct, attacker, credType)); - env.close(); - auto const credKey = credentials::keylet(ammAcct, attacker, credType); - BEAST_EXPECT(env.le(credKey)); - - // Emptying the AMM would auto-delete it, but the pinned credential makes - // deleteAMMAccount fail; the withdraw is rolled back and the AMM stays. - amm.withdrawAll(alice_, std::nullopt, Ter(tecINTERNAL)); - BEAST_EXPECT(amm.ammExists()); - - env.enableFeature(fixCleanup3_4_0); - env.close(); - - // The pre-existing pin is cleaned up and the AMM deletes. - amm.withdrawAll(alice_); - BEAST_EXPECT(!amm.ammExists()); - BEAST_EXPECT(!env.le(credKey)); - BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount()))); - } - void testAutoDelete() { @@ -7505,7 +7459,6 @@ private: FeatureBitset const all{testableAmendments()}; testInvalidInstance(); testInstanceCreate(); - testCredentialPinsPseudoAccount(); for (auto const& f : amendmentCombinations({fixCleanup3_3_0, featureAMMClawback})) testInvalidDeposit(f); testDeposit(); diff --git a/src/test/app/lending/LoanBroker_test.cpp b/src/test/app/lending/LoanBroker_test.cpp index 5b3ea854f8..3bcda42c7e 100644 --- a/src/test/app/lending/LoanBroker_test.cpp +++ b/src/test/app/lending/LoanBroker_test.cpp @@ -2968,126 +2968,6 @@ class LoanBroker_test : public beast::unit_test::Suite runTestCases(all_ - fixCleanup3_2_0); } - void - testCredentialPinsPseudoAccount() - { - using namespace test::jtx; - using namespace loan_broker; - - // 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_3_0 - fixCleanup3_4_0}; - env.fund(XRP(1'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); - - testcase("Credential pins broker pseudo-account"); - env(credentials::create(pseudo, attacker, credType)); - env.close(); - - auto const credKey = credentials::keylet(pseudo, attacker, credType); - BEAST_EXPECT(env.le(credKey)); - BEAST_EXPECT(ownerCount(env, attacker) == 1); - - env(del(alice.id(), brokerKeylet.key), Ter(tecHAS_OBLIGATIONS)); - env.close(); - - env.enableFeature(fixCleanup3_4_0); - env.close(); - - // 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(); - - BEAST_EXPECT(!env.le(credKey)); - BEAST_EXPECT(!env.le(brokerKeylet)); - BEAST_EXPECT(!env.le(keylet::account(pseudo.id()))); - 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 @@ -3106,8 +2986,6 @@ public: testDisabled(); testLifecycle(); - testCredentialPinsPseudoAccount(); - testCredentialPinOverflow(); testInvalidLoanBrokerDelete(); testInvalidLoanBrokerSet(); testRequireAuth(); diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 02949b8619..cc30bd6091 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1356,117 +1356,6 @@ private: } } - void - testCredentialPinsPseudoAccount() - { - using namespace test::jtx; - - // 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_3_0 - fixCleanup3_4_0}; - env.fund(XRP(1'000'000), owner, attacker); - env.close(); - - Vault const vault{env}; - PrettyAsset const asset = xrpIssue(); - auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); - env(tx); - env.close(); - - auto const vaultSle = env.le(keylet); - BEAST_EXPECT(vaultSle); - Account const pseudo{"vault pseudo-account", vaultSle->at(sfAccount)}; - env.memoize(pseudo); - - // The pseudo-account owns the share issuance; the pin must not change - // its owner count (an unaccepted credential is owned by the issuer). - auto const pseudoOwnerCount = ownerCount(env, pseudo); - - testcase("Credential pins vault pseudo-account"); - env(credentials::create(pseudo, attacker, credType)); - env.close(); - - auto const credKey = credentials::keylet(pseudo, attacker, credType); - BEAST_EXPECT(env.le(credKey)); - BEAST_EXPECT(ownerCount(env, attacker) == 1); - BEAST_EXPECT(ownerCount(env, pseudo) == pseudoOwnerCount); - - // The pin blocks deletion of an otherwise-empty vault. - env(vault.del({.owner = owner, .id = keylet.key}), Ter(tecHAS_OBLIGATIONS)); - env.close(); - - env.enableFeature(fixCleanup3_4_0); - env.close(); - - // 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(); - - BEAST_EXPECT(!env.le(credKey)); - BEAST_EXPECT(!env.le(keylet)); - BEAST_EXPECT(!env.le(::xrpl::keylet::account(pseudo.id()))); - BEAST_EXPECT(ownerCount(env, attacker) == 0); - } - - void - testCredentialPinOverflow() - { - using namespace test::jtx; - testcase("Credential pin cleanup is bounded (tecINCOMPLETE)"); - - // A pseudo-account can be pinned with more credentials than one - // transaction is allowed to clean up. VaultDelete then removes them a - // bounded batch at a time, returning tecINCOMPLETE until the last batch. - Account const owner{"owner"}; - Account const attacker{"attacker"}; - - Env env{*this, all_ - fixCleanup3_3_0 - fixCleanup3_4_0}; - env.fund(XRP(10'000'000), owner, attacker); - env.close(); - - Vault const vault{env}; - auto [tx, keylet] = vault.create({.owner = owner, .asset = xrpIssue()}); - env(tx); - env.close(); - auto const vaultSle = env.le(keylet); - BEAST_EXPECT(vaultSle); - Account const pseudo{"vault pseudo-account", vaultSle->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(vault.del({.owner = owner, .id = keylet.key}), Ter(tecINCOMPLETE)); - env.close(); - BEAST_EXPECT(env.le(keylet)); // vault still exists - auto const remaining = ownerCount(env, attacker); - BEAST_EXPECT(remaining > 0 && remaining < count); - - // Second delete finishes the cleanup and removes the vault. - env(vault.del({.owner = owner, .id = keylet.key})); - env.close(); - BEAST_EXPECT(!env.le(keylet)); - BEAST_EXPECT(!env.le(::xrpl::keylet::account(pseudo.id()))); - BEAST_EXPECT(ownerCount(env, attacker) == 0); - } - struct ImpairedLoanVault { test::jtx::Account issuer; @@ -2867,8 +2756,6 @@ public: testBugVaultDepositOvercreditsAcrossScaleBoundary(); testBugVaultLockedByPartialWithdraw(); testVaultDepositNegativeBalanceFromOppositeLimit(); - testCredentialPinsPseudoAccount(); - testCredentialPinOverflow(); testBug6LimitBypassWithShares(); testBugClawbackRoundTripOvershoot(); testBugWithdrawRoundTripOvershoot(); From f0fd6ad85e89cfbda9676a8ce8e9b84817aaea11 Mon Sep 17 00:00:00 2001 From: Bart Date: Wed, 2 Sep 2026 18:06:48 +0000 Subject: [PATCH 03/62] fix: Clamp the depth used to index selectBranch's key byte (#7941) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> --- src/libxrpl/shamap/SHAMapNodeID.cpp | 63 ++++++- src/tests/libxrpl/shamap/SHAMapNodeID.cpp | 193 ++++++++++++++++++++++ 2 files changed, 247 insertions(+), 9 deletions(-) create mode 100644 src/tests/libxrpl/shamap/SHAMapNodeID.cpp diff --git a/src/libxrpl/shamap/SHAMapNodeID.cpp b/src/libxrpl/shamap/SHAMapNodeID.cpp index 8fd7afe8fc..42b946b921 100644 --- a/src/libxrpl/shamap/SHAMapNodeID.cpp +++ b/src/libxrpl/shamap/SHAMapNodeID.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -40,11 +41,41 @@ depthMask(unsigned int depth) return kMasks.entry[depth]; } +// The prefix of `key` at `depth`: the leading nibbles naming the subtree a node at that depth +// identifies, with the remainder of the key masked off. +static uint256 +maskedToDepth(uint256 const& key, unsigned int depth) +{ + return key & depthMask(depth); +} + +// Whether `id` at `depth` is what `key` looks like once masked down to that depth, i.e. +// whether an ID with this depth and id names a subtree that `key` falls under. +static bool +isPrefixOfAtDepth(uint256 const& id, unsigned int depth, uint256 const& key) +{ + return maskedToDepth(key, depth) == id; +} + // canonicalize the hash to a node ID for this depth SHAMapNodeID::SHAMapNodeID(unsigned int depth, uint256 const& hash) : id_(hash), depth_(depth) { - XRPL_ASSERT( - depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::SHAMapNodeID : maximum depth input"); + // Every SHAMapNodeID's depth is stored here, so this is the one place that can stop an + // out-of-range one from being kept: a depth past kLeafDepth would go on to index depthMask + // out of bounds, and getRawString would narrow it to a byte, silently renaming the node. + // Clamp rather than throw, since node IDs are built from peer-supplied depths on the ledger + // data path, where no caller catches an exception before it reaches a thread boundary. + if (depth_ > SHAMap::kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMapNodeID::SHAMapNodeID : depth within tree"); + depth_ = SHAMap::kLeafDepth; + id_ = maskedToDepth(id_, depth_); + // LCOV_EXCL_STOP + } + + // Reads the clamped member rather than the depth argument, so it cannot index depthMask past + // its last entry even once the clamp above has reported the bad input and carried on. XRPL_ASSERT( isPrefixOf(id_), "xrpl::SHAMapNodeID::SHAMapNodeID : hash and depth inputs do match"); } @@ -89,7 +120,7 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const bool SHAMapNodeID::isPrefixOf(uint256 const& key) const { - return (key & depthMask(depth_)) == id_; + return isPrefixOfAtDepth(id_, depth_, key); } [[nodiscard]] std::optional @@ -102,9 +133,9 @@ deserializeSHAMapNodeID(void const* data, std::size_t size) unsigned int const depth = *(static_cast(data) + 32); if (depth <= SHAMap::kLeafDepth) { - auto const id = uint256::fromVoid(data); - - if (id == (id & depthMask(depth))) + // Reject a serialized ID carrying bits below its own depth. Checked before + // constructing, since the constructor asserts that same property. + if (auto const id = uint256::fromVoid(data); isPrefixOfAtDepth(id, depth, id)) ret.emplace(depth, id); } } @@ -115,7 +146,11 @@ deserializeSHAMapNodeID(void const* data, std::size_t size) [[nodiscard]] unsigned int selectBranch(SHAMapNodeID const& id, uint256 const& hash) { - auto const depth = id.getDepth(); + XRPL_ASSERT(id.getDepth() < SHAMap::kLeafDepth, "xrpl::selectBranch : depth below leaf depth"); + + // A depth-64 ID has no nibble left to select. Callers must not ask, but clamp anyway to keep + // the read below the end of the 32-byte key. + auto const depth = std::min(id.getDepth(), SHAMap::kLeafDepth - 1u); auto branch = static_cast(*(hash.begin() + (depth / 2))); if ((depth & 1) != 0u) @@ -134,8 +169,18 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash) SHAMapNodeID SHAMapNodeID::createID(unsigned int depth, uint256 const& key) { - XRPL_ASSERT(depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth"); - return SHAMapNodeID(depth, key & depthMask(depth)); + // The mask is chosen here, before the constructor runs, so the clamp there cannot cover this + // call: an out-of-range depth would index depthMask's table while still evaluating this + // argument. A public factory has to hold its own bound. + if (depth > SHAMap::kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMapNodeID::createID : depth within tree"); + depth = SHAMap::kLeafDepth; + // LCOV_EXCL_STOP + } + + return SHAMapNodeID(depth, maskedToDepth(key, depth)); } } // namespace xrpl diff --git a/src/tests/libxrpl/shamap/SHAMapNodeID.cpp b/src/tests/libxrpl/shamap/SHAMapNodeID.cpp new file mode 100644 index 0000000000..95b7497c9c --- /dev/null +++ b/src/tests/libxrpl/shamap/SHAMapNodeID.cpp @@ -0,0 +1,193 @@ +#include + +#include +#include +#include + +#include + +#include + +namespace xrpl::tests { + +// An arbitrary 32-byte key reused across tests below that don't care about its specific value, +// only that it is a well-formed key. +constexpr uint256 kTestKey("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"); + +TEST(SHAMapNodeIDTest, root_is_prefix_of_every_key) +{ + SHAMapNodeID const root; + EXPECT_EQ(root.getDepth(), 0u); + EXPECT_TRUE(root.isPrefixOf(uint256{})); + EXPECT_TRUE(root.isPrefixOf(kTestKey)); +} + +TEST(SHAMapNodeIDTest, child_id_is_prefix_of_keys_in_that_branch) +{ + // Walking the branches spelled by the key's own nibbles must keep every + // intermediate ID a prefix of that key. + SHAMapNodeID id; + for (auto depth = 0u; depth < SHAMap::kLeafDepth; ++depth) + { + id = id.getChildNodeID(selectBranch(id, kTestKey)); + EXPECT_EQ(id.getDepth(), depth + 1); + EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << id.getDepth(); + } +} + +TEST(SHAMapNodeIDTest, wrong_branch_is_not_prefix_of_key) +{ + SHAMapNodeID const root; + auto const correct = selectBranch(root, kTestKey); + ASSERT_EQ(correct, 0xbu); + + // An ID built from the wrong branch still has a valid depth and a self-consistent mask, so + // isPrefixOf(kTestKey) below is what actually distinguishes the correct branch from the rest. + for (auto branch = 0u; branch < SHAMap::kBranchFactor; ++branch) + { + auto const child = root.getChildNodeID(branch); + EXPECT_EQ(child.getDepth(), 1u); + EXPECT_EQ(child.isPrefixOf(kTestKey), branch == correct) << "branch " << branch; + } +} + +TEST(SHAMapNodeIDTest, prefix_check_is_depth_sensitive) +{ + // kTestKey and kOther agree on the first two nibbles ("b9") and then diverge. + constexpr uint256 kOther("b99891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8"); + + auto id = SHAMapNodeID{}.getChildNodeID(selectBranch(SHAMapNodeID{}, kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kOther)) << "shared first nibble"; + + id = id.getChildNodeID(selectBranch(id, kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kOther)) << "shared second nibble"; + + // Third nibble differs, so the deeper ID no longer covers kOther. + id = id.getChildNodeID(selectBranch(id, kTestKey)); + EXPECT_TRUE(id.isPrefixOf(kTestKey)); + EXPECT_FALSE(id.isPrefixOf(kOther)); +} + +TEST(SHAMapNodeIDTest, leaf_id_from_key_is_prefix_of_that_key) +{ + SHAMapNodeID const leaf{SHAMap::kLeafDepth, kTestKey}; + EXPECT_TRUE(leaf.isPrefixOf(kTestKey)); + + // At full depth the prefix is the whole key, so nothing else matches. + constexpr uint256 kOther("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca9"); + EXPECT_FALSE(leaf.isPrefixOf(kOther)); +} + +TEST(SHAMapNodeIDTest, create_id_masks_key_to_depth) +{ + for (auto depth = 0u; depth <= SHAMap::kLeafDepth; ++depth) + { + auto const id = SHAMapNodeID::createID(depth, kTestKey); + EXPECT_EQ(id.getDepth(), depth); + EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << depth; + } +} + +// The guards below must hold with XRPL_ASSERT compiled out (NDEBUG), so each one +// has to be a real runtime check rather than an assert. + +TEST(SHAMapNodeIDTest, child_of_leaf_depth_id_throws) +{ + auto const leafDepthID = SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey); + ASSERT_EQ(leafDepthID.getDepth(), SHAMap::kLeafDepth); + EXPECT_THROW((void)leafDepthID.getChildNodeID(0), std::logic_error); +} + +TEST(SHAMapNodeIDDeathTest, out_of_range_depth_is_clamped) +{ + // A depth past kLeafDepth has no mask in depthMask's 65-entry table, so both the constructor + // and createID clamp it. createID needs its own clamp: it picks the mask while evaluating the + // constructor's argument, so the constructor's clamp cannot cover that read. + // + // Both clamps are marked UNREACHABLE, which is an assert and therefore fatal wherever asserts + // are live. Only a build with them compiled out (or routed to Antithesis's non-fatal handler) + // reaches the clamp itself, so that is the only configuration that can assert on the result. +#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR) + for (auto const depth : {SHAMap::kLeafDepth + 1u, 100u, 255u, 256u, 320u}) + { + auto const id = SHAMapNodeID::createID(depth, kTestKey); + + // Clamped to a real depth, not the depth asked for, and not a byte-narrowed version of it: + // 256 would otherwise become 0 and name the root, 320 would become 64. + EXPECT_EQ(id.getDepth(), SHAMap::kLeafDepth) << "depth " << depth; + + // id_ and depth_ still agree, so the object is usable rather than merely non-crashing. + EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << depth; + EXPECT_EQ(id, SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey)) << "depth " << depth; + + // The clamp holds through the wire format too, which encodes the depth in one byte. + auto const roundTripped = deserializeSHAMapNodeID(id.getRawString()); + ASSERT_TRUE(roundTripped.has_value()) << "depth " << depth; + EXPECT_EQ(roundTripped->getDepth(), SHAMap::kLeafDepth) << "depth " << depth; + } + + // The constructor clamps on its own, for the paths that do not go through createID. + SHAMapNodeID const direct{SHAMap::kLeafDepth + 1u, uint256{}}; + EXPECT_EQ(direct.getDepth(), SHAMap::kLeafDepth); +#else + EXPECT_DEATH( + (void)SHAMapNodeID::createID(SHAMap::kLeafDepth + 1u, kTestKey), "depth within tree"); +#endif +} + +TEST(SHAMapNodeIDDeathTest, select_branch_clamps_leaf_depth) +{ + // selectBranch's own precondition is depth < kLeafDepth: a depth-64 ID has no nibble left + // to select. That makes it unlike the guards above, which have a throw/return reachable + // even with XRPL_ASSERT compiled out; selectBranch has no such path, so the two build + // configurations have to be tested differently. + // + // Under ENABLE_VOIDSTAR, XRPL_ASSERT routes to Antithesis's assert_impl, which only records + // the hit and returns rather than aborting, even though NDEBUG is undefined there (voidstar + // requires a Debug build). So the assert is live in name but never fatal, the same as the + // NDEBUG case below. + auto const leafDepthID = SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey); + +#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR) + // With the assert compiled out or routed to a non-fatal handler, the clamp is what stands + // between this call and reading past the end of the 32-byte key. Clamping means it reads the + // same byte, and returns the same branch, as the deepest ID that still has one: depth 63. + auto const deepestWithBranchID = SHAMapNodeID::createID(SHAMap::kLeafDepth - 1u, kTestKey); + auto const branch = selectBranch(leafDepthID, kTestKey); + EXPECT_LT(branch, SHAMap::kBranchFactor); + EXPECT_EQ(branch, selectBranch(deepestWithBranchID, kTestKey)); +#else + // In a debug build the assert is live and must reject this call outright, in a forked + // process so a failure here cannot take down the rest of the suite. + EXPECT_DEATH((void)selectBranch(leafDepthID, kTestKey), "depth below leaf depth"); +#endif +} + +TEST(SHAMapNodeIDTest, deserialize_rejects_out_of_range_depth) +{ + // getRawString() only serializes a depth already accepted by the constructor's own + // assertion, so an out-of-range depth here is built by hand instead. + auto serializeWithRawDepth = [](unsigned int depth) { + Serializer s; + s.addBitString(uint256{}); + s.add8(static_cast(depth)); + return s.getString(); + }; + + for (auto const depth : {65u, 100u, 255u}) + { + EXPECT_FALSE(deserializeSHAMapNodeID(serializeWithRawDepth(depth)).has_value()) + << "depth " << depth; + } + + // A depth-64 ID is legal, since leaves live there, but it has no children. + auto const id = + deserializeSHAMapNodeID(SHAMapNodeID{SHAMap::kLeafDepth, uint256{}}.getRawString()); + ASSERT_TRUE(id.has_value()); + // NOLINTNEXTLINE(bugprone-unchecked-optional-access) has_value checked above + EXPECT_THROW((void)id->getChildNodeID(0), std::logic_error); +} + +} // namespace xrpl::tests From 7d7275847d1fb7bcf2b179f4cf5d755254d6fe6b Mon Sep 17 00:00:00 2001 From: Kassaking7 <96991820+Kassaking7@users.noreply.github.com> Date: Wed, 2 Sep 2026 19:38:01 +0000 Subject: [PATCH 04/62] fix: PermissionedDEX (CreateOffer/Payment) never deletes expired credentials (#6827) --- .../tx/transactors/dex/OfferCreate.cpp | 49 +++- .../tx/transactors/payment/Payment.cpp | 63 ++++- src/test/app/PermissionedDEX_test.cpp | 215 +++++++++++++++++- 3 files changed, 317 insertions(+), 10 deletions(-) diff --git a/src/libxrpl/tx/transactors/dex/OfferCreate.cpp b/src/libxrpl/tx/transactors/dex/OfferCreate.cpp index 7ab1143d12..57ba6eff0d 100644 --- a/src/libxrpl/tx/transactors/dex/OfferCreate.cpp +++ b/src/libxrpl/tx/transactors/dex/OfferCreate.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -242,8 +243,31 @@ OfferCreate::preclaim(PreclaimContext const& ctx) // is part of the domain if (ctx.tx.isFieldPresent(sfDomainID)) { - if (!permissioned_dex::accountInDomain(ctx.view, id, ctx.tx[sfDomainID])) - return tecNO_PERMISSION; + if (ctx.view.rules().enabled(fixCleanup3_4_0)) + { + auto const domainID = ctx.tx[sfDomainID]; + auto const sleDomain = ctx.view.read(keylet::permissionedDomain(domainID)); + if (!sleDomain) + return tecNO_PERMISSION; + + // Domain owner is always considered in the domain, no credential check + // needed. For all other accounts, use validDomain which detects expired + // credentials. Suppress tecEXPIRED here so doApply can run and delete + // the expired credential SLEs from the ledger. + if (sleDomain->getAccountID(sfOwner) != id) + { + // validDomain returns tecNO_AUTH when no matching credential is + // found. Map it to tecNO_PERMISSION to preserve existing behavior. + if (auto const err = credentials::validDomain(ctx.view, domainID, id); + !isTesSuccess(err) && err != tecEXPIRED) + return tecNO_PERMISSION; + } + } + else + { + if (!permissioned_dex::accountInDomain(ctx.view, id, ctx.tx[sfDomainID])) + return tecNO_PERMISSION; + } } if (auto const ter = canTrade(ctx.view, saTakerPays.asset()); !isTesSuccess(ter)) @@ -1000,6 +1024,27 @@ OfferCreate::applyGuts(Sandbox& sb, Sandbox& sbCancel) TER OfferCreate::doApply() { + // If a DomainID is present, verify the account is still in the domain and + // delete any expired credential SLEs. This must happen before the Sandboxes + // are created: if we return a tec error, the engine applies sbCancel (not + // sb) to rawView, so deletions made inside sb would be lost. Deletions made + // directly to ctx_.view() here are preserved regardless of which branch + // applyGuts takes. + if (ctx_.tx.isFieldPresent(sfDomainID) && ctx_.view().rules().enabled(fixCleanup3_4_0)) + { + auto const domainID = ctx_.tx[sfDomainID]; + auto const sleDomain = ctx_.view().read(keylet::permissionedDomain(domainID)); + if (!sleDomain) + return tecINTERNAL; // LCOV_EXCL_LINE + + if (sleDomain->getAccountID(sfOwner) != accountID_) + { + if (auto const err = verifyValidDomain(ctx_.view(), accountID_, domainID, j_); + !isTesSuccess(err)) + return err; + } + } + // This is the ledger view that we work against. Transactions are applied // as we go on processing transactions. Sandbox sb(&ctx_.view()); diff --git a/src/libxrpl/tx/transactors/payment/Payment.cpp b/src/libxrpl/tx/transactors/payment/Payment.cpp index c8b00f0193..c4c2f9227b 100644 --- a/src/libxrpl/tx/transactors/payment/Payment.cpp +++ b/src/libxrpl/tx/transactors/payment/Payment.cpp @@ -458,11 +458,41 @@ Payment::preclaim(PreclaimContext const& ctx) if (ctx.tx.isFieldPresent(sfDomainID)) { - if (!permissioned_dex::accountInDomain(ctx.view, ctx.tx[sfAccount], ctx.tx[sfDomainID])) - return tecNO_PERMISSION; + if (ctx.view.rules().enabled(fixCleanup3_4_0)) + { + auto const domainID = ctx.tx[sfDomainID]; + auto const sleDomain = ctx.view.read(keylet::permissionedDomain(domainID)); + if (!sleDomain) + return tecNO_PERMISSION; - if (!permissioned_dex::accountInDomain(ctx.view, ctx.tx[sfDestination], ctx.tx[sfDomainID])) - return tecNO_PERMISSION; + // Domain owner is always considered in the domain. For other accounts, + // suppress tecEXPIRED so doApply can run and delete expired credential + // SLEs from the ledger. + auto const checkAccount = [&](AccountID const& acct) -> TER { + if (sleDomain->getAccountID(sfOwner) == acct) + return tesSUCCESS; + // validDomain returns tecNO_AUTH when no matching credential is + // found. Map it to tecNO_PERMISSION to preserve existing behavior. + if (auto const err = credentials::validDomain(ctx.view, domainID, acct); + !isTesSuccess(err) && err != tecEXPIRED) + return tecNO_PERMISSION; + return tesSUCCESS; + }; + + if (auto const err = checkAccount(ctx.tx[sfAccount]); !isTesSuccess(err)) + return err; + if (auto const err = checkAccount(ctx.tx[sfDestination]); !isTesSuccess(err)) + return err; + } + else + { + if (!permissioned_dex::accountInDomain(ctx.view, ctx.tx[sfAccount], ctx.tx[sfDomainID])) + return tecNO_PERMISSION; + + if (!permissioned_dex::accountInDomain( + ctx.view, ctx.tx[sfDestination], ctx.tx[sfDomainID])) + return tecNO_PERMISSION; + } } return tesSUCCESS; @@ -471,6 +501,31 @@ Payment::preclaim(PreclaimContext const& ctx) TER Payment::doApply() { + // If a DomainID is present, verify both sender and destination are still in + // the domain and delete any expired credential SLEs from the ledger. + if (ctx_.tx.isFieldPresent(sfDomainID) && ctx_.view().rules().enabled(fixCleanup3_4_0)) + { + auto const domainID = ctx_.tx[sfDomainID]; + auto const sleDomain = ctx_.view().read(keylet::permissionedDomain(domainID)); + if (!sleDomain) + return tecINTERNAL; // LCOV_EXCL_LINE + + auto const cleanupFor = [&](AccountID const& acct) -> TER { + if (sleDomain->getAccountID(sfOwner) == acct) + return tesSUCCESS; + return verifyValidDomain(ctx_.view(), acct, domainID, j_); + }; + + auto const destination = ctx_.tx[sfDestination]; + auto const senderErr = cleanupFor(accountID_); + auto const destinationErr = accountID_ == destination ? senderErr : cleanupFor(destination); + + if (!isTesSuccess(senderErr)) + return senderErr; + if (!isTesSuccess(destinationErr)) + return destinationErr; + } + auto const deliverMin = ctx_.tx[~sfDeliverMin]; // Ripple if source or destination is non-native or if there are paths. diff --git a/src/test/app/PermissionedDEX_test.cpp b/src/test/app/PermissionedDEX_test.cpp index ddb56a1480..fbe942948d 100644 --- a/src/test/app/PermissionedDEX_test.cpp +++ b/src/test/app/PermissionedDEX_test.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -179,7 +180,10 @@ class PermissionedDEX_test : public beast::unit_test::Suite void testOfferCreate(FeatureBitset features) { - testcase("OfferCreate"); + bool const fixEnabled = features[fixCleanup3_4_0]; + + testcase << "OfferCreate" + << (fixEnabled ? " (Cleanup3_4_0 enabled)" : " (Cleanup3_4_0 disabled)"); // test preflight { @@ -273,8 +277,10 @@ class PermissionedDEX_test : public beast::unit_test::Suite // time advance env.close(std::chrono::seconds(20)); - // devin cannot create offer with expired cred - env(offer(devin, XRP(10), USD(10)), Domain(domainID), Ter(tecNO_PERMISSION)); + // Devin cannot create offer with expired cred. After fixCleanup3_4_0, + // doApply deletes the expired credential SLE and returns tecEXPIRED. + TER const expectedExpiredCredTer = fixEnabled ? tecEXPIRED : tecNO_PERMISSION; + env(offer(devin, XRP(10), USD(10)), Domain(domainID), Ter(expectedExpiredCredTer)); env.close(); } @@ -1510,7 +1516,9 @@ class PermissionedDEX_test : public beast::unit_test::Suite env.close(std::chrono::seconds(100)); // Confirm devin can no longer create domain offers. - env(offer(devin, XRP(1), USD(1)), Domain(domainID), Ter(tecNO_PERMISSION)); + // After fixCleanup3_4_0, OfferCreate deletes the expired credential and + // returns tecEXPIRED (covered in depth by testExpiredCredentialCleanup). + env(offer(devin, XRP(1), USD(1)), Domain(domainID), Ter(tecEXPIRED)); env.close(); // The hybrid offer must still exist in the open book after expiry. @@ -1635,6 +1643,202 @@ class PermissionedDEX_test : public beast::unit_test::Suite BEAST_EXPECT(!offerExists(env, bob, carolOfferSeq)); } + void + testExpiredCredentialCleanup(FeatureBitset features) + { + bool const fixEnabled = features[fixCleanup3_4_0]; + + testcase << "Expired credential cleanup" + << (fixEnabled ? " (Cleanup3_4_0 enabled)" : " (Cleanup3_4_0 disabled)"); + + TER const expectedExpiredCredTer = fixEnabled ? tecEXPIRED : tecNO_PERMISSION; + + auto const fundAccount = + [](Env& env, Account const& account, Account const& gw, IOU const& usd) { + env.fund(XRP(1000), account); + env.close(); + env.trust(usd(1000), account); + env.close(); + env(pay(gw, account, usd(100))); + env.close(); + }; + + auto const fundDevin = [&](Env& env, Account const& gw, IOU const& usd) { + Account const devin("devin"); + fundAccount(env, devin, gw, usd); + return devin; + }; + + auto const createExpiringCredential = [](Env& env, + Account const& subject, + Account const& issuer, + std::string const& credType) { + auto jv = credentials::create(subject, issuer, credType); + uint32_t const t = env.current()->header().parentCloseTime.time_since_epoch().count(); + jv[sfExpiration.jsonName] = t + 20; + env(jv); + env(credentials::accept(subject, issuer, credType)); + env.close(); + + return keylet::credential(subject.id(), issuer.id(), makeSlice(credType)); + }; + + auto const expectExpiredCredentialState = [&](Env const& env, Keylet const& credKey) { + if (fixEnabled) + { + BEAST_EXPECT(!env.le(credKey)); + } + else + { + BEAST_EXPECT(env.le(credKey)); + } + }; + + // A payment referencing a non-existent domain is rejected in preclaim. + { + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + uint256 const badDomain{ + "F10D0CC9A0F9A3CBF585B80BE09A186483668FDBDD39AA7E3370F3649CE134" + "E5"}; + + env(offer(bob, XRP(10), USD(10)), Domain(domainID)); + env.close(); + + env(pay(alice, bob, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(badDomain), + Ter(tecNO_PERMISSION)); + env.close(); + } + + // OfferCreate with an expired credential. + { + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + Account const devin = fundDevin(env, gw, USD); + auto const credKey = createExpiringCredential(env, devin, domainOwner, credType); + BEAST_EXPECT(env.le(credKey)); // credential exists before expiry + + env.close(std::chrono::seconds(20)); + + env(offer(devin, XRP(10), USD(10)), Domain(domainID), Ter(expectedExpiredCredTer)); + env.close(); + + expectExpiredCredentialState(env, credKey); + } + + // Payment where the sender's credential is expired. + { + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + Account const devin = fundDevin(env, gw, USD); + auto const credKey = createExpiringCredential(env, devin, domainOwner, credType); + + auto const bobOfferSeq{env.seq(bob)}; + auto const bobCredKey = + keylet::credential(bob.id(), domainOwner.id(), makeSlice(credType)); + env(offer(bob, XRP(10), USD(10)), Domain(domainID)); + env.close(); + + BEAST_EXPECT(env.le(credKey)); + BEAST_EXPECT(env.le(bobCredKey)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + + env.close(std::chrono::seconds(20)); + + env(pay(devin, alice, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(domainID), + Ter(expectedExpiredCredTer)); + env.close(); + + expectExpiredCredentialState(env, credKey); + BEAST_EXPECT(env.le(bobCredKey)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + } + + // Payment where the destination's credential is expired. + { + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + Account const devin = fundDevin(env, gw, USD); + auto const credKey = createExpiringCredential(env, devin, domainOwner, credType); + + auto const bobOfferSeq{env.seq(bob)}; + auto const bobCredKey = + keylet::credential(bob.id(), domainOwner.id(), makeSlice(credType)); + env(offer(bob, XRP(10), USD(10)), Domain(domainID)); + env.close(); + + BEAST_EXPECT(env.le(credKey)); + BEAST_EXPECT(env.le(bobCredKey)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + + env.close(std::chrono::seconds(20)); + + env(pay(alice, devin, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(domainID), + Ter(expectedExpiredCredTer)); + env.close(); + + expectExpiredCredentialState(env, credKey); + BEAST_EXPECT(env.le(bobCredKey)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + } + + // Payment where both sender and destination credentials are expired. + { + Env env(*this, features); + auto const& [gw, domainOwner, alice, bob, carol, USD, domainID, credType] = + PermissionedDEX(env); + + Account const devin = fundDevin(env, gw, USD); + Account const erin("erin"); + fundAccount(env, erin, gw, USD); + + auto const devinCredKey = createExpiringCredential(env, devin, domainOwner, credType); + auto const erinCredKey = createExpiringCredential(env, erin, domainOwner, credType); + + auto const bobOfferSeq{env.seq(bob)}; + auto const bobCredKey = + keylet::credential(bob.id(), domainOwner.id(), makeSlice(credType)); + env(offer(bob, XRP(10), USD(10)), Domain(domainID)); + env.close(); + + BEAST_EXPECT(env.le(devinCredKey)); + BEAST_EXPECT(env.le(erinCredKey)); + BEAST_EXPECT(env.le(bobCredKey)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + + env.close(std::chrono::seconds(20)); + + env(pay(devin, erin, USD(10)), + Path(~USD), + Sendmax(XRP(10)), + Domain(domainID), + Ter(expectedExpiredCredTer)); + env.close(); + + expectExpiredCredentialState(env, devinCredKey); + expectExpiredCredentialState(env, erinCredKey); + BEAST_EXPECT(env.le(bobCredKey)); + BEAST_EXPECT(offerExists(env, bob, bobOfferSeq)); + } + } + void testHybridMalformedOffer(FeatureBitset features) { @@ -2209,6 +2413,7 @@ public: // Test domain offer (w/o hybrid) testOfferCreate(all); testOfferCreate(all - fixCleanup3_2_0); + testOfferCreate(all - fixCleanup3_4_0); testPayment(all); testPayment(all - fixCleanup3_2_0); testBookStep(all); @@ -2219,6 +2424,8 @@ public: testAmmQualityNotLeaked(all); testAmmQualityNotLeaked(all - fixCleanup3_3_0); testAutoBridge(all); + testExpiredCredentialCleanup(all); + testExpiredCredentialCleanup(all - fixCleanup3_4_0); // Test hybrid offers testHybridOfferCreate(all); From 636d2d4851219f81f6475e6c18c088e3a895d544 Mon Sep 17 00:00:00 2001 From: Chenna Keshava B S <21219765+ckeshava@users.noreply.github.com> Date: Wed, 2 Sep 2026 20:48:53 +0000 Subject: [PATCH 05/62] fix: Reinforce the priority of AMMClawback in case of insufficient reserves (#7796) Co-authored-by: Claude Opus 4.8 (1M context) --- include/xrpl/ledger/helpers/TokenHelpers.h | 6 ++ include/xrpl/tx/transactors/dex/AMMWithdraw.h | 12 +++ .../tx/transactors/dex/AMMClawback.cpp | 4 + .../tx/transactors/dex/AMMWithdraw.cpp | 14 +++ src/test/app/AMMClawbackMPT_test.cpp | 85 +++++++++++++++++++ src/test/app/AMMClawback_test.cpp | 77 +++++++++++++++++ 6 files changed, 198 insertions(+) diff --git a/include/xrpl/ledger/helpers/TokenHelpers.h b/include/xrpl/ledger/helpers/TokenHelpers.h index 2a2f1b568e..12fa8a105e 100644 --- a/include/xrpl/ledger/helpers/TokenHelpers.h +++ b/include/xrpl/ledger/helpers/TokenHelpers.h @@ -38,6 +38,12 @@ enum class FreezeHandling { IgnoreFreeze, ZeroIfFrozen }; */ enum class AuthHandling { IgnoreAuth, ZeroIfUnauthorized }; +/** + * Controls whether the recipient owner-reserve check is enforced when + * auto-creating a trustline or MPToken during AMMWithdraw or AMMClawback. + */ +enum class ReserveHandling : bool { EnforceReserve, IgnoreReserve }; + /** * Controls whether to include the account's full spendable balance */ diff --git a/include/xrpl/tx/transactors/dex/AMMWithdraw.h b/include/xrpl/tx/transactors/dex/AMMWithdraw.h index 6861fa7bc4..ea0ad3b253 100644 --- a/include/xrpl/tx/transactors/dex/AMMWithdraw.h +++ b/include/xrpl/tx/transactors/dex/AMMWithdraw.h @@ -109,6 +109,11 @@ public: * @param lpTokens current LPT balance * @param lpTokensWithdraw amount of tokens to withdraw * @param tfee trading fee in basis points + * @param freezeHandling whether a frozen balance is reported as zero + * @param authHandling whether an unauthorized MPT balance is reported as + * zero + * @param reserveHandling whether the recipient owner-reserve check is + * enforced when a trustline or MPToken has to be auto-created * @param withdrawAll if withdrawing all lptokens * @param priorBalance balance before fees * @return @@ -128,6 +133,7 @@ public: std::uint16_t tfee, FreezeHandling freezeHandling, AuthHandling authHandling, + ReserveHandling reserveHandling, WithdrawAll withdrawAll, XRPAmount const& priorBalance, beast::Journal const& journal); @@ -150,6 +156,11 @@ public: * @param lpTokensAMMBalance current AMM LPT balance * @param lpTokensWithdraw amount of lptokens to withdraw * @param tfee trading fee in basis points + * @param freezeHandling whether a frozen balance is reported as zero + * @param authHandling whether an unauthorized MPT balance is reported as + * zero + * @param reserveHandling whether the recipient owner-reserve check is + * enforced when a trustline or MPToken has to be auto-created * @param withdrawAll if withdraw all lptokens * @param priorBalance balance before fees * @return @@ -169,6 +180,7 @@ public: std::uint16_t tfee, FreezeHandling freezeHandling, AuthHandling authHandling, + ReserveHandling reserveHandling, WithdrawAll withdrawAll, XRPAmount const& priorBalance, beast::Journal const& journal); diff --git a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp index b25c90069c..f95f257ab6 100644 --- a/src/libxrpl/tx/transactors/dex/AMMClawback.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMClawback.cpp @@ -237,6 +237,7 @@ AMMClawback::applyGuts(Sandbox& sb) 0, FreezeHandling::IgnoreFreeze, AuthHandling::IgnoreAuth, + ReserveHandling::IgnoreReserve, WithdrawAll::Yes, preFeeBalance_, ctx_.journal); @@ -345,6 +346,7 @@ AMMClawback::equalWithdrawMatchingOneAmount( 0, FreezeHandling::IgnoreFreeze, AuthHandling::IgnoreAuth, + ReserveHandling::IgnoreReserve, WithdrawAll::Yes, preFeeBalance_, ctx_.journal); @@ -385,6 +387,7 @@ AMMClawback::equalWithdrawMatchingOneAmount( 0, FreezeHandling::IgnoreFreeze, AuthHandling::IgnoreAuth, + ReserveHandling::IgnoreReserve, WithdrawAll::No, preFeeBalance_, ctx_.journal); @@ -406,6 +409,7 @@ AMMClawback::equalWithdrawMatchingOneAmount( 0, FreezeHandling::IgnoreFreeze, AuthHandling::IgnoreAuth, + ReserveHandling::IgnoreReserve, WithdrawAll::No, preFeeBalance_, ctx_.journal); diff --git a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp index 7744c128af..77b9071cf8 100644 --- a/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp +++ b/src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp @@ -527,6 +527,7 @@ AMMWithdraw::withdraw( tfee, issuerFreezeHandling(), AuthHandling::ZeroIfUnauthorized, + ReserveHandling::EnforceReserve, isWithdrawAll(ctx_.tx), preFeeBalance_, j_); @@ -548,6 +549,7 @@ AMMWithdraw::withdraw( std::uint16_t tfee, FreezeHandling freezeHandling, AuthHandling authHandling, + ReserveHandling reserveHandling, WithdrawAll withdrawAll, XRPAmount const& priorBalance, beast::Journal const& journal) @@ -681,6 +683,14 @@ AMMWithdraw::withdraw( }); if (assetNotExists) { + // Intentionally ignore the reserve check for AMMClawback, so the + // holder can not avoid clawback by deleting the trustline/MPToken + // and keeping a low spendable balance. AMMClawback has a higher + // priority than the reserve check. + if (view.rules().enabled(fixCleanup3_4_0) && + reserveHandling == ReserveHandling::IgnoreReserve) + return tesSUCCESS; + auto sleAccount = view.peek(keylet::account(account)); if (!sleAccount) return tecINTERNAL; // LCOV_EXCL_LINE @@ -850,6 +860,7 @@ AMMWithdraw::equalWithdrawTokens( tfee, issuerFreezeHandling(), AuthHandling::ZeroIfUnauthorized, + ReserveHandling::EnforceReserve, isWithdrawAll(ctx_.tx), preFeeBalance_, ctx_.journal); @@ -903,6 +914,7 @@ AMMWithdraw::equalWithdrawTokens( std::uint16_t tfee, FreezeHandling freezeHandling, AuthHandling authHandling, + ReserveHandling reserveHandling, WithdrawAll withdrawAll, XRPAmount const& priorBalance, beast::Journal const& journal) @@ -926,6 +938,7 @@ AMMWithdraw::equalWithdrawTokens( tfee, freezeHandling, authHandling, + reserveHandling, WithdrawAll::Yes, priorBalance, journal); @@ -962,6 +975,7 @@ AMMWithdraw::equalWithdrawTokens( tfee, freezeHandling, authHandling, + reserveHandling, withdrawAll, priorBalance, journal); diff --git a/src/test/app/AMMClawbackMPT_test.cpp b/src/test/app/AMMClawbackMPT_test.cpp index 1d75c4db22..44eb61395a 100644 --- a/src/test/app/AMMClawbackMPT_test.cpp +++ b/src/test/app/AMMClawbackMPT_test.cpp @@ -2198,6 +2198,89 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite BEAST_EXPECT(amm.ammExists()); } + void + testClawbackBypassesReserve(FeatureBitset features) + { + // Same as the IOU case, but the paired asset is an MPT alice does not + // hold yet. The reserve check is skipped on the clawback path while + // createMPToken() still runs, so alice's MPToken is created even though + // neither she nor the low-XRP issuer can cover the owner reserve. + testcase("test clawback bypasses recipient reserve (MPT)"); + using namespace jtx; + + Env env(*this, features); + Account const gw{"gateway"}; // IOU issuer + claw authority, low XRP + Account const gw2{"gateway2"}; // MPT issuer of the paired asset + Account const carol{"carol"}; + Account const alice{"alice"}; + + auto const usd = gw["USD"]; + auto const baseFee = env.current()->fees().base; + + env.fund(XRP(1'000'000), gw2, carol); + // Low XRP so the legacy issuer-balance check cannot pass. + env.fund(env.current()->fees().accountReserve(0, 1) + baseFee * 10, gw); + // Reserve for the USD trustline and LP token trustline. + env.fund(env.current()->fees().accountReserve(2, 1) + baseFee * 5, alice); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + + // The paired MPT: transferable so an AMM can hold it, and no + // RequireAuth so createMPToken()'s WeakAuth check passes. + MPT const btc = MPTTester( + {.env = env, + .issuer = gw2, + .holders = {carol}, + .pay = 1'000'000, + .flags = kMptDexFlags}); + + env.trust(usd(1'000'000), carol); + env(pay(gw, carol, usd(100'000))); + env.close(); + AMM amm(env, carol, usd(1'000), btc(1'000), Ter(tesSUCCESS)); + env.close(); + + // alice holds a USD trustline and LP tokens, but no BTC MPToken. + env.trust(usd(100'000), alice); + env(pay(gw, alice, usd(1'000))); + env.close(); + amm.deposit(alice, usd(100)); + + BEAST_EXPECT(env.ownerCount(alice) == 2); + BEAST_EXPECT(!env.le(keylet::mptoken(btc.issuanceID, alice.id()))); + + // AMMWithdraw still enforces the reserve check. + amm.withdrawAll(alice, std::nullopt, Ter(tecINSUFFICIENT_RESERVE)); + BEAST_EXPECT(!env.le(keylet::mptoken(btc.issuanceID, alice.id()))); + BEAST_EXPECT(env.ownerCount(alice) == 2); + // alice cannot afford a third owner object. + BEAST_EXPECT(env.balance(alice) < STAmount(env.current()->fees().accountReserve(3, 1))); + + if (features[fixCleanup3_4_0]) + { + // Reserve check skipped; the paired BTC returns to alice on a + // newly created MPToken. + env(amm::ammClawback(gw, alice, usd, btc, usd(10)), Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT(env.le(keylet::mptoken(btc.issuanceID, alice.id()))); + BEAST_EXPECT(env.balance(alice, btc) > btc(0)); + BEAST_EXPECT(env.ownerCount(alice) == 3); + } + else + { + // Legacy path: the check runs against max(issuer, holder) XRP, + // neither of which covers a third owner object. + env(amm::ammClawback(gw, alice, usd, btc, usd(10)), Ter(tecINSUFFICIENT_RESERVE)); + env.close(); + + BEAST_EXPECT(!env.le(keylet::mptoken(btc.issuanceID, alice.id()))); + BEAST_EXPECT(env.ownerCount(alice) == 2); + } + } + void run() override { @@ -2225,6 +2308,8 @@ class AMMClawbackMPT_test : public beast::unit_test::Suite featureLendingProtocol); testLastHolderLPTokenBalance(all - fixAMMClawbackRounding); testClawAssetCheck(all); + testClawbackBypassesReserve(all); + testClawbackBypassesReserve(all - fixCleanup3_4_0); } }; diff --git a/src/test/app/AMMClawback_test.cpp b/src/test/app/AMMClawback_test.cpp index 230d148ff9..4f025f08eb 100644 --- a/src/test/app/AMMClawback_test.cpp +++ b/src/test/app/AMMClawback_test.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -2715,6 +2716,81 @@ class AMMClawback_test : public beast::unit_test::Suite } } + void + testClawbackBypassesReserve(FeatureBitset features) + { + // Clawback must not fail the holder-side reserve check: a holder could + // otherwise veto it by omitting the paired trustline. AMMWithdraw still + // enforces the check. Pre-fixCleanup3_4_0 the holder's reserve was + // compared against max(issuer pre-fee, holder current) XRP, so the + // clawback was blocked when neither balance covered it. + testcase("test clawback bypasses recipient reserve"); + using namespace jtx; + + Env env(*this, features); + Account const gw{"gateway"}; + Account const carol{"carol"}; + Account const alice{"alice"}; + + auto const usd = gw["USD"]; + auto const eur = gw["EUR"]; + auto const baseFee = env.current()->fees().base; + + env.fund(XRP(1'000'000), carol); + // Low XRP so the legacy issuer-balance check cannot pass. + env.fund(env.current()->fees().accountReserve(0, 1) + baseFee * 10, gw); + // Reserve for the USD trustline and LP token trustline. + env.fund(env.current()->fees().accountReserve(2, 1) + baseFee * 5, alice); + env.close(); + + env(fset(gw, asfAllowTrustLineClawback)); + env.close(); + + env.trust(usd(1'000'000), carol); + env.trust(eur(1'000'000), carol); + env(pay(gw, carol, usd(100'000))); + env(pay(gw, carol, eur(100'000))); + env.close(); + AMM amm(env, carol, usd(1'000), eur(1'000), Ter(tesSUCCESS)); + env.close(); + + // alice holds a USD trustline and LP tokens, but no EUR trustline. + env.trust(usd(100'000), alice); + env(pay(gw, alice, usd(1'000))); + env.close(); + amm.deposit(alice, usd(100)); + + BEAST_EXPECT(env.ownerCount(alice) == 2); + // alice cannot afford a third owner object. + BEAST_EXPECT(env.balance(alice) < STAmount(env.current()->fees().accountReserve(3, 1))); + + // AMMWithdraw still enforces the reserve check. + amm.withdraw( + WithdrawArg{ + .account = alice, .asset1Out = eur(1), .err = Ter(tecINSUFFICIENT_RESERVE)}); + BEAST_EXPECT(env.ownerCount(alice) == 2); + + if (features[fixCleanup3_4_0]) + { + // Reserve check skipped; the paired EUR returns to alice on a + // newly created EUR trustline. + env(amm::ammClawback(gw, alice, usd, eur, usd(10)), Ter(tesSUCCESS)); + env.close(); + + BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), eur.issue()))); + BEAST_EXPECT(env.balance(alice, eur) > eur(0)); + BEAST_EXPECT(env.ownerCount(alice) == 3); + } + else + { + // Legacy path: the check runs against max(issuer, holder) XRP, + // neither of which covers a third owner object. + env(amm::ammClawback(gw, alice, usd, eur, usd(10)), Ter(tecINSUFFICIENT_RESERVE)); + env.close(); + BEAST_EXPECT(env.ownerCount(alice) == 2); + } + } + void testExactLPTokenEquality(FeatureBitset features) { @@ -2809,6 +2885,7 @@ class AMMClawback_test : public beast::unit_test::Suite testAssetFrozen(features); testSingleDepositAndClawback(features); testLastHolderLPTokenBalance(features); + testClawbackBypassesReserve(features); testExactLPTokenEquality(features); } } From 49cdc105de5c3d5773ae0ebb0c523649d7c86439 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Wed, 2 Sep 2026 21:18:13 +0000 Subject: [PATCH 06/62] fix: Correct and simplify Linux packaging (#8165) --- .github/workflows/reusable-package.yml | 173 +++++++++++++++++++++++-- bin/install-packaging-tools.sh | 6 +- package/README.md | 84 +++++++----- package/build_pkg.py | 16 ++- package/debian/control | 3 +- package/debian/copyright | 75 ++++++++++- package/debian/rules | 22 ++++ package/debian/xrpld.docs | 1 + package/debian/xrpld.links | 3 +- package/debian/xrpld.lintian-overrides | 6 + package/docker/Dockerfile | 6 +- package/docker/publish_pkg.py | 17 ++- package/rpm/xrpld.spec | 23 +++- package/shared/xrpld.service | 2 + package/sign_rpm.py | 2 + 15 files changed, 365 insertions(+), 74 deletions(-) mode change 100644 => 100755 package/debian/rules create mode 100644 package/debian/xrpld.lintian-overrides diff --git a/.github/workflows/reusable-package.yml b/.github/workflows/reusable-package.yml index 2a5e6a8c04..3986feed7f 100644 --- a/.github/workflows/reusable-package.yml +++ b/.github/workflows/reusable-package.yml @@ -1,11 +1,14 @@ -# Build Linux packages from the pre-built xrpld and validator-keys artifacts: +# Build, verify and publish Linux packages from the pre-built xrpld and +# validator-keys artifacts, in three stages: # -# - one job per config that carries a "package" map in linux.json -# - that map names the container image and the format it builds there -# - every job ends with the image's publish_pkg.py, uploading what it built -# with 'publish: true' and doing a --dry-run otherwise +# - 'package' builds and signs one format per config that carries a "package" +# map in linux.json; that map names the container image and the format +# - 'test-install' installs what was built on a range of distros and runs the +# binaries there, so a package that cannot be installed never reaches Nexus +# - 'publish' uploads with the image's publish_pkg.py, doing a --dry-run +# unless 'publish: true' # -# Only linux/amd64 is supported; the runner is hardcoded in the job below. +# Only linux/amd64 is supported; the runner is hardcoded in the jobs below. name: Package on: @@ -39,6 +42,7 @@ defaults: env: BUILD_DIR: build + PACKAGE_DIR: packages jobs: generate-matrix: @@ -70,7 +74,7 @@ jobs: contents: read runs-on: ["self-hosted", "Linux", "X64", "heavy"] container: ${{ matrix.image }} - timeout-minutes: 30 + timeout-minutes: 10 steps: - name: Checkout repository @@ -112,24 +116,167 @@ jobs: --pkg-release "${PKG_RELEASE}" \ --channel "${CHANNEL}" - # Before the upload, so the artifact and the published package are the - # same bytes. DEBs are not signed, so the key is never set on that job. + # Before the upload, so the artifact, the tested package and the published + # package are the same bytes. - name: Sign RPM if: ${{ inputs.publish && matrix.package_type == 'rpm' }} env: PKG_SIGNING_KEY: ${{ secrets.signing_key }} run: ./package/sign_rpm.py --package-dir "${BUILD_DIR}" + # Split from the debug symbols, which are an order of magnitude larger, so + # that test-install downloads only what it installs. - name: Upload package artifact uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: ${{ matrix.xrpld_artifact_name }}-pkg path: | - ${{ env.BUILD_DIR }}/debbuild/*.deb - ${{ env.BUILD_DIR }}/debbuild/*.ddeb - ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/*.rpm + ${{ env.BUILD_DIR }}/debbuild/xrpld_*.deb + ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/xrpld-[0-9]*.rpm if-no-files-found: error + - name: Upload debug symbol artifact + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: ${{ matrix.xrpld_artifact_name }}-pkg-debug + path: | + ${{ env.BUILD_DIR }}/debbuild/xrpld-dbgsym_*.deb + ${{ env.BUILD_DIR }}/debbuild/xrpld-dbgsym_*.ddeb + ${{ env.BUILD_DIR }}/rpmbuild/RPMS/**/xrpld-debuginfo-*.rpm + if-no-files-found: error + + # Every distro family the packages target, oldest release first, so both ends + # of the dependency range they declare are exercised. + test-install: + needs: [package] + strategy: + fail-fast: false + matrix: + include: + - package_type: deb + image: debian:11 + - package_type: deb + image: debian:12 + - package_type: deb + image: debian:13 + - package_type: deb + image: ubuntu:20.04 + - package_type: deb + image: ubuntu:22.04 + - package_type: deb + image: ubuntu:24.04 + - package_type: deb + image: ubuntu:26.04 + + - package_type: rpm + image: almalinux:9 + - package_type: rpm + image: almalinux:10 + - package_type: rpm + image: rockylinux/rockylinux:9 + - package_type: rpm + image: rockylinux/rockylinux:10 + - package_type: rpm + image: registry.access.redhat.com/ubi9/ubi + - package_type: rpm + image: registry.access.redhat.com/ubi10/ubi + name: "install ${{ matrix.package_type }} on ${{ matrix.image }}" + permissions: + contents: read + runs-on: ubuntu-latest + container: ${{ matrix.image }} + timeout-minutes: 5 + + steps: + # Both formats land in one directory; the step below picks its own by + # extension, so this stays independent of the artifact names. + - name: Download package artifacts + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: "*-pkg" + merge-multiple: true + path: ${{ env.PACKAGE_DIR }} + + - name: Find the package + id: find + env: + PACKAGE_TYPE: ${{ matrix.package_type }} + run: | + package="$(find "${PACKAGE_DIR}" -type f -name "*.${PACKAGE_TYPE}" -print -quit)" + test -n "${package}" || { + echo "no .${PACKAGE_TYPE} found in ${PACKAGE_DIR}" >&2 + exit 1 + } + echo "package=${package}" >>"${GITHUB_OUTPUT}" + + - name: Install the DEB + if: ${{ matrix.package_type == 'deb' }} + env: + DEBIAN_FRONTEND: noninteractive + PACKAGE: ${{ steps.find.outputs.package }} + run: | + # Stock Debian and Ubuntu images carry no package lists, so apt has + # nothing to resolve the systemd dependency from until it fetches them. + apt-get update -qq + apt-get install -y "./${PACKAGE}" + + - name: Install the RPM + if: ${{ matrix.package_type == 'rpm' }} + env: + PACKAGE: ${{ steps.find.outputs.package }} + run: dnf install -y "./${PACKAGE}" + + - name: Run xrpld + run: xrpld --version + + - name: Run validator-keys + run: validator-keys --version + + - name: Run rippled, the legacy compatibility symlink + run: rippled --version + + - name: Check the service account + run: id xrpld + + - name: Check the state directory + run: test -d /var/lib/xrpld + + - name: Check the log directory + run: test -d /var/log/xrpld + + publish: + needs: [generate-matrix, package, test-install] + strategy: + fail-fast: false + matrix: ${{ fromJson(needs.generate-matrix.outputs.matrix) }} + name: "publish ${{ matrix.xrpld_artifact_name }}" + permissions: + contents: read + runs-on: ["self-hosted", "Linux", "X64", "heavy"] + container: ${{ matrix.image }} + timeout-minutes: 30 + + steps: + - name: Checkout repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Prepare runner + uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + with: + enable_ccache: false + + # Both artifacts, so the debug symbols are published alongside the package. + - name: Download package artifacts + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + pattern: ${{ matrix.xrpld_artifact_name }}-pkg* + merge-multiple: true + path: ${{ env.PACKAGE_DIR }} + + - name: Determine release info + id: release_info + uses: ./.github/actions/release-info + - name: Publish package env: CHANNEL: ${{ steps.release_info.outputs.channel }} @@ -140,6 +287,6 @@ jobs: run: | publish_pkg.py \ --channel "${CHANNEL}" \ - --package-dir "${BUILD_DIR}" \ + --package-dir "${PACKAGE_DIR}" \ --nexus-url "${NEXUS_URL}" \ ${DRY_RUN_OPTION} diff --git a/bin/install-packaging-tools.sh b/bin/install-packaging-tools.sh index 36557364ae..0d1fce3055 100755 --- a/bin/install-packaging-tools.sh +++ b/bin/install-packaging-tools.sh @@ -25,7 +25,9 @@ esac # Packaging runs in a vanilla distro image, so the tooling comes from the distro's # archive rather than from nixpkgs: # -# - debhelper and dpkg-dev build the DEB +# - debhelper and dpkg-dev build the DEB, and lintian checks it +# - binutils gives debian/rules the readelf its glibc-floor check runs; it +# already arrives via dpkg-dev, but that tool is called directly # - rpm-build builds the RPM, with systemd-rpm-macros and redhat-rpm-config # supplying the systemd and find-debuginfo macros the spec uses # - rpm-sign and gnupg2 sign the built RPM @@ -37,11 +39,13 @@ function install() { debian | ubuntu) apt-get update -y apt-get install -y --no-install-recommends \ + binutils \ ca-certificates \ debhelper \ debhelper-compat \ dpkg-dev \ git \ + lintian \ python3 ;; diff --git a/package/README.md b/package/README.md index 027a374898..6e88309ecd 100644 --- a/package/README.md +++ b/package/README.md @@ -15,7 +15,7 @@ package/ publish_pkg.py Uploads built packages to the XRPLF Nexus repositories (called by CI, and shipped in that image) rpm/ xrpld.spec RPM spec - debian/ Debian control files (control, rules, copyright, xrpld.docs, xrpld.links, source/format) + debian/ Debian control files (control, rules, copyright, xrpld.docs, xrpld.links, xrpld.lintian-overrides, source/format) shared/ xrpld.service systemd unit file (used by both RPM and DEB) xrpld.sysusers sysusers.d config (used by both RPM and DEB) @@ -34,10 +34,10 @@ image and both CI and local builds pick it up — and names the format that imag builds in `type`, which CI passes to `build_pkg.py` as `--package-type`; the two have to stay in step. -| Package type | Image (`configs.[].package.image` in `linux.json`) | Tools required | -| ------------ | ---------------------------------------------------------- | --------------------------------------------------- | -| RPM | `ghcr.io/xrplf/xrpld/packaging-rhel:sha-` | `rpmbuild`, `rpmsign` | -| DEB | `ghcr.io/xrplf/xrpld/packaging-debian:sha-` | `dpkg-buildpackage`, debhelper with compat level 13 | +| Package type | Image (`configs.[].package.image` in `linux.json`) | Tools required | +| ------------ | ---------------------------------------------------------- | -------------------------------------------------------------- | +| RPM | `ghcr.io/xrplf/xrpld/packaging-rhel:sha-` | `rpmbuild`, `rpmsign` | +| DEB | `ghcr.io/xrplf/xrpld/packaging-debian:sha-` | `dpkg-buildpackage`, debhelper with compat level 13, `lintian` | To print the full packaging matrix (artifact names and images) for the current `linux.json`: @@ -51,13 +51,19 @@ To print the full packaging matrix (artifact names and images) for the current ### Via CI Caller workflows (`on-pr.yml`, `on-tag.yml`, `on-trigger.yml`) call -`reusable-package.yml`. That workflow generates its own packaging matrix from -the configs that carry a `package` map (via `generate.py --packaging`) and fans -out one job per distro. Each job downloads the pre-built `xrpld` and -`validator-keys` binary artifacts and runs in that distro's container, building -the format `package.type` declares. The packaging script derives the package -version from the downloaded binary's `xrpld --version` output; no CMake -configure or build step is needed inside the packaging job. +`reusable-package.yml`, which runs in three stages: + +1. `package` fans out one job per config carrying a `package` map, building and + signing in that config's container, and uploading `-pkg` alongside + `-pkg-debug` for the much larger debug symbols. +2. `test-install` installs `-pkg` in the container of every distro the + packages target and runs the binaries there, so one that cannot be installed + never reaches Nexus. +3. `publish` uploads both artifacts, or lists what it would upload. + +The packaging script derives the package version from the downloaded binary's +`xrpld --version` output; no CMake configure or build step is needed inside the +packaging job. The binaries come from the `debian` and `rhel` build configs themselves — the ones carrying the `package` map — which pass `-Dvalidator_keys=ON` so that the @@ -94,8 +100,7 @@ docker run --rm \ --pkg-release "${PKG_RELEASE}" \ --channel UNRELEASED -# Output: -# build/debbuild/*.deb (DEB + dbgsym; Debian names both .deb) +# Output (the deb image writes build/debbuild/*.deb instead): # build/rpmbuild/RPMS/x86_64/*.rpm ``` @@ -155,9 +160,9 @@ the last, and the date and hash say which commit a package on `packages.xrplf.org` came from. Both reach the packaging scripts as arguments, so neither script derives anything itself. -Publishing is the last step of each packaging job, uploading from the container -that built the packages with the `publish_pkg.py` shipped in the image — the -same copy other repositories run. Without `publish: true` the step is a +Publishing is its own job, gated behind `test-install`, uploading from the same +image that built the packages with the `publish_pkg.py` shipped in it — the +same copy other repositories run. Without `publish: true` the job is a `--dry-run`, listing the uploads it would make without needing credentials, so any run that builds packages also exercises the upload routing. `on-trigger.yml` passes `publish: true` for develop pushes in `XRPLF/rippled` and `on-tag.yml` @@ -202,6 +207,9 @@ the final release. If that normalized package version still contains `-`, packaging fails because RPM forbids `-` in `Version`, and Debian uses `-` as the upstream/revision separator. +> [!NOTE] +> Debug and sanitizer builds are not packaged yet. + `pkg_version` is the normalized package metadata version derived inside `build_pkg.py` from the binary-reported `xrpld` version (`-` pre-release separator converted to `~`). It is not a separate user input. @@ -279,37 +287,45 @@ service restart. 2. Stages the binaries, configs, `README.md`, `LICENSE.md`, and `validator-keys-LICENSE`. 3. Copies `package/debian/` control files into `debbuild/source/debian/`. -4. Copies shared service/sysusers/tmpfiles into `debian/` where `dh_installsystemd`, `dh_installsysusers`, and `dh_installtmpfiles` pick them up automatically. +4. Copies shared service/sysusers/tmpfiles/logrotate into `debian/` where `dh_installsystemd`, `dh_installsysusers`, `dh_installtmpfiles` and `dh_installlogrotate` pick them up automatically. 5. Generates a minimal `debian/changelog` using `${pkg_version}-${PKG_RELEASE}`, where `pkg_version` is derived from the binary-reported `xrpld` version. 6. Runs `dpkg-buildpackage -b --no-sign -d` (`-d` skips the build-dependency check, since the binary is already built). `debian/rules` uses manual `install` commands. + + It also rewrites the `libc6` bound to `LIBC_MIN` in `debian/rules`, the glibc + the Nix toolchain builds against. `dpkg-shlibdeps` would otherwise derive it + from the build host's symbols file — on trixie that yields `libc6 (>= 2.34)` + because of `sysconf`, locking out distros the binaries run on. A check fails + the build if either binary outgrows `LIBC_MIN`. + 7. Output: `debbuild/*.deb`, the binary package and the `-dbgsym` package. Debian gives dbgsym packages a `.deb` extension; only Ubuntu uses `.ddeb`. ## Post-build verification ```bash -# DEB -dpkg-deb -c debbuild/*.deb | grep -E 'systemd|sysusers|tmpfiles' +# DEB (one invocation per package: the dbgsym package is a .deb too) +for deb in debbuild/*.deb; do dpkg-deb -c "${deb}"; done | grep -E 'systemd|sysusers|tmpfiles' +lintian -I debbuild/*.deb # RPM rpm -qlp rpmbuild/RPMS/x86_64/*.rpm - -# Optional, and not in the packaging image: apt-get install -y lintian -lintian -I debbuild/*.deb ``` +`lintian` still reports `embedded-library zlib`, `no-manual-page` and +`initial-upload-closes-no-bugs`; only the `/usr/local` tags are overridden. + ## Reproducibility -`build_pkg.py` sets `SOURCE_DATE_EPOCH` from the latest git commit time and -exports it; the RPM spec clamps file modification times to it via -`%build_mtime_policy`. The remaining variables -below further improve reproducibility but are _not_ set by the script — export -them yourself if needed: +Both formats build reproducibly as they are: the same binaries at the same +commit give byte-identical packages on a rebuild, and nothing has to be +exported by hand. -```bash -export TZ=UTC -export LC_ALL=C.UTF-8 -export GZIP=-n -export DEB_BUILD_OPTIONS="noautodbgsym reproducible=+fixfilepath" -``` +`build_pkg.py` sets `SOURCE_DATE_EPOCH` from the latest git commit time. +`dpkg-buildpackage` honours it on its own; the RPM spec sets three macros: + +- `%clamp_mtime_to_source_date_epoch` — file modification times, from + `SOURCE_DATE_EPOCH`. +- `%use_source_date_epoch_as_buildtime` — the `BUILDTIME` header, from the + same. +- `%_buildhost` — pinned, so the builder's hostname stays out of the header. diff --git a/package/build_pkg.py b/package/build_pkg.py index 2518d8c1db..1aaf53d5ff 100755 --- a/package/build_pkg.py +++ b/package/build_pkg.py @@ -19,7 +19,7 @@ from pathlib import Path # This script lives in the repository it packages. SRC_DIR = Path(__file__).resolve().parents[1] -PRE_RELEASE = re.compile(r"^(b0|b[1-9][0-9]*|rc[0-9]+)(\+.*)?$") +PRE_RELEASE = re.compile(r"^(b|rc)(0|[1-9][0-9]*)(\+.*)?$") # Files both packaging systems consume, staged under the same names. STAGED_FROM_BUILD = ("xrpld", "validator-keys", "validator-keys-LICENSE") @@ -133,6 +133,14 @@ def stage_common(build_dir: Path, dest: Path) -> None: shutil.copy2(build_dir / name, dest / name) for source, name in STAGED_FROM_SRC.items(): shutil.copy2(SRC_DIR / source, dest / name) + + +def stage_units(dest: Path) -> None: + """Copy the systemd, sysusers, tmpfiles and logrotate files into dest. + + Each format wants them somewhere else: rpmbuild reads them from SOURCES, + debhelper from debian/. + """ for name in STAGED_UNITS: shutil.copy2(SRC_DIR / "package" / "shared" / name, dest / name) @@ -146,6 +154,7 @@ def build_rpm(build_dir: Path, *, version: str, pkg_release: str) -> None: spec = topdir / "SPECS" / "xrpld.spec" shutil.copy2(SRC_DIR / "package" / "rpm" / "xrpld.spec", spec) stage_common(build_dir, topdir / "SOURCES") + stage_units(topdir / "SOURCES") run( "rpmbuild", @@ -178,8 +187,7 @@ def build_deb( shutil.copytree(SRC_DIR / "package" / "debian", staging / "debian") # debhelper picks these up from debian/ automatically. - for name in STAGED_UNITS: - shutil.copy2(staging / name, staging / "debian" / name) + stage_units(staging / "debian") date = datetime.fromtimestamp(epoch, timezone.utc).strftime( "%a, %d %b %Y %H:%M:%S %z" @@ -193,8 +201,6 @@ def build_deb( """) (staging / "debian" / "changelog").write_text(changelog) - (staging / "debian" / "rules").chmod(0o755) - run("dpkg-buildpackage", "-b", "--no-sign", "-d", cwd=staging) diff --git a/package/debian/control b/package/debian/control index 62e5d79ef1..359f39f770 100644 --- a/package/debian/control +++ b/package/debian/control @@ -4,6 +4,7 @@ Priority: optional Maintainer: XRPL Foundation Rules-Requires-Root: no Build-Depends: + binutils, debhelper-compat (= 13) Standards-Version: 4.7.0 Homepage: https://github.com/XRPLF/rippled @@ -11,8 +12,6 @@ Vcs-Git: https://github.com/XRPLF/rippled.git Vcs-Browser: https://github.com/XRPLF/rippled Package: xrpld -Section: net -Priority: optional Architecture: any Depends: ${shlibs:Depends}, diff --git a/package/debian/copyright b/package/debian/copyright index 2cf673854a..baaa12e13c 100644 --- a/package/debian/copyright +++ b/package/debian/copyright @@ -1,5 +1,5 @@ Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ -Upstream-Name: rippled +Upstream-Name: xrpld Source: https://github.com/XRPLF/rippled Files: * @@ -15,7 +15,7 @@ Copyright: 2016, Ripple Labs Inc. 2009-2010, Satoshi Nakamoto 2011, The Bitcoin developers 2003-2005, Tom Wu -License: ISC +License: ISC and BSL-1.0 and MIT and Tom-Wu Comment: Built from https://github.com/ripple/validator-keys-tool at the commit pinned in cmake/XrplValidatorKeys.cmake. Besides ISC-licensed code it incorporates work under the Boost Software License 1.0 (ASIO), the MIT/X11 @@ -35,3 +35,74 @@ License: ISC WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + +License: BSL-1.0 + Boost Software License - Version 1.0 - August 17th, 2003 + . + Permission is hereby granted, free of charge, to any person or organization + obtaining a copy of the software and accompanying documentation covered by + this license (the "Software") to use, reproduce, display, distribute, + execute, and transmit the Software, and to prepare derivative works of the + Software, and to permit third-parties to whom the Software is furnished to + do so, all subject to the following: + . + The copyright notices in the Software and this entire statement, including + the above license grant, this restriction and the following disclaimer, + must be included in all copies of the Software, in whole or in part, and + all derivative works of the Software, unless such copies or derivative + works are solely in the form of machine-executable object code generated by + a source language processor. + . + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT + SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE + FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR OTHERWISE, + ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. + +License: MIT + Permission is hereby granted, free of charge, to any person obtaining a + copy of this software and associated documentation files (the "Software"), + to deal in the Software without restriction, including without limitation + the rights to use, copy, modify, merge, publish, distribute, sublicense, + and/or sell copies of the Software, and to permit persons to whom the + Software is furnished to do so, subject to the following conditions: + . + The above copyright notice and this permission notice shall be included in + all copies or substantial portions of the Software. + . + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + DEALINGS IN THE SOFTWARE. + +License: Tom-Wu + Permission is hereby granted, free of charge, to any person obtaining + a copy of this software and associated documentation files (the + "Software"), to deal in the Software without restriction, including + without limitation the rights to use, copy, modify, merge, publish, + distribute, sublicense, and/or sell copies of the Software, and to + permit persons to whom the Software is furnished to do so, subject to + the following conditions: + . + The above copyright notice and this permission notice shall be + included in all copies or substantial portions of the Software. + . + THE SOFTWARE IS PROVIDED "AS-IS" AND WITHOUT WARRANTY OF ANY KIND, + EXPRESS, IMPLIED OR OTHERWISE, INCLUDING WITHOUT LIMITATION, ANY + WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE. + . + IN NO EVENT SHALL TOM WU BE LIABLE FOR ANY SPECIAL, INCIDENTAL, + INDIRECT OR CONSEQUENTIAL DAMAGES OF ANY KIND, OR ANY DAMAGES WHATSOEVER + RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER OR NOT ADVISED OF + THE POSSIBILITY OF DAMAGE, AND ON ANY THEORY OF LIABILITY, ARISING OUT + OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + . + In addition, the following condition applies: + . + All redistributions must retain an intact copy of this copyright notice + and disclaimer. diff --git a/package/debian/rules b/package/debian/rules old mode 100644 new mode 100755 index 8f880b8192..4a9e4ab281 --- a/package/debian/rules +++ b/package/debian/rules @@ -2,6 +2,12 @@ export DH_VERBOSE = 1 +# The glibc the Nix toolchain builds against, and so the real floor for the +# binaries. dpkg-shlibdeps would instead derive libc6 (>= 2.34) from the build +# host's symbols file, where sysconf carries that minver, locking out distros +# the binaries actually run on. +LIBC_MIN = 2.31 + %: dh $@ @@ -11,6 +17,8 @@ override_dh_auto_configure override_dh_auto_build override_dh_auto_test: override_dh_installsystemd: dh_installsystemd --no-stop-on-upgrade xrpld.service +# The tmpfiles snippet sets ownership to the xrpld user, so the sysusers snippet +# has to be emitted first: run it early and make its own sequence slot a no-op. execute_before_dh_installtmpfiles: dh_installsysusers @@ -22,5 +30,19 @@ override_dh_install: install -D -m 0644 xrpld.cfg debian/xrpld/etc/xrpld/xrpld.cfg install -D -m 0644 validators.txt debian/xrpld/etc/xrpld/validators.txt +override_dh_shlibdeps: + dh_shlibdeps + # Guards against the toolchain moving past LIBC_MIN and the packages then + # claiming a floor they do not meet. + for binary in xrpld validator-keys; do \ + needed=$$(readelf --dyn-syms --wide $$binary \ + | grep -o 'GLIBC_[0-9.]*' | sed 's/GLIBC_//' | sort -uV | tail -1); \ + if dpkg --compare-versions "$$needed" gt "$(LIBC_MIN)"; then \ + echo "$$binary needs glibc $$needed, above LIBC_MIN $(LIBC_MIN)" >&2; \ + exit 1; \ + fi; \ + done + sed -i 's/libc6 (>= [0-9.]*)/libc6 (>= $(LIBC_MIN))/' debian/xrpld.substvars + override_dh_dwz: @: diff --git a/package/debian/xrpld.docs b/package/debian/xrpld.docs index 77681ddc6e..97325dfcf5 100644 --- a/package/debian/xrpld.docs +++ b/package/debian/xrpld.docs @@ -1,2 +1,3 @@ README.md +LICENSE.md validator-keys-LICENSE diff --git a/package/debian/xrpld.links b/package/debian/xrpld.links index 10d34f5b8c..6dea4f28f3 100644 --- a/package/debian/xrpld.links +++ b/package/debian/xrpld.links @@ -1,2 +1,3 @@ -# Legacy compat symlinks (remove next major release) +# Legacy compatibility for pre-FHS package layouts. +# TODO: remove after rippled fully deprecated. usr/bin/xrpld usr/local/bin/rippled diff --git a/package/debian/xrpld.lintian-overrides b/package/debian/xrpld.lintian-overrides new file mode 100644 index 0000000000..a0b3f583ed --- /dev/null +++ b/package/debian/xrpld.lintian-overrides @@ -0,0 +1,6 @@ +# The /usr/local/bin/rippled symlink is deliberate compatibility for pre-FHS +# layouts, so the Policy 9.1.2 tags it raises are expected. +# TODO: remove alongside debian/xrpld.links after rippled fully deprecated. +xrpld: dir-in-usr-local [usr/local/bin/] +xrpld: file-in-usr-local [usr/local/bin/rippled] +xrpld: file-in-unusual-dir [usr/local/bin/rippled] diff --git a/package/docker/Dockerfile b/package/docker/Dockerfile index b55c37b02a..adf372b6fa 100644 --- a/package/docker/Dockerfile +++ b/package/docker/Dockerfile @@ -2,9 +2,9 @@ ARG BASE_IMAGE=debian:trixie FROM ${BASE_IMAGE} -COPY bin/install-packaging-tools.sh /tmp/install-packaging-tools.sh - -RUN /tmp/install-packaging-tools.sh +# Bind-mounted rather than copied in, so the installer never lands in a layer. +RUN --mount=type=bind,source=bin/install-packaging-tools.sh,target=/install-packaging-tools.sh \ + /install-packaging-tools.sh # See ../README.md, "Publishing from other repositories". COPY package/docker/publish_pkg.py /usr/local/bin/publish_pkg.py diff --git a/package/docker/publish_pkg.py b/package/docker/publish_pkg.py index c9a6d3db1e..84a0448e7b 100755 --- a/package/docker/publish_pkg.py +++ b/package/docker/publish_pkg.py @@ -1,8 +1,8 @@ #!/usr/bin/env python3 """Publish built DEB and RPM packages to the XRPLF repositories on Nexus. -Takes packages and a channel, and nothing else, so it publishes whatever built -them; see package/README.md, "Publishing from other repositories". +Knows nothing about what it uploads beyond the channel, so it publishes whatever +built the packages; see package/README.md, "Publishing from other repositories". RPMs are uploaded to the hosted repository, but yum clients install from the 'rpm-' group repository in front of it, which serves signed metadata. @@ -29,6 +29,9 @@ STALL_TIMEOUT = 300 ATTEMPTS = 4 RETRY_DELAY = 5 +# 429 is Nexus asking to slow down, not a rejection, so it retries like a 5xx. +RETRYABLE_STATUSES = (429,) + def build_opener() -> urllib.request.OpenerDirector: """An opener with no redirect handler, so a 3xx raises instead of being followed. @@ -47,9 +50,9 @@ def build_opener() -> urllib.request.OpenerDirector: def upload(url: str, method: str, headers: dict[str, str], package: Path) -> None: """Send one package, retrying only what is worth retrying. - A 4xx is a deterministic rejection, so it is reported at once rather than - re-sending the whole body three more times. Nexus explains what it rejected - in the response body, so that body is always surfaced. + A 4xx other than 429 is a deterministic rejection, so it is reported at once + rather than re-sending the whole body three more times. Nexus explains what + it rejected in the response body, so that body is always surfaced. """ opener = build_opener() @@ -67,7 +70,7 @@ def upload(url: str, method: str, headers: dict[str, str], package: Path) -> Non except urllib.error.HTTPError as error: detail = error.read().decode(errors="replace").strip() reason = f"HTTP {error.code}: {detail}" - retryable = error.code >= 500 + retryable = error.code >= 500 or error.code in RETRYABLE_STATUSES except (urllib.error.URLError, OSError) as error: reason = str(error) retryable = True @@ -121,6 +124,8 @@ def main() -> None: token = base64.b64encode(f"{username}:{password}".encode()).decode() auth = {"Authorization": f"Basic {token}"} + # Deliberately not shared with sign_rpm.py: this script ships standalone in + # the packaging image for other repositories to run. packages = sorted( path for path in package_dir.rglob("*") diff --git a/package/rpm/xrpld.spec b/package/rpm/xrpld.spec index 23974c8900..5139cd54e5 100644 --- a/package/rpm/xrpld.spec +++ b/package/rpm/xrpld.spec @@ -17,6 +17,11 @@ URL: https://github.com/XRPLF/rippled ExclusiveArch: x86_64 aarch64 BuildRequires: systemd-rpm-macros +# These have to precede %%debug_package: it opens the debuginfo subpackage, and +# any tag after it is silently dropped from the main package. +%{?systemd_requires} +%{?sysusers_requires_compat} + %undefine _debugsource_packages %debug_package # Level 3 rather than the el9 default of 19: it shrinks the multi-gigabyte @@ -25,10 +30,13 @@ BuildRequires: systemd-rpm-macros %global _binary_payload w3.zstdio %global _find_debuginfo_dwz_opts %{nil} -%build_mtime_policy clamp_to_source_date_epoch +# Reproducibility: the first two take their value from the SOURCE_DATE_EPOCH +# build_pkg.py exports. Without these the header records the wall clock and the +# build container's hostname, so two builds of the same commit differ. +%global clamp_mtime_to_source_date_epoch 1 +%global use_source_date_epoch_as_buildtime 1 +%global _buildhost xrplf.org -%{?systemd_requires} -%{?sysusers_requires_compat} %description xrpld is the reference implementation of the XRP Ledger protocol. It @@ -53,7 +61,7 @@ install -Dm0644 %{_sourcedir}/validators.txt %{buildroot}%{_sysconfdir}/%{ install -Dm0644 %{_sourcedir}/xrpld.service %{buildroot}%{_unitdir}/xrpld.service install -Dm0644 %{_sourcedir}/xrpld.sysusers %{buildroot}%{_sysusersdir}/xrpld.conf install -Dm0644 %{_sourcedir}/xrpld.tmpfiles %{buildroot}%{_tmpfilesdir}/xrpld.conf -install -Dm0644 /dev/null %{buildroot}%{_presetdir}/50-xrpld.preset +install -d %{buildroot}%{_presetdir} cat >%{buildroot}%{_presetdir}/50-xrpld.preset <<'EOF' enable xrpld.service EOF @@ -76,7 +84,7 @@ ln -s %{_bindir}/%{name} %{buildroot}/usr/local/bin/rippled %sysusers_create_package %{name} %{_sourcedir}/xrpld.sysusers %post -systemd-tmpfiles --create %{_tmpfilesdir}/xrpld.conf || : +%tmpfiles_create_package %{name} %{_sourcedir}/xrpld.tmpfiles %systemd_post xrpld.service %preun @@ -86,11 +94,12 @@ systemd-tmpfiles --create %{_tmpfilesdir}/xrpld.conf || : %systemd_postun xrpld.service %files +%attr(0755,root,root) %dir %{_docdir}/%{name} %license %{_docdir}/%{name}/LICENSE.md %license %{_docdir}/%{name}/validator-keys-LICENSE %doc %{_docdir}/%{name}/README.md -%dir %{_sysconfdir}/%{name} +%attr(0755,root,root) %dir %{_sysconfdir}/%{name} %{_bindir}/%{name} %{_bindir}/validator-keys @@ -101,7 +110,7 @@ systemd-tmpfiles --create %{_tmpfilesdir}/xrpld.conf || : %{_unitdir}/xrpld.service -%{_presetdir}/50-xrpld.preset +%attr(0644,root,root) %{_presetdir}/50-xrpld.preset %{_sysusersdir}/xrpld.conf %{_tmpfilesdir}/xrpld.conf %ghost %dir /var/lib/xrpld diff --git a/package/shared/xrpld.service b/package/shared/xrpld.service index 22e6359ef0..27dd6a5a3a 100644 --- a/package/shared/xrpld.service +++ b/package/shared/xrpld.service @@ -17,6 +17,8 @@ ProtectHome=true PrivateTmp=true User=xrpld Group=xrpld +# xrpld.tmpfiles creates these at install and boot; these recreate them on +# every start, so a removed directory does not stop the service. StateDirectory=xrpld StateDirectoryMode=0750 LogsDirectory=xrpld diff --git a/package/sign_rpm.py b/package/sign_rpm.py index 05c719b710..07bda9f392 100755 --- a/package/sign_rpm.py +++ b/package/sign_rpm.py @@ -107,6 +107,8 @@ def main() -> None: args = parser.parse_args() package_dir: Path = args.package_dir + # Deliberately not shared with publish_pkg.py, which ships standalone in the + # packaging image. rpms = sorted(path for path in package_dir.rglob("*.rpm") if path.is_file()) # Signing nothing would otherwise look like a successful signing. assert rpms, f"no RPMs found in {package_dir}" From 6e1eb88e6eab916b2659aa71c951cfc255dafa21 Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 3 Sep 2026 11:52:08 +0000 Subject: [PATCH 07/62] ci: Update prepare-runner SHA (#8168) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> --- .github/workflows/check-tools.yml | 2 +- .github/workflows/publish-docs.yml | 2 +- .github/workflows/reusable-build-test-config.yml | 2 +- .github/workflows/reusable-clang-tidy.yml | 2 +- .github/workflows/reusable-package.yml | 4 ++-- .github/workflows/reusable-upload-recipe.yml | 2 +- .github/workflows/upload-conan-deps.yml | 2 +- 7 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/workflows/check-tools.yml b/.github/workflows/check-tools.yml index 1169140481..148ee9a781 100644 --- a/.github/workflows/check-tools.yml +++ b/.github/workflows/check-tools.yml @@ -79,7 +79,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false diff --git a/.github/workflows/publish-docs.yml b/.github/workflows/publish-docs.yml index 8c5d10929c..072d861456 100644 --- a/.github/workflows/publish-docs.yml +++ b/.github/workflows/publish-docs.yml @@ -47,7 +47,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 2846c3fb85..13e20b2211 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -129,7 +129,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: ${{ inputs.ccache_enabled }} diff --git a/.github/workflows/reusable-clang-tidy.yml b/.github/workflows/reusable-clang-tidy.yml index 045d384181..5f45fcf732 100644 --- a/.github/workflows/reusable-clang-tidy.yml +++ b/.github/workflows/reusable-clang-tidy.yml @@ -43,7 +43,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false diff --git a/.github/workflows/reusable-package.yml b/.github/workflows/reusable-package.yml index 3986feed7f..c3df348faa 100644 --- a/.github/workflows/reusable-package.yml +++ b/.github/workflows/reusable-package.yml @@ -81,7 +81,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false @@ -261,7 +261,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false diff --git a/.github/workflows/reusable-upload-recipe.yml b/.github/workflows/reusable-upload-recipe.yml index 6fa289665a..beb1104ab0 100644 --- a/.github/workflows/reusable-upload-recipe.yml +++ b/.github/workflows/reusable-upload-recipe.yml @@ -50,7 +50,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index 184f13cc5e..99e25c8914 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -68,7 +68,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Prepare runner - uses: XRPLF/actions/prepare-runner@7bf7ceca5932114abdd0d43493c3c30c5a654e13 + uses: XRPLF/actions/prepare-runner@c83c0e6a4d270cb022277b48cfdaa68c906e9ded with: enable_ccache: false From 8ce4f71427af7acc25fe8964b9a5a337163ebf40 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 3 Sep 2026 14:48:32 +0000 Subject: [PATCH 08/62] docs: Use consistent heading levels in PR template (#8155) --- .github/pull_request_template.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 95d75c04b4..e0d511c467 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -18,7 +18,7 @@ If too broad, please consider splitting into multiple PRs. If there is a relevant task or issue, please link it here. --> -### Context of Change +## Context of Change -### API Impact +## API Impact