Merge branch 'develop' into ximinez/acquireAsyncDispatch

* XRPLF/develop:
  chore: Add -fix to clang-tidy invocation (6990)
  chore: Remove empty Taker.h (6984)
  chore: Enable clang-tidy modernize checks (6975)
  ci: Upload clang-tidy git diff (6983)
  fix: Add rounding to Vault invariants (6217) (6955)

include/xrpl/basics/CanProcess.h

@@ -0,0 +1,139 @@
+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2024 Ripple Labs Inc.
+
+    Permission to use, copy, modify, and/or distribute this software for any
+    purpose  with  or without fee is hereby granted, provided that the above
+    copyright notice and this permission notice appear in all copies.
+
+    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
+    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
+    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+//==============================================================================
+
+#ifndef RIPPLE_BASICS_CANPROCESS_H_INCLUDED
+#define RIPPLE_BASICS_CANPROCESS_H_INCLUDED
+
+#include <functional>
+#include <mutex>
+#include <set>
+
+/** RAII class to check if an Item is already being processed on another thread,
+ * as indicated by it's presence in a Collection.
+ *
+ * If the Item is not in the Collection, it will be added under lock in the
+ * ctor, and removed under lock in the dtor. The object will be considered
+ * "usable" and evaluate to `true`.
+ *
+ * If the Item is in the Collection, no changes will be made to the collection,
+ * and the CanProcess object will be considered "unusable".
+ *
+ * It's up to the caller to decide what "usable" and "unusable" mean. (e.g.
+ * Process or skip a block of code, or set a flag.)
+ *
+ * The current use is to avoid lock contention that would be involved in
+ * processing something associated with the Item.
+ *
+ * Examples:
+ *
+ * void IncomingLedgers::acquireAsync(LedgerHash const& hash, ...)
+ * {
+ *    if (CanProcess check{acquiresMutex_, pendingAcquires_, hash})
+ *    {
+ *      acquire(hash, ...);
+ *    }
+ * }
+ *
+ * bool
+ * NetworkOPsImp::recvValidation(
+ *   std::shared_ptr<STValidation> const& val,
+ *   std::string const& source)
+ *   {
+ *       CanProcess check(
+ *           validationsMutex_, pendingValidations_, val->getLedgerHash());
+ *       BypassAccept bypassAccept =
+ *           check ? BypassAccept::no : BypassAccept::yes;
+ *       handleNewValidation(app_, val, source, bypassAccept, m_journal);
+ *   }
+ *
+ */
+class CanProcess
+{
+public:
+    template <class Mutex, class Collection, class Item>
+    CanProcess(Mutex& mtx, Collection& collection, Item const& item)
+        : cleanup_(insert(mtx, collection, item))
+    {
+    }
+
+    ~CanProcess()
+    {
+        if (cleanup_)
+            cleanup_();
+    }
+
+    CanProcess(CanProcess const&) = delete;
+
+    CanProcess&
+    operator=(CanProcess const&) = delete;
+
+    explicit
+    operator bool() const
+    {
+        return static_cast<bool>(cleanup_);
+    }
+
+private:
+    template <bool useIterator, class Mutex, class Collection, class Item>
+    std::function<void()>
+    doInsert(Mutex& mtx, Collection& collection, Item const& item)
+    {
+        std::unique_lock<Mutex> lock(mtx);
+        // TODO: Use structured binding once LLVM 16 is the minimum supported
+        // version. See also: https://github.com/llvm/llvm-project/issues/48582
+        // https://github.com/llvm/llvm-project/commit/127bf44385424891eb04cff8e52d3f157fc2cb7c
+        auto const insertResult = collection.insert(item);
+        auto const it = insertResult.first;
+        if (!insertResult.second)
+            return {};
+        if constexpr (useIterator)
+            return [&, it]() {
+                std::unique_lock<Mutex> lock(mtx);
+                collection.erase(it);
+            };
+        else
+            return [&]() {
+                std::unique_lock<Mutex> lock(mtx);
+                collection.erase(item);
+            };
+    }
+
+    // Generic insert() function doesn't use iterators because they may get
+    // invalidated
+    template <class Mutex, class Collection, class Item>
+    std::function<void()>
+    insert(Mutex& mtx, Collection& collection, Item const& item)
+    {
+        return doInsert<false>(mtx, collection, item);
+    }
+
+    // Specialize insert() for std::set, which does not invalidate iterators for
+    // insert and erase
+    template <class Mutex, class Item>
+    std::function<void()>
+    insert(Mutex& mtx, std::set<Item>& collection, Item const& item)
+    {
+        return doInsert<true>(mtx, collection, item);
+    }
+
+    // If set, then the item is "usable"
+    std::function<void()> cleanup_;
+};
+
+#endif

include/xrpl/core/HashRouter.h

@@ -197,7 +197,7 @@ public:
 
     /** Add a suppression peer and get message's relay status.
      * Return pair:
-     * element 1: true if the peer is added.
+     * element 1: true if the key is added.
      * element 2: optional is seated to the relay time point or
      * is unseated if has not relayed yet. */
     std::pair<bool, std::optional<Stopwatch::time_point>>

include/xrpl/protocol/LedgerHeader.h

@@ -35,6 +35,8 @@ struct LedgerHeader
 
     // If validated is false, it means "not yet validated."
     // Once validated is true, it will never be set false at a later time.
+    // NOTE: If you are accessing this directly, you are probably doing it
+    //   wrong. Use LedgerMaster::isValidated().
     // VFALCO TODO Make this not mutable
     bool mutable validated = false;
     bool accepted = false;

include/xrpl/server/NetworkOPs.h

@@ -185,7 +185,7 @@ public:
     virtual bool
     isFull() = 0;
     virtual void
-    setMode(OperatingMode om) = 0;
+    setMode(OperatingMode om, char const* reason) = 0;
     virtual bool
     isBlocked() = 0;
     virtual bool

src/libxrpl/ledger/helpers/LendingHelpers.cpp

@@ -1857,18 +1857,8 @@ 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 < roundedAmount &&
+        paymentRemainingProxy > 0 && totalPaid < amount &&
         numPayments < loanMaximumPaymentsPerTransaction)
     {
         TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)};
@@ -1877,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(roundedAmount - totalPaid, *totalValueOutstandingProxy);
+        Number const overpayment = std::min(amount - totalPaid, *totalValueOutstandingProxy);
 
         detail::ExtendedPaymentComponents const overpaymentComponents =
             detail::computeOverpaymentComponents(

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

@@ -30,7 +30,6 @@
 #include <bit>
 #include <cstdint>
 #include <memory>
-#include <vector>
 
 namespace xrpl {
 
@@ -144,7 +143,6 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx)
     Number const numPaymentEstimate = static_cast<std::int64_t>(amount / regularPayment);
 
     // Charge one base fee per paymentsPerFeeIncrement payments, rounding up.
-    // This set round is safe because there's a mode guard just above
     Number::setround(Number::upward);
     auto const feeIncrements = std::max(
         std::int64_t(1),
@@ -442,10 +440,9 @@ 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,
@@ -469,6 +466,16 @@ 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
@@ -494,68 +501,12 @@ LoanPay::doApply()
     associateAsset(*vaultSle, asset);
 
     // Duplicate some checks after rounding
-    Number const assetsAvailableAfter = *assetsAvailableProxy;
-    Number const assetsTotalAfter = *assetsTotalProxy;
-
     XRPL_ASSERT_PARTS(
-        assetsAvailableAfter <= assetsTotalAfter,
+        *assetsAvailableProxy <= *assetsTotalProxy,
         "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
-    }
 
-    // These three values are used to check that funds are conserved after the transfers
+#if !NDEBUG
     auto const accountBalanceBefore = accountHolds(
         view,
         account_,
@@ -584,6 +535,7 @@ LoanPay::doApply()
               ahIGNORE_AUTH,
               j_,
               SpendableHandling::shFULL_BALANCE);
+#endif
 
     if (totalPaidToVaultRounded != beast::zero)
     {
@@ -619,22 +571,19 @@ LoanPay::doApply()
         return ter;
 
 #if !NDEBUG
-    {
-        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
+    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");
 
-    // Check that funds are conserved
     auto const accountBalanceAfter = accountHolds(
         view,
         account_,
@@ -663,118 +612,14 @@ 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::ranges::minmax_element(exponents);
-        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);
-    }();
-
-    // No object changes are made below this point
-    XRPL_ASSERT(Number::getround() == Number::to_nearest);
-    NumberRoundModeGuard mg(Number::to_nearest);
-
-    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(
-        totalBalanceBeforeRounded == totalBalanceAfterRounded ||
-            totalBalanceChangeRounded == beast::zero,
+        accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore ==
+            accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter,
         "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",
@@ -795,6 +640,7 @@ LoanPay::doApply()
         vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore,
         "xrpl::LoanPay::doApply",
         "vault and/or broker balance increased");
+#endif
 
     return tesSUCCESS;
 }

src/test/app/LedgerReplay_test.cpp

@@ -130,7 +130,12 @@ public:
     }
 
     void
-    acquireAsync(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason reason) override
+    acquireAsync(
+        JobType type,
+        std::string const& name,
+        uint256 const& hash,
+        std::uint32_t seq,
+        InboundLedger::Reason reason) override
     {
     }
 

src/test/app/Loan_test.cpp

@@ -370,11 +370,16 @@ protected:
                         env.balance(vaultPseudo, broker.asset).number());
                     if (ownerCount == 0)
                     {
-                        // The Vault must be perfectly balanced if there
-                        // are no loans outstanding
+                        // Allow some slop for rounding IOUs
+
+                        // TODO: This needs to be an exact match once all the
+                        // other rounding issues are worked out.
                         auto const total = vaultSle->at(sfAssetsTotal);
                         auto const available = vaultSle->at(sfAssetsAvailable);
-                        env.test.BEAST_EXPECT(total == available);
+                        env.test.BEAST_EXPECT(
+                            total == available ||
+                            (!broker.asset.integral() && available != 0 &&
+                             ((total - available) / available < Number(1, -6))));
                         env.test.BEAST_EXPECT(vaultSle->at(sfLossUnrealized) == 0);
                     }
                 }
@@ -7063,140 +7068,6 @@ protected:
         BEAST_EXPECT(afterSecondCoverAvailable == 0);
     }
 
-    void
-    testYieldTheftRounding(std::uint32_t flags)
-    {
-        testcase("Rounding manipulation does not permit yield theft");
-        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, vaultKeylet] = [&]() {
-            auto const brokerSle = env.le(keylet::loanbroker(brokerInfo.brokerID));
-            auto const currentSeq = brokerSle->at(sfLoanSequence);
-            auto const vaultKeylet = keylet::vault(brokerSle->at(sfVaultID));
-            return std::make_tuple(currentSeq, 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 maybePeriodicPayment = [&]() -> std::optional<STAmount> {
-            auto const loanSle = env.le(loanKeylet);
-            if (!BEAST_EXPECT(loanSle))
-                return std::nullopt;
-            // Construct Payment
-            return STAmount{iou, loanSle->at(sfPeriodicPayment)};
-        }();
-        if (!maybePeriodicPayment)
-            return;
-        auto const periodicPayment = *maybePeriodicPayment;
-        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 maybeInitialVaultAssets = [&]() -> std::optional<Number> {
-            auto const vault = env.le(vaultKeylet);
-            if (!BEAST_EXPECT(vault))
-                return std::nullopt;
-            return vault->at(sfAssetsTotal);
-        }();
-        if (!maybeInitialVaultAssets)
-            return;
-        auto const initialVaultAssets = *maybeInitialVaultAssets;
-
-        // 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));
-    }
-
     // Tests that vault withdrawals work correctly when the vault has unrealized
     // loss from an impaired loan, ensuring the invariant check properly
     // accounts for the loss.
@@ -7335,11 +7206,6 @@ public:
         testLoanPayLateFullPaymentBypassesPenalties();
         testLoanCoverMinimumRoundingExploit();
 #endif
-        for (auto const flags : {0u, tfLoanOverpayment})
-        {
-            testYieldTheftRounding(flags);
-        }
-
         testWithdrawReflectsUnrealizedLoss();
         testInvalidLoanSet();
 

src/test/basics/CanProcess_test.cpp

@@ -0,0 +1,165 @@
+//------------------------------------------------------------------------------
+/*
+    This file is part of rippled: https://github.com/ripple/rippled
+    Copyright (c) 2012-2016 Ripple Labs Inc.
+
+    Permission to use, copy, modify, and/or distribute this software for any
+    purpose  with  or without fee is hereby granted, provided that the above
+    copyright notice and this permission notice appear in all copies.
+
+    THE  SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+    WITH  REGARD  TO  THIS  SOFTWARE  INCLUDING  ALL  IMPLIED  WARRANTIES  OF
+    MERCHANTABILITY  AND  FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+    ANY  SPECIAL ,  DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+    WHATSOEVER  RESULTING  FROM  LOSS  OF USE, DATA OR PROFITS, WHETHER IN AN
+    ACTION  OF  CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+*/
+//==============================================================================
+
+#include <xrpl/basics/CanProcess.h>
+#include <xrpl/beast/unit_test.h>
+
+#include <memory>
+
+namespace ripple {
+namespace test {
+
+struct CanProcess_test : beast::unit_test::suite
+{
+    template <class Mutex, class Collection, class Item>
+    void
+    test(
+        std::string const& name,
+        Mutex& mtx,
+        Collection& collection,
+        std::vector<Item> const& items)
+    {
+        testcase(name);
+
+        if (!BEAST_EXPECT(!items.empty()))
+            return;
+        if (!BEAST_EXPECT(collection.empty()))
+            return;
+
+        // CanProcess objects can't be copied or moved. To make that easier,
+        // store shared_ptrs
+        std::vector<std::shared_ptr<CanProcess>> trackers;
+        // Fill up the vector with two CanProcess for each Item. The first
+        // inserts the item into the collection and is "good". The second does
+        // not and is "bad".
+        for (int i = 0; i < items.size(); ++i)
+        {
+            {
+                auto const& good =
+                    trackers.emplace_back(std::make_shared<CanProcess>(mtx, collection, items[i]));
+                BEAST_EXPECT(*good);
+            }
+            BEAST_EXPECT(trackers.size() == (2 * i) + 1);
+            BEAST_EXPECT(collection.size() == i + 1);
+            {
+                auto const& bad =
+                    trackers.emplace_back(std::make_shared<CanProcess>(mtx, collection, items[i]));
+                BEAST_EXPECT(!*bad);
+            }
+            BEAST_EXPECT(trackers.size() == 2 * (i + 1));
+            BEAST_EXPECT(collection.size() == i + 1);
+        }
+        BEAST_EXPECT(collection.size() == items.size());
+        // Now remove the items from the vector<CanProcess> two at a time, and
+        // try to get another CanProcess for that item.
+        for (int i = 0; i < items.size(); ++i)
+        {
+            // Remove the "bad" one in the second position
+            // This will have no effect on the collection
+            {
+                auto const iter = trackers.begin() + 1;
+                BEAST_EXPECT(!**iter);
+                trackers.erase(iter);
+            }
+            BEAST_EXPECT(trackers.size() == (2 * items.size()) - 1);
+            BEAST_EXPECT(collection.size() == items.size());
+            {
+                // Append a new "bad" one
+                auto const& bad =
+                    trackers.emplace_back(std::make_shared<CanProcess>(mtx, collection, items[i]));
+                BEAST_EXPECT(!*bad);
+            }
+            BEAST_EXPECT(trackers.size() == 2 * items.size());
+            BEAST_EXPECT(collection.size() == items.size());
+
+            // Remove the "good" one from the front
+            {
+                auto const iter = trackers.begin();
+                BEAST_EXPECT(**iter);
+                trackers.erase(iter);
+            }
+            BEAST_EXPECT(trackers.size() == (2 * items.size()) - 1);
+            BEAST_EXPECT(collection.size() == items.size() - 1);
+            {
+                // Append a new "good" one
+                auto const& good =
+                    trackers.emplace_back(std::make_shared<CanProcess>(mtx, collection, items[i]));
+                BEAST_EXPECT(*good);
+            }
+            BEAST_EXPECT(trackers.size() == 2 * items.size());
+            BEAST_EXPECT(collection.size() == items.size());
+        }
+        // Now remove them all two at a time
+        for (int i = items.size() - 1; i >= 0; --i)
+        {
+            // Remove the "bad" one from the front
+            {
+                auto const iter = trackers.begin();
+                BEAST_EXPECT(!**iter);
+                trackers.erase(iter);
+            }
+            BEAST_EXPECT(trackers.size() == (2 * i) + 1);
+            BEAST_EXPECT(collection.size() == i + 1);
+            // Remove the "good" one now in front
+            {
+                auto const iter = trackers.begin();
+                BEAST_EXPECT(**iter);
+                trackers.erase(iter);
+            }
+            BEAST_EXPECT(trackers.size() == 2 * i);
+            BEAST_EXPECT(collection.size() == i);
+        }
+        BEAST_EXPECT(trackers.empty());
+        BEAST_EXPECT(collection.empty());
+    }
+
+    void
+    run() override
+    {
+        {
+            std::mutex m;
+            std::set<int> collection;
+            std::vector<int> const items{1, 2, 3, 4, 5};
+            test("set of int", m, collection, items);
+        }
+        {
+            std::mutex m;
+            std::set<std::string> collection;
+            std::vector<std::string> const items{"one", "two", "three", "four", "five"};
+            test("set of string", m, collection, items);
+        }
+        {
+            std::mutex m;
+            std::unordered_set<char> collection;
+            std::vector<char> const items{'1', '2', '3', '4', '5'};
+            test("unorderd_set of char", m, collection, items);
+        }
+        {
+            std::mutex m;
+            std::unordered_set<std::uint64_t> collection;
+            std::vector<std::uint64_t> const items{100u, 1000u, 150u, 4u, 0u};
+            test("unordered_set of uint64_t", m, collection, items);
+        }
+    }
+};
+
+BEAST_DEFINE_TESTSUITE(CanProcess, ripple_basics, ripple);
+
+}  // namespace test
+}  // namespace ripple

src/xrpld/app/consensus/RCLConsensus.cpp

@@ -159,10 +159,8 @@ RCLConsensus::Adaptor::acquireLedger(LedgerHash const& hash)
             // Tell the ledger acquire system that we need the consensus ledger
             acquiringLedger_ = hash;
 
-            app_.getJobQueue().addJob(jtADVANCE, "GetConsL1", [id = hash, &app = app_, this]() {
-                JLOG(j_.debug()) << "JOB advanceLedger getConsensusLedger1 started";
-                app.getInboundLedgers().acquireAsync(id, 0, InboundLedger::Reason::CONSENSUS);
-            });
+            app_.getInboundLedgers().acquireAsync(
+                jtADVANCE, "GetConsL1", hash, 0, InboundLedger::Reason::CONSENSUS);
         }
         return std::nullopt;
     }
@@ -1052,7 +1050,7 @@ void
 RCLConsensus::Adaptor::updateOperatingMode(std::size_t const positions) const
 {
     if ((positions == 0u) && app_.getOPs().isFull())
-        app_.getOPs().setMode(OperatingMode::CONNECTED);
+        app_.getOPs().setMode(OperatingMode::CONNECTED, "updateOperatingMode: no positions");
 }
 
 void

src/xrpld/app/consensus/RCLValidations.cpp

@@ -128,12 +128,8 @@ RCLValidationsAdaptor::acquire(LedgerHash const& hash)
     {
         JLOG(j_.warn()) << "Need validated ledger for preferred ledger analysis " << hash;
 
-        Application* pApp = &app_;
-
-        app_.getJobQueue().addJob(jtADVANCE, "GetConsL2", [pApp, hash, this]() {
-            JLOG(j_.debug()) << "JOB advanceLedger getConsensusLedger2 started";
-            pApp->getInboundLedgers().acquireAsync(hash, 0, InboundLedger::Reason::CONSENSUS);
-        });
+        app_.getInboundLedgers().acquireAsync(
+            jtADVANCE, "GetConsL2", hash, 0, InboundLedger::Reason::CONSENSUS);
         return std::nullopt;
     }
 

src/xrpld/app/ledger/InboundLedgers.h

@@ -26,7 +26,12 @@ public:
     // Queue. TODO review whether all callers of acquire() can use this
     // instead. Inbound ledger acquisition is asynchronous anyway.
     virtual void
-    acquireAsync(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason reason) = 0;
+    acquireAsync(
+        JobType type,
+        std::string const& name,
+        uint256 const& hash,
+        std::uint32_t seq,
+        InboundLedger::Reason reason) = 0;
 
     virtual std::shared_ptr<InboundLedger>
     find(LedgerHash const& hash) = 0;

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

@@ -385,7 +385,14 @@ InboundLedger::onTimer(bool wasProgress, ScopedLockType&)
 
     if (!wasProgress)
     {
-        checkLocal();
+        if (checkLocal())
+        {
+            // Done. Something else (probably consensus) built the ledger
+            // locally while waiting for data (or possibly before requesting)
+            XRPL_ASSERT(isDone(), "ripple::InboundLedger::onTimer : done");
+            JLOG(journal_.info()) << "Finished while waiting " << hash_;
+            return;
+        }
 
         mByHash = true;
 

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

@@ -6,6 +6,7 @@
 #include <xrpld/overlay/PeerSet.h>
 
 #include <xrpl/basics/Blob.h>
+#include <xrpl/basics/CanProcess.h>
 #include <xrpl/basics/DecayingSample.h>
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Slice.h>
@@ -83,12 +84,15 @@ public:
                 (reason != InboundLedger::Reason::CONSENSUS))
                 return {};
 
+            std::stringstream ss;
+
             bool isNew = true;
             std::shared_ptr<InboundLedger> inbound;
             {
                 ScopedLockType sl(mLock);
                 if (stopping_)
                 {
+                    JLOG(j_.debug()) << "Abort(stopping): " << ss.str();
                     return {};
                 }
 
@@ -107,47 +111,61 @@ public:
                     ++mCounter;
                 }
             }
+            ss << " IsNew: " << (isNew ? "true" : "false");
 
             if (inbound->isFailed())
+            {
+                JLOG(j_.debug()) << "Abort(failed): " << ss.str();
                 return {};
+            }
 
             if (!isNew)
                 inbound->update(seq);
 
             if (!inbound->isComplete())
+            {
+                JLOG(j_.debug()) << "InProgress: " << ss.str();
                 return {};
+            }
 
+            JLOG(j_.debug()) << "Complete: " << ss.str();
             return inbound->getLedger();
         };
         using namespace std::chrono_literals;
-        std::shared_ptr<Ledger const> ledger =
-            perf::measureDurationAndLog(doAcquire, "InboundLedgersImp::acquire", 500ms, j_);
-
-        return ledger;
+        return perf::measureDurationAndLog(doAcquire, "InboundLedgersImp::acquire", 500ms, j_);
     }
 
     void
-    acquireAsync(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason reason) override
+    acquireAsync(
+        JobType type,
+        std::string const& name,
+        uint256 const& hash,
+        std::uint32_t seq,
+        InboundLedger::Reason reason) override
     {
-        std::unique_lock lock(acquiresMutex_);
-        try
+        if (auto check = std::make_shared<CanProcess const>(acquiresMutex_, pendingAcquires_, hash);
+            *check)
         {
-            if (pendingAcquires_.contains(hash))
-                return;
-            pendingAcquires_.insert(hash);
-            scope_unlock const unlock(lock);
-            acquire(hash, seq, reason);
+            app_.getJobQueue().addJob(type, name, [check, name, hash, seq, reason, this]() {
+                JLOG(j_.debug()) << "JOB acquireAsync " << name << " started ";
+                try
+                {
+                    acquire(hash, seq, reason);
+                }
+                catch (std::exception const& e)
+                {
+                    JLOG(j_.warn()) << "Exception thrown for acquiring new "
+                                       "inbound ledger "
+                                    << hash << ": " << e.what();
+                }
+                catch (...)
+                {
+                    JLOG(j_.warn()) << "Unknown exception thrown for acquiring new "
+                                       "inbound ledger "
+                                    << hash;
+                }
+            });
         }
-        catch (std::exception const& e)
-        {
-            JLOG(j_.warn()) << "Exception thrown for acquiring new inbound ledger " << hash << ": "
-                            << e.what();
-        }
-        catch (...)
-        {
-            JLOG(j_.warn()) << "Unknown exception thrown for acquiring new inbound ledger " << hash;
-        }
-        pendingAcquires_.erase(hash);
     }
 
     std::shared_ptr<InboundLedger>

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

@@ -965,8 +965,9 @@ LedgerMaster::checkAccept(std::shared_ptr<Ledger const> const& ledger)
         return;
     }
 
-    JLOG(m_journal.info()) << "Advancing accepted ledger to " << ledger->header().seq
-                           << " with >= " << minVal << " validations";
+    JLOG(m_journal.info()) << "Advancing accepted ledger to " << ledger->header().seq << " ("
+                           << to_short_string(ledger->header().hash) << ") with >= " << minVal
+                           << " validations";
 
     ledger->setValidated();
     ledger->setFull();

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

@@ -25,7 +25,8 @@ TimeoutCounter::TimeoutCounter(
     QueueJobParameter&& jobParameter,
     beast::Journal journal)
     : app_(app)
-    , journal_(journal)
+    , sink_(journal, to_short_string(hash) + " ")
+    , journal_(sink_)
     , hash_(hash)
     , timerInterval_(interval)
     , queueJobParameter_(std::move(jobParameter))
@@ -41,6 +42,7 @@ TimeoutCounter::setTimer(ScopedLockType& sl)
 {
     if (isDone())
         return;
+    JLOG(journal_.debug()) << "Setting timer for " << timerInterval_.count() << "ms";
     timer_.expires_after(timerInterval_);
     timer_.async_wait([wptr = pmDowncast()](boost::system::error_code const& ec) {
         if (ec == boost::asio::error::operation_aborted)
@@ -48,6 +50,10 @@ TimeoutCounter::setTimer(ScopedLockType& sl)
 
         if (auto ptr = wptr.lock())
         {
+            JLOG(ptr->journal_.debug())
+                << "timer: ec: " << ec
+                << " (operation_aborted: " << boost::asio::error::operation_aborted << " - "
+                << (ec == boost::asio::error::operation_aborted ? "aborted" : "other") << ")";
             ScopedLockType sl(ptr->mtx_);
             ptr->queueJob(sl);
         }

src/xrpld/app/ledger/detail/TimeoutCounter.h

@@ -3,6 +3,7 @@
 #include <xrpld/app/main/Application.h>
 
 #include <xrpl/beast/utility/Journal.h>
+#include <xrpl/beast/utility/WrappedSink.h>
 #include <xrpl/core/Job.h>
 
 #include <boost/asio/basic_waitable_timer.hpp>
@@ -103,6 +104,7 @@ protected:
     // Used in this class for access to boost::asio::io_context and
     // xrpl::Overlay. Used in subtypes for the kitchen sink.
     Application& app_;
+    beast::WrappedSink sink_;
     beast::Journal journal_;
     mutable std::recursive_mutex mtx_;
 

src/xrpld/app/misc/NetworkOPs.cpp

@@ -35,6 +35,7 @@
 #include <xrpld/rpc/MPTokenIssuanceID.h>
 #include <xrpld/rpc/ServerHandler.h>
 
+#include <xrpl/basics/CanProcess.h>
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/ToString.h>
 #include <xrpl/basics/UnorderedContainers.h>
@@ -485,7 +486,7 @@ public:
     isFull() override;
 
     void
-    setMode(OperatingMode om) override;
+    setMode(OperatingMode om, char const* reason) override;
 
     bool
     isBlocked() override;
@@ -923,7 +924,7 @@ NetworkOPsImp::strOperatingMode(bool const admin /* = false */) const
 inline void
 NetworkOPsImp::setStandAlone()
 {
-    setMode(OperatingMode::FULL);
+    setMode(OperatingMode::FULL, "setStandAlone");
 }
 
 inline void
@@ -1066,7 +1067,7 @@ NetworkOPsImp::processHeartbeatTimer()
         {
             if (mMode != OperatingMode::DISCONNECTED)
             {
-                setMode(OperatingMode::DISCONNECTED);
+                setMode(OperatingMode::DISCONNECTED, "Heartbeat: insufficient peers");
                 std::stringstream ss;
                 ss << "Node count (" << numPeers << ") has fallen "
                    << "below required minimum (" << minPeerCount_ << ").";
@@ -1090,7 +1091,7 @@ NetworkOPsImp::processHeartbeatTimer()
 
         if (mMode == OperatingMode::DISCONNECTED)
         {
-            setMode(OperatingMode::CONNECTED);
+            setMode(OperatingMode::CONNECTED, "Heartbeat: sufficient peers");
             JLOG(m_journal.info()) << "Node count (" << numPeers << ") is sufficient.";
             CLOG(clog.ss()) << "setting mode to CONNECTED based on " << numPeers << " peers. ";
         }
@@ -1101,11 +1102,11 @@ NetworkOPsImp::processHeartbeatTimer()
         CLOG(clog.ss()) << "mode: " << strOperatingMode(origMode, true);
         if (mMode == OperatingMode::SYNCING)
         {
-            setMode(OperatingMode::SYNCING);
+            setMode(OperatingMode::SYNCING, "Heartbeat: check syncing");
         }
         else if (mMode == OperatingMode::CONNECTED)
         {
-            setMode(OperatingMode::CONNECTED);
+            setMode(OperatingMode::CONNECTED, "Heartbeat: check connected");
         }
         auto newMode = mMode.load();
         if (origMode != newMode)
@@ -1809,7 +1810,7 @@ void
 NetworkOPsImp::setAmendmentBlocked()
 {
     amendmentBlocked_ = true;
-    setMode(OperatingMode::CONNECTED);
+    setMode(OperatingMode::CONNECTED, "setAmendmentBlocked");
 }
 
 inline bool
@@ -1840,7 +1841,7 @@ void
 NetworkOPsImp::setUNLBlocked()
 {
     unlBlocked_ = true;
-    setMode(OperatingMode::CONNECTED);
+    setMode(OperatingMode::CONNECTED, "setUNLBlocked");
 }
 
 inline void
@@ -1940,7 +1941,7 @@ NetworkOPsImp::checkLastClosedLedger(Overlay::PeerSequence const& peerList, uint
 
     if ((mMode == OperatingMode::TRACKING) || (mMode == OperatingMode::FULL))
     {
-        setMode(OperatingMode::CONNECTED);
+        setMode(OperatingMode::CONNECTED, "check LCL: not on consensus ledger");
     }
 
     if (consensus)
@@ -2028,8 +2029,8 @@ NetworkOPsImp::beginConsensus(
         // this shouldn't happen unless we jump ledgers
         if (mMode == OperatingMode::FULL)
         {
-            JLOG(m_journal.warn()) << "Don't have LCL, going to tracking";
-            setMode(OperatingMode::TRACKING);
+            JLOG(m_journal.warn()) << "beginConsensus Don't have LCL, going to tracking";
+            setMode(OperatingMode::TRACKING, "beginConsensus: No LCL");
             CLOG(clog) << "beginConsensus Don't have LCL, going to tracking. ";
         }
 
@@ -2157,7 +2158,7 @@ NetworkOPsImp::endConsensus(std::unique_ptr<std::stringstream> const& clog)
         // validations we have for LCL.  If the ledger is good enough, go to
         // TRACKING - TODO
         if (!needNetworkLedger_)
-            setMode(OperatingMode::TRACKING);
+            setMode(OperatingMode::TRACKING, "endConsensus: check tracking");
     }
 
     if (((mMode == OperatingMode::CONNECTED) || (mMode == OperatingMode::TRACKING)) &&
@@ -2170,7 +2171,7 @@ NetworkOPsImp::endConsensus(std::unique_ptr<std::stringstream> const& clog)
         if (registry_.get().getTimeKeeper().now() <
             (current->header().parentCloseTime + 2 * current->header().closeTimeResolution))
         {
-            setMode(OperatingMode::FULL);
+            setMode(OperatingMode::FULL, "endConsensus: check full");
         }
     }
 
@@ -2182,7 +2183,7 @@ NetworkOPsImp::consensusViewChange()
 {
     if ((mMode == OperatingMode::FULL) || (mMode == OperatingMode::TRACKING))
     {
-        setMode(OperatingMode::CONNECTED);
+        setMode(OperatingMode::CONNECTED, "consensusViewChange");
     }
 }
 
@@ -2486,7 +2487,7 @@ NetworkOPsImp::pubPeerStatus(std::function<Json::Value(void)> const& func)
 }
 
 void
-NetworkOPsImp::setMode(OperatingMode om)
+NetworkOPsImp::setMode(OperatingMode om, char const* reason)
 {
     using namespace std::chrono_literals;
     if (om == OperatingMode::CONNECTED)
@@ -2506,11 +2507,12 @@ NetworkOPsImp::setMode(OperatingMode om)
     if (mMode == om)
         return;
 
+    auto const sink = om < mMode ? m_journal.warn() : m_journal.info();
     mMode = om;
 
     accounting_.mode(om);
 
-    JLOG(m_journal.info()) << "STATE->" << strOperatingMode();
+    JLOG(sink) << "STATE->" << strOperatingMode() << " - " << reason;
     pubServer();
 }
 
@@ -2519,36 +2521,24 @@ NetworkOPsImp::recvValidation(std::shared_ptr<STValidation> const& val, std::str
 {
     JLOG(m_journal.trace()) << "recvValidation " << val->getLedgerHash() << " from " << source;
 
-    std::unique_lock lock(validationsMutex_);
-    BypassAccept bypassAccept = BypassAccept::no;
-    try
     {
-        if (pendingValidations_.contains(val->getLedgerHash()))
+        CanProcess const check(validationsMutex_, pendingValidations_, val->getLedgerHash());
+        try
         {
-            bypassAccept = BypassAccept::yes;
+            BypassAccept bypassAccept = check ? BypassAccept::no : BypassAccept::yes;
+            handleNewValidation(registry_.app(), val, source, bypassAccept, m_journal);
         }
-        else
+        catch (std::exception const& e)
         {
-            pendingValidations_.insert(val->getLedgerHash());
+            JLOG(m_journal.warn()) << "Exception thrown for handling new validation "
+                                   << val->getLedgerHash() << ": " << e.what();
+        }
+        catch (...)
+        {
+            JLOG(m_journal.warn())
+                << "Unknown exception thrown for handling new validation " << val->getLedgerHash();
         }
-        scope_unlock const unlock(lock);
-        handleNewValidation(registry_.get().getApp(), val, source, bypassAccept, m_journal);
     }
-    catch (std::exception const& e)
-    {
-        JLOG(m_journal.warn()) << "Exception thrown for handling new validation "
-                               << val->getLedgerHash() << ": " << e.what();
-    }
-    catch (...)
-    {
-        JLOG(m_journal.warn()) << "Unknown exception thrown for handling new validation "
-                               << val->getLedgerHash();
-    }
-    if (bypassAccept == BypassAccept::no)
-    {
-        pendingValidations_.erase(val->getLedgerHash());
-    }
-    lock.unlock();
 
     pubValidation(val);