From 6951ddcefd9e05f31ee7634bbfbf1d19e04ec00e Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 20 Mar 2023 10:37:23 +0000 Subject: [PATCH 01/14] [txrequest] GetCandidatePeers Needed for a later commit adding logic to ask the TxRequestTracker for a list of announcers. These announcers should know the parents of the transaction they announced. --- src/txrequest.cpp | 18 ++++++++++++++++++ src/txrequest.h | 3 +++ 2 files changed, 21 insertions(+) diff --git a/src/txrequest.cpp b/src/txrequest.cpp index 96ea716481c..ca68a998680 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 cd3042c87e5..95a1e9e7f6b 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; From 62a9ff187076686b39dca64ad4f2f439da0875d1 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 26 Jul 2024 14:10:21 +0100 Subject: [PATCH 02/14] [refactor] change type of unique_parents to Txid --- src/net_processing.cpp | 2 +- src/node/txdownloadman.h | 2 +- src/node/txdownloadman_impl.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e503a683827..663dbb7634b 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); } diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 81b0c76e0a2..2d066cab53f 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; }; diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index f9635d049ad..e9fe958f5dc 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -308,7 +308,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; From e810842acda6fe56e0536ebecfbb9d17d26e1513 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 18 Apr 2023 16:02:58 +0100 Subject: [PATCH 03/14] [txorphanage] support multiple announcers Add ability to add and track multiple announcers per orphan transaction, erasing announcers but not the entire orphan. The tx creation code in orphanage_tests needs to be updated so that each tx is unique, because the CountOrphans() check assumes that calling EraseForPeer necessarily means its orphans are deleted. Unused for now. --- src/rpc/mempool.cpp | 4 +- src/test/orphanage_tests.cpp | 4 +- src/txorphanage.cpp | 71 +++++++++++++++++++++++++++--------- src/txorphanage.h | 12 +++++- 4 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 6d462a7e851..5239886fafd 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -843,7 +843,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/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 799f2c0fec4..07e7002cadc 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -132,7 +132,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 +148,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)); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index ba4ba6c3b62..a108ce94370 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,12 @@ bool TxOrphanage::HaveTx(const Wtxid& wtxid) const return m_orphans.count(wtxid); } +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 +252,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); } } @@ -258,7 +291,7 @@ std::vector> TxOrphanage::GetChildrenFromDiff 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); } } @@ -273,7 +306,9 @@ std::vector> TxOrphanage::GetChildrenFromDiff 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); + // Use first peer in announcers list + auto peer = *(child_iter->second.announcers.begin()); + children_found.emplace_back(child_iter->second.tx, peer); } return children_found; } @@ -283,7 +318,7 @@ std::vector TxOrphanage::GetOrphanTransactions() cons 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 b1fae3fa008..76ff8313a5a 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -30,9 +30,15 @@ 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); + /** 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 +49,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 */ @@ -75,7 +82,8 @@ public: /** Allows providing orphan information externally */ struct OrphanTxBase { CTransactionRef tx; - NodeId fromPeer; + /** Peers added with AddTx or AddAnnouncer. */ + std::set announcers; NodeSeconds nTimeExpire; }; From 04448ce32a3bc4b6d12293f71e40333abe35c224 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 3 Jan 2025 09:04:10 -0500 Subject: [PATCH 04/14] [txorphanage] add GetTx so that orphan vin can be read --- src/test/fuzz/txorphan.cpp | 3 +++ src/test/orphanage_tests.cpp | 2 ++ src/txorphanage.cpp | 6 ++++++ src/txorphanage.h | 2 ++ 4 files changed, 13 insertions(+) diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 31af1afff52..09056225db1 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -157,5 +157,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 07e7002cadc..5d09fac4af7 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -250,7 +250,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)); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index a108ce94370..17b6622a567 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -179,6 +179,12 @@ 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); diff --git a/src/txorphanage.h b/src/txorphanage.h index 76ff8313a5a..6998a9aab18 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -33,6 +33,8 @@ public: /** 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; From 96c1a822a274689611f409246ef1573906b0083e Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 31 Jul 2024 10:24:39 +0100 Subject: [PATCH 05/14] [unit test] TxOrphanage EraseForBlock --- src/test/orphanage_tests.cpp | 54 ++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 5d09fac4af7..49ddd10863e 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -392,4 +392,58 @@ 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_SUITE_END() From 22b023b09da3e2fe00467c77b105a61c1961081f Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 3 Jan 2025 10:44:36 -0500 Subject: [PATCH 06/14] [unit test] multiple orphan announcers --- src/test/orphanage_tests.cpp | 126 +++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 49ddd10863e..a33db09ad77 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -446,4 +446,130 @@ BOOST_AUTO_TEST_CASE(process_block) 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() From 163aaf285af91b49c2d788463dc1e1654c88ade6 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 31 Jul 2024 10:52:01 +0100 Subject: [PATCH 07/14] [fuzz] orphanage multiple announcer functions --- src/test/fuzz/txorphan.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 09056225db1..26afff5bff5 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -128,6 +128,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 +154,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) }, [&] { orphanage.EraseForPeer(peer_id); + Assert(!orphanage.HaveTxFromPeer(tx->GetWitnessHash(), peer_id)); }, [&] { // test mocktime and expiry From c6893b0f0b7b205c8da4b9d281a55c9eb843b582 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 28 Nov 2024 06:43:08 -0500 Subject: [PATCH 08/14] [txdownload] remove unique_parents that we already have This means we no longer return parents we already have in the m_unique_parents result from MempoolRejectedTx. We need to separate the loop that checks AlreadyHave parents from the loop that adds parents as announcements, because we may do the latter loop multiple times for different peers. --- src/node/txdownloadman_impl.cpp | 12 +++++++----- src/test/txdownload_tests.cpp | 9 +++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index e9fe958f5dc..804c00494fb 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -348,6 +348,12 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction } } if (!fRejectedParents) { + // 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 current_time{GetTime()}; for (const uint256& parent_txid : unique_parents) { @@ -357,11 +363,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // 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); - } + AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false); } // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index 6d277b5629f..88064b5a5f8 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -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); } From 1d2e1d709ce3d95d409254c860347bc3fedf30e1 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 3 Jan 2025 09:05:04 -0500 Subject: [PATCH 09/14] [refactor] move creation of unique_parents to helper function This function will be reused in a later commit. --- src/node/txdownloadman_impl.cpp | 23 ++++++++++++++++------- src/node/txdownloadman_impl.h | 4 ++++ 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 804c00494fb..2057c8560a7 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -301,6 +301,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}; @@ -320,13 +335,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 diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index 8039ddb3cbb..f97ff0609a6 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -189,6 +189,10 @@ 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); }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_IMPL_H From b6ea4a9afe2d8bbf49b6b6c42f0a3ce4390c4535 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 26 Jul 2024 15:02:02 +0100 Subject: [PATCH 10/14] [p2p] try multiple peers for orphan resolution Co-authored-by: dergoegge --- src/node/txdownloadman_impl.cpp | 102 +++++++++++++++++++++++++++----- src/node/txdownloadman_impl.h | 6 ++ test/functional/p2p_segwit.py | 3 + 3 files changed, 97 insertions(+), 14 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 2057c8560a7..c7eacff4b6b 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -174,9 +174,37 @@ void TxDownloadManagerImpl::DisconnectedPeer(NodeId nodeid) bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, std::chrono::microseconds now, bool p2p_inv) { + // If this is an orphan we are trying to resolve, consider this peer as a orphan resolution candidate instead. + // - received as an p2p inv + // - is wtxid matching something in orphanage + // - exists in orphanage + // - peer can be an orphan resolution candidate + if (p2p_inv && 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; auto it = m_peer_info.find(peer); @@ -204,6 +232,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; @@ -363,26 +421,42 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction std::erase_if(unique_parents, [&](const auto& txid){ return AlreadyHaveTx(GenTxid::Txid(txid), /*include_reconsiderable=*/false); }); - const auto current_time{GetTime()}; + 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)}; - 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()); + } + }; + + // 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); } - // 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); - // 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 f97ff0609a6..e5e487c4792 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -193,6 +193,12 @@ public: 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/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 677a1120d6a..9caf5a19aad 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: From 0da693f7e129fccaecf9a2c177083d2e80d37781 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 26 Jul 2024 16:34:55 +0100 Subject: [PATCH 11/14] [functional test] orphan handling with multiple announcers --- test/functional/p2p_1p1c_network.py | 6 - test/functional/p2p_orphan_handling.py | 181 ++++++++++++++++++++++++- 2 files changed, 177 insertions(+), 10 deletions(-) diff --git a/test/functional/p2p_1p1c_network.py b/test/functional/p2p_1p1c_network.py index cdc4e1691d6..eef307d1eb0 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_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 0864deeb4fa..4a2d53cd2b3 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__': From 063c1324c143d98e6d5108bb51b3ca59b45f9b85 Mon Sep 17 00:00:00 2001 From: glozow Date: Wed, 11 Dec 2024 07:18:10 -0500 Subject: [PATCH 12/14] [functional test] getorphantxs reflects multiple announcers --- test/functional/rpc_orphans.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py index 4871166a390..3dff1ac9d97 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) From f7658d9b1475ecaa5cb8e543e5c66a3a3a2dc1fb Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 6 Dec 2024 07:02:14 -0500 Subject: [PATCH 13/14] [cleanup] remove p2p_inv from AddTxAnnouncement This param is no longer needed since orphan parent requests are added to the TxRequestTracker directly. --- src/net_processing.cpp | 2 +- src/node/txdownloadman.h | 3 +-- src/node/txdownloadman_impl.cpp | 11 +++++------ src/node/txdownloadman_impl.h | 2 +- src/test/fuzz/txdownloadman.cpp | 4 ++-- src/test/txdownload_tests.cpp | 4 ++-- 6 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 663dbb7634b..e4c57c0336d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3935,7 +3935,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 2d066cab53f..1ae833a6be6 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -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 c7eacff4b6b..82443115bad 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,14 +172,13 @@ 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. - // - received as an p2p inv // - is wtxid matching something in orphanage // - exists in orphanage // - peer can be an orphan resolution candidate - if (p2p_inv && gtxid.IsWtxid()) { + 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){ @@ -205,7 +204,7 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, } // If this is an inv received from a peer and we already have it, we can drop it. - 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; diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index e5e487c4792..ab563b22415 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); diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index 8a4d7d55c26..f78cd71ce9c 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/src/test/fuzz/txdownloadman.cpp @@ -227,7 +227,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); @@ -370,7 +370,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/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index 88064b5a5f8..463eb210139 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); From 86d7135e36efd39781cf4c969011df99f0cbb69d Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 20 Aug 2024 11:39:18 +0100 Subject: [PATCH 14/14] [p2p] only attempt 1p1c when both txns provided by the same peer Now that we track all announcers of an orphan, it's not helpful to consider an orphan provided by a peer that didn't send us this parent. It can only hurt our chances of finding the right orphan when there are multiple candidates. Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c packages from different peers. Instead of checking that the right peer is punished, we now check that the package is not submitted. We can't use the functional test to see that the package was not considered because the behavior is indistinguishable (except for the logs). --- src/node/txdownloadman_impl.cpp | 34 +++-------------------- src/test/fuzz/txorphan.cpp | 6 ---- src/test/orphanage_tests.cpp | 18 ------------ src/txorphanage.cpp | 32 --------------------- src/txorphanage.h | 4 --- test/functional/p2p_opportunistic_1p1c.py | 21 +++++++------- 6 files changed, 14 insertions(+), 101 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 82443115bad..fe961048d34 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -300,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 @@ -315,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; } diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 26afff5bff5..61c2308a296 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 diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index a33db09ad77..f30d4b402f8 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) { @@ -312,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()); @@ -322,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 @@ -346,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: @@ -354,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: @@ -362,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: @@ -370,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))); } } } diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 17b6622a567..90b3381428b 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -287,38 +287,6 @@ 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.announcers.contains(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) { - // Use first peer in announcers list - auto peer = *(child_iter->second.announcers.begin()); - children_found.emplace_back(child_iter->second.tx, peer); - } - return children_found; -} - std::vector TxOrphanage::GetOrphanTransactions() const { std::vector ret; diff --git a/src/txorphanage.h b/src/txorphanage.h index 6998a9aab18..868741e7890 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -71,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 { diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index 4477046c8d6..5cf616b3ef8 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)]))