From 795dc5e3643b4d66314f1240dc6728cfa71f11c3 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Thu, 21 May 2026 16:46:26 +0200 Subject: [PATCH 1/4] fix: Avoid principal-zeroing in non-final loan payments at coarse scale (#7050) Co-authored-by: Ed Hennis --- src/libxrpl/ledger/helpers/LendingHelpers.cpp | 17 +- src/test/app/Loan_test.cpp | 388 ++++++++++++------ 2 files changed, 278 insertions(+), 127 deletions(-) diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 26e320c15d..2c756c2877 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -1058,11 +1058,22 @@ computePaymentComponents( rules, periodicPayment, periodicRate, paymentRemaining - 1, managementFeeRate); // Round the target to the loan's scale to match how actual loan values - // are stored. + // are stored. With fixCleanup3_2_0 enabled, principal is rounded upward + // and interest downward so that at coarse scale principal sticks at the + // floor (until the final payment clears it) while interest absorbs each + // periodic payment. Without the amendment the pre-existing round-to- + // nearest behavior is preserved (which can hit the "Partial principal + // payment" assertion on degenerate integer-scale loans). + bool const fixCleanup320Enabled = rules.enabled(fixCleanup3_2_0); + Number::RoundingMode const principalRounding = + fixCleanup320Enabled ? Number::RoundingMode::Upward : Number::getround(); + Number::RoundingMode const interestRounding = + fixCleanup320Enabled ? Number::RoundingMode::Downward : Number::getround(); LoanState const roundedTarget = LoanState{ .valueOutstanding = roundToAsset(asset, trueTarget.valueOutstanding, scale), - .principalOutstanding = roundToAsset(asset, trueTarget.principalOutstanding, scale), - .interestDue = roundToAsset(asset, trueTarget.interestDue, scale), + .principalOutstanding = + roundToAsset(asset, trueTarget.principalOutstanding, scale, principalRounding), + .interestDue = roundToAsset(asset, trueTarget.interestDue, scale, interestRounding), .managementFeeDue = roundToAsset(asset, trueTarget.managementFeeDue, scale)}; // Get the current actual loan state from the ledger values diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 201bb6942a..b5687265ab 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -69,6 +69,7 @@ #include #include #include +#include #include #include #include @@ -90,8 +91,25 @@ class Loan_test : public beast::unit_test::Suite protected: // Ensure that all the features needed for Lending Protocol are included, // even if they are set to unsupported. + FeatureBitset const all_{jtx::testableAmendments()}; + // All 2^N permutations of `all_` with each subset of the given features + // excluded. The first entry is always `all_` itself (empty exclusion); + // the last excludes every feature in the list. + std::vector + amendmentCombinations(std::initializer_list features) const + { + std::vector result{all_}; + for (auto const& f : features) + { + auto const n = result.size(); + for (std::size_t i = 0; i < n; ++i) + result.push_back(result[i] - f); + } + return result; + } + std::string const iouCurrency_{"IOU"}; void @@ -1201,7 +1219,8 @@ protected: runLoan( AssetType assetType, BrokerParameters const& brokerParams, - LoanParameters const& loanParams) + LoanParameters const& loanParams, + FeatureBitset features) { using namespace jtx; @@ -1209,7 +1228,7 @@ protected: Account const lender("lender"); Account const borrower("borrower"); - Env env(*this, all_); + Env env(*this, features); auto loanResult = createLoan(env, assetType, brokerParams, loanParams, issuer, lender, borrower); @@ -2896,7 +2915,7 @@ protected: } void - testLoanSet() + testLoanSet(FeatureBitset features) { using namespace jtx; @@ -2915,7 +2934,7 @@ protected: std::function mptTest, std::function iouTest, CaseArgs args = {}) { - Env env(*this, all_); + Env env(*this, features); env.fund(XRP(args.initialXRP), issuer, lender, borrower); env.close(); if (args.requireAuth) @@ -3453,14 +3472,14 @@ protected: } void - testLifecycle() + testLifecycle(FeatureBitset features) { testcase("Lifecycle"); using namespace jtx; // Create 3 loan brokers: one for XRP, one for an IOU, and one for // an MPT. That'll require three corresponding SAVs. - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; // For simplicity, lender will be the sole actor for the vault & @@ -3547,7 +3566,7 @@ protected: } void - testSelfLoan() + testSelfLoan(FeatureBitset features) { testcase << "Self Loan"; @@ -3555,7 +3574,7 @@ protected: using namespace std::chrono_literals; // Create 3 loan brokers: one for XRP, one for an IOU, and one for // an MPT. That'll require three corresponding SAVs. - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; // For simplicity, lender will be the sole actor for the vault & @@ -3682,7 +3701,7 @@ protected: } void - testBatchBypassCounterparty() + testBatchBypassCounterparty(FeatureBitset features) { // From FIND-001 testcase << "Batch Bypass Counterparty"; @@ -3693,7 +3712,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; - Env env(*this, all_); + Env env(*this, features); Account const lender{"lender"}; Account const borrower{"borrower"}; @@ -3749,14 +3768,14 @@ protected: } void - testWrongMaxDebtBehavior() + testWrongMaxDebtBehavior(FeatureBitset features) { // From FIND-003 testcase << "Wrong Max Debt Behavior"; using namespace jtx; using namespace std::chrono_literals; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -3795,7 +3814,7 @@ protected: } void - testLoanPayComputePeriodicPaymentValidRateInvariant() + testLoanPayComputePeriodicPaymentValidRateInvariant(FeatureBitset features) { // From FIND-012 testcase << "LoanPay xrpl::detail::computePeriodicPayment : " @@ -3803,7 +3822,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -3863,7 +3882,7 @@ protected: } void - testRPC() + testRPC(FeatureBitset features) { // This will expand as more test cases are added. Some functionality // is tested in other test functions. @@ -3871,7 +3890,7 @@ protected: using namespace jtx; - Env env(*this, all_); + Env env(*this, features); auto lowerFee = [&]() { // Run the local fee back down. @@ -4542,7 +4561,7 @@ protected: } void - testAccountSendMptMinAmountInvariant() + testAccountSendMptMinAmountInvariant(FeatureBitset features) { // (From FIND-006) testcase << "LoanSet trigger xrpl::accountSendMPT : minimum amount " @@ -4550,7 +4569,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -4602,7 +4621,7 @@ protected: } void - testLoanPayDebtDecreaseInvariant() + testLoanPayDebtDecreaseInvariant(FeatureBitset features) { // From FIND-007 testcase << "LoanPay xrpl::LoanPay::doApply : debtDecrease " @@ -4611,7 +4630,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; using namespace Lending; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -4695,14 +4714,14 @@ protected: } void - testLoanPayComputePeriodicPaymentValidTotalInterestInvariant() + testLoanPayComputePeriodicPaymentValidTotalInterestInvariant(FeatureBitset features) { // From FIND-010 testcase << "xrpl::loanComputePaymentParts : valid total interest"; using namespace jtx; using namespace std::chrono_literals; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -4902,7 +4921,7 @@ protected: } void - testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant() + testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant(FeatureBitset features) { // From FIND-009 testcase << "xrpl::loanComputePaymentParts : totalPrincipalPaid " @@ -4911,7 +4930,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; using namespace Lending; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -5004,7 +5023,7 @@ protected: } void - testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant() + testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant(FeatureBitset features) { // From FIND-008 testcase << "xrpl::loanComputePaymentParts : loanValueChange rounded"; @@ -5012,7 +5031,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; using namespace Lending; - Env env(*this, all_); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -5089,7 +5108,7 @@ protected: } void - testLoanNextPaymentDueDateOverflow() + testLoanNextPaymentDueDateOverflow(FeatureBitset features) { // For FIND-013 testcase << "Prevent nextPaymentDueDate overflow"; @@ -5097,7 +5116,7 @@ protected: using namespace jtx; using namespace std::chrono_literals; using namespace Lending; - Env env(*this, all_); + Env env{*this, features}; Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -5621,14 +5640,14 @@ protected: #if LOAN_TODO void - testLoanPayLateFullPaymentBypassesPenalties() + testLoanPayLateFullPaymentBypassesPenalties(FeatureBitset features) { testcase("LoanPay full payment skips late penalties"); using namespace jtx; using namespace loan; using namespace std::chrono_literals; - Env env(*this, all); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -5774,7 +5793,7 @@ protected: } void - testLoanCoverMinimumRoundingExploit() + testLoanCoverMinimumRoundingExploit(FeatureBitset features) { auto testLoanCoverMinimumRoundingExploit = [&, this](Number const& principalRequest) { testcase << "LoanBrokerCoverClawback drains cover via rounding" @@ -5784,7 +5803,7 @@ protected: using namespace loan; using namespace loanBroker; - Env env(*this, all); + Env env(*this, features); Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -5860,7 +5879,7 @@ protected: #endif void - testPoCUnsignedUnderflowOnFullPayAfterEarlyPeriodic() + testPoCUnsignedUnderflowOnFullPayAfterEarlyPeriodic(FeatureBitset features) { // --- PoC Summary ---------------------------------------------------- // Scenario: Borrower makes one periodic payment early (before next due) @@ -5882,7 +5901,7 @@ protected: using namespace loan; using namespace std::chrono_literals; - Env env(*this, all_); + Env env{*this, features}; Account const lender{"poc_lender4"}; Account const borrower{"poc_borrower4"}; @@ -6085,13 +6104,13 @@ protected: } void - testDustManipulation() + testDustManipulation(FeatureBitset features) { testcase("Dust manipulation"); using namespace jtx; using namespace std::chrono_literals; - Env env(*this, all_); + Env env{*this, features}; // Setup: Create accounts Account const issuer{"issuer"}; @@ -6225,7 +6244,7 @@ protected: } void - testRIPD3831() + testRIPD3831(FeatureBitset features) { using namespace jtx; @@ -6252,7 +6271,7 @@ protected: auto const assetType = AssetType::XRP; - Env env(*this, all_); + Env env{*this, features}; auto loanResult = createLoan(env, assetType, brokerParams, loanParams, issuer, lender, borrower); @@ -6298,7 +6317,7 @@ protected: } void - testRIPD3459() + testRIPD3459(FeatureBitset features) { testcase("RIPD-3459 - LoanBroker incorrect debt total"); @@ -6323,7 +6342,7 @@ protected: auto const assetType = AssetType::MPT; - Env env(*this, all_); + Env env{*this, features}; auto loanResult = createLoan(env, assetType, brokerParams, loanParams, issuer, lender, borrower); @@ -6423,14 +6442,14 @@ protected: } void - testRoundingAllowsUndercoverage() + testRoundingAllowsUndercoverage(FeatureBitset features) { testcase("Minimum cover rounding allows undercoverage (XRP)"); using namespace jtx; using namespace loanBroker; - Env env(*this, all_); + Env env{*this, features}; Account const lender{"lender"}; Account const borrower{"borrower"}; @@ -6502,7 +6521,7 @@ protected: } void - testRIPD3902() + testRIPD3902(FeatureBitset features) { testcase("RIPD-3902 - 1 IOU loan payments"); @@ -6529,7 +6548,7 @@ protected: auto const assetType = AssetType::IOU; - Env env(*this, all_); + Env env{*this, features}; auto loanResult = createLoan(env, assetType, brokerParams, loanParams, issuer, lender, borrower); @@ -6673,7 +6692,7 @@ protected: } void - testIssuerIsBorrower() + testIssuerIsBorrower(FeatureBitset features) { testcase("RIPD-4096 - Issuer as borrower"); @@ -6693,7 +6712,7 @@ protected: auto const assetType = AssetType::IOU; - Env env(*this, all_); + Env env{*this, features}; auto loanResult = createLoan(env, assetType, brokerParams, loanParams, issuer, lender, issuer); @@ -6790,14 +6809,14 @@ protected: } void - testOverpaymentManagementFee() + testOverpaymentManagementFee(FeatureBitset features) { testcase("testOverpaymentManagementFee"); using namespace jtx; using namespace loan; - Env env(*this, all_); + Env env{*this, features}; Account const lender{"lender"}, borrower{"borrower"}; @@ -6843,7 +6862,7 @@ protected: } void - testLoanPayBrokerOwnerMissingTrustline() + testLoanPayBrokerOwnerMissingTrustline(FeatureBitset features) { testcase << "LoanPay Broker Owner Missing Trustline (PoC)"; using namespace jtx; @@ -6852,7 +6871,7 @@ protected: Account const borrower("borrower"); Account const broker("broker"); auto const iou = issuer["IOU"]; - Env env(*this, all_); + Env env(*this, features); env.fund(XRP(20'000), issuer, broker, borrower); env.close(); // Set up trustlines and fund accounts @@ -6911,7 +6930,7 @@ protected: } void - testLoanPayBrokerOwnerUnauthorizedMPT() + testLoanPayBrokerOwnerUnauthorizedMPT(FeatureBitset features) { testcase << "LoanPay Broker Owner MPT unauthorized"; using namespace jtx; @@ -6921,7 +6940,7 @@ protected: Account const borrower("borrower"); Account const broker("broker"); - Env env(*this, all_); + Env env{*this, features}; env.fund(XRP(20'000), issuer, broker, borrower); env.close(); @@ -6992,7 +7011,7 @@ protected: } void - testLoanPayBrokerOwnerNoPermissionedDomainMPT() + testLoanPayBrokerOwnerNoPermissionedDomainMPT(FeatureBitset features) { testcase << "LoanPay Broker Owner without permissioned domain of the MPT"; using namespace jtx; @@ -7002,7 +7021,7 @@ protected: Account const borrower("borrower"); Account const broker("broker"); - Env env(*this, all_); + Env env{*this, features}; env.fund(XRP(20'000), issuer, broker, borrower); env.close(); @@ -7095,7 +7114,7 @@ protected: } void - testLoanSetBrokerOwnerNoPermissionedDomainMPT() + testLoanSetBrokerOwnerNoPermissionedDomainMPT(FeatureBitset features) { testcase << "LoanSet Broker Owner without permissioned domain of the MPT"; using namespace jtx; @@ -7105,7 +7124,7 @@ protected: Account const borrower("borrower"); Account const broker("broker"); - Env env(*this, all_); + Env env{*this, features}; env.fund(XRP(20'000), issuer, broker, borrower); env.close(); @@ -7167,7 +7186,7 @@ protected: } void - testSequentialFLCDepletion() + testSequentialFLCDepletion(FeatureBitset features) { testcase << "First-Loss Capital Depletion on Sequential Defaults"; @@ -7175,7 +7194,7 @@ protected: using namespace loan; using namespace loanBroker; - Env env(*this, all_); + Env env{*this, features}; Account const issuer{"issuer"}; Account const lender{"lender"}; @@ -7427,7 +7446,7 @@ protected: // loss from an impaired loan, ensuring the invariant check properly // accounts for the loss. void - testWithdrawReflectsUnrealizedLoss() + testWithdrawReflectsUnrealizedLoss(FeatureBitset features) { using namespace jtx; using namespace loan; @@ -7446,7 +7465,7 @@ protected: static constexpr std::uint32_t kLocalPaymentInterval = 600; static constexpr std::uint32_t kLocalPaymentTotal = 2; - Env env(*this, all_); + Env env{*this, features}; // Setup accounts Account const issuer{"issuer"}; @@ -7553,6 +7572,110 @@ protected: attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS); } + // Regression for the dual-rounding fix at coarse (integer-MPT) scale. + // + // Loan: P=1, r=50% (50000 tenth-bips), n=3, yearly interval. The + // amortization schedule produces a fractional principal + // (~0.47) which under round-to-nearest collapses to 0 in a single + // step, causing `doPayment`'s strict `>` assertion on principal to + // fire mid-loan. With fixCleanup3_2_0 enabled, principal is rounded + // upward (sticks at 1 across the first two periods) and only clears + // in the final payment. + // + // The test pays one period at a time across three LoanPay + // transactions and verifies the loan completes (paymentRemaining=0) + // with totals matching the loan's economics (1 principal + 2 interest). + void + testIntegerScalePrincipalSticks(FeatureBitset features) + { + // Without fixCleanup3_2_0, this behavior will abort the server, so + // don't run without it. + if (!features[fixCleanup3_2_0]) + return; + + testcase("edge: integer MPT principal stuck mid-loan completes via final"); + + using namespace jtx; + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(100'000), issuer, lender, borrower); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.maxAmt = 100'000, .flags = tfMPTCanTransfer}); + PrettyAsset const asset{mptt.issuanceID()}; + + mptt.authorize({.account = lender}); + mptt.authorize({.account = borrower}); + + env(pay(issuer, lender, asset(10'000))); + env(pay(issuer, borrower, asset(10'000))); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = lender, .asset = asset}); + env(vaultTx); + env.close(); + + env(vault.deposit({.depositor = lender, .id = vaultKeylet.key, .amount = asset(5'000)})); + env.close(); + + auto const brokerKeylet = keylet::loanbroker(lender.id(), env.seq(lender)); + env(loanBroker::set(lender, vaultKeylet.key), + loanBroker::kDebtMaximum(Number{100}), + Fee(env.current()->fees().base * 2)); + env.close(); + + auto const brokerStateBefore = env.le(brokerKeylet); + if (!BEAST_EXPECT(brokerStateBefore)) + return; + auto const loanSequence = brokerStateBefore->at(sfLoanSequence); + auto const loanKeylet = keylet::loan(brokerKeylet.key, loanSequence); + + env(loan::set(borrower, brokerKeylet.key, Number{1}), + Sig(sfCounterpartySignature, lender), + loan::kInterestRate(TenthBips32{50'000}), + loan::kPaymentTotal(3), + loan::kPaymentInterval(31'536'000), + Fee(env.current()->fees().base * 2)); + env.close(); + + auto const borrowerStart = env.balance(borrower, asset).value(); + + // Three separate periodic payments of 1 each. Expected per-period + // evolution at integer MPT scale (TVO = PO + interestDue + + // managementFeeDue): + // start: PO=1, TVO=3, paymentRemaining=3 + // after pay #1: PO=1, TVO=2, paymentRemaining=2 (principal sticks) + // after pay #2: PO=1, TVO=1, paymentRemaining=1 (principal sticks) + // after pay #3: PO=0, TVO=0, paymentRemaining=0 (final clears) + std::array const expectedPO{Number{1}, Number{1}, Number{0}}; + std::array const expectedTVO{Number{2}, Number{1}, Number{0}}; + std::array const expectedRemaining{2, 1, 0}; + + for (int i = 0; i < 3; ++i) + { + env(loan::pay(borrower, loanKeylet.key, asset(1)), Ter(tesSUCCESS)); + env.close(); + + auto const sle = env.le(loanKeylet); + if (!BEAST_EXPECT(sle)) + return; + BEAST_EXPECT(sle->at(sfPrincipalOutstanding) == expectedPO[i]); + BEAST_EXPECT(sle->at(sfTotalValueOutstanding) == expectedTVO[i]); + BEAST_EXPECT(sle->at(sfPaymentRemaining) == expectedRemaining[i]); + } + + // Borrower paid 3 total regardless of fee split (1 principal + 2 + // interest+fee, matching loan economics). + auto const borrowerEnd = env.balance(borrower, asset).value(); + BEAST_EXPECT(borrowerStart - borrowerEnd == asset(3).value()); + } + // A near-zero interest rate on a 100 USD loan // produces total interest of ~6 units at loanScale -9. Numerical error // in the amortization formula pushes the theoretical principal above @@ -7735,74 +7858,91 @@ protected: to_string(tolerance)); } + void + runAmendmentIndependent() + { + testDisabled(); + testInvalidLoanSet(); + testInvalidLoanDelete(); + testInvalidLoanManage(); + testInvalidLoanPay(); + testIssuerLoan(); + testServiceFeeOnBrokerDeepFreeze(); + testRequireAuth(); + testRIPD3901(); + testBorrowerIsBroker(); + testLimitExceeded(); + + for (auto const flags : {0u, tfLoanOverpayment}) + testYieldTheftRounding(flags); + testBugInterestDueDeltaCrash(); + testFullLifecycleVaultPnLNearZeroRate(); + } + + // Tests run under each entry in amendmentCombinations(). + void + runAmendmentSensitive(FeatureBitset features) + { +#if LOAN_TODO + testLoanPayLateFullPaymentBypassesPenalties(features); + testLoanCoverMinimumRoundingExploit(features); +#endif + + // Lifecycle + testSelfLoan(features); + testLoanSet(features); + testLifecycle(features); + + // Payment paths + testWithdrawReflectsUnrealizedLoss(features); + testPoCUnsignedUnderflowOnFullPayAfterEarlyPeriodic(features); + testBatchBypassCounterparty(features); + testLoanNextPaymentDueDateOverflow(features); + testCoverDepositWithdrawNonTransferableMPT(features); + testSequentialFLCDepletion(features); + + // Invariants + testLoanPayComputePeriodicPaymentValidRateInvariant(features); + testAccountSendMptMinAmountInvariant(features); + testLoanPayDebtDecreaseInvariant(features); + testWrongMaxDebtBehavior(features); + testLoanPayComputePeriodicPaymentValidTotalInterestInvariant(features); + testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant(features); + testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant(features); + + // RPC + testRPC(features); + + // Edge / rounding + testDustManipulation(features); + testRoundingAllowsUndercoverage(features); + testOverpaymentManagementFee(features); + testIssuerIsBorrower(features); + testIntegerScalePrincipalSticks(features); + + // RIPD regressions + testRIPD3831(features); + testRIPD3459(features); + testRIPD3902(features); + + // Broker-owner permissions + testLoanPayBrokerOwnerMissingTrustline(features); + testLoanPayBrokerOwnerUnauthorizedMPT(features); + testLoanPayBrokerOwnerNoPermissionedDomainMPT(features); + testLoanSetBrokerOwnerNoPermissionedDomainMPT(features); + } + public: void run() override { -#if LOAN_TODO - testLoanPayLateFullPaymentBypassesPenalties(); - testLoanCoverMinimumRoundingExploit(); -#endif - for (auto const flags : {0u, tfLoanOverpayment}) - { - testYieldTheftRounding(flags); - } - - testBugInterestDueDeltaCrash(); - testFullLifecycleVaultPnLNearZeroRate(); - - testWithdrawReflectsUnrealizedLoss(); - testInvalidLoanSet(); - - auto const all = jtx::testableAmendments(); - testCoverDepositWithdrawNonTransferableMPT(all); - testCoverDepositWithdrawNonTransferableMPT(all - featureMPTokensV2); - testCoverDepositWithdrawNonTransferableMPT(all - fixCleanup3_2_0); + runAmendmentIndependent(); testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared(); testLendingCanTradeClearedNoImpact(); - testPoCUnsignedUnderflowOnFullPayAfterEarlyPeriodic(); - - testDisabled(); - testSelfLoan(); - testIssuerLoan(); - testLoanSet(); - testLifecycle(); - testServiceFeeOnBrokerDeepFreeze(); - - testRPC(); - testInvalidLoanDelete(); - testInvalidLoanManage(); - testInvalidLoanPay(); - - testBatchBypassCounterparty(); - testLoanPayComputePeriodicPaymentValidRateInvariant(); - testAccountSendMptMinAmountInvariant(); - testLoanPayDebtDecreaseInvariant(); - testWrongMaxDebtBehavior(); - testLoanPayComputePeriodicPaymentValidTotalInterestInvariant(); - testDosLoanPay(all | fixCleanup3_1_3); - testDosLoanPay(all - fixCleanup3_1_3); - testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant(); - testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant(); - testLoanNextPaymentDueDateOverflow(); - - testRequireAuth(); - testDustManipulation(); - - testRIPD3831(); - testRIPD3459(); - testRIPD3901(); - testRIPD3902(); - testRoundingAllowsUndercoverage(); - testBorrowerIsBroker(); - testIssuerIsBorrower(); - testLimitExceeded(); - testOverpaymentManagementFee(); - testLoanPayBrokerOwnerMissingTrustline(); - testLoanPayBrokerOwnerUnauthorizedMPT(); - testLoanPayBrokerOwnerNoPermissionedDomainMPT(); - testLoanSetBrokerOwnerNoPermissionedDomainMPT(); - testSequentialFLCDepletion(); + testDosLoanPay(all_ | fixCleanup3_1_3); + testDosLoanPay(all_ - fixCleanup3_1_3); + for (auto const& features : amendmentCombinations({fixCleanup3_2_0, featureMPTokensV2})) + runAmendmentSensitive(features); } }; @@ -7867,7 +8007,7 @@ protected: .payInterval = payInterval, }; - runLoan(assetType, brokerParams, loanParams); + runLoan(assetType, brokerParams, loanParams, all_); } public: @@ -7926,7 +8066,7 @@ class LoanArbitrary_test : public LoanBatch_test .payTotal = 2, .payInterval = 200}; - runLoan(AssetType::XRP, brokerParams, loanParams); + runLoan(AssetType::XRP, brokerParams, loanParams, all_); } }; From 7fdaa0a5efdd3a11615790cb5c14af50887969d2 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Thu, 21 May 2026 16:51:58 +0200 Subject: [PATCH 2/4] fix: Fix IOU precision issues in LoanBrokerCover transactions (#7274) --- include/xrpl/ledger/helpers/LendingHelpers.h | 30 +++ include/xrpl/protocol/STAmount.h | 18 ++ src/libxrpl/ledger/helpers/LendingHelpers.cpp | 33 +++ src/libxrpl/protocol/STAmount.cpp | 5 + .../lending/LoanBrokerCoverClawback.cpp | 4 + .../lending/LoanBrokerCoverDeposit.cpp | 54 ++++- .../lending/LoanBrokerCoverWithdraw.cpp | 5 + src/test/app/LendingHelpers_test.cpp | 101 ++++++++- src/test/app/LoanBroker_test.cpp | 212 ++++++++++++++++++ src/test/protocol/STAmount_test.cpp | 95 ++++++++ 10 files changed, 546 insertions(+), 11 deletions(-) diff --git a/include/xrpl/ledger/helpers/LendingHelpers.h b/include/xrpl/ledger/helpers/LendingHelpers.h index a6ab42254b..cce41a38c5 100644 --- a/include/xrpl/ledger/helpers/LendingHelpers.h +++ b/include/xrpl/ledger/helpers/LendingHelpers.h @@ -4,8 +4,38 @@ #include #include +#include + namespace xrpl { +/** + * Broker cover preclaim precision guard (fixCleanup3_2_0). + * + * Prevents a "silent sub-ULP no-op" where a deposit, withdrawal, or clawback + * amount is so small that it rounds to zero at `sfCoverAvailable`'s scale. + * Without this guard, both the pseudo trust-line and `sfCoverAvailable` would + * identically absorb the rounded zero, resulting in a successful transaction + * (tesSUCCESS) where no funds actually moved. + * + * @param view Read view (rules used for amendment gating). + * @param sleBroker The loan broker SLE (read-only). + * @param vaultAsset The underlying vault asset (the broker's cover asset). + * @param amount The effective subtraction/addition amount. + * @param j Journal for logging. + * @param logPrefix Transactor name for log diagnostics. + * + * @return `tecPRECISION_LOSS` if the request rounds to zero at cover scale. + * `tesSUCCESS` if the amendment is disabled or the request is safely supra-ULP. + */ +[[nodiscard]] TER +canApplyToBrokerCover( + ReadView const& view, + SLE::const_ref sleBroker, + Asset const& vaultAsset, + STAmount const& amount, + beast::Journal j, + std::string_view logPrefix); + // Lending protocol has dependencies, so capture them here. bool checkLendingProtocolDependencies(Rules const& rules, STTx const& tx); diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 01511c23ea..c576c0da31 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -184,6 +184,24 @@ public: [[nodiscard]] STAmount const& value() const noexcept; + /** + * Checks if this amount evaluates to zero when constrained to a specific + * accounting scale. + * + * For XRP and MPT `roundToScale` is a no-op, returns true only when the amount itself is zero. + * The `scale` argument is ignored in that case. + * For IOU, the amount is rounded to the given scale using Number::RoundingMode::ToNearest mode + * and the result is checked for zero; if `scale <= exponent()`, `roundToScale` short-circuits + * and returns the value unchanged, so this returns false for any non-zero amount. + * + * @param scale The target accounting scale to evaluate against. + * @return `true` if this amount rounds to zero at the given scale, `false` otherwise. + * + * @see roundToScale + */ + [[nodiscard]] bool + isZeroAtScale(int scale) const; + //-------------------------------------------------------------------------- // // Operators diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 2c756c2877..1fedbb5f13 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -24,10 +25,42 @@ #include #include #include +#include #include namespace xrpl { +[[nodiscard]] TER +canApplyToBrokerCover( + ReadView const& view, + SLE::const_ref sleBroker, + Asset const& vaultAsset, + STAmount const& amount, + beast::Journal j, + std::string_view logPrefix) +{ + XRPL_ASSERT( + sleBroker && sleBroker->getType() == ltLOAN_BROKER, + "xrpl::canApplyToBrokerCover : valid LoanBroker sle"); + XRPL_ASSERT(vaultAsset == amount.asset(), "xrpl::canApplyToBrokerCover : valid asset"); + + if (!view.rules().enabled(fixCleanup3_2_0)) + return tesSUCCESS; + + if (amount == beast::kZero) + return tecPRECISION_LOSS; + + int const coverScale = scale(sleBroker->at(sfCoverAvailable), vaultAsset); + if (amount.isZeroAtScale(coverScale)) + { + JLOG(j.warn()) << logPrefix << ": amount " << amount.getFullText() + << " rounds to zero at cover scale " << coverScale; + return tecPRECISION_LOSS; + } + + return tesSUCCESS; +} + bool checkLendingProtocolDependencies(Rules const& rules, STTx const& tx) { diff --git a/src/libxrpl/protocol/STAmount.cpp b/src/libxrpl/protocol/STAmount.cpp index 2d051722bf..c40beabf12 100644 --- a/src/libxrpl/protocol/STAmount.cpp +++ b/src/libxrpl/protocol/STAmount.cpp @@ -1738,4 +1738,9 @@ divRoundStrict(STAmount const& num, STAmount const& den, Asset const& asset, boo return divRoundImpl(num, den, asset, roundUp); } +[[nodiscard]] bool +STAmount::isZeroAtScale(int scale) const +{ + return roundToScale(*this, scale, Number::RoundingMode::ToNearest).signum() == 0; +} } // namespace xrpl diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp index ab26353a1c..48cb6b90aa 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverClawback.cpp @@ -291,6 +291,10 @@ LoanBrokerCoverClawback::preclaim(PreclaimContext const& ctx) } STAmount const& clawAmount = *findClawAmount; + if (auto const ret = canApplyToBrokerCover( + ctx.view, sleBroker, vaultAsset, clawAmount, ctx.j, "LoanBrokerCoverClawback")) + return ret; + // Explicitly check the balance of the trust line / MPT to make sure the // balance is actually there. It should always match `sfCoverAvailable`, so // if there isn't, this is an internal error. diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp index 04905d5ea3..e84f277f5b 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp @@ -1,9 +1,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -87,6 +89,29 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) if (auto const ret = requireAuth(ctx.view, vaultAsset, account, AuthType::StrongAuth)) return ret; + // Deposit must round the amount Downward to cover scale and then reuse that rounded + // value for the actual transfer in doApply — otherwise implicit round-to-nearest during + // `sfCoverAvailable +=` could credit the broker more than the depositor paid Computing it + // here in preclaim lets us reject sub-cover-scale dust early with tecPRECISION_LOSS instead of + // failing only in doApply. + bool const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0); + auto const roundedAmount = [&]() -> STAmount { + if (!fix320Enabled) + return tx[sfAmount]; + + return roundToScale( + tx[sfAmount], + scale(sleBroker->at(sfCoverAvailable), vaultAsset), + Number::RoundingMode::Downward); + }(); + + if (fix320Enabled && roundedAmount == beast::kZero) + { + JLOG(ctx.j.warn()) << "LoanBrokerCoverDeposit: deposit amount: " << tx[sfAmount] + << " is zero at loan broker scale"; + return tecPRECISION_LOSS; + } + if (accountHolds( ctx.view, account, @@ -94,7 +119,7 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) FreezeHandling::ZeroIfFrozen, AuthHandling::ZeroIfUnauthorized, ctx.j, - SpendableHandling::FullBalance) < amount) + SpendableHandling::FullBalance) < roundedAmount) return tecINSUFFICIENT_FUNDS; return tesSUCCESS; @@ -106,8 +131,6 @@ LoanBrokerCoverDeposit::doApply() auto const& tx = ctx_.tx; auto const brokerID = tx[sfLoanBrokerID]; - auto const amount = tx[sfAmount]; - auto broker = view().peek(keylet::loanbroker(brokerID)); if (!broker) return tecINTERNAL; // LCOV_EXCL_LINE @@ -117,9 +140,32 @@ LoanBrokerCoverDeposit::doApply() return tecINTERNAL; // LCOV_EXCL_LINE auto const vaultAsset = vault->at(sfAsset); - auto const brokerPseudoID = broker->at(sfAccount); + // Re-round here (matches preclaim) so the same cover-scale-quantized + // value drives both the trustline transfer and the cover increment; + // see the rationale comment in preclaim. + bool const fix320Enabled = view().rules().enabled(fixCleanup3_2_0); + auto const amount = [&]() -> STAmount { + if (!fix320Enabled) + return tx[sfAmount]; + + return roundToScale( + tx[sfAmount], + scale(broker->at(sfCoverAvailable), vaultAsset), + Number::RoundingMode::Downward); + }(); + + // We validated zero-amount in preclaim, if we ended up with zero now, fail hard. + if (amount == beast::kZero) + { + // LCOV_EXCL_START + JLOG(j_.error()) << "LoanBrokerCoverDeposit: deposit amount: " << tx[sfAmount] + << " is zero"; + return tecINTERNAL; + // LCOV_EXCL_STOP + } + // Transfer assets from depositor to pseudo-account. if (auto ter = accountSend(view(), accountID_, brokerPseudoID, amount, j_, WaiveTransferFee::Yes)) diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index 331c44b1e8..dbe2de2100 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp @@ -94,6 +94,11 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) if (amount.asset() != vaultAsset) return tecWRONG_ASSET; + // Helper handles both IOU and MPT correctly without explicit branching. + if (auto const ret = canApplyToBrokerCover( + ctx.view, sleBroker, vaultAsset, amount, ctx.j, "LoanBrokerCoverWithdraw")) + return ret; + // The broker's pseudo-account is the source of funds. auto const pseudoAccountID = sleBroker->at(sfAccount); // Post-fixCleanup3_2_0: cover withdraw is a recovery path that bypasses diff --git a/src/test/app/LendingHelpers_test.cpp b/src/test/app/LendingHelpers_test.cpp index 96d0722732..af46dd2e0f 100644 --- a/src/test/app/LendingHelpers_test.cpp +++ b/src/test/app/LendingHelpers_test.cpp @@ -8,9 +8,15 @@ #include #include #include +#include +#include +#include +#include +#include #include #include +#include #include #include @@ -287,9 +293,9 @@ class LendingHelpers_test : public beast::unit_test::Suite std::uint32_t n; }; auto const cases = std::vector{ - {"r=5%, n=3", Number{5, -2}, 3}, - {"r=0.1%, n=1000", Number{1, -3}, 1'000}, - {"r=1e-7, n=100 (above threshold by 10x)", Number{1, -7}, 100}, + {.name = "r=5%, n=3", .r = Number{5, -2}, .n = 3}, + {.name = "r=0.1%, n=1000", .r = Number{1, -3}, .n = 1'000}, + {.name = "r=1e-7, n=100 (above threshold by 10x)", .r = Number{1, -7}, .n = 100}, }; for (auto const& tc : cases) { @@ -318,8 +324,10 @@ class LendingHelpers_test : public beast::unit_test::Suite auto const cases = std::vector{ // bug regime: r = 1 TenthBips32 over 600s payment interval // → r ≈ 1.9e-10, r*n ≈ 3.8e-10 < 1e-9. - {"bug regime: r~1.9e-10, n=2", loanPeriodicRate(TenthBips32{1}, 600), 2}, - {"r=1e-12, n=100", Number{1, -12}, 100}, + {.name = "bug regime: r~1.9e-10, n=2", + .r = loanPeriodicRate(TenthBips32{1}, 600), + .n = 2}, + {.name = "r=1e-12, n=100", .r = Number{1, -12}, .n = 100}, }; for (auto const& tc : cases) { @@ -356,8 +364,8 @@ class LendingHelpers_test : public beast::unit_test::Suite std::uint32_t n; }; auto const cases = std::vector{ - {"r=1e-9, n=1", Number{1, -9}, 1}, - {"r=1e-12, n=1000", Number{1, -12}, 1'000}, + {.name = "r=1e-9, n=1", .r = Number{1, -9}, .n = 1}, + {.name = "r=1e-12, n=1000", .r = Number{1, -12}, .n = 1'000}, }; for (auto const& tc : cases) @@ -1439,6 +1447,84 @@ class LendingHelpers_test : public beast::unit_test::Suite } public: + void + testCanApplyToBrokerCover() + { + using namespace jtx; + + Account const issuer{"issuer"}; + PrettyAsset const iou = issuer["IOU"]; + + // sfCoverAvailable = Number{10} on an IOU → STAmount exponent = -14, + // so coverScale = -14. The ULP boundary is 5e-15; anything below + // that rounds to zero at cover scale. Number{1,-16} = 1e-16 is our + // representative sub-ULP probe. + struct TestCase + { + std::string name; + Number coverAvailable; + STAmount amount; + TER expected; + }; + + auto const testCases = std::vector{ + { + .name = "Zero amount", + .coverAvailable = Number{10}, + .amount = STAmount{iou, Number{0}}, + .expected = tecPRECISION_LOSS, + }, + { + .name = "Rounds to zero at cover scale", + .coverAvailable = Number{10}, + .amount = STAmount{iou, Number{1, -16}}, + .expected = tecPRECISION_LOSS, + }, + { + .name = "Zero coverAvailable, whole-unit amount", + // coverScale = 0 (zero STAmount exponent); 1 IOU is not + // zero at integer scale → tesSUCCESS. + .coverAvailable = Number{0}, + .amount = STAmount{iou, Number{1}}, + .expected = tesSUCCESS, + }, + { + .name = "Supra-ULP amount", + .coverAvailable = Number{10}, + .amount = STAmount{iou, Number{1, -13}}, + .expected = tesSUCCESS, + }, + }; + + Env const env{*this}; + + for (auto const& tc : testCases) + { + testcase("canApplyToBrokerCover: " + tc.name); + auto sle = std::make_shared(ltLOAN_BROKER, uint256{1u}); + sle->at(sfCoverAvailable) = tc.coverAvailable; + BEAST_EXPECT( + canApplyToBrokerCover(*env.current(), sle, iou, tc.amount, env.journal, "test") == + tc.expected); + } + + // Amendment off → guard is bypassed regardless of amount. + { + testcase("canApplyToBrokerCover: amendment disabled"); + Env const envOff{*this, testableAmendments() - fixCleanup3_2_0}; + auto sle = std::make_shared(ltLOAN_BROKER, uint256{1u}); + sle->at(sfCoverAvailable) = Number{10}; + BEAST_EXPECT( + canApplyToBrokerCover( + *envOff.current(), + sle, + iou, + STAmount{iou, Number{0}}, + envOff.journal, + "test") == tesSUCCESS); + } + } + void run() override { @@ -1462,6 +1548,7 @@ public: testComputePaymentFactorNearZeroRate(); testComputeOverpaymentComponents(); testComputeInterestAndFeeParts(); + testCanApplyToBrokerCover(); } }; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index a7e036a621..c899778391 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -55,6 +55,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -1823,10 +1824,221 @@ class LoanBroker_test : public beast::unit_test::Suite testRIPD4274MPT(); } + // Exercises canApplyToBrokerCover (fixCleanup3_2_0): a deposit, withdraw, + // or clawback whose amount rounds to zero at sfCoverAvailable's precision + // scale must be rejected with tecPRECISION_LOSS once the amendment is on, + // and must silently succeed without changing sfCoverAvailable when off. + void + testCoverPrecisionGuard() + { + using namespace jtx; + using namespace loanBroker; + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + // sfCoverAvailable = 10 IOU → STAmount exponent = -14. + // Anything < 5e-15 rounds to zero at that scale. + // 1e-16 is the representative sub-ULP probe amount. + + // Shared setup: funds accounts, creates a vault + broker with 10 IOU + // cover, and returns {brokerKeylet, iou}. + auto const setup = [&](Env& env) -> std::pair { + Vault const vault{env}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const iou = issuer["IOU"]; + env(trust(alice, iou(1'000'000))); + env.close(); + env(pay(issuer, alice, iou(1'000))); + env.close(); + + auto [createTx, vaultKeylet] = vault.create({.owner = alice, .asset = iou}); + env(createTx); + env.close(); + + auto const brokerKeylet = keylet::loanbroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + env(coverDeposit(alice, brokerKeylet.key, iou(10))); + env.close(); + + return {brokerKeylet, iou}; + }; + + auto runTestCases = [&](FeatureBitset features) { + TER const expected = + features[fixCleanup3_2_0] ? TER{tecPRECISION_LOSS} : TER{tesSUCCESS}; + + { + testcase("Cover precision guard: Deposit zero-at-scale"); + Env env{*this, features}; + auto const [brokerKeylet, iou] = setup(env); + PrettyAmount const subUlpAmt = iou(Number{1, -16}); + auto const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + env(coverDeposit(alice, brokerKeylet.key, subUlpAmt), Ter(expected)); + env.close(); + if (expected == tesSUCCESS) + { + if (auto const broker = env.le(brokerKeylet); BEAST_EXPECT(broker)) + BEAST_EXPECT(broker->at(sfCoverAvailable) == coverBefore); + } + } + + { + testcase("Cover precision guard: Deposit rounds down"); + // Both cases succeed; post-fix the amount is rounded DOWN to + // cover scale first, so the delta differs from pre-fix + // Input: 1.8e-14 IOU (sub-scale at cover scale -14) + // Pre-fix: 10 + 1.8e-14 → round-to-nearest → + // 10.00000000000002 → delta 2e-14 + // Post-fix: roundToScale(1.8e-14, -14, Downward) = 1e-14; + // 10 + 1e-14 = 10.00000000000001 → delta 1e-14 + Env env{*this, features}; + auto const [brokerKeylet, iou] = setup(env); + PrettyAmount const subUlpAmt = iou(Number{18, -15}); + auto const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + env(coverDeposit(alice, brokerKeylet.key, subUlpAmt), Ter(tesSUCCESS)); + env.close(); + auto const brokerAfter = env.le(brokerKeylet); + if (!BEAST_EXPECT(brokerAfter)) + return; + + Number const delta = features[fixCleanup3_2_0] ? Number{1, -14} : Number{2, -14}; + BEAST_EXPECT(brokerAfter->at(sfCoverAvailable) - coverBefore == delta); + } + + // Property: post-fix, when the user deposits `x` and cover + // gains `x'`, we always have 0 <= x - x' < 1 ULP at cover + // scale (cover holds 10 IOU → ULP = 1e-14). Pre-fix uses + // STAmount's default round-to-nearest during `+=`, which can + // over-deposit (x' > x), so the property only holds with + // fixCleanup3_2_0 enabled. + if (features[fixCleanup3_2_0]) + { + testcase("Cover precision guard: Deposit rounding bound"); + Env env{*this, features}; + auto const [brokerKeylet, iou] = setup(env); + Number const oneUlp{1, -14}; + // Each requested amount lies strictly between 1·ULP and + // 2·ULP at cover scale; post-fix `roundDown` credits + // exactly `oneUlp` and leaves a strictly-positive, + // strictly-sub-ULP residual. + for (Number const requested : {Number{11, -15}, Number{15, -15}, Number{19, -15}}) + { + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + Number const coverBefore = broker->at(sfCoverAvailable); + env(coverDeposit(alice, brokerKeylet.key, iou(requested)), Ter(tesSUCCESS)); + env.close(); + auto const brokerAfter = env.le(brokerKeylet); + if (!BEAST_EXPECT(brokerAfter)) + return; + Number const coverAfter = brokerAfter->at(sfCoverAvailable); + Number const actual = coverAfter - coverBefore; + Number const lost = requested - actual; + BEAST_EXPECT(lost >= Number{0}); + BEAST_EXPECT(lost < oneUlp); + } + } + + { + testcase("Cover precision guard: Withdraw"); + Env env{*this, features}; + auto const [brokerKeylet, iou] = setup(env); + PrettyAmount const subUlpAmt = iou(Number{1, -16}); + auto const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + auto const aliceBalanceBefore = env.balance(alice, iou); + env(coverWithdraw(alice, brokerKeylet.key, subUlpAmt), Ter(expected)); + env.close(); + if (expected == tesSUCCESS) + { + if (auto const broker = env.le(brokerKeylet); BEAST_EXPECT(broker)) + BEAST_EXPECT(broker->at(sfCoverAvailable) == coverBefore); + BEAST_EXPECT(env.balance(alice, iou) == aliceBalanceBefore); + } + } + + { + testcase("Cover precision guard: Clawback"); + Env env{*this, features}; + auto const [brokerKeylet, iou] = setup(env); + PrettyAmount const subUlpAmt = iou(Number{1, -16}); + auto const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + env(coverClawback(issuer), + kLoanBrokerId(brokerKeylet.key), + kAmount(subUlpAmt), + Ter(expected)); + env.close(); + if (expected == tesSUCCESS) + { + if (auto const broker = env.le(brokerKeylet); BEAST_EXPECT(broker)) + BEAST_EXPECT(broker->at(sfCoverAvailable) == coverBefore); + } + } + + // MPT amounts are integers; scale is 0; the guard never rejects a + // positive integer amount. Verify all three callsites pass with amendment on. + { + testcase("Cover precision guard: MPT min amount passes"); + Env env{*this, all_}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + env.close(); + + PrettyAsset const mptAsset = mptt["MPT"]; + mptt.authorize({.account = alice}); + env.close(); + + env(pay(issuer, alice, mptAsset(100))); + env.close(); + + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = alice, .asset = mptAsset}); + env(createTx); + env.close(); + + auto const brokerKeylet = keylet::loanbroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + env(coverDeposit(alice, brokerKeylet.key, mptAsset(10))); + env.close(); + + env(coverDeposit(alice, brokerKeylet.key, mptAsset(1)), Ter(tesSUCCESS)); + env.close(); + + env(coverWithdraw(alice, brokerKeylet.key, mptAsset(1)), Ter(tesSUCCESS)); + env.close(); + + env(coverClawback(issuer), + kLoanBrokerId(brokerKeylet.key), + kAmount(mptAsset(1)), + Ter(tesSUCCESS)); + env.close(); + } + }; + + runTestCases(all_); + runTestCases(all_ - fixCleanup3_2_0); + } + public: void run() override { + testCoverPrecisionGuard(); + testLoanBrokerSetDebtMaximum(); testLoanBrokerCoverDepositNullVault(); diff --git a/src/test/protocol/STAmount_test.cpp b/src/test/protocol/STAmount_test.cpp index 322d51f84d..c7207589ff 100644 --- a/src/test/protocol/STAmount_test.cpp +++ b/src/test/protocol/STAmount_test.cpp @@ -1205,6 +1205,100 @@ public: //-------------------------------------------------------------------------- + void + testIsZeroAtScale() + { + testcase("isZeroAtScale"); + + Issue const usd{Currency(0x5553440000000000), AccountID(0x4985601)}; + + // IOU: 10 IOU — mantissa = kMinValue (10^15), exponent = -14. + // One ULP at this scale is 10^-14; half-ULP is 5*10^-15. + { + STAmount const ref{usd, STAmount::kMinValue, -14}; + int const refScale = ref.exponent(); // -14 + BEAST_EXPECT(refScale == -14); + + // Zero rounds to zero at any scale. + STAmount const iouZero{usd, 0}; + BEAST_EXPECT(iouZero.isZeroAtScale(refScale)); + + // Sub-ULP: 1e-16 IOU (mantissa = kMinValue, exponent = -31). + // Far below half-ULP → rounds to zero. + STAmount const subUlp{usd, STAmount::kMinValue, -31}; + BEAST_EXPECT(subUlp.isZeroAtScale(refScale)); + + // One ULP: 1e-14 IOU (mantissa = kMinValue, exponent = -29). + // Exactly the smallest representable unit at refScale → not zero. + STAmount const oneUlp{usd, STAmount::kMinValue, -29}; + BEAST_EXPECT(!oneUlp.isZeroAtScale(refScale)); + + // The reference value itself: exponent == scale → returned + // unchanged → not zero. + BEAST_EXPECT(!ref.isZeroAtScale(refScale)); + + // A much larger value: certainly not zero at this scale. + STAmount const large{usd, STAmount::kMinValue, 0}; // 1e15 IOU + BEAST_EXPECT(!large.isZeroAtScale(refScale)); + + // When scale equals the value's own exponent, roundToScale + // short-circuits and returns the value unchanged. + BEAST_EXPECT(!subUlp.isZeroAtScale(subUlp.exponent())); + BEAST_EXPECT(!oneUlp.isZeroAtScale(oneUlp.exponent())); + + // Half-ULP boundary. roundToScale forms (value + ref) - ref + // where ref = 10 IOU has mantissa 1e15 (LSB 0, even). + // Number's default rounding is to-nearest-even, so an exact + // half-ULP tie rounds toward the even-LSB neighbour — the + // reference itself — and the round-trip result is zero. + // Just below half-ULP rounds the same way; just above + // clears half-ULP and bumps the mantissa to 1e15 + 1. + STAmount const justBelowHalf{usd, STAmount::kMinValue * 4, -30}; + BEAST_EXPECT(justBelowHalf.isZeroAtScale(refScale)); + + STAmount const halfUlp{usd, STAmount::kMinValue * 5, -30}; + BEAST_EXPECT(halfUlp.isZeroAtScale(refScale)); + + STAmount const justAboveHalf{usd, STAmount::kMinValue * 6, -30}; + BEAST_EXPECT(!justAboveHalf.isZeroAtScale(refScale)); + + // Large magnitude gap: dust value far below an enormous scale. + // 1e-80 with scale +15 — the value vanishes utterly. + STAmount const dust{usd, STAmount::kMinValue, -95}; + BEAST_EXPECT(dust.isZeroAtScale(15)); + + // Negative values mirror positive behaviour. + STAmount const negSubUlp{usd, STAmount::kMinValue, -31, true}; + BEAST_EXPECT(negSubUlp.isZeroAtScale(refScale)); + + STAmount const negOneUlp{usd, STAmount::kMinValue, -29, true}; + BEAST_EXPECT(!negOneUlp.isZeroAtScale(refScale)); + } + + // XRP is integral — roundToScale short-circuits, value is preserved. + { + STAmount const xrp{XRPAmount{1}}; + BEAST_EXPECT(!xrp.isZeroAtScale(-14)); + BEAST_EXPECT(!xrp.isZeroAtScale(0)); + + STAmount const xrpZero{XRPAmount{0}}; + BEAST_EXPECT(xrpZero.isZeroAtScale(-14)); + } + + // MPT is integral — same short-circuit behaviour as XRP. + { + MPTIssue const mpt{makeMptID(1, AccountID(0x4985601))}; + STAmount const mptAmt{mpt, 1}; + BEAST_EXPECT(!mptAmt.isZeroAtScale(0)); + BEAST_EXPECT(!mptAmt.isZeroAtScale(-14)); + + STAmount const mptZero{mpt, 0}; + BEAST_EXPECT(mptZero.isZeroAtScale(0)); + } + } + + //-------------------------------------------------------------------------- + void run() override { @@ -1223,6 +1317,7 @@ public: testCanSubtractXRP(); testCanSubtractIOU(); testCanSubtractMPT(); + testIsZeroAtScale(); } }; From e24de65f42afc48e8f6743a159dcf24d5db2be0a Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Thu, 21 May 2026 18:13:41 +0200 Subject: [PATCH 3/4] chore: Revert graceful peer disconnection and follow-up fix (#7296) --- include/xrpl/server/detail/Door.h | 8 +- src/xrpld/overlay/detail/ConnectAttempt.cpp | 422 ++++++-------------- src/xrpld/overlay/detail/ConnectAttempt.h | 204 +--------- src/xrpld/overlay/detail/PeerImp.cpp | 302 +++++--------- src/xrpld/overlay/detail/PeerImp.h | 205 +--------- 5 files changed, 246 insertions(+), 895 deletions(-) diff --git a/include/xrpl/server/detail/Door.h b/include/xrpl/server/detail/Door.h index e273ca791f..b72bc9bdea 100644 --- a/include/xrpl/server/detail/Door.h +++ b/include/xrpl/server/detail/Door.h @@ -90,11 +90,11 @@ private: acceptor_type acceptor_; boost::asio::strand strand_; bool ssl_{ - port_.protocol.count("https") > 0 || port_.protocol.count("wss") > 0 || - port_.protocol.count("wss2") > 0 || port_.protocol.count("peer") > 0}; + port_.protocol.contains("https") || port_.protocol.contains("wss") || + port_.protocol.contains("wss2") || port_.protocol.contains("peer")}; bool plain_{ - port_.protocol.count("http") > 0 || port_.protocol.count("ws") > 0 || - (port_.protocol.count("ws2") != 0u)}; + port_.protocol.contains("http") || port_.protocol.contains("ws") || + (port_.protocol.contains("ws2"))}; static constexpr std::chrono::milliseconds kInitialAcceptDelay{50}; static constexpr std::chrono::milliseconds kMaxAcceptDelay{2000}; std::chrono::milliseconds acceptDelay_{kInitialAcceptDelay}; diff --git a/src/xrpld/overlay/detail/ConnectAttempt.cpp b/src/xrpld/overlay/detail/ConnectAttempt.cpp index 1167e19ca3..295f4f5497 100644 --- a/src/xrpld/overlay/detail/ConnectAttempt.cpp +++ b/src/xrpld/overlay/detail/ConnectAttempt.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -28,19 +29,17 @@ #include #include #include -#include #include #include #include #include #include -#include #include #include #include #include -#include +#include #include #include @@ -52,7 +51,7 @@ ConnectAttempt::ConnectAttempt( endpoint_type remoteEndpoint, Resource::Consumer usage, shared_context const& context, - std::uint32_t id, + Peer::id_t id, std::shared_ptr const& slot, beast::Journal journal, OverlayImpl& overlay) @@ -65,7 +64,6 @@ ConnectAttempt::ConnectAttempt( , usage_(usage) , strand_(boost::asio::make_strand(ioContext)) , timer_(ioContext) - , stepTimer_(ioContext) , streamPtr_( std::make_unique( socket_type(std::forward(ioContext)), @@ -78,10 +76,9 @@ ConnectAttempt::ConnectAttempt( ConnectAttempt::~ConnectAttempt() { - // slot_ will be null if we successfully connected - // and transferred ownership to a PeerImp if (slot_ != nullptr) overlay_.peerFinder().onClosed(slot_); + JLOG(journal_.trace()) << "~ConnectAttempt"; } void @@ -92,30 +89,17 @@ ConnectAttempt::stop() boost::asio::post(strand_, std::bind(&ConnectAttempt::stop, shared_from_this())); return; } - - if (!socket_.is_open()) - return; - - JLOG(journal_.debug()) << "stop: Stop"; - - shutdown(); + if (socket_.is_open()) + { + JLOG(journal_.debug()) << "Stop"; + } + close(); } void ConnectAttempt::run() { - if (!strand_.running_in_this_thread()) - { - boost::asio::post(strand_, std::bind(&ConnectAttempt::run, shared_from_this())); - return; - } - - JLOG(journal_.debug()) << "run: connecting to " << remoteEndpoint_; - - ioPending_ = true; - - // Allow up to connectTimeout_ seconds to establish remote peer connection - setTimer(ConnectionStep::TcpConnect); + setTimer(); stream_.next_layer().async_connect( remoteEndpoint_, @@ -126,73 +110,6 @@ ConnectAttempt::run() //------------------------------------------------------------------------------ -void -ConnectAttempt::shutdown() -{ - XRPL_ASSERT( - strand_.running_in_this_thread(), "xrpl::ConnectAttempt::shutdown: strand in this thread"); - - if (!socket_.is_open()) - return; - - shutdown_ = true; - boost::beast::get_lowest_layer(stream_).cancel(); - - tryAsyncShutdown(); -} - -void -ConnectAttempt::tryAsyncShutdown() -{ - XRPL_ASSERT( - strand_.running_in_this_thread(), - "xrpl::ConnectAttempt::tryAsyncShutdown : strand in this thread"); - - if (!shutdown_ || currentStep_ == ConnectionStep::ShutdownStarted) - return; - - if (ioPending_) - return; - - // gracefully shutdown the SSL socket, performing a shutdown handshake - if (currentStep_ != ConnectionStep::TcpConnect && currentStep_ != ConnectionStep::TlsHandshake) - { - setTimer(ConnectionStep::ShutdownStarted); - stream_.async_shutdown(bind_executor( - strand_, - std::bind(&ConnectAttempt::onShutdown, shared_from_this(), std::placeholders::_1))); - return; - } - - close(); -} - -void -ConnectAttempt::onShutdown(error_code ec) -{ - cancelTimer(); - - if (ec) - { - // - eof: the stream was cleanly closed - // - operation_aborted: an expired timer (slow shutdown) - // - stream_truncated: the tcp connection closed (no handshake) it could - // occur if a peer does not perform a graceful disconnect - // - broken_pipe: the peer is gone - // - application data after close notify: benign SSL shutdown condition - bool const shouldLog = - (ec != boost::asio::error::eof && ec != boost::asio::error::operation_aborted && - ec.message().find("application data after close notify") == std::string::npos); - - if (shouldLog) - { - JLOG(journal_.debug()) << "onShutdown: " << ec.message(); - } - } - - close(); -} - void ConnectAttempt::close() { @@ -201,93 +118,50 @@ ConnectAttempt::close() if (!socket_.is_open()) return; - cancelTimer(); + try + { + timer_.cancel(); + socket_.close(); + } + catch (boost::system::system_error const&) // NOLINT(bugprone-empty-catch) + { + // ignored + } - error_code ec; - socket_.close(ec); // NOLINT(bugprone-unused-return-value) + JLOG(journal_.debug()) << "Closed"; } void ConnectAttempt::fail(std::string const& reason) { JLOG(journal_.debug()) << reason; - shutdown(); + close(); } void ConnectAttempt::fail(std::string const& name, error_code ec) { JLOG(journal_.debug()) << name << ": " << ec.message(); - shutdown(); + close(); } void -ConnectAttempt::setTimer(ConnectionStep step) +ConnectAttempt::setTimer() { - currentStep_ = step; - - // Set global timer (only if not already set) - if (timer_.expiry() == std::chrono::steady_clock::time_point{}) - { - try - { - timer_.expires_after(kConnectTimeout); - timer_.async_wait( - boost::asio::bind_executor( - strand_, - std::bind( - &ConnectAttempt::onTimer, shared_from_this(), std::placeholders::_1))); - } - catch (std::exception const& ex) - { - JLOG(journal_.error()) << "setTimer (global): " << ex.what(); - close(); - return; - } - } - - // Set step-specific timer try { - std::chrono::seconds stepTimeout; - switch (step) - { - case ConnectionStep::TcpConnect: - stepTimeout = StepTimeouts::kTcpConnect; - break; - case ConnectionStep::TlsHandshake: - stepTimeout = StepTimeouts::kTlsHandshake; - break; - case ConnectionStep::HttpWrite: - stepTimeout = StepTimeouts::kHttpWrite; - break; - case ConnectionStep::HttpRead: - stepTimeout = StepTimeouts::kHttpRead; - break; - case ConnectionStep::ShutdownStarted: - stepTimeout = StepTimeouts::kTlsShutdown; - break; - case ConnectionStep::Complete: - case ConnectionStep::Init: - return; // No timer needed for init or complete step - } - - // call to expires_after cancels previous timer - stepTimer_.expires_after(stepTimeout); - stepTimer_.async_wait( - boost::asio::bind_executor( - strand_, - std::bind(&ConnectAttempt::onTimer, shared_from_this(), std::placeholders::_1))); - - JLOG(journal_.trace()) << "setTimer: " << stepToString(step) - << " timeout=" << stepTimeout.count() << "s"; + timer_.expires_after(std::chrono::seconds(15)); } - catch (std::exception const& ex) + catch (boost::system::system_error const& e) { - JLOG(journal_.error()) << "setTimer (step " << stepToString(step) << "): " << ex.what(); - close(); + JLOG(journal_.error()) << "setTimer: " << e.code(); return; } + + timer_.async_wait( + boost::asio::bind_executor( + strand_, + std::bind(&ConnectAttempt::onTimer, shared_from_this(), std::placeholders::_1))); } void @@ -296,7 +170,6 @@ ConnectAttempt::cancelTimer() try { timer_.cancel(); - stepTimer_.cancel(); } catch (boost::system::system_error const&) // NOLINT(bugprone-empty-catch) { @@ -321,40 +194,18 @@ ConnectAttempt::onTimer(error_code ec) close(); return; } - - // Determine which timer expired by checking their expiry times - auto const now = std::chrono::steady_clock::now(); - bool const globalExpired = (timer_.expiry() <= now); - bool const stepExpired = (stepTimer_.expiry() <= now); - - if (globalExpired) - { - JLOG(journal_.debug()) << "onTimer: Global timeout; step: " << stepToString(currentStep_); - } - else if (stepExpired) - { - JLOG(journal_.debug()) << "onTimer: Step timeout; step: " << stepToString(currentStep_); - } - else - { - JLOG(journal_.warn()) << "onTimer: Unexpected timer callback"; - } - - close(); + fail("Timeout"); } void ConnectAttempt::onConnect(error_code ec) { - ioPending_ = false; + cancelTimer(); if (ec) { if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); return; - } fail("onConnect", ec); return; @@ -371,15 +222,7 @@ ConnectAttempt::onConnect(error_code ec) return; } - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - - ioPending_ = true; - - setTimer(ConnectionStep::TlsHandshake); + setTimer(); stream_.set_verify_mode(boost::asio::ssl::verify_none); stream_.async_handshake( @@ -392,15 +235,14 @@ ConnectAttempt::onConnect(error_code ec) void ConnectAttempt::onHandshake(error_code ec) { - ioPending_ = false; + cancelTimer(); + if (!socket_.is_open()) + return; if (ec) { if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); return; - } fail("onHandshake", ec); return; @@ -413,21 +255,18 @@ ConnectAttempt::onHandshake(error_code ec) return; } - setTimer(ConnectionStep::HttpWrite); - - // check if we connected to ourselves if (!overlay_.peerFinder().onConnected( slot_, beast::IPAddressConversion::fromAsio(localEndpoint))) { - fail("Self connection"); + fail("Duplicate connection"); return; } auto const sharedValue = makeSharedValue(*streamPtr_, journal_); if (!sharedValue) { - shutdown(); - return; // makeSharedValue logs + close(); // makeSharedValue logs + return; } req_ = makeRequest( @@ -445,14 +284,7 @@ ConnectAttempt::onHandshake(error_code ec) remoteEndpoint_.address(), app_); - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - - ioPending_ = true; - + setTimer(); boost::beast::http::async_write( stream_, req_, @@ -464,30 +296,20 @@ ConnectAttempt::onHandshake(error_code ec) void ConnectAttempt::onWrite(error_code ec) { - ioPending_ = false; + cancelTimer(); + + if (!socket_.is_open()) + return; if (ec) { if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); return; - } fail("onWrite", ec); return; } - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - - ioPending_ = true; - - setTimer(ConnectionStep::HttpRead); - boost::beast::http::async_read( stream_, readBuf_, @@ -501,21 +323,24 @@ void ConnectAttempt::onRead(error_code ec) { cancelTimer(); - ioPending_ = false; - currentStep_ = ConnectionStep::Complete; + + if (!socket_.is_open()) + return; if (ec) { + if (ec == boost::asio::error::operation_aborted) + return; + if (ec == boost::asio::error::eof) { JLOG(journal_.debug()) << "EOF"; - shutdown(); - return; - } - - if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); + setTimer(); + stream_.async_shutdown( + boost::asio::bind_executor( + strand_, + std::bind( + &ConnectAttempt::onShutdown, shared_from_this(), std::placeholders::_1))); return; } @@ -523,13 +348,25 @@ ConnectAttempt::onRead(error_code ec) return; } - if (shutdown_) + processResponse(); +} + +void +ConnectAttempt::onShutdown(error_code ec) +{ + cancelTimer(); + if (!ec) { - tryAsyncShutdown(); + close(); return; } - processResponse(); + if (ec != boost::asio::error::eof) + { + fail("onShutdown", ec); + return; + } + close(); } //-------------------------------------------------------------------------- @@ -537,71 +374,47 @@ ConnectAttempt::onRead(error_code ec) void ConnectAttempt::processResponse() { - if (!OverlayImpl::isPeerUpgrade(response_)) + if (response_.result() == boost::beast::http::status::service_unavailable) { - // A peer may respond with service_unavailable and a list of alternative - // peers to connect to, a differing status code is unexpected - if (response_.result() != boost::beast::http::status::service_unavailable) - { - JLOG(journal_.warn()) << "Unable to upgrade to peer protocol: " << response_.result() - << " (" << response_.reason() << ")"; - shutdown(); - return; - } - - // Parse response body to determine if this is a redirect or other - // service unavailable - std::string responseBody; - responseBody.reserve(boost::asio::buffer_size(response_.body().data())); + json::Value json; + json::Reader r; + std::string s; + s.reserve(boost::asio::buffer_size(response_.body().data())); for (auto const buffer : response_.body().data()) { - responseBody.append( - static_cast(buffer.data()), boost::asio::buffer_size(buffer)); + s.append(static_cast(buffer.data()), boost::asio::buffer_size(buffer)); } - - json::Value json; - json::Reader reader; - auto const isValidJson = reader.parse(responseBody, json); - - // Check if this is a redirect response (contains peer-ips field) - auto const isRedirect = isValidJson && json.isObject() && json.isMember("peer-ips"); - - if (!isRedirect) + auto const success = r.parse(s, json); + if (success) { - JLOG(journal_.warn()) << "processResponse: " << remoteEndpoint_ - << " failed to upgrade to peer protocol: " << response_.result() - << " (" << response_.reason() << ")"; - - shutdown(); - return; + if (json.isObject() && json.isMember("peer-ips")) + { + json::Value const& ips = json["peer-ips"]; + if (ips.isArray()) + { + std::vector eps; + eps.reserve(ips.size()); + for (auto const& v : ips) + { + if (v.isString()) + { + error_code ec; + auto const ep = parseEndpoint(v.asString(), ec); + if (!ec) + eps.push_back(ep); + } + } + overlay_.peerFinder().onRedirects(remoteEndpoint_, eps); + } + } } + } - json::Value const& peerIps = json["peer-ips"]; - if (!peerIps.isArray()) - { - fail("processResponse: invalid peer-ips format"); - return; - } - - // Extract and validate peer endpoints - std::vector redirectEndpoints; - redirectEndpoints.reserve(peerIps.size()); - - for (auto const& ipValue : peerIps) - { - if (!ipValue.isString()) - continue; - - error_code ec; - auto const endpoint = parseEndpoint(ipValue.asString(), ec); - if (!ec) - redirectEndpoints.push_back(endpoint); - } - - // Notify PeerFinder about the redirect redirectEndpoints may be empty - overlay_.peerFinder().onRedirects(remoteEndpoint_, redirectEndpoints); - - fail("processResponse: failed to connect to peer: redirected"); + if (!OverlayImpl::isPeerUpgrade(response_)) + { + JLOG(journal_.info()) << "Unable to upgrade to peer protocol: " << response_.result() + << " (" << response_.reason() << ")"; + close(); return; } @@ -625,8 +438,8 @@ ConnectAttempt::processResponse() auto const sharedValue = makeSharedValue(*streamPtr_, journal_); if (!sharedValue) { - shutdown(); - return; // makeSharedValue logs + close(); // makeSharedValue logs + return; } try @@ -641,30 +454,21 @@ ConnectAttempt::processResponse() usage_.setPublicKey(publicKey); - JLOG(journal_.debug()) << "Protocol: " << to_string(*negotiatedProtocol); JLOG(journal_.info()) << "Public Key: " << toBase58(TokenType::NodePublic, publicKey); + JLOG(journal_.debug()) << "Protocol: " << to_string(*negotiatedProtocol); + auto const member = app_.getCluster().member(publicKey); if (member) { JLOG(journal_.info()) << "Cluster name: " << *member; } - auto const result = overlay_.peerFinder().activate(slot_, publicKey, member.has_value()); + auto const result = + overlay_.peerFinder().activate(slot_, publicKey, static_cast(member)); if (result != PeerFinder::Result::Success) { - std::stringstream ss; - ss << "Outbound Connect Attempt " << remoteEndpoint_ << " " << to_string(result); - fail(ss.str()); - return; - } - - if (!socket_.is_open()) - return; - - if (shutdown_) - { - tryAsyncShutdown(); + fail("Outbound " + std::string(to_string(result))); return; } diff --git a/src/xrpld/overlay/detail/ConnectAttempt.h b/src/xrpld/overlay/detail/ConnectAttempt.h index 949e6accbf..aba224d5c7 100644 --- a/src/xrpld/overlay/detail/ConnectAttempt.h +++ b/src/xrpld/overlay/detail/ConnectAttempt.h @@ -1,42 +1,13 @@ #pragma once +#include #include -#include +#include namespace xrpl { -/** - * @class ConnectAttempt - * @brief Manages outbound peer connection attempts with comprehensive timeout - * handling - * - * The ConnectAttempt class handles the complete lifecycle of establishing an - * outbound connection to a peer in the XRPL network. It implements a - * sophisticated dual-timer system that provides both global timeout protection - * and per-step timeout diagnostics. - * - * The connection establishment follows these steps: - * 1. **TCP Connect**: Establish basic network connection - * 2. **TLS Handshake**: Negotiate SSL/TLS encryption - * 3. **HTTP Write**: Send peer handshake request - * 4. **HTTP Read**: Receive and validate peer response - * 5. **Complete**: Connection successfully established - * - * Uses a hybrid timeout approach: - * - **Global Timer**: Hard limit (20s) for entire connection process - * - **Step Timers**: Individual timeouts for each connection phase - * - * - All errors result in connection termination - * - * All operations are serialized using boost::asio::strand to ensure thread - * safety. The class is designed to be used exclusively within the ASIO event - * loop. - * - * @note This class should not be used directly. It is managed by OverlayImpl - * as part of the peer discovery and connection management system. - * - */ +/** Manages an outbound connection attempt. */ class ConnectAttempt : public OverlayImpl::Child, public std::enable_shared_from_this { @@ -45,95 +16,29 @@ private: using endpoint_type = boost::asio::ip::tcp::endpoint; using request_type = boost::beast::http::request; using response_type = boost::beast::http::response; + using socket_type = boost::asio::ip::tcp::socket; using middle_type = boost::beast::tcp_stream; using stream_type = boost::beast::ssl_stream; using shared_context = std::shared_ptr; - /** - * @enum ConnectionStep - * @brief Represents the current phase of the connection establishment - * process - * - * Used for tracking progress and providing detailed timeout diagnostics. - * Each step has its own timeout value defined in StepTimeouts. - */ - enum class ConnectionStep { - Init, // Initial state, nothing started - TcpConnect, // Establishing TCP connection to remote peer - TlsHandshake, // Performing SSL/TLS handshake - HttpWrite, // Sending HTTP upgrade request - HttpRead, // Reading HTTP upgrade response - Complete, // Connection successfully established - ShutdownStarted // Connection shutdown has started - }; - - // A timeout for connection process, greater than all step timeouts - static constexpr std::chrono::seconds kConnectTimeout{25}; - - /** - * @struct StepTimeouts - * @brief Defines timeout values for each connection step - * - * These timeouts are designed to detect slow individual phases while - * allowing the global timeout to enforce the overall time limit. - */ - struct StepTimeouts - { - // TCP connection timeout - static constexpr std::chrono::seconds kTcpConnect{8}; - // SSL handshake timeout - static constexpr std::chrono::seconds kTlsHandshake{8}; - // HTTP write timeout - static constexpr std::chrono::seconds kHttpWrite{3}; - // HTTP read timeout - static constexpr std::chrono::seconds kHttpRead{3}; - // SSL shutdown timeout - static constexpr std::chrono::seconds kTlsShutdown{2}; - }; - - // Core application and networking components Application& app_; - Peer::id_t const id_; + std::uint32_t const id_; beast::WrappedSink sink_; beast::Journal const journal_; endpoint_type remoteEndpoint_; Resource::Consumer usage_; - boost::asio::strand strand_; boost::asio::basic_waitable_timer timer_; - boost::asio::basic_waitable_timer stepTimer_; - - std::unique_ptr streamPtr_; // SSL stream (owned) + std::unique_ptr streamPtr_; socket_type& socket_; stream_type& stream_; boost::beast::multi_buffer readBuf_; - response_type response_; std::shared_ptr slot_; request_type req_; - bool shutdown_ = false; // Shutdown has been initiated - bool ioPending_ = false; // Async I/O operation in progress - ConnectionStep currentStep_ = ConnectionStep::Init; - public: - /** - * @brief Construct a new ConnectAttempt object - * - * @param app Application context providing configuration and services - * @param ioContext ASIO I/O context for async operations - * @param remoteEndpoint Target peer endpoint to connect to - * @param usage Resource usage tracker for rate limiting - * @param context Shared SSL context for encryption - * @param id Unique peer identifier for this connection attempt - * @param slot PeerFinder slot representing this connection - * @param journal Logging interface for diagnostics - * @param overlay Parent overlay manager - * - * @note The constructor only initializes the object. Call run() to begin - * the actual connection attempt. - */ ConnectAttempt( Application& app, boost::asio::io_context& ioContext, @@ -147,111 +52,38 @@ public: ~ConnectAttempt() override; - /** - * @brief Stop the connection attempt - * - * This method is thread-safe and can be called from any thread. - */ void stop() override; - /** - * @brief Begin the connection attempt - * - * This method is thread-safe and posts to the strand if needed. - */ void run(); private: - /** - * @brief Set timers for the specified connection step - * - * @param step The connection step to set timers for - * - * Sets both the step-specific timer and the global timer (if not already - * set). - */ void - setTimer(ConnectionStep step); - - /** - * @brief Cancel both global and step timers - * - * Used during cleanup and when connection completes successfully. - * Exceptions from timer cancellation are safely ignored. - */ + close(); + void + fail(std::string const& reason); + void + fail(std::string const& name, error_code ec); + void + setTimer(); void cancelTimer(); - - /** - * @brief Handle timer expiration events - * - * @param ec Error code from timer operation - * - * Determines which timer expired (global vs step) and logs appropriate - * diagnostic information before terminating the connection. - */ void onTimer(error_code ec); - - // Connection phase handlers void - onConnect(error_code ec); // TCP connection completion handler + onConnect(error_code ec); void - onHandshake(error_code ec); // TLS handshake completion handler + onHandshake(error_code ec); void - onWrite(error_code ec); // HTTP write completion handler + onWrite(error_code ec); void - onRead(error_code ec); // HTTP read completion handler - - // Error and cleanup handlers + onRead(error_code ec); void - fail(std::string const& reason); // Fail with custom reason - void - fail(std::string const& name, error_code ec); // Fail with system error - void - shutdown(); // Initiate graceful shutdown - void - tryAsyncShutdown(); // Attempt async SSL shutdown - void - onShutdown(error_code ec); // SSL shutdown completion handler - void - close(); // Force close socket - - /** - * @brief Process the HTTP upgrade response from peer - * - * Validates the peer's response, extracts protocol information, - * verifies handshake, and either creates a PeerImp or handles - * redirect responses. - */ + onShutdown(error_code ec); void processResponse(); - static std::string - stepToString(ConnectionStep step) - { - switch (step) - { - case ConnectionStep::Init: - return "Init"; - case ConnectionStep::TcpConnect: - return "TcpConnect"; - case ConnectionStep::TlsHandshake: - return "TlsHandshake"; - case ConnectionStep::HttpWrite: - return "HttpWrite"; - case ConnectionStep::HttpRead: - return "HttpRead"; - case ConnectionStep::Complete: - return "Complete"; - case ConnectionStep::ShutdownStarted: - return "ShutdownStarted"; - } - return "Unknown"; - }; - template static boost::asio::ip::tcp::endpoint parseEndpoint(std::string const& s, boost::system::error_code& ec) diff --git a/src/xrpld/overlay/detail/PeerImp.cpp b/src/xrpld/overlay/detail/PeerImp.cpp index 23e1785f6b..325f8ba038 100644 --- a/src/xrpld/overlay/detail/PeerImp.cpp +++ b/src/xrpld/overlay/detail/PeerImp.cpp @@ -74,7 +74,7 @@ #include #include #include -#include +#include #include @@ -110,9 +110,6 @@ constexpr std::chrono::milliseconds kPeerHighLatency{300}; /** How often we PING the peer to check for latency and sendq probe */ constexpr std::chrono::seconds kPeerTimerInterval{60}; -/** The timeout for a shutdown timer */ -constexpr std::chrono::seconds kShutdownTimerInterval{5}; - } // namespace // TODO: Remove this exclusion once unit tests are added after the hotfix @@ -270,13 +267,7 @@ PeerImp::stop() if (!socket_.is_open()) return; - // The rationale for using different severity levels is that - // outbound connections are under our control and may be logged - // at a higher level, but inbound connections are more numerous and - // uncontrolled so to prevent log flooding the severity is reduced. - JLOG(journal_.debug()) << "stop: Stop"; - - shutdown(); + close(); } //------------------------------------------------------------------------------ @@ -289,17 +280,13 @@ PeerImp::send(std::shared_ptr const& m) post(strand_, std::bind(&PeerImp::send, shared_from_this(), m)); return; } - + if (gracefulClose_) + return; + if (detaching_) + return; if (!socket_.is_open()) return; - // we are in progress of closing the connection - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - auto validator = m->getValidatorKey(); if (validator && !squelch_.expireSquelch(*validator)) { @@ -338,7 +325,6 @@ PeerImp::send(std::shared_ptr const& m) if (sendqSize != 0) return; - writePending_ = true; boost::asio::async_write( stream_, boost::asio::buffer(sendQueue_.front()->getBuffer(compressionEnabled_)), @@ -621,16 +607,20 @@ PeerImp::hasRange(std::uint32_t uMin, std::uint32_t uMax) //------------------------------------------------------------------------------ void -PeerImp::fail(std::string const& name, error_code ec) +PeerImp::close() { - XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::fail : strand in this thread"); - + XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::close : strand in this thread"); if (!socket_.is_open()) return; - JLOG(journal_.warn()) << name << ": " << ec.message(); + detaching_ = true; // DEPRECATED - shutdown(); + cancelTimer(); + error_code ec; + socket_.close(ec); // NOLINT(bugprone-unused-return-value) + + overlay_.incPeerDisconnect(); + JLOG((inbound_ ? journal_.debug() : journal_.info())) << "close: Closed"; } void @@ -644,123 +634,71 @@ PeerImp::fail(std::string const& reason) (void (Peer::*)(std::string const&))&PeerImp::fail, shared_from_this(), reason)); return; } - - if (!socket_.is_open()) - return; - - // Call to name() locks, log only if the message will be outputted - if (journal_.active(beast::Severity::Warning)) + if (journal_.active(beast::Severity::Warning) && socket_.is_open()) { std::string const n = name(); JLOG(journal_.warn()) << n << " failed: " << reason; } - - shutdown(); + close(); } void -PeerImp::tryAsyncShutdown() +PeerImp::fail(std::string const& name, error_code ec) { - XRPL_ASSERT( - strand_.running_in_this_thread(), - "xrpl::PeerImp::tryAsyncShutdown : strand in this thread"); - - if (!shutdown_ || shutdownStarted_) + XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::fail : strand in this thread"); + if (!socket_.is_open()) return; - if (readPending_ || writePending_) - return; - - shutdownStarted_ = true; - - setTimer(kShutdownTimerInterval); - - // gracefully shutdown the SSL socket, performing a shutdown handshake - stream_.async_shutdown(bind_executor( - strand_, std::bind(&PeerImp::onShutdown, shared_from_this(), std::placeholders::_1))); -} - -void -PeerImp::shutdown() -{ - XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::shutdown: strand in this thread"); - - if (!socket_.is_open() || shutdown_) - return; - - shutdown_ = true; - - boost::beast::get_lowest_layer(stream_).cancel(); - - tryAsyncShutdown(); -} - -void -PeerImp::onShutdown(error_code ec) -{ - cancelTimer(); - if (ec) - { - // - eof: the stream was cleanly closed - // - operation_aborted: an expired timer (slow shutdown) - // - stream_truncated: the tcp connection closed (no handshake) it could - // occur if a peer does not perform a graceful disconnect - // - broken_pipe: the peer is gone - bool const shouldLog = - (ec != boost::asio::error::eof && ec != boost::asio::error::operation_aborted && - ec.message().find("application data after close notify") == std::string::npos); - - if (shouldLog) - { - JLOG(journal_.debug()) << "onShutdown: " << ec.message(); - } - } + JLOG(journal_.warn()) << name << ": " << ec.message(); close(); } void -PeerImp::close() +PeerImp::gracefulClose() { - XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::close : strand in this thread"); - - if (!socket_.is_open()) + XRPL_ASSERT( + strand_.running_in_this_thread(), "xrpl::PeerImp::gracefulClose : strand in this thread"); + XRPL_ASSERT(socket_.is_open(), "xrpl::PeerImp::gracefulClose : socket is open"); + XRPL_ASSERT(!gracefulClose_, "xrpl::PeerImp::gracefulClose : socket is not closing"); + gracefulClose_ = true; + if (!sendQueue_.empty()) return; - - cancelTimer(); - - error_code ec; - socket_.close(ec); // NOLINT(bugprone-unused-return-value) - - overlay_.incPeerDisconnect(); - - // The rationale for using different severity levels is that - // outbound connections are under our control and may be logged - // at a higher level, but inbound connections are more numerous and - // uncontrolled so to prevent log flooding the severity is reduced. - JLOG((inbound_ ? journal_.debug() : journal_.info())) << "close: Closed"; + setTimer(); + stream_.async_shutdown(bind_executor( + strand_, std::bind(&PeerImp::onShutdown, shared_from_this(), std::placeholders::_1))); } -//------------------------------------------------------------------------------ - void -PeerImp::setTimer(std::chrono::seconds interval) +PeerImp::setTimer() { try { - timer_.expires_after(interval); + timer_.expires_after(kPeerTimerInterval); } - catch (std::exception const& ex) + catch (boost::system::system_error const& e) { - JLOG(journal_.error()) << "setTimer: " << ex.what(); - shutdown(); + JLOG(journal_.error()) << "setTimer: " << e.code(); return; } - timer_.async_wait(bind_executor( strand_, std::bind(&PeerImp::onTimer, shared_from_this(), std::placeholders::_1))); } +// convenience for ignoring the error code +void +PeerImp::cancelTimer() noexcept +{ + try + { + timer_.cancel(); + } + catch (boost::system::system_error const&) // NOLINT(bugprone-empty-catch) + { + // ignored + } +} + //------------------------------------------------------------------------------ std::string @@ -774,14 +712,11 @@ PeerImp::makePrefix(std::string const& fingerprint) void PeerImp::onTimer(error_code const& ec) { - XRPL_ASSERT(strand_.running_in_this_thread(), "xrpl::PeerImp::onTimer : strand in this thread"); - if (!socket_.is_open()) return; if (ec) { - // do not initiate shutdown, timers are frequently cancelled if (ec == boost::asio::error::operation_aborted) return; @@ -791,15 +726,6 @@ PeerImp::onTimer(error_code const& ec) return; } - // the timer expired before the shutdown completed - // force close the connection - if (shutdown_) - { - JLOG(journal_.debug()) << "onTimer: shutdown timer expired"; - close(); - return; - } - if (largeSendq_++ >= Tuning::kSendqIntervals) { fail("Large send queue"); @@ -840,20 +766,32 @@ PeerImp::onTimer(error_code const& ec) send(std::make_shared(message, protocol::mtPING)); - setTimer(kPeerTimerInterval); + setTimer(); } void -PeerImp::cancelTimer() noexcept +PeerImp::onShutdown(error_code ec) { - try + cancelTimer(); + + if (ec) { - timer_.cancel(); - } - catch (std::exception const& ex) - { - JLOG(journal_.error()) << "cancelTimer: " << ex.what(); + // - eof: the stream was cleanly closed + // - operation_aborted: an expired timer (slow shutdown) + // - stream_truncated: the tcp connection closed (no handshake) it could + // occur if a peer does not perform a graceful disconnect + // - broken_pipe: the peer is gone + bool const shouldLog = + (ec != boost::asio::error::eof && ec != boost::asio::error::operation_aborted && + ec.message().find("application data after close notify") == std::string::npos); + + if (shouldLog) + { + JLOG(journal_.debug()) << "onShutdown: " << ec.message(); + } } + + close(); } //------------------------------------------------------------------------------ @@ -862,15 +800,6 @@ PeerImp::doAccept() { XRPL_ASSERT(readBuffer_.size() == 0, "xrpl::PeerImp::doAccept : empty read buffer"); - JLOG(journal_.debug()) << "doAccept"; - - // a shutdown was initiated before the handshake, there is nothing to do - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - auto const sharedValue = makeSharedValue(*streamPtr_, journal_); // This shouldn't fail since we already computed @@ -881,7 +810,7 @@ PeerImp::doAccept() return; } - JLOG(journal_.debug()) << "Protocol: " << to_string(protocol_); + JLOG(journal_.info()) << "Protocol: " << to_string(protocol_); if (auto member = app_.getCluster().member(publicKey_)) { @@ -921,16 +850,15 @@ PeerImp::doAccept() error_code ec, std::size_t bytesTransferred) { if (!socket_.is_open()) return; - if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); - return; - } if (ec) { + if (ec == boost::asio::error::operation_aborted) + return; + fail("onWriteResponse", ec); return; } + if (writeBuffer->size() == bytesTransferred) { doProtocolStart(); @@ -961,13 +889,6 @@ PeerImp::domain() const void PeerImp::doProtocolStart() { - // a shutdown was initiated before the handshare, there is nothing to do - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - onReadMessage(error_code(), 0); // Send all the validator lists that have been loaded @@ -999,45 +920,31 @@ PeerImp::doProtocolStart() if (auto m = overlay_.getManifestsMessage()) send(m); - setTimer(kPeerTimerInterval); + setTimer(); } // Called repeatedly with protocol message data void PeerImp::onReadMessage(error_code ec, std::size_t bytesTransferred) { - XRPL_ASSERT( - strand_.running_in_this_thread(), "xrpl::PeerImp::onReadMessage : strand in this thread"); - - readPending_ = false; - if (!socket_.is_open()) return; if (ec) { + if (ec == boost::asio::error::operation_aborted) + return; + if (ec == boost::asio::error::eof) { - JLOG(journal_.debug()) << "EOF"; - shutdown(); - return; - } - - if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); + JLOG(journal_.info()) << "EOF"; + gracefulClose(); return; } fail("onReadMessage", ec); return; } - // we started shutdown, no reason to process further data - if (shutdown_) - { - tryAsyncShutdown(); - return; - } if (auto stream = journal_.trace()) { @@ -1062,34 +969,23 @@ PeerImp::onReadMessage(error_code ec, std::size_t bytesTransferred) 350ms, journal_); - if (!socket_.is_open()) - return; - - // the error_code is produced by invokeProtocolMessage - // it could be due to a bad message if (ec) { fail("onReadMessage", ec); return; } + if (!socket_.is_open()) + return; + + if (gracefulClose_) + return; + if (bytesConsumed == 0) break; - readBuffer_.consume(bytesConsumed); } - // check if a shutdown was initiated while processing messages - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - - readPending_ = true; - - XRPL_ASSERT(!shutdownStarted_, "xrpl::PeerImp::onReadMessage : shutdown started"); - // Timeout on writes only stream_.async_read_some( readBuffer_.prepare(std::max(Tuning::kReadBufferBytes, hint)), @@ -1105,26 +1001,17 @@ PeerImp::onReadMessage(error_code ec, std::size_t bytesTransferred) void PeerImp::onWriteMessage(error_code ec, std::size_t bytesTransferred) { - XRPL_ASSERT( - strand_.running_in_this_thread(), "xrpl::PeerImp::onWriteMessage : strand in this thread"); - - writePending_ = false; - if (!socket_.is_open()) return; if (ec) { if (ec == boost::asio::error::operation_aborted) - { - tryAsyncShutdown(); return; - } fail("onWriteMessage", ec); return; } - if (auto stream = journal_.trace()) { stream << "onWriteMessage: " @@ -1135,18 +1022,8 @@ PeerImp::onWriteMessage(error_code ec, std::size_t bytesTransferred) XRPL_ASSERT(!sendQueue_.empty(), "xrpl::PeerImp::onWriteMessage : non-empty send buffer"); sendQueue_.pop(); - - if (shutdown_) - { - tryAsyncShutdown(); - return; - } - if (!sendQueue_.empty()) { - writePending_ = true; - XRPL_ASSERT(!shutdownStarted_, "xrpl::PeerImp::onWriteMessage : shutdown started"); - // Timeout on writes only boost::asio::async_write( stream_, @@ -1160,6 +1037,13 @@ PeerImp::onWriteMessage(error_code ec, std::size_t bytesTransferred) std::placeholders::_2))); return; } + + if (gracefulClose_) + { + stream_.async_shutdown(bind_executor( + strand_, std::bind(&PeerImp::onShutdown, shared_from_this(), std::placeholders::_1))); + return; + } } //------------------------------------------------------------------------------ diff --git a/src/xrpld/overlay/detail/PeerImp.h b/src/xrpld/overlay/detail/PeerImp.h index dca70d88bd..f5d87371be 100644 --- a/src/xrpld/overlay/detail/PeerImp.h +++ b/src/xrpld/overlay/detail/PeerImp.h @@ -30,68 +30,6 @@ namespace xrpl { struct ValidatorBlobInfo; class SHAMap; -/** - * @class PeerImp - * @brief This class manages established peer-to-peer connections, handles - message exchange, monitors connection health, and graceful shutdown. - * - - * The PeerImp shutdown mechanism is a multi-stage process - * designed to ensure graceful connection termination while handling ongoing - * I/O operations safely. The shutdown can be initiated from multiple points - * and follows a deterministic state machine. - * - * The shutdown process can be triggered from several entry points: - * - **External requests**: `stop()` method called by overlay management - * - **Error conditions**: `fail(error_code)` or `fail(string)` on protocol - * violations - * - **Timer expiration**: Various timeout scenarios (ping timeout, large send - * queue) - * - **Connection health**: Peer tracking divergence or unknown state timeouts - * - * The shutdown follows this progression: - * - * Normal Operation → shutdown() → tryAsyncShutdown() → onShutdown() → close() - * ↓ ↓ ↓ ↓ - * Set shutdown_ SSL graceful Timer cancel Socket close - * Cancel timer shutdown start & cleanup & metrics - * 5s safety timer Set shutdownStarted_ update - * - * Two primary flags coordinate the shutdown process: - * - `shutdown_`: Set when shutdown is requested - * - `shutdownStarted_`: Set when SSL shutdown begins - * - * The shutdown mechanism carefully coordinates with ongoing read/write - * operations: - * - * **Read Operations (`onReadMessage`)**: - * - Checks `shutdown_` flag after processing each message batch - * - If shutdown initiated during processing, calls `tryAsyncShutdown()` - * - * **Write Operations (`onWriteMessage`)**: - * - Checks `shutdown_` flag before queuing new writes - * - Calls `tryAsyncShutdown()` when shutdown flag detected - * - * Multiple timers require coordination during shutdown: - * 1. **Peer Timer**: Regular ping/pong timer cancelled immediately in - * `shutdown()` - * 2. **Shutdown Timer**: 5-second safety timer ensures shutdown completion - * 3. **Operation Cancellation**: All pending async operations are cancelled - * - * The shutdown implements fallback mechanisms: - * - **Graceful Path**: SSL shutdown → Socket close → Cleanup - * - **Forced Path**: If SSL shutdown fails or times out, proceeds to socket - * close - * - **Safety Timer**: 5-second timeout prevents hanging shutdowns - * - * All shutdown operations are serialized through the boost::asio::strand to - * ensure thread safety. The strand guarantees that shutdown state changes - * and I/O operation callbacks are executed sequentially. - * - * @note This class requires careful coordination between async operations, - * timer management, and shutdown procedures to ensure no resource leaks - * or hanging connections in high-throughput networking scenarios. - */ class PeerImp : public Peer, public std::enable_shared_from_this, public OverlayImpl::Child { public: @@ -121,8 +59,6 @@ private: socket_type& socket_; stream_type& stream_; boost::asio::strand strand_; - - // Multi-purpose timer for peer activity monitoring and shutdown safety waitable_timer timer_; // Updated at each stage of the connection process to reflect @@ -139,6 +75,7 @@ private: std::atomic tracking_; clock_type::time_point trackingTime_; + bool detaching_ = false; // Node public key of peer. PublicKey const publicKey_; std::string name_; @@ -216,19 +153,7 @@ private: http_response_type response_; boost::beast::http::fields const& headers_; std::queue> sendQueue_; - - // Primary shutdown flag set when shutdown is requested - bool shutdown_ = false; - - // SSL shutdown coordination flag - bool shutdownStarted_ = false; - - // Indicates a read operation is currently pending - bool readPending_ = false; - - // Indicates a write operation is currently pending - bool writePending_ = false; - + bool gracefulClose_ = false; int largeSendq_ = 0; std::unique_ptr loadEvent_; // The highest sequence of each PublisherList that has @@ -476,6 +401,9 @@ public: bool isHighLatency() const override; + void + fail(std::string const& reason); + bool compressionEnabled() const override { @@ -489,129 +417,32 @@ public: } private: - /** - * @brief Handles a failure associated with a specific error code. - * - * This function is called when an operation fails with an error code. It - * logs the warning message and gracefully shutdowns the connection. - * - * The function will do nothing if the connection is already closed or if a - * shutdown is already in progress. - * - * @param name The name of the operation that failed (e.g., "read", - * "write"). - * @param ec The error code associated with the failure. - * @note This function must be called from within the object's strand. - */ - void - fail(std::string const& name, error_code ec); - - /** - * @brief Handles a failure described by a reason string. - * - * This overload is used for logical errors or protocol violations not - * associated with a specific error code. It logs a warning with the - * given reason, then initiates a graceful shutdown. - * - * The function will do nothing if the connection is already closed or if a - * shutdown is already in progress. - * - * @param reason A descriptive string explaining the reason for the failure. - * @note This function must be called from within the object's strand. - */ - void - fail(std::string const& reason); - - /** @brief Initiates the peer disconnection sequence. - * - * This is the primary entry point to start closing a peer connection. It - * marks the peer for shutdown and cancels any outstanding asynchronous - * operations. This cancellation allows the graceful shutdown to proceed - * once the handlers for the cancelled operations have completed. - * - * @note This method must be called on the peer's strand. - */ - void - shutdown(); - - /** @brief Attempts to perform a graceful SSL shutdown if conditions are - * met. - * - * This helper function checks if the peer is in a state where a graceful - * SSL shutdown can be performed (i.e., shutdown has been requested and no - * I/O operations are currently in progress). - * - * @note This method must be called on the peer's strand. - */ - void - tryAsyncShutdown(); - - /** - * @brief Handles the completion of the asynchronous SSL shutdown. - * - * This function is the callback for the `async_shutdown` operation started - * in `shutdown()`. Its first action is to cancel the timer. It - * then inspects the error code to determine the outcome. - * - * Regardless of the result, this function proceeds to call `close()` to - * ensure the underlying socket is fully closed. - * - * @param ec The error code resulting from the `async_shutdown` operation. - */ - void - onShutdown(error_code ec); - - /** - * @brief Forcibly closes the underlying socket connection. - * - * This function provides the final, non-graceful shutdown of the peer - * connection. It ensures any pending timers are cancelled and then - * immediately closes the TCP socket, bypassing the SSL shutdown handshake. - * - * After closing, it notifies the overlay manager of the disconnection. - * - * @note This function must be called from within the object's strand. - */ void close(); - /** - * @brief Sets and starts the peer timer. - * - * This function starts timer, which is used to detect inactivity - * and prevent stalled connections. It sets the timer to expire after the - * predefined `peerTimerInterval`. - * - * @note This function will terminate the connection in case of any errors. - */ void - setTimer(std::chrono::seconds interval); + fail(std::string const& name, error_code ec); - /** - * @brief Handles the expiration of the peer activity timer. - * - * This callback is invoked when the timer set by `setTimer` expires. It - * watches the peer connection, checking for various timeout and health - * conditions. - * - * @param ec The error code associated with the timer's expiration. - * `operation_aborted` is expected if the timer was cancelled. - */ void - onTimer(error_code const& ec); + gracefulClose(); + + void + setTimer(); - /** - * @brief Cancels any pending wait on the peer activity timer. - * - * This function is called to stop the timer. It gracefully manages any - * errors that might occur during the cancellation process. - */ void cancelTimer() noexcept; static std::string makePrefix(std::string const& fingerprint); + // Called when the timer wait completes + void + onTimer(boost::system::error_code const& ec); + + // Called when SSL shutdown completes + void + onShutdown(error_code ec); + void doAccept(); From 79308705c5f90440b81a3a371807a40d357df95e Mon Sep 17 00:00:00 2001 From: Bart Date: Thu, 21 May 2026 18:50:59 +0100 Subject: [PATCH 4/4] release: Bump version to 3.2.0-b6 (#7311) Co-authored-by: Bart <11445373+bthomee@users.noreply.github.com> --- src/libxrpl/protocol/BuildInfo.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxrpl/protocol/BuildInfo.cpp b/src/libxrpl/protocol/BuildInfo.cpp index 820936a22d..3d4fff9f04 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -23,7 +23,7 @@ namespace { //------------------------------------------------------------------------------ // clang-format off // NOLINTNEXTLINE(readability-identifier-naming) -char const* const versionString = "3.3.0-b0" +char const* const versionString = "3.2.0-b6" // clang-format on ;