net_processing: drop Misbehavior for unconnecting headers

This misbehavior was originally intended to prevent bandwidth wastage due to
actually observed very broken (but likely non-malicious) nodes that respond
to GETHEADERS with a response unrelated to the request, triggering a request
cycle.

This has however largely been addressed by the previous commit, which causes
non-connecting HEADERS that are received while a GETHEADERS has not been
responded to, to be ignored, as long as they do not time out (2 minutes).
With that, the specific misbehavior is largely irrelevant (for inbound peers,
it is now harmless; for outbound peers, the eviction logic will eventually
kick them out if they're not keeping up with headers at all).

Author: sipa

src/net_processing.cpp

@@ -130,8 +130,6 @@ static constexpr double BLOCK_DOWNLOAD_TIMEOUT_BASE = 1;
 static constexpr double BLOCK_DOWNLOAD_TIMEOUT_PER_PEER = 0.5;
 /** Maximum number of headers to announce when relaying blocks with headers message.*/
 static const unsigned int MAX_BLOCKS_TO_ANNOUNCE = 8;
-/** Maximum number of unconnecting headers announcements before DoS score */
-static const int MAX_NUM_UNCONNECTING_HEADERS_MSGS = 10;
 /** Minimum blocks required to signal NODE_NETWORK_LIMITED */
 static const unsigned int NODE_NETWORK_LIMITED_MIN_BLOCKS = 288;
 /** Window, in blocks, for connecting to NODE_NETWORK_LIMITED peers */
@@ -382,9 +380,6 @@ struct Peer {
     /** Whether we've sent our peer a sendheaders message. **/
     std::atomic<bool> m_sent_sendheaders{false};
 
-    /** Length of current-streak of unconnecting headers announcements */
-    int m_num_unconnecting_headers_msgs GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0};
-
     /** When to potentially disconnect peer for stalling headers download */
     std::chrono::microseconds m_headers_sync_timeout GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0us};
 
@@ -2719,37 +2714,24 @@ arith_uint256 PeerManagerImpl::GetAntiDoSWorkThreshold()
  * announcement.
  *
  * We'll send a getheaders message in response to try to connect the chain.
- *
- * The peer can send up to MAX_NUM_UNCONNECTING_HEADERS_MSGS in a row that
- * don't connect before given DoS points.
- *
- * Once a headers message is received that is valid and does connect,
- * m_num_unconnecting_headers_msgs gets reset back to 0.
  */
 void PeerManagerImpl::HandleFewUnconnectingHeaders(CNode& pfrom, Peer& peer,
         const std::vector<CBlockHeader>& headers)
 {
-    peer.m_num_unconnecting_headers_msgs++;
     // Try to fill in the missing headers.
     const CBlockIndex* best_header{WITH_LOCK(cs_main, return m_chainman.m_best_header)};
     if (MaybeSendGetHeaders(pfrom, GetLocator(best_header), peer)) {
-        LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d, m_num_unconnecting_headers_msgs=%d)\n",
+        LogPrint(BCLog::NET, "received header %s: missing prev block %s, sending getheaders (%d) to end (peer=%d)\n",
             headers[0].GetHash().ToString(),
             headers[0].hashPrevBlock.ToString(),
             best_header->nHeight,
-            pfrom.GetId(), peer.m_num_unconnecting_headers_msgs);
+            pfrom.GetId());
     }
 
     // Set hashLastUnknownBlock for this peer, so that if we
     // eventually get the headers - even from a different peer -
     // we can use this peer to download.
     WITH_LOCK(cs_main, UpdateBlockAvailability(pfrom.GetId(), headers.back().GetHash()));
-
-    // The peer may just be broken, so periodically assign DoS points if this
-    // condition persists.
-    if (peer.m_num_unconnecting_headers_msgs % MAX_NUM_UNCONNECTING_HEADERS_MSGS == 0) {
-        Misbehaving(peer, 20, strprintf("%d non-connecting headers", peer.m_num_unconnecting_headers_msgs));
-    }
 }
 
 bool PeerManagerImpl::CheckHeadersAreContinuous(const std::vector<CBlockHeader>& headers) const
@@ -2991,11 +2973,6 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c
 void PeerManagerImpl::UpdatePeerStateForReceivedHeaders(CNode& pfrom, Peer& peer,
         const CBlockIndex& last_header, bool received_new_header, bool may_have_more_headers)
 {
-    if (peer.m_num_unconnecting_headers_msgs > 0) {
-        LogPrint(BCLog::NET, "peer=%d: resetting m_num_unconnecting_headers_msgs (%d -> 0)\n", pfrom.GetId(), peer.m_num_unconnecting_headers_msgs);
-    }
-    peer.m_num_unconnecting_headers_msgs = 0;
-
     LOCK(cs_main);
     CNodeState *nodestate = State(pfrom.GetId());
 
@@ -3122,8 +3099,6 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
             // special logic for handling headers that don't connect, as this
             // could be benign.
             HandleFewUnconnectingHeaders(pfrom, peer, headers);
-        } else {
-            Misbehaving(peer, 10, "invalid header received");
         }
         return;
     }

test/functional/p2p_sendheaders.py

@@ -71,19 +71,13 @@
    Expect: no response.
 
 Part 5: Test handling of headers that don't connect.
-a. Repeat 10 times:
+a. Repeat 100 times:
    1. Announce a header that doesn't connect.
       Expect: getheaders message
    2. Send headers chain.
       Expect: getdata for the missing blocks, tip update.
-b. Then send 9 more headers that don't connect.
+b. Then send 99 more headers that don't connect.
    Expect: getheaders message each time.
-c. Announce a header that does connect.
-   Expect: no response.
-d. Announce 49 headers that don't connect.
-   Expect: getheaders message each time.
-e. Announce one more that doesn't connect.
-   Expect: disconnect.
 """
 from test_framework.blocktools import create_block, create_coinbase
 from test_framework.messages import CInv
@@ -526,7 +520,8 @@ def test_nonnull_locators(self, test_node, inv_node):
         # First we test that receipt of an unconnecting header doesn't prevent
         # chain sync.
         expected_hash = tip
-        for i in range(10):
+        NUM_HEADERS = 100
+        for i in range(NUM_HEADERS):
             self.log.debug("Part 5.{}: starting...".format(i))
             test_node.last_message.pop("getdata", None)
             blocks = []
@@ -550,15 +545,14 @@ def test_nonnull_locators(self, test_node, inv_node):
         blocks = []
         # Now we test that if we repeatedly don't send connecting headers, we
         # don't go into an infinite loop trying to get them to connect.
-        MAX_NUM_UNCONNECTING_HEADERS_MSGS = 10
-        for _ in range(MAX_NUM_UNCONNECTING_HEADERS_MSGS + 1):
+        for _ in range(NUM_HEADERS + 1):
             blocks.append(create_block(tip, create_coinbase(height), block_time))
             blocks[-1].solve()
             tip = blocks[-1].sha256
             block_time += 1
             height += 1
 
-        for i in range(1, MAX_NUM_UNCONNECTING_HEADERS_MSGS):
+        for i in range(1, NUM_HEADERS):
             with p2p_lock:
                 test_node.last_message.pop("getheaders", None)
             # Send an empty header as a failed response to the received getheaders
@@ -569,29 +563,6 @@ def test_nonnull_locators(self, test_node, inv_node):
             test_node.send_header_for_blocks([blocks[i]])
             test_node.wait_for_getheaders(block_hash=expected_hash)
 
-        # Next header will connect, should re-set our count:
-        test_node.send_header_for_blocks([blocks[0]])
-        expected_hash = blocks[0].sha256
-
-        # Remove the first two entries (blocks[1] would connect):
-        blocks = blocks[2:]
-
-        # Now try to see how many unconnecting headers we can send
-        # before we get disconnected.  Should be 5*MAX_NUM_UNCONNECTING_HEADERS_MSGS
-        for i in range(5 * MAX_NUM_UNCONNECTING_HEADERS_MSGS - 1):
-            # Send a header that doesn't connect, check that we get a getheaders.
-            test_node.send_header_for_blocks([])
-            test_node.send_header_for_blocks([blocks[i % len(blocks)]])
-            test_node.wait_for_getheaders(block_hash=expected_hash)
-
-        # Eventually this stops working.
-        test_node.send_header_for_blocks([blocks[-1]])
-
-        # Should get disconnected
-        test_node.wait_for_disconnect()
-
-        self.log.info("Part 5: success!")
-
         # Finally, check that the inv node never received a getdata request,
         # throughout the test
         assert "getdata" not in inv_node.last_message