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

pull dergoegge wants to merge 3 commits into bitcoin:master from dergoegge:2022-11-foreachnode-txrelay-😭 changing 3 files +102 −2
  1. dergoegge commented at 7:42 pm on November 24, 2022: member

    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.

  2. DrahtBot commented at 7:42 pm on November 24, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK MarcoFalke, jnewbery
    Concept ACK naumenkogs, 1440000bytes
    Stale ACK mzumsande, luke-jr, glozow

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24125 (p2p: Replace RecursiveMutex m_tx_inventory_mutex with Mutex and rename it by w0xlt)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label P2P on Nov 24, 2022
  4. fanquake added the label Needs backport (24.x) on Nov 24, 2022
  5. dergoegge force-pushed on Nov 24, 2022
  6. dergoegge force-pushed on Nov 24, 2022
  7. dergoegge force-pushed on Nov 24, 2022
  8. in src/net_processing.cpp:2010 in 8ea1f7dac3 outdated
    2010-        Peer& peer = *it.second;
    2011-        auto tx_relay = peer.GetTxRelay();
    2012-        if (!tx_relay) continue;
    2013+    m_connman.ForEachNode([this, &wtxid, &txid](CNode* node) {
    2014+        PeerRef peer{this->GetPeerRef(node->GetId())};
    2015+        if (!peer) return false;
    


    maflcko commented at 8:29 pm on November 24, 2022:
    0        if (!peer) return;
    

    nit: The function return type is void

  9. dergoegge force-pushed on Nov 24, 2022
  10. in test/functional/p2p_tx_privacy.py:46 in c51d8b9dfc outdated
    41+        spy = self.nodes[0].add_p2p_connection(P2PTxSpy(), send_version=False, wait_for_verack=False)
    42+
    43+        # Spy sends version message to enable tx relay but doesn't send a verack
    44+        txrelay_version_msg = msg_version()
    45+        txrelay_version_msg.nVersion = P2P_VERSION
    46+        txrelay_version_msg.strSubVer = "😈😈😈"
    


    maflcko commented at 8:40 pm on November 24, 2022:

    Most of this code in the test can be removed:

     0diff --git a/test/functional/p2p_tx_privacy.py b/test/functional/p2p_tx_privacy.py
     1index 7944095abc..d7139f5ab7 100755
     2--- a/test/functional/p2p_tx_privacy.py
     3+++ b/test/functional/p2p_tx_privacy.py
     4@@ -17,54 +17,29 @@ from test_framework.p2p import (
     5     p2p_lock,
     6 )
     7 from test_framework.test_framework import BitcoinTestFramework
     8+from test_framework.wallet import MiniWallet
     9 
    10 class P2PTxSpy(P2PInterface):
    11-    def __init__(self, wtxidrelay=True):
    12-        super().__init__(wtxidrelay=wtxidrelay)
    13-
    14     def on_version(self, message):
    15         # Avoid sending verack in response to version.
    16         # When calling add_p2p_connection, wait_for_verack=False must be set (see
    17         # comment in add_p2p_connection).
    18-        if message.nVersion >= 70016 and self.wtxidrelay:
    19-            self.send_message(msg_wtxidrelay())
    20+        self.send_message(msg_wtxidrelay())
    21 
    22 class TxPrivacyTest(BitcoinTestFramework):
    23     def set_test_params(self):
    24-        self.setup_clean_chain = True
    25         self.num_nodes = 1
    26 
    27     def run_test(self):
    28-        self.generate(self.nodes[0], 200)
    29+        self.wallet = MiniWallet(self.nodes[0])
    30+        self.wallet.rescan_utxos()
    31 
    32         tx_originator = self.nodes[0].add_p2p_connection(P2PInterface())
    33-        spy = self.nodes[0].add_p2p_connection(P2PTxSpy(), send_version=False, wait_for_verack=False)
    34-
    35-        # Spy sends version message to enable tx relay but doesn't send a verack
    36-        txrelay_version_msg = msg_version()
    37-        txrelay_version_msg.nVersion = P2P_VERSION
    38-        txrelay_version_msg.strSubVer = "😈😈😈"
    39-        txrelay_version_msg.nServices = P2P_SERVICES
    40-        txrelay_version_msg.relay = 1
    41-        spy.send_message(txrelay_version_msg)
    42+        spy = self.nodes[0].add_p2p_connection(P2PTxSpy(), wait_for_verack=False)
    43         spy.wait_for_verack()
    44 
    45         # Create a tx to send
    46-        prevtx = self.nodes[0].getblock(self.nodes[0].getblockhash(5), 2)['tx'][0]
    47-        rawtx = self.nodes[0].createrawtransaction(
    48-            inputs=[{'txid': prevtx['txid'], 'vout': 0}],
    49-            outputs=[{self.nodes[0].get_deterministic_priv_key().address: 50 - 0.00125}],
    50-        )
    51-        sigtx = self.nodes[0].signrawtransactionwithkey(
    52-            hexstring=rawtx,
    53-            privkeys=[self.nodes[0].get_deterministic_priv_key().key],
    54-            prevtxs=[{
    55-                'txid': prevtx['txid'],
    56-                'vout': 0,
    57-                'scriptPubKey': prevtx['vout'][0]['scriptPubKey']['hex'],
    58-            }],
    59-        )['hex']
    60-        tx = tx_from_hex(sigtx)
    61+        tx = self.wallet.create_self_transfer()["tx"]
    62 
    63         # tx_originator sends a tx
    64         tx_originator.send_and_ping(msg_tx(tx))
    

    dergoegge commented at 8:52 pm on November 24, 2022:
    Amazing! Done.
  11. maflcko commented at 8:40 pm on November 24, 2022: member
    good catch
  12. dergoegge force-pushed on Nov 24, 2022
  13. jnewbery commented at 8:58 pm on November 24, 2022: contributor

    Concept ACK.

    Did you consider setting m_next_inv_send_time to a time point in the future inside SetTxRelay()? That seems like a simpler change, and avoids adding a new instance of ForEachNode() which we’d like to get rid off eventually.

  14. dergoegge commented at 12:55 pm on November 25, 2022: member

    @jnewbery Really good question! I should have put my reasoning for this in the PR description.

    Did you consider setting m_next_inv_send_time to a time point in the future inside SetTxRelay()? That seems like a simpler change, and avoids adding a new instance of ForEachNode() which we’d like to get rid off eventually.

    This actually was my initial approach, however I ended up going with ForEachNode for a couple reasons:

    • If we initialize m_next_inv_send_time to a time point in the future then we will still announce transactions that were received during the handshake which seems fine from a privacy perspective given the random delay. However,
      • I could see how that leads to a followup bug in which the spy is able to tell which of the announced transactions were received during the handshake (e.g. see point below). We can eliminate the possibility entirely by going with the ForEachNode approach.
      • A node can’t know if a peer enables wtxid relay until the handshake completes and it might add txids to m_tx_inventory_to_send but announce them as MSG_WTX after the handshake. (Didn’t test but i think) This again leaks which transactions were received between the spy sending version and wtxidrelay.
    • Storing the txids pre-handshake completion feels a bit weird from a resource consumption perspective. We rely on the 60 second p2p timeout (-peertimeout) to clear m_tx_inventory_to_send for peers that do not complete the handshake. Again not really an issue now but this doesn’t feel robust w.r.t. to future changes to me.
  15. glozow commented at 11:34 am on November 28, 2022: member

    ACK 4dc67b2a82cc71fc8b11247bd0f3028a6cae5494

    Agree with the approach to not add to m_tx_inventory_to_send until the handshake completes, and that not knowing wtxidrelay yet is also a good reason. Likewise not a fan of ForEachNode here but don’t see a way around it given only CNode knows whether it’s fully connected.

  16. in src/net_processing.cpp:2012 in 4dc67b2a82 outdated
    2004@@ -2005,18 +2005,27 @@ void PeerManagerImpl::SendPings()
    2005 
    2006 void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid)
    2007 {
    2008-    LOCK(m_peer_mutex);
    2009-    for(auto& it : m_peer_map) {
    2010-        Peer& peer = *it.second;
    2011-        auto tx_relay = peer.GetTxRelay();
    2012-        if (!tx_relay) continue;
    


    maflcko commented at 12:13 pm on November 28, 2022:

    Wouldn’t it be better to make this a one-line patch?

    m_next_inv_send_time will remain 0 for at least as long as the fSuccessfullyConnected is false. So an alternative fix would be to check m_next_inv_send_time==0 in this line.

    This should behave the same, because SendMessages is run after fSuccessfullyConnected = true without processing transactions from different peers in-between.


    dergoegge commented at 2:02 pm on November 28, 2022:
    Hm I kinda prefer the current patch. m_next_inv_send_time is not meant to be used as a “successfully connected check” so i prefer not to use it in that way here.

    jnewbery commented at 3:56 pm on November 28, 2022:

    I agree with Marco that it’d be better to use state in the Peer object rather than reaching into the net layer. It feels like that’d make it easier to unit test net procesing (using #25515).

    Using the special value m_next_inv_send_time == 0 to mean “we’re not queueing up an inv message for this peer so don’t save hashes to send” seems ok to me as long as it’s commented. Alternatively, it’d be easy enough to add a bool to TxRelay to gate the logic for saving hashes.


    dergoegge commented at 4:47 pm on November 28, 2022:

    Alright, amended the patch (m_next_inv_send_time is now guarded by m_tx_inventory_mutex).

    Alternatively, it’d be easy enough to add a bool to TxRelay to gate the logic for saving hashes.

    Yes but it is a little annoying because some of our tests manually set CNode::fSuccessfullyConnected and then the new flag on peer would be out of sync. We should be able to move fSuccessfullyConnected to Peer entirely though which would be nice since its an application level thing anyway (will do in a separate PR).

  17. dergoegge force-pushed on Nov 28, 2022
  18. luke-jr approved
  19. luke-jr commented at 4:31 am on November 29, 2022: member
    ACK 959cc24fcd19270d7d851dfde572c4d5848f2579
  20. in test/functional/p2p_tx_privacy.py:39 in 959cc24fcd outdated
    34+        # tx_originator sends a tx
    35+        tx = self.wallet.create_self_transfer()["tx"]
    36+        tx_originator.send_and_ping(msg_tx(tx))
    37+
    38+        with p2p_lock:
    39+            spy.last_message.pop("inv", None)
    


    mzumsande commented at 5:40 am on November 29, 2022:
    What is the reason for including this pop? If bitcoind ever sent an inv to a peer before receiving a verack, shouldn’t this test rather fail than silently remove it?

    dergoegge commented at 2:09 pm on November 29, 2022:
    No reason, removed it. Thanks!
  21. mzumsande commented at 5:49 am on November 29, 2022: contributor
    Concept ACK
  22. in src/net_processing.cpp:3438 in 959cc24fcd outdated
    3432@@ -3428,6 +3433,18 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
    3433             }
    3434         }
    3435 
    3436+        if (auto tx_relay = peer->GetTxRelay(); tx_relay) {
    


    maflcko commented at 8:02 am on November 29, 2022:
    0        if (auto tx_relay = peer->GetTxRelay()) {
    

    nit?


    dergoegge commented at 2:08 pm on November 29, 2022:
    Done
  23. maflcko approved
  24. maflcko commented at 8:02 am on November 29, 2022: member
    lgtm
  25. in src/net_processing.cpp:2017 in 07c67c4771 outdated
    2010@@ -2011,8 +2011,13 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid
    2011         auto tx_relay = peer.GetTxRelay();
    2012         if (!tx_relay) continue;
    2013 
    2014-        const uint256& hash{peer.m_wtxid_relay ? wtxid : txid};
    2015         LOCK(tx_relay->m_tx_inventory_mutex);
    2016+        // Only queue transactions for announcement once the version handshake
    2017+        // is completed. The time of arrival for these transactions is
    2018+        // otherwise at risk of leaking to a spy.
    


    naumenkogs commented at 8:15 am on November 29, 2022:
    I can’t fully grasp the exact risk here… How does it help the spy to learn something new, as opposed to listening/analyzing after the handshake?

    maflcko commented at 8:18 am on November 29, 2022:
    There is no delay, so they can poll all announcements in real time by just creating a new incoming connection in a loop

    naumenkogs commented at 8:20 am on November 29, 2022:

    Yeah, the comment from the following commit also helped :) Feel free to resolve this. This is a great finding.

    I can only suggest explaining it here, and not in the following comment, because IMO the justification belongs where the transactions are added.


    dergoegge commented at 2:08 pm on November 29, 2022:
    Improved the comment in this location a little bit.
  26. naumenkogs commented at 8:25 am on November 29, 2022: member

    Concept ACK.

    Looking forward to resolving the comments :)

  27. [net processing] Ensure transaction announcements are only queued for fully connected peers 845e3a34c4
  28. dergoegge force-pushed on Nov 29, 2022
  29. dergoegge force-pushed on Nov 29, 2022
  30. fanquake added this to the milestone 24.0.1 on Nov 29, 2022
  31. mzumsande commented at 4:16 pm on November 29, 2022: contributor

    Code Review ACK 1220dfaf7c64f2eebe7c8061634d1f3fd2100a61

    If you touch again, could run flake8 or similar over the test (it complains about an unused p2p_lock import and some blank lines)

  32. dergoegge force-pushed on Nov 29, 2022
  33. in test/functional/p2p_tx_privacy.py:4 in 152a42a334 outdated
    0@@ -0,0 +1,44 @@
    1+#!/usr/bin/env python3
    2+# Copyright (c) 2022 The Bitcoin Core developers
    3+# Distributed under the MIT software license, see the accompanying
    4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
    


    jnewbery commented at 6:20 pm on November 29, 2022:
    Perhaps add a module-level docstring to describe what the test is testing, and how it is testing it. (https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)
  34. in test/functional/p2p_tx_privacy.py:41 in 152a42a334 outdated
    36+
    37+        # Spy sends the verack
    38+        spy.send_and_ping(msg_verack())
    39+
    40+        # Spy should not get an inv for the tx
    41+        assert 'inv' not in spy.last_message
    


    jnewbery commented at 6:22 pm on November 29, 2022:
    Perhaps send a second tx from tx_originator to the node after the version handshake with spy is complete, and then asset that the node announces the second tx, but not the first.
  35. jnewbery commented at 6:22 pm on November 29, 2022: contributor
    Looks good. A couple of minor suggestions inline in the functional test.
  36. luke-jr approved
  37. luke-jr commented at 11:31 pm on November 29, 2022: member
    re-ACK 152a42a3344b473c838b94f2ec07b25d6739bd2d
  38. [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack
    This commit documents our assumption about
    TxRelay::m_tx_inventory_to_send being empty prior to version handshake
    completion.
    
    The added Assume acts as testing oracle for our fuzzing tests to
    potentially detect if the assumption is violated.
    ce63fca13e
  39. dergoegge force-pushed on Nov 30, 2022
  40. in src/net_processing.cpp:298 in 845e3a34c4 outdated
    294@@ -295,7 +295,7 @@ struct Peer {
    295         std::atomic<std::chrono::seconds> m_last_mempool_req{0s};
    296         /** The next time after which we will send an `inv` message containing
    297          *  transaction announcements to this peer. */
    298-        std::chrono::microseconds m_next_inv_send_time GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0};
    299+        std::chrono::microseconds m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){0};
    


    glozow commented at 2:19 pm on November 30, 2022:
    in 845e3a34c49abcc634b5a10ccdd6b10fb4fcf449, noting to myself that this change is because m_next_inv_send_time can now be accessed from the scheduler thread in ReattemptInitialBroadcast.
  41. glozow commented at 2:20 pm on November 30, 2022: member
    ACK fbcd991f8c9fe6ecdac181b09f04756cab99f47d
  42. in test/functional/p2p_tx_privacy.py:66 in fbcd991f8c outdated
    61+        tx2 = self.wallet.create_self_transfer()["tx"]
    62+        tx_originator.send_message(msg_tx(tx2))
    63+
    64+        # Spy should only get an inv for the second transaction as the first
    65+        # one was received pre-verack with the spy
    66+        spy.wait_for_inv([CInv(MSG_WTX, tx2.calc_sha256(True))])
    


    maflcko commented at 2:54 pm on November 30, 2022:
    The test doesn’t test anything. It passes on master as-is. I think you can just revert to the earlier commit where it was working?

    jnewbery commented at 4:34 pm on November 30, 2022:
    I think you could add an on_inv method to the spy connection that saves the announced hash, and then assert at the end that you haven’t received an annoucement for tx1.

    dergoegge commented at 4:43 pm on November 30, 2022:
    Did pretty much what John suggested. It’s now using on_inv to collect all announced invs and in the end it checks that there is only one announcement for tx2.
  43. [test] Add p2p_tx_privacy.py 8f2dac5409
  44. dergoegge force-pushed on Nov 30, 2022
  45. ghost commented at 10:00 pm on November 30, 2022: none
    Concept ACK
  46. maflcko approved
  47. maflcko commented at 6:32 pm on December 1, 2022: member

    ACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e 🔝

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e 🔝
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUghBAwAyGNtXO8fvuygZ5qpYaJJCN8updyEfpF8Q//EHzHYtA7gyW1HhZKTXASV
     87/R41rYKVtSE6XqJ8piivfuYNNTDZmpa1/LX030yZnSJsUGfr28bWPkXxrcUmdNE
     9CsiwIUbrPAzwfiPciowfipA/7d894Ado8X54ESkADJFZvYMuZ63aUuby/aPa/lCC
    104GotuNiYjXJpC8FfeR6VL7yJORVdgMav8jNPzvUYM5DNfGex39Uzh1kACtXrtXTn
    11zeT3zqrVf/rhr9izKqQKN5utGt3vcK9+8F8zh82NOr4heNG8ZufPWHP8F+D/uQPY
    12h/qNQhvT47NOODa4KKurLYWSEQXYaYRdOVLXq40lZOPMbU5ByAGwMjNWusemAb4/
    13Af+Xt7UlK4irsQQOj+D9EClqzllVL0gfqPvDmYYutgr/6co2rgg+MJSg8LHN9t+X
    14rTe2pwtee+wvLTYZGgKg7uJrcanmrUL87sSFvXutOy0TXbnJ67FR3B5fGCFkXnbJ
    15wYTAXZpf
    16=/QGZ
    17-----END PGP SIGNATURE-----
    
  48. maflcko requested review from luke-jr on Dec 1, 2022
  49. maflcko requested review from mzumsande on Dec 1, 2022
  50. in src/net_processing.cpp:3449 in 8f2dac5409
    3444+            // immediately be advertised without random delay, potentially
    3445+            // leaking the time of arrival to a spy.
    3446+            Assume(WITH_LOCK(
    3447+                tx_relay->m_tx_inventory_mutex,
    3448+                return tx_relay->m_tx_inventory_to_send.empty() &&
    3449+                       tx_relay->m_next_inv_send_time == 0s));
    


    jnewbery commented at 2:11 pm on December 2, 2022:
    (not in this PR) it might be nice to set explicitly set m_next_inv_send_time to a future time here. It’s slightly odd that we go through the transaction sending logic on the first pass through SendMessages() even though m_tx_inventory_to_send will be empty.
  51. jnewbery commented at 2:15 pm on December 2, 2022: contributor
    utACK 8f2dac54096c20afd8fd12c21a2ee5465fea085e
  52. fanquake merged this on Dec 2, 2022
  53. fanquake closed this on Dec 2, 2022

  54. fanquake referenced this in commit e15b306017 on Dec 2, 2022
  55. fanquake referenced this in commit c8426706de on Dec 2, 2022
  56. fanquake referenced this in commit e5d097b639 on Dec 2, 2022
  57. fanquake removed the label Needs backport (24.x) on Dec 2, 2022
  58. fanquake commented at 4:08 pm on December 2, 2022: member
    Added this for backport in #26616.
  59. sidhujag referenced this in commit ac5bdf6c45 on Dec 2, 2022
  60. ghost commented at 0:10 am on December 3, 2022: none
    Love that it was merged in less than 10 days, improves bitcoin and reviewed by more than 6 devs :)
  61. maflcko referenced this in commit 3afbc7d67d on Dec 6, 2022
  62. bitcoin locked this on Dec 3, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-23 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me