Reject data columns that does not descend from finalize root instead of ignoring it (sigp#8179)

jimmygchen · web-flow · commit 538b70495ccc · 2025-10-09T07:32:43.000Z
This issue was identified during the fusaka audit competition. The [`verify_parent_block_and_finalized_descendant`](https://github.com/sigp/lighthouse/blob/62d9302e0f9dd9f94d0325411a3029b36ad90685/beacon_node/beacon_chain/src/data_column_verification.rs#L606-L627) in data column gossip verification currently load the parent first before checking if the column descends from the finalized root. However, the `fork_choice.get_block(&block_parent_root)` function also make the same check internally: https://github.com/sigp/lighthouse/blob/8a4f6cf0d5b6b261b2c3439ce7c05383a53d30c5/consensus/fork_choice/src/fork_choice.rs#L1242-L1249 Therefore, if the column does not descend from the finalized root, we return an `UnknownParent` error, before hitting the `is_finalized_checkpoint_or_descendant` check just below. Which means we `IGNORE` the gossip message instead `REJECT`, and the gossip peer is not _immediately_ penalised. This deviates from the spec. However, worth noting that lighthouse will currently attempt to request the parent from this peer, and if the peer is not able to serve the parent, it gets penalised with a `LowToleranceError`, and will get banned after ~5 occurences. https://github.com/sigp/lighthouse/blob/ffa7b2b2b9e3b4e70678e2c749b8bc45234febd7/beacon_node/network/src/sync/network_context.rs#L1530-L1532 This PR will penalise the bad peer immediately instead of performing block lookups before penalising it. Co-Authored-By: Jimmy Chen <jchen.tc@gmail.com>
diff --git a/beacon_node/beacon_chain/src/data_column_verification.rs b/beacon_node/beacon_chain/src/data_column_verification.rs
@@ -608,22 +608,21 @@ fn verify_parent_block_and_finalized_descendant<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
 ) -> Result<ProtoBlock, GossipDataColumnError> {
     let fork_choice = chain.canonical_head.fork_choice_read_lock();
+    let block_parent_root = data_column.block_parent_root();
+
+    // Do not process a column that does not descend from the finalized root.
+    if !fork_choice.is_finalized_checkpoint_or_descendant(block_parent_root) {
+        return Err(GossipDataColumnError::NotFinalizedDescendant { block_parent_root });
+    }
 
     // We have already verified that the column is past finalization, so we can
     // just check fork choice for the block's parent.
-    let block_parent_root = data_column.block_parent_root();
     let Some(parent_block) = fork_choice.get_block(&block_parent_root) else {
         return Err(GossipDataColumnError::ParentUnknown {
             parent_root: block_parent_root,
         });
     };
 
-    // Do not process a column that does not descend from the finalized root.
-    // We just loaded the parent_block, so we can be sure that it exists in fork choice.
-    if !fork_choice.is_finalized_checkpoint_or_descendant(block_parent_root) {
-        return Err(GossipDataColumnError::NotFinalizedDescendant { block_parent_root });
-    }
-
     Ok(parent_block)
 }
 
