From ad190a8d6fa804352d1673559f59e4d20b5cf6a4 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 1 Dec 2025 16:18:12 -0500 Subject: [PATCH 1/4] Review feedback from @dangell7: early return & coverage - Exclude LogicError lines in ApplyView.cpp (specifically directory operations) from code coverage. - Replace the ability to set the next page on a new directory page with an assert, because nothing uses it right now. - Early return with success for batch inner transactions in preflight2. --- src/libxrpl/ledger/ApplyView.cpp | 41 +++++++++++++++++++++----- src/xrpld/app/tx/detail/Transactor.cpp | 26 +++++++++------- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/src/libxrpl/ledger/ApplyView.cpp b/src/libxrpl/ledger/ApplyView.cpp index 5e8d03b506..911bec1722 100644 --- a/src/libxrpl/ledger/ApplyView.cpp +++ b/src/libxrpl/ledger/ApplyView.cpp @@ -40,7 +40,10 @@ findPreviousPage(ApplyView& view, Keylet const& directory, SLE::ref start) { node = view.peek(keylet::page(directory, page)); if (!node) + { // LCOV_EXCL_START LogicError("Directory chain: root back-pointer broken."); + // LCOV_EXCL_STOP + } } auto indexes = node->getFieldV256(sfIndexes); @@ -59,7 +62,7 @@ insertKey( if (preserveOrder) { if (std::find(indexes.begin(), indexes.end(), key) != indexes.end()) - LogicError("dirInsert: double insertion"); + LogicError("dirInsert: double insertion"); // LCOV_EXCL_LINE indexes.push_back(key); } @@ -73,7 +76,7 @@ insertKey( auto pos = std::lower_bound(indexes.begin(), indexes.end(), key); if (pos != indexes.end() && key == *pos) - LogicError("dirInsert: double insertion"); + LogicError("dirInsert: double insertion"); // LCOV_EXCL_LINE indexes.insert(pos, key); } @@ -131,8 +134,15 @@ insertPage( // it's the default. if (page != 1) node->setFieldU64(sfIndexPrevious, page - 1); + XRPL_ASSERT_PARTS( + !nextPage, + "ripple::directory::insertPage", + "nextPage has default value"); + /* Reserved for future use when directory pages may be inserted in + * between two other pages instead of only at the end of the chain. if (nextPage) node->setFieldU64(sfIndexNext, nextPage); + */ describe(node); view.insert(node); @@ -197,10 +207,10 @@ ApplyView::emptyDirDelete(Keylet const& directory) auto nextPage = node->getFieldU64(sfIndexNext); if (nextPage == rootPage && prevPage != rootPage) - LogicError("Directory chain: fwd link broken"); + LogicError("Directory chain: fwd link broken"); // LCOV_EXCL_LINE if (prevPage == rootPage && nextPage != rootPage) - LogicError("Directory chain: rev link broken"); + LogicError("Directory chain: rev link broken"); // LCOV_EXCL_LINE // Older versions of the code would, in some cases, allow the last // page to be empty. Remove such pages: @@ -209,7 +219,10 @@ ApplyView::emptyDirDelete(Keylet const& directory) auto last = peek(keylet::page(directory, nextPage)); if (!last) + { // LCOV_EXCL_START LogicError("Directory chain: fwd link broken."); + // LCOV_EXCL_STOP + } if (!last->getFieldV256(sfIndexes).empty()) return false; @@ -281,10 +294,16 @@ ApplyView::dirRemove( if (page == rootPage) { if (nextPage == page && prevPage != page) + { // LCOV_EXCL_START LogicError("Directory chain: fwd link broken"); + // LCOV_EXCL_STOP + } if (prevPage == page && nextPage != page) + { // LCOV_EXCL_START LogicError("Directory chain: rev link broken"); + // LCOV_EXCL_STOP + } // Older versions of the code would, in some cases, // allow the last page to be empty. Remove such @@ -293,7 +312,10 @@ ApplyView::dirRemove( { auto last = peek(keylet::page(directory, nextPage)); if (!last) + { // LCOV_EXCL_START LogicError("Directory chain: fwd link broken."); + // LCOV_EXCL_STOP + } if (last->getFieldV256(sfIndexes).empty()) { @@ -325,10 +347,10 @@ ApplyView::dirRemove( // This can never happen for nodes other than the root: if (nextPage == page) - LogicError("Directory chain: fwd link broken"); + LogicError("Directory chain: fwd link broken"); // LCOV_EXCL_LINE if (prevPage == page) - LogicError("Directory chain: rev link broken"); + LogicError("Directory chain: rev link broken"); // LCOV_EXCL_LINE // This node isn't the root, so it can either be in the // middle of the list, or at the end. Unlink it first @@ -336,14 +358,14 @@ ApplyView::dirRemove( // root: auto prev = peek(keylet::page(directory, prevPage)); if (!prev) - LogicError("Directory chain: fwd link broken."); + LogicError("Directory chain: fwd link broken."); // LCOV_EXCL_LINE // Fix previous to point to its new next. prev->setFieldU64(sfIndexNext, nextPage); update(prev); auto next = peek(keylet::page(directory, nextPage)); if (!next) - LogicError("Directory chain: rev link broken."); + LogicError("Directory chain: rev link broken."); // LCOV_EXCL_LINE // Fix next to point to its new previous. next->setFieldU64(sfIndexPrevious, prevPage); update(next); @@ -367,7 +389,10 @@ ApplyView::dirRemove( // And the root points to the last page: auto root = peek(keylet::page(directory, rootPage)); if (!root) + { // LCOV_EXCL_START LogicError("Directory chain: root link broken."); + // LCOV_EXCL_STOP + } root->setFieldU64(sfIndexPrevious, prevPage); update(root); diff --git a/src/xrpld/app/tx/detail/Transactor.cpp b/src/xrpld/app/tx/detail/Transactor.cpp index 5da62f2cde..2ddef72c39 100644 --- a/src/xrpld/app/tx/detail/Transactor.cpp +++ b/src/xrpld/app/tx/detail/Transactor.cpp @@ -205,17 +205,23 @@ Transactor::preflight2(PreflightContext const& ctx) return *ret; // Skip signature check on batch inner transactions - if (!ctx.tx.isFlag(tfInnerBatchTxn) || !ctx.rules.enabled(featureBatch)) - { - auto const sigValid = checkValidity( - ctx.app.getHashRouter(), ctx.tx, ctx.rules, ctx.app.config()); - if (sigValid.first == Validity::SigBad) - { // LCOV_EXCL_START - JLOG(ctx.j.debug()) - << "preflight2: bad signature. " << sigValid.second; - return temINVALID; - } // LCOV_EXCL_STOP + if (ctx.tx.isFlag(tfInnerBatchTxn) && !ctx.rules.enabled(featureBatch)) + return tesSUCCESS; + // Do not add any checks after this point that are relevant for + // batch inner transactions. They will be skipped. + + auto const sigValid = checkValidity( + ctx.app.getHashRouter(), ctx.tx, ctx.rules, ctx.app.config()); + if (sigValid.first == Validity::SigBad) + { // LCOV_EXCL_START + JLOG(ctx.j.debug()) << "preflight2: bad signature. " << sigValid.second; + return temINVALID; + // LCOV_EXCL_STOP } + + // Do not add any checks after this point that are relevant for + // batch inner transactions. They will be skipped. + return tesSUCCESS; } From 803380c53cb6932f02caaccf6839c0ac2d5ef73b Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Mon, 1 Dec 2025 20:59:12 +0100 Subject: [PATCH 2/4] Fix overpayment asserts (#6084) --- src/xrpld/app/misc/detail/LendingHelpers.cpp | 22 +++++++------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index 8020b47ba9..1361b0679a 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -547,6 +547,14 @@ tryOverpayment( auto const deltas = rounded - newRounded; + // The change in loan management fee is equal to the change between the old + // and the new outstanding management fees + XRPL_ASSERT_PARTS( + deltas.managementFee == + rounded.managementFeeDue - managementFeeOutstanding, + "ripple::detail::tryOverpayment", + "no fee change"); + auto const hypotheticalValueOutstanding = rounded.valueOutstanding - deltas.principal; @@ -561,7 +569,6 @@ tryOverpayment( "the loan. Ignore the overpayment"; return Unexpected(tesSUCCESS); } - return LoanPaymentParts{ // Principal paid is the reduction in principal outstanding .principalPaid = deltas.principal, @@ -676,12 +683,6 @@ doOverpayment( "ripple::detail::doOverpayment", "principal change agrees"); - XRPL_ASSERT_PARTS( - overpaymentComponents.trackedManagementFeeDelta == - managementFeeOutstandingProxy - managementFeeOutstanding, - "ripple::detail::doOverpayment", - "no fee change"); - // I'm not 100% sure the following asserts are correct. If in doubt, and // everything else works, remove any that cause trouble. @@ -712,13 +713,6 @@ doOverpayment( "ripple::detail::doOverpayment", "principal payment matches"); - XRPL_ASSERT_PARTS( - loanPaymentParts.feePaid == - overpaymentComponents.untrackedManagementFee + - overpaymentComponents.trackedManagementFeeDelta, - "ripple::detail::doOverpayment", - "fee payment matches"); - // All validations passed, so update the proxy objects (which will // modify the actual Loan ledger object) totalValueOutstandingProxy = totalValueOutstanding; From e2e9582ff102fbc69cf4372eee8d2dc12856b70c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 2 Dec 2025 10:50:57 -0500 Subject: [PATCH 3/4] Test updates - show balances in runLoan() --- src/test/app/Loan_test.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index b2ad47c2b4..aa196d0e65 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -1271,7 +1271,8 @@ protected: verifyLoanStatus, issuer, lender, - borrower); + borrower, + PaymentParameters{.showStepBalances = true}); } /** Runs through the complete lifecycle of a loan @@ -7193,15 +7194,15 @@ class LoanArbitrary_test : public LoanBatch_test .vaultDeposit = 10000, .debtMax = 0, .coverRateMin = TenthBips32{0}, - // .managementFeeRate = TenthBips16{5919}, + .managementFeeRate = TenthBips16{0}, .coverRateLiquidation = TenthBips32{0}}; LoanParameters const loanParams{ .account = Account("lender"), .counter = Account("borrower"), - .principalRequest = Number{10000, 0}, - // .interest = TenthBips32{0}, - // .payTotal = 5816, - .payInterval = 150}; + .principalRequest = Number{200000, -6}, + .interest = TenthBips32{50000}, + .payTotal = 2, + .payInterval = 200}; runLoan(AssetType::XRP, brokerParams, loanParams); } From 43a6f10050bc6c6680ffe2f753bcb80f89efa2e7 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 2 Dec 2025 22:56:19 -0500 Subject: [PATCH 4/4] MPTTester::operator() parameter should be std::int64_t - Originally defined as uint64_t, but the testIssuerLoan() test called it with a negative number, causing an overflow to a very large number that in some circumstances could be silently cast back to an int64_t, but might not be. I believe this is UB, and we don't want to rely on that. --- src/test/jtx/impl/mpt.cpp | 2 +- src/test/jtx/mpt.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/jtx/impl/mpt.cpp b/src/test/jtx/impl/mpt.cpp index fc831790f1..d7b4a1570e 100644 --- a/src/test/jtx/impl/mpt.cpp +++ b/src/test/jtx/impl/mpt.cpp @@ -644,7 +644,7 @@ MPTTester::operator[](std::string const& name) const } PrettyAmount -MPTTester::operator()(std::uint64_t amount) const +MPTTester::operator()(std::int64_t amount) const { return MPT("", issuanceID())(amount); } diff --git a/src/test/jtx/mpt.h b/src/test/jtx/mpt.h index f9c58ebc9e..e3097b28b0 100644 --- a/src/test/jtx/mpt.h +++ b/src/test/jtx/mpt.h @@ -272,7 +272,7 @@ public: operator[](std::string const& name) const; PrettyAmount - operator()(std::uint64_t amount) const; + operator()(std::int64_t amount) const; operator Asset() const;