From 18235067af9f5e6cd226c9f044f65d50254097da Mon Sep 17 00:00:00 2001
From: Nik Bougalis <nikb@bougalis.net>
Date: Wed, 12 Jan 2022 22:28:23 -0800
Subject: [PATCH] Simplify and optimize the processing of inbound transactions

---
 .../app/ledger/impl/InboundTransactions.cpp   | 25 +++++++---------
 .../app/ledger/impl/TransactionAcquire.cpp    | 29 +++++++------------
 .../app/ledger/impl/TransactionAcquire.h      |  3 +-
 3 files changed, 23 insertions(+), 34 deletions(-)

diff --git a/src/ripple/app/ledger/impl/InboundTransactions.cpp b/src/ripple/app/ledger/impl/InboundTransactions.cpp
index 7bccf26aa46..7a863bce16b 100644
--- a/src/ripple/app/ledger/impl/InboundTransactions.cpp
+++ b/src/ripple/app/ledger/impl/InboundTransactions.cpp
@@ -71,6 +71,7 @@ class InboundTransactionsImp : public InboundTransactions
         , m_zeroSet(m_map[uint256()])
         , m_gotSet(std::move(gotSet))
         , m_peerSetBuilder(std::move(peerSetBuilder))
+        , j_(app_.journal("InboundTransactions"))
     {
         m_zeroSet.mSet = std::make_shared<SHAMap>(
             SHAMapType::TRANSACTION, uint256(), app_.getNodeFamily());
@@ -99,9 +100,7 @@ class InboundTransactionsImp : public InboundTransactions
         {
             std::lock_guard sl(mLock);
 
-            auto it = m_map.find(hash);
-
-            if (it != m_map.end())
+            if (auto it = m_map.find(hash); it != m_map.end())
             {
                 if (acquire)
                 {
@@ -140,11 +139,8 @@ class InboundTransactionsImp : public InboundTransactions
     {
         protocol::TMLedgerData& packet = *packet_ptr;
 
-        JLOG(app_.journal("InboundLedger").trace())
-            << "Got data (" << packet.nodes().size()
-            << ") "
-               "for acquiring ledger: "
-            << hash;
+        JLOG(j_.trace()) << "Got data (" << packet.nodes().size()
+                         << ") for acquiring ledger: " << hash;
 
         TransactionAcquire::pointer ta = getAcquire(hash);
 
@@ -154,8 +150,9 @@ class InboundTransactionsImp : public InboundTransactions
             return;
         }
 
-        std::list<SHAMapNodeID> nodeIDs;
-        std::list<Blob> nodeData;
+        std::vector<std::pair<SHAMapNodeID, Slice>> data;
+        data.reserve(packet.nodes().size());
+
         for (auto const& node : packet.nodes())
         {
             if (!node.has_nodeid() || !node.has_nodedata())
@@ -172,12 +169,10 @@ class InboundTransactionsImp : public InboundTransactions
                 return;
             }
 
-            nodeIDs.emplace_back(*id);
-            nodeData.emplace_back(
-                node.nodedata().begin(), node.nodedata().end());
+            data.emplace_back(std::make_pair(*id, makeSlice(node.nodedata())));
         }
 
-        if (!ta->takeNodes(nodeIDs, nodeData, peer).isUseful())
+        if (!ta->takeNodes(data, peer).isUseful())
             peer->charge(Resource::feeUnwantedData);
     }
 
@@ -262,6 +257,8 @@ class InboundTransactionsImp : public InboundTransactions
     std::function<void(std::shared_ptr<SHAMap> const&, bool)> m_gotSet;
 
     std::unique_ptr<PeerSetBuilder> m_peerSetBuilder;
+
+    beast::Journal j_;
 };
 
 //------------------------------------------------------------------------------
diff --git a/src/ripple/app/ledger/impl/TransactionAcquire.cpp b/src/ripple/app/ledger/impl/TransactionAcquire.cpp
index 7d958cba869..24a03a16ffb 100644
--- a/src/ripple/app/ledger/impl/TransactionAcquire.cpp
+++ b/src/ripple/app/ledger/impl/TransactionAcquire.cpp
@@ -65,7 +65,7 @@ TransactionAcquire::done()
 
     if (failed_)
     {
-        JLOG(journal_.warn()) << "Failed to acquire TX set " << hash_;
+        JLOG(journal_.debug()) << "Failed to acquire TX set " << hash_;
     }
     else
     {
@@ -176,8 +176,7 @@ TransactionAcquire::trigger(std::shared_ptr<Peer> const& peer)
 
 SHAMapAddNode
 TransactionAcquire::takeNodes(
-    const std::list<SHAMapNodeID>& nodeIDs,
-    const std::list<Blob>& data,
+    std::vector<std::pair<SHAMapNodeID, Slice>> const& data,
     std::shared_ptr<Peer> const& peer)
 {
     ScopedLockType sl(mtx_);
@@ -196,24 +195,20 @@ TransactionAcquire::takeNodes(
 
     try
     {
-        if (nodeIDs.empty())
+        if (data.empty())
             return SHAMapAddNode::invalid();
 
-        std::list<SHAMapNodeID>::const_iterator nodeIDit = nodeIDs.begin();
-        std::list<Blob>::const_iterator nodeDatait = data.begin();
         ConsensusTransSetSF sf(app_, app_.getTempNodeCache());
 
-        while (nodeIDit != nodeIDs.end())
+        for (auto const& d : data)
         {
-            if (nodeIDit->isRoot())
+            if (d.first.isRoot())
             {
                 if (mHaveRoot)
                     JLOG(journal_.debug())
                         << "Got root TXS node, already have it";
                 else if (!mMap->addRootNode(
-                                  SHAMapHash{hash_},
-                                  makeSlice(*nodeDatait),
-                                  nullptr)
+                                  SHAMapHash{hash_}, d.second, nullptr)
                               .isGood())
                 {
                     JLOG(journal_.warn()) << "TX acquire got bad root node";
@@ -221,24 +216,22 @@ TransactionAcquire::takeNodes(
                 else
                     mHaveRoot = true;
             }
-            else if (!mMap->addKnownNode(*nodeIDit, makeSlice(*nodeDatait), &sf)
-                          .isGood())
+            else if (!mMap->addKnownNode(d.first, d.second, &sf).isGood())
             {
                 JLOG(journal_.warn()) << "TX acquire got bad non-root node";
                 return SHAMapAddNode::invalid();
             }
-
-            ++nodeIDit;
-            ++nodeDatait;
         }
 
         trigger(peer);
         progress_ = true;
         return SHAMapAddNode::useful();
     }
-    catch (std::exception const&)
+    catch (std::exception const& ex)
     {
-        JLOG(journal_.error()) << "Peer sends us junky transaction node data";
+        JLOG(journal_.error())
+            << "Peer " << peer->id()
+            << " sent us junky transaction node data: " << ex.what();
         return SHAMapAddNode::invalid();
     }
 }
diff --git a/src/ripple/app/ledger/impl/TransactionAcquire.h b/src/ripple/app/ledger/impl/TransactionAcquire.h
index 611448a444e..3863868fae0 100644
--- a/src/ripple/app/ledger/impl/TransactionAcquire.h
+++ b/src/ripple/app/ledger/impl/TransactionAcquire.h
@@ -44,8 +44,7 @@ class TransactionAcquire final
 
     SHAMapAddNode
     takeNodes(
-        const std::list<SHAMapNodeID>& IDs,
-        const std::list<Blob>& data,
+        std::vector<std::pair<SHAMapNodeID, Slice>> const& data,
         std::shared_ptr<Peer> const&);
 
     void