Review fixes and refactor:

1) refactor filtering of validations to specifically avoid
   concurrent checkAccept() calls for the same validation hash.
2) Log when duplicate concurrent inbound ledger and validation requests
   are filtered.
3) RAII for containers that track concurrent inbound ledger and
   validation requests.
4) Comment on when to asynchronously acquire inbound ledgers, which
   is possible to be always OK, but should have further review.

src/ripple/app/consensus/RCLValidations.cpp

@@ -161,7 +161,9 @@ void
 handleNewValidation(
     Application& app,
     std::shared_ptr<STValidation> const& val,
-    std::string const& source)
+    std::string const& source,
+    BypassAccept const bypassAccept,
+    std::optional<beast::Journal> j)
 {
     auto const& signingKey = val->getSignerPublic();
     auto const& hash = val->getLedgerHash();
@@ -186,7 +188,21 @@ handleNewValidation(
     if (outcome == ValStatus::current)
     {
         if (val->isTrusted())
-            app.getLedgerMaster().checkAccept(hash, seq);
+        {
+            if (bypassAccept == BypassAccept::TRUE)
+            {
+                assert(j.has_value());
+                if (j.has_value())
+                {
+                    JLOG(j->trace()) << "Bypassing checkAccept for validation "
+                                     << val->getLedgerHash();
+                }
+            }
+            else
+            {
+                app.getLedgerMaster().checkAccept(hash, seq);
+            }
+        }
         return;
     }
 

src/ripple/app/consensus/RCLValidations.h

@@ -25,12 +25,16 @@
 #include <ripple/protocol/Protocol.h>
 #include <ripple/protocol/RippleLedgerHash.h>
 #include <ripple/protocol/STValidation.h>
+#include <optional>
+#include <set>
 #include <vector>
 
 namespace ripple {
 
 class Application;
 
+enum class BypassAccept { FALSE, TRUE };
+
 /** Wrapper over STValidation for generic Validation code
 
     Wraps an STValidation for compatibility with the generic validation code.
@@ -248,7 +252,9 @@ void
 handleNewValidation(
     Application& app,
     std::shared_ptr<STValidation> const& val,
-    std::string const& source);
+    std::string const& source,
+    BypassAccept const bypassAccept = BypassAccept::FALSE,
+    std::optional<beast::Journal> j = std::nullopt);
 
 }  // namespace ripple
 

src/ripple/app/ledger/InboundLedgers.h

@@ -23,6 +23,7 @@
 #include <ripple/app/ledger/InboundLedger.h>
 #include <ripple/protocol/RippleLedgerHash.h>
 #include <memory>
+#include <set>
 
 namespace ripple {
 
@@ -37,11 +38,14 @@ class InboundLedgers
 
     virtual ~InboundLedgers() = default;
 
-    // VFALCO TODO Should this be called findOrAdd ?
-    //
+    // Callers should use this if they possibly need an authoritative
+    // response immediately.
     virtual std::shared_ptr<Ledger const>
     acquire(uint256 const& hash, std::uint32_t seq, InboundLedger::Reason) = 0;
 
+    // Callers should use this if they are known to be executing on the Job
+    // Queue. TODO review whether all callers of acquire() can use this
+    // instead. Inbound ledger acquisition is asynchronous anyway.
     virtual void
     acquireAsync(
         uint256 const& hash,

src/ripple/app/ledger/impl/InboundLedgers.cpp

@@ -29,6 +29,7 @@
 #include <ripple/core/JobQueue.h>
 #include <ripple/nodestore/DatabaseShard.h>
 #include <ripple/protocol/jss.h>
+#include <exception>
 #include <memory>
 #include <mutex>
 #include <vector>
@@ -156,11 +157,26 @@ class InboundLedgersImp : public InboundLedgers
         InboundLedger::Reason reason) override
     {
         std::unique_lock lock(acquiresMutex_);
-        if (pendingAcquires_.contains(hash))
-            return;
-        pendingAcquires_.insert(hash);
-        lock.unlock();
-        acquire(hash, seq, reason);
+        try
+        {
+            if (pendingAcquires_.contains(hash))
+                return;
+            pendingAcquires_.insert(hash);
+            lock.unlock();
+            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;
+        }
         lock.lock();
         pendingAcquires_.erase(hash);
     }

src/ripple/app/misc/NetworkOPs.cpp

@@ -72,8 +72,10 @@
 #include <boost/asio/steady_timer.hpp>
 
 #include <algorithm>
+#include <exception>
 #include <mutex>
 #include <optional>
+#include <set>
 #include <string>
 #include <tuple>
 #include <unordered_map>
@@ -781,7 +783,7 @@ class NetworkOPsImp final : public NetworkOPs
 
     StateAccounting accounting_{};
 
-    std::set<std::pair<PublicKey, uint256>> pendingValidations_;
+    std::set<uint256> pendingValidations_;
     std::mutex validationsMutex_;
 
 private:
@@ -2312,14 +2314,30 @@ NetworkOPsImp::recvValidation(
         << "recvValidation " << val->getLedgerHash() << " from " << source;
 
     std::unique_lock lock(validationsMutex_);
-    if (pendingValidations_.contains(
-            {val->getSignerPublic(), val->getLedgerHash()}))
-        return false;
-    pendingValidations_.insert({val->getSignerPublic(), val->getLedgerHash()});
-    lock.unlock();
-    handleNewValidation(app_, val, source);
+    try
+    {
+        BypassAccept bypassAccept = BypassAccept::FALSE;
+        if (pendingValidations_.contains(val->getLedgerHash()))
+            bypassAccept = BypassAccept::TRUE;
+        else
+            pendingValidations_.insert(val->getLedgerHash());
+        lock.unlock();
+        handleNewValidation(app_, 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();
+    }
     lock.lock();
-    pendingValidations_.erase({val->getSignerPublic(), val->getLedgerHash()});
+    pendingValidations_.erase(val->getLedgerHash());
     lock.unlock();
 
     pubValidation(val);