diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a19443c0f5..dab0f72800 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2978,7 +2978,7 @@ std::optional PeerManagerImpl::ProcessInvalidTx(NodeId if (add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } - for (const uint256& parent_txid : unique_parents) { + for (const Txid& parent_txid : unique_parents) { if (peer) AddKnownTx(*peer, parent_txid); } @@ -3934,7 +3934,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, AddKnownTx(*peer, inv.hash); if (!m_chainman.IsInitialBlockDownload()) { - const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time, /*p2p_inv=*/true)}; + const bool fAlreadyHave{m_txdownloadman.AddTxAnnouncement(pfrom.GetId(), gtxid, current_time)}; LogDebug(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId()); } } else { diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 81b0c76e0a..1ae833a6be 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -92,7 +92,7 @@ struct PackageToValidate { struct RejectedTxTodo { bool m_should_add_extra_compact_tx; - std::vector m_unique_parents; + std::vector m_unique_parents; std::optional m_package_to_validate; }; @@ -136,9 +136,8 @@ public: /** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received. * Also called internally when a transaction is missing parents so that we can request them. - * @param[in] p2p_inv When true, only add this announcement if we don't already have the tx. * Returns true if this was a dropped inv (p2p_inv=true and we already have the tx), false otherwise. */ - bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv); + bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index f9635d049a..fe961048d3 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -39,9 +39,9 @@ void TxDownloadManager::DisconnectedPeer(NodeId nodeid) { m_impl->DisconnectedPeer(nodeid); } -bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv) +bool TxDownloadManager::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now) { - return m_impl->AddTxAnnouncement(peer, gtxid, now, p2p_inv); + return m_impl->AddTxAnnouncement(peer, gtxid, now); } std::vector TxDownloadManager::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) { @@ -172,12 +172,39 @@ void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) } -bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv) +bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now) { + // If this is an orphan we are trying to resolve, consider this peer as a orphan resolution candidate instead. + // - is wtxid matching something in orphanage + // - exists in orphanage + // - peer can be an orphan resolution candidate + if (gtxid.IsWtxid()) { + if (auto orphan_tx{m_orphanage.GetTx(Wtxid::FromUint256(gtxid.GetHash()))}) { + auto unique_parents{GetUniqueParents(*orphan_tx)}; + std::erase_if(unique_parents, [&](const auto& txid){ + return AlreadyHaveTx(GenTxid::Txid(txid), /*include_reconsiderable=*/false); + }); + + if (unique_parents.empty()) return true; + + if (auto delay{OrphanResolutionCandidate(peer, Wtxid::FromUint256(gtxid.GetHash()), unique_parents.size())}) { + m_orphanage.AddAnnouncer(Wtxid::FromUint256(gtxid.GetHash()), peer); + + const auto& info = m_peer_info.at(peer).m_connection_info; + for (const auto& parent_txid : unique_parents) { + m_txrequest.ReceivedInv(peer, GenTxid::Txid(parent_txid), info.m_preferred, now + *delay); + } + + LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", peer, gtxid.GetHash().ToString()); + } + + // Return even if the peer isn't an orphan resolution candidate. This would be caught by AlreadyHaveTx. + return true; + } + } + // If this is an inv received from a peer and we already have it, we can drop it. - // If this is a request for the parent of an orphan, we don't drop transactions that we already have. In particular, - // we *do* want to request parents that are in m_lazy_recent_rejects_reconsiderable, since they can be CPFP'd. - if (p2p_inv && AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) return true; + if (AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) return true; auto it = m_peer_info.find(peer); if (it == m_peer_info.end()) return false; @@ -204,6 +231,36 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, return false; } +std::optional TxDownloadManagerImpl::OrphanResolutionCandidate(NodeId nodeid, const Wtxid& orphan_wtxid, size_t num_parents) +{ + if (m_peer_info.count(nodeid) == 0) return std::nullopt; + if (m_orphanage.HaveTxFromPeer(orphan_wtxid, nodeid)) return std::nullopt; + + const auto& peer_entry = m_peer_info.at(nodeid); + const auto& info = peer_entry.m_connection_info; + // TODO: add delays and limits based on the amount of orphan resolution we are already doing + // with this peer, how much they are using the orphanage, etc. + if (!info.m_relay_permissions) { + // This mirrors the delaying and dropping behavior in AddTxAnnouncement in order to preserve + // existing behavior: drop if we are tracking too many invs for this peer already. Each + // orphan resolution involves at least 1 transaction request which may or may not be + // currently tracked in m_txrequest, so we include that in the count. + if (m_txrequest.Count(nodeid) + num_parents > MAX_PEER_TX_ANNOUNCEMENTS) return std::nullopt; + } + + std::chrono::seconds delay{0s}; + if (!info.m_preferred) delay += NONPREF_PEER_TX_DELAY; + // The orphan wtxid is used, but resolution entails requesting the parents by txid. Sometimes + // parent and child are announced and thus requested around the same time, and we happen to + // receive child sooner. Waiting a few seconds may allow us to cancel the orphan resolution + // request if the parent arrives in that time. + if (m_num_wtxid_peers > 0) delay += TXID_RELAY_DELAY; + const bool overloaded = !info.m_relay_permissions && m_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_REQUEST_IN_FLIGHT; + if (overloaded) delay += OVERLOADED_PEER_TX_DELAY; + + return delay; +} + std::vector TxDownloadManagerImpl::GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time) { std::vector requests; @@ -243,9 +300,11 @@ std::optional TxDownloadManagerImpl::Find1P1CPackage(const CT Assume(RecentRejectsReconsiderableFilter().contains(parent_wtxid.ToUint256())); - // Prefer children from this peer. This helps prevent censorship attempts in which an attacker + // Only consider children from this peer. This helps prevent censorship attempts in which an attacker // sends lots of fake children for the parent, and we (unluckily) keep selecting the fake - // children instead of the real one provided by the honest peer. + // children instead of the real one provided by the honest peer. Since we track all announcers + // of an orphan, this does not exclude parent + orphan pairs that we happened to request from + // different peers. const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)}; // These children should be sorted from newest to oldest. In the (probably uncommon) case @@ -258,34 +317,6 @@ std::optional TxDownloadManagerImpl::Find1P1CPackage(const CT return PackageToValidate{ptx, child, nodeid, nodeid}; } } - - // If no suitable candidate from the same peer is found, also try children that were provided by - // a different peer. This is useful because sometimes multiple peers announce both transactions - // to us, and we happen to download them from different peers (we wouldn't have known that these - // 2 transactions are related). We still want to find 1p1c packages then. - // - // If we start tracking all announcers of orphans, we can restrict this logic to parent + child - // pairs in which both were provided by the same peer, i.e. delete this step. - const auto cpfp_candidates_different_peer{m_orphanage.GetChildrenFromDifferentPeer(ptx, nodeid)}; - - // Find the first 1p1c that hasn't already been rejected. We randomize the order to not - // create a bias that attackers can use to delay package acceptance. - // - // Create a random permutation of the indices. - std::vector tx_indices(cpfp_candidates_different_peer.size()); - std::iota(tx_indices.begin(), tx_indices.end(), 0); - std::shuffle(tx_indices.begin(), tx_indices.end(), m_opts.m_rng); - - for (const auto index : tx_indices) { - // If we already tried a package and failed for any reason, the combined hash was - // cached in m_lazy_recent_rejects_reconsiderable. - const auto [child_tx, child_sender] = cpfp_candidates_different_peer.at(index); - Package maybe_cpfp_package{ptx, child_tx}; - if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package)) && - !RecentRejectsFilter().contains(child_tx->GetHash().ToUint256())) { - return PackageToValidate{ptx, child_tx, nodeid, child_sender}; - } - } return std::nullopt; } @@ -301,6 +332,21 @@ void TxDownloadManagerImpl::MempoolAcceptedTx(const CTransactionRef& tx) m_orphanage.EraseTx(tx->GetWitnessHash()); } +std::vector TxDownloadManagerImpl::GetUniqueParents(const CTransaction& tx) +{ + std::vector unique_parents; + unique_parents.reserve(tx.vin.size()); + for (const CTxIn& txin : tx.vin) { + // We start with all parents, and then remove duplicates below. + unique_parents.push_back(txin.prevout.hash); + } + + std::sort(unique_parents.begin(), unique_parents.end()); + unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end()); + + return unique_parents; +} + node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransactionRef& ptx, const TxValidationState& state, NodeId nodeid, bool first_time_failure) { const CTransaction& tx{*ptx}; @@ -308,7 +354,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Whether we should call AddToCompactExtraTransactions at the end bool add_extra_compact_tx{first_time_failure}; // Hashes to pass to AddKnownTx later - std::vector unique_parents; + std::vector unique_parents; // Populated if failure is reconsiderable and eligible package is found. std::optional package_to_validate; @@ -320,13 +366,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Deduplicate parent txids, so that we don't have to loop over // the same parent txid more than once down below. - unique_parents.reserve(tx.vin.size()); - for (const CTxIn& txin : tx.vin) { - // We start with all parents, and then remove duplicates below. - unique_parents.push_back(txin.prevout.hash); - } - std::sort(unique_parents.begin(), unique_parents.end()); - unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end()); + unique_parents = GetUniqueParents(tx); // Distinguish between parents in m_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable. // We can tolerate having up to 1 parent in m_lazy_recent_rejects_reconsiderable since we @@ -348,30 +388,48 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction } } if (!fRejectedParents) { - const auto current_time{GetTime()}; + // Filter parents that we already have. + // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been + // previously rejected for being too low feerate. This orphan might CPFP it. + std::erase_if(unique_parents, [&](const auto& txid){ + return AlreadyHaveTx(GenTxid::Txid(txid), /*include_reconsiderable=*/false); + }); + const auto now{GetTime()}; + const auto& wtxid = ptx->GetWitnessHash(); + // Potentially flip add_extra_compact_tx to false if tx is already in orphanage, which + // means it was already added to vExtraTxnForCompact. + add_extra_compact_tx &= !m_orphanage.HaveTx(wtxid); - for (const uint256& parent_txid : unique_parents) { - // Here, we only have the txid (and not wtxid) of the - // inputs, so we only request in txid mode, even for - // wtxidrelay peers. - // Eventually we should replace this with an improved - // protocol for getting all unconfirmed parents. - const auto gtxid{GenTxid::Txid(parent_txid)}; - // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been - // previously rejected for being too low feerate. This orphan might CPFP it. - if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { - AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false); + auto add_orphan_reso_candidate = [&](const CTransactionRef& orphan_tx, const std::vector& unique_parents, NodeId nodeid, std::chrono::microseconds now) { + const auto& wtxid = orphan_tx->GetWitnessHash(); + if (auto delay{OrphanResolutionCandidate(nodeid, wtxid, unique_parents.size())}) { + const auto& info = m_peer_info.at(nodeid).m_connection_info; + m_orphanage.AddTx(orphan_tx, nodeid); + + // Treat finding orphan resolution candidate as equivalent to the peer announcing all missing parents + // In the future, orphan resolution may include more explicit steps + for (const auto& parent_txid : unique_parents) { + m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(parent_txid), info.m_preferred, now + *delay); + } + LogDebug(BCLog::TXPACKAGES, "added peer=%d as a candidate for resolving orphan %s\n", nodeid, wtxid.ToString()); } - } + }; - // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there - add_extra_compact_tx &= m_orphanage.AddTx(ptx, 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); + } // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + // Note that, if the orphanage reaches capacity, it's possible that we immediately evict + // the transaction we just added. m_orphanage.LimitOrphans(m_opts.m_max_orphan_txs, m_opts.m_rng); } else { unique_parents.clear(); diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 8039ddb3cb..ab563b2241 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -163,7 +163,7 @@ public: /** Consider adding this tx hash to txrequest. Should be called whenever a new inv has been received. * Also called internally when a transaction is missing parents so that we can request them. */ - bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv); + bool AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now); /** Get getdata requests to send. */ std::vector GetRequestsToSend(NodeId nodeid, std::chrono::microseconds current_time); @@ -189,6 +189,16 @@ public: void CheckIsEmpty(NodeId nodeid); std::vector GetOrphanTransactions() const; + +protected: + /** Helper for getting deduplicated vector of Txids in vin. */ + std::vector GetUniqueParents(const CTransaction& tx); + + /** Determine candidacy (and delay) for potential orphan resolution candidate. + * @returns delay for orphan resolution if this peer is a good candidate for orphan resolution, + * std::nullopt if this peer cannot be added because it has reached download/orphanage limits. + * */ + std::optional OrphanResolutionCandidate(NodeId nodeid, const Wtxid& orphan_wtxid, size_t num_parents); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 52260d5794..2b883322aa 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -845,7 +845,9 @@ static UniValue OrphanToJSON(const TxOrphanage::OrphanTxBase& orphan) o.pushKV("entry", int64_t{TicksSinceEpoch(orphan.nTimeExpire - ORPHAN_TX_EXPIRE_TIME)}); o.pushKV("expiration", int64_t{TicksSinceEpoch(orphan.nTimeExpire)}); UniValue from(UniValue::VARR); - from.push_back(orphan.fromPeer); // only one fromPeer for now + for (const auto fromPeer: orphan.announcers) { + from.push_back(fromPeer); + } o.pushKV("from", from); return o; } diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index bb1331c37c..a786068256 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -230,7 +230,7 @@ FUZZ_TARGET(txdownloadman, .init = initialize) GenTxid gtxid = fuzzed_data_provider.ConsumeBool() ? GenTxid::Txid(rand_tx->GetHash()) : GenTxid::Wtxid(rand_tx->GetWitnessHash()); - txdownloadman.AddTxAnnouncement(rand_peer, gtxid, time, /*p2p_inv=*/fuzzed_data_provider.ConsumeBool()); + txdownloadman.AddTxAnnouncement(rand_peer, gtxid, time); }, [&] { txdownloadman.GetRequestsToSend(rand_peer, time); @@ -375,7 +375,7 @@ FUZZ_TARGET(txdownloadman_impl, .init = initialize) GenTxid gtxid = fuzzed_data_provider.ConsumeBool() ? GenTxid::Txid(rand_tx->GetHash()) : GenTxid::Wtxid(rand_tx->GetWitnessHash()); - txdownload_impl.AddTxAnnouncement(rand_peer, gtxid, time, /*p2p_inv=*/fuzzed_data_provider.ConsumeBool()); + txdownload_impl.AddTxAnnouncement(rand_peer, gtxid, time); }, [&] { const auto getdata_requests = txdownload_impl.GetRequestsToSend(rand_peer, time); diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 31af1afff5..61c2308a29 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -88,12 +88,6 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) return input.prevout.hash == ptx_potential_parent->GetHash(); })); } - for (const auto& [child, peer] : orphanage.GetChildrenFromDifferentPeer(ptx_potential_parent, peer_id)) { - assert(std::any_of(child->vin.cbegin(), child->vin.cend(), [&](const auto& input) { - return input.prevout.hash == ptx_potential_parent->GetHash(); - })); - assert(peer != peer_id); - } } // trigger orphanage functions @@ -128,6 +122,18 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) Assert(!have_tx || !add_tx); } }, + [&] { + bool have_tx = orphanage.HaveTx(tx->GetWitnessHash()); + bool have_tx_and_peer = orphanage.HaveTxFromPeer(tx->GetWitnessHash(), peer_id); + // AddAnnouncer should return false if tx doesn't exist or we already HaveTxFromPeer. + { + bool added_announcer = orphanage.AddAnnouncer(tx->GetWitnessHash(), peer_id); + // have_tx == false -> added_announcer == false + Assert(have_tx || !added_announcer); + // have_tx_and_peer == true -> added_announcer == false + Assert(!have_tx_and_peer || !added_announcer); + } + }, [&] { bool have_tx = orphanage.HaveTx(tx->GetWitnessHash()); // EraseTx should return 0 if m_orphans doesn't have the tx @@ -142,6 +148,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) }, [&] { orphanage.EraseForPeer(peer_id); + Assert(!orphanage.HaveTxFromPeer(tx->GetWitnessHash(), peer_id)); }, [&] { // test mocktime and expiry @@ -157,5 +164,8 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) ptx_potential_parent = tx; } + const bool have_tx{orphanage.HaveTx(tx->GetWitnessHash())}; + const bool get_tx_nonnull{orphanage.GetTx(tx->GetWitnessHash()) != nullptr}; + Assert(have_tx == get_tx_nonnull); } } diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 799f2c0fec..f30d4b402f 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -93,15 +93,6 @@ static bool EqualTxns(const std::set& set_txns, const std::vect } return true; } -static bool EqualTxns(const std::set& set_txns, - const std::vector>& vec_txns) -{ - if (vec_txns.size() != set_txns.size()) return false; - for (const auto& [tx, nodeid] : vec_txns) { - if (!set_txns.contains(tx)) return false; - } - return true; -} BOOST_AUTO_TEST_CASE(DoS_mapOrphans) { @@ -132,7 +123,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vin[0].prevout.hash = Txid::FromUint256(m_rng.rand256()); tx.vin[0].scriptSig << OP_1; tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; + tx.vout[0].nValue = i*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); orphanage.AddTx(MakeTransactionRef(tx), i); @@ -148,7 +139,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vin[0].prevout.n = 0; tx.vin[0].prevout.hash = txPrev->GetHash(); tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; + tx.vout[0].nValue = i*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); SignatureData empty; BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL, empty)); @@ -250,7 +241,9 @@ BOOST_AUTO_TEST_CASE(same_txid_diff_witness) // EraseTx fails as transaction by this wtxid doesn't exist. BOOST_CHECK_EQUAL(orphanage.EraseTx(mutated_wtxid), 0); BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(orphanage.GetTx(normal_wtxid) == child_normal); BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); + BOOST_CHECK(orphanage.GetTx(mutated_wtxid) == nullptr); // Must succeed. Both transactions should be present in orphanage. BOOST_CHECK(orphanage.AddTx(child_mutated, peer)); @@ -310,9 +303,6 @@ BOOST_AUTO_TEST_CASE(get_children) BOOST_CHECK(EqualTxns(expected_parent1_children, orphanage.GetChildrenFromSamePeer(parent1, node1))); BOOST_CHECK(EqualTxns(expected_parent2_children, orphanage.GetChildrenFromSamePeer(parent2, node1))); - BOOST_CHECK(EqualTxns(expected_parent1_children, orphanage.GetChildrenFromDifferentPeer(parent1, node2))); - BOOST_CHECK(EqualTxns(expected_parent2_children, orphanage.GetChildrenFromDifferentPeer(parent2, node2))); - // The peer must match BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty()); BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent2, node2).empty()); @@ -320,8 +310,6 @@ BOOST_AUTO_TEST_CASE(get_children) // There shouldn't be any children of this tx in the orphanage BOOST_CHECK(orphanage.GetChildrenFromSamePeer(child_p1n0_p2n0, node1).empty()); BOOST_CHECK(orphanage.GetChildrenFromSamePeer(child_p1n0_p2n0, node2).empty()); - BOOST_CHECK(orphanage.GetChildrenFromDifferentPeer(child_p1n0_p2n0, node1).empty()); - BOOST_CHECK(orphanage.GetChildrenFromDifferentPeer(child_p1n0_p2n0, node2).empty()); } // Orphans provided by node1 and node2 @@ -344,7 +332,6 @@ BOOST_AUTO_TEST_CASE(get_children) std::set expected_parent1_node1{child_p1n0}; BOOST_CHECK(EqualTxns(expected_parent1_node1, orphanage.GetChildrenFromSamePeer(parent1, node1))); - BOOST_CHECK(EqualTxns(expected_parent1_node1, orphanage.GetChildrenFromDifferentPeer(parent1, node2))); } // Children of parent2 from node1: @@ -352,7 +339,6 @@ BOOST_AUTO_TEST_CASE(get_children) std::set expected_parent2_node1{child_p2n1}; BOOST_CHECK(EqualTxns(expected_parent2_node1, orphanage.GetChildrenFromSamePeer(parent2, node1))); - BOOST_CHECK(EqualTxns(expected_parent2_node1, orphanage.GetChildrenFromDifferentPeer(parent2, node2))); } // Children of parent1 from node2: @@ -360,7 +346,6 @@ BOOST_AUTO_TEST_CASE(get_children) std::set expected_parent1_node2{child_p1n0_p1n1, child_p1n0_p2n0}; BOOST_CHECK(EqualTxns(expected_parent1_node2, orphanage.GetChildrenFromSamePeer(parent1, node2))); - BOOST_CHECK(EqualTxns(expected_parent1_node2, orphanage.GetChildrenFromDifferentPeer(parent1, node1))); } // Children of parent2 from node2: @@ -368,7 +353,6 @@ BOOST_AUTO_TEST_CASE(get_children) std::set expected_parent2_node2{child_p1n0_p2n0}; BOOST_CHECK(EqualTxns(expected_parent2_node2, orphanage.GetChildrenFromSamePeer(parent2, node2))); - BOOST_CHECK(EqualTxns(expected_parent2_node2, orphanage.GetChildrenFromDifferentPeer(parent2, node1))); } } } @@ -390,4 +374,184 @@ BOOST_AUTO_TEST_CASE(too_large_orphan_tx) BOOST_CHECK(orphanage.AddTx(MakeTransactionRef(tx), 0)); } +BOOST_AUTO_TEST_CASE(process_block) +{ + FastRandomContext det_rand{true}; + TxOrphanageTest orphanage{det_rand}; + + // Create outpoints that will be spent by transactions in the block + std::vector outpoints; + const uint32_t num_outpoints{6}; + outpoints.reserve(num_outpoints); + for (uint32_t i{0}; i < num_outpoints; ++i) { + // All the hashes should be different, but change the n just in case. + outpoints.emplace_back(Txid::FromUint256(det_rand.rand256()), i); + } + + CBlock block; + const NodeId node{0}; + + auto control_tx = MakeTransactionSpending({}, det_rand); + BOOST_CHECK(orphanage.AddTx(control_tx, node)); + + auto bo_tx_same_txid = MakeTransactionSpending({outpoints.at(0)}, det_rand); + BOOST_CHECK(orphanage.AddTx(bo_tx_same_txid, node)); + block.vtx.emplace_back(bo_tx_same_txid); + + // 2 transactions with the same txid but different witness + auto b_tx_same_txid_diff_witness = MakeTransactionSpending({outpoints.at(1)}, det_rand); + block.vtx.emplace_back(b_tx_same_txid_diff_witness); + + auto o_tx_same_txid_diff_witness = MakeMutation(b_tx_same_txid_diff_witness); + BOOST_CHECK(orphanage.AddTx(o_tx_same_txid_diff_witness, node)); + + // 2 different transactions that spend the same input. + auto b_tx_conflict = MakeTransactionSpending({outpoints.at(2)}, det_rand); + block.vtx.emplace_back(b_tx_conflict); + + auto o_tx_conflict = MakeTransactionSpending({outpoints.at(2)}, det_rand); + BOOST_CHECK(orphanage.AddTx(o_tx_conflict, node)); + + // 2 different transactions that have 1 overlapping input. + auto b_tx_conflict_partial = MakeTransactionSpending({outpoints.at(3), outpoints.at(4)}, det_rand); + block.vtx.emplace_back(b_tx_conflict_partial); + + auto o_tx_conflict_partial_2 = MakeTransactionSpending({outpoints.at(4), outpoints.at(5)}, det_rand); + BOOST_CHECK(orphanage.AddTx(o_tx_conflict_partial_2, node)); + + orphanage.EraseForBlock(block); + for (const auto& expected_removed : {bo_tx_same_txid, o_tx_same_txid_diff_witness, o_tx_conflict, o_tx_conflict_partial_2}) { + const auto& expected_removed_wtxid = expected_removed->GetWitnessHash(); + BOOST_CHECK(!orphanage.HaveTx(expected_removed_wtxid)); + } + // Only remaining tx is control_tx + BOOST_CHECK_EQUAL(orphanage.Size(), 1); + BOOST_CHECK(orphanage.HaveTx(control_tx->GetWitnessHash())); +} + +BOOST_AUTO_TEST_CASE(multiple_announcers) +{ + const NodeId node0{0}; + const NodeId node1{1}; + const NodeId node2{2}; + size_t expected_total_count{0}; + FastRandomContext det_rand{true}; + TxOrphanageTest orphanage{det_rand}; + + // Check accounting per peer. + // Check that EraseForPeer works with multiple announcers. + { + auto ptx = MakeTransactionSpending({}, det_rand); + const auto& wtxid = ptx->GetWitnessHash(); + BOOST_CHECK(orphanage.AddTx(ptx, node0)); + BOOST_CHECK(orphanage.HaveTx(wtxid)); + expected_total_count += 1; + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + + // Adding again should do nothing. + BOOST_CHECK(!orphanage.AddTx(ptx, node0)); + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + + // We can add another tx with the same txid but different witness. + auto ptx_mutated{MakeMutation(ptx)}; + BOOST_CHECK(orphanage.AddTx(ptx_mutated, node0)); + BOOST_CHECK(orphanage.HaveTx(ptx_mutated->GetWitnessHash())); + expected_total_count += 1; + + BOOST_CHECK(!orphanage.AddTx(ptx, node0)); + + // Adding a new announcer should not change overall accounting. + BOOST_CHECK(orphanage.AddAnnouncer(ptx->GetWitnessHash(), node2)); + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + + // If we already have this announcer, AddAnnouncer returns false. + BOOST_CHECK(orphanage.HaveTxFromPeer(ptx->GetWitnessHash(), node2)); + BOOST_CHECK(!orphanage.AddAnnouncer(ptx->GetWitnessHash(), node2)); + + // Same with using AddTx for an existing tx, which is equivalent to using AddAnnouncer + BOOST_CHECK(!orphanage.AddTx(ptx, node1)); + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + + // if EraseForPeer is called for an orphan with multiple announcers, the orphanage should only + // erase that peer from the announcers set. + orphanage.EraseForPeer(node0); + BOOST_CHECK(orphanage.HaveTx(ptx->GetWitnessHash())); + BOOST_CHECK(!orphanage.HaveTxFromPeer(ptx->GetWitnessHash(), node0)); + // node0 is the only one that announced ptx_mutated + BOOST_CHECK(!orphanage.HaveTx(ptx_mutated->GetWitnessHash())); + expected_total_count -= 1; + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + + // EraseForPeer should delete the orphan if it's the only announcer left. + orphanage.EraseForPeer(node1); + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + BOOST_CHECK(orphanage.HaveTx(ptx->GetWitnessHash())); + orphanage.EraseForPeer(node2); + expected_total_count -= 1; + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + BOOST_CHECK(!orphanage.HaveTx(ptx->GetWitnessHash())); + } + + // Check that erasure for blocks removes for all peers. + { + CBlock block; + auto tx_block = MakeTransactionSpending({}, det_rand); + block.vtx.emplace_back(tx_block); + BOOST_CHECK(orphanage.AddTx(tx_block, node0)); + BOOST_CHECK(!orphanage.AddTx(tx_block, node1)); + + expected_total_count += 1; + + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + + orphanage.EraseForBlock(block); + + expected_total_count -= 1; + + BOOST_CHECK_EQUAL(orphanage.Size(), expected_total_count); + } +} +BOOST_AUTO_TEST_CASE(peer_worksets) +{ + const NodeId node0{0}; + const NodeId node1{1}; + const NodeId node2{2}; + FastRandomContext det_rand{true}; + TxOrphanageTest orphanage{det_rand}; + // AddChildrenToWorkSet should pick an announcer randomly + { + auto tx_missing_parent = MakeTransactionSpending({}, det_rand); + auto tx_orphan = MakeTransactionSpending({COutPoint{tx_missing_parent->GetHash(), 0}}, det_rand); + const auto& orphan_wtxid = tx_orphan->GetWitnessHash(); + + // All 3 peers are announcers. + BOOST_CHECK(orphanage.AddTx(tx_orphan, node0)); + BOOST_CHECK(!orphanage.AddTx(tx_orphan, node1)); + BOOST_CHECK(orphanage.AddAnnouncer(orphan_wtxid, node2)); + for (NodeId node = node0; node <= node2; ++node) { + 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. + + // EraseForPeer also removes that tx from the workset. + orphanage.EraseForPeer(node0); + 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); + for (NodeId node = node0; node <= node2; ++node) { + BOOST_CHECK_EQUAL(orphanage.GetTxToReconsider(node), nullptr); + BOOST_CHECK(!orphanage.HaveTxFromPeer(orphan_wtxid, node)); + } + } +} BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index 6d277b5629..463eb21013 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -146,8 +146,8 @@ BOOST_FIXTURE_TEST_CASE(tx_rejection_types, TestChain100Setup) /*txid_recon=*/txdownload_impl.RecentRejectsReconsiderableFilter().contains(parent_txid), /*wtxid_recon=*/txdownload_impl.RecentRejectsReconsiderableFilter().contains(parent_wtxid), /*keep=*/keep, - /*txid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, GenTxid::Txid(parent_txid), now, /*p2p_inv=*/true), - /*wtxid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, GenTxid::Wtxid(parent_wtxid), now, /*p2p_inv=*/true), + /*txid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, GenTxid::Txid(parent_txid), now), + /*wtxid_inv=*/txdownload_impl.AddTxAnnouncement(nodeid, GenTxid::Wtxid(parent_wtxid), now), }; BOOST_TEST_MESSAGE("Testing behavior for " << result << (segwit_parent ? " segwit " : " nonsegwit")); actual_behavior.CheckEqual(expected_behavior, /*segwit=*/segwit_parent); @@ -231,10 +231,14 @@ BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup) // it's in RecentRejectsFilter. Specifically, the parent is allowed to be in // RecentRejectsReconsiderableFilter, but it cannot be in RecentRejectsFilter. const bool expect_keep_orphan = !parent_recent_rej; + const unsigned int expected_parents = parent_recent_rej || parent_recent_conf || parent_in_mempool ? 0 : 1; + // If we don't expect to keep the orphan then expected_parents is 0. + // !expect_keep_orphan => (expected_parents == 0) + BOOST_CHECK(expect_keep_orphan || expected_parents == 0); const auto ret_1p1c = txdownload_impl.MempoolRejectedTx(orphan, state_orphan, nodeid, /*first_time_failure=*/true); std::string err_msg; const bool ok = CheckOrphanBehavior(txdownload_impl, orphan, ret_1p1c, err_msg, - /*expect_orphan=*/expect_keep_orphan, /*expect_keep=*/true, /*expected_parents=*/expect_keep_orphan ? 1 : 0); + /*expect_orphan=*/expect_keep_orphan, /*expect_keep=*/true, /*expected_parents=*/expected_parents); BOOST_CHECK_MESSAGE(ok, err_msg); } @@ -278,11 +282,12 @@ BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup) for (int32_t i = 1; i < num_parents; ++i) { txdownload_impl.RecentConfirmedTransactionsFilter().insert(parents[i]->GetHash().ToUint256()); } + const unsigned int expected_parents = 1; const auto ret_1recon_conf = txdownload_impl.MempoolRejectedTx(orphan, state_orphan, nodeid, /*first_time_failure=*/true); std::string err_msg; const bool ok = CheckOrphanBehavior(txdownload_impl, orphan, ret_1recon_conf, err_msg, - /*expect_orphan=*/true, /*expect_keep=*/true, /*expected_parents=*/num_parents); + /*expect_orphan=*/true, /*expect_keep=*/true, /*expected_parents=*/expected_parents); BOOST_CHECK_MESSAGE(ok, err_msg); } diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index ba4ba6c3b6..90b3381428 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -16,8 +16,11 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) { const Txid& hash = tx->GetHash(); const Wtxid& wtxid = tx->GetWitnessHash(); - if (m_orphans.count(wtxid)) + if (auto it{m_orphans.find(wtxid)}; it != m_orphans.end()) { + AddAnnouncer(wtxid, peer); + // No new orphan entry was created. An announcer may have been added. return false; + } // Ignore big transactions, to avoid a // send-big-orphans memory exhaustion attack. If a peer has a legitimate @@ -33,7 +36,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return false; } - auto ret = m_orphans.emplace(wtxid, OrphanTx{{tx, peer, Now() + ORPHAN_TX_EXPIRE_TIME}, m_orphan_list.size()}); + auto ret = m_orphans.emplace(wtxid, OrphanTx{{tx, {peer}, Now() + ORPHAN_TX_EXPIRE_TIME}, m_orphan_list.size()}); assert(ret.second); m_orphan_list.push_back(ret.first); for (const CTxIn& txin : tx->vin) { @@ -45,6 +48,20 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return true; } +bool TxOrphanage::AddAnnouncer(const Wtxid& wtxid, NodeId peer) +{ + const auto it = m_orphans.find(wtxid); + if (it != m_orphans.end()) { + Assume(!it->second.announcers.empty()); + const auto ret = it->second.announcers.insert(peer); + if (ret.second) { + LogDebug(BCLog::TXPACKAGES, "added peer=%d as announcer of orphan tx %s\n", peer, wtxid.ToString()); + return true; + } + } + return false; +} + int TxOrphanage::EraseTx(const Wtxid& wtxid) { std::map::iterator it = m_orphans.find(wtxid); @@ -89,9 +106,15 @@ void TxOrphanage::EraseForPeer(NodeId peer) while (iter != m_orphans.end()) { // increment to avoid iterator becoming invalid after erasure - const auto& [wtxid, orphan] = *iter++; - if (orphan.fromPeer == peer) { - nErased += EraseTx(wtxid); + auto& [wtxid, orphan] = *iter++; + auto orphan_it = orphan.announcers.find(peer); + if (orphan_it != orphan.announcers.end()) { + orphan.announcers.erase(peer); + + // No remaining annnouncers: clean up entry + if (orphan.announcers.empty()) { + nErased += EraseTx(orphan.tx->GetWitnessHash()); + } } } if (nErased > 0) LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); @@ -110,7 +133,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { std::map::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += EraseTx(maybeErase->second.tx->GetWitnessHash()); + nErased += EraseTx(maybeErase->first); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -123,7 +146,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size()); - EraseTx(m_orphan_list[randompos]->second.tx->GetWitnessHash()); + EraseTx(m_orphan_list[randompos]->first); ++nEvicted; } if (nEvicted > 0) LogDebug(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); @@ -135,13 +158,17 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) 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) { - // 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(elem->second.fromPeer).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(), elem->second.fromPeer); + // 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); + } } } } @@ -152,6 +179,18 @@ bool TxOrphanage::HaveTx(const Wtxid& wtxid) const return m_orphans.count(wtxid); } +CTransactionRef TxOrphanage::GetTx(const Wtxid& wtxid) const +{ + auto it = m_orphans.find(wtxid); + return it != m_orphans.end() ? it->second.tx : nullptr; +} + +bool TxOrphanage::HaveTxFromPeer(const Wtxid& wtxid, NodeId peer) const +{ + auto it = m_orphans.find(wtxid); + return (it != m_orphans.end() && it->second.announcers.contains(peer)); +} + CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) { auto work_set_it = m_peer_work_set.find(peer); @@ -219,7 +258,7 @@ std::vector TxOrphanage::GetChildrenFromSamePeer(const CTransac const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i)); if (it_by_prev != m_outpoint_to_orphan_it.end()) { for (const auto& elem : it_by_prev->second) { - if (elem->second.fromPeer == nodeid) { + if (elem->second.announcers.contains(nodeid)) { iters.emplace_back(elem); } } @@ -248,42 +287,12 @@ std::vector TxOrphanage::GetChildrenFromSamePeer(const CTransac return children_found; } -std::vector> TxOrphanage::GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const -{ - // First construct vector of iterators to ensure we do not return duplicates of the same tx. - std::vector iters; - - // For each output, get all entries spending this prevout, filtering for ones not from the specified peer. - for (unsigned int i = 0; i < parent->vout.size(); i++) { - const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i)); - if (it_by_prev != m_outpoint_to_orphan_it.end()) { - for (const auto& elem : it_by_prev->second) { - if (elem->second.fromPeer != nodeid) { - iters.emplace_back(elem); - } - } - } - } - - // Erase duplicates - std::sort(iters.begin(), iters.end(), IteratorComparator()); - iters.erase(std::unique(iters.begin(), iters.end()), iters.end()); - - // Convert iterators to pair - std::vector> children_found; - children_found.reserve(iters.size()); - for (const auto& child_iter : iters) { - children_found.emplace_back(child_iter->second.tx, child_iter->second.fromPeer); - } - return children_found; -} - std::vector TxOrphanage::GetOrphanTransactions() const { std::vector ret; ret.reserve(m_orphans.size()); for (auto const& o : m_orphans) { - ret.push_back({o.second.tx, o.second.fromPeer, o.second.nTimeExpire}); + ret.push_back({o.second.tx, o.second.announcers, o.second.nTimeExpire}); } return ret; } diff --git a/src/txorphanage.h b/src/txorphanage.h index b1fae3fa00..868741e789 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -30,9 +30,17 @@ public: /** Add a new orphan transaction */ bool AddTx(const CTransactionRef& tx, NodeId peer); + /** Add an additional announcer to an orphan if it exists. Otherwise, do nothing. */ + bool AddAnnouncer(const Wtxid& wtxid, NodeId peer); + + CTransactionRef GetTx(const Wtxid& wtxid) const; + /** Check if we already have an orphan transaction (by wtxid only) */ bool HaveTx(const Wtxid& wtxid) const; + /** Check if a {tx, peer} exists in the orphanage.*/ + bool HaveTxFromPeer(const Wtxid& wtxid, NodeId peer) const; + /** Extract a transaction from a peer's work set * Returns nullptr if there are no transactions to work on. * Otherwise returns the transaction reference, and removes @@ -43,7 +51,8 @@ public: /** Erase an orphan by wtxid */ int EraseTx(const Wtxid& wtxid); - /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ + /** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan + * has been announced by another peer, don't erase, just remove this peer from the list of announcers. */ void EraseForPeer(NodeId peer); /** Erase all orphans included in or invalidated by a new block */ @@ -62,10 +71,6 @@ public: * recent to least recent. */ std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const; - /** Get all children that spend from this tx but were not received from nodeid. Also return - * which peer provided each tx. */ - std::vector> GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const; - /** Return how many entries exist in the orphange */ size_t Size() const { @@ -75,7 +80,8 @@ public: /** Allows providing orphan information externally */ struct OrphanTxBase { CTransactionRef tx; - NodeId fromPeer; + /** Peers added with AddTx or AddAnnouncer. */ + std::set announcers; NodeSeconds nTimeExpire; }; diff --git a/src/txrequest.cpp b/src/txrequest.cpp index 96ea716481..ca68a99868 100644 --- a/src/txrequest.cpp +++ b/src/txrequest.cpp @@ -574,6 +574,23 @@ public: } } + std::vector GetCandidatePeers(const CTransactionRef& tx) const + { + // Search by txid and, if the tx has a witness, wtxid + std::vector hashes{tx->GetHash().ToUint256()}; + if (tx->HasWitness()) hashes.emplace_back(tx->GetWitnessHash().ToUint256()); + + std::vector result_peers; + for (const uint256& txhash : hashes) { + auto it = m_index.get().lower_bound(ByTxHashView{txhash, State::CANDIDATE_DELAYED, 0}); + while (it != m_index.get().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, std::chrono::microseconds reqtime) { @@ -721,6 +738,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 TxRequestTracker::GetCandidatePeers(const CTransactionRef& tx) const { return m_impl->GetCandidatePeers(tx); } void TxRequestTracker::SanityCheck() const { m_impl->SanityCheck(); } void TxRequestTracker::PostGetRequestableSanityCheck(std::chrono::microseconds now) const diff --git a/src/txrequest.h b/src/txrequest.h index cd3042c87e..95a1e9e7f6 100644 --- a/src/txrequest.h +++ b/src/txrequest.h @@ -195,6 +195,9 @@ public: /** 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 GetCandidatePeers(const CTransactionRef& tx) const; + /** Access to the internal priority computation (testing only) */ uint64_t ComputePriority(const uint256& txhash, NodeId peer, bool preferred) const; diff --git a/test/functional/p2p_1p1c_network.py b/test/functional/p2p_1p1c_network.py index cdc4e1691d..eef307d1eb 100755 --- a/test/functional/p2p_1p1c_network.py +++ b/test/functional/p2p_1p1c_network.py @@ -143,12 +143,6 @@ class PackageRelayTest(BitcoinTestFramework): for (i, peer) in enumerate(self.peers): for tx in transactions_to_presend[i]: peer.send_and_ping(msg_tx(tx)) - # This disconnect removes any sent orphans from the orphanage (EraseForPeer) and times - # out the in-flight requests. It is currently required for the test to pass right now, - # because the node will not reconsider an orphan tx and will not (re)try requesting - # orphan parents from multiple peers if the first one didn't respond. - # TODO: remove this in the future if the node tries orphan resolution with multiple peers. - peer.peer_disconnect() self.log.info("Submit full packages to node0") for package_hex in packages_to_submit: diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index 4477046c8d..5cf616b3ef 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/test/functional/p2p_opportunistic_1p1c.py @@ -215,7 +215,7 @@ class PackageRelayTest(BitcoinTestFramework): @cleanup def test_orphan_consensus_failure(self): - self.log.info("Check opportunistic 1p1c logic with consensus-invalid orphan causes disconnect of the correct peer") + self.log.info("Check opportunistic 1p1c logic requires parent and child to be from the same peer") node = self.nodes[0] low_fee_parent = self.create_tx_below_mempoolminfee(self.wallet) coin = low_fee_parent["new_utxo"] @@ -239,15 +239,17 @@ class PackageRelayTest(BitcoinTestFramework): parent_txid_int = int(low_fee_parent["txid"], 16) bad_orphan_sender.wait_for_getdata([parent_txid_int]) - # 3. A different peer relays the parent. Parent+Child are evaluated as a package and rejected. - parent_sender.send_message(msg_tx(low_fee_parent["tx"])) + # 3. A different peer relays the parent. Package is not evaluated because the transactions + # were not sent from the same peer. + parent_sender.send_and_ping(msg_tx(low_fee_parent["tx"])) # 4. Transactions should not be in mempool. node_mempool = node.getrawmempool() assert low_fee_parent["txid"] not in node_mempool assert tx_orphan_bad_wit.rehash() not in node_mempool - # 5. Peer that sent a consensus-invalid transaction should be disconnected. + # 5. Have the other peer send the tx too, so that tx_orphan_bad_wit package is attempted. + bad_orphan_sender.send_message(msg_tx(low_fee_parent["tx"])) bad_orphan_sender.wait_for_disconnect() # The peer that didn't provide the orphan should not be disconnected. @@ -279,20 +281,17 @@ class PackageRelayTest(BitcoinTestFramework): package_sender.wait_for_getdata([parent_txid_int]) # 3. A different node relays the parent. The parent is first evaluated by itself and - # rejected for being too low feerate. Then it is evaluated as a package and, after passing - # feerate checks, rejected for having a bad signature (consensus error). - fake_parent_sender.send_message(msg_tx(tx_parent_bad_wit)) + # rejected for being too low feerate. It is not evaluated as a package because the child was + # sent from a different peer, so we don't find out that the child is consensus-invalid. + fake_parent_sender.send_and_ping(msg_tx(tx_parent_bad_wit)) # 4. Transactions should not be in mempool. node_mempool = node.getrawmempool() assert tx_parent_bad_wit.rehash() not in node_mempool assert high_fee_child["txid"] not in node_mempool - # 5. Peer sent a consensus-invalid transaction. - fake_parent_sender.wait_for_disconnect() - self.log.info("Check that fake parent does not cause orphan to be deleted and real package can still be submitted") - # 6. Child-sending should not have been punished and the orphan should remain in orphanage. + # 5. Child-sending should not have been punished and the orphan should remain in orphanage. # It can send the "real" parent transaction, and the package is accepted. parent_wtxid_int = int(low_fee_parent["tx"].getwtxid(), 16) package_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=parent_wtxid_int)])) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 0864deeb4f..4a2d53cd2b 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -58,6 +58,10 @@ def cleanup(func): self.generate(self.nodes[0], 1) self.nodes[0].disconnect_p2ps() self.nodes[0].bumpmocktime(LONG_TIME_SKIP) + # Check that mempool and orphanage have been cleared + assert_equal(0, len(self.nodes[0].getorphantxs())) + assert_equal(0, len(self.nodes[0].getrawmempool())) + self.wallet.rescan_utxos(include_mempool=True) return wrapper class PeerTxRelayer(P2PTxInvStore): @@ -533,7 +537,7 @@ class OrphanHandlingTest(BitcoinTestFramework): assert tx_middle["txid"] in node_mempool assert tx_grandchild["txid"] in node_mempool assert_equal(node.getmempoolentry(tx_middle["txid"])["wtxid"], tx_middle["wtxid"]) - assert_equal(len(node.getorphantxs()), 0) + self.wait_until(lambda: len(node.getorphantxs()) == 0) @cleanup def test_orphan_txid_inv(self): @@ -585,7 +589,7 @@ class OrphanHandlingTest(BitcoinTestFramework): assert tx_parent["txid"] in node_mempool assert tx_child["txid"] in node_mempool assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) - assert_equal(len(node.getorphantxs()), 0) + self.wait_until(lambda: len(node.getorphantxs()) == 0) @cleanup def test_max_orphan_amount(self): @@ -610,7 +614,7 @@ class OrphanHandlingTest(BitcoinTestFramework): peer_1.sync_with_ping() orphanage = node.getorphantxs() - assert_equal(len(orphanage), DEFAULT_MAX_ORPHAN_TRANSACTIONS) + self.wait_until(lambda: len(node.getorphantxs()) == DEFAULT_MAX_ORPHAN_TRANSACTIONS) for orphan in orphans: assert tx_in_orphanage(node, orphan) @@ -626,8 +630,173 @@ class OrphanHandlingTest(BitcoinTestFramework): self.log.info("Clearing the orphanage") for index, parent_orphan in enumerate(parent_orphans): peer_1.send_and_ping(msg_tx(parent_orphan)) - assert_equal(len(node.getorphantxs()),0) + self.wait_until(lambda: len(node.getorphantxs()) == 0) + @cleanup + def test_orphan_handling_prefer_outbound(self): + self.log.info("Test that the node prefers requesting from outbound peers") + node = self.nodes[0] + orphan_wtxid, orphan_tx, parent_tx = self.create_parent_and_child() + orphan_inv = CInv(t=MSG_WTX, h=int(orphan_wtxid, 16)) + + peer_inbound = node.add_p2p_connection(PeerTxRelayer()) + peer_outbound = node.add_outbound_p2p_connection(PeerTxRelayer(), p2p_idx=1) + + # Inbound peer relays the transaction. + peer_inbound.send_and_ping(msg_inv([orphan_inv])) + self.nodes[0].bumpmocktime(TXREQUEST_TIME_SKIP) + peer_inbound.wait_for_getdata([int(orphan_wtxid, 16)]) + + # Both peers send invs for the orphan, so the node can expect both to know its ancestors. + peer_outbound.send_and_ping(msg_inv([orphan_inv])) + + peer_inbound.send_and_ping(msg_tx(orphan_tx)) + + # There should be 1 orphan with 2 announcers (we don't know what their peer IDs are) + orphanage = node.getorphantxs(verbosity=2) + assert_equal(orphanage[0]["wtxid"], orphan_wtxid) + assert_equal(len(orphanage[0]["from"]), 2) + + # The outbound peer should be preferred for getting orphan parents + self.nodes[0].bumpmocktime(TXID_RELAY_DELAY) + peer_outbound.wait_for_parent_requests([int(parent_tx.rehash(), 16)]) + + # There should be no request to the inbound peer + peer_inbound.assert_never_requested(int(parent_tx.rehash(), 16)) + + self.log.info("Test that, if the preferred peer doesn't respond, the node sends another request") + self.nodes[0].bumpmocktime(GETDATA_TX_INTERVAL) + peer_inbound.sync_with_ping() + peer_inbound.wait_for_parent_requests([int(parent_tx.rehash(), 16)]) + + @cleanup + def test_announcers_before_and_after(self): + self.log.info("Test that the node uses all peers who announced the tx prior to realizing it's an orphan") + node = self.nodes[0] + orphan_wtxid, orphan_tx, parent_tx = self.create_parent_and_child() + orphan_inv = CInv(t=MSG_WTX, h=int(orphan_wtxid, 16)) + + # Announces before tx is sent, disconnects while node is requesting parents + peer_early_disconnected = node.add_outbound_p2p_connection(PeerTxRelayer(), p2p_idx=3) + # Announces before tx is sent, doesn't respond to parent request + peer_early_unresponsive = node.add_p2p_connection(PeerTxRelayer()) + + # Announces after tx is sent + peer_late_announcer = node.add_p2p_connection(PeerTxRelayer()) + + # Both peers send invs for the orphan, so the node can expect both to know its ancestors. + peer_early_disconnected.send_and_ping(msg_inv([orphan_inv])) + self.nodes[0].bumpmocktime(TXREQUEST_TIME_SKIP) + peer_early_disconnected.wait_for_getdata([int(orphan_wtxid, 16)]) + peer_early_unresponsive.send_and_ping(msg_inv([orphan_inv])) + peer_early_disconnected.send_and_ping(msg_tx(orphan_tx)) + + # There should be 1 orphan with 2 announcers (we don't know what their peer IDs are) + orphanage = node.getorphantxs(verbosity=2) + assert_equal(len(orphanage), 1) + assert_equal(orphanage[0]["wtxid"], orphan_wtxid) + assert_equal(len(orphanage[0]["from"]), 2) + + # Peer disconnects before responding to request + self.nodes[0].bumpmocktime(TXID_RELAY_DELAY) + peer_early_disconnected.wait_for_parent_requests([int(parent_tx.rehash(), 16)]) + peer_early_disconnected.peer_disconnect() + + # The orphan should have 1 announcer left after the node finishes disconnecting peer_early_disconnected. + self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) == 1) + + # The node should retry with the other peer that announced the orphan earlier. + # This node's request was additionally delayed because it's an inbound peer. + self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY) + peer_early_unresponsive.wait_for_parent_requests([int(parent_tx.rehash(), 16)]) + + self.log.info("Test that the node uses peers who announce the tx after realizing it's an orphan") + peer_late_announcer.send_and_ping(msg_inv([orphan_inv])) + + # The orphan should have 2 announcers now + orphanage = node.getorphantxs(verbosity=2) + assert_equal(orphanage[0]["wtxid"], orphan_wtxid) + assert_equal(len(orphanage[0]["from"]), 2) + + self.nodes[0].bumpmocktime(GETDATA_TX_INTERVAL) + peer_late_announcer.wait_for_parent_requests([int(parent_tx.rehash(), 16)]) + + @cleanup + def test_parents_change(self): + self.log.info("Test that, if a parent goes missing during orphan reso, it is requested") + node = self.nodes[0] + # Orphan will have 2 parents, 1 missing and 1 already in mempool when received. + # Create missing parent. + parent_missing = self.wallet.create_self_transfer() + + # Create parent that will already be in mempool, but become missing during orphan resolution. + # Get 3 UTXOs for replacement-cycled parent, UTXOS A, B, C + coin_A = self.wallet.get_utxo(confirmed_only=True) + coin_B = self.wallet.get_utxo(confirmed_only=True) + coin_C = self.wallet.get_utxo(confirmed_only=True) + # parent_peekaboo_AB spends A and B. It is replaced by tx_replacer_BC (conflicting UTXO B), + # and then replaced by tx_replacer_C (conflicting UTXO C). This replacement cycle is used to + # ensure that parent_peekaboo_AB can be reintroduced without requiring package RBF. + FEE_INCREMENT = 2400 + parent_peekaboo_AB = self.wallet.create_self_transfer_multi( + utxos_to_spend=[coin_A, coin_B], + num_outputs=1, + fee_per_output=FEE_INCREMENT + ) + tx_replacer_BC = self.wallet.create_self_transfer_multi( + utxos_to_spend=[coin_B, coin_C], + num_outputs=1, + fee_per_output=2*FEE_INCREMENT + ) + tx_replacer_C = self.wallet.create_self_transfer( + utxo_to_spend=coin_C, + fee_per_output=3*FEE_INCREMENT + ) + + # parent_peekaboo_AB starts out in the mempool + node.sendrawtransaction(parent_peekaboo_AB["hex"]) + + orphan = self.wallet.create_self_transfer_multi(utxos_to_spend=[parent_peekaboo_AB["new_utxos"][0], parent_missing["new_utxo"]]) + orphan_wtxid = orphan["wtxid"] + orphan_inv = CInv(t=MSG_WTX, h=int(orphan_wtxid, 16)) + + # peer1 sends the orphan and gets a request for the missing parent + peer1 = node.add_p2p_connection(PeerTxRelayer()) + peer1.send_and_ping(msg_inv([orphan_inv])) + node.bumpmocktime(TXREQUEST_TIME_SKIP) + peer1.wait_for_getdata([int(orphan_wtxid, 16)]) + peer1.send_and_ping(msg_tx(orphan["tx"])) + self.wait_until(lambda: node.getorphantxs(verbosity=0) == [orphan["txid"]]) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + peer1.wait_for_getdata([int(parent_missing["txid"], 16)]) + + # Replace parent_peekaboo_AB so that is is a newly missing parent. + # Then, replace the replacement so that it can be resubmitted. + node.sendrawtransaction(tx_replacer_BC["hex"]) + assert tx_replacer_BC["txid"] in node.getrawmempool() + node.sendrawtransaction(tx_replacer_C["hex"]) + assert tx_replacer_BC["txid"] not in node.getrawmempool() + assert tx_replacer_C["txid"] in node.getrawmempool() + + # Second peer is an additional announcer for this orphan + peer2 = node.add_p2p_connection(PeerTxRelayer()) + peer2.send_and_ping(msg_inv([orphan_inv])) + assert_equal(len(node.getorphantxs(verbosity=2)[0]["from"]), 2) + + # Disconnect peer1. peer2 should become the new candidate for orphan resolution. + peer1.peer_disconnect() + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + self.wait_until(lambda: len(node.getorphantxs(verbosity=2)[0]["from"]) == 1) + # Both parents should be requested, now that they are both missing. + peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)]) + peer2.send_and_ping(msg_tx(parent_missing["tx"])) + peer2.send_and_ping(msg_tx(parent_peekaboo_AB["tx"])) + + final_mempool = node.getrawmempool() + assert parent_missing["txid"] in final_mempool + assert parent_peekaboo_AB["txid"] in final_mempool + assert orphan["txid"] in final_mempool + assert tx_replacer_C["txid"] in final_mempool def run_test(self): self.nodes[0].setmocktime(int(time.time())) @@ -635,6 +804,7 @@ class OrphanHandlingTest(BitcoinTestFramework): self.generate(self.wallet_nonsegwit, 10) self.wallet = MiniWallet(self.nodes[0]) self.generate(self.wallet, 160) + self.test_arrival_timing_orphan() self.test_orphan_rejected_parents_exceptions() self.test_orphan_multiple_parents() @@ -645,6 +815,9 @@ class OrphanHandlingTest(BitcoinTestFramework): self.test_same_txid_orphan_of_orphan() self.test_orphan_txid_inv() self.test_max_orphan_amount() + self.test_orphan_handling_prefer_outbound() + self.test_announcers_before_and_after() + self.test_parents_change() if __name__ == '__main__': diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 677a1120d6..9caf5a19aa 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -2051,6 +2051,9 @@ class SegWitTest(BitcoinTestFramework): self.wtx_node.last_message.pop("getdata", None) test_transaction_acceptance(self.nodes[0], self.wtx_node, tx2, with_witness=True, accepted=False) + # Disconnect tx_node to avoid the possibility of it being selected for orphan resolution. + self.tx_node.peer_disconnect() + # Expect a request for parent (tx) by txid despite use of WTX peer self.wtx_node.wait_for_getdata([tx.sha256], timeout=60) with p2p_lock: diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py index 4871166a39..3dff1ac9d9 100755 --- a/test/functional/rpc_orphans.py +++ b/test/functional/rpc_orphans.py @@ -10,7 +10,12 @@ from test_framework.mempool_util import ( ORPHAN_TX_EXPIRE_TIME, tx_in_orphanage, ) -from test_framework.messages import msg_tx +from test_framework.messages import ( + CInv, + msg_inv, + msg_tx, + MSG_WTX, +) from test_framework.p2p import P2PInterface from test_framework.util import ( assert_equal, @@ -106,13 +111,19 @@ class OrphanRPCsTest(BitcoinTestFramework): self.log.info("Check that orphan 1 and 2 were from different peers") assert orphanage[0]["from"][0] != orphanage[1]["from"][0] + peer_ids = [orphanage[0]["from"][0], orphanage[1]["from"][0]] self.log.info("Unorphan child 2") peer_2.send_and_ping(msg_tx(tx_parent_2["tx"])) assert not tx_in_orphanage(node, tx_child_2["tx"]) + self.log.info("Check that additional announcers are reflected in RPC result") + peer_2.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=int(tx_child_1["wtxid"], 16))])) + + orphanage = node.getorphantxs(verbosity=2) + assert_equal(set(orphanage[0]["from"]), set(peer_ids)) + self.log.info("Checking orphan details") - orphanage = node.getorphantxs(verbosity=1) assert_equal(len(node.getorphantxs()), 1) orphan_1 = orphanage[0] self.orphan_details_match(orphan_1, tx_child_1, verbosity=1)