From 86d7135e36efd39781cf4c969011df99f0cbb69d Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 20 Aug 2024 11:39:18 +0100 Subject: [PATCH] [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 82443115bad92..fe961048d347c 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 26afff5bff5d9..61c2308a29679 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 a33db09ad77b1..f30d4b402f8b9 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 17b6622a56787..90b3381428bc7 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 6998a9aab1814..868741e789030 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -71,10 +71,6 @@ class TxOrphanage { * 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 4477046c8d6d1..5cf616b3ef8fa 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/test/functional/p2p_opportunistic_1p1c.py @@ -215,7 +215,7 @@ def test_low_and_high_child(self, wallet): @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 @@ def test_orphan_consensus_failure(self): 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 @@ def test_parent_consensus_failure(self): 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)]))