[p2p] assign just 1 random announcer in AddChildrenToWorkSet

Author: glozow

src/node/txdownloadman_impl.cpp

@@ -333,7 +333,7 @@ void TxDownloadManagerImpl::MempoolAcceptedTx(const CTransactionRef& tx)
     m_txrequest.ForgetTxHash(tx->GetHash());
     m_txrequest.ForgetTxHash(tx->GetWitnessHash());
 
-    m_orphanage.AddChildrenToWorkSet(*tx);
+    m_orphanage.AddChildrenToWorkSet(*tx, m_opts.m_rng);
     // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage.
     m_orphanage.EraseTx(tx->GetWitnessHash());
 }

src/test/fuzz/txorphan.cpp

@@ -33,7 +33,7 @@ void initialize_orphanage()
 FUZZ_TARGET(txorphan, .init = initialize_orphanage)
 {
     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
-    FastRandomContext limit_orphans_rng{/*fDeterministic=*/true};
+    FastRandomContext orphanage_rng{/*fDeterministic=*/true};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
 
     TxOrphanage orphanage;
@@ -79,7 +79,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
         // previous loop and potentially the parent of this tx.
         if (ptx_potential_parent) {
             // Set up future GetTxToReconsider call.
-            orphanage.AddChildrenToWorkSet(*ptx_potential_parent);
+            orphanage.AddChildrenToWorkSet(*ptx_potential_parent, orphanage_rng);
 
             // Check that all txns returned from GetChildrenFrom* are indeed a direct child of this tx.
             NodeId peer_id = fuzzed_data_provider.ConsumeIntegral<NodeId>();
@@ -154,7 +154,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
                     // test mocktime and expiry
                     SetMockTime(ConsumeTime(fuzzed_data_provider));
                     auto limit = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
-                    orphanage.LimitOrphans(limit, limit_orphans_rng);
+                    orphanage.LimitOrphans(limit, orphanage_rng);
                     Assert(orphanage.Size() <= limit);
                 });
 

src/test/orphanage_tests.cpp

@@ -532,19 +532,27 @@ BOOST_AUTO_TEST_CASE(peer_worksets)
             BOOST_CHECK(orphanage.HaveTxFromPeer(orphan_wtxid, node));
         }
 
-        // Parent accepted: add child to all 3 worksets.
-        orphanage.AddChildrenToWorkSet(*tx_missing_parent);
-        BOOST_CHECK_EQUAL(orphanage.GetTxToReconsider(node0), tx_orphan);
-        BOOST_CHECK_EQUAL(orphanage.GetTxToReconsider(node1), tx_orphan);
-        // Don't call GetTxToReconsider(node2) yet because it mutates the workset.
+        // Parent accepted: child is added to 1 of 3 worksets.
+        orphanage.AddChildrenToWorkSet(*tx_missing_parent, det_rand);
+        int node0_reconsider = orphanage.HaveTxToReconsider(node0);
+        int node1_reconsider = orphanage.HaveTxToReconsider(node1);
+        int node2_reconsider = orphanage.HaveTxToReconsider(node2);
+        BOOST_CHECK_EQUAL(node0_reconsider + node1_reconsider + node2_reconsider, 1);
+
+        NodeId assigned_peer;
+        if (node0_reconsider) {
+            assigned_peer = node0;
+        } else if (node1_reconsider) {
+            assigned_peer = node1;
+        } else {
+            BOOST_CHECK(node2_reconsider);
+            assigned_peer = node2;
+        }
 
         // EraseForPeer also removes that tx from the workset.
-        orphanage.EraseForPeer(node0);
+        orphanage.EraseForPeer(assigned_peer);
         BOOST_CHECK_EQUAL(orphanage.GetTxToReconsider(node0), nullptr);
 
-        // However, the other peers' worksets are not touched.
-        BOOST_CHECK_EQUAL(orphanage.GetTxToReconsider(node2), tx_orphan);
-
         // Delete this tx, clearing the orphanage.
         BOOST_CHECK_EQUAL(orphanage.EraseTx(orphan_wtxid), 1);
         BOOST_CHECK_EQUAL(orphanage.Size(), 0);

src/txorphanage.cpp

@@ -152,23 +152,29 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng)
     if (nEvicted > 0) LogDebug(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted);
 }
 
-void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
+void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng)
 {
     for (unsigned int i = 0; i < tx.vout.size(); i++) {
         const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i));
         if (it_by_prev != m_outpoint_to_orphan_it.end()) {
             for (const auto& elem : it_by_prev->second) {
                 // Belt and suspenders, each orphan should always have at least 1 announcer.
                 if (!Assume(!elem->second.announcers.empty())) continue;
-                for (const auto announcer: elem->second.announcers) {
-                    // Get this source peer's work set, emplacing an empty set if it didn't exist
-                    // (note: if this peer wasn't still connected, we would have removed the orphan tx already)
-                    std::set<Wtxid>& orphan_work_set = m_peer_work_set.try_emplace(announcer).first->second;
-                    // Add this tx to the work set
-                    orphan_work_set.insert(elem->first);
-                    LogDebug(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n",
-                             tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), announcer);
-                }
+
+                // Select a random peer to assign orphan processing, reducing wasted work if the orphan is still missing
+                // inputs. However, we don't want to create an issue in which the assigned peer can purposefully stop us
+                // from processing the orphan by disconnecting.
+                auto announcer_iter = std::begin(elem->second.announcers);
+                std::advance(announcer_iter, rng.randrange(elem->second.announcers.size()));
+                auto announcer = *(announcer_iter);
+
+                // Get this source peer's work set, emplacing an empty set if it didn't exist
+                // (note: if this peer wasn't still connected, we would have removed the orphan tx already)
+                std::set<Wtxid>& orphan_work_set = m_peer_work_set.try_emplace(announcer).first->second;
+                // Add this tx to the work set
+                orphan_work_set.insert(elem->first);
+                LogDebug(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n",
+                         tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), announcer);
             }
         }
     }

src/txorphanage.h

@@ -62,7 +62,7 @@ class TxOrphanage {
     void LimitOrphans(unsigned int max_orphans, FastRandomContext& rng);
 
     /** Add any orphans that list a particular tx as a parent into the from peer's work set */
-    void AddChildrenToWorkSet(const CTransaction& tx);
+    void AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng);
 
     /** Does this peer have any work to do? */
     bool HaveTxToReconsider(NodeId peer);