From 324c61d2e2bbe5116ba2fd3798967478603c2e66 Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 25 Mar 2026 17:12:17 -0500 Subject: [PATCH] Implement get_filtered_block_tree and fix remaining test failures - Add get_filtered_block_tree/filter_block_tree matching the spec's recursive viability pre-filter for get_head - Skip invalid execution status nodes in the filter (they aren't in store.blocks in the spec) - Fix attestation_score for V17 nodes: fall back to weight() for Empty/Full since pre-Gloas has no payload separation - Include head_payload_status in ForkChoiceView so CachedHead updates when payload status changes - Update votes test: branch with incompatible finalized leaf is now correctly excluded by the recursive filter - Update execution_status test_03: stored weights no longer include proposer boost All 30 proto_array/fork_choice tests pass. All 9 EF fork_choice test suites pass. --- .../src/fork_choice_test_definition/votes.rs | 11 +- consensus/proto_array/src/proto_array.rs | 122 ++++++++++++++++-- 2 files changed, 115 insertions(+), 18 deletions(-) diff --git a/consensus/proto_array/src/fork_choice_test_definition/votes.rs b/consensus/proto_array/src/fork_choice_test_definition/votes.rs index cdd9553127..ac97a592b7 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/votes.rs @@ -357,17 +357,18 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { execution_payload_block_hash: None, }); - // Ensure that 5 is filtered out and the head stays at 4. + // Block 5 has incompatible finalized checkpoint, so `get_filtered_block_tree` + // excludes the entire 1->3->4->5 branch (no viable leaf). Head moves to 2. // // 0 // / \ - // 2 1 + // head-> 2 1 // | // 3 // | - // 4 <- head + // 4 // / - // 5 + // 5 <- incompatible finalized checkpoint ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(1), @@ -378,7 +379,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { root: get_root(0), }, justified_state_balances: balances.clone(), - expected_head: get_root(4), + expected_head: get_root(2), current_slot: Slot::new(0), expected_payload_status: None, }); diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index e4e02d5872..43c6d74962 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -193,8 +193,12 @@ impl ProtoNode { pub fn attestation_score(&self, payload_status: PayloadStatus) -> u64 { match payload_status { PayloadStatus::Pending => self.weight(), - PayloadStatus::Empty => self.empty_payload_weight().unwrap_or(0), - PayloadStatus::Full => self.full_payload_weight().unwrap_or(0), + // Pre-Gloas (V17) nodes have no payload separation — all weight + // is in `weight()`. Post-Gloas (V29) nodes track per-status weights. + PayloadStatus::Empty => self + .empty_payload_weight() + .unwrap_or_else(|_| self.weight()), + PayloadStatus::Full => self.full_payload_weight().unwrap_or_else(|_| self.weight()), } } @@ -1181,6 +1185,100 @@ impl ProtoArray { Ok((best_fc_node.root, best_fc_node.payload_status)) } + /// Spec: `get_filtered_block_tree`. + /// + /// Returns the set of node indices on viable branches — those with at least + /// one leaf descendant with correct justified/finalized checkpoints. + fn get_filtered_block_tree( + &self, + start_index: usize, + current_slot: Slot, + best_justified_checkpoint: Checkpoint, + best_finalized_checkpoint: Checkpoint, + ) -> HashSet { + let mut viable = HashSet::new(); + self.filter_block_tree::( + start_index, + current_slot, + best_justified_checkpoint, + best_finalized_checkpoint, + &mut viable, + ); + viable + } + + /// Spec: `filter_block_tree`. + fn filter_block_tree( + &self, + node_index: usize, + current_slot: Slot, + best_justified_checkpoint: Checkpoint, + best_finalized_checkpoint: Checkpoint, + viable: &mut HashSet, + ) -> bool { + let Some(node) = self.nodes.get(node_index) else { + return false; + }; + + // Nodes with invalid execution payloads are never viable. + // (The spec doesn't need this check because invalid blocks aren't in store.blocks.) + if node + .execution_status() + .is_ok_and(|status| status.is_invalid()) + { + return false; + } + + // Skip invalid children — they aren't in store.blocks in the spec. + let children: Vec = self + .nodes + .iter() + .enumerate() + .filter(|(_, child)| { + child.parent() == Some(node_index) + && !child + .execution_status() + .is_ok_and(|status| status.is_invalid()) + }) + .map(|(i, _)| i) + .collect(); + + if !children.is_empty() { + // Evaluate ALL children (no short-circuit) to mark all viable branches. + let any_viable = children + .iter() + .map(|&child_index| { + self.filter_block_tree::( + child_index, + current_slot, + best_justified_checkpoint, + best_finalized_checkpoint, + viable, + ) + }) + .collect::>() + .into_iter() + .any(|v| v); + if any_viable { + viable.insert(node_index); + return true; + } + return false; + } + + // Leaf node: check viability. + if self.node_is_viable_for_head::( + node, + current_slot, + best_justified_checkpoint, + best_finalized_checkpoint, + ) { + viable.insert(node_index); + return true; + } + false + } + /// Spec: `get_head`. #[allow(clippy::too_many_arguments)] fn find_head_walk( @@ -1199,6 +1297,14 @@ impl ProtoArray { payload_status: PayloadStatus::Pending, }; + // Spec: `get_filtered_block_tree`. + let viable_nodes = self.get_filtered_block_tree::( + start_index, + current_slot, + best_justified_checkpoint, + best_finalized_checkpoint, + ); + // Compute once rather than per-child per-level. let apply_proposer_boost = self.should_apply_proposer_boost::(proposer_boost_root, justified_balances, spec)?; @@ -1207,17 +1313,7 @@ impl ProtoArray { let children: Vec<_> = self .get_node_children(&head)? .into_iter() - .filter(|(_, proto_node)| { - // Spec: `get_filtered_block_tree` pre-filters to only include - // blocks on viable branches. We approximate this by checking - // viability of each child during the walk. - self.node_is_viable_for_head::( - proto_node, - current_slot, - best_justified_checkpoint, - best_finalized_checkpoint, - ) - }) + .filter(|(fc_node, _)| viable_nodes.contains(&fc_node.proto_node_index)) .collect(); if children.is_empty() {