Fix clang-tidy issues

* upstream/develop: (30 commits)
  chore: Enable clang-tidy include cleaner (6947)
  fix: Change AMMClawback return code to tecNO_PERMISSION (6946)
  ci: [DEPENDABOT] bump actions/upload-pages-artifact from 4.0.0 to 5.0.0 (6927)
  ci: [DEPENDABOT] bump actions/upload-artifact from 7.0.0 to 7.0.1 (6928)
  chore: Enable clang-tidy readability checks (6930)
  fix: Fix unity build for book step (6942)
  chore: Move codegen venv setup into build stage (6617)
  chore: Enable most clang-tidy bugprone checks (6929)
  refactor: Improve exception handling (6540) (6735)
  refactor: Remove unused notTooManyOffers function from NFTokenUtils (6737)
  fix: Change `Tuning::bookOffers` minimum limit to 1 (6812)
  chore: Make pre-commit line ending conversions work on Windows (6832) (6833)
  fix: Add description for `terLOCKED` error (6811)
  fix: Address AI reviewer comments for Permission Delegation (6675)
  refactor: Combine `AMMHelpers` and `AMMUtils` (6733)
  feat: Add MPT support to DEX (5285)
  fix: Handle WSClient write failure when server closes WebSocket (6671)
  ci: Change conditions for uploading artifacts in public/private/org repos (6734)
  refactor: Rename non-functional uses of `ripple(d)` to `xrpl(d)` (6676)
  refactor: Move more helper files into `libxrpl/ledger/helpers` (6731)
  ...

cfg/xrpld-example.cfg

@@ -1033,11 +1033,10 @@
 #                           The online delete process checks periodically
 #                           that xrpld is still in sync with the network,
 #                           and that the validated ledger is less than
-#                           'age_threshold_seconds' old, and that all
-#                           recent ledgers are available. If not, then continue
+#                           'age_threshold_seconds' old. If not, then continue
 #                           sleeping for this number of seconds and
 #                           checking until healthy.
-#                           Default is 1.
+#                           Default is 5.
 #
 #   Notes:
 #       The 'node_db' entry configures the primary, persistent storage.

include/xrpl/protocol/STAmount.h

@@ -42,8 +42,8 @@ private:
 public:
     using value_type = STAmount;
 
-    static int const cMinOffset = -96;
-    static int const cMaxOffset = 80;
+    static constexpr int cMinOffset = -96;
+    static constexpr int cMaxOffset = 80;
 
     // Maximum native value supported by the code
     constexpr static std::uint64_t cMinValue = 1'000'000'000'000'000ull;

src/libxrpl/tx/transactors/lending/LendingHelpers.cpp

@@ -1847,8 +1847,18 @@ loanMakePayment(
 
     // -------------------------------------------------------------
     // overpayment handling
+    //
+    // If the "fixSecurity3_1_3" amendment is enabled, truncate "amount",
+    // at the loan scale. If the raw value is used, the overpayment
+    // amount could be meaningless dust. Trying to process such a small
+    // amount will, at best, waste time when all the result values round
+    // to zero. At worst, it can cause logical errors with tiny amounts
+    // of interest that don't add up correctly.
+    auto const roundedAmount = view.rules().enabled(fixSecurity3_1_3)
+        ? roundToAsset(asset, amount, loanScale, Number::towards_zero)
+        : amount;
     if (paymentType == LoanPaymentType::overpayment && loan->isFlag(lsfLoanOverpayment) &&
-        paymentRemainingProxy > 0 && totalPaid < amount &&
+        paymentRemainingProxy > 0 && totalPaid < roundedAmount &&
         numPayments < loanMaximumPaymentsPerTransaction)
     {
         TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)};
@@ -1857,7 +1867,7 @@ loanMakePayment(
         // It shouldn't be possible for the overpayment to be greater than
         // totalValueOutstanding, because that would have been processed as
         // another normal payment. But cap it just in case.
-        Number const overpayment = std::min(amount - totalPaid, *totalValueOutstandingProxy);
+        Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy);
 
         detail::ExtendedPaymentComponents const overpaymentComponents =
             detail::computeOverpaymentComponents(

src/libxrpl/tx/transactors/lending/LoanPay.cpp

@@ -27,6 +27,7 @@
 #include <algorithm>
 #include <bit>
 #include <cstdint>
+#include <vector>
 
 namespace xrpl {
 
@@ -437,9 +438,10 @@ LoanPay::doApply()
     // Vault object state changes
     view.update(vaultSle);
 
+    Number const assetsAvailableBefore = *assetsAvailableProxy;
+    Number const assetsTotalBefore = *assetsTotalProxy;
 #if !NDEBUG
     {
-        Number const assetsAvailableBefore = *assetsAvailableProxy;
         Number const pseudoAccountBalanceBefore = accountHolds(
             view,
             vaultPseudoAccount,
@@ -463,16 +465,6 @@ LoanPay::doApply()
         "xrpl::LoanPay::doApply",
         "assets available must not be greater than assets outstanding");
 
-    if (*assetsAvailableProxy > *assetsTotalProxy)
-    {
-        // LCOV_EXCL_START
-        JLOG(j_.fatal()) << "Vault assets available must not be greater "
-                            "than assets outstanding. Available: "
-                         << *assetsAvailableProxy << ", Total: " << *assetsTotalProxy;
-        return tecINTERNAL;
-        // LCOV_EXCL_STOP
-    }
-
     JLOG(j_.debug()) << "total paid to vault raw: " << totalPaidToVaultRaw
                      << ", total paid to vault rounded: " << totalPaidToVaultRounded
                      << ", total paid to broker: " << totalPaidToBroker
@@ -498,12 +490,68 @@ LoanPay::doApply()
     associateAsset(*vaultSle, asset);
 
     // Duplicate some checks after rounding
+    Number const assetsAvailableAfter = *assetsAvailableProxy;
+    Number const assetsTotalAfter = *assetsTotalProxy;
+
     XRPL_ASSERT_PARTS(
-        *assetsAvailableProxy <= *assetsTotalProxy,
+        assetsAvailableAfter <= assetsTotalAfter,
         "xrpl::LoanPay::doApply",
         "assets available must not be greater than assets outstanding");
+    if (assetsAvailableAfter == assetsAvailableBefore)
+    {
+        // An unchanged assetsAvailable indicates that the amount paid to the
+        // vault was zero, or rounded to zero. That should be impossible, but I
+        // can't rule it out for extreme edge cases, so fail gracefully if it
+        // happens.
+        //
+        // LCOV_EXCL_START
+        JLOG(j_.warn()) << "LoanPay: Vault assets available unchanged after rounding: "  //
+                        << "Before: " << assetsAvailableBefore                           //
+                        << ", After: " << assetsAvailableAfter;
+        return tecPRECISION_LOSS;
+        // LCOV_EXCL_STOP
+    }
+    if (paymentParts->valueChange != beast::zero && assetsTotalAfter == assetsTotalBefore)
+    {
+        // Non-zero valueChange with an unchanged assetsTotal indicates that the
+        // actual value change rounded to zero. That should be impossible, but I
+        // can't rule it out for extreme edge cases, so fail gracefully if it
+        // happens.
+        //
+        // LCOV_EXCL_START
+        JLOG(j_.warn())
+            << "LoanPay: Vault assets expected change, but unchanged after rounding: "  //
+            << "Before: " << assetsTotalBefore                                          //
+            << ", After: " << assetsTotalAfter                                          //
+            << ", ValueChange: " << paymentParts->valueChange;
+        return tecPRECISION_LOSS;
+        // LCOV_EXCL_STOP
+    }
+    if (paymentParts->valueChange == beast::zero && assetsTotalAfter != assetsTotalBefore)
+    {
+        // A change in assetsTotal when there was no valueChange indicates that
+        // something really weird happened. That should be flat out impossible.
+        //
+        // LCOV_EXCL_START
+        JLOG(j_.fatal()) << "LoanPay: Vault assets changed unexpectedly after rounding: "  //
+                         << "Before: " << assetsTotalBefore                                //
+                         << ", After: " << assetsTotalAfter                                //
+                         << ", ValueChange: " << paymentParts->valueChange;
+        return tecINTERNAL;
+        // LCOV_EXCL_STOP
+    }
+    if (assetsAvailableAfter > assetsTotalAfter)
+    {
+        // Assets available are not allowed to be larger than assets total.
+        // LCOV_EXCL_START
+        JLOG(j_.fatal()) << "LoanPay: Vault assets available must not be greater "
+                            "than assets outstanding. Available: "
+                         << assetsAvailableAfter << ", Total: " << assetsTotalAfter;
+        return tecINTERNAL;
+        // LCOV_EXCL_STOP
+    }
 
-#if !NDEBUG
+    // These three values are used to check that funds are conserved after the transfers
     auto const accountBalanceBefore = accountHolds(
         view,
         account_,
@@ -532,7 +580,6 @@ LoanPay::doApply()
               ahIGNORE_AUTH,
               j_,
               SpendableHandling::shFULL_BALANCE);
-#endif
 
     if (totalPaidToVaultRounded != beast::zero)
     {
@@ -568,19 +615,22 @@ LoanPay::doApply()
         return ter;
 
 #if !NDEBUG
-    Number const assetsAvailableAfter = *assetsAvailableProxy;
-    Number const pseudoAccountBalanceAfter = accountHolds(
-        view,
-        vaultPseudoAccount,
-        asset,
-        FreezeHandling::fhIGNORE_FREEZE,
-        AuthHandling::ahIGNORE_AUTH,
-        j_);
-    XRPL_ASSERT_PARTS(
-        assetsAvailableAfter == pseudoAccountBalanceAfter,
-        "xrpl::LoanPay::doApply",
-        "vault pseudo balance agrees after");
+    {
+        Number const pseudoAccountBalanceAfter = accountHolds(
+            view,
+            vaultPseudoAccount,
+            asset,
+            FreezeHandling::fhIGNORE_FREEZE,
+            AuthHandling::ahIGNORE_AUTH,
+            j_);
+        XRPL_ASSERT_PARTS(
+            assetsAvailableAfter == pseudoAccountBalanceAfter,
+            "xrpl::LoanPay::doApply",
+            "vault pseudo balance agrees after");
+    }
+#endif
 
+    // Check that funds are conserved
     auto const accountBalanceAfter = accountHolds(
         view,
         account_,
@@ -609,14 +659,114 @@ LoanPay::doApply()
               ahIGNORE_AUTH,
               j_,
               SpendableHandling::shFULL_BALANCE);
+    auto const balanceScale = [&]() {
+        // Find a reasonable scale to use for the balance comparisons.
+        //
+        // First find the minimum and maximum exponent of all the non-zero balances, before and
+        // after. If min and max are equal, use that value. If they are not, use "max + 1" to reduce
+        // rounding discrepancies without making the result meaningless. Cap the scale at
+        // STAmount::cMaxOffset, just in case the numbers are all very large.
+        std::vector<int> exponents;
 
+        for (auto const& a : {
+                 accountBalanceBefore,
+                 vaultBalanceBefore,
+                 brokerBalanceBefore,
+                 accountBalanceAfter,
+                 vaultBalanceAfter,
+                 brokerBalanceAfter,
+             })
+        {
+            // Exclude zeroes
+            if (a != beast::zero)
+                exponents.push_back(a.exponent());
+        }
+        if (exponents.empty())
+        {
+            UNREACHABLE("xrpl::LoanPay::doApply : all zeroes");
+            return 0;
+        }
+        auto const [minItr, maxItr] = std::minmax_element(exponents.begin(), exponents.end());
+        auto const min = *minItr;
+        auto const max = *maxItr;
+        JLOG(j_.trace()) << "Min scale: " << min << ", max scale: " << max;
+        // IOU rounding can be interesting. We want all the balance checks to agree, but don't want
+        // to round to such an extreme that it becomes meaningless.  e.g. Everything rounds to one
+        // digit. So add 1 to the max (reducing the number of digits after the decimal point by 1)
+        // if the scales are not already all the same.
+        return std::min(min == max ? max : max + 1, STAmount::cMaxOffset);
+    }();
+
+    auto const accountBalanceBeforeRounded = roundToScale(accountBalanceBefore, balanceScale);
+    auto const vaultBalanceBeforeRounded = roundToScale(vaultBalanceBefore, balanceScale);
+    auto const brokerBalanceBeforeRounded = roundToScale(brokerBalanceBefore, balanceScale);
+
+    auto const totalBalanceBefore = accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore;
+    auto const totalBalanceBeforeRounded = roundToScale(totalBalanceBefore, balanceScale);
+
+    JLOG(j_.trace()) << "Before: "  //
+                     << "account " << Number(accountBalanceBeforeRounded) << " ("
+                     << Number(accountBalanceBefore) << ")"
+                     << ", vault " << Number(vaultBalanceBeforeRounded) << " ("
+                     << Number(vaultBalanceBefore) << ")"
+                     << ", broker " << Number(brokerBalanceBeforeRounded) << " ("
+                     << Number(brokerBalanceBefore) << ")"
+                     << ", total " << Number(totalBalanceBeforeRounded) << " ("
+                     << Number(totalBalanceBefore) << ")";
+
+    auto const accountBalanceAfterRounded = roundToScale(accountBalanceAfter, balanceScale);
+    auto const vaultBalanceAfterRounded = roundToScale(vaultBalanceAfter, balanceScale);
+    auto const brokerBalanceAfterRounded = roundToScale(brokerBalanceAfter, balanceScale);
+
+    auto const totalBalanceAfter = accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter;
+    auto const totalBalanceAfterRounded = roundToScale(totalBalanceAfter, balanceScale);
+
+    JLOG(j_.trace()) << "After: "  //
+                     << "account " << Number(accountBalanceAfterRounded) << " ("
+                     << Number(accountBalanceAfter) << ")"
+                     << ", vault " << Number(vaultBalanceAfterRounded) << " ("
+                     << Number(vaultBalanceAfter) << ")"
+                     << ", broker " << Number(brokerBalanceAfterRounded) << " ("
+                     << Number(brokerBalanceAfter) << ")"
+                     << ", total " << Number(totalBalanceAfterRounded) << " ("
+                     << Number(totalBalanceAfter) << ")";
+
+    auto const accountBalanceChange = accountBalanceAfter - accountBalanceBefore;
+    auto const vaultBalanceChange = vaultBalanceAfter - vaultBalanceBefore;
+    auto const brokerBalanceChange = brokerBalanceAfter - brokerBalanceBefore;
+
+    auto const totalBalanceChange = accountBalanceChange + vaultBalanceChange + brokerBalanceChange;
+    auto const totalBalanceChangeRounded = roundToScale(totalBalanceChange, balanceScale);
+
+    JLOG(j_.trace()) << "Changes: "                                    //
+                     << "account " << to_string(accountBalanceChange)  //
+                     << ", vault " << to_string(vaultBalanceChange)    //
+                     << ", broker " << to_string(brokerBalanceChange)  //
+                     << ", total " << to_string(totalBalanceChangeRounded) << " ("
+                     << Number(totalBalanceChange) << ")";
+
+    if (totalBalanceBeforeRounded != totalBalanceAfterRounded)
+    {
+        JLOG(j_.warn()) << "Total rounded balances don't match"
+                        << (totalBalanceChangeRounded == beast::zero ? ", but total changes do"
+                                                                     : "");
+    }
+    if (totalBalanceChangeRounded != beast::zero)
+    {
+        JLOG(j_.warn()) << "Total balance changes don't match"
+                        << (totalBalanceBeforeRounded == totalBalanceAfterRounded
+                                ? ", but total balances do"
+                                : "");
+    }
+
+    // Rounding for IOUs can be weird, so check a few different ways to show
+    // that funds are conserved.
     XRPL_ASSERT_PARTS(
-        accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore ==
-            accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter,
+        totalBalanceBeforeRounded == totalBalanceAfterRounded ||
+            totalBalanceChangeRounded == beast::zero,
         "xrpl::LoanPay::doApply",
         "funds are conserved (with rounding)");
-    XRPL_ASSERT_PARTS(
-        accountBalanceAfter >= beast::zero, "xrpl::LoanPay::doApply", "positive account balance");
+
     XRPL_ASSERT_PARTS(
         accountBalanceAfter < accountBalanceBefore || account_ == asset.getIssuer(),
         "xrpl::LoanPay::doApply",
@@ -637,7 +787,6 @@ LoanPay::doApply()
         vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore,
         "xrpl::LoanPay::doApply",
         "vault and/or broker balance increased");
-#endif
 
     return tesSUCCESS;
 }

src/test/app/LedgerMaster_test.cpp

@@ -5,21 +5,17 @@
 #include <test/jtx/noop.h>
 
 #include <xrpld/app/ledger/LedgerMaster.h>
-#include <xrpld/app/misc/SHAMapStore.h>
 #include <xrpld/core/Config.h>
 
-#include <xrpl/basics/ToString.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/unit_test/suite.h>
 #include <xrpl/protocol/Feature.h>
-#include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STObject.h>
 #include <xrpl/protocol/STTx.h>
 
 #include <cstdint>
 #include <memory>
-#include <sstream>
 #include <vector>
 
 namespace xrpl {
@@ -116,73 +112,6 @@ class LedgerMaster_test : public beast::unit_test::suite
         }
     }
 
-    void
-    testCompleteLedgerRange(FeatureBitset features)
-    {
-        // Note that this test is intentionally very similar to
-        // SHAMapStore_test::testLedgerGaps, but has a different
-        // focus.
-
-        testcase("Complete Ledger operations");
-
-        using namespace test::jtx;
-
-        auto const deleteInterval = 8;
-
-        Env env{*this, envconfig([](auto cfg) {
-                    return online_delete(std::move(cfg), deleteInterval);
-                })};
-
-        auto const alice = Account("alice");
-        env.fund(XRP(1000), alice);
-        env.close();
-
-        auto& lm = env.app().getLedgerMaster();
-        LedgerIndex minSeq = 2;
-        LedgerIndex maxSeq = env.closed()->header().seq;
-        auto& store = env.app().getSHAMapStore();
-        store.rendezvous();
-        LedgerIndex lastRotated = store.getLastRotated();
-        BEAST_EXPECTS(maxSeq == 3, to_string(maxSeq));
-        BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers());
-        BEAST_EXPECTS(lastRotated == 3, to_string(lastRotated));
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
-
-        // Close enough ledgers to rotate a few times
-        for (int i = 0; i < 24; ++i)
-        {
-            for (int t = 0; t < 3; ++t)
-            {
-                env(noop(alice));
-            }
-            env.close();
-            store.rendezvous();
-
-            ++maxSeq;
-
-            if (maxSeq == lastRotated + deleteInterval)
-            {
-                minSeq = lastRotated;
-                lastRotated = maxSeq;
-            }
-            BEAST_EXPECTS(
-                env.closed()->header().seq == maxSeq, to_string(env.closed()->header().seq));
-            BEAST_EXPECTS(store.getLastRotated() == lastRotated, to_string(store.getLastRotated()));
-            std::stringstream expectedRange;
-            expectedRange << minSeq << "-" << maxSeq;
-            BEAST_EXPECTS(lm.getCompleteLedgers() == expectedRange.str(), lm.getCompleteLedgers());
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
-        }
-    }
-
 public:
     void
     run() override
@@ -196,7 +125,6 @@ public:
     testWithFeats(FeatureBitset features)
     {
         testTxnIdFromIndex(features);
-        testCompleteLedgerRange(features);
     }
 };
 

src/test/app/Loan_test.cpp

@@ -368,16 +368,11 @@ protected:
                         env.balance(vaultPseudo, broker.asset).number());
                     if (ownerCount == 0)
                     {
-                        // Allow some slop for rounding IOUs
-
-                        // TODO: This needs to be an exact match once all the
-                        // other rounding issues are worked out.
+                        // The Vault must be perfectly balanced if there
+                        // are no loans outstanding
                         auto const total = vaultSle->at(sfAssetsTotal);
                         auto const available = vaultSle->at(sfAssetsAvailable);
-                        env.test.BEAST_EXPECT(
-                            total == available ||
-                            (!broker.asset.integral() && available != 0 &&
-                             ((total - available) / available < Number(1, -6))));
+                        env.test.BEAST_EXPECT(total == available);
                         env.test.BEAST_EXPECT(vaultSle->at(sfLossUnrealized) == 0);
                     }
                 }
@@ -7067,6 +7062,129 @@ protected:
         BEAST_EXPECT(afterSecondCoverAvailable == 0);
     }
 
+    void
+    testYieldTheftRounding(std::uint32_t flags)
+    {
+        testcase("Yield Theft via Rounding Manipulation");
+        using namespace jtx;
+        using namespace loan;
+
+        // 1. Setup Environment
+        Env env(*this, all);
+        Account const issuer{"issuer"};
+        Account const lender{"lender"};
+        Account const borrower{"borrower"};
+
+        env.fund(XRP(1000), issuer, lender, borrower);
+        env.close();
+
+        // 2. Asset Selection
+        PrettyAsset const iou = issuer["USD"];
+        env(trust(lender, iou(100'000'000)));
+        env(trust(borrower, iou(100'000'000)));
+        env(pay(issuer, lender, iou(100'000'000)));
+        env(pay(issuer, borrower, iou(100'000'000)));
+        env.close();
+
+        // 3. Create Vault and Broker with High Debt Limit (100M)
+        auto const brokerInfo = createVaultAndBroker(
+            env,
+            iou,
+            lender,
+            {
+                .vaultDeposit = 5'000'000,
+                .debtMax = Number{100'000'000},
+                .coverDeposit = 500'000,
+            });
+        auto const [currentSeq, vaultId, vaultKeylet] = [&]() {
+            auto const brokerSle = env.le(keylet::loanbroker(brokerInfo.brokerID));
+            auto const currentSeq = brokerSle->at(sfLoanSequence);
+            auto const vaultKeylet = keylet::vault(brokerSle->at(sfVaultID));
+            auto const vaultId = brokerSle->at(sfVaultID);
+            return std::make_tuple(currentSeq, vaultId, vaultKeylet);
+        }();
+
+        // 4. Loan Parameters (Attack Vector)
+        Number const principal = 1'000'000;
+        TenthBips32 const interestRate = TenthBips32{1};  // 0.001%
+        std::uint32_t const paymentInterval = 86400;
+        std::uint32_t const paymentTotal = 3650;
+
+        auto const loanSetFee = fee(env.current()->fees().base * 2);
+        env(set(borrower, brokerInfo.brokerID, iou(principal).value(), flags),
+            sig(sfCounterpartySignature, lender),
+            loan::interestRate(interestRate),
+            loan::paymentInterval(paymentInterval),
+            loan::paymentTotal(paymentTotal),
+            fee(loanSetFee));
+        env.close();
+
+        // --- RETRIEVE OBJECTS & SETUP ATTACK ---
+
+        auto borrowerBalance = [&]() { return env.balance(borrower, iou); };
+        auto const borrowerScale = static_cast<STAmount const&>(borrowerBalance()).exponent();
+
+        auto const loanKeylet = keylet::loan(brokerInfo.brokerID, currentSeq);
+        auto const [periodicPayment, loanScale] = [&]() {
+            auto const loanSle = env.le(loanKeylet);
+            // Construct Payment
+            return std::make_tuple(
+                STAmount{iou, loanSle->at(sfPeriodicPayment)}, loanSle->at(sfLoanScale));
+        }();
+        auto const roundedPayment = roundToScale(periodicPayment, borrowerScale, Number::upward);
+
+        // ATTACK: Add dust buffer (1e-9) to force 'excess' logic execution
+        STAmount const paymentBuffer{iou, Number(1, -9)};
+        STAmount const attackPayment = periodicPayment + paymentBuffer;
+
+        auto const initialVaultAssets = env.le(vaultKeylet)->at(sfAssetsTotal);
+
+        // 5. Execution Loop
+        int yieldTheftCount = 0;
+        auto previousAssetsTotal = initialVaultAssets;
+
+        for (int i = 0; i < 100; ++i)
+        {
+            auto const balanceBefore = borrowerBalance();
+            env(pay(borrower, loanKeylet.key, attackPayment, flags));
+            env.close();
+            auto const borrowerDelta = balanceBefore - borrowerBalance();
+            BEAST_EXPECT(borrowerDelta.signum() == roundedPayment.signum());
+
+            auto const loanSle = env.le(loanKeylet);
+            if (!BEAST_EXPECT(loanSle))
+                break;
+            auto const updatedPayment = STAmount{iou, loanSle->at(sfPeriodicPayment)};
+            BEAST_EXPECT(
+                (roundToScale(updatedPayment, borrowerScale, Number::upward) == roundedPayment));
+            BEAST_EXPECT(
+                (updatedPayment == periodicPayment) ||
+                (flags == tfLoanOverpayment && i >= 2 && updatedPayment < periodicPayment));
+
+            auto const currentVaultSle = env.le(vaultKeylet);
+            if (!BEAST_EXPECT(currentVaultSle))
+                break;
+
+            auto const currentAssetsTotal = currentVaultSle->at(sfAssetsTotal);
+            auto const delta = currentAssetsTotal - previousAssetsTotal;
+
+            BEAST_EXPECT(
+                (delta == beast::zero && borrowerDelta <= roundedPayment) ||
+                (delta > beast::zero && borrowerDelta > roundedPayment));
+
+            // If tx succeeded but Assets Total didn't change, interest was
+            // stolen.
+            if (delta == beast::zero && borrowerDelta > roundedPayment)
+            {
+                yieldTheftCount++;
+            }
+
+            previousAssetsTotal = currentAssetsTotal;
+        }
+
+        BEAST_EXPECTS(yieldTheftCount == 0, std::to_string(yieldTheftCount));
+    }
+
 public:
     void
     run() override
@@ -7075,6 +7193,11 @@ public:
         testLoanPayLateFullPaymentBypassesPenalties();
         testLoanCoverMinimumRoundingExploit();
 #endif
+        for (auto const flags : {0u, tfLoanOverpayment})
+        {
+            testYieldTheftRounding(flags);
+        }
+
         testInvalidLoanSet();
 
         auto const all = jtx::testable_amendments();

src/test/app/SHAMapStore_test.cpp

@@ -1,9 +1,7 @@
 #include <test/jtx/Env.h>
 #include <test/jtx/amount.h>
 #include <test/jtx/envconfig.h>
-#include <test/jtx/noop.h>
 
-#include <xrpld/app/ledger/LedgerMaster.h>
 #include <xrpld/app/main/Application.h>
 #include <xrpld/app/main/NodeStoreScheduler.h>
 #include <xrpld/app/misc/SHAMapStore.h>
@@ -31,9 +29,7 @@
 #include <map>
 #include <memory>
 #include <optional>
-#include <sstream>
 #include <string>
-#include <thread>
 #include <utility>
 
 namespace xrpl {
@@ -46,8 +42,10 @@ class SHAMapStore_test : public beast::unit_test::suite
     static auto
     onlineDelete(std::unique_ptr<Config> cfg)
     {
-        using namespace jtx;
-        return online_delete(std::move(cfg), deleteInterval);
+        cfg->LEDGER_HISTORY = deleteInterval;
+        auto& section = cfg->section(ConfigSection::nodeDatabase());
+        section.set("online_delete", std::to_string(deleteInterval));
+        return cfg;
     }
 
     static auto
@@ -587,160 +585,6 @@ public:
         BEAST_EXPECT(dbr->getName() == "3");
     }
 
-    void
-    testLedgerGaps()
-    {
-        // Note that this test is intentionally very similar to
-        // LedgerMaster_test::testCompleteLedgerRange, but has a different
-        // focus.
-
-        testcase("Wait for ledger gaps to fill in");
-
-        using namespace test::jtx;
-
-        Env env{*this, envconfig(onlineDelete)};
-
-        auto failureMessage = [&](char const* label, auto expected, auto actual) {
-            std::stringstream ss;
-            ss << label << ": Expected: " << expected << ", Got: " << actual;
-            return ss.str();
-        };
-
-        auto const alice = Account("alice");
-        env.fund(XRP(1000), alice);
-        env.close();
-
-        auto& lm = env.app().getLedgerMaster();
-        LedgerIndex minSeq = 2;
-        LedgerIndex maxSeq = env.closed()->header().seq;
-        auto& store = env.app().getSHAMapStore();
-        store.rendezvous();
-        LedgerIndex lastRotated = store.getLastRotated();
-        BEAST_EXPECTS(maxSeq == 3, to_string(maxSeq));
-        BEAST_EXPECTS(lm.getCompleteLedgers() == "2-3", lm.getCompleteLedgers());
-        BEAST_EXPECTS(lastRotated == 3, to_string(lastRotated));
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
-        BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
-
-        // Close enough ledgers to rotate a few times
-        while (maxSeq < 20)
-        {
-            for (int t = 0; t < 3; ++t)
-            {
-                env(noop(alice));
-            }
-            env.close();
-            store.rendezvous();
-
-            ++maxSeq;
-
-            if (maxSeq + 1 == lastRotated + deleteInterval)
-            {
-                using namespace std::chrono_literals;
-
-                // The next ledger will trigger a rotation. Delete the
-                // current ledger from LedgerMaster.
-                std::this_thread::sleep_for(100ms);
-                LedgerIndex const deleteSeq = maxSeq;
-                while (!lm.haveLedger(deleteSeq))
-                {
-                    std::this_thread::sleep_for(100ms);
-                }
-                lm.clearLedger(deleteSeq);
-
-                auto expectedRange = [](auto minSeq, auto deleteSeq, auto maxSeq) {
-                    std::stringstream expectedRange;
-                    expectedRange << minSeq << "-" << (deleteSeq - 1);
-                    if (deleteSeq + 1 == maxSeq)
-                    {
-                        expectedRange << "," << maxSeq;
-                    }
-                    else if (deleteSeq < maxSeq)
-                    {
-                        expectedRange << "," << (deleteSeq + 1) << "-" << maxSeq;
-                    }
-                    return expectedRange.str();
-                };
-                BEAST_EXPECTS(
-                    lm.getCompleteLedgers() == expectedRange(minSeq, deleteSeq, maxSeq),
-                    failureMessage(
-                        "Complete ledgers",
-                        expectedRange(minSeq, deleteSeq, maxSeq),
-                        lm.getCompleteLedgers()));
-                BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 1);
-
-                // Close another ledger, which will trigger a rotation, but the
-                // rotation will be stuck until the missing ledger is filled in.
-                env.close();
-                // DO NOT CALL rendezvous()! You'll end up with a deadlock.
-                ++maxSeq;
-
-                // Nothing has changed
-                BEAST_EXPECTS(
-                    store.getLastRotated() == lastRotated,
-                    failureMessage("lastRotated", lastRotated, store.getLastRotated()));
-                BEAST_EXPECTS(
-                    lm.getCompleteLedgers() == expectedRange(minSeq, deleteSeq, maxSeq),
-                    failureMessage(
-                        "Complete ledgers",
-                        expectedRange(minSeq, deleteSeq, maxSeq),
-                        lm.getCompleteLedgers()));
-
-                // Close 5 more ledgers, waiting one second in between to
-                // simulate the ledger making progress while online delete waits
-                // for the missing ledger to be filled in.
-                // This ensures the healthWait check has time to run and
-                // detect the gap.
-                for (int l = 0; l < 5; ++l)
-                {
-                    env.close();
-                    // DO NOT CALL rendezvous()! You'll end up with a deadlock.
-                    ++maxSeq;
-                    // Nothing has changed
-                    BEAST_EXPECTS(
-                        store.getLastRotated() == lastRotated,
-                        failureMessage("lastRotated", lastRotated, store.getLastRotated()));
-                    BEAST_EXPECTS(
-                        lm.getCompleteLedgers() == expectedRange(minSeq, deleteSeq, maxSeq),
-                        failureMessage(
-                            "Complete Ledgers",
-                            expectedRange(minSeq, deleteSeq, maxSeq),
-                            lm.getCompleteLedgers()));
-                    std::this_thread::sleep_for(1s);
-                }
-
-                // Put the missing ledger back in LedgerMaster
-                lm.setLedgerRangePresent(deleteSeq, deleteSeq);
-
-                // Wait for the rotation to finish
-                store.rendezvous();
-
-                minSeq = lastRotated;
-                lastRotated = deleteSeq + 1;
-            }
-            BEAST_EXPECT(maxSeq != lastRotated + deleteInterval);
-            BEAST_EXPECTS(
-                env.closed()->header().seq == maxSeq,
-                failureMessage("maxSeq", maxSeq, env.closed()->header().seq));
-            BEAST_EXPECTS(
-                store.getLastRotated() == lastRotated,
-                failureMessage("lastRotated", lastRotated, store.getLastRotated()));
-            std::stringstream expectedRange;
-            expectedRange << minSeq << "-" << maxSeq;
-            BEAST_EXPECTS(
-                lm.getCompleteLedgers() == expectedRange.str(),
-                failureMessage("CompleteLedgers", expectedRange.str(), lm.getCompleteLedgers()));
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq, maxSeq) == 0);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 1, maxSeq - 1) == 0);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 1, maxSeq + 1) == 2);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq - 2, maxSeq - 2) == 2);
-            BEAST_EXPECT(lm.missingFromCompleteLedgerRange(minSeq + 2, maxSeq + 2) == 2);
-        }
-    }
-
     void
     run() override
     {
@@ -748,7 +592,6 @@ public:
         testAutomatic();
         testCanDelete();
         testRotate();
-        testLedgerGaps();
     }
 };
 

src/test/jtx/envconfig.h

@@ -57,17 +57,6 @@ envconfig(F&& modfunc, Args&&... args)
     return modfunc(envconfig(), std::forward<Args>(args)...);
 }
 
-/// @brief adjust config to enable online_delete
-///
-/// @param cfg config instance to be modified
-///
-/// @param deleteInterval how many new ledgers should be available before
-/// rotating. Defaults to 8, because the standalone minimum is 8.
-///
-/// @return unique_ptr to Config instance
-std::unique_ptr<Config>
-online_delete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval = 8);
-
 /// @brief adjust config so no admin ports are enabled
 ///
 /// this is intended for use with envconfig, as in

src/test/jtx/impl/envconfig.cpp

@@ -6,10 +6,8 @@
 #include <xrpld/core/ConfigSections.h>
 
 #include <atomic>
-#include <cstdint>
 #include <map>
 #include <memory>
-#include <string>
 #include <vector>
 
 namespace xrpl {
@@ -62,15 +60,6 @@ setupConfigForUnitTests(Config& cfg)
 
 namespace jtx {
 
-std::unique_ptr<Config>
-online_delete(std::unique_ptr<Config> cfg, std::uint32_t deleteInterval)
-{
-    cfg->LEDGER_HISTORY = deleteInterval;
-    auto& section = cfg->section(ConfigSection::nodeDatabase());
-    section.set("online_delete", std::to_string(deleteInterval));
-    return cfg;
-}
-
 std::unique_ptr<Config>
 no_admin(std::unique_ptr<Config> cfg)
 {

src/xrpld/app/ledger/LedgerMaster.h

@@ -105,10 +105,7 @@ public:
     failedSave(std::uint32_t seq, uint256 const& hash);
 
     std::string
-    getCompleteLedgers() const;
-
-    std::size_t
-    missingFromCompleteLedgerRange(LedgerIndex first, LedgerIndex last) const;
+    getCompleteLedgers();
 
     /** Apply held transactions to the open ledger
         This is normally called as we close the ledger.
@@ -325,7 +322,7 @@ private:
     // A set of transactions to replay during the next close
     std::unique_ptr<LedgerReplay> replayData;
 
-    std::recursive_mutex mutable mCompleteLock;
+    std::recursive_mutex mCompleteLock;
     RangeSet<std::uint32_t> mCompleteLedgers;
 
     // Publish thread is running.

src/xrpld/app/ledger/detail/LedgerMaster.cpp

@@ -1570,34 +1570,12 @@ LedgerMaster::getPublishedLedger()
 }
 
 std::string
-LedgerMaster::getCompleteLedgers() const
+LedgerMaster::getCompleteLedgers()
 {
     std::lock_guard const sl(mCompleteLock);
     return to_string(mCompleteLedgers);
 }
 
-std::size_t
-LedgerMaster::missingFromCompleteLedgerRange(LedgerIndex first, LedgerIndex last) const
-{
-    // Make a copy of the range to avoid holding the lock
-    auto const range = [&] {
-        std::lock_guard const sl(mCompleteLock);
-        return mCompleteLedgers;
-    }();
-
-    std::size_t missing = 0;
-
-    for (LedgerIndex idx = first; idx <= last; ++idx)
-    {
-        if (!boost::icl::contains(range, idx))
-        {
-            ++missing;
-        }
-    }
-
-    return missing;
-}
-
 std::optional<NetClock::time_point>
 LedgerMaster::getCloseTimeBySeq(LedgerIndex ledgerIndex)
 {

src/xrpld/app/misc/SHAMapStoreImp.cpp

@@ -292,17 +292,6 @@ SHAMapStoreImp::run()
         bool const readyToRotate = validatedSeq >= lastRotated + deleteInterval_ &&
             canDelete_ >= lastRotated - 1 && healthWait() == keepGoing;
 
-        JLOG(journal_.debug()) << "run: Setting lastGoodValidatedLedger_ to " << validatedSeq;
-
-        {
-            // Note that this is set after the healthWait() check, so that we
-            // don't start the rotation until the validated ledger is fully
-            // processed. It is not guaranteed to be done at this point. It also
-            // allows the testLedgerGaps unit test to work.
-            std::unique_lock<std::mutex> const lock(mutex_);
-            lastGoodValidatedLedger_ = validatedSeq;
-        }
-
         // will delete up to (not including) lastRotated
         if (readyToRotate)
         {
@@ -310,8 +299,7 @@ SHAMapStoreImp::run()
                                   << lastRotated << " deleteInterval " << deleteInterval_
                                   << " canDelete_ " << canDelete_ << " state "
                                   << app_.getOPs().strOperatingMode(false) << " age "
-                                  << ledgerMaster_->getValidatedLedgerAge().count()
-                                  << "s. Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
+                                  << ledgerMaster_->getValidatedLedgerAge().count() << 's';
 
             clearPrior(lastRotated);
             if (healthWait() == stopping)
@@ -371,9 +359,7 @@ SHAMapStoreImp::run()
                     clearCaches(validatedSeq);
                 });
 
-            JLOG(journal_.warn()) << "finished rotation. validatedSeq: " << validatedSeq
-                                  << ", lastRotated: " << lastRotated
-                                  << ". Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
+            JLOG(journal_.warn()) << "finished rotation " << validatedSeq;
         }
     }
 }
@@ -613,41 +599,22 @@ SHAMapStoreImp::clearPrior(LedgerIndex lastRotated)
 SHAMapStoreImp::HealthResult
 SHAMapStoreImp::healthWait()
 {
-    auto index = ledgerMaster_->getValidLedgerIndex();
     auto age = ledgerMaster_->getValidatedLedgerAge();
     OperatingMode mode = netOPs_->getOperatingMode();
     std::unique_lock lock(mutex_);
-
-    auto numMissing =
-        ledgerMaster_->missingFromCompleteLedgerRange(lastGoodValidatedLedger_, index);
-    while (!stop_ && (mode != OperatingMode::FULL || age > ageThreshold_ || numMissing > 0))
+    while (!stop_ && (mode != OperatingMode::FULL || age > ageThreshold_))
     {
-        // this value shouldn't change, so grab it while we have the
-        // lock
-        auto const lowerBound = lastGoodValidatedLedger_;
-
         lock.unlock();
-
-        auto const stream =
-            mode != OperatingMode::FULL || age > ageThreshold_ ? journal_.warn() : journal_.info();
-        JLOG(stream) << "Waiting " << recoveryWaitTime_.count()
-                     << "s for node to stabilize. state: "
-                     << app_.getOPs().strOperatingMode(mode, false) << ". age " << age.count()
-                     << "s. Missing ledgers: " << numMissing << ".  Expect: " << lowerBound << "-"
-                     << index << ". Complete ledgers: " << ledgerMaster_->getCompleteLedgers();
+        JLOG(journal_.warn()) << "Waiting " << recoveryWaitTime_.count()
+                              << "s for node to stabilize. state: "
+                              << app_.getOPs().strOperatingMode(mode, false) << ". age "
+                              << age.count() << 's';
         std::this_thread::sleep_for(recoveryWaitTime_);
-        index = ledgerMaster_->getValidLedgerIndex();
         age = ledgerMaster_->getValidatedLedgerAge();
         mode = netOPs_->getOperatingMode();
-        numMissing =
-            lowerBound == 0 ? 0 : ledgerMaster_->missingFromCompleteLedgerRange(lowerBound, index);
-
         lock.lock();
     }
 
-    JLOG(journal_.debug()) << "healthWait: Setting lastGoodValidatedLedger_ to " << index;
-    lastGoodValidatedLedger_ = index;
-
     return stop_ ? stopping : keepGoing;
 }
 

src/xrpld/app/misc/SHAMapStoreImp.h

@@ -70,11 +70,6 @@ private:
     std::thread thread_;
     bool stop_ = false;
     bool healthy_ = true;
-    // Used to prevent ledger gaps from forming during online deletion. Keeps
-    // track of the last validated ledger that was processed without gaps. There
-    // are no guarantees about gaps while online delete is not running. For
-    // that, use advisory_delete and check for gaps externally.
-    LedgerIndex lastGoodValidatedLedger_ = 0;
     mutable std::condition_variable cond_;
     mutable std::condition_variable rendezvous_;
     mutable std::mutex mutex_;
@@ -88,11 +83,11 @@ private:
     std::uint32_t deleteBatch_ = 100;
     std::chrono::milliseconds backOff_{100};
     std::chrono::seconds ageThreshold_{60};
-    /// If  the node is out of sync, or any recent ledgers are not
-    /// available during an online_delete healthWait() call, sleep
-    /// the thread for this time, and continue checking until recovery.
+    /// If  the node is out of sync during an online_delete healthWait()
+    /// call, sleep the thread for this time, and continue checking until
+    /// recovery.
     /// See also: "recovery_wait_seconds" in xrpld-example.cfg
-    std::chrono::seconds recoveryWaitTime_{1};
+    std::chrono::seconds recoveryWaitTime_{5};
 
     // these do not exist upon SHAMapStore creation, but do exist
     // as of run() or before
@@ -209,8 +204,6 @@ private:
     enum HealthResult { stopping, keepGoing };
     [[nodiscard]] HealthResult
     healthWait();
-    bool
-    hasCompleteRange(LedgerIndex first, LedgerIndex last);
 
 public:
     void