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)]))