Merge bitcoin/bitcoin#31397: p2p: track and use all potential peers for orphan resolution

86d7135e36 [p2p] only attempt 1p1c when both txns provided by the same peer (glozow)
f7658d9b14 [cleanup] remove p2p_inv from AddTxAnnouncement (glozow)
063c1324c1 [functional test] getorphantxs reflects multiple announcers (glozow)
0da693f7e1 [functional test] orphan handling with multiple announcers (glozow)
b6ea4a9afe [p2p] try multiple peers for orphan resolution (glozow)
1d2e1d709c [refactor] move creation of unique_parents to helper function (glozow)
c6893b0f0b [txdownload] remove unique_parents that we already have (glozow)
163aaf285a [fuzz] orphanage multiple announcer functions (glozow)
22b023b09d [unit test] multiple orphan announcers (glozow)
96c1a822a2 [unit test] TxOrphanage EraseForBlock (glozow)
04448ce32a [txorphanage] add GetTx so that orphan vin can be read (glozow)
e810842acd [txorphanage] support multiple announcers (glozow)
62a9ff1870 [refactor] change type of unique_parents to Txid (glozow)
6951ddcefd [txrequest] GetCandidatePeers (glozow)

Pull request description:

  Part of #27463.

  (Transaction) **orphan resolution** is a process that kicks off when we are missing UTXOs to validate an unconfirmed transaction. We currently request missing parents by txid; BIP 331 also defines a way to [explicitly request ancestors](https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better).

  Currently, when we find that a transaction is an orphan, we only try to resolve it with the peer who provided the `tx`. If this doesn't work out (e.g. they send a `notfound` or don't respond), we do not try again. We actually can't, because we've already forgotten who else could resolve this orphan (i.e. all the other peers who announced the transaction).

  What is wrong with this? It makes transaction download less reliable, particularly for 1p1c packages which must go through orphan resolution in order to be downloaded.

  Can we fix this with BIP 331 / is this "duct tape" before the real solution?
  BIP 331 (receiver-initiated ancestor package relay) is also based on the idea that there is an orphan that needs resolution, but it's just a new way of communicating information. It's not inherently more honest; you can request ancestor package information and get a `notfound`. So ancestor package relay still requires some kind of procedure for retrying when an orphan resolution attempt fails. See the #27742 implementation which builds on this orphan resolution tracker to keep track of what packages to download (it just isn't rebased on this exact branch). The difference when using BIP 331 is that we request `ancpkginfo` and then `pkgtxns` instead of the parent txids.

  Zooming out, we'd like orphan handling to be:
  - Bandwidth-efficient: don't have too many requests out at once. As already implemented today, transaction requests for orphan parents and regular download both go through the `TxRequestTracker` so that we don't have duplicate requests out.
  - Not vulnerable to censorship: don't give up too easily, use all candidate peers. See e.g. https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for/
  - Load-balance between peers: don't overload peers; use all peers available. This is also useful for when we introduce per-peer orphan protection, since each peer will have limited slots.

  The approach taken in this PR is to think of each peer who announces an orphan as a potential "orphan resolution candidate." These candidates include:
  - the peer who sent us the orphan tx
  - any peers who announced the orphan prior to us downloading it
  - any peers who subsequently announce the orphan after we have started trying to resolve it
  For each orphan resolution candidate, we treat them as having "announced" all of the missing parents to us at the time of receipt of this orphan transaction (or at the time they announced the tx if they do so after we've already started tracking it as an orphan). We add the missing parents as entries to `m_txrequest`, incorporating the logic of typical txrequest processing, which means we prefer outbounds, try not to have duplicate requests in flight, don't overload peers, etc.

ACKs for top commit:
  marcofleon:
    Code review ACK 86d7135e36
  instagibbs:
    reACK 86d7135e36
  dergoegge:
    Code review ACK 86d7135e36
  mzumsande:
    ACK 86d7135e36

Tree-SHA512: 618d523b86e60c3ea039e88326d50db4e55e8e18309c6a20e8f2b10ed9e076f1de0315c335fd3b8abdabcc8b53cbceb66fb59147d05470ea25b83a2b4bd9c877

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:

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

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__':

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:

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)