Merge #15759: p2p: Add 2 outbound block-relay-only connections

0ba08020c9791f7caf5986ad6490c16a2b66cd83 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1550bc3038dc541c236ac83e0a0e6d5 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027f15c1e4948ea4378954df24e3fee88 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306ea954d7eccdc11502e91a51dab8ec6 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bbe29bf130a2b0c0e85e5bffea107f75 Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd425f8c4e5b6bbb2beecb9c80034fefe1 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba82211ea5988ed7fe21e1b08bc3367e6d4 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b286c42a9b10132b7c2d76712f1a319 [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29fd813e1402b013f402869c25b656d1 Remove unused variable (Suhas Daftuar)

Pull request description:

  Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.

  Network topology is better kept private for (at least) two reasons:

  (a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.

  (b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).

  We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.

  After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)

ACKs for top commit:
  sipa:
    ACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83
  ajtowns:
    ACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
  TheBlueMatt:
    re-utACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
  jnewbery:
    utACK 0ba08020c9791f7caf5986ad6490c16a2b66cd83
  jamesob:
    ACK https://github.com/bitcoin/bitcoin/commit/0ba08020c9791f7caf5986ad6490c16a2b66cd83

Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf

1 files changed, 66 insertions, 41 deletions

diff --git a/src/net.h b/src/net.h
index 6c77d8135f..e9ea0162c9 100644
--- a/src/net.h
+++ b/src/net.h
@@ -61,10 +61,12 @@ static const unsigned int MAX_ADDR_TO_SEND = 1000;
 static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 4 * 1000 * 1000;
 /** Maximum length of the user agent string in `version` message */
 static const unsigned int MAX_SUBVERSION_LENGTH = 256;
-/** Maximum number of automatic outgoing nodes */
-static const int MAX_OUTBOUND_CONNECTIONS = 8;
+/** Maximum number of automatic outgoing nodes over which we'll relay everything (blocks, tx, addrs, etc) */
+static const int MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8;
 /** Maximum number of addnode outgoing nodes */
 static const int MAX_ADDNODE_CONNECTIONS = 8;
+/** Maximum number of block-relay-only outgoing connections */
+static const int MAX_BLOCKS_ONLY_CONNECTIONS = 2;
 /** -listen default */
 static const bool DEFAULT_LISTEN = true;
 /** -upnp default */
@@ -131,7 +133,8 @@ public:
     {
         ServiceFlags nLocalServices = NODE_NONE;
         int nMaxConnections = 0;
-        int nMaxOutbound = 0;
+        int m_max_outbound_full_relay = 0;
+        int m_max_outbound_block_relay = 0;
         int nMaxAddnode = 0;
         int nMaxFeeler = 0;
         int nBestHeight = 0;
@@ -155,10 +158,12 @@ public:
     void Init(const Options& connOptions) {
         nLocalServices = connOptions.nLocalServices;
         nMaxConnections = connOptions.nMaxConnections;
-        nMaxOutbound = std::min(connOptions.nMaxOutbound, connOptions.nMaxConnections);
+        m_max_outbound_full_relay = std::min(connOptions.m_max_outbound_full_relay, connOptions.nMaxConnections);
+        m_max_outbound_block_relay = connOptions.m_max_outbound_block_relay;
         m_use_addrman_outgoing = connOptions.m_use_addrman_outgoing;
         nMaxAddnode = connOptions.nMaxAddnode;
         nMaxFeeler = connOptions.nMaxFeeler;
+        m_max_outbound = m_max_outbound_full_relay + m_max_outbound_block_relay + nMaxFeeler;
         nBestHeight = connOptions.nBestHeight;
         clientInterface = connOptions.uiInterface;
         m_banman = connOptions.m_banman;
@@ -197,7 +202,7 @@ public:
     bool GetNetworkActive() const { return fNetworkActive; };
     bool GetUseAddrmanOutgoing() const { return m_use_addrman_outgoing; };
     void SetNetworkActive(bool active);
-    void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool fOneShot = false, bool fFeeler = false, bool manual_connection = false);
+    void OpenNetworkConnection(const CAddress& addrConnect, bool fCountFailure, CSemaphoreGrant *grantOutbound = nullptr, const char *strDest = nullptr, bool fOneShot = false, bool fFeeler = false, bool manual_connection = false, bool block_relay_only = false);
     bool CheckIncomingNonce(uint64_t nonce);
 
     bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
@@ -253,7 +258,7 @@ public:
     void AddNewAddresses(const std::vector<CAddress>& vAddr, const CAddress& addrFrom, int64_t nTimePenalty = 0);
     std::vector<CAddress> GetAddresses();
 
-    // This allows temporarily exceeding nMaxOutbound, with the goal of finding
+    // This allows temporarily exceeding m_max_outbound_full_relay, with the goal of finding
     // a peer that is better than all our current peers.
     void SetTryNewOutboundPeer(bool flag);
     bool GetTryNewOutboundPeer();
@@ -355,7 +360,7 @@ private:
     CNode* FindNode(const CService& addr);
 
     bool AttemptToEvictConnection();
-    CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool manual_connection);
+    CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, bool manual_connection, bool block_relay_only);
     void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const;
 
     void DeleteNode(CNode* pnode);
@@ -414,9 +419,17 @@ private:
     std::unique_ptr<CSemaphore> semOutbound;
     std::unique_ptr<CSemaphore> semAddnode;
     int nMaxConnections;
-    int nMaxOutbound;
+
+    // How many full-relay (tx, block, addr) outbound peers we want
+    int m_max_outbound_full_relay;
+
+    // How many block-relay only outbound peers we want
+    // We do not relay tx or addr messages with these peers
+    int m_max_outbound_block_relay;
+
     int nMaxAddnode;
     int nMaxFeeler;
+    int m_max_outbound;
     bool m_use_addrman_outgoing;
     std::atomic<int> nBestHeight;
     CClientUIInterface* clientInterface;
@@ -442,7 +455,7 @@ private:
     std::thread threadMessageHandler;
 
     /** flag for deciding to connect to an extra outbound peer,
-     *  in excess of nMaxOutbound
+     *  in excess of m_max_outbound_full_relay
      *  This takes the place of a feeler connection */
     std::atomic_bool m_try_another_outbound_peer;
 
@@ -681,15 +694,8 @@ public:
     // Setting fDisconnect to true will cause the node to be disconnected the
     // next time DisconnectNodes() runs
     std::atomic_bool fDisconnect{false};
-    // We use fRelayTxes for two purposes -
-    // a) it allows us to not relay tx invs before receiving the peer's version message
-    // b) the peer may tell us in its version message that we should not relay tx invs
-    //    unless it loads a bloom filter.
-    bool fRelayTxes GUARDED_BY(cs_filter){false};
     bool fSentAddr{false};
     CSemaphoreGrant grantOutbound;
-    mutable CCriticalSection cs_filter;
-    std::unique_ptr<CBloomFilter> pfilter PT_GUARDED_BY(cs_filter);
     std::atomic<int> nRefCount{0};
 
     const uint64_t nKeyedNetGroup;
@@ -708,28 +714,51 @@ public:
     std::vector<CAddress> vAddrToSend;
     CRollingBloomFilter addrKnown;
     bool fGetAddr{false};
-    std::set<uint256> setKnown;
     int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0};
     int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0};
 
-    // inventory based relay
-    CRollingBloomFilter filterInventoryKnown GUARDED_BY(cs_inventory);
-    // Set of transaction ids we still have to announce.
-    // They are sorted by the mempool before relay, so the order is not important.
-    std::set<uint256> setInventoryTxToSend;
+    const bool m_addr_relay_peer;
+    bool IsAddrRelayPeer() const { return m_addr_relay_peer; }
+
     // List of block ids we still have announce.
     // There is no final sorting before sending, as they are always sent immediately
     // and in the order requested.
     std::vector<uint256> vInventoryBlockToSend GUARDED_BY(cs_inventory);
     CCriticalSection cs_inventory;
-    int64_t nNextInvSend{0};
+
+    struct TxRelay {
+        TxRelay() { pfilter = MakeUnique<CBloomFilter>(); }
+        mutable CCriticalSection cs_filter;
+        // We use fRelayTxes for two purposes -
+        // a) it allows us to not relay tx invs before receiving the peer's version message
+        // b) the peer may tell us in its version message that we should not relay tx invs
+        //    unless it loads a bloom filter.
+        bool fRelayTxes GUARDED_BY(cs_filter){false};
+        std::unique_ptr<CBloomFilter> pfilter PT_GUARDED_BY(cs_filter) GUARDED_BY(cs_filter);
+
+        mutable CCriticalSection cs_tx_inventory;
+        CRollingBloomFilter filterInventoryKnown GUARDED_BY(cs_tx_inventory){50000, 0.000001};
+        // Set of transaction ids we still have to announce.
+        // They are sorted by the mempool before relay, so the order is not important.
+        std::set<uint256> setInventoryTxToSend;
+        // Used for BIP35 mempool sending
+        bool fSendMempool GUARDED_BY(cs_tx_inventory){false};
+        // Last time a "MEMPOOL" request was serviced.
+        std::atomic<int64_t> timeLastMempoolReq{0};
+        int64_t nNextInvSend{0};
+
+        CCriticalSection cs_feeFilter;
+        // Minimum fee rate with which to filter inv's to this node
+        CAmount minFeeFilter GUARDED_BY(cs_feeFilter){0};
+        CAmount lastSentFeeFilter{0};
+        int64_t nextSendTimeFeeFilter{0};
+    };
+
+    // m_tx_relay == nullptr if we're not relaying transactions with this peer
+    std::unique_ptr<TxRelay> m_tx_relay;
+
     // Used for headers announcements - unfiltered blocks to relay
     std::vector<uint256> vBlockHashesToAnnounce GUARDED_BY(cs_inventory);
-    // Used for BIP35 mempool sending
-    bool fSendMempool GUARDED_BY(cs_inventory){false};
-
-    // Last time a "MEMPOOL" request was serviced.
-    std::atomic<int64_t> timeLastMempoolReq{0};
 
     // Block and TXN accept times
     std::atomic<int64_t> nLastBlockTime{0};
@@ -746,15 +775,10 @@ public:
     std::atomic<int64_t> nMinPingUsecTime{std::numeric_limits<int64_t>::max()};
     // Whether a ping is requested.
     std::atomic<bool> fPingQueued{false};
-    // Minimum fee rate with which to filter inv's to this node
-    CAmount minFeeFilter GUARDED_BY(cs_feeFilter){0};
-    CCriticalSection cs_feeFilter;
-    CAmount lastSentFeeFilter{0};
-    int64_t nextSendTimeFeeFilter{0};
 
     std::set<uint256> orphan_work_set;
 
-    CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", bool fInboundIn = false);
+    CNode(NodeId id, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn, SOCKET hSocketIn, const CAddress &addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress &addrBindIn, const std::string &addrNameIn = "", bool fInboundIn = false, bool block_relay_only = false);
     ~CNode();
     CNode(const CNode&) = delete;
     CNode& operator=(const CNode&) = delete;
@@ -847,20 +871,21 @@ public:
 
     void AddInventoryKnown(const CInv& inv)
     {
-        {
-            LOCK(cs_inventory);
-            filterInventoryKnown.insert(inv.hash);
+        if (m_tx_relay != nullptr) {
+            LOCK(m_tx_relay->cs_tx_inventory);
+            m_tx_relay->filterInventoryKnown.insert(inv.hash);
         }
     }
 
     void PushInventory(const CInv& inv)
     {
-        LOCK(cs_inventory);
-        if (inv.type == MSG_TX) {
-            if (!filterInventoryKnown.contains(inv.hash)) {
-                setInventoryTxToSend.insert(inv.hash);
+        if (inv.type == MSG_TX && m_tx_relay != nullptr) {
+            LOCK(m_tx_relay->cs_tx_inventory);
+            if (!m_tx_relay->filterInventoryKnown.contains(inv.hash)) {
+                m_tx_relay->setInventoryTxToSend.insert(inv.hash);
             }
         } else if (inv.type == MSG_BLOCK) {
+            LOCK(cs_inventory);
             vInventoryBlockToSend.push_back(inv.hash);
         }
     }