feat(fork-choice): implement fork choice spec test #305
feat(fork-choice): implement fork choice spec test #305GrapeBaBa wants to merge 4 commits intogr/feature/forkchoice-zfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the fork choice module to lodestar-z, porting core functionality from the Lodestar TypeScript implementation. It establishes the necessary infrastructure for fork choice management, including spec test runners and performance benchmarks, while also laying the groundwork for the upcoming Gloas fork by updating consensus types and state transition logic to accommodate ePBS requirements. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the fork_choice module and adds support for the gloas fork, implementing ePBS (Enshrined Proposer-Builder Separation) by decoupling execution payloads from beacon blocks. Key additions include the computeDeltas weight-propagation function, SoA-based vote tracking, and a robust set of benchmarks and spec test runners. Review feedback primarily addresses violations of the repository's strict style guide, specifically regarding function length limits, the use of architecture-specific types, and assertion density. Recommendations were also made to optimize performance-critical paths by avoiding heap allocations and to improve call-site safety using options structs for similar parameter types.
| pub fn computeDeltas( | ||
| allocator: Allocator, | ||
| deltas_cache: *DeltasCache, | ||
| num_proto_nodes: u32, | ||
| vote_current_indices: []VoteIndex, | ||
| vote_next_indices: []const VoteIndex, | ||
| old_balances: []const u16, | ||
| new_balances: []const u16, | ||
| equivocating_indices: *const EquivocatingIndices, | ||
| ) !ComputeDeltasResult { | ||
| assert(vote_current_indices.len == vote_next_indices.len); | ||
| assert(num_proto_nodes < NULL_VOTE_INDEX); | ||
|
|
||
| // deltas.length = numProtoNodes; deltas.fill(0) | ||
| try deltas_cache.resize(allocator, @intCast(num_proto_nodes)); | ||
| const deltas = deltas_cache.items; | ||
| @memset(deltas, 0); | ||
|
|
||
| const num_validators = vote_next_indices.len; | ||
|
|
||
| // Sort equivocating indices for pointer advancement in the loop. | ||
| const sorted_eq = try sortEquivocatingKeys(allocator, equivocating_indices.*); | ||
| defer allocator.free(sorted_eq); | ||
|
|
||
| var result: ComputeDeltasResult = .{ .deltas = deltas, .equivocating_validators = @intCast(sorted_eq.len) }; | ||
| // Pre-fetch the first equivocating validator index for pointer advancement comparison. | ||
| // Use maxInt as sentinel when empty so the equivocating check is always false. | ||
| var equivocating_validator_index: ValidatorIndex = if (sorted_eq.len > 0) sorted_eq[0] else std.math.maxInt(ValidatorIndex); | ||
| var equivocating_index: usize = 0; | ||
|
|
||
| for (0..num_validators) |v_index| { | ||
| const current_index = vote_current_indices[v_index]; | ||
| const next_index = vote_next_indices[v_index]; | ||
|
|
||
| // Validator has never voted and has no pending vote. | ||
| if (current_index == NULL_VOTE_INDEX) { | ||
| if (next_index == NULL_VOTE_INDEX) { | ||
| result.old_inactive_validators += 1; | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| const bal = resolveBalances(v_index, old_balances, new_balances); | ||
|
|
||
| // Check if this validator is equivocating (sorted pointer advancement). | ||
| if (@as(ValidatorIndex, @intCast(v_index)) == equivocating_validator_index) { | ||
| // Remove weight from current vote. Only process once: after zeroing | ||
| // current_index, subsequent calls skip this validator. | ||
| subtractOldBalance(deltas, current_index, bal.old); | ||
| vote_current_indices[v_index] = NULL_VOTE_INDEX; | ||
| equivocating_index += 1; | ||
| // Advance to next equivocating validator, or set sentinel when exhausted. | ||
| equivocating_validator_index = if (equivocating_index < sorted_eq.len) | ||
| sorted_eq[equivocating_index] | ||
| else | ||
| std.math.maxInt(ValidatorIndex); | ||
| continue; | ||
| } | ||
|
|
||
| if (bal.old == 0) { | ||
| if (bal.new == 0) { | ||
| result.new_inactive_validators += 1; | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // Vote or balance changed: apply delta. | ||
| if (current_index != next_index or bal.old != bal.new) { | ||
| subtractOldBalance(deltas, current_index, bal.old); | ||
| addNewBalance(deltas, next_index, bal.new); | ||
| vote_current_indices[v_index] = next_index; | ||
| result.new_vote_validators += 1; | ||
| } else { | ||
| result.unchanged_vote_validators += 1; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The computeDeltas function body is 79 lines long, which exceeds the repository's hard limit of 70 lines per function. Please refactor this function by extracting logic (e.g., the loop body or the setup phase) into helper functions.
References
- Functions must not exceed a hard limit of 70 lines. (link)
There was a problem hiding this comment.
not too long, just keep it as is
| pub fn computeDeltas( | ||
| allocator: Allocator, | ||
| deltas_cache: *DeltasCache, | ||
| num_proto_nodes: u32, | ||
| vote_current_indices: []VoteIndex, | ||
| vote_next_indices: []const VoteIndex, | ||
| old_balances: []const u16, | ||
| new_balances: []const u16, | ||
| equivocating_indices: *const EquivocatingIndices, | ||
| ) !ComputeDeltasResult { |
There was a problem hiding this comment.
The computeDeltas function takes multiple slices of the same or similar types (vote_current_indices and vote_next_indices are both u32 slices; old_balances and new_balances are both u16 slices). This increases the risk of mixing up arguments at the call site. Per the style guide, functions with arguments that can be mixed up should use an options struct.
References
- Use an options struct when arguments can be mixed up. (link)
There was a problem hiding this comment.
not options similar arg here
| const sorted_eq = try sortEquivocatingKeys(allocator, equivocating_indices.*); | ||
| defer allocator.free(sorted_eq); |
There was a problem hiding this comment.
The computeDeltas function is a hot path in the fork choice module. Calling sortEquivocatingKeys here triggers a heap allocation and a sort operation on every call. This violates the design goal of minimizing dynamic memory allocation in performance-critical paths. Consider maintaining a sorted list of equivocating indices in the ForkChoiceStore and updating it only when an attester slashing occurs.
References
- Most memory should be statically allocated at startup where possible and optimal. (link)
| var equivocating_validator_index: ValidatorIndex = if (sorted_eq.len > 0) sorted_eq[0] else std.math.maxInt(ValidatorIndex); | ||
| var equivocating_index: usize = 0; | ||
|
|
||
| for (0..num_validators) |v_index| { |
There was a problem hiding this comment.
The loop uses v_index which is implicitly a usize. The style guide explicitly discourages the use of architecture-specific usize in favor of explicitly-sized types like u32 or u64.
var v_index: u32 = 0;
while (v_index < @as(u32, @intCast(num_validators))) : (v_index += 1) {
References
- Use explicitly-sized types like u32 for everything, avoid architecture-specific usize. (link)
| } | ||
|
|
||
| // Vote or balance changed: apply delta. | ||
| if (current_index != next_index or bal.old != bal.new) { |
There was a problem hiding this comment.
Compound conditions using or make it harder to verify that all logical branches are handled. Per the style guide, these should be split into nested if/else branches.
if (current_index != next_index) {
subtractOldBalance(deltas, current_index, bal.old);
addNewBalance(deltas, next_index, bal.new);
vote_current_indices[v_index] = next_index;
result.new_vote_validators += 1;
} else if (bal.old != bal.new) {
subtractOldBalance(deltas, current_index, bal.old);
addNewBalance(deltas, next_index, bal.new);
vote_current_indices[v_index] = next_index;
result.new_vote_validators += 1;
} else {
References
- Split compound conditions into simple conditions using nested if/else branches. (link)
|
|
||
| /// Resolves old and new effective balance for a validator, handling mismatched slice lengths | ||
| /// and the same-pointer optimisation (when old_balances == new_balances). | ||
| fn resolveBalances( |
There was a problem hiding this comment.
The resolveBalances function lacks assertions for its arguments. The style guide requires an average of at least two assertions per function to detect programmer errors.
References
- Assert all function arguments and return values. Average a minimum of two assertions per function. (link)
| fn init(allocator: Allocator, pool: *Node.Pool, dir: std.fs.Dir) !Self { | ||
| // 1. Load anchor state | ||
| var anchor_state = try tc_utils.loadPreStateFromFile(allocator, pool, dir, "anchor_state.ssz_snappy"); | ||
| errdefer anchor_state.deinit(); | ||
|
|
||
| // 2. Load anchor block and compute block_root | ||
| const anchor_block = try loadBeaconBlock(allocator, fork, dir, "anchor_block.ssz_snappy"); | ||
| defer deinitBeaconBlock(anchor_block, allocator); | ||
|
|
||
| var block_root: Root = undefined; | ||
| try anchor_block.hashTreeRoot(allocator, &block_root); | ||
|
|
||
| const anchor_slot = anchor_block.slot(); | ||
| const parent_root = anchor_block.parentRoot().*; | ||
| const state_root = anchor_block.stateRoot().*; | ||
|
|
||
| // 3. Get justified/finalized checkpoints from anchor state | ||
| // | ||
| // Note: The anchor checkpoint root must match the block_root stored in ProtoArray. | ||
| // For genesis, the state's currentJustifiedCheckpoint has root=0x00..00, but | ||
| // ProtoArray stores the actual block hash. Following the Lodestar TS pattern: | ||
| // use the computed block_root as the checkpoint root (matching computeAnchorCheckpoint). | ||
| const anchor_cached = anchor_state.cached_state; | ||
| var justified_cp_val: Checkpoint = undefined; | ||
| try anchor_cached.state.currentJustifiedCheckpoint(&justified_cp_val); | ||
| var finalized_cp_val: Checkpoint = undefined; | ||
| try anchor_cached.state.finalizedCheckpoint(&finalized_cp_val); | ||
|
|
||
| // Override checkpoint roots with the anchor block root | ||
| // (mirrors TS computeAnchorCheckpoint which uses BeaconBlockHeader hash) | ||
| justified_cp_val.root = block_root; | ||
| finalized_cp_val.root = block_root; | ||
|
|
||
| const justified_cp = CheckpointWithPayloadStatus{ | ||
| .epoch = justified_cp_val.epoch, | ||
| .root = justified_cp_val.root, | ||
| }; | ||
| const finalized_cp = CheckpointWithPayloadStatus{ | ||
| .epoch = finalized_cp_val.epoch, | ||
| .root = finalized_cp_val.root, | ||
| }; | ||
|
|
||
| // 4. Build anchor ProtoBlock | ||
| const anchor_proto_block = ProtoBlock{ | ||
| .slot = anchor_slot, | ||
| .block_root = block_root, | ||
| .parent_root = parent_root, | ||
| .state_root = state_root, | ||
| .target_root = block_root, // ProtoArray.initialize sets this | ||
| .justified_epoch = justified_cp_val.epoch, | ||
| .justified_root = justified_cp_val.root, | ||
| .finalized_epoch = finalized_cp_val.epoch, | ||
| .finalized_root = finalized_cp_val.root, | ||
| .unrealized_justified_epoch = justified_cp_val.epoch, | ||
| .unrealized_justified_root = justified_cp_val.root, | ||
| .unrealized_finalized_epoch = finalized_cp_val.epoch, | ||
| .unrealized_finalized_root = finalized_cp_val.root, | ||
| .extra_meta = .pre_merge, | ||
| .timeliness = false, | ||
| }; | ||
|
|
||
| // 5. Initialize ProtoArray | ||
| const proto_array = try allocator.create(ProtoArray); | ||
| errdefer allocator.destroy(proto_array); | ||
| proto_array.* = undefined; | ||
| try proto_array.initialize(allocator, anchor_proto_block, anchor_slot); | ||
| errdefer proto_array.deinit(allocator); | ||
|
|
||
| // 6. Compute justified balances | ||
| var justified_balances = try state_transition.getEffectiveBalanceIncrementsZeroInactive(allocator, anchor_cached); | ||
| defer justified_balances.deinit(); | ||
|
|
||
| // 7. Initialize ForkChoiceStore | ||
| const fc_store = try allocator.create(ForkChoiceStore); | ||
| errdefer allocator.destroy(fc_store); | ||
| try fc_store.init( | ||
| allocator, | ||
| anchor_slot, | ||
| justified_cp, | ||
| finalized_cp, | ||
| justified_balances.items, | ||
| JustifiedBalancesGetter{ | ||
| .getFn = specTestBalancesGetter, | ||
| }, | ||
| .{}, | ||
| ); | ||
| errdefer fc_store.deinit(allocator); | ||
|
|
||
| // 8. Initialize ForkChoice | ||
| const fc = try allocator.create(ForkChoice); | ||
| errdefer allocator.destroy(fc); | ||
| try fc.init( | ||
| allocator, | ||
| anchor_state.config, | ||
| fc_store, | ||
| proto_array, | ||
| @intCast(justified_balances.items.len), | ||
| .{ | ||
| .proposer_boost = true, | ||
| .proposer_boost_reorg = true, | ||
| .compute_unrealized = true, | ||
| }, | ||
| ); | ||
| errdefer fc.deinit(allocator); | ||
|
|
||
| // 9. Parse steps.yaml | ||
| const steps = try parseSteps(allocator, dir); | ||
|
|
||
| return Self{ | ||
| .allocator = allocator, | ||
| .pool = pool, | ||
| .anchor_state = anchor_state, | ||
| .fc = fc, | ||
| .fc_store = fc_store, | ||
| .proto_array = proto_array, | ||
| .state_cache = std.AutoHashMap(Root, *CachedBeaconState).init(allocator), | ||
| .pending_payload_statuses = std.AutoHashMap(Root, PayloadStatusStep).init(allocator), | ||
| .anchor_block_root = block_root, | ||
| .tick_time = 0, | ||
| .steps = steps, | ||
| .test_dir = dir, | ||
| }; |
There was a problem hiding this comment.
The init function is 122 lines long, exceeding the 70-line limit. Please refactor by splitting the initialization steps into smaller helper functions.
References
- Hard limit of 70 lines per function. (link)
| fn handleBlock(self: *Self, block_step: BlockStep) !void { | ||
| const file_name = try std.fmt.allocPrint(self.allocator, "{s}.ssz_snappy", .{block_step.name}); | ||
| defer self.allocator.free(file_name); | ||
|
|
||
| const signed_block = try loadSignedBeaconBlock(self.allocator, fork, self.test_dir, file_name); | ||
| defer deinitSignedBeaconBlock(signed_block, self.allocator); | ||
|
|
||
| const beacon_block = signed_block.beaconBlock(); | ||
|
|
||
| // Look up the parent's post-state from cache; fall back to anchor state | ||
| const parent_root = beacon_block.parentRoot().*; | ||
| const input_state = if (self.state_cache.get(parent_root)) |cs| | ||
| cs | ||
| else if (std.mem.eql(u8, &parent_root, &self.anchor_block_root)) | ||
| self.anchor_state.cached_state | ||
| else | ||
| self.anchor_state.cached_state; | ||
|
|
||
| // Run state_transition to get post-state | ||
| const post_state_result = state_transition.stateTransition( | ||
| self.allocator, | ||
| input_state, | ||
| signed_block, | ||
| .{ | ||
| .verify_signatures = false, | ||
| .verify_proposer = false, | ||
| .verify_state_root = false, | ||
| }, | ||
| ); | ||
|
|
||
| if (block_step.valid) { | ||
| // Block expected to be valid | ||
| const post_state = try post_state_result; | ||
| errdefer { | ||
| post_state.deinit(); | ||
| self.allocator.destroy(post_state); | ||
| } | ||
|
|
||
| // Compute block_root | ||
| var block_root: Root = undefined; | ||
| try beacon_block.hashTreeRoot(self.allocator, &block_root); | ||
|
|
||
| // Determine block_delay: if tick happened in the same slot, compute seconds into slot | ||
| const seconds_per_slot = self.anchor_state.config.chain.SECONDS_PER_SLOT; | ||
| const block_slot = beacon_block.slot(); | ||
| const slot_start_time = block_slot * seconds_per_slot; | ||
| const block_delay: u32 = if (self.tick_time >= slot_start_time) | ||
| @intCast(self.tick_time - slot_start_time) | ||
| else | ||
| 0; | ||
|
|
||
| const current_slot: Slot = @intCast(self.tick_time / seconds_per_slot); | ||
|
|
||
| // Determine execution status — check pending payload statuses first. | ||
| // In TS, the execution engine mock returns a predefined status if one was | ||
| // stored by a prior payload_status step; otherwise defaults to VALID. | ||
| const execution_status = self.getBlockExecutionStatus(&beacon_block); | ||
| const da_status = getDataAvailabilityStatus(beacon_block); | ||
|
|
||
| // Validate blob/proof counts for deneb/electra blocks (matching TS behavior). | ||
| // TS verifies blobs.length === commitments.length && proofs.length === commitments.length. | ||
| // Fulu uses columns (not blobs/proofs), Gloas has no DA in beacon block. | ||
| if (comptime fork.gte(.deneb) and !fork.gte(.fulu)) { | ||
| try self.validateBlobsProofs(&beacon_block, block_step); | ||
| } | ||
|
|
||
| // Call fork choice onBlock | ||
| _ = try self.fc.onBlock( | ||
| self.allocator, | ||
| &beacon_block, | ||
| post_state, | ||
| block_delay, | ||
| current_slot, | ||
| execution_status, | ||
| da_status, | ||
| ); | ||
|
|
||
| // Import attestations from block body into fork choice. | ||
| // The pyspec processes block body attestations via on_attestation(is_from_block=True), | ||
| // and Lodestar TS does this via importAttestations: Force. Without this, | ||
| // fork choice has no validator votes and proposer boost alone determines the head. | ||
| try self.importBlockAttestations(&beacon_block, post_state); | ||
|
|
||
| // Store post_state in cache keyed by block_root. | ||
| // Prune old entries to avoid pool exhaustion — keep only the | ||
| // last few states (most tests are linear chains; forked tests | ||
| // have short branches). | ||
| try self.state_cache.put(block_root, post_state); | ||
| self.pruneStateCache(block_root); | ||
| } else { | ||
| // Block expected to be invalid — either state_transition, blob validation, | ||
| // or onBlock should error. | ||
|
|
||
| // Check blob/proof validation first (may catch the expected failure). | ||
| if (comptime fork.gte(.deneb) and !fork.gte(.fulu)) { | ||
| self.validateBlobsProofs(&beacon_block, block_step) catch { | ||
| if (post_state_result) |ps| { | ||
| ps.deinit(); | ||
| self.allocator.destroy(ps); | ||
| } else |_| {} | ||
| return; // Expected failure in blob validation | ||
| }; | ||
| } | ||
|
|
||
| if (post_state_result) |post_state| { | ||
| defer { | ||
| post_state.deinit(); | ||
| self.allocator.destroy(post_state); | ||
| } | ||
| // state_transition succeeded, try onBlock (it may still fail) | ||
| var block_root: Root = undefined; | ||
| try beacon_block.hashTreeRoot(self.allocator, &block_root); | ||
|
|
||
| const seconds_per_slot = self.anchor_state.config.chain.SECONDS_PER_SLOT; | ||
| const current_slot: Slot = @intCast(self.tick_time / seconds_per_slot); | ||
| const execution_status = self.getBlockExecutionStatus(&beacon_block); | ||
|
|
||
| _ = self.fc.onBlock( | ||
| self.allocator, | ||
| &beacon_block, | ||
| post_state, | ||
| 0, | ||
| current_slot, | ||
| execution_status, | ||
| .not_required, | ||
| ) catch { | ||
| return; // Expected failure in onBlock | ||
| }; | ||
| // If both succeed but block should be invalid, that's OK for some tests | ||
| // (e.g., deneb blob validation happens outside state_transition) | ||
| } else |_| { | ||
| return; // Expected failure in state_transition | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The handleBlock function is 135 lines long, exceeding the 70-line limit. Consider extracting the block validation and processing logic into separate methods.
References
- Hard limit of 70 lines per function. (link)
GrapeBaBa
changed the title
Add fork_choice to the spec test infrastructure: - runner/fork_choice.zig: step-based test runner that parses steps.yaml and executes tick/block/attestation/attester_slashing/payload_status/checks - writer/fork_choice.zig: generates test cases from spec test vectors - Wire fork_choice into build.zig, runner_kind.zig, write_spec_tests.zig - Add loadPreStateFromFile, loadBeaconBlock helpers to test_case.zig 260/331 fork_choice spec tests pass. Remaining failures: - 26 tests: YAML parser doesn't handle blobs/columns fields yet - 45 tests: logic differences in reorg/withholding/proposer_head handlers
- Fix attestation step: use force_import=false for step attestations (only block-body attestations use force_import=true, matching TS importAttestations: Force) - Add importBlockAttestations to process block body attestations through fork choice after onBlock (mirrors pyspec is_from_block=True) - Add pending_payload_statuses map to store execution engine mock statuses for consumption during block processing (mirrors TS ExecutionEngineMockBackend.addPredefinedPayloadStatus) - Add getBlockExecutionStatus to look up stored payload status by execution payload block hash before defaulting to .valid - Add validateBlobsProofs for deneb/electra blocks: validate that blobs and proofs counts match blobKzgCommitments (count-only, no KZG proof verification) - Parse blobs/proofs fields from YAML block steps - Handle multi-line should_override_forkchoice_update YAML values - Add skip patterns in writer matching Lodestar TS skip list - Bump spec test version to v1.7.0-alpha.2 - Add TODO for v1.7.0-alpha.1 proposer index check in onBlock
… fork choice tests - P1: Assert ExpectedInvalidBlock when valid:false blocks pass all checks (deneb/electra) - P2: Validate blobs_count matches blobKzgCommitments length (matching TS behavior) - Add getSszSnappyDecompressedSize utility to resolve blob count from SSZ files - Fix YAML parser: handle multi-line blobs: field and stop proofs scanner at YAML keys
- migrate test_case.zig + runner/writer to std.Io.Dir/std.Io.Writer APIs - pass std.Io to ForkChoice.onBlock and computeUnrealizedCheckpoints - carry allocator on OnBlockBalancesCtx (balances.allocator removed in 0.16) - bump spec_test_version default to v1.7.0-alpha.5 - include .fork_choice in spec_tests imports Build: clean. fork_choice spec tests: 311/311 pass on minimal preset.
GrapeBaBa
force-pushed
the
gr/feature/forkchoice-spec-tests
branch
from
2ab3274 to
eb84320
Compare
1 participant
Summary
Add fork choice spec test runner and writer, aligned with Lodestar TS
fork_choice.test.tsonunstablebranch.Known Gaps
on_block_peerdas__*tests)sync/optimistictests not generated (requiresonlyPredefinedResponsesmode)