Merge bitcoin#24178: p2p: Respond to getheaders if we have sufficient chainwork

a35f963 Add test for getheaders behavior (Suhas Daftuar)
ef6dbe6 Respond to getheaders if we have sufficient chainwork (Suhas Daftuar)

Pull request description:

  Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes are getting restarted during that time.

  To make things simpler to reason about, just use `nMinimumChainWork` as our anti-DoS threshold for responding to a getheaders request; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed).

ACKs for top commit:
  klementtan:
    crACK a35f963
  naumenkogs:
    ACK a35f963
  MarcoFalke:
    review ACK a35f963 🗯

Tree-SHA512: 131e3872e7fe80382ea9c1ec202d6c2dc59c006355c69000aa3f4ce6bccd02a6c689c8cb8f3542b5d9bc48bfa61edcbd1a78535c0b79018971d02bed2655d284

Author: MacroFake

src/net_processing.cpp

@@ -3300,9 +3300,23 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             return;
         }
 
+        if (fImporting || fReindex) {
+            LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d while importing/reindexing\n", pfrom.GetId());
+            return;
+        }
+
         LOCK(cs_main);
-        if (m_chainman.ActiveChainstate().IsInitialBlockDownload() && !pfrom.HasPermission(NetPermissionFlags::Download)) {
-            LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom.GetId());
+
+        // Note that if we were to be on a chain that forks from the checkpointed
+        // chain, then serving those headers to a peer that has seen the
+        // checkpointed chain would cause that peer to disconnect us. Requiring
+        // that our chainwork exceed nMinimumChainWork is a protection against
+        // being fed a bogus chain when we started up for the first time and
+        // getting partitioned off the honest network for serving that chain to
+        // others.
+        if (m_chainman.ActiveTip() == nullptr ||
+                (m_chainman.ActiveTip()->nChainWork < nMinimumChainWork && !pfrom.HasPermission(NetPermissionFlags::Download))) {
+            LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work\n", pfrom.GetId());
             return;
         }
 

test/functional/feature_minchainwork.py

@@ -17,6 +17,7 @@
 
 import time
 
+from test_framework.p2p import P2PInterface, msg_getheaders
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
@@ -41,6 +42,9 @@ def setup_network(self):
         for i in range(self.num_nodes-1):
             self.connect_nodes(i+1, i)
 
+        # Set clock of node2 2 days ahead, to keep it in IBD during this test.
+        self.nodes[2].setmocktime(int(time.time()) + 48*60*60)
+
     def run_test(self):
         # Start building a chain on node0.  node2 shouldn't be able to sync until node1's
         # minchainwork is exceeded
@@ -71,6 +75,15 @@ def run_test(self):
         assert self.nodes[1].getbestblockhash() != self.nodes[0].getbestblockhash()
         assert_equal(self.nodes[2].getblockcount(), starting_blockcount)
 
+        self.log.info("Check that getheaders requests to node2 are ignored")
+        peer = self.nodes[2].add_p2p_connection(P2PInterface())
+        msg = msg_getheaders()
+        msg.locator.vHave = [int(self.nodes[2].getbestblockhash(), 16)]
+        msg.hashstop = 0
+        peer.send_and_ping(msg)
+        time.sleep(5)
+        assert "headers" not in peer.last_message
+
         self.log.info("Generating one more block")
         self.generate(self.nodes[0], 1)
 
@@ -85,5 +98,13 @@ def run_test(self):
         self.sync_all()
         self.log.info(f"Blockcounts: {[n.getblockcount() for n in self.nodes]}")
 
+        self.log.info("Test that getheaders requests to node2 are not ignored")
+        peer.send_and_ping(msg)
+        assert "headers" in peer.last_message
+
+        # Verify that node2 is in fact still in IBD (otherwise this test may
+        # not be exercising the logic we want!)
+        assert_equal(self.nodes[2].getblockchaininfo()['initialblockdownload'], True)
+
 if __name__ == '__main__':
     MinimumChainWorkTest().main()