[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).

Author: glozow

src/node/txdownloadman_impl.cpp

@@ -300,9 +300,11 @@ std::optional<PackageToValidate> 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<PackageToValidate> 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<size_t> 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;
 }
 

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

src/test/orphanage_tests.cpp

@@ -93,15 +93,6 @@ static bool EqualTxns(const std::set<CTransactionRef>& set_txns, const std::vect
     }
     return true;
 }
-static bool EqualTxns(const std::set<CTransactionRef>& set_txns,
-                      const std::vector<std::pair<CTransactionRef, NodeId>>& 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,18 +303,13 @@ 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());
 
         // 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,31 +332,27 @@ BOOST_AUTO_TEST_CASE(get_children)
             std::set<CTransactionRef> 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:
         {
             std::set<CTransactionRef> 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:
         {
             std::set<CTransactionRef> 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:
         {
             std::set<CTransactionRef> 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)));
         }
     }
 }

src/txorphanage.cpp

@@ -287,38 +287,6 @@ std::vector<CTransactionRef> TxOrphanage::GetChildrenFromSamePeer(const CTransac
     return children_found;
 }
 
-std::vector<std::pair<CTransactionRef, NodeId>> 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<OrphanMap::iterator> 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<CTransactionRef, NodeId>
-    std::vector<std::pair<CTransactionRef, NodeId>> 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::OrphanTxBase> TxOrphanage::GetOrphanTransactions() const
 {
     std::vector<OrphanTxBase> ret;

src/txorphanage.h

@@ -71,10 +71,6 @@ class TxOrphanage {
      * recent to least recent. */
     std::vector<CTransactionRef> 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<std::pair<CTransactionRef, NodeId>> GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const;
-
     /** Return how many entries exist in the orphange */
     size_t Size() const
     {

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