Merge bitcoin/bitcoin#26569: p2p: Ensure transaction announcements are only queued for fully connected peers

8f2dac5 [test] Add p2p_tx_privacy.py (dergoegge)
ce63fca [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
845e3a3 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)

Pull request description:

  `TxRelay::m_next_inv_send_time` is initialized to 0, which means that any txids in `TxRelay::m_tx_inventory_to_send` will be announced on the first call to `PeerManagerImpl::SendMessages` for a fully connected peer (i.e. it completed the version handshake).

  Prior to #21160, `TxRelay::m_tx_inventory_to_send` was guaranteed to be empty on the first `SendMessages` call, as transaction announcements were only queued for fully connected peers. #21160 replaced a `CConnman::ForEachNode` call with a loop over `PeerManagerImpl::m_peer_map`, in which the txid for a transaction to be relayed is added to `TxRelay::m_tx_inventory_to_send` for all peers. Even for those peers that have not completed the version handshake. Prior to the PR this was not the case as `ForEachNode` has a "fully connected check" before calling a function for each node.

ACKs for top commit:
  MarcoFalke:
    ACK 8f2dac5 🔝
  jnewbery:
    utACK 8f2dac5

Tree-SHA512: e9eaccf7e00633ee0806fff1068b0e413a69a5e389d96c9659f68079915a6381ad5040c61f716cfcde77931d1b563b1049da97a232a95c6cd8355bd3d13404b9

Author: fanquake

src/net_processing.cpp

@@ -295,7 +295,7 @@ struct Peer {
         std::atomic<std::chrono::seconds> m_last_mempool_req{0s};
         /** The next time after which we will send an `inv` message containing
          *  transaction announcements to this peer. */
-        std::chrono::microseconds m_next_inv_send_time GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0};
+        std::chrono::microseconds m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){0};
 
         /** Minimum fee rate with which to filter transaction announcements to this node. See BIP133. */
         std::atomic<CAmount> m_fee_filter_received{0};
@@ -2017,8 +2017,15 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid
         auto tx_relay = peer.GetTxRelay();
         if (!tx_relay) continue;
 
-        const uint256& hash{peer.m_wtxid_relay ? wtxid : txid};
         LOCK(tx_relay->m_tx_inventory_mutex);
+        // Only queue transactions for announcement once the version handshake
+        // is completed. The time of arrival for these transactions is
+        // otherwise at risk of leaking to a spy, if the spy is able to
+        // distinguish transactions received during the handshake from the rest
+        // in the announcement.
+        if (tx_relay->m_next_inv_send_time == 0s) continue;
+
+        const uint256& hash{peer.m_wtxid_relay ? wtxid : txid};
         if (!tx_relay->m_tx_inventory_known_filter.contains(hash)) {
             tx_relay->m_tx_inventory_to_send.insert(hash);
         }
@@ -3424,6 +3431,20 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             }
         }
 
+        if (auto tx_relay = peer->GetTxRelay()) {
+            // `TxRelay::m_tx_inventory_to_send` must be empty before the
+            // version handshake is completed as
+            // `TxRelay::m_next_inv_send_time` is first initialised in
+            // `SendMessages` after the verack is received. Any transactions
+            // received during the version handshake would otherwise
+            // immediately be advertised without random delay, potentially
+            // leaking the time of arrival to a spy.
+            Assume(WITH_LOCK(
+                tx_relay->m_tx_inventory_mutex,
+                return tx_relay->m_tx_inventory_to_send.empty() &&
+                       tx_relay->m_next_inv_send_time == 0s));
+        }
+
         pfrom.fSuccessfullyConnected = true;
         return;
     }

test/functional/p2p_tx_privacy.py

@@ -0,0 +1,78 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test that transaction announcements are only queued for peers that have
+successfully completed the version handshake.
+
+Topology:
+
+  tx_originator ----> node[0] <---- spy
+
+We test that a transaction sent by tx_originator is only relayed to spy
+if it was received after spy's version handshake completed.
+
+1. Fully connect tx_originator
+2. Connect spy (no version handshake)
+3. tx_originator sends tx1
+4. spy completes the version handshake
+5. tx_originator sends tx2
+6. We check that only tx2 is announced on the spy interface
+"""
+from test_framework.messages import (
+    msg_wtxidrelay,
+    msg_verack,
+    msg_tx,
+    CInv,
+    MSG_WTX,
+)
+from test_framework.p2p import (
+    P2PInterface,
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.wallet import MiniWallet
+
+class P2PTxSpy(P2PInterface):
+    def __init__(self):
+        super().__init__()
+        self.all_invs = []
+
+    def on_version(self, message):
+        self.send_message(msg_wtxidrelay())
+
+    def on_inv(self, message):
+        self.all_invs += message.inv
+
+    def wait_for_inv_match(self, expected_inv):
+        self.wait_until(lambda: len(self.all_invs) == 1 and self.all_invs[0] == expected_inv)
+
+class TxPrivacyTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def run_test(self):
+        self.wallet = MiniWallet(self.nodes[0])
+        self.wallet.rescan_utxos()
+
+        tx_originator = self.nodes[0].add_p2p_connection(P2PInterface())
+        spy = self.nodes[0].add_p2p_connection(P2PTxSpy(), wait_for_verack=False)
+        spy.wait_for_verack()
+
+        # tx_originator sends tx1
+        tx1 = self.wallet.create_self_transfer()["tx"]
+        tx_originator.send_and_ping(msg_tx(tx1))
+
+        # Spy sends the verack
+        spy.send_and_ping(msg_verack())
+
+        # tx_originator sends tx2
+        tx2 = self.wallet.create_self_transfer()["tx"]
+        tx_originator.send_and_ping(msg_tx(tx2))
+
+        # Spy should only get an inv for the second transaction as the first
+        # one was received pre-verack with the spy
+        spy.wait_for_inv_match(CInv(MSG_WTX, tx2.calc_sha256(True)))
+
+if __name__ == '__main__':
+    TxPrivacyTest().main()

test/functional/test_runner.py

@@ -320,6 +320,7 @@
     'rpc_deriveaddresses.py',
     'rpc_deriveaddresses.py --usecli',
     'p2p_ping.py',
+    'p2p_tx_privacy.py',
     'rpc_scanblocks.py',
     'p2p_sendtxrcncl.py',
     'rpc_scantxoutset.py',