merge bitcoin#25720: Reduce bandwidth during initial headers sync whe…

…n a block is found

src/net_processing.cpp

@@ -370,6 +370,9 @@ struct Peer {
     /** Set of txids to reconsider once their parent transactions have been accepted **/
     std::set<uint256> m_orphan_work_set GUARDED_BY(g_cs_orphans);
 
+    /** Whether we've sent this peer a getheaders in response to an inv prior to initial-headers-sync completing */
+    bool m_inv_triggered_getheaders_before_sync{false};
+
     /** Protects m_getdata_requests **/
     Mutex m_getdata_requests_mutex;
     /** Work queue of items requested by this peer **/
@@ -664,6 +667,9 @@ class PeerManagerImpl final : public PeerManager
     /** Number of nodes with fSyncStarted. */
     int nSyncStarted GUARDED_BY(cs_main) = 0;
 
+    /** Hash of the last block we received via INV */
+    uint256 m_last_block_inv_triggering_headers_sync{};
+
     /**
      * Sources of received blocks, saved to be able punish them when processing
      * happens afterwards.
@@ -3939,8 +3945,9 @@ void PeerManagerImpl::ProcessMessage(
                 UpdateBlockAvailability(pfrom.GetId(), inv.hash);
                 if (!fAlreadyHave && !fImporting && !fReindex && !mapBlocksInFlight.count(inv.hash)) {
                     // Headers-first is the primary method of announcement on
-                    // the network. If a node fell back to sending blocks by inv,
-                    // it's probably for a re-org. The final block hash
+                    // the network. If a node fell back to sending blocks by
+                    // inv, it may be for a re-org, or because we haven't
+                    // completed initial headers sync. The final block hash
                     // provided should be the highest, so send a getheaders and
                     // then fetch the blocks we need to catch up.
                     best_block = &inv.hash;
@@ -3979,11 +3986,31 @@ void PeerManagerImpl::ProcessMessage(
             }
         }
         if (best_block != nullptr) {
-            std::string msg_type = UsesCompressedHeaders(*peer) ? NetMsgType::GETHEADERS2 : NetMsgType::GETHEADERS;
-            if (MaybeSendGetHeaders(pfrom, msg_type, m_chainman.ActiveChain().GetLocator(m_chainman.m_best_header), *peer)) {
-                LogPrint(BCLog::NET, "%s (%d) %s to peer=%d\n",
-                        msg_type, m_chainman.m_best_header->nHeight, best_block->ToString(),
-                        pfrom.GetId());
+            // If we haven't started initial headers-sync with this peer, then
+            // consider sending a getheaders now. On initial startup, there's a
+            // reliability vs bandwidth tradeoff, where we are only trying to do
+            // initial headers sync with one peer at a time, with a long
+            // timeout (at which point, if the sync hasn't completed, we will
+            // disconnect the peer and then choose another). In the meantime,
+            // as new blocks are found, we are willing to add one new peer per
+            // block to sync with as well, to sync quicker in the case where
+            // our initial peer is unresponsive (but less bandwidth than we'd
+            // use if we turned on sync with all peers).
+            CNodeState& state{*Assert(State(pfrom.GetId()))};
+            if (state.fSyncStarted || (!peer->m_inv_triggered_getheaders_before_sync && *best_block != m_last_block_inv_triggering_headers_sync)) {
+                std::string msg_type = UsesCompressedHeaders(*peer) ? NetMsgType::GETHEADERS2 : NetMsgType::GETHEADERS;
+                if (MaybeSendGetHeaders(pfrom, msg_type, m_chainman.ActiveChain().GetLocator(m_chainman.m_best_header), *peer)) {
+                    LogPrint(BCLog::NET, "%s (%d) %s to peer=%d\n",
+                            msg_type, m_chainman.m_best_header->nHeight, best_block->ToString(),
+                            pfrom.GetId());
+                }
+                if (!state.fSyncStarted) {
+                    peer->m_inv_triggered_getheaders_before_sync = true;
+                    // Update the last block hash that triggered a new headers
+                    // sync, so that we don't turn on headers sync with more
+                    // than 1 new peer every new block.
+                    m_last_block_inv_triggering_headers_sync = *best_block;
+                }
             }
         }
 

test/functional/p2p_initial_headers_sync.py

@@ -0,0 +1,119 @@
+#!/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 initial headers download
+
+Test that we only try to initially sync headers from one peer (until our chain
+is close to caught up), and that each block announcement results in only one
+additional peer receiving a getheaders message.
+"""
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.messages import (
+    CInv,
+    MSG_BLOCK,
+    msg_headers2,
+    msg_inv,
+)
+from test_framework.p2p import (
+    p2p_lock,
+    P2PInterface,
+)
+from test_framework.util import (
+    assert_equal,
+)
+import random
+
+class HeadersSyncTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def setup_chain(self):
+        # This test operates under the assumption that the adjusted time is well ahead of block
+        # time.
+        #
+        # By default when we setup a new chain, we also adjust the mocktime (this is not done in
+        # Bitcoin's test suite), which violates this test's assumption and causes it to fail. We
+        # remedy this by ensuring the test's assumptions are met (i.e. we don't adjust mocktime)
+        #
+        self.log.info("Initializing test directory " + self.options.tmpdir)
+        if self.setup_clean_chain:
+            self._initialize_chain_clean()
+        else:
+            self._initialize_chain()
+
+    def announce_random_block(self, peers):
+        new_block_announcement = msg_inv(inv=[CInv(MSG_BLOCK, random.randrange(1<<256))])
+        for p in peers:
+            p.send_and_ping(new_block_announcement)
+
+    def run_test(self):
+        self.log.info("Adding a peer to node0")
+        peer1 = self.nodes[0].add_p2p_connection(P2PInterface())
+
+        # Wait for peer1 to receive a getheaders
+        peer1.wait_for_getheaders()
+        # An empty reply will clear the outstanding getheaders request,
+        # allowing additional getheaders requests to be sent to this peer in
+        # the future.
+        peer1.send_message(msg_headers2())
+
+        self.log.info("Connecting two more peers to node0")
+        # Connect 2 more peers; they should not receive a getheaders yet
+        peer2 = self.nodes[0].add_p2p_connection(P2PInterface())
+        peer3 = self.nodes[0].add_p2p_connection(P2PInterface())
+
+        all_peers = [peer1, peer2, peer3]
+
+        self.log.info("Verify that peer2 and peer3 don't receive a getheaders after connecting")
+        for p in all_peers:
+            p.sync_with_ping()
+        with p2p_lock:
+            assert "getheaders2" not in peer2.last_message
+            assert "getheaders2" not in peer3.last_message
+
+        with p2p_lock:
+            peer1.last_message.pop("getheaders2", None)
+
+        self.log.info("Have all peers announce a new block")
+        self.announce_random_block(all_peers)
+
+        self.log.info("Check that peer1 receives a getheaders in response")
+        peer1.wait_for_getheaders()
+        peer1.send_message(msg_headers2()) # Send empty response, see above
+        with p2p_lock:
+            peer1.last_message.pop("getheaders2", None)
+
+        self.log.info("Check that exactly 1 of {peer2, peer3} received a getheaders in response")
+        count = 0
+        peer_receiving_getheaders = None
+        for p in [peer2, peer3]:
+            with p2p_lock:
+                if "getheaders2" in p.last_message:
+                    count += 1
+                    peer_receiving_getheaders = p
+                    p.last_message.pop("getheaders2", None)
+                    p.send_message(msg_headers2()) # Send empty response, see above
+
+        assert_equal(count, 1)
+
+        self.log.info("Announce another new block, from all peers")
+        self.announce_random_block(all_peers)
+
+        self.log.info("Check that peer1 receives a getheaders in response")
+        peer1.wait_for_getheaders()
+
+        self.log.info("Check that the remaining peer received a getheaders as well")
+        expected_peer = peer2
+        if peer2 == peer_receiving_getheaders:
+            expected_peer = peer3
+
+        expected_peer.wait_for_getheaders()
+
+        self.log.info("Success!")
+
+if __name__ == '__main__':
+    HeadersSyncTest().main()
+

test/functional/test_runner.py

@@ -255,6 +255,7 @@
     'rpc_generate.py',
     'wallet_balance.py --legacy-wallet',
     'wallet_balance.py --descriptors',
+    'p2p_initial_headers_sync.py',
     'feature_nulldummy.py --legacy-wallet',
     'feature_nulldummy.py --descriptors',
     'mempool_accept.py',