From 7712cbdfcc1b1bfc7ebe11373a20b08fd30e3c84 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 21:02:01 -0500 Subject: [PATCH 01/16] Fix build issue - unused variable --- src/test/app/Loan_test.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 1778e60b37..ef7a47c3ee 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -6523,8 +6523,6 @@ protected: Account const lender{"lender"}; Account const borrower{"borrower"}; - auto const asset = xrpIssue(); - env.fund(XRP(200'000), lender, borrower); env.close(); From 1c99243ec2abcf63c14b97067cedea64e54beb6d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 21:10:13 -0500 Subject: [PATCH 02/16] Fix service fee accounting when a borrower is the broker (#6016) - Add unit-test to verify the fix. --- src/libxrpl/ledger/View.cpp | 9 +-- src/test/app/Loan_test.cpp | 126 ++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 6 deletions(-) diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index deaf53c05b..84e3073465 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2376,8 +2376,6 @@ accountSendMultiIOU( auto const& receiverID = r.first; STAmount amount{asset, r.second}; - takeFromSender += amount; - if (view.rules().enabled(fixAMMv1_1)) { if (amount < beast::zero) @@ -2423,6 +2421,9 @@ accountSendMultiIOU( view.creditHook(xrpAccount(), receiverID, amount, -rcvBal); view.update(receiver); + + // Take what is actually sent + takeFromSender += amount; } if (auto stream = j.trace()) @@ -2626,10 +2627,6 @@ rippleSendMultiMPT( auto const& receiverID = r.first; STAmount amount{asset, r.second}; - XRPL_ASSERT( - senderID != receiverID, - "ripple::rippleSendMultiMPT : sender is not receiver"); - XRPL_ASSERT( amount >= beast::zero, "ripple::rippleSendMultiMPT : minimum amount "); diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index ef7a47c3ee..f425778a7e 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -6642,6 +6642,131 @@ protected: env, broker, loanParams, loanKeylet, verifyLoanStatus, true); } + void + testBorrowerIsBroker() + { + testcase("Test Borrower is Broker"); + using namespace jtx; + using namespace loan; + Account const broker{"broker"}; + Account const issuer{"issuer"}; + Account const borrower_{"borrower"}; + Account const depositor{"depositor"}; + + auto testLoanAsset = [&](auto&& getMaxDebt, auto const& borrower) { + Env env(*this); + Vault vault(env); + + if (borrower == broker) + env.fund(XRP(10'000), broker, issuer, depositor); + else + env.fund(XRP(10'000), broker, borrower, issuer, depositor); + env.close(); + + auto const xrpFee = XRP(100); + auto const txFee = fee(xrpFee); + + STAmount const debtMaximumRequest = getMaxDebt(env); + + auto const& asset = debtMaximumRequest.asset(); + auto const initialVault = asset(debtMaximumRequest * 100); + + auto [tx, vaultKeylet] = + vault.create({.owner = broker, .asset = asset}); + env(tx, txFee); + env.close(); + + env(vault.deposit( + {.depositor = depositor, + .id = vaultKeylet.key, + .amount = initialVault}), + txFee); + env.close(); + + auto const brokerKeylet = + keylet::loanbroker(broker.id(), env.seq(broker)); + + env(loanBroker::set(broker, vaultKeylet.key), txFee); + env.close(); + + auto const serviceFee = 101; + + env(set(broker, brokerKeylet.key, debtMaximumRequest), + counterparty(borrower), + sig(sfCounterpartySignature, borrower), + loanServiceFee(serviceFee), + paymentTotal(10), + txFee); + env.close(); + + std::uint32_t const loanSequence = 1; + auto const loanKeylet = + keylet::loan(brokerKeylet.key, loanSequence); + + auto const brokerBalanceBefore = env.balance(broker, asset); + + if (auto const loanSle = env.le(loanKeylet); + env.test.BEAST_EXPECT(loanSle)) + { + auto const payment = loanSle->at(sfPeriodicPayment); + auto const totalPayment = payment + serviceFee; + env(loan::pay(borrower, loanKeylet.key, asset(totalPayment)), + txFee); + env.close(); + if (auto const vaultSle = env.le(vaultKeylet); + BEAST_EXPECT(vaultSle)) + { + auto const expected = [&]() { + // The service fee is transferred to the broker if + // a borrower is not the broker + if (borrower != broker) + return brokerBalanceBefore.number() + serviceFee; + // Since a borrower is the broker, the payment is + // transferred to the Vault from the broker but not + // the service fee. + // If the asset is XRP then the broker pays the txfee. + if (asset.native()) + return brokerBalanceBefore.number() - payment - + xrpFee.number(); + return brokerBalanceBefore.number() - payment; + }(); + BEAST_EXPECT( + env.balance(broker, asset).value() == + asset(expected).value()); + } + } + }; + // Test when a borrower is the broker and is not to verify correct + // service fee transfer in both cases. + for (auto const& borrowerAcct : {broker, borrower_}) + { + testLoanAsset( + [&](Env&) -> STAmount { return STAmount{XRPAmount{200'000}}; }, + borrowerAcct); + testLoanAsset( + [&](Env& env) -> STAmount { + auto const IOU = issuer["USD"]; + env(trust(broker, IOU(1'000'000'000))); + env(trust(depositor, IOU(1'000'000'000))); + env(pay(issuer, broker, IOU(100'000'000))); + env(pay(issuer, depositor, IOU(100'000'000))); + env.close(); + return IOU(200'000); + }, + borrowerAcct); + testLoanAsset( + [&](Env& env) -> STAmount { + MPTTester mpt( + {.env = env, + .issuer = issuer, + .holders = {broker, depositor}, + .pay = 100'000'000}); + return mpt(200'000); + }, + borrowerAcct); + } + } + public: void run() override @@ -6687,6 +6812,7 @@ public: testRIPD3901(); testRIPD3902(); testRoundingAllowsUndercoverage(); + testBorrowerIsBroker(); } }; From 8d22409ab507542fa4e23e9060c31d825b8ccb2c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 20:25:57 -0500 Subject: [PATCH 03/16] review feedback: Use the specific type in the "SendMulti" functions --- src/libxrpl/ledger/View.cpp | 72 +++++++++++++------------------------ 1 file changed, 24 insertions(+), 48 deletions(-) diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 84e3073465..0ddea91c5d 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2113,26 +2113,26 @@ static TER rippleSendMultiIOU( ApplyView& view, AccountID const& senderID, - Asset const& asset, + Issue const& issue, MultiplePaymentDestinations const& receivers, STAmount& actual, beast::Journal j, WaiveTransferFee waiveFee) { - auto const issuer = asset.getIssuer(); + auto const issuer = issue.getIssuer(); XRPL_ASSERT( !isXRP(senderID), "ripple::rippleSendMultiIOU : sender is not XRP"); // These may diverge - STAmount takeFromSender{asset}; + STAmount takeFromSender{issue}; actual = takeFromSender; // Failures return immediately. for (auto const& r : receivers) { auto const& receiverID = r.first; - STAmount amount{asset, r.second}; + STAmount amount{issue, r.second}; /* If we aren't sending anything or if the sender is the same as the * receiver then we don't need to do anything. @@ -2315,7 +2315,7 @@ static TER accountSendMultiIOU( ApplyView& view, AccountID const& senderID, - Asset const& asset, + Issue const& issue, MultiplePaymentDestinations const& receivers, beast::Journal j, WaiveTransferFee waiveFee) @@ -2325,27 +2325,14 @@ accountSendMultiIOU( "ripple::accountSendMultiIOU", "multiple recipients provided"); - if (view.rules().enabled(fixAMMv1_1)) - { - if (asset.holds()) - { - return tecINTERNAL; - } - } - else - { - XRPL_ASSERT( - !asset.holds(), "ripple::accountSendMultiIOU : not MPT"); - } - - if (!asset.native()) + if (!issue.native()) { STAmount actual; JLOG(j.trace()) << "accountSendMultiIOU: " << to_string(senderID) << " sending " << receivers.size() << " IOUs"; return rippleSendMultiIOU( - view, senderID, asset, receivers, actual, j, waiveFee); + view, senderID, issue, receivers, actual, j, waiveFee); } /* XRP send which does not check reserve and can do pure adjustment. @@ -2370,24 +2357,15 @@ accountSendMultiIOU( } // Failures return immediately. - STAmount takeFromSender{asset}; + STAmount takeFromSender{issue}; for (auto const& r : receivers) { auto const& receiverID = r.first; - STAmount amount{asset, r.second}; + STAmount amount{issue, r.second}; - if (view.rules().enabled(fixAMMv1_1)) + if (amount < beast::zero) { - if (amount < beast::zero) - { - return tecINTERNAL; - } - } - else - { - XRPL_ASSERT( - amount >= beast::zero, - "ripple::accountSendMultiIOU : minimum amount"); + return tecINTERNAL; // LCOV_EXCL_LINE } /* If we aren't sending anything or if the sender is the same as the @@ -2603,7 +2581,7 @@ static TER rippleSendMultiMPT( ApplyView& view, AccountID const& senderID, - Asset const& asset, + MPTIssue const& mptIssue, MultiplePaymentDestinations const& receivers, STAmount& actual, beast::Journal j, @@ -2611,25 +2589,25 @@ rippleSendMultiMPT( { // Safe to get MPT since rippleSendMultiMPT is only called by // accountSendMultiMPT - auto const issuer = asset.getIssuer(); + auto const issuer = mptIssue.getIssuer(); - auto const sle = - view.read(keylet::mptIssuance(asset.get().getMptID())); + auto const sle = view.read(keylet::mptIssuance(mptIssue.getMptID())); if (!sle) return tecOBJECT_NOT_FOUND; // These may diverge - STAmount takeFromSender{asset}; + STAmount takeFromSender{mptIssue}; actual = takeFromSender; for (auto const& r : receivers) { auto const& receiverID = r.first; - STAmount amount{asset, r.second}; + STAmount amount{mptIssue, r.second}; - XRPL_ASSERT( - amount >= beast::zero, - "ripple::rippleSendMultiMPT : minimum amount "); + if (amount < beast::zero) + { + return tecINTERNAL; // LCOV_EXCL_LINE + } /* If we aren't sending anything or if the sender is the same as the * receiver then we don't need to do anything. @@ -2720,17 +2698,15 @@ static TER accountSendMultiMPT( ApplyView& view, AccountID const& senderID, - Asset const& asset, + MPTIssue const& mptIssue, MultiplePaymentDestinations const& receivers, beast::Journal j, WaiveTransferFee waiveFee) { - XRPL_ASSERT(asset.holds(), "ripple::accountSendMultiMPT : MPT"); - STAmount actual; return rippleSendMultiMPT( - view, senderID, asset, receivers, actual, j, waiveFee); + view, senderID, mptIssue, receivers, actual, j, waiveFee); } TER @@ -2771,10 +2747,10 @@ accountSendMulti( [&](TIss const& issue) { if constexpr (std::is_same_v) return accountSendMultiIOU( - view, senderID, asset, receivers, j, waiveFee); + view, senderID, issue, receivers, j, waiveFee); else return accountSendMultiMPT( - view, senderID, asset, receivers, j, waiveFee); + view, senderID, issue, receivers, j, waiveFee); }, asset.value()); } From 4396b77c4b9d6587233f10d18b918a635f00ff2d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 23:17:42 -0500 Subject: [PATCH 04/16] Add tfLoanLatePayment flag; full payment is no longer a special case - A regular payment that is late, or a tfLoanLatePayment that is not late will fail. - Flags are mutually exclusive. - Add a few interest computation shortcuts and overflow prevention checks that return 0 if there's no time to compute for. --- include/xrpl/protocol/TxFlags.h | 3 +- src/test/app/Loan_test.cpp | 34 +- src/xrpld/app/misc/LendingHelpers.h | 27 +- src/xrpld/app/misc/detail/LendingHelpers.cpp | 386 +++++++++---------- src/xrpld/app/tx/detail/LoanPay.cpp | 54 +-- 5 files changed, 260 insertions(+), 244 deletions(-) diff --git a/include/xrpl/protocol/TxFlags.h b/include/xrpl/protocol/TxFlags.h index d3a2fecf45..0f3ea94b21 100644 --- a/include/xrpl/protocol/TxFlags.h +++ b/include/xrpl/protocol/TxFlags.h @@ -274,10 +274,11 @@ constexpr std::uint32_t const tfLoanOverpayment = 0x00010000; // LoanPay exclusive flags: // tfLoanFullPayment: True, indicates that the payment is constexpr std::uint32_t const tfLoanFullPayment = 0x00020000; +constexpr std::uint32_t const tfLoanLatePayment = 0x00040000; constexpr std::uint32_t const tfLoanSetMask = ~(tfUniversal | tfLoanOverpayment); constexpr std::uint32_t const tfLoanPayMask = ~(tfUniversal | - tfLoanOverpayment | tfLoanFullPayment); + tfLoanOverpayment | tfLoanFullPayment | tfLoanLatePayment); // LoanManage flags: constexpr std::uint32_t const tfLoanDefault = 0x00010000; diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index f425778a7e..536ac31eda 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -1059,8 +1059,7 @@ protected: // Make the payment env(pay(borrower, loanKeylet.key, transactionAmount)); - env.close( - d{(state.previousPaymentDate + state.nextPaymentDate) / 2}); + env.close(d{state.paymentInterval / 2}); // Need to account for fees if the loan is in XRP PrettyAmount adjustment = broker.asset(0); @@ -2227,14 +2226,29 @@ protected: (Number{15, -1} / loanPaymentsPerFeeIncrement + 1)}), ter(temINVALID_FLAG)); } - // Try to send a payment marked as both full payment and - // overpayment. Do not include `txFlags`, so we don't duplicate the - // prior test transaction. + // Try to send a payment marked as multiple mutually exclusive + // payment types. Do not include `txFlags`, so we don't duplicate + // the prior test transaction. + env(pay(borrower, + loanKeylet.key, + broker.asset(state.periodicPayment * 2), + tfLoanLatePayment | tfLoanFullPayment), + ter(temINVALID_FLAG)); + env(pay(borrower, + loanKeylet.key, + broker.asset(state.periodicPayment * 2), + tfLoanLatePayment | tfLoanOverpayment), + ter(temINVALID_FLAG)); env(pay(borrower, loanKeylet.key, broker.asset(state.periodicPayment * 2), tfLoanOverpayment | tfLoanFullPayment), ter(temINVALID_FLAG)); + env(pay(borrower, + loanKeylet.key, + broker.asset(state.periodicPayment * 2), + tfLoanLatePayment | tfLoanOverpayment | tfLoanFullPayment), + ter(temINVALID_FLAG)); { auto const otherAsset = broker.asset.raw() == assets[0].raw() @@ -4562,7 +4576,11 @@ protected: issuer, lender["IOU"](1'000), tfClearFreeze | tfClearDeepFreeze)); env.close(); - env(pay(borrower, loanKeylet.key, debtMaximumRequest)); + // The payment is late by this point + env(pay(borrower, loanKeylet.key, debtMaximumRequest), ter(tecEXPIRED)); + env.close(); + env(pay( + borrower, loanKeylet.key, debtMaximumRequest, tfLoanLatePayment)); env.close(); // preclaim: tecKILLED @@ -6777,11 +6795,10 @@ public: testPoC_UnsignedUnderflowOnFullPayAfterEarlyPeriodic(); testLoanCoverMinimumRoundingExploit(); #endif - testDustManipulation(); - testIssuerLoan(); testDisabled(); testSelfLoan(); + testIssuerLoan(); testLoanSet(); testLifecycle(); testServiceFeeOnBrokerDeepFreeze(); @@ -6806,6 +6823,7 @@ public: testLoanNextPaymentDueDateOverflow(); testRequireAuth(); + testDustManipulation(); testRIPD3831(); testRIPD3459(); diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index d9e27414fa..c32b578d0b 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -30,21 +30,24 @@ roundPeriodicPayment( struct LoanPaymentParts { /// principal_paid is the amount of principal that the payment covered. - Number principalPaid; + Number principalPaid = numZero; /// interest_paid is the amount of interest that the payment covered. - Number interestPaid; + Number interestPaid = numZero; /** * value_change is the amount by which the total value of the Loan changed. * If value_change < 0, Loan value decreased. * If value_change > 0, Loan value increased. * This is 0 for regular payments. */ - Number valueChange; + Number valueChange = numZero; /// feePaid is amount of fee that is paid to the broker - Number feePaid; + Number feePaid = numZero; LoanPaymentParts& operator+=(LoanPaymentParts const& other); + + bool + operator==(LoanPaymentParts const& other) const; }; /** This structure describes the initial "computed" properties of a loan. @@ -240,15 +243,11 @@ computeLoanProperties( bool isRounded(Asset const& asset, Number const& value, std::int32_t scale); -Expected -loanMakeFullPayment( - Asset const& asset, - ApplyView& view, - SLE::ref loan, - SLE::const_ref brokerSle, - STAmount const& amount, - bool const overpaymentAllowed, - beast::Journal j); +// Indicates what type of payment is being made. +// regular, late, and full are mutually exclusive. +// overpayment is an "add on" to a regular payment, and follows that path with +// potential extra work at the end. +enum class LoanPaymentType { regular = 0, late, full, overpayment }; Expected loanMakePayment( @@ -257,7 +256,7 @@ loanMakePayment( SLE::ref loan, SLE::const_ref brokerSle, STAmount const& amount, - bool const overpaymentAllowed, + LoanPaymentType const paymentType, beast::Journal j); } // namespace ripple diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index 8c349aafa2..b980e73353 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -35,6 +35,14 @@ LoanPaymentParts::operator+=(LoanPaymentParts const& other) return *this; } +bool +LoanPaymentParts::operator==(LoanPaymentParts const& other) const +{ + return principalPaid == other.principalPaid && + interestPaid == other.interestPaid && + valueChange == other.valueChange && feePaid == other.feePaid; +} + Number loanPeriodicRate(TenthBips32 interestRate, std::uint32_t paymentInterval) { @@ -170,6 +178,12 @@ loanLatePaymentInterest( * * The spec is to be updated to base the duration on the next due date */ + + // If the payment is not late by any amount of time, then there's no late + // interest + if (parentCloseTime.time_since_epoch().count() <= nextPaymentDueDate) + return 0; + auto const secondsOverdue = parentCloseTime.time_since_epoch().count() - nextPaymentDueDate; @@ -193,6 +207,11 @@ loanAccruedInterest( */ auto const lastPaymentDate = std::max(prevPaymentDate, startDate); + // If the loan has been paid ahead, then "lastPaymentDate" is in the future, + // and no interest has accrued. + if (parentCloseTime.time_since_epoch().count() <= lastPaymentDate) + return 0; + auto const secondsSinceLastPayment = parentCloseTime.time_since_epoch().count() - lastPaymentDate; @@ -818,7 +837,7 @@ computeLatePayment( beast::Journal j) { if (!hasExpired(view, nextDueDate)) - return Unexpected(tesSUCCESS); + return Unexpected(tecTOO_SOON); // the payment is late // Late payment interest is only the part of the interest that comes @@ -972,9 +991,19 @@ computeFullPayment( "ripple::detail::computeFullPayment", "total due is rounded"); + JLOG(j.trace()) << "computeFullPayment result: periodicPayment: " + << periodicPayment << ", periodicRate: " << periodicRate + << ", paymentRemaining: " << paymentRemaining + << ", rawPrincipalOutstanding: " << rawPrincipalOutstanding + << ", fullPaymentInterest: " << fullPaymentInterest + << ", roundedFullInterest: " << roundedFullInterest + << ", roundedFullManagementFee: " + << roundedFullManagementFee + << ", untrackedInterest: " << full.untrackedInterest; + if (amount < full.totalDue) // If the payment is less than the full payment amount, it's not - // sufficient to be a full payment, but that's not an error. + // sufficient to be a full payment. return Unexpected(tecINSUFFICIENT_PAYMENT); return full; @@ -1639,111 +1668,6 @@ computeLoanProperties( .firstPaymentPrincipal = firstPaymentPrincipal}; } -Expected -loanMakeFullPayment( - Asset const& asset, - ApplyView& view, - SLE::ref loan, - SLE::const_ref brokerSle, - STAmount const& amount, - bool const overpaymentAllowed, - beast::Journal j) -{ - auto principalOutstandingProxy = loan->at(sfPrincipalOutstanding); - auto paymentRemainingProxy = loan->at(sfPaymentRemaining); - - if (paymentRemainingProxy == 0 || principalOutstandingProxy == 0) - { - // Loan complete - JLOG(j.warn()) << "Loan is already paid off."; - return Unexpected(tecKILLED); - } - - auto totalValueOutstandingProxy = loan->at(sfTotalValueOutstanding); - auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding); - - // Next payment due date must be set unless the loan is complete - auto nextDueDateProxy = loan->at(~sfNextPaymentDueDate); - if (!nextDueDateProxy) - { - JLOG(j.warn()) << "Loan next payment due date is not set."; - return Unexpected(tecINTERNAL); - } - - std::int32_t const loanScale = loan->at(sfLoanScale); - - TenthBips32 const interestRate{loan->at(sfInterestRate)}; - TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)}; - - Number const closePaymentFee = - roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale); - TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)}; - - auto const periodicPayment = loan->at(sfPeriodicPayment); - - auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDate); - std::uint32_t const startDate = loan->at(sfStartDate); - - std::uint32_t const paymentInterval = loan->at(sfPaymentInterval); - // Compute the normal periodic rate, payment, etc. - // We'll need it in the remaining calculations - Number const periodicRate = loanPeriodicRate(interestRate, paymentInterval); - XRPL_ASSERT( - interestRate == 0 || periodicRate > 0, - "ripple::loanMakeFullPayment : valid rate"); - - XRPL_ASSERT( - *totalValueOutstandingProxy > 0, - "ripple::loanMakeFullPayment : valid total value"); - - view.update(loan); - - // ------------------------------------------------------------- - // full payment handling - LoanState const roundedLoanState = calculateRoundedLoanState( - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy); - - if (auto const fullPaymentComponents = detail::computeFullPayment( - asset, - view, - principalOutstandingProxy, - managementFeeOutstandingProxy, - periodicPayment, - paymentRemainingProxy, - prevPaymentDateProxy, - startDate, - paymentInterval, - closeInterestRate, - loanScale, - roundedLoanState.interestDue, - periodicRate, - closePaymentFee, - amount, - managementFeeRate, - j)) - return doPayment( - *fullPaymentComponents, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - else if (fullPaymentComponents.error()) - // error() will be the TER returned if a payment is not made. It - // will only evaluate to true if it's unsuccessful. Otherwise, - // tesSUCCESS means nothing was done, so continue. - return Unexpected(fullPaymentComponents.error()); - - // LCOV_EXCL_START - UNREACHABLE("ripple::loanMakeFullPayment : invalid result"); - return Unexpected(tecINTERNAL); - // LCOV_EXCL_STOP -} - Expected loanMakePayment( Asset const& asset, @@ -1751,7 +1675,7 @@ loanMakePayment( SLE::ref loan, SLE::const_ref brokerSle, STAmount const& amount, - bool const overpaymentAllowed, + LoanPaymentType const paymentType, beast::Journal j) { /* @@ -1785,16 +1709,14 @@ loanMakePayment( std::int32_t const loanScale = loan->at(sfLoanScale); TenthBips32 const interestRate{loan->at(sfInterestRate)}; - TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)}; - TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)}; Number const serviceFee = loan->at(sfLoanServiceFee); - Number const latePaymentFee = loan->at(sfLatePaymentFee); TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)}; Number const periodicPayment = loan->at(sfPeriodicPayment); auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDate); + std::uint32_t const startDate = loan->at(sfStartDate); std::uint32_t const paymentInterval = loan->at(sfPaymentInterval); // Compute the normal periodic rate, payment, etc. @@ -1810,7 +1732,7 @@ loanMakePayment( view.update(loan); - detail::PaymentComponentsPlus const periodic{ + detail::PaymentComponentsPlus periodic{ detail::computePaymentComponents( asset, loanScale, @@ -1829,21 +1751,141 @@ loanMakePayment( // ------------------------------------------------------------- // late payment handling - if (auto const latePaymentComponents = detail::computeLatePayment( - asset, - view, - principalOutstandingProxy, - *nextDueDateProxy, - periodic, - lateInterestRate, - loanScale, - latePaymentFee, - amount, - managementFeeRate, - j)) + if (paymentType == LoanPaymentType::late) { - return doPayment( - *latePaymentComponents, + TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)}; + Number const latePaymentFee = loan->at(sfLatePaymentFee); + + if (auto const latePaymentComponents = detail::computeLatePayment( + asset, + view, + principalOutstandingProxy, + *nextDueDateProxy, + periodic, + lateInterestRate, + loanScale, + latePaymentFee, + amount, + managementFeeRate, + j)) + { + return doPayment( + *latePaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + paymentRemainingProxy, + prevPaymentDateProxy, + nextDueDateProxy, + paymentInterval); + } + else if (latePaymentComponents.error()) + { + // error() will be the TER returned if a payment is not made. It + // will only evaluate to true if it's unsuccessful. + return Unexpected(latePaymentComponents.error()); + } + + // LCOV_EXCL_START + UNREACHABLE("ripple::loanMakePayment : invalid late payment result"); + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + else if (hasExpired(view, nextDueDateProxy)) + { + // If the payment is late, and the late flag was not set, it's not valid + JLOG(j.warn()) + << "Loan payment is overdue. Use the tfLoanLatePayment transaction " + "flag to make a late payment. Loan was created on " + << startDate << ", prev payment due date is " + << prevPaymentDateProxy << ", next payment due date is " + << *nextDueDateProxy << ", ledger time is " + << view.parentCloseTime().time_since_epoch().count(); + return Unexpected(tecEXPIRED); + } + + // ------------------------------------------------------------- + // full payment handling + if (paymentType == LoanPaymentType::full) + { + TenthBips32 const closeInterestRate{loan->at(sfCloseInterestRate)}; + Number const closePaymentFee = + roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale); + + LoanState const roundedLoanState = calculateRoundedLoanState( + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy); + + if (auto const fullPaymentComponents = detail::computeFullPayment( + asset, + view, + principalOutstandingProxy, + managementFeeOutstandingProxy, + periodicPayment, + paymentRemainingProxy, + prevPaymentDateProxy, + startDate, + paymentInterval, + closeInterestRate, + loanScale, + roundedLoanState.interestDue, + periodicRate, + closePaymentFee, + amount, + managementFeeRate, + j)) + { + return doPayment( + *fullPaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + paymentRemainingProxy, + prevPaymentDateProxy, + nextDueDateProxy, + paymentInterval); + } + else if (fullPaymentComponents.error()) + // error() will be the TER returned if a payment is not made. It + // will only evaluate to true if it's unsuccessful. Otherwise, + // tesSUCCESS means nothing was done, so continue. + return Unexpected(fullPaymentComponents.error()); + + // LCOV_EXCL_START + UNREACHABLE("ripple::loanMakePayment : invalid full payment result"); + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + + // ------------------------------------------------------------- + // regular periodic payment handling + + XRPL_ASSERT_PARTS( + paymentType == LoanPaymentType::regular || + paymentType == LoanPaymentType::overpayment, + "ripple::loanMakePayment", + "regular payment type"); + + // This will keep a running total of what is actually paid, if the payment + // is sufficient for any payment + LoanPaymentParts totalParts; + Number totalPaid; + std::size_t numPayments = 0; + + while (amount >= totalPaid + periodic.totalDue && + paymentRemainingProxy > 0 && + numPayments < loanMaximumPaymentsPerTransaction) + { + // Try to make more payments + XRPL_ASSERT_PARTS( + periodic.trackedPrincipalDelta >= 0, + "ripple::loanMakePayment", + "payment pays non-negative principal"); + + totalPaid += periodic.totalDue; + totalParts += detail::doPayment( + periodic, totalValueOutstandingProxy, principalOutstandingProxy, managementFeeOutstandingProxy, @@ -1851,47 +1893,19 @@ loanMakePayment( prevPaymentDateProxy, nextDueDateProxy, paymentInterval); - } - else if (latePaymentComponents.error()) - // error() will be the TER returned if a payment is not made. It will - // only evaluate to true if it's unsuccessful. Otherwise, tesSUCCESS - // means nothing was done, so continue. - return Unexpected(latePaymentComponents.error()); + ++numPayments; - // ------------------------------------------------------------- - // regular periodic payment handling + XRPL_ASSERT_PARTS( + (periodic.specialCase == detail::PaymentSpecialCase::final) == + (paymentRemainingProxy == 0), + "ripple::loanMakePayment", + "final payment is the final payment"); - // if the payment is not late nor if it's a full payment, then it must - // be a periodic one, with possible overpayments + // Don't compute the next payment if this was the last payment + if (periodic.specialCase == detail::PaymentSpecialCase::final) + break; - // This will keep a running total of what is actually paid, if the payment - // is sufficient for a single payment - Number totalPaid = periodic.totalDue; - - if (amount < totalPaid) - { - JLOG(j.warn()) << "Periodic loan payment amount is insufficient. Due: " - << totalPaid << ", paid: " << amount; - return Unexpected(tecINSUFFICIENT_PAYMENT); - } - - LoanPaymentParts totalParts = detail::doPayment( - periodic, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - - std::size_t numPayments = 1; - - while (totalPaid < amount && paymentRemainingProxy > 0 && - numPayments < loanMaximumPaymentsPerTransaction) - { - // Try to make more payments - detail::PaymentComponentsPlus const nextPayment{ + periodic = detail::PaymentComponentsPlus{ detail::computePaymentComponents( asset, loanScale, @@ -1903,40 +1917,13 @@ loanMakePayment( paymentRemainingProxy, managementFeeRate), serviceFee}; - XRPL_ASSERT_PARTS( - nextPayment.trackedPrincipalDelta >= 0, - "ripple::loanMakePayment", - "additional payment pays non-negative principal"); -#if LOANCOMPLETE - XRPL_ASSERT( - nextPayment.rawInterest <= periodic.rawInterest, - "ripple::loanMakePayment : decreasing interest"); - XRPL_ASSERT( - nextPayment.rawPrinicpal >= periodic.rawPrincipal, - "ripple::loanMakePayment : increasing principal"); -#endif + } - if (amount < totalPaid + nextPayment.totalDue) - // We're done making payments. - break; - - totalPaid += nextPayment.totalDue; - totalParts += detail::doPayment( - nextPayment, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - ++numPayments; - - XRPL_ASSERT_PARTS( - (nextPayment.specialCase == detail::PaymentSpecialCase::final) == - (paymentRemainingProxy == 0), - "ripple::loanMakePayment", - "final payment is the final payment"); + if (numPayments == 0) + { + JLOG(j.warn()) << "Regular loan payment amount is insufficient. Due: " + << periodic.totalDue << ", paid: " << amount; + return Unexpected(tecINSUFFICIENT_PAYMENT); } XRPL_ASSERT_PARTS( @@ -1952,8 +1939,9 @@ loanMakePayment( // ------------------------------------------------------------- // overpayment handling - if (overpaymentAllowed && loan->isFlag(lsfLoanOverpayment) && - paymentRemainingProxy > 0 && nextDueDateProxy && totalPaid < amount && + if (paymentType == LoanPaymentType::overpayment && + loan->isFlag(lsfLoanOverpayment) && paymentRemainingProxy > 0 && + nextDueDateProxy && totalPaid < amount && numPayments < loanMaximumPaymentsPerTransaction) { TenthBips32 const overpaymentInterestRate{ diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index fe19ed1c0d..64e18016e1 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -30,9 +30,21 @@ LoanPay::preflight(PreflightContext const& ctx) if (ctx.tx[sfAmount] <= beast::zero) return temBAD_AMOUNT; - // isFlag requires an exact match - all flags to be set - to return true. - if (ctx.tx.isFlag(tfLoanOverpayment | tfLoanFullPayment)) + // The loan payment flags are all mutually exclusive. If more than one is + // set, the tx is malformed. + int flagsSet = 0; + for (auto const flag : + {tfLoanLatePayment, tfLoanFullPayment, tfLoanOverpayment}) + { + if (ctx.tx.isFlag(flag)) + ++flagsSet; + } + if (flagsSet > 1) + { + JLOG(ctx.j.warn()) << "Only one LoanPay flag can be set per tx. " + << flagsSet << " is too many."; return temINVALID_FLAG; + } return tesSUCCESS; } @@ -42,8 +54,8 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) { auto const normalCost = Transactor::calculateBaseFee(view, tx); - if (tx.isFlag(tfLoanFullPayment)) - // The loan will be making one set of calculations for one (large) + if (tx.isFlag(tfLoanFullPayment) || tx.isFlag(tfLoanLatePayment)) + // The loan will be making one set of calculations for one full or late // payment return normalCost; @@ -65,7 +77,8 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) } if (hasExpired(view, loanSle->at(sfNextPaymentDueDate))) - // If the payment is late, it'll only make one payment + // If the payment is late, and the late payment flag is not set, it'll + // fail return normalCost; auto const brokerSle = @@ -97,6 +110,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) // Estimate how many payments will be made Number const numPaymentEstimate = static_cast(amount / regularPayment); + // Charge one base fee per paymentsPerFeeIncrement payments, rounding up. Number::setround(Number::upward); auto const feeIncrements = std::max( @@ -291,23 +305,19 @@ LoanPay::doApply() LoanManage::unimpairLoan(view, loanSle, vaultSle, j_); } - Expected const paymentParts = - tx.isFlag(tfLoanFullPayment) ? loanMakeFullPayment( - asset, - view, - loanSle, - brokerSle, - amount, - tx.isFlag(tfLoanOverpayment), - j_) - : loanMakePayment( - asset, - view, - loanSle, - brokerSle, - amount, - tx.isFlag(tfLoanOverpayment), - j_); + LoanPaymentType const paymentType = [&tx]() { + // preflight already checked that at most one flag is set. + if (tx.isFlag(tfLoanLatePayment)) + return LoanPaymentType::late; + if (tx.isFlag(tfLoanFullPayment)) + return LoanPaymentType::full; + if (tx.isFlag(tfLoanOverpayment)) + return LoanPaymentType::overpayment; + return LoanPaymentType::regular; + }(); + + Expected const paymentParts = loanMakePayment( + asset, view, loanSle, brokerSle, amount, paymentType, j_); if (!paymentParts) { From 21eb13de0c7075d8cd5d8d2deb13b94277668d61 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 23:28:25 -0500 Subject: [PATCH 05/16] Reorder payment options to do full early payment first - Since all the special cases are now specified with flags, the order is less important. - Avoids computing the periodic payment parts that are not needed for full payment computation. - A late payment without the late payment flag will override everything else, though. --- src/xrpld/app/misc/detail/LendingHelpers.cpp | 125 ++++++++++--------- 1 file changed, 66 insertions(+), 59 deletions(-) diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index b980e73353..325fb0954a 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -1732,66 +1732,10 @@ loanMakePayment( view.update(loan); - detail::PaymentComponentsPlus periodic{ - detail::computePaymentComponents( - asset, - loanScale, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - periodicPayment, - periodicRate, - paymentRemainingProxy, - managementFeeRate), - serviceFee}; - XRPL_ASSERT_PARTS( - periodic.trackedPrincipalDelta >= 0, - "ripple::loanMakePayment", - "regular payment valid principal"); - // ------------------------------------------------------------- - // late payment handling - if (paymentType == LoanPaymentType::late) - { - TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)}; - Number const latePaymentFee = loan->at(sfLatePaymentFee); - - if (auto const latePaymentComponents = detail::computeLatePayment( - asset, - view, - principalOutstandingProxy, - *nextDueDateProxy, - periodic, - lateInterestRate, - loanScale, - latePaymentFee, - amount, - managementFeeRate, - j)) - { - return doPayment( - *latePaymentComponents, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - paymentRemainingProxy, - prevPaymentDateProxy, - nextDueDateProxy, - paymentInterval); - } - else if (latePaymentComponents.error()) - { - // error() will be the TER returned if a payment is not made. It - // will only evaluate to true if it's unsuccessful. - return Unexpected(latePaymentComponents.error()); - } - - // LCOV_EXCL_START - UNREACHABLE("ripple::loanMakePayment : invalid late payment result"); - return Unexpected(tecINTERNAL); - // LCOV_EXCL_STOP - } - else if (hasExpired(view, nextDueDateProxy)) + // A late payment not flagged as late overrides all other options. + if (paymentType != LoanPaymentType::late && + hasExpired(view, nextDueDateProxy)) { // If the payment is late, and the late flag was not set, it's not valid JLOG(j.warn()) @@ -1858,6 +1802,69 @@ loanMakePayment( // LCOV_EXCL_STOP } + // ------------------------------------------------------------- + // compute the periodic payment info that will be needed whether the payment + // is late or regular + detail::PaymentComponentsPlus periodic{ + detail::computePaymentComponents( + asset, + loanScale, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + periodicPayment, + periodicRate, + paymentRemainingProxy, + managementFeeRate), + serviceFee}; + XRPL_ASSERT_PARTS( + periodic.trackedPrincipalDelta >= 0, + "ripple::loanMakePayment", + "regular payment valid principal"); + + // ------------------------------------------------------------- + // late payment handling + if (paymentType == LoanPaymentType::late) + { + TenthBips32 const lateInterestRate{loan->at(sfLateInterestRate)}; + Number const latePaymentFee = loan->at(sfLatePaymentFee); + + if (auto const latePaymentComponents = detail::computeLatePayment( + asset, + view, + principalOutstandingProxy, + *nextDueDateProxy, + periodic, + lateInterestRate, + loanScale, + latePaymentFee, + amount, + managementFeeRate, + j)) + { + return doPayment( + *latePaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + paymentRemainingProxy, + prevPaymentDateProxy, + nextDueDateProxy, + paymentInterval); + } + else if (latePaymentComponents.error()) + { + // error() will be the TER returned if a payment is not made. It + // will only evaluate to true if it's unsuccessful. + return Unexpected(latePaymentComponents.error()); + } + + // LCOV_EXCL_START + UNREACHABLE("ripple::loanMakePayment : invalid late payment result"); + return Unexpected(tecINTERNAL); + // LCOV_EXCL_STOP + } + // ------------------------------------------------------------- // regular periodic payment handling From 9b332d88c1197a8720d0610b1a67399a3b097377 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde Date: Tue, 11 Nov 2025 14:07:45 +0000 Subject: [PATCH 06/16] refactor: Retire PayChanRecipientOwnerDir amendment (#5946) Amendments activated for more than 2 years can be retired. This change retires the PayChanRecipientOwnerDir amendment. --- include/xrpl/protocol/detail/features.macro | 4 +- src/test/app/AccountDelete_test.cpp | 107 +--------- src/test/app/PayChan_test.cpp | 215 ++------------------ src/xrpld/app/tx/detail/PayChan.cpp | 4 +- 4 files changed, 28 insertions(+), 302 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 53da9eb38d..e282919311 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -68,7 +68,6 @@ XRPL_FEATURE(TicketBatch, Supported::yes, VoteBehavior::DefaultYe XRPL_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FIX (PayChanRecipientOwnerDir, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) @@ -109,9 +108,10 @@ XRPL_RETIRE(fix1623) XRPL_RETIRE(fix1781) XRPL_RETIRE(fixAmendmentMajorityCalc) XRPL_RETIRE(fixCheckThreading) +XRPL_RETIRE(fixMasterKeyAsRegularKey) XRPL_RETIRE(fixNonFungibleTokensV1_2) XRPL_RETIRE(fixNFTokenRemint) -XRPL_RETIRE(fixMasterKeyAsRegularKey) +XRPL_RETIRE(fixPayChanRecipientOwnerDir) XRPL_RETIRE(fixQualityUpperBound) XRPL_RETIRE(fixReducedOffersV1) XRPL_RETIRE(fixRmSmallIncreasedQOffers) diff --git a/src/test/app/AccountDelete_test.cpp b/src/test/app/AccountDelete_test.cpp index 9328b4e017..9d5d1cd877 100644 --- a/src/test/app/AccountDelete_test.cpp +++ b/src/test/app/AccountDelete_test.cpp @@ -268,12 +268,8 @@ public: testcase("Owned types"); - // We want to test both... - // o Old-style PayChannels without a recipient backlink as well as - // o New-styled PayChannels with the backlink. - // So we start the test using old-style PayChannels. Then we pass - // the amendment to get new-style PayChannels. - Env env{*this, testable_amendments() - fixPayChanRecipientOwnerDir}; + // We want to test PayChannels with the backlink. + Env env{*this, testable_amendments()}; Account const alice("alice"); Account const becky("becky"); Account const gw("gw"); @@ -374,16 +370,14 @@ public: alice, becky, XRP(57), 4s, env.now() + 2s, alice.pk())); env.close(); - // An old-style PayChannel does not add a back link from the - // destination. So with the PayChannel in place becky should be - // able to delete her account, but alice should not. + // With the PayChannel in place becky and alice should not be + // able to delete her account auto const beckyBalance{env.balance(becky)}; env(acctdelete(alice, gw), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); - env(acctdelete(becky, gw), fee(acctDelFee)); - verifyDeliveredAmount(env, beckyBalance - acctDelFee); + env(acctdelete(becky, gw), fee(acctDelFee), ter(tecHAS_OBLIGATIONS)); env.close(); - // Alice cancels her PayChannel which will leave her with only offers + // Alice cancels her PayChannel, which will leave her with only offers // in her directory. // Lambda to close a PayChannel. @@ -401,14 +395,8 @@ public: env(payChanClose(alice, alicePayChanKey, alice.pk())); env.close(); - // Now enable the amendment so PayChannels add a backlink from the - // destination. - env.enableFeature(fixPayChanRecipientOwnerDir); - env.close(); - - // gw creates a PayChannel with alice as the destination. With the - // amendment passed this should prevent alice from deleting her - // account. + // gw creates a PayChannel with alice as the destination, this should + // prevent alice from deleting her account. Keylet const gwPayChanKey{keylet::payChan(gw, alice, env.seq(gw))}; env(payChanCreate(gw, alice, XRP(68), 4s, env.now() + 2s, alice.pk())); @@ -430,84 +418,6 @@ public: env.close(); } - void - testResurrection() - { - // Create an account with an old-style PayChannel. Delete the - // destination of the PayChannel then resurrect the destination. - // The PayChannel should still work. - using namespace jtx; - - testcase("Resurrection"); - - // We need an old-style PayChannel that doesn't provide a backlink - // from the destination. So don't enable the amendment with that fix. - Env env{*this, testable_amendments() - fixPayChanRecipientOwnerDir}; - Account const alice("alice"); - Account const becky("becky"); - - env.fund(XRP(10000), alice, becky); - env.close(); - - // Verify that becky's account root is present. - Keylet const beckyAcctKey{keylet::account(becky.id())}; - BEAST_EXPECT(env.closed()->exists(beckyAcctKey)); - - using namespace std::chrono_literals; - Keylet const payChanKey{keylet::payChan(alice, becky, env.seq(alice))}; - auto const payChanXRP = XRP(37); - - env(payChanCreate( - alice, becky, payChanXRP, 4s, env.now() + 1h, alice.pk())); - env.close(); - BEAST_EXPECT(env.closed()->exists(payChanKey)); - - // Close enough ledgers to be able to delete becky's account. - incLgrSeqForAccDel(env, becky); - - auto const beckyPreDelBalance{env.balance(becky)}; - - auto const acctDelFee{drops(env.current()->fees().increment)}; - env(acctdelete(becky, alice), fee(acctDelFee)); - verifyDeliveredAmount(env, beckyPreDelBalance - acctDelFee); - env.close(); - - // Verify that becky's account root is gone. - BEAST_EXPECT(!env.closed()->exists(beckyAcctKey)); - - // All it takes is a large enough XRP payment to resurrect - // becky's account. Try too small a payment. - env(pay(alice, - becky, - drops(env.current()->fees().accountReserve(0)) - XRP(1)), - ter(tecNO_DST_INSUF_XRP)); - env.close(); - - // Actually resurrect becky's account. - env(pay(alice, becky, XRP(10))); - env.close(); - - // becky's account root should be back. - BEAST_EXPECT(env.closed()->exists(beckyAcctKey)); - BEAST_EXPECT(env.balance(becky) == XRP(10)); - - // becky's resurrected account can be the destination of alice's - // PayChannel. - auto payChanClaim = [&]() { - Json::Value jv; - jv[jss::TransactionType] = jss::PaymentChannelClaim; - jv[jss::Account] = alice.human(); - jv[sfChannel.jsonName] = to_string(payChanKey.key); - jv[sfBalance.jsonName] = - payChanXRP.value().getJson(JsonOptions::none); - return jv; - }; - env(payChanClaim()); - env.close(); - - BEAST_EXPECT(env.balance(becky) == XRP(10) + payChanXRP); - } - void testAmendmentEnable() { @@ -1238,7 +1148,6 @@ public: testBasics(); testDirectories(); testOwnedTypes(); - testResurrection(); testAmendmentEnable(); testTooManyOffers(); testImplicitlyCreatedTrustline(); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 650542f827..1756b8fdbf 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -1820,36 +1820,6 @@ struct PayChan_test : public beast::unit_test::suite return std::distance(ownerDir.begin(), ownerDir.end()); }; - { - // Test without adding the paychan to the recipient's owner - // directory - Env env(*this, features - fixPayChanRecipientOwnerDir); - env.fund(XRP(10000), alice, bob); - env(create(alice, bob, XRP(1000), settleDelay, pk)); - env.close(); - auto const [chan, chanSle] = - channelKeyAndSle(*env.current(), alice, bob); - BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); - if (features[fixIncludeKeyletFields]) - { - BEAST_EXPECT((*chanSle)[sfSequence] == env.seq(alice) - 1); - } - else - { - BEAST_EXPECT(!chanSle->isFieldPresent(sfSequence)); - } - // close the channel - env(claim(bob, chan), txflags(tfClose)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 0); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); - } - { // Test with adding the paychan to the recipient's owner directory Env env{*this, features}; @@ -1874,7 +1844,7 @@ struct PayChan_test : public beast::unit_test::suite { // Test removing paychans created before adding to the recipient's // owner directory - Env env(*this, features - fixPayChanRecipientOwnerDir); + Env env(*this, features); env.fund(XRP(10000), alice, bob); // create the channel before the amendment activates env(create(alice, bob, XRP(1000), settleDelay, pk)); @@ -1883,21 +1853,9 @@ struct PayChan_test : public beast::unit_test::suite channelKeyAndSle(*env.current(), alice, bob); BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); BEAST_EXPECT(ownerDirCount(*env.current(), alice) == 1); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); - env.enableFeature(fixPayChanRecipientOwnerDir); - env.close(); - BEAST_EXPECT( - env.current()->rules().enabled(fixPayChanRecipientOwnerDir)); - // These checks look redundant, but if you don't `close` after the - // `create` these checks will fail. I believe this is due to the - // create running with one set of amendments initially, then with a - // different set with the ledger closes (tho I haven't dug into it) - BEAST_EXPECT(inOwnerDir(*env.current(), alice, chanSle)); - BEAST_EXPECT(!inOwnerDir(*env.current(), bob, chanSle)); - BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 0); + BEAST_EXPECT(inOwnerDir(*env.current(), bob, chanSle)); + BEAST_EXPECT(ownerDirCount(*env.current(), bob) == 1); - // close the channel after the amendment activates env(claim(bob, chan), txflags(tfClose)); BEAST_EXPECT(!channelExists(*env.current(), chan)); BEAST_EXPECT(!inOwnerDir(*env.current(), alice, chanSle)); @@ -1939,12 +1897,8 @@ struct PayChan_test : public beast::unit_test::suite auto const bob = Account("bob"); auto const carol = Account("carol"); - for (bool const withOwnerDirFix : {false, true}) { - auto const amd = withOwnerDirFix - ? features - : features - fixPayChanRecipientOwnerDir; - Env env{*this, amd}; + Env env{*this, features}; env.fund(XRP(10000), alice, bob, carol); env.close(); @@ -1959,11 +1913,7 @@ struct PayChan_test : public beast::unit_test::suite rmAccount(env, alice, carol, tecHAS_OBLIGATIONS); // can only remove bob if the channel isn't in their owner direcotry - rmAccount( - env, - bob, - carol, - withOwnerDirFix ? TER(tecHAS_OBLIGATIONS) : TER(tesSUCCESS)); + rmAccount(env, bob, carol, TER(tecHAS_OBLIGATIONS)); auto const feeDrops = env.current()->fees().base; auto chanBal = channelBalance(*env.current(), chan); @@ -1978,152 +1928,21 @@ struct PayChan_test : public beast::unit_test::suite assert(reqBal <= chanAmt); // claim should fail if the dst was removed - if (withOwnerDirFix) - { - env(claim(alice, chan, reqBal, authAmt)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob + delta); - chanBal = reqBal; - } - else - { - auto const preAlice = env.balance(alice); - env(claim(alice, chan, reqBal, authAmt), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == chanBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - } + env(claim(alice, chan, reqBal, authAmt)); + env.close(); + BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); + BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); + BEAST_EXPECT(env.balance(bob) == preBob + delta); + chanBal = reqBal; // fund should fail if the dst was removed - if (withOwnerDirFix) - { - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000))); - env.close(); - BEAST_EXPECT( - env.balance(alice) == preAlice - XRP(1000) - feeDrops); - BEAST_EXPECT( - channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); - chanAmt = chanAmt + XRP(1000); - } - else - { - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000)), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - } - - { - // Owner closes, will close after settleDelay - env(claim(alice, chan), txflags(tfClose)); - env.close(); - // settle delay hasn't ellapsed. Channels should exist. - BEAST_EXPECT(channelExists(*env.current(), chan)); - auto const closeTime = env.current()->info().parentCloseTime; - auto const minExpiration = closeTime + settleDelay; - env.close(minExpiration); - env(claim(alice, chan), txflags(tfClose)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - } - } - - { - // test resurrected account - Env env{*this, features - fixPayChanRecipientOwnerDir}; - env.fund(XRP(10000), alice, bob, carol); + auto const preAlice = env.balance(alice); + env(fund(alice, chan, XRP(1000))); env.close(); - - // Create a channel from alice to bob - auto const pk = alice.pk(); - auto const settleDelay = 100s; - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), settleDelay, pk)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == XRP(0)); - BEAST_EXPECT(channelAmount(*env.current(), chan) == XRP(1000)); - - // Since `fixPayChanRecipientOwnerDir` is not active, can remove bob - rmAccount(env, bob, carol); - BEAST_EXPECT(!env.closed()->exists(keylet::account(bob.id()))); - - auto const feeDrops = env.current()->fees().base; - auto chanBal = channelBalance(*env.current(), chan); - auto chanAmt = channelAmount(*env.current(), chan); - BEAST_EXPECT(chanBal == XRP(0)); - BEAST_EXPECT(chanAmt == XRP(1000)); - auto preBob = env.balance(bob); - auto const delta = XRP(50); - auto reqBal = chanBal + delta; - auto authAmt = reqBal + XRP(100); - assert(reqBal <= chanAmt); - - { - // claim should fail, since bob doesn't exist - auto const preAlice = env.balance(alice); - env(claim(alice, chan, reqBal, authAmt), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(channelBalance(*env.current(), chan) == chanBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - } - - { - // fund should fail, sincebob doesn't exist - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000)), ter(tecNO_DST)); - env.close(); - BEAST_EXPECT(env.balance(alice) == preAlice - feeDrops); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - } - - // resurrect bob - env(pay(alice, bob, XRP(20))); - env.close(); - BEAST_EXPECT(env.closed()->exists(keylet::account(bob.id()))); - - { - // alice should be able to claim - preBob = env.balance(bob); - reqBal = chanBal + delta; - authAmt = reqBal + XRP(100); - env(claim(alice, chan, reqBal, authAmt)); - BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob + delta); - chanBal = reqBal; - } - - { - // bob should be able to claim - preBob = env.balance(bob); - reqBal = chanBal + delta; - authAmt = reqBal + XRP(100); - auto const sig = - signClaimAuth(alice.pk(), alice.sk(), chan, authAmt); - env(claim(bob, chan, reqBal, authAmt, Slice(sig), alice.pk())); - BEAST_EXPECT(channelBalance(*env.current(), chan) == reqBal); - BEAST_EXPECT(channelAmount(*env.current(), chan) == chanAmt); - BEAST_EXPECT(env.balance(bob) == preBob + delta - feeDrops); - chanBal = reqBal; - } - - { - // alice should be able to fund - auto const preAlice = env.balance(alice); - env(fund(alice, chan, XRP(1000))); - BEAST_EXPECT( - env.balance(alice) == preAlice - XRP(1000) - feeDrops); - BEAST_EXPECT( - channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); - chanAmt = chanAmt + XRP(1000); - } + BEAST_EXPECT(env.balance(alice) == preAlice - XRP(1000) - feeDrops); + BEAST_EXPECT( + channelAmount(*env.current(), chan) == chanAmt + XRP(1000)); + chanAmt = chanAmt + XRP(1000); { // Owner closes, will close after settleDelay diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index 27b8380f5f..a6d9996b89 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -116,8 +116,7 @@ closeChannel( } // Remove PayChan from recipient's owner directory, if present. - if (auto const page = (*slep)[~sfDestinationNode]; - page && view.rules().enabled(fixPayChanRecipientOwnerDir)) + if (auto const page = (*slep)[~sfDestinationNode]) { auto const dst = (*slep)[sfDestination]; if (!view.dirRemove(keylet::ownerDir(dst), *page, key, true)) @@ -284,7 +283,6 @@ PayChanCreate::doApply() } // Add PayChan to the recipient's owner directory - if (ctx_.view().rules().enabled(fixPayChanRecipientOwnerDir)) { auto const page = ctx_.view().dirInsert( keylet::ownerDir(dst), payChanKeylet, describeOwnerDir(dst)); From 03704f712b12848d5711881d675b15cb61b6514e Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 11 Nov 2025 14:55:16 +0000 Subject: [PATCH 07/16] chore: Move running of unit tests out of coverage target (#6018) This change makes the progress of unit tests visible and also gives more flexibility when running them. --- .../workflows/reusable-build-test-config.yml | 36 +++++++++---------- BUILD.md | 19 ++++------ cmake/CodeCoverage.cmake | 30 ++++++++++------ cmake/XrplCov.cmake | 9 ++--- cmake/XrplSettings.cmake | 7 ---- src/tests/libxrpl/CMakeLists.txt | 9 +++++ 6 files changed, 59 insertions(+), 51 deletions(-) diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 4a8bc71321..79d21a870a 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -172,6 +172,24 @@ jobs: -C "${BUILD_TYPE}" \ -j "${PARALLELISM}" + - name: Run the embedded tests + if: ${{ !inputs.build_only }} + working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', inputs.build_dir, inputs.build_type) || inputs.build_dir }} + shell: bash + env: + BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} + run: | + ./rippled --unittest --unittest-jobs "${BUILD_NPROC}" + + - name: Debug failure (Linux) + if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }} + shell: bash + run: | + echo "IPv4 local port range:" + cat /proc/sys/net/ipv4/ip_local_port_range + echo "Netstat:" + netstat -an + - name: Prepare coverage report if: ${{ !inputs.build_only && env.ENABLED_COVERAGE == 'true' }} working-directory: ${{ inputs.build_dir }} @@ -197,21 +215,3 @@ jobs: plugins: noop token: ${{ secrets.CODECOV_TOKEN }} verbose: true - - - name: Run the embedded tests - if: ${{ !inputs.build_only && env.ENABLED_COVERAGE != 'true' }} - working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', inputs.build_dir, inputs.build_type) || inputs.build_dir }} - shell: bash - env: - BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} - run: | - ./rippled --unittest --unittest-jobs "${BUILD_NPROC}" - - - name: Debug failure (Linux) - if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }} - shell: bash - run: | - echo "IPv4 local port range:" - cat /proc/sys/net/ipv4/ip_local_port_range - echo "Netstat:" - netstat -an diff --git a/BUILD.md b/BUILD.md index f13204ea4c..39570edbd3 100644 --- a/BUILD.md +++ b/BUILD.md @@ -495,18 +495,18 @@ A coverage report is created when the following steps are completed, in order: 1. `rippled` binary built with instrumentation data, enabled by the `coverage` option mentioned above -2. completed run of unit tests, which populates coverage capture data +2. completed one or more run of the unit tests, which populates coverage capture data 3. completed run of the `gcovr` tool (which internally invokes either `gcov` or `llvm-cov`) to assemble both instrumentation data and the coverage capture data into a coverage report -The above steps are automated into a single target `coverage`. The instrumented +The last step of the above is automated into a single target `coverage`. The instrumented `rippled` binary can also be used for regular development or testing work, at the cost of extra disk space utilization and a small performance hit -(to store coverage capture). In case of a spurious failure of unit tests, it is -possible to re-run the `coverage` target without rebuilding the `rippled` binary -(since it is simply a dependency of the coverage report target). It is also possible -to select only specific tests for the purpose of the coverage report, by setting -the `coverage_test` variable in `cmake` +(to store coverage capture data). Since `rippled` binary is simply a dependency of the +coverage report target, it is possible to re-run the `coverage` target without +rebuilding the `rippled` binary. Note, running of the unit tests before the `coverage` +target is left to the developer. Each such run will append to the coverage data +collected in the build directory. The default coverage report format is `html-details`, but the user can override it to any of the formats listed in `Builds/CMake/CodeCoverage.cmake` @@ -515,11 +515,6 @@ to generate more than one format at a time by setting the `coverage_extra_args` variable in `cmake`. The specific command line used to run the `gcovr` tool will be displayed if the `CODE_COVERAGE_VERBOSE` variable is set. -By default, the code coverage tool runs parallel unit tests with `--unittest-jobs` -set to the number of available CPU cores. This may cause spurious test -errors on Apple. Developers can override the number of unit test jobs with -the `coverage_test_parallelism` variable in `cmake`. - Example use with some cmake variables set: ``` diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake index c2b66c9cac..e1b44e656d 100644 --- a/cmake/CodeCoverage.cmake +++ b/cmake/CodeCoverage.cmake @@ -109,6 +109,9 @@ # - add a new function add_code_coverage_to_target # - remove some unused code # +# 2025-11-11, Bronek Kozicki +# - make EXECUTABLE and EXECUTABLE_ARGS optional +# # USAGE: # # 1. Copy this file into your cmake modules path. @@ -317,6 +320,10 @@ function(setup_target_for_coverage_gcovr) set(Coverage_FORMAT xml) endif() + if(NOT DEFINED Coverage_EXECUTABLE AND DEFINED Coverage_EXECUTABLE_ARGS) + message(FATAL_ERROR "EXECUTABLE_ARGS must not be set if EXECUTABLE is not set") + endif() + if("--output" IN_LIST GCOVR_ADDITIONAL_ARGS) message(FATAL_ERROR "Unsupported --output option detected in GCOVR_ADDITIONAL_ARGS! Aborting...") else() @@ -398,17 +405,18 @@ function(setup_target_for_coverage_gcovr) endforeach() # Set up commands which will be run to generate coverage data - # Run tests - set(GCOVR_EXEC_TESTS_CMD - ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} - ) + # If EXECUTABLE is not set, the user is expected to run the tests manually + # before running the coverage target NAME + if(DEFINED Coverage_EXECUTABLE) + set(GCOVR_EXEC_TESTS_CMD + ${Coverage_EXECUTABLE} ${Coverage_EXECUTABLE_ARGS} + ) + endif() # Create folder if(DEFINED GCOVR_CREATE_FOLDER) set(GCOVR_FOLDER_CMD ${CMAKE_COMMAND} -E make_directory ${GCOVR_CREATE_FOLDER}) - else() - set(GCOVR_FOLDER_CMD echo) # dummy endif() # Running gcovr @@ -425,11 +433,13 @@ function(setup_target_for_coverage_gcovr) if(CODE_COVERAGE_VERBOSE) message(STATUS "Executed command report") - message(STATUS "Command to run tests: ") - string(REPLACE ";" " " GCOVR_EXEC_TESTS_CMD_SPACED "${GCOVR_EXEC_TESTS_CMD}") - message(STATUS "${GCOVR_EXEC_TESTS_CMD_SPACED}") + if(NOT "${GCOVR_EXEC_TESTS_CMD}" STREQUAL "") + message(STATUS "Command to run tests: ") + string(REPLACE ";" " " GCOVR_EXEC_TESTS_CMD_SPACED "${GCOVR_EXEC_TESTS_CMD}") + message(STATUS "${GCOVR_EXEC_TESTS_CMD_SPACED}") + endif() - if(NOT GCOVR_FOLDER_CMD STREQUAL "echo") + if(NOT "${GCOVR_FOLDER_CMD}" STREQUAL "") message(STATUS "Command to create a folder: ") string(REPLACE ";" " " GCOVR_FOLDER_CMD_SPACED "${GCOVR_FOLDER_CMD}") message(STATUS "${GCOVR_FOLDER_CMD_SPACED}") diff --git a/cmake/XrplCov.cmake b/cmake/XrplCov.cmake index 288f74c0ef..b212d60b64 100644 --- a/cmake/XrplCov.cmake +++ b/cmake/XrplCov.cmake @@ -11,6 +11,9 @@ if(CMAKE_CXX_COMPILER_ID MATCHES "MSVC") return() endif() +include(ProcessorCount) +ProcessorCount(PROCESSOR_COUNT) + include(CodeCoverage) # The instructions for these commands come from the `CodeCoverage` module, @@ -26,15 +29,13 @@ list(APPEND GCOVR_ADDITIONAL_ARGS --exclude-throw-branches --exclude-noncode-lines --exclude-unreachable-branches -s - -j ${coverage_test_parallelism}) + -j ${PROCESSOR_COUNT}) setup_target_for_coverage_gcovr( NAME coverage FORMAT ${coverage_format} - EXECUTABLE xrpld - EXECUTABLE_ARGS --unittest$<$:=${coverage_test}> --unittest-jobs ${coverage_test_parallelism} --quiet --unittest-log EXCLUDE "src/test" "src/tests" "include/xrpl/beast/test" "include/xrpl/beast/unit_test" "${CMAKE_BINARY_DIR}/pb-xrpl.libpb" - DEPENDENCIES xrpld + DEPENDENCIES xrpld xrpl.tests ) add_code_coverage_to_target(opts INTERFACE) diff --git a/cmake/XrplSettings.cmake b/cmake/XrplSettings.cmake index be339c135e..a16513afc5 100644 --- a/cmake/XrplSettings.cmake +++ b/cmake/XrplSettings.cmake @@ -51,17 +51,10 @@ if(is_gcc OR is_clang) option(coverage "Generates coverage info." OFF) option(profile "Add profiling flags" OFF) - set(coverage_test_parallelism "${PROCESSOR_COUNT}" CACHE STRING - "Unit tests parallelism for the purpose of coverage report.") set(coverage_format "html-details" CACHE STRING "Output format of the coverage report.") set(coverage_extra_args "" CACHE STRING "Additional arguments to pass to gcovr.") - set(coverage_test "" CACHE STRING - "On gcc & clang, the specific unit test(s) to run for coverage. Default is all tests.") - if(coverage_test AND NOT coverage) - set(coverage ON CACHE BOOL "gcc/clang only" FORCE) - endif() option(wextra "compile with extra gcc/clang warnings enabled" ON) else() set(profile OFF CACHE BOOL "gcc/clang only" FORCE) diff --git a/src/tests/libxrpl/CMakeLists.txt b/src/tests/libxrpl/CMakeLists.txt index 880fdb2948..a2374698d9 100644 --- a/src/tests/libxrpl/CMakeLists.txt +++ b/src/tests/libxrpl/CMakeLists.txt @@ -3,6 +3,9 @@ include(XrplAddTest) # Test requirements. find_package(doctest REQUIRED) +# Custom target for all tests defined in this file +add_custom_target(xrpl.tests) + # Common library dependencies for the rest of the tests. add_library(xrpl.imports.test INTERFACE) target_link_libraries(xrpl.imports.test INTERFACE doctest::doctest xrpl.libxrpl) @@ -10,13 +13,19 @@ target_link_libraries(xrpl.imports.test INTERFACE doctest::doctest xrpl.libxrpl) # One test for each module. xrpl_add_test(basics) target_link_libraries(xrpl.test.basics PRIVATE xrpl.imports.test) +add_dependencies(xrpl.tests xrpl.test.basics) + xrpl_add_test(crypto) target_link_libraries(xrpl.test.crypto PRIVATE xrpl.imports.test) +add_dependencies(xrpl.tests xrpl.test.crypto) + xrpl_add_test(json) target_link_libraries(xrpl.test.json PRIVATE xrpl.imports.test) +add_dependencies(xrpl.tests xrpl.test.json) # Network unit tests are currently not supported on Windows if(NOT WIN32) xrpl_add_test(net) target_link_libraries(xrpl.test.net PRIVATE xrpl.imports.test) + add_dependencies(xrpl.tests xrpl.test.net) endif() From ff18cfef9626aa1cd7f70b3f6d8e603ff4e2360e Mon Sep 17 00:00:00 2001 From: Jingchen Date: Tue, 11 Nov 2025 15:21:07 +0000 Subject: [PATCH 08/16] refactor: Retire DepositPreAuth and DepositAuth amendments (#5978) Amendments activated for more than 2 years can be retired. This change retires the fDepositPreAuth and DepositAuth amendments. --- include/xrpl/protocol/detail/features.macro | 4 +- .../xrpl/protocol/detail/transactions.macro | 2 +- src/test/app/AMMExtended_test.cpp | 14 +- src/test/app/Delegate_test.cpp | 1 - src/test/app/DepositAuth_test.cpp | 242 ++++++------------ src/test/app/Escrow_test.cpp | 10 - src/test/app/Offer_test.cpp | 23 +- src/test/app/PayChan_test.cpp | 23 -- src/test/app/TxQ_test.cpp | 22 +- src/test/rpc/AccountSet_test.cpp | 16 +- src/test/rpc/Feature_test.cpp | 3 +- src/xrpld/app/tx/detail/CreateOffer.cpp | 21 +- src/xrpld/app/tx/detail/DeleteAccount.cpp | 6 +- src/xrpld/app/tx/detail/Escrow.cpp | 17 +- src/xrpld/app/tx/detail/PayChan.cpp | 24 +- src/xrpld/app/tx/detail/Payment.cpp | 110 ++++---- src/xrpld/app/tx/detail/SetAccount.cpp | 19 +- 17 files changed, 168 insertions(+), 389 deletions(-) diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index e282919311..076307ff36 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -69,9 +69,7 @@ XRPL_FEATURE(NegativeUNL, Supported::yes, VoteBehavior::DefaultYe XRPL_FEATURE(RequireFullyCanonicalSig, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(DeletableAccounts, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(MultiSignReserve, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DepositPreauth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Checks, Supported::yes, VoteBehavior::DefaultYes) -XRPL_FEATURE(DepositAuth, Supported::yes, VoteBehavior::DefaultYes) XRPL_FEATURE(Flow, Supported::yes, VoteBehavior::DefaultYes) // The following amendments are obsolete, but must remain supported @@ -120,6 +118,8 @@ XRPL_RETIRE(fixTakerDryOfferRemoval) XRPL_RETIRE(fixTrustLinesToSelf) XRPL_RETIRE(CheckCashMakesTrustLine) XRPL_RETIRE(CryptoConditions) +XRPL_RETIRE(DepositAuth) +XRPL_RETIRE(DepositPreauth) XRPL_RETIRE(Escrow) XRPL_RETIRE(EnforceInvariants) XRPL_RETIRE(FeeEscalation) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 1ddc5831b4..fd651fb94e 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -268,7 +268,7 @@ TRANSACTION(ttCHECK_CANCEL, 18, CheckCancel, #endif TRANSACTION(ttDEPOSIT_PREAUTH, 19, DepositPreauth, Delegation::delegatable, - featureDepositPreauth, + uint256{}, noPriv, ({ {sfAuthorize, soeOPTIONAL}, diff --git a/src/test/app/AMMExtended_test.cpp b/src/test/app/AMMExtended_test.cpp index 2cfd690183..6b09ea0ce5 100644 --- a/src/test/app/AMMExtended_test.cpp +++ b/src/test/app/AMMExtended_test.cpp @@ -3039,8 +3039,6 @@ private: using namespace jtx; Account const becky{"becky"}; - bool const supportsPreauth = {features[featureDepositPreauth]}; - // The initial implementation of DepositAuth had a bug where an // account with the DepositAuth flag set could not make a payment // to itself. That bug was fixed in the DepositPreauth amendment. @@ -3068,15 +3066,11 @@ private: env(fset(becky, asfDepositAuth)); env.close(); - // becky pays herself again. Whether it succeeds depends on - // whether featureDepositPreauth is enabled. - TER const expect{ - supportsPreauth ? TER{tesSUCCESS} : TER{tecNO_PERMISSION}}; - + // becky pays herself again. env(pay(becky, becky, USD(10)), path(~USD), sendmax(XRP(10)), - ter(expect)); + ter(tesSUCCESS)); env.close(); } @@ -3784,9 +3778,7 @@ private: void testDepositAuth() { - auto const supported{jtx::testable_amendments()}; - testPayment(supported - featureDepositPreauth); - testPayment(supported); + testPayment(jtx::testable_amendments()); testPayIOU(); } diff --git a/src/test/app/Delegate_test.cpp b/src/test/app/Delegate_test.cpp index bec720a556..c0fd4b1f3b 100644 --- a/src/test/app/Delegate_test.cpp +++ b/src/test/app/Delegate_test.cpp @@ -1707,7 +1707,6 @@ class Delegate_test : public beast::unit_test::suite {"CheckCreate", featureChecks}, {"CheckCash", featureChecks}, {"CheckCancel", featureChecks}, - {"DepositPreauth", featureDepositPreauth}, {"Clawback", featureClawback}, {"AMMClawback", featureAMMClawback}, {"AMMCreate", featureAMM}, diff --git a/src/test/app/DepositAuth_test.cpp b/src/test/app/DepositAuth_test.cpp index 8d12a31088..54d5dd6254 100644 --- a/src/test/app/DepositAuth_test.cpp +++ b/src/test/app/DepositAuth_test.cpp @@ -33,21 +33,6 @@ struct DepositAuth_test : public beast::unit_test::suite Account const alice{"alice"}; { - // featureDepositAuth is disabled. - Env env(*this, testable_amendments() - featureDepositAuth); - env.fund(XRP(10000), alice); - - // Note that, to support old behavior, invalid flags are ignored. - env(fset(alice, asfDepositAuth)); - env.close(); - BEAST_EXPECT(!hasDepositAuth(env, alice)); - - env(fclear(alice, asfDepositAuth)); - env.close(); - BEAST_EXPECT(!hasDepositAuth(env, alice)); - } - { - // featureDepositAuth is enabled. Env env(*this); env.fund(XRP(10000), alice); @@ -281,8 +266,6 @@ struct DepositAuth_test : public beast::unit_test::suite bool noRipplePrev, bool noRippleNext, bool withDepositAuth) { - assert(!withDepositAuth || features[featureDepositAuth]); - Env env(*this, features); env.fund(XRP(10000), gw1, alice, bob); @@ -305,8 +288,6 @@ struct DepositAuth_test : public beast::unit_test::suite bool noRipplePrev, bool noRippleNext, bool withDepositAuth) { - assert(!withDepositAuth || features[featureDepositAuth]); - Env env(*this, features); env.fund(XRP(10000), gw1, gw2, alice); @@ -333,30 +314,16 @@ struct DepositAuth_test : public beast::unit_test::suite auto const noRippleNext = i & 0x2; auto const withDepositAuth = i & 0x4; testIssuer( - testable_amendments() | featureDepositAuth, + testable_amendments(), noRipplePrev, noRippleNext, withDepositAuth); - if (!withDepositAuth) - testIssuer( - testable_amendments() - featureDepositAuth, - noRipplePrev, - noRippleNext, - withDepositAuth); - testNonIssuer( - testable_amendments() | featureDepositAuth, + testable_amendments(), noRipplePrev, noRippleNext, withDepositAuth); - - if (!withDepositAuth) - testNonIssuer( - testable_amendments() - featureDepositAuth, - noRipplePrev, - noRippleNext, - withDepositAuth); } } @@ -400,26 +367,6 @@ struct DepositPreauth_test : public beast::unit_test::suite Account const alice{"alice"}; Account const becky{"becky"}; { - // featureDepositPreauth is disabled. - Env env(*this, testable_amendments() - featureDepositPreauth); - env.fund(XRP(10000), alice, becky); - env.close(); - - // Should not be able to add a DepositPreauth to alice. - env(deposit::auth(alice, becky), ter(temDISABLED)); - env.close(); - env.require(owners(alice, 0)); - env.require(owners(becky, 0)); - - // Should not be able to remove a DepositPreauth from alice. - env(deposit::unauth(alice, becky), ter(temDISABLED)); - env.close(); - env.require(owners(alice, 0)); - env.require(owners(becky, 0)); - } - { - // featureDepositPreauth is enabled. The valid case is really - // simple: // o We should be able to add and remove an entry, and // o That entry should cost one reserve. // o The reserve should be returned when the entry is removed. @@ -602,8 +549,6 @@ struct DepositPreauth_test : public beast::unit_test::suite Account const gw{"gw"}; IOU const USD(gw["USD"]); - bool const supportsPreauth = {features[featureDepositPreauth]}; - { // The initial implementation of DepositAuth had a bug where an // account with the DepositAuth flag set could not make a payment @@ -632,15 +577,11 @@ struct DepositPreauth_test : public beast::unit_test::suite env(fset(becky, asfDepositAuth)); env.close(); - // becky pays herself again. Whether it succeeds depends on - // whether featureDepositPreauth is enabled. - TER const expect{ - supportsPreauth ? TER{tesSUCCESS} : TER{tecNO_PERMISSION}}; - + // becky pays herself again. env(pay(becky, becky, USD(10)), path(~USD), sendmax(XRP(10)), - ter(expect)); + ter(tesSUCCESS)); env.close(); { @@ -652,29 +593,17 @@ struct DepositPreauth_test : public beast::unit_test::suite bool const supportsCredentials = features[featureCredentials]; - TER const expectCredentials( - supportsCredentials ? TER(tesSUCCESS) : TER(temDISABLED)); - TER const expectPayment( - !supportsCredentials - ? TER(temDISABLED) - : (!supportsPreauth ? TER(tecNO_PERMISSION) - : TER(tesSUCCESS))); - TER const expectDP( - !supportsPreauth - ? TER(temDISABLED) - : (!supportsCredentials ? TER(temDISABLED) - : TER(tesSUCCESS))); + TER const expectTer( + !supportsCredentials ? TER(temDISABLED) : TER(tesSUCCESS)); env(deposit::authCredentials(becky, {{carol, credType}}), - ter(expectDP)); + ter(expectTer)); env.close(); // gw accept credentials - env(credentials::create(gw, carol, credType), - ter(expectCredentials)); + env(credentials::create(gw, carol, credType), ter(expectTer)); env.close(); - env(credentials::accept(gw, carol, credType), - ter(expectCredentials)); + env(credentials::accept(gw, carol, credType), ter(expectTer)); env.close(); auto jv = credentials::ledgerEntry(env, gw, carol, credType); @@ -685,115 +614,94 @@ struct DepositPreauth_test : public beast::unit_test::suite env(pay(gw, becky, USD(100)), credentials::ids({credIdx}), - ter(expectPayment)); + ter(expectTer)); env.close(); } - - { - using namespace std::chrono; - - if (!supportsPreauth) - { - auto const seq1 = env.seq(alice); - env(escrow::create(alice, becky, XRP(100)), - escrow::finish_time(env.now() + 1s)); - env.close(); - - // Failed as rule is disabled - env(escrow::finish(gw, alice, seq1), - fee(1500), - ter(tecNO_PERMISSION)); - env.close(); - } - } } - if (supportsPreauth) - { - // Make sure DepositPreauthorization works for payments. + // Make sure DepositPreauthorization works for payments. - Account const carol{"carol"}; + Account const carol{"carol"}; - Env env(*this, features); - env.fund(XRP(5000), alice, becky, carol, gw); - env.close(); + Env env(*this, features); + env.fund(XRP(5000), alice, becky, carol, gw); + env.close(); - env.trust(USD(1000), alice); - env.trust(USD(1000), becky); - env.trust(USD(1000), carol); - env.close(); + env.trust(USD(1000), alice); + env.trust(USD(1000), becky); + env.trust(USD(1000), carol); + env.close(); - env(pay(gw, alice, USD(1000))); - env.close(); + env(pay(gw, alice, USD(1000))); + env.close(); - // Make XRP and IOU payments from alice to becky. Should be fine. - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + // Make XRP and IOU payments from alice to becky. Should be fine. + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // becky decides to require authorization for deposits. - env(fset(becky, asfDepositAuth)); - env.close(); + // becky decides to require authorization for deposits. + env(fset(becky, asfDepositAuth)); + env.close(); - // alice can no longer pay becky. - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // alice can no longer pay becky. + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky preauthorizes carol for deposit, which doesn't provide - // authorization for alice. - env(deposit::auth(becky, carol)); - env.close(); + // becky preauthorizes carol for deposit, which doesn't provide + // authorization for alice. + env(deposit::auth(becky, carol)); + env.close(); - // alice still can't pay becky. - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // alice still can't pay becky. + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky preauthorizes alice for deposit. - env(deposit::auth(becky, alice)); - env.close(); + // becky preauthorizes alice for deposit. + env(deposit::auth(becky, alice)); + env.close(); - // alice can now pay becky. - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + // alice can now pay becky. + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // alice decides to require authorization for deposits. - env(fset(alice, asfDepositAuth)); - env.close(); + // alice decides to require authorization for deposits. + env(fset(alice, asfDepositAuth)); + env.close(); - // Even though alice is authorized to pay becky, becky is not - // authorized to pay alice. - env(pay(becky, alice, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(becky, alice, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + // Even though alice is authorized to pay becky, becky is not + // authorized to pay alice. + env(pay(becky, alice, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(becky, alice, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky unauthorizes carol. Should have no impact on alice. - env(deposit::unauth(becky, carol)); - env.close(); + // becky unauthorizes carol. Should have no impact on alice. + env(deposit::unauth(becky, carol)); + env.close(); - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); - // becky unauthorizes alice. alice now can't pay becky. - env(deposit::unauth(becky, alice)); - env.close(); + // becky unauthorizes alice. alice now can't pay becky. + env(deposit::unauth(becky, alice)); + env.close(); - env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); - env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); - env.close(); + env(pay(alice, becky, XRP(100)), ter(tecNO_PERMISSION)); + env(pay(alice, becky, USD(100)), ter(tecNO_PERMISSION)); + env.close(); - // becky decides to remove authorization for deposits. Now - // alice can pay becky again. - env(fclear(becky, asfDepositAuth)); - env.close(); + // becky decides to remove authorization for deposits. Now + // alice can pay becky again. + env(fclear(becky, asfDepositAuth)); + env.close(); - env(pay(alice, becky, XRP(100))); - env(pay(alice, becky, USD(100))); - env.close(); - } + env(pay(alice, becky, XRP(100))); + env(pay(alice, becky, USD(100))); + env.close(); } void @@ -1545,8 +1453,6 @@ struct DepositPreauth_test : public beast::unit_test::suite testEnable(); testInvalid(); auto const supported{jtx::testable_amendments()}; - testPayment(supported - featureDepositPreauth - featureCredentials); - testPayment(supported - featureDepositPreauth); testPayment(supported - featureCredentials); testPayment(supported); testCredentialsPayment(); diff --git a/src/test/app/Escrow_test.cpp b/src/test/app/Escrow_test.cpp index 22e81ccca4..e3b2340022 100644 --- a/src/test/app/Escrow_test.cpp +++ b/src/test/app/Escrow_test.cpp @@ -252,16 +252,6 @@ struct Escrow_test : public beast::unit_test::suite using namespace jtx; using namespace std::chrono; - { - // Respect the "asfDisallowXRP" account flag: - Env env(*this, features - featureDepositAuth); - - env.fund(XRP(5000), "bob", "george"); - env(fset("george", asfDisallowXRP)); - env(escrow::create("bob", "george", XRP(10)), - escrow::finish_time(env.now() + 1s), - ter(tecNO_TARGET)); - } { // Ignore the "asfDisallowXRP" account flag, which we should // have been doing before. diff --git a/src/test/app/Offer_test.cpp b/src/test/app/Offer_test.cpp index aff617fdba..2cbf2598e1 100644 --- a/src/test/app/Offer_test.cpp +++ b/src/test/app/Offer_test.cpp @@ -180,13 +180,10 @@ public: // an offer. Show that the attempt to remove the offer fails. env.require(offers(alice, 2)); - // featureDepositPreauths changes the return code on an expired Offer. - // Adapt to that. - bool const featPreauth{features[featureDepositPreauth]}; env(offer(alice, XRP(5), USD(2)), json(sfExpiration.fieldName, lastClose(env)), json(jss::OfferSequence, offer2Seq), - ter(featPreauth ? TER{tecEXPIRED} : TER{tesSUCCESS})); + ter(tecEXPIRED)); env.close(); env.require(offers(alice, 2)); @@ -1082,13 +1079,9 @@ public: offers(alice, 0), owners(alice, 1)); - // Place an offer that should have already expired. - // The DepositPreauth amendment changes the return code; adapt to that. - bool const featPreauth{features[featureDepositPreauth]}; - env(offer(alice, xrpOffer, usdOffer), json(sfExpiration.fieldName, lastClose(env)), - ter(featPreauth ? TER{tecEXPIRED} : TER{tesSUCCESS})); + ter(tecEXPIRED)); env.require( balance(alice, startBalance - f - f), @@ -4499,21 +4492,13 @@ public: env(fset(gw, asfRequireAuth)); env.close(); - // The test behaves differently with or without DepositPreauth. - bool const preauth = features[featureDepositPreauth]; - // Before DepositPreauth an account with lsfRequireAuth set could not // create an offer to buy their own currency. After DepositPreauth // they can. - env(offer(gw, gwUSD(40), XRP(4000)), - ter(preauth ? TER{tesSUCCESS} : TER{tecNO_LINE})); + env(offer(gw, gwUSD(40), XRP(4000)), ter(tesSUCCESS)); env.close(); - env.require(offers(gw, preauth ? 1 : 0)); - - if (!preauth) - // The rest of the test verifies DepositPreauth behavior. - return; + env.require(offers(gw, 1)); // Set up an authorized trust line and pay alice gwUSD 50. env(trust(gw, aliceUSD(100)), txflags(tfSetfAuth)); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 1756b8fdbf..893d71bfa8 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -656,16 +656,6 @@ struct PayChan_test : public beast::unit_test::suite auto const alice = Account("alice"); auto const bob = Account("bob"); - { - // Create a channel where dst disallows XRP - Env env(*this, features - featureDepositAuth); - env.fund(XRP(10000), alice, bob); - env(fset(bob, asfDisallowXRP)); - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), 3600s, alice.pk()), - ter(tecNO_TARGET)); - BEAST_EXPECT(!channelExists(*env.current(), chan)); - } { // Create a channel where dst disallows XRP. Ignore that flag, // since it's just advisory. @@ -677,19 +667,6 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(channelExists(*env.current(), chan)); } - { - // Claim to a channel where dst disallows XRP - // (channel is created before disallow xrp is set) - Env env(*this, features - featureDepositAuth); - env.fund(XRP(10000), alice, bob); - auto const chan = channel(alice, bob, env.seq(alice)); - env(create(alice, bob, XRP(1000), 3600s, alice.pk())); - BEAST_EXPECT(channelExists(*env.current(), chan)); - - env(fset(bob, asfDisallowXRP)); - auto const reqBal = XRP(500).value(); - env(claim(alice, chan, reqBal, reqBal), ter(tecNO_TARGET)); - } { // Claim to a channel where dst disallows XRP (channel is // created before disallow xrp is set). Ignore that flag diff --git a/src/test/app/TxQ_test.cpp b/src/test/app/TxQ_test.cpp index d176744c5c..4b7f156738 100644 --- a/src/test/app/TxQ_test.cpp +++ b/src/test/app/TxQ_test.cpp @@ -4330,9 +4330,10 @@ public: Account const ellie("ellie"); Account const fiona("fiona"); + constexpr int ledgersInQueue = 10; auto cfg = makeConfig( {{"minimum_txn_in_ledger_standalone", "1"}, - {"ledgers_in_queue", "5"}, + {"ledgers_in_queue", std::to_string(ledgersInQueue)}, {"maximum_txn_per_account", "10"}}, {{"account_reserve", "1000"}, {"owner_reserve", "50"}}); @@ -4358,7 +4359,9 @@ public: env.close(); env.fund(XRP(10000), fiona); env.close(); - checkMetrics(*this, env, 0, 10, 0, 2); + + auto const metrics = env.app().getTxQ().getMetrics(*env.current()); + checkMetrics(*this, env, 0, ledgersInQueue * metrics.txPerLedger, 0, 2); // Close ledgers until the amendments show up. int i = 0; @@ -4370,7 +4373,12 @@ public: } auto expectedPerLedger = ripple::detail::numUpVotedAmendments() + 1; checkMetrics( - *this, env, 0, 5 * expectedPerLedger, 0, expectedPerLedger); + *this, + env, + 0, + ledgersInQueue * expectedPerLedger, + 0, + expectedPerLedger); // Now wait 2 weeks modulo 256 ledgers for the amendments to be // enabled. Speed the process by closing ledgers every 80 minutes, @@ -4389,7 +4397,7 @@ public: *this, env, 0, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedPerLedger + 1, expectedPerLedger); @@ -4435,12 +4443,12 @@ public: prepareFee(++multiplier), ter(terQUEUED)); } - std::size_t expectedInQueue = 60; + std::size_t expectedInQueue = multiplier; checkMetrics( *this, env, expectedInQueue, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedPerLedger + 1, expectedPerLedger); @@ -4467,7 +4475,7 @@ public: *this, env, expectedInQueue, - 5 * expectedPerLedger, + ledgersInQueue * expectedPerLedger, expectedInLedger, expectedPerLedger); { diff --git a/src/test/rpc/AccountSet_test.cpp b/src/test/rpc/AccountSet_test.cpp index 31fc345862..3df3606a03 100644 --- a/src/test/rpc/AccountSet_test.cpp +++ b/src/test/rpc/AccountSet_test.cpp @@ -35,8 +35,7 @@ public: using namespace test::jtx; Account const alice("alice"); - // Test without DepositAuth enabled initially. - Env env(*this, testable_amendments() - featureDepositAuth); + Env env(*this, testable_amendments()); env.fund(XRP(10000), noripple(alice)); // Give alice a regular key so she can legally set and clear @@ -116,19 +115,6 @@ public: } } }; - - // Test with featureDepositAuth disabled. - testFlags( - {asfRequireDest, - asfRequireAuth, - asfDisallowXRP, - asfGlobalFreeze, - asfDisableMaster, - asfDefaultRipple}); - - // Enable featureDepositAuth and retest. - env.enableFeature(featureDepositAuth); - env.close(); testFlags( {asfRequireDest, asfRequireAuth, diff --git a/src/test/rpc/Feature_test.cpp b/src/test/rpc/Feature_test.cpp index 1a53f39f60..13d5fd3969 100644 --- a/src/test/rpc/Feature_test.cpp +++ b/src/test/rpc/Feature_test.cpp @@ -322,8 +322,7 @@ class Feature_test : public beast::unit_test::suite testcase("No Params, Some Enabled"); using namespace test::jtx; - Env env{ - *this, FeatureBitset(featureDepositAuth, featureDepositPreauth)}; + Env env{*this, FeatureBitset{}}; std::map const& votes = ripple::detail::supportedAmendments(); diff --git a/src/xrpld/app/tx/detail/CreateOffer.cpp b/src/xrpld/app/tx/detail/CreateOffer.cpp index a1179da81b..fe96436f2f 100644 --- a/src/xrpld/app/tx/detail/CreateOffer.cpp +++ b/src/xrpld/app/tx/detail/CreateOffer.cpp @@ -177,13 +177,7 @@ CreateOffer::preclaim(PreclaimContext const& ctx) { // Note that this will get checked again in applyGuts, but it saves // us a call to checkAcceptAsset and possible false negative. - // - // The return code change is attached to featureDepositPreauth as a - // convenience, as the change is not big enough to deserve its own - // amendment. - return ctx.view.rules().enabled(featureDepositPreauth) - ? TER{tecEXPIRED} - : TER{tesSUCCESS}; + return tecEXPIRED; } // Make sure that we are authorized to hold what the taker will pay us. @@ -235,10 +229,7 @@ CreateOffer::checkAcceptAsset( return (flags & tapRETRY) ? TER{terNO_ACCOUNT} : TER{tecNO_ISSUER}; } - // This code is attached to the DepositPreauth amendment as a matter of - // convenience. The change is not significant enough to deserve its - // own amendment. - if (view.rules().enabled(featureDepositPreauth) && (issue.account == id)) + if (issue.account == id) // An account can always accept its own issuance. return tesSUCCESS; @@ -599,13 +590,7 @@ CreateOffer::applyGuts(Sandbox& sb, Sandbox& sbCancel) { // If the offer has expired, the transaction has successfully // done nothing, so short circuit from here. - // - // The return code change is attached to featureDepositPreauth as a - // convenience. The change is not big enough to deserve a fix code. - TER const ter{ - sb.rules().enabled(featureDepositPreauth) ? TER{tecEXPIRED} - : TER{tesSUCCESS}}; - return {ter, true}; + return {tecEXPIRED, true}; } bool const bOpenLedger = sb.open(); diff --git a/src/xrpld/app/tx/detail/DeleteAccount.cpp b/src/xrpld/app/tx/detail/DeleteAccount.cpp index ed72900248..0654c8dbce 100644 --- a/src/xrpld/app/tx/detail/DeleteAccount.cpp +++ b/src/xrpld/app/tx/detail/DeleteAccount.cpp @@ -232,8 +232,7 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) if (!ctx.tx.isFieldPresent(sfCredentialIDs)) { // Check whether the destination account requires deposit authorization. - if (ctx.view.rules().enabled(featureDepositAuth) && - (sleDst->getFlags() & lsfDepositAuth)) + if (sleDst->getFlags() & lsfDepositAuth) { if (!ctx.view.exists(keylet::depositPreauth(dst, account))) return tecNO_PERMISSION; @@ -353,8 +352,7 @@ DeleteAccount::doApply() if (!src || !dst) return tefBAD_LEDGER; // LCOV_EXCL_LINE - if (ctx_.view().rules().enabled(featureDepositAuth) && - ctx_.tx.isFieldPresent(sfCredentialIDs)) + if (ctx_.tx.isFieldPresent(sfCredentialIDs)) { if (auto err = verifyDepositPreauth( ctx_.tx, ctx_.view(), account_, dstID, dst, ctx_.journal); diff --git a/src/xrpld/app/tx/detail/Escrow.cpp b/src/xrpld/app/tx/detail/Escrow.cpp index fb3e3c509e..5cf90809a2 100644 --- a/src/xrpld/app/tx/detail/Escrow.cpp +++ b/src/xrpld/app/tx/detail/Escrow.cpp @@ -459,12 +459,6 @@ EscrowCreate::doApply() if (((*sled)[sfFlags] & lsfRequireDestTag) && !ctx_.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - if (!ctx_.view().rules().enabled(featureDepositAuth) && - ((*sled)[sfFlags] & lsfDisallowXRP)) - return tecNO_TARGET; } // Create escrow in ledger. Note that we we use the value from the @@ -1041,13 +1035,10 @@ EscrowFinish::doApply() if (!sled) return tecNO_DST; - if (ctx_.view().rules().enabled(featureDepositAuth)) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, destID, sled, ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), account_, destID, sled, ctx_.journal); + !isTesSuccess(err)) + return err; AccountID const account = (*slep)[sfAccount]; diff --git a/src/xrpld/app/tx/detail/PayChan.cpp b/src/xrpld/app/tx/detail/PayChan.cpp index a6d9996b89..c3dc99e7fb 100644 --- a/src/xrpld/app/tx/detail/PayChan.cpp +++ b/src/xrpld/app/tx/detail/PayChan.cpp @@ -209,12 +209,6 @@ PayChanCreate::preclaim(PreclaimContext const& ctx) if ((flags & lsfRequireDestTag) && !ctx.tx[~sfDestinationTag]) return tecDST_TAG_NEEDED; - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - if (!ctx.view.rules().enabled(featureDepositAuth) && - (flags & lsfDisallowXRP)) - return tecNO_TARGET; - // Pseudo-accounts cannot receive payment channels, other than native // to their underlying ledger object - implemented in their respective // transaction types. Note, this is not amendment-gated because all @@ -525,20 +519,10 @@ PayChanClaim::doApply() if (!sled) return tecNO_DST; - // Obeying the lsfDisallowXRP flag was a bug. Piggyback on - // featureDepositAuth to remove the bug. - bool const depositAuth{ctx_.view().rules().enabled(featureDepositAuth)}; - if (!depositAuth && - (txAccount == src && (sled->getFlags() & lsfDisallowXRP))) - return tecNO_TARGET; - - if (depositAuth) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, ctx_.view(), txAccount, dst, sled, ctx_.journal); + !isTesSuccess(err)) + return err; (*slep)[sfBalance] = ctx_.tx[sfBalance]; XRPAmount const reqDelta = reqBalance - chanBalance; diff --git a/src/xrpld/app/tx/detail/Payment.cpp b/src/xrpld/app/tx/detail/Payment.cpp index 305d54438e..3aff4db5e1 100644 --- a/src/xrpld/app/tx/detail/Payment.cpp +++ b/src/xrpld/app/tx/detail/Payment.cpp @@ -417,43 +417,28 @@ Payment::doApply() view().update(sleDst); } - // Determine whether the destination requires deposit authorization. - bool const depositAuth = view().rules().enabled(featureDepositAuth); - bool const reqDepositAuth = - sleDst->getFlags() & lsfDepositAuth && depositAuth; - - bool const depositPreauth = view().rules().enabled(featureDepositPreauth); - bool const ripple = (hasPaths || sendMax || !dstAmount.native()) && !mptDirect; - // If the destination has lsfDepositAuth set, then only direct XRP - // payments (no intermediate steps) are allowed to the destination. - if (!depositPreauth && ripple && reqDepositAuth) - return tecNO_PERMISSION; - if (ripple) { // Ripple payment with at least one intermediate step and uses // transitive balances. - if (depositPreauth && depositAuth) - { - // If depositPreauth is enabled, then an account that requires - // authorization has two ways to get an IOU Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination. + // An account that requires authorization has two ways to get an + // IOU Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination. - if (auto err = verifyDepositPreauth( - ctx_.tx, - ctx_.view(), - account_, - dstAccountID, - sleDst, - ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; path::RippleCalc::Input rcInput; rcInput.partialPaymentAllowed = partialPaymentAllowed; @@ -630,43 +615,40 @@ Payment::doApply() // The source account does have enough money. Make sure the // source account has authority to deposit to the destination. - if (depositAuth) + // An account that requires authorization has three ways to get an XRP + // Payment in: + // 1. If Account == Destination, or + // 2. If Account is deposit preauthorized by destination, or + // 3. If the destination's XRP balance is + // a. less than or equal to the base reserve and + // b. the deposit amount is less than or equal to the base reserve, + // then we allow the deposit. + // + // Rule 3 is designed to keep an account from getting wedged + // in an unusable state if it sets the lsfDepositAuth flag and + // then consumes all of its XRP. Without the rule if an + // account with lsfDepositAuth set spent all of its XRP, it + // would be unable to acquire more XRP required to pay fees. + // + // We choose the base reserve as our bound because it is + // a small number that seldom changes but is always sufficient + // to get the account un-wedged. + + // Get the base reserve. + XRPAmount const dstReserve{view().fees().reserve}; + + if (dstAmount > dstReserve || + sleDst->getFieldAmount(sfBalance) > dstReserve) { - // If depositPreauth is enabled, then an account that requires - // authorization has three ways to get an XRP Payment in: - // 1. If Account == Destination, or - // 2. If Account is deposit preauthorized by destination, or - // 3. If the destination's XRP balance is - // a. less than or equal to the base reserve and - // b. the deposit amount is less than or equal to the base reserve, - // then we allow the deposit. - // - // Rule 3 is designed to keep an account from getting wedged - // in an unusable state if it sets the lsfDepositAuth flag and - // then consumes all of its XRP. Without the rule if an - // account with lsfDepositAuth set spent all of its XRP, it - // would be unable to acquire more XRP required to pay fees. - // - // We choose the base reserve as our bound because it is - // a small number that seldom changes but is always sufficient - // to get the account un-wedged. - - // Get the base reserve. - XRPAmount const dstReserve{view().fees().reserve}; - - if (dstAmount > dstReserve || - sleDst->getFieldAmount(sfBalance) > dstReserve) - { - if (auto err = verifyDepositPreauth( - ctx_.tx, - ctx_.view(), - account_, - dstAccountID, - sleDst, - ctx_.journal); - !isTesSuccess(err)) - return err; - } + if (auto err = verifyDepositPreauth( + ctx_.tx, + ctx_.view(), + account_, + dstAccountID, + sleDst, + ctx_.journal); + !isTesSuccess(err)) + return err; } // Do the arithmetic for the transfer and make the ledger change. diff --git a/src/xrpld/app/tx/detail/SetAccount.cpp b/src/xrpld/app/tx/detail/SetAccount.cpp index e966dba8f5..db513d9f05 100644 --- a/src/xrpld/app/tx/detail/SetAccount.cpp +++ b/src/xrpld/app/tx/detail/SetAccount.cpp @@ -463,18 +463,15 @@ SetAccount::doApply() // // DepositAuth // - if (view().rules().enabled(featureDepositAuth)) + if (uSetFlag == asfDepositAuth) { - if (uSetFlag == asfDepositAuth) - { - JLOG(j_.trace()) << "Set lsfDepositAuth."; - uFlagsOut |= lsfDepositAuth; - } - else if (uClearFlag == asfDepositAuth) - { - JLOG(j_.trace()) << "Clear lsfDepositAuth."; - uFlagsOut &= ~lsfDepositAuth; - } + JLOG(j_.trace()) << "Set lsfDepositAuth."; + uFlagsOut |= lsfDepositAuth; + } + else if (uClearFlag == asfDepositAuth) + { + JLOG(j_.trace()) << "Clear lsfDepositAuth."; + uFlagsOut &= ~lsfDepositAuth; } // From 6c375f1346e2227f1e9c4536a1f907a9b82db549 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 11 Nov 2025 12:02:58 -0500 Subject: [PATCH 09/16] Convert sfNextPaymentDueDate from optional to default - Simplifies some of the updates and checks --- .../xrpl/protocol/detail/ledger_entries.macro | 2 +- src/test/app/Loan_test.cpp | 13 ++++++----- src/xrpld/app/misc/detail/LendingHelpers.cpp | 23 ++++++++----------- src/xrpld/app/tx/detail/LoanManage.cpp | 4 +++- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/include/xrpl/protocol/detail/ledger_entries.macro b/include/xrpl/protocol/detail/ledger_entries.macro index 9e9a0c17a4..5f6d1b844e 100644 --- a/include/xrpl/protocol/detail/ledger_entries.macro +++ b/include/xrpl/protocol/detail/ledger_entries.macro @@ -542,7 +542,7 @@ LEDGER_ENTRY(ltLOAN, 0x0089, Loan, loan, ({ {sfPaymentInterval, soeREQUIRED}, {sfGracePeriod, soeDEFAULT}, {sfPreviousPaymentDate, soeDEFAULT}, - {sfNextPaymentDueDate, soeOPTIONAL}, + {sfNextPaymentDueDate, soeDEFAULT}, // The loan object tracks these values: // // - PaymentRemaining: The number of payments left in the loan. When it diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 536ac31eda..47b76d2a8a 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -361,11 +361,8 @@ protected: loan->at(sfPreviousPaymentDate) == previousPaymentDate); env.test.BEAST_EXPECT( loan->at(sfPaymentRemaining) == paymentRemaining); - if (paymentRemaining == 0) - env.test.BEAST_EXPECT(!loan->at(~sfNextPaymentDueDate)); - else - env.test.BEAST_EXPECT( - loan->at(sfNextPaymentDueDate) == nextPaymentDate); + env.test.BEAST_EXPECT( + loan->at(sfNextPaymentDueDate) == nextPaymentDate); env.test.BEAST_EXPECT(loan->at(sfLoanScale) == loanScale); env.test.BEAST_EXPECT( loan->at(sfTotalValueOutstanding) == totalValue); @@ -498,7 +495,7 @@ protected: return LoanState{ .previousPaymentDate = loan->at(sfPreviousPaymentDate), .startDate = tp{d{loan->at(sfStartDate)}}, - .nextPaymentDate = loan->at(~sfNextPaymentDueDate).value_or(0), + .nextPaymentDate = loan->at(sfNextPaymentDueDate), .paymentRemaining = loan->at(sfPaymentRemaining), .loanScale = loan->at(sfLoanScale), .totalValue = loan->at(sfTotalValueOutstanding), @@ -1110,6 +1107,7 @@ protected: detail::PaymentSpecialCase::final) { state.paymentRemaining = 0; + state.nextPaymentDate = 0; } else { @@ -2169,6 +2167,7 @@ protected: state.totalValue = 0; state.principalOutstanding = 0; state.managementFeeOutstanding = 0; + state.nextPaymentDate = 0; verifyLoanStatus(state); // Once a loan is defaulted, it can't be managed @@ -2313,6 +2312,7 @@ protected: state.managementFeeOutstanding = 0; state.previousPaymentDate = state.nextPaymentDate + state.paymentInterval * (numPayments - 1); + state.nextPaymentDate = 0; verifyLoanStatus(state); verifyLoanStatus.checkPayment( @@ -2815,6 +2815,7 @@ protected: detail::PaymentSpecialCase::final) { state.paymentRemaining = 0; + state.nextPaymentDate = 0; } else { diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index 325fb0954a..eac586d1a0 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -518,9 +518,9 @@ doPayment( paymentRemainingProxy = 0; prevPaymentDateProxy = *nextDueDateProxy; - // Remove the field. This is the only condition where nextDueDate is - // allowed to be removed. - nextDueDateProxy = std::nullopt; + // Zero out the next due date. Since it's default, it'll be removed from + // the object. + nextDueDateProxy = 0; // Always zero out the the tracked values on a final payment principalOutstandingProxy = 0; @@ -533,10 +533,8 @@ doPayment( { paymentRemainingProxy -= 1; - prevPaymentDateProxy = *nextDueDateProxy; - // STObject::OptionalField does not define operator+=, so do it the - // old-fashioned way. - nextDueDateProxy = *nextDueDateProxy + paymentInterval; + prevPaymentDateProxy = nextDueDateProxy; + nextDueDateProxy += paymentInterval; } XRPL_ASSERT_PARTS( principalOutstandingProxy > payment.trackedPrincipalDelta, @@ -1699,8 +1697,8 @@ loanMakePayment( auto managementFeeOutstandingProxy = loan->at(sfManagementFeeOutstanding); // Next payment due date must be set unless the loan is complete - auto nextDueDateProxy = loan->at(~sfNextPaymentDueDate); - if (!nextDueDateProxy) + auto nextDueDateProxy = loan->at(sfNextPaymentDueDate); + if (*nextDueDateProxy == 0) { JLOG(j.warn()) << "Loan next payment due date is not set."; return Unexpected(tecINTERNAL); @@ -1743,7 +1741,7 @@ loanMakePayment( "flag to make a late payment. Loan was created on " << startDate << ", prev payment due date is " << prevPaymentDateProxy << ", next payment due date is " - << *nextDueDateProxy << ", ledger time is " + << nextDueDateProxy << ", ledger time is " << view.parentCloseTime().time_since_epoch().count(); return Unexpected(tecEXPIRED); } @@ -1833,7 +1831,7 @@ loanMakePayment( asset, view, principalOutstandingProxy, - *nextDueDateProxy, + nextDueDateProxy, periodic, lateInterestRate, loanScale, @@ -1948,8 +1946,7 @@ loanMakePayment( // overpayment handling if (paymentType == LoanPaymentType::overpayment && loan->isFlag(lsfLoanOverpayment) && paymentRemainingProxy > 0 && - nextDueDateProxy && totalPaid < amount && - numPayments < loanMaximumPaymentsPerTransaction) + totalPaid < amount && numPayments < loanMaximumPaymentsPerTransaction) { TenthBips32 const overpaymentInterestRate{ loan->at(sfOverpaymentInterestRate)}; diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index c45d317dea..73754404e5 100644 --- a/src/xrpld/app/tx/detail/LoanManage.cpp +++ b/src/xrpld/app/tx/detail/LoanManage.cpp @@ -276,7 +276,9 @@ LoanManage::defaultLoan( paymentRemainingProxy = 0; principalOutstandingProxy = 0; managementFeeOutstandingProxy = 0; - loanSle->at(~sfNextPaymentDueDate) = std::nullopt; + // Zero out the next due date. Since it's default, it'll be removed from + // the object. + loanSle->at(sfNextPaymentDueDate) = 0; view.update(loanSle); // Return funds from the LoanBroker pseudo-account to the From 9ffb434315cc616627da909bc98b74351ce1bb6c Mon Sep 17 00:00:00 2001 From: Jingchen Date: Tue, 11 Nov 2025 17:45:13 +0000 Subject: [PATCH 10/16] refactor: Add `XRPL_RETIRE_FIX` and `XRPL_RETIRE_FEATURE` macros (#6014) Rather than having a single `XRPL_RETIRE` macro that applies to both feature and fix amendments, this change replaces it by new `XRPL_RETIRE_FIX` and `XRPL_RETIRE_FEATURE` macros that avoids confusion between whether to prefix the amendment name with `feature` or `fix`. --- include/xrpl/protocol/Feature.h | 30 ++++--- include/xrpl/protocol/detail/features.macro | 92 +++++++++++---------- src/libxrpl/protocol/Feature.cpp | 25 ++++-- 3 files changed, 85 insertions(+), 62 deletions(-) diff --git a/include/xrpl/protocol/Feature.h b/include/xrpl/protocol/Feature.h index 42f2db41f2..193f0665dc 100644 --- a/include/xrpl/protocol/Feature.h +++ b/include/xrpl/protocol/Feature.h @@ -78,12 +78,15 @@ namespace detail { #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX -#pragma push_macro("XRPL_RETIRE") -#undef XRPL_RETIRE +#pragma push_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FEATURE +#pragma push_macro("XRPL_RETIRE_FIX") +#undef XRPL_RETIRE_FIX #define XRPL_FEATURE(name, supported, vote) +1 #define XRPL_FIX(name, supported, vote) +1 -#define XRPL_RETIRE(name) +1 +#define XRPL_RETIRE_FEATURE(name) +1 +#define XRPL_RETIRE_FIX(name) +1 // This value SHOULD be equal to the number of amendments registered in // Feature.cpp. Because it's only used to reserve storage, and determine how @@ -94,8 +97,10 @@ static constexpr std::size_t numFeatures = #include ); -#undef XRPL_RETIRE -#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_RETIRE_FEATURE +#pragma pop_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FIX +#pragma pop_macro("XRPL_RETIRE_FIX") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE @@ -339,17 +344,22 @@ foreachFeature(FeatureBitset bs, F&& f) #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX -#pragma push_macro("XRPL_RETIRE") -#undef XRPL_RETIRE +#pragma push_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FEATURE +#pragma push_macro("XRPL_RETIRE_FIX") +#undef XRPL_RETIRE_FIX #define XRPL_FEATURE(name, supported, vote) extern uint256 const feature##name; #define XRPL_FIX(name, supported, vote) extern uint256 const fix##name; -#define XRPL_RETIRE(name) +#define XRPL_RETIRE_FEATURE(name) +#define XRPL_RETIRE_FIX(name) #include -#undef XRPL_RETIRE -#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_RETIRE_FEATURE +#pragma pop_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FIX +#pragma pop_macro("XRPL_RETIRE_FIX") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE diff --git a/include/xrpl/protocol/detail/features.macro b/include/xrpl/protocol/detail/features.macro index 076307ff36..f38793cb9e 100644 --- a/include/xrpl/protocol/detail/features.macro +++ b/include/xrpl/protocol/detail/features.macro @@ -4,8 +4,11 @@ #if !defined(XRPL_FIX) #error "undefined macro: XRPL_FIX" #endif -#if !defined(XRPL_RETIRE) -#error "undefined macro: XRPL_RETIRE" +#if !defined(XRPL_RETIRE_FEATURE) +#error "undefined macro: XRPL_RETIRE_FEATURE" +#endif +#if !defined(XRPL_RETIRE_FIX) +#error "undefined macro: XRPL_RETIRE_FIX" #endif // Add new amendments to the top of this list. @@ -90,45 +93,46 @@ XRPL_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete) // All known amendments and amendments that may appear in a validated ledger // must be registered either here or above with the "active" amendments // -// Please keep this list sorted alphabetically for convenience. -XRPL_RETIRE(fix1201) -XRPL_RETIRE(fix1368) -XRPL_RETIRE(fix1373) -XRPL_RETIRE(fix1512) -XRPL_RETIRE(fix1513) -XRPL_RETIRE(fix1515) -XRPL_RETIRE(fix1523) -XRPL_RETIRE(fix1528) -XRPL_RETIRE(fix1543) -XRPL_RETIRE(fix1571) -XRPL_RETIRE(fix1578) -XRPL_RETIRE(fix1623) -XRPL_RETIRE(fix1781) -XRPL_RETIRE(fixAmendmentMajorityCalc) -XRPL_RETIRE(fixCheckThreading) -XRPL_RETIRE(fixMasterKeyAsRegularKey) -XRPL_RETIRE(fixNonFungibleTokensV1_2) -XRPL_RETIRE(fixNFTokenRemint) -XRPL_RETIRE(fixPayChanRecipientOwnerDir) -XRPL_RETIRE(fixQualityUpperBound) -XRPL_RETIRE(fixReducedOffersV1) -XRPL_RETIRE(fixRmSmallIncreasedQOffers) -XRPL_RETIRE(fixSTAmountCanonicalize) -XRPL_RETIRE(fixTakerDryOfferRemoval) -XRPL_RETIRE(fixTrustLinesToSelf) -XRPL_RETIRE(CheckCashMakesTrustLine) -XRPL_RETIRE(CryptoConditions) -XRPL_RETIRE(DepositAuth) -XRPL_RETIRE(DepositPreauth) -XRPL_RETIRE(Escrow) -XRPL_RETIRE(EnforceInvariants) -XRPL_RETIRE(FeeEscalation) -XRPL_RETIRE(FlowCross) -XRPL_RETIRE(HardenedValidations) -XRPL_RETIRE(ImmediateOfferKilled) -XRPL_RETIRE(MultiSign) -XRPL_RETIRE(NonFungibleTokensV1_1) -XRPL_RETIRE(PayChan) -XRPL_RETIRE(SortedDirectories) -XRPL_RETIRE(TickSize) -XRPL_RETIRE(TrustSetAuth) +// Please keep both lists sorted alphabetically for convenience. +XRPL_RETIRE_FIX(1201) +XRPL_RETIRE_FIX(1368) +XRPL_RETIRE_FIX(1373) +XRPL_RETIRE_FIX(1512) +XRPL_RETIRE_FIX(1513) +XRPL_RETIRE_FIX(1515) +XRPL_RETIRE_FIX(1523) +XRPL_RETIRE_FIX(1528) +XRPL_RETIRE_FIX(1543) +XRPL_RETIRE_FIX(1571) +XRPL_RETIRE_FIX(1578) +XRPL_RETIRE_FIX(1623) +XRPL_RETIRE_FIX(1781) +XRPL_RETIRE_FIX(AmendmentMajorityCalc) +XRPL_RETIRE_FIX(CheckThreading) +XRPL_RETIRE_FIX(MasterKeyAsRegularKey) +XRPL_RETIRE_FIX(NonFungibleTokensV1_2) +XRPL_RETIRE_FIX(NFTokenRemint) +XRPL_RETIRE_FIX(PayChanRecipientOwnerDir) +XRPL_RETIRE_FIX(QualityUpperBound) +XRPL_RETIRE_FIX(ReducedOffersV1) +XRPL_RETIRE_FIX(RmSmallIncreasedQOffers) +XRPL_RETIRE_FIX(STAmountCanonicalize) +XRPL_RETIRE_FIX(TakerDryOfferRemoval) +XRPL_RETIRE_FIX(TrustLinesToSelf) + +XRPL_RETIRE_FEATURE(CheckCashMakesTrustLine) +XRPL_RETIRE_FEATURE(CryptoConditions) +XRPL_RETIRE_FEATURE(DepositAuth) +XRPL_RETIRE_FEATURE(DepositPreauth) +XRPL_RETIRE_FEATURE(Escrow) +XRPL_RETIRE_FEATURE(EnforceInvariants) +XRPL_RETIRE_FEATURE(FeeEscalation) +XRPL_RETIRE_FEATURE(FlowCross) +XRPL_RETIRE_FEATURE(HardenedValidations) +XRPL_RETIRE_FEATURE(ImmediateOfferKilled) +XRPL_RETIRE_FEATURE(MultiSign) +XRPL_RETIRE_FEATURE(NonFungibleTokensV1_1) +XRPL_RETIRE_FEATURE(PayChan) +XRPL_RETIRE_FEATURE(SortedDirectories) +XRPL_RETIRE_FEATURE(TickSize) +XRPL_RETIRE_FEATURE(TrustSetAuth) diff --git a/src/libxrpl/protocol/Feature.cpp b/src/libxrpl/protocol/Feature.cpp index 8d09378fc7..10c42ccb8a 100644 --- a/src/libxrpl/protocol/Feature.cpp +++ b/src/libxrpl/protocol/Feature.cpp @@ -411,8 +411,10 @@ featureToName(uint256 const& f) #undef XRPL_FEATURE #pragma push_macro("XRPL_FIX") #undef XRPL_FIX -#pragma push_macro("XRPL_RETIRE") -#undef XRPL_RETIRE +#pragma push_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FEATURE +#pragma push_macro("XRPL_RETIRE_FIX") +#undef XRPL_RETIRE_FIX #define XRPL_FEATURE(name, supported, vote) \ uint256 const feature##name = registerFeature(#name, supported, vote); @@ -420,16 +422,23 @@ featureToName(uint256 const& f) uint256 const fix##name = registerFeature("fix" #name, supported, vote); // clang-format off -#define XRPL_RETIRE(name) \ - [[deprecated("The referenced amendment has been retired")]] \ - [[maybe_unused]] \ - uint256 const retired##name = retireFeature(#name); +#define XRPL_RETIRE_FEATURE(name) \ + [[deprecated("The referenced feature amendment has been retired")]] \ + [[maybe_unused]] \ + uint256 const retiredFeature##name = retireFeature(#name); + +#define XRPL_RETIRE_FIX(name) \ + [[deprecated("The referenced fix amendment has been retired")]] \ + [[maybe_unused]] \ + uint256 const retiredFix##name = retireFeature("fix" #name); // clang-format on #include -#undef XRPL_RETIRE -#pragma pop_macro("XRPL_RETIRE") +#undef XRPL_RETIRE_FEATURE +#pragma pop_macro("XRPL_RETIRE_FEATURE") +#undef XRPL_RETIRE_FIX +#pragma pop_macro("XRPL_RETIRE_FIX") #undef XRPL_FIX #pragma pop_macro("XRPL_FIX") #undef XRPL_FEATURE From c2a90b706fdd8ebce191fdd3885852ced888f4d9 Mon Sep 17 00:00:00 2001 From: Ayaz Salikhov Date: Tue, 11 Nov 2025 18:17:03 +0000 Subject: [PATCH 11/16] ci: Specify bash as the default shell in workflows (#6021) --- .github/workflows/reusable-build-test-config.yml | 12 ++++-------- .github/workflows/reusable-strategy-matrix.yml | 4 ++++ .github/workflows/upload-conan-deps.yml | 4 ++++ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 79d21a870a..7032fa5a43 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -55,6 +55,10 @@ on: description: "The Codecov token to use for uploading coverage reports." required: true +defaults: + run: + shell: bash + jobs: build-and-test: name: ${{ inputs.config_name }} @@ -100,7 +104,6 @@ jobs: log_verbosity: ${{ runner.os == 'Windows' && 'quiet' || 'verbose' }} - name: Configure CMake - shell: bash working-directory: ${{ inputs.build_dir }} env: BUILD_TYPE: ${{ inputs.build_type }} @@ -114,7 +117,6 @@ jobs: .. - name: Build the binary - shell: bash working-directory: ${{ inputs.build_dir }} env: BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} @@ -141,7 +143,6 @@ jobs: - name: Check linking (Linux) if: ${{ runner.os == 'Linux' }} working-directory: ${{ inputs.build_dir }} - shell: bash run: | ldd ./rippled if [ "$(ldd ./rippled | grep -E '(libstdc\+\+|libgcc)' | wc -l)" -eq 0 ]; then @@ -154,7 +155,6 @@ jobs: - name: Verify presence of instrumentation (Linux) if: ${{ runner.os == 'Linux' && env.ENABLED_VOIDSTAR == 'true' }} working-directory: ${{ inputs.build_dir }} - shell: bash run: | ./rippled --version | grep libvoidstar @@ -165,7 +165,6 @@ jobs: env: BUILD_TYPE: ${{ inputs.build_type }} PARALLELISM: ${{ runner.os == 'Windows' && '1' || steps.nproc.outputs.nproc }} - shell: bash run: | ctest \ --output-on-failure \ @@ -175,7 +174,6 @@ jobs: - name: Run the embedded tests if: ${{ !inputs.build_only }} working-directory: ${{ runner.os == 'Windows' && format('{0}/{1}', inputs.build_dir, inputs.build_type) || inputs.build_dir }} - shell: bash env: BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} run: | @@ -183,7 +181,6 @@ jobs: - name: Debug failure (Linux) if: ${{ failure() && runner.os == 'Linux' && !inputs.build_only }} - shell: bash run: | echo "IPv4 local port range:" cat /proc/sys/net/ipv4/ip_local_port_range @@ -196,7 +193,6 @@ jobs: env: BUILD_NPROC: ${{ steps.nproc.outputs.nproc }} BUILD_TYPE: ${{ inputs.build_type }} - shell: bash run: | cmake \ --build . \ diff --git a/.github/workflows/reusable-strategy-matrix.yml b/.github/workflows/reusable-strategy-matrix.yml index 129f65938b..c975347307 100644 --- a/.github/workflows/reusable-strategy-matrix.yml +++ b/.github/workflows/reusable-strategy-matrix.yml @@ -18,6 +18,10 @@ on: description: "The generated strategy matrix." value: ${{ jobs.generate-matrix.outputs.matrix }} +defaults: + run: + shell: bash + jobs: generate-matrix: runs-on: ubuntu-latest diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index f6262a2f1f..357d756fa7 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -40,6 +40,10 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true +defaults: + run: + shell: bash + jobs: # Generate the strategy matrix to be used by the following job. generate-matrix: From 2ebc2ca885ffa90fa6c7e8cf421dc0a12a34c3a8 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 11 Nov 2025 19:39:09 +0000 Subject: [PATCH 12/16] fix: floating point representation errors in vault (#5997) This change fixes floating point errors in conversion of shares to assets and other way, used in `VaultDeposit`, `VaultWithdraw` and `VaultClawback`. In the floating point calculations the division introduces a larger error than multiplication. If we do division first, then the error introduced will be increased by the multiplication that follows, which is therefore the wrong order to perform these two operations. This change flips the order of arithmetic operations, which minimizes the error. --- src/libxrpl/ledger/View.cpp | 8 ++-- src/test/app/Vault_test.cpp | 83 ++++++++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0175b099ea..069bd3a4d8 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -2885,7 +2885,7 @@ assetsToSharesDeposit( .truncate()}; Number const shareTotal = issuance->at(sfOutstandingAmount); - shares = (shareTotal * (assets / assetTotal)).truncate(); + shares = ((shareTotal * assets) / assetTotal).truncate(); return shares; } @@ -2914,7 +2914,7 @@ sharesToAssetsDeposit( false}; Number const shareTotal = issuance->at(sfOutstandingAmount); - assets = assetTotal * (shares / shareTotal); + assets = (assetTotal * shares) / shareTotal; return assets; } @@ -2940,7 +2940,7 @@ assetsToSharesWithdraw( if (assetTotal == 0) return shares; Number const shareTotal = issuance->at(sfOutstandingAmount); - Number result = shareTotal * (assets / assetTotal); + Number result = (shareTotal * assets) / assetTotal; if (truncate == TruncateShares::yes) result = result.truncate(); shares = result; @@ -2968,7 +2968,7 @@ sharesToAssetsWithdraw( if (assetTotal == 0) return assets; Number const shareTotal = issuance->at(sfOutstandingAmount); - assets = assetTotal * (shares / shareTotal); + assets = (assetTotal * shares) / shareTotal; return assets; } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index ccbf0fed42..2ca525c036 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -2454,6 +2454,7 @@ class Vault_test : public beast::unit_test::suite struct CaseArgs { int initialXRP = 1000; + Number initialIOU = 200; double transferRate = 1.0; }; @@ -2481,7 +2482,7 @@ class Vault_test : public beast::unit_test::suite PrettyAsset const asset = issuer["IOU"]; env.trust(asset(1000), owner); env.trust(asset(1000), charlie); - env(pay(issuer, owner, asset(200))); + env(pay(issuer, owner, asset(args.initialIOU))); env(rate(issuer, args.transferRate)); env.close(); @@ -2859,6 +2860,86 @@ class Vault_test : public beast::unit_test::suite env(tx1); }); + testCase( + [&, this]( + Env& env, + Account const& owner, + Account const& issuer, + Account const& charlie, + auto const& vaultAccount, + Vault& vault, + PrettyAsset const& asset, + auto&&...) { + testcase("IOU calculation rounding"); + + auto [tx, keylet] = + vault.create({.owner = owner, .asset = asset}); + tx[sfScale] = 1; + env(tx); + env.close(); + + auto const startingOwnerBalance = env.balance(owner, asset); + BEAST_EXPECT( + (startingOwnerBalance.value() == + STAmount{asset, 11875, -2})); + + // This operation (first deposit 100, then 3.75 x 5) is known to + // have triggered calculation rounding errors in Number + // (addition and division), causing the last deposit to be + // blocked by Vault invariants. + env(vault.deposit( + {.depositor = owner, + .id = keylet.key, + .amount = asset(100)})); + + auto const tx1 = vault.deposit( + {.depositor = owner, + .id = keylet.key, + .amount = asset(Number(375, -2))}); + for (auto i = 0; i < 5; ++i) + { + env(tx1); + } + env.close(); + + { + STAmount const xfer{asset, 1185, -1}; + BEAST_EXPECT( + env.balance(owner, asset) == + startingOwnerBalance.value() - xfer); + BEAST_EXPECT( + env.balance(vaultAccount(keylet), asset) == xfer); + + auto const vault = env.le(keylet); + BEAST_EXPECT(vault->at(sfAssetsAvailable) == xfer); + BEAST_EXPECT(vault->at(sfAssetsTotal) == xfer); + } + + // Total vault balance should be 118.5 IOU. Withdraw and delete + // the vault to verify this exact amount was deposited and the + // owner has matching shares + env(vault.withdraw( + {.depositor = owner, + .id = keylet.key, + .amount = asset(Number(1000 + 37 * 5, -1))})); + + { + BEAST_EXPECT( + env.balance(owner, asset) == + startingOwnerBalance.value()); + BEAST_EXPECT( + env.balance(vaultAccount(keylet), asset) == + beast::zero); + auto const vault = env.le(keylet); + BEAST_EXPECT(vault->at(sfAssetsAvailable) == beast::zero); + BEAST_EXPECT(vault->at(sfAssetsTotal) == beast::zero); + } + + env(vault.del({.owner = owner, .id = keylet.key})); + env.close(); + }, + {.initialIOU = Number(11875, -2)}); + auto const [acctReserve, incReserve] = [this]() -> std::pair { Env env{*this, testable_amendments()}; return { From 5fc07e3979da38ff3e04221caecb19670f195a4b Mon Sep 17 00:00:00 2001 From: hustrust Date: Wed, 12 Nov 2025 21:23:45 +0800 Subject: [PATCH 13/16] docs: fix spelling in comments (#6002) --- src/libxrpl/basics/BasicConfig.cpp | 2 +- src/test/app/NFToken_test.cpp | 2 +- src/test/app/NetworkOPs_test.cpp | 2 +- src/test/app/PayChan_test.cpp | 4 ++-- src/test/csf/Peer.h | 2 +- src/test/csf/collectors.h | 2 +- src/test/csf/events.h | 2 +- src/test/csf/ledgers.h | 2 +- src/test/nodestore/TestBase.h | 2 +- src/test/nodestore/import_test.cpp | 2 +- src/test/overlay/tx_reduce_relay_test.cpp | 2 +- src/test/rpc/NoRippleCheck_test.cpp | 2 +- 12 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libxrpl/basics/BasicConfig.cpp b/src/libxrpl/basics/BasicConfig.cpp index 280c660794..63a001bcc3 100644 --- a/src/libxrpl/basics/BasicConfig.cpp +++ b/src/libxrpl/basics/BasicConfig.cpp @@ -30,7 +30,7 @@ Section::append(std::vector const& lines) // '=' static boost::regex const re1( "^" // start of line - "(?:\\s*)" // whitespace (optonal) + "(?:\\s*)" // whitespace (optional) "([a-zA-Z][_a-zA-Z0-9]*)" // "(?:\\s*)" // whitespace (optional) "(?:=)" // '=' diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index 238042d388..a11446df71 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -6808,7 +6808,7 @@ class NFTokenBaseUtil_test : public beast::unit_test::suite mintAndCreateSellOffer(env, alice, XRP(0)); // Bob can accept the offer because the new NFT is stored in - // an existing NFTokenPage so no new reserve is requried + // an existing NFTokenPage so no new reserve is required env(token::acceptSellOffer(bob, sellOfferIndex)); env.close(); } diff --git a/src/test/app/NetworkOPs_test.cpp b/src/test/app/NetworkOPs_test.cpp index 166c3140d4..d9afe6d628 100644 --- a/src/test/app/NetworkOPs_test.cpp +++ b/src/test/app/NetworkOPs_test.cpp @@ -21,7 +21,7 @@ public: void testAllBadHeldTransactions() { - // All trasactions are already marked as SF_BAD, and we should be able + // All transactions are already marked as SF_BAD, and we should be able // to handle the case properly without an assertion failure testcase("No valid transactions in batch"); diff --git a/src/test/app/PayChan_test.cpp b/src/test/app/PayChan_test.cpp index 893d71bfa8..4a1f1c14eb 100644 --- a/src/test/app/PayChan_test.cpp +++ b/src/test/app/PayChan_test.cpp @@ -1889,7 +1889,7 @@ struct PayChan_test : public beast::unit_test::suite BEAST_EXPECT(channelAmount(*env.current(), chan) == XRP(1000)); rmAccount(env, alice, carol, tecHAS_OBLIGATIONS); - // can only remove bob if the channel isn't in their owner direcotry + // can only remove bob if the channel isn't in their owner directory rmAccount(env, bob, carol, TER(tecHAS_OBLIGATIONS)); auto const feeDrops = env.current()->fees().base; @@ -1925,7 +1925,7 @@ struct PayChan_test : public beast::unit_test::suite // Owner closes, will close after settleDelay env(claim(alice, chan), txflags(tfClose)); env.close(); - // settle delay hasn't ellapsed. Channels should exist. + // settle delay hasn't elapsed. Channels should exist. BEAST_EXPECT(channelExists(*env.current(), chan)); auto const closeTime = env.current()->info().parentCloseTime; auto const minExpiration = closeTime + settleDelay; diff --git a/src/test/csf/Peer.h b/src/test/csf/Peer.h index 3a76e7b841..5f6f006b0f 100644 --- a/src/test/csf/Peer.h +++ b/src/test/csf/Peer.h @@ -404,7 +404,7 @@ struct Peer { minDuration = std::min(minDuration, link.data.delay); - // Send a messsage to neighbors to find the ledger + // Send a message to neighbors to find the ledger net.send( this, link.target, [to = link.target, from = this, ledgerID]() { if (auto it = to->ledgers.find(ledgerID); diff --git a/src/test/csf/collectors.h b/src/test/csf/collectors.h index c361701b4e..53494dad44 100644 --- a/src/test/csf/collectors.h +++ b/src/test/csf/collectors.h @@ -28,7 +28,7 @@ namespace csf { /** Group of collectors. Presents a group of collectors as a single collector which process an event - by calling each collector sequentially. This is analagous to CollectorRefs + by calling each collector sequentially. This is analogous to CollectorRefs in CollectorRef.h, but does *not* erase the type information of the combined collectors. */ diff --git a/src/test/csf/events.h b/src/test/csf/events.h index bf01de4eb2..b25dc4b339 100644 --- a/src/test/csf/events.h +++ b/src/test/csf/events.h @@ -13,7 +13,7 @@ namespace test { namespace csf { // Events are emitted by peers at a variety of points during the simulation. -// Each event is emitted by a particlar peer at a particular time. Collectors +// Each event is emitted by a particular peer at a particular time. Collectors // process these events, perhaps calculating statistics or storing events to // a log for post-processing. // diff --git a/src/test/csf/ledgers.h b/src/test/csf/ledgers.h index 26b416a2fb..80e431c1e2 100644 --- a/src/test/csf/ledgers.h +++ b/src/test/csf/ledgers.h @@ -35,7 +35,7 @@ namespace csf { Ledgers are immutable value types. All ledgers with the same sequence number, transactions, close time, etc. will have the same ledger ID. The - LedgerOracle class below manges ID assignments for a simulation and is the + LedgerOracle class below manages ID assignments for a simulation and is the only way to close and create a new ledger. Since the parent ledger ID is part of type, this also means ledgers with distinct histories will have distinct ids, even if they have the same set of transactions, sequence diff --git a/src/test/nodestore/TestBase.h b/src/test/nodestore/TestBase.h index c1401abc33..0675ad85d4 100644 --- a/src/test/nodestore/TestBase.h +++ b/src/test/nodestore/TestBase.h @@ -81,7 +81,7 @@ public: case 3: return hotUNKNOWN; } - // will never happen, but make static analysys tool happy. + // will never happen, but make static analysis tool happy. return hotUNKNOWN; }(); diff --git a/src/test/nodestore/import_test.cpp b/src/test/nodestore/import_test.cpp index 69ef50d453..30aaaa2ea9 100644 --- a/src/test/nodestore/import_test.cpp +++ b/src/test/nodestore/import_test.cpp @@ -235,7 +235,7 @@ parse_args(std::string const& s) // '=' static boost::regex const re1( "^" // start of line - "(?:\\s*)" // whitespace (optonal) + "(?:\\s*)" // whitespace (optional) "([a-zA-Z][_a-zA-Z0-9]*)" // "(?:\\s*)" // whitespace (optional) "(?:=)" // '=' diff --git a/src/test/overlay/tx_reduce_relay_test.cpp b/src/test/overlay/tx_reduce_relay_test.cpp index 3eafa08713..1ca9e7aac6 100644 --- a/src/test/overlay/tx_reduce_relay_test.cpp +++ b/src/test/overlay/tx_reduce_relay_test.cpp @@ -245,7 +245,7 @@ private: // (20+0.25*(60-20)-5=25), queue the rest, skip counts towards relayed // (60-25-5=30) testRelay("skip", true, 60, 0, 20, 25, 25, 30, skip); - // relay to minPeers + disabled + 25% of (nPeers - minPeers - disalbed) + // relay to minPeers + disabled + 25% of (nPeers - minPeers - disabled) // (20+10+0.25*(70-20-10)=40), queue the rest (30) testRelay("disabled", true, 70, 10, 20, 25, 40, 30); // relay to minPeers + disabled-not-in-skip + 25% of (nPeers - minPeers diff --git a/src/test/rpc/NoRippleCheck_test.cpp b/src/test/rpc/NoRippleCheck_test.cpp index 94c85a59ef..81ec097181 100644 --- a/src/test/rpc/NoRippleCheck_test.cpp +++ b/src/test/rpc/NoRippleCheck_test.cpp @@ -252,7 +252,7 @@ class NoRippleCheckLimits_test : public beast::unit_test::suite auto checkBalance = [&env]() { // this is endpoint drop prevention. Non admin ports will drop // requests if they are coming too fast, so we manipulate the - // resource manager here to reset the enpoint balance (for + // resource manager here to reset the endpoint balance (for // localhost) if we get too close to the drop limit. It would // be better if we could add this functionality to Env somehow // or otherwise disable endpoint charging for certain test From 9b53bd9871bf4e3ef8e19c58c827ef0a8361b4c2 Mon Sep 17 00:00:00 2001 From: Bart Date: Wed, 12 Nov 2025 09:30:45 -0500 Subject: [PATCH 14/16] ci: Clean workspace on Windows self-hosted runners (#6024) This change updates the `cleanup-workspace` action to its latest version, which added support for Windows. --- .github/workflows/reusable-build-test-config.yml | 6 +++--- .github/workflows/upload-conan-deps.yml | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/reusable-build-test-config.yml b/.github/workflows/reusable-build-test-config.yml index 7032fa5a43..768897f665 100644 --- a/.github/workflows/reusable-build-test-config.yml +++ b/.github/workflows/reusable-build-test-config.yml @@ -69,9 +69,9 @@ jobs: ENABLED_VOIDSTAR: ${{ contains(inputs.cmake_args, '-Dvoidstar=ON') }} ENABLED_COVERAGE: ${{ contains(inputs.cmake_args, '-Dcoverage=ON') }} steps: - - name: Cleanup workspace - if: ${{ runner.os == 'macOS' }} - uses: XRPLF/actions/.github/actions/cleanup-workspace@3f044c7478548e3c32ff68980eeb36ece02b364e + - name: Cleanup workspace (macOS and Windows) + if: ${{ runner.os == 'macOS' || runner.os == 'Windows' }} + uses: XRPLF/actions/.github/actions/cleanup-workspace@01b244d2718865d427b499822fbd3f15e7197fcc - name: Checkout repository uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 diff --git a/.github/workflows/upload-conan-deps.yml b/.github/workflows/upload-conan-deps.yml index 357d756fa7..320396c899 100644 --- a/.github/workflows/upload-conan-deps.yml +++ b/.github/workflows/upload-conan-deps.yml @@ -62,9 +62,9 @@ jobs: runs-on: ${{ matrix.architecture.runner }} container: ${{ contains(matrix.architecture.platform, 'linux') && format('ghcr.io/xrplf/ci/{0}-{1}:{2}-{3}-sha-{4}', matrix.os.distro_name, matrix.os.distro_version, matrix.os.compiler_name, matrix.os.compiler_version, matrix.os.image_sha) || null }} steps: - - name: Cleanup workspace - if: ${{ runner.os == 'macOS' }} - uses: XRPLF/actions/.github/actions/cleanup-workspace@3f044c7478548e3c32ff68980eeb36ece02b364e + - name: Cleanup workspace (macOS and Windows) + if: ${{ runner.os == 'macOS' || runner.os == 'Windows' }} + uses: XRPLF/actions/.github/actions/cleanup-workspace@01b244d2718865d427b499822fbd3f15e7197fcc - name: Checkout repository uses: actions/checkout@08eba0b27e820071cde6df949e0beb9ba4906955 # v4.3.0 From ef66a1cc0eee294fbe73ae8734672d69c926d501 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Wed, 12 Nov 2025 13:46:11 -0500 Subject: [PATCH 15/16] Disable inner Batch transactions for Vault and Loan types --- src/test/app/Batch_test.cpp | 21 ++++++++++++++++----- src/test/app/Loan_test.cpp | 9 ++++++++- src/xrpld/app/tx/detail/Batch.cpp | 11 ++++++++++- src/xrpld/app/tx/detail/Batch.h | 18 ++++++++++++++++++ 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/test/app/Batch_test.cpp b/src/test/app/Batch_test.cpp index b74d492cc1..2f3b9337e0 100644 --- a/src/test/app/Batch_test.cpp +++ b/src/test/app/Batch_test.cpp @@ -2543,6 +2543,11 @@ class Batch_test : public beast::unit_test::suite { testcase("loan"); + bool const lendingBatchEnabled = !std::any_of( + Batch::disabledTxTypes.begin(), + Batch::disabledTxTypes.end(), + [](auto const& disabled) { return disabled == ttLOAN_BROKER_SET; }); + using namespace test::jtx; test::jtx::Env env{ @@ -2609,7 +2614,8 @@ class Batch_test : public beast::unit_test::suite { auto const [txIDs, batchID] = submitBatch( env, - temBAD_SIGNATURE, + lendingBatchEnabled ? temBAD_SIGNATURE + : temINVALID_INNER_BATCH, batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2648,7 +2654,8 @@ class Batch_test : public beast::unit_test::suite { auto const [txIDs, batchID] = submitBatch( env, - temBAD_SIGNER, + lendingBatchEnabled ? temBAD_SIGNER + : temINVALID_INNER_BATCH, batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2672,7 +2679,8 @@ class Batch_test : public beast::unit_test::suite auto const batchFee = batch::calcBatchFee(env, 1, 2); auto const [txIDs, batchID] = submitBatch( env, - tesSUCCESS, + lendingBatchEnabled ? TER(tesSUCCESS) + : TER(temINVALID_INNER_BATCH), batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2704,7 +2712,8 @@ class Batch_test : public beast::unit_test::suite auto const batchFee = batch::calcBatchFee(env, 1, 2); auto const [txIDs, batchID] = submitBatch( env, - tesSUCCESS, + lendingBatchEnabled ? TER(tesSUCCESS) + : TER(temINVALID_INNER_BATCH), batch::outer(lender, lenderSeq, batchFee, tfAllOrNothing), batch::inner( env.json( @@ -2721,7 +2730,9 @@ class Batch_test : public beast::unit_test::suite } env.close(); BEAST_EXPECT(env.le(brokerKeylet)); - if (auto const sleLoan = env.le(loanKeylet); BEAST_EXPECT(sleLoan)) + if (auto const sleLoan = env.le(loanKeylet); lendingBatchEnabled + ? BEAST_EXPECT(sleLoan) + : !BEAST_EXPECT(!sleLoan)) { BEAST_EXPECT(sleLoan->isFlag(lsfLoanImpaired)); } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 47b76d2a8a..da8b964c94 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -3801,6 +3802,11 @@ protected: // From FIND-001 testcase << "Batch Bypass Counterparty"; + bool const lendingBatchEnabled = !std::any_of( + Batch::disabledTxTypes.begin(), + Batch::disabledTxTypes.end(), + [](auto const& disabled) { return disabled == ttLOAN_BROKER_SET; }); + using namespace jtx; using namespace std::chrono_literals; Env env(*this, all); @@ -3846,7 +3852,8 @@ protected: env(batch::outer(borrower, seq, batchFee, tfAllOrNothing), batch::inner(forgedLoanSet, seq + 1), batch::inner(pay(borrower, lender, XRP(1)), seq + 2), - ter(temBAD_SIGNATURE)); + ter(lendingBatchEnabled ? temBAD_SIGNATURE + : temINVALID_INNER_BATCH)); env.close(); // ? Check that the loan was NOT created diff --git a/src/xrpld/app/tx/detail/Batch.cpp b/src/xrpld/app/tx/detail/Batch.cpp index 052e90e19e..9d2fe4e47d 100644 --- a/src/xrpld/app/tx/detail/Batch.cpp +++ b/src/xrpld/app/tx/detail/Batch.cpp @@ -263,7 +263,8 @@ Batch::preflight(PreflightContext const& ctx) return temREDUNDANT; } - if (stx.getFieldU16(sfTransactionType) == ttBATCH) + auto const txType = stx.getFieldU16(sfTransactionType); + if (txType == ttBATCH) { JLOG(ctx.j.debug()) << "BatchTrace[" << parentBatchId << "]: " << "batch cannot have an inner batch txn. " @@ -271,6 +272,14 @@ Batch::preflight(PreflightContext const& ctx) return temINVALID; } + if (std::any_of( + disabledTxTypes.begin(), + disabledTxTypes.end(), + [txType](auto const& disabled) { return txType == disabled; })) + { + return temINVALID_INNER_BATCH; + } + if (!(stx.getFlags() & tfInnerBatchTxn)) { JLOG(ctx.j.debug()) diff --git a/src/xrpld/app/tx/detail/Batch.h b/src/xrpld/app/tx/detail/Batch.h index 1b1d7614d5..7889e91bdc 100644 --- a/src/xrpld/app/tx/detail/Batch.h +++ b/src/xrpld/app/tx/detail/Batch.h @@ -35,6 +35,24 @@ public: TER doApply() override; + + static constexpr auto disabledTxTypes = std::to_array({ + ttVAULT_CREATE, + ttVAULT_SET, + ttVAULT_DELETE, + ttVAULT_DEPOSIT, + ttVAULT_WITHDRAW, + ttVAULT_CLAWBACK, + ttLOAN_BROKER_SET, + ttLOAN_BROKER_DELETE, + ttLOAN_BROKER_COVER_DEPOSIT, + ttLOAN_BROKER_COVER_WITHDRAW, + ttLOAN_BROKER_COVER_CLAWBACK, + ttLOAN_SET, + ttLOAN_DELETE, + ttLOAN_MANAGE, + ttLOAN_PAY, + }); }; } // namespace ripple From 8580a5795af647f4274d6210faaa3b96bbac7cd2 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 12 Nov 2025 18:55:49 +0000 Subject: [PATCH 16/16] chore: Set version 3.1.0-b0 (#5986) Technically b0 is not a release, so no "release" prefix here. It marks the point at which we moved the preceding release (3.0.0 in this case) from Beta to Release Candidate. --- 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 af21ce5985..7690ae586b 100644 --- a/src/libxrpl/protocol/BuildInfo.cpp +++ b/src/libxrpl/protocol/BuildInfo.cpp @@ -17,7 +17,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "3.0.0-b1" +char const* const versionString = "3.1.0-b0" // clang-format on #if defined(DEBUG) || defined(SANITIZER)