[refactor] make GetCandidatePeers take uint256 and in-out vector

The txrequest fuzzer uses uint256s, not transactions, so it's best if
GetCandidatePeers takes that as an input.

Author: glozow

src/node/txdownloadman_impl.cpp

@@ -401,18 +401,19 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
                 // means it was already added to vExtraTxnForCompact.
                 add_extra_compact_tx &= !m_orphanage.HaveTx(wtxid);
 
-                auto add_orphan_reso_candidate = [&](const CTransactionRef& orphan_tx, const std::vector<Txid>& unique_parents, NodeId nodeid, std::chrono::microseconds now) {
-                    if (MaybeAddOrphanResolutionCandidate(unique_parents, orphan_tx->GetWitnessHash(), nodeid, now)) {
-                        m_orphanage.AddTx(orphan_tx, nodeid);
-                    }
-                };
-
                 // If there is no candidate for orphan resolution, AddTx will not be called. This means
                 // that if a peer is overloading us with invs and orphans, they will eventually not be
                 // able to add any more transactions to the orphanage.
-                add_orphan_reso_candidate(ptx, unique_parents, nodeid, now);
-                for (const auto& candidate : m_txrequest.GetCandidatePeers(ptx)) {
-                    add_orphan_reso_candidate(ptx, unique_parents, candidate, now);
+                //
+                // Search by txid and, if the tx has a witness, wtxid
+                std::vector<NodeId> orphan_resolution_candidates{nodeid};
+                m_txrequest.GetCandidatePeers(ptx->GetHash().ToUint256(), orphan_resolution_candidates);
+                if (ptx->HasWitness()) m_txrequest.GetCandidatePeers(ptx->GetWitnessHash().ToUint256(), orphan_resolution_candidates);
+
+                for (const auto& nodeid : orphan_resolution_candidates) {
+                    if (MaybeAddOrphanResolutionCandidate(unique_parents, ptx->GetWitnessHash(), nodeid, now)) {
+                        m_orphanage.AddTx(ptx, nodeid);
+                    }
                 }
 
                 // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore.

src/txrequest.cpp

@@ -574,21 +574,13 @@ class TxRequestTracker::Impl {
         }
     }
 
-    std::vector<NodeId> GetCandidatePeers(const CTransactionRef& tx) const
+    void GetCandidatePeers(const uint256& txhash, std::vector<NodeId>& result_peers) const
     {
-        // Search by txid and, if the tx has a witness, wtxid
-        std::vector<uint256> hashes{tx->GetHash().ToUint256()};
-        if (tx->HasWitness()) hashes.emplace_back(tx->GetWitnessHash().ToUint256());
-
-        std::vector<NodeId> result_peers;
-        for (const uint256& txhash : hashes) {
-            auto it = m_index.get<ByTxHash>().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0});
-            while (it != m_index.get<ByTxHash>().end() && it->m_txhash == txhash && it->GetState() != State::COMPLETED) {
-                result_peers.push_back(it->m_peer);
-                ++it;
-            }
+        auto it = m_index.get<ByTxHash>().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0});
+        while (it != m_index.get<ByTxHash>().end() && it->m_txhash == txhash && it->GetState() != State::COMPLETED) {
+            result_peers.push_back(it->m_peer);
+            ++it;
         }
-        return result_peers;
     }
 
     void ReceivedInv(NodeId peer, const GenTxid& gtxid, bool preferred,
@@ -738,7 +730,7 @@ size_t TxRequestTracker::CountInFlight(NodeId peer) const { return m_impl->Count
 size_t TxRequestTracker::CountCandidates(NodeId peer) const { return m_impl->CountCandidates(peer); }
 size_t TxRequestTracker::Count(NodeId peer) const { return m_impl->Count(peer); }
 size_t TxRequestTracker::Size() const { return m_impl->Size(); }
-std::vector<NodeId> TxRequestTracker::GetCandidatePeers(const CTransactionRef& tx) const { return m_impl->GetCandidatePeers(tx); }
+void TxRequestTracker::GetCandidatePeers(const uint256& txhash, std::vector<NodeId>& result_peers) const { return m_impl->GetCandidatePeers(txhash, result_peers); }
 void TxRequestTracker::SanityCheck() const { m_impl->SanityCheck(); }
 
 void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds now) const

src/txrequest.h

@@ -195,8 +195,9 @@ class TxRequestTracker {
     /** Count how many announcements are being tracked in total across all peers and transaction hashes. */
     size_t Size() const;
 
-    /** For some tx return all peers with non-COMPLETED announcements for its txid or wtxid. The resulting vector may contain duplicate NodeIds. */
-    std::vector<NodeId> GetCandidatePeers(const CTransactionRef& tx) const;
+    /** For some txhash (txid or wtxid), finds all peers with non-COMPLETED announcements and appends them to
+     * result_peers. Does not try to ensure that result_peers contains no duplicates. */
+    void GetCandidatePeers(const uint256& txhash, std::vector<NodeId>& result_peers) const;
 
     /** Access to the internal priority computation (testing only) */
     uint64_t ComputePriority(const uint256& txhash, NodeId peer, bool preferred) const;