diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 61eefea5994..8933e88a87f 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/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()); } diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 61c2308a296..3a025f62796 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/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(); @@ -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(); - orphanage.LimitOrphans(limit, limit_orphans_rng); + orphanage.LimitOrphans(limit, orphanage_rng); Assert(orphanage.Size() <= limit); }); diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index f30d4b402f8..9beb24f7dff 100644 --- a/src/test/orphanage_tests.cpp +++ b/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); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 07f7fa021fd..06b6ab4af20 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -152,7 +152,7 @@ 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)); @@ -160,15 +160,21 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) 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& 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& 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); } } } diff --git a/src/txorphanage.h b/src/txorphanage.h index 868741e7890..4205da01994 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -62,7 +62,7 @@ public: 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);