p2p: no longer allow received headers to only connect to a block in requested_blocks

Author: ImplOfAnImpl

p2p/src/sync/peer_v1.rs

@@ -611,43 +611,35 @@ where
             return Err(P2pError::ProtocolError(ProtocolError::DisconnectedHeaders));
         }
 
-        // The first header must be connected to a known block (it can be in
-        // the chainstate or requested_blocks).
+        // The first header must be connected to the chainstate.
         let first_header_prev_id = *headers
             .first()
             // This is OK because of the `headers.is_empty()` check above.
             .expect("Headers shouldn't be empty")
             .prev_block_id();
 
         // Note: we require a peer to send headers starting from a block that we already have
-        // in our chainstate or from one that we've already requested from the peer.
-        // I.e. peers shouldn't track what block headers they've sent us already and use
-        // the last header (best_sent_block_header) as a starting point for future HeaderList
-        // updates.
-        // This restriction is needed to prevent malicious peers from flooding the node with
-        // headers, potentially exhausting the node's memory.
-        // The downside of this is that the peer may have to send the same headers multiple times.
-        // So, to avoid extra traffic, an honest peer should't send header updates when the node
-        // is already downloading blocks. But still, the node shouldn't punish the peer for
-        // doing so, because it's possible for it to do so on accident, e.g. a "new tip" event
-        // may happen on the peer's side after it has sent us the last requested block but
-        // before we've asked it for more.
+        // in our chainstate. I.e. we don't allow:
+        // 1) Basing new headers on a previously sent header, because this would give a malicious
+        // peer an opportunity to flood the node with headers, potentially exhausting its memory.
+        // The downside of this restriction is that the peer may have to send the same headers
+        // multiple times. So, to avoid extra traffic, an honest peer should't send header updates
+        // when the node is already downloading blocks. (But still, the node shouldn't punish
+        // the peer for doing so, because it's possible for it to do so by accident, e.g.
+        // a "new tip" event may happen on the peer's side after it has sent us the last requested
+        // block but before we've asked it for more.)
+        // 2) Basing new headers on a block that we've requested from them but that has not yet
+        // been sent. This is a rather useless optimization (provided that peers don't send
+        // header updates when we're downloading blocks from them, as mentioned above) that
+        // would only complicate the logic.
 
         let first_header_is_connected_to_chainstate = self
             .chainstate_handle
             .call(move |c| Ok(c.get_gen_block_index(&first_header_prev_id)?))
             .await?
             .is_some();
 
-        let first_header_is_connected_to_requested_blocks = first_header_prev_id
-            .classify(&self.chain_config)
-            .chain_block_id()
-            .and_then(|id| self.incoming.requested_blocks.get(&id))
-            .is_some();
-
-        if !(first_header_is_connected_to_chainstate
-            || first_header_is_connected_to_requested_blocks)
-        {
+        if !first_header_is_connected_to_chainstate {
             // Note: legacy nodes will send singular unconnected headers during block announcement,
             // so we have to handle this behavior here.
             if headers.len() == 1 {

p2p/src/sync/peer_v2.rs

@@ -607,43 +607,35 @@ where
         let last_header = headers.last().expect("Headers shouldn't be empty");
         self.wait_for_clock_diff(last_header.timestamp()).await;
 
-        // The first header must be connected to a known block (it can be in
-        // the chainstate or requested_blocks).
+        // The first header must be connected to the chainstate.
         let first_header_prev_id = *headers
             .first()
             // This is OK because of the `headers.is_empty()` check above.
             .expect("Headers shouldn't be empty")
             .prev_block_id();
 
         // Note: we require a peer to send headers starting from a block that we already have
-        // in our chainstate or from one that we've already requested from the peer.
-        // I.e. peers shouldn't track what block headers they've sent us already and use
-        // the last header (best_sent_block_header) as a starting point for future HeaderList
-        // updates.
-        // This restriction is needed to prevent malicious peers from flooding the node with
-        // headers, potentially exhausting the node's memory.
-        // The downside of this is that the peer may have to send the same headers multiple times.
-        // So, to avoid extra traffic, an honest peer should't send header updates when the node
-        // is already downloading blocks. But still, the node shouldn't punish the peer for
-        // doing so, because it's possible for it to do so on accident, e.g. a "new tip" event
-        // may happen on the peer's side after it has sent us the last requested block but
-        // before we've asked it for more.
+        // in our chainstate. I.e. we don't allow:
+        // 1) Basing new headers on a previously sent header, because this would give a malicious
+        // peer an opportunity to flood the node with headers, potentially exhausting its memory.
+        // The downside of this restriction is that the peer may have to send the same headers
+        // multiple times. So, to avoid extra traffic, an honest peer should't send header updates
+        // when the node is already downloading blocks. (But still, the node shouldn't punish
+        // the peer for doing so, because it's possible for it to do so by accident, e.g.
+        // a "new tip" event may happen on the peer's side after it has sent us the last requested
+        // block but before we've asked it for more.)
+        // 2) Basing new headers on a block that we've requested from them but that has not yet
+        // been sent. This is a rather useless optimization (provided that peers don't send
+        // header updates when we're downloading blocks from them, as mentioned above) that
+        // would only complicate the logic.
 
         let first_header_is_connected_to_chainstate = self
             .chainstate_handle
             .call(move |c| Ok(c.get_gen_block_index(&first_header_prev_id)?))
             .await?
             .is_some();
 
-        let first_header_is_connected_to_requested_blocks = first_header_prev_id
-            .classify(&self.chain_config)
-            .chain_block_id()
-            .and_then(|id| self.incoming.requested_blocks.get(&id))
-            .is_some();
-
-        if !(first_header_is_connected_to_chainstate
-            || first_header_is_connected_to_requested_blocks)
-        {
+        if !first_header_is_connected_to_chainstate {
             return Err(P2pError::ProtocolError(ProtocolError::DisconnectedHeaders));
         }
 

p2p/src/sync/tests/block_announcement.rs

@@ -780,7 +780,9 @@ async fn send_headers_connected_to_previously_sent_headers(#[case] seed: Seed) {
 
 // This is similar to send_headers_connected_to_previously_sent_headers, but here the second
 // header list is connected to a block which the node is trying to download. The headers
-// should not be considered disconnected in this case.
+// should also be considered disconnected in this case.
+// Note that this behavior was allowed at some point in V1. But the implementation never used
+// this fact when sending headers, so we could disallow it without breaking the compatibility.
 #[tracing::instrument(skip(seed))]
 #[rstest::rstest]
 #[trace]
@@ -860,11 +862,16 @@ async fn send_headers_connected_to_block_which_is_being_downloaded(#[case] seed:
         );
         node.assert_no_peer_manager_event().await;
 
-        // Announce blocks 1, 2, 3 to the node - since block 1 is connected to block 0, which
-        // the node has already requested, the header list should not be considered disconnected.
+        // Announce blocks 1, 2, 3 to the node - block 1 is connected to block 0, which
+        // the node has already requested, but which it hasn't received yet.
         log::debug!("Sending headers 1, 2, 3");
         peer.send_headers(peers_block_headers[1..4].to_owned()).await;
-        node.assert_no_peer_manager_event().await;
+
+        node.assert_peer_score_adjustment(
+            peer.get_id(),
+            P2pError::ProtocolError(ProtocolError::DisconnectedHeaders).ban_score(),
+        )
+        .await;
 
         node.assert_no_event().await;
         node.join_subsystem_manager().await;