Merge bitcoin/bitcoin#24171: p2p: Sync chain more readily from inbound peers during IBD

48262a00f58489d705314ee3c31136133040bb0e Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bbf8f725e3969d76f7cb081cdf1e4195 Sync chain more readily from inbound peers during IBD (Suhas Daftuar)

Pull request description:

  When in IBD, if the honest chain is only known by inbound peers, then we must
  eventually sync from them in order to learn it. This change allows us to
  perform initial headers sync and fetch blocks from inbound peers, if we have no
  blocks in flight.

  The restriction on having no blocks in flight means that we will naturally
  throttle our block downloads to any such inbound peers that we may be
  downloading from, until we leave IBD. This is a tradeoff between preferring
  outbound peers for most of our block download, versus making sure we always
  eventually will get blocks we need that are only known by inbound peers even
  during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
  cascading failure on the network, if a large fraction of the network managed to
  get stuck in IBD).

  Note that the test in the second commit fails on master, without the first commit.

ACKs for top commit:
  ajtowns:
    ACK 48262a00f58489d705314ee3c31136133040bb0e
  sipa:
    ACK 48262a00f58489d705314ee3c31136133040bb0e

Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d

3 files changed, 62 insertions, 3 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 6d17ab4be8..70b81b3eb2 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4717,10 +4717,31 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         if (m_chainman.m_best_header == nullptr) {
             m_chainman.m_best_header = m_chainman.ActiveChain().Tip();
         }
-        bool fFetch = state.fPreferredDownload || (m_num_preferred_download_peers == 0 && !pto->fClient && !pto->IsAddrFetchConn()); // Download if this is a nice peer, or we have no nice peers and this one might do.
+
+        // Determine whether we might try initial headers sync or parallel
+        // block download from this peer -- this mostly affects behavior while
+        // in IBD (once out of IBD, we sync from all peers).
+        bool sync_blocks_and_headers_from_peer = false;
+        if (state.fPreferredDownload) {
+            sync_blocks_and_headers_from_peer = true;
+        } else if (!pto->fClient && !pto->IsAddrFetchConn()) {
+            // Typically this is an inbound peer. If we don't have any outbound
+            // peers, or if we aren't downloading any blocks from such peers,
+            // then allow block downloads from this peer, too.
+            // We prefer downloading blocks from outbound peers to avoid
+            // putting undue load on (say) some home user who is just making
+            // outbound connections to the network, but if our only source of
+            // the latest blocks is from an inbound peer, we have to be sure to
+            // eventually download it (and not just wait indefinitely for an
+            // outbound peer to have it).
+            if (m_num_preferred_download_peers == 0 || mapBlocksInFlight.empty()) {
+                sync_blocks_and_headers_from_peer = true;
+            }
+        }
+
         if (!state.fSyncStarted && !pto->fClient && !fImporting && !fReindex) {
             // Only actively request headers from a single peer, unless we're close to today.
-            if ((nSyncStarted == 0 && fFetch) || m_chainman.m_best_header->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) {
+            if ((nSyncStarted == 0 && sync_blocks_and_headers_from_peer) || m_chainman.m_best_header->GetBlockTime() > GetAdjustedTime() - 24 * 60 * 60) {
                 state.fSyncStarted = true;
                 state.m_headers_sync_timeout = current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE +
                     (
@@ -5093,7 +5114,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
         // Message: getdata (blocks)
         //
         std::vector<CInv> vGetData;
-        if (!pto->fClient && ((fFetch && !pto->m_limited_node) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
+        if (!pto->fClient && ((sync_blocks_and_headers_from_peer && !pto->m_limited_node) || !m_chainman.ActiveChainstate().IsInitialBlockDownload()) && state.nBlocksInFlight < MAX_BLOCKS_IN_TRANSIT_PER_PEER) {
             std::vector<const CBlockIndex*> vToDownload;
             NodeId staller = -1;
             FindNextBlocksToDownload(pto->GetId(), MAX_BLOCKS_IN_TRANSIT_PER_PEER - state.nBlocksInFlight, vToDownload, staller);
diff --git a/test/functional/p2p_block_sync.py b/test/functional/p2p_block_sync.py
new file mode 100755
index 0000000000..d821edc1b1
--- /dev/null
+++ b/test/functional/p2p_block_sync.py
@@ -0,0 +1,37 @@
+#!/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 block download
+
+Ensure that even in IBD, we'll eventually sync chain from inbound peers
+(whether we have only inbound peers or both inbound and outbound peers).
+"""
+
+from test_framework.test_framework import BitcoinTestFramework
+
+class BlockSyncTest(BitcoinTestFramework):
+
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 3
+
+    def setup_network(self):
+        self.setup_nodes()
+        # Construct a network:
+        # node0 -> node1 -> node2
+        # So node1 has both an inbound and outbound peer.
+        # In our test, we will mine a block on node0, and ensure that it makes
+        # to to both node1 and node2.
+        self.connect_nodes(0, 1)
+        self.connect_nodes(1, 2)
+
+    def run_test(self):
+        self.log.info("Setup network: node0->node1->node2")
+        self.log.info("Mining one block on node0 and verify all nodes sync")
+        self.generate(self.nodes[0], 1)
+        self.log.info("Success!")
+
+
+if __name__ == '__main__':
+    BlockSyncTest().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 40e08c3f1f..6a44f9d21d 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -157,6 +157,7 @@ BASE_SCRIPTS = [
     'wallet_avoidreuse.py --descriptors',
     'mempool_reorg.py',
     'mempool_persist.py',
+    'p2p_block_sync.py',
     'wallet_multiwallet.py --legacy-wallet',
     'wallet_multiwallet.py --descriptors',
     'wallet_multiwallet.py --usecli',