feat(forkchoice): implement the forkchoice module#246
feat(forkchoice): implement the forkchoice module#246
Conversation
Register fork_choice module in zbuild.zon and implement foundational types ported from Lodestar's protoArray/interface.ts and errors.ts: - ExecutionStatus, DataAvailabilityStatus enums - ProtoBlock, ProtoNode, BlockExtraMeta structs - LVHExecResponse types for EL validity responses - ProtoArrayError and ForkChoiceError error sets
- Delete errors.zig; move error sets to proto_node.zig with TODOs for future relocation (TigerStyle: declare at point of use) - Add vote_tracker.zig with SoA-backed Votes (MultiArrayList) and NULL_VOTE_INDEX sentinel for cache-efficient computeDeltas - Refine BlockExtraMeta: 2-variant union(enum) with PostMergeMeta.init() assert to reject pre_merge status - Re-export ZERO_HASH from constants (remove duplication) - Rename block_hash_hex -> block_hash, slices() -> fields() - Clean up comments: remove emoji and TS-equivalent references
…s spec
- ProtoNode: flat layout (all ProtoBlock fields inline) with
fromBlock()/toBlock() comptime conversion
- VoteTracker: next_epoch -> next_slot, add payload_present
(Gloas LatestMessage spec)
- Use field defaults instead of DEFAULT constants (.{} construction)
- Remove Votes.init(), use field default for multi_list
- Add doc comments to all ProtoBlock/ProtoNode fields
- Remove emoji and TS-equivalent references from comments
- Update task-04, task-05 design docs
- Add lodestar-ts-reading-guide.md
Port computeDeltas from Lodestar TS with Lighthouse-style checked arithmetic. Uses pointer-equality optimization for balance comparison, sorted equivocating index advancement, and heap-allocated sort buffer. Includes 9 tests ported from TS and Lighthouse test suites.
…row sequence Consolidate 7 tests into 4 by merging init/defaults/no-op cases into a single table-driven grow sequence test.
…rison, and ePBS support - Add proto_array.zig with ProtoArray struct: onBlock (pre-Gloas/Gloas), onPayload, maybeUpdateBestChildAndDescendant, getAncestor, nodeIsViableForHead, isFinalizedRootOrDescendant, and PTC (notifyPtcMessages, isPayloadTimely, shouldExtendPayload, getPayloadStatusTiebreaker) - Add validateNodeByIndex/propagateValidExecutionStatusByIndex with error on Invalid→Valid transition (consensus safety) - Use TigerStyle infallible pattern: ensureUnusedCapacity + appendAssumeCapacity - Inline node comparison in maybeUpdateBestChildAndDescendant matching TS structure - Import computeEpochAtSlot/computeStartSlotAtEpoch from state_transition module - Refactor indices from u32 to usize throughout, remove wrapper functions - Add PayloadStatus enum, PTC_SIZE preset constant, VariantIndices type - Remove duplicate ProtoArrayError from proto_node.zig (now in proto_array.zig) - Change NULL_VOTE_INDEX to std.math.maxInt(u32), use strict < in len() assert - Port TS interface.ts comments to proto_node.zig type definitions
…y logic Implement weight propagation (two-pass backward iteration), head selection via best_descendant chains, and Gloas-specific node comparison with payload status tiebreakers. Key additions to ProtoArray: - applyScoreChanges: proposer boost undo/redo, checked arithmetic, weight back-propagation, best child/descendant recomputation - findHead: O(1) head via justified -> best_descendant chain - nodeIsViableForHead: FFG filter with pull-up justification - isFinalizedRootOrDescendant: 4 fast-path checkpoint checks + parent chain walk fallback - getAncestor: slot-based ancestor lookup via parent_root chain - shouldExtendPayload / isPayloadTimely: PTC vote counting for Gloas payload timeliness - getPayloadStatusTiebreaker: EMPTY vs FULL comparison for same-root Gloas nodes - propagateValidExecutionStatusByIndex: upward EL status promotion Also changes ProtoNode index fields from ?usize to ?u32 for TigerStyle explicit-size compliance.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
- Merge 3 Gloas test helpers into 2: asGloas + asGloasWithParentBlockHash - Remove unused block_hash_from_bid from test helpers (no production code reads it) - Add tree/diagram comments to all 50 proto_array tests - Clean up stale block_hash_from_bid references in comments - Rename onPayload → onExecutionPayload, add UnknownBlock/PreGloasBlock errors
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 lays the groundwork for the 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. Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 a new forkchoice module, which is a substantial and well-implemented feature. The code quality is high, with good use of data structures for performance (like SoA in vote_tracker.zig) and comprehensive tests. My review focuses on the compute_deltas.zig file, where I've identified a critical correctness issue regarding a type mismatch, a violation of the function length limit as per the style guide, and some opportunities to improve clarity by splitting compound conditions. I also noted a minor memory leak in the test setup. The rest of the new modules are well-structured and adhere to the repository's style guide.
…ork parameterization Convert ProtoNode and ProtoArray from plain structs to comptime functions parameterized by ForkSeq, following the BeaconState(fork) pattern. This eliminates the VariantIndices tagged union and replaces runtime fork branching with compile-time monomorphization: - ProtoNode(fork): Gloas ePBS fields (builder_index, block_hash_from_bid, parent_block_hash, payload_status) are void for pre-Gloas forks, achieving zero storage cost. - ProtoArray(fork): IndexEntry is u32 (pre-Gloas) or GloasIndices (Gloas+) at compile time. PTC votes map is void for pre-Gloas. All methods that previously switched on VariantIndices now use comptime if(is_gloas) branches. - Fork transition support: Gloas ProtoArray correctly handles pre-Gloas blocks (parent_block_hash == null) by routing them through onBlockPreGloas and storing GloasIndices with all fields pointing to the same node index. - Tests: 3 VariantIndices unit tests removed (type no longer exists). Pre-Gloas tests use ProtoArray(.phase0), Gloas tests use ProtoArray(.gloas). All 88 fork_choice tests pass.
- Simplify getParent param from degenerate comptime conditional to ?Root - Clean up redundant dead logic in getParentPayloadStatus - Add safe switch guard in onExecutionPayload for union access
Add AnyProtoArray = union(ForkSeq) as a runtime dispatch layer for the comptime-parameterized ProtoArray(ForkSeq). Follows the AnyBeaconState pattern with inline else dispatch across all fork variants. Wrapped public methods: init, deinit, initialize, getDefaultVariant, getDefaultNodeIndex, getNodeIndexByRootAndStatus, hasBlock, onBlock, onExecutionPayload, applyScoreChanges, findHeadBlock, getBlock, length, isDescendant, validateLatestHash, maybePrune. Includes upgradeToFork for fork transitions with node/index migration (pre-Gloas u32 -> Gloas GloasIndices).
…mptime fork parameterization" This reverts commit 826e5b5.
…stor queries (tasks 9-11) Complete ProtoArray with pruning, execution status validation, ancestor iteration, isDescendant, getCommonAncestor, and getAllAncestor/NonAncestor queries. All methods include comprehensive tests covering edge cases including Gloas ePBS variant handling.
Add ForkChoice wrapping ProtoArray, Votes, and checkpoint state with full public API: onBlock, onAttestation, getHead, setProposerBoost, onAttesterSlashing, prune, validateLatestHash, and Gloas ePBS methods (onExecutionPayload, notifyPtcMessages). Pre-allocated DeltasCache eliminates per-slot allocation in the getHead hot path.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
GrapeBaBa
force-pushed
the
gr/feature/forkchoice-z
branch
2 times, most recently
from
428f6db to
427dd23
Compare
Gloas fork is not yet defined in upstream config. Revert premature addition.
…ockByRoot - Add fork_choice tests to CI workflow - Update ForkChoice.isDescendant to accept explicit PayloadStatus params - Add getCanonicalBlockByRoot to walk head ancestor chain - Add payload_status to HeadResult for Gloas ePBS support
…rray - Extract ForkChoiceStore to store.zig with Rc-based balance sharing (matching state_transition pattern: init/acquire/release) - Add CheckpointWithPayloadStatus, JustifiedBalancesGetter (context+fn), EventCallback, ForkChoiceStoreEvents, computeTotalBalance - Merge proto_node.zig types and tests into proto_array.zig - Update compute_deltas to take *const EquivocatingIndices - Add store unit tests (Rc sharing, separation, events, leak detection)
|
Hey @GrapeBaBa — CI is failing with |
lodekeeper-z
left a comment
lodekeeper-z
left a comment
There was a problem hiding this comment.
Thorough review of the forkchoice module. Impressive scope — 6.3k lines covering the core DAG (ProtoArray), LMD-GHOST weight computation, Gloas ePBS multi-node model, PTC voting, and optimistic sync. The test suite is comprehensive with good coverage of Gloas variant linking and weight propagation.
Main blockers are the CI-breaking import and a couple of type mismatches that will prevent compilation. Architecture concerns are lower priority but worth discussing before this moves out of draft.
See inline comments for details.
| /// | ||
| /// This is only an approximation for two reasons: | ||
| /// - The actual block DAG in `ProtoArray`. | ||
| /// - `time` is represented using `Slot` instead of UNIX epoch `u64`. |
There was a problem hiding this comment.
🟡 ForkChoiceStore overlaps significantly with ForkChoice
Both structs track:
- justified/finalized checkpoints
- equivocating indices
- current slot
- justified balances
ForkChoice has its own checkpoint management (updateJustifiedCheckpoint, updateFinalizedCheckpoint, updateUnrealizedCheckpoints) and ForkChoiceStore has setJustified, setFinalizedCheckpoint.
Is the intent that ForkChoiceStore replaces the checkpoint management in ForkChoice, or do they serve different layers? If ForkChoiceStore is the high-level store (matching the spec's Store) and ForkChoice is the lower-level ProtoArray wrapper, the boundary should be clearer — currently both could manage the same state independently.
There was a problem hiding this comment.
this is intentional
There was a problem hiding this comment.
Got it — makes sense as a deliberate two-layer design. ForkChoiceStore = spec Store (high-level checkpoint management), ForkChoice = ProtoArray wrapper (weight/head computation). The overlap is intentional separation of concerns. 👍
|
|
||
| // Check if parent exists by getting variants | ||
| if (self.indices.get(block.parent_root)) |parent_vi| { | ||
| parent_index = switch (parent_vi) { |
There was a problem hiding this comment.
🟡 onBlockPreGloas allows orphan blocks (unknown parent)
node.parent = self.getNodeIndexByRootAndStatus(block.parent_root, .full);If the parent root is not in the DAG, node.parent is null and the block is silently inserted as an orphan. This is fine for genesis, but for non-genesis blocks it means:
- The block won't participate in weight propagation (no parent to propagate to)
- It won't be prunable via finalization ancestry
The Lodestar TS implementation rejects blocks with unknown parents in onBlock. The ForkChoice.onBlock wrapper does validate this, but ProtoArray.onBlock doesn't — so direct callers of ProtoArray could insert orphans.
Consider: should ProtoArray enforce this invariant, or is it intentionally delegated to the ForkChoice layer?
There was a problem hiding this comment.
This is same with TS implementation, @lodekeeper-z can you recheck?
There was a problem hiding this comment.
You're right — I rechecked. TS ProtoArray.onBlock does allow orphan insertion (line 455: // else: parent doesn't exist, parentIndex remains undefined). The parent validation happens at the ForkChoice layer, not ProtoArray. Same pattern here. Thanks for the correction.
There was a problem hiding this comment.
You're right — I rechecked. TS ProtoArray.onBlock does allow orphan insertion (line 455: // else: parent doesn't exist, parentIndex remains undefined). The parent validation happens at the ForkChoice layer, not ProtoArray. Same pattern here. Thanks for the correction.
Copy GrapeBaBa's fork choice implementation (6.3k lines): - ProtoArray DAG with LMD-GHOST head selection - ForkChoice with onBlock/onAttestation/getHead/prune - compute_deltas for vote weight calculation - ForkChoiceStore for checkpoint tracking - VoteTracker for per-validator votes Based on ChainSafe#246. 🤖 Generated with AI assistance
Replace naive HeadTracker with GrapeBaBa's proto-array fork choice. Wire onBlock into block import pipeline. Use getHead() for head selection. Based on ChainSafe#246. Changes: - ForkChoice wired into BlockImporter.importBlock(): builds ProtoBlock from post-state and calls fc.onBlock() - BeaconNode.initFromGenesis() initializes ForkChoice with genesis anchor - BeaconNode.getHead() uses fc.head for slot/root/state_root - BeaconNode.getStatus() uses fc checkpoints for finalized_epoch/root - BeaconNode.getSyncStatus() reads head_slot from fork choice - preset: add PTC_SIZE (Gloas ePBS) required by proto_array.zig - Optional ForkChoice (?*ForkChoice): falls back to HeadTracker when initFromGenesis has not been called yet (e.g., in tests) - HeadTracker slot_roots map preserved for req/resp range queries 🤖 Generated with AI assistance
- Fork choice: 6.3k lines from GrapeBaBa's PR ChainSafe#246 (proto-array, LMD-GHOST) - Sync manager: range sync with batching/retry (WIP - 0.16 API compat) - Discovery service: bridges discv5 → P2P with bootnode seeding - Bootnodes: mainnet defaults (Teku, Prysm, Lighthouse ENRs) - Build.zig: discv5 module wired into networking Some sync tests have 0.16 ArrayList/BoundedArray compat issues (WIP). 🤖 Generated with AI assistance
Convert remaining ProtoArray.init and ForkChoiceStore.init test calls to use the in-place initialization pattern introduced in 1d83b2d.
Consolidate two error sets into one unified ForkChoiceError, following the zig std convention of a single named error set per module. Removes the redundant ProtoArrayErr placeholder and simplifies all function signatures from (Allocator.Error || ProtoArrayError)!T to (Allocator.Error || ForkChoiceError)!T.
…ayError
Keep ProtoArrayError for precise error types within proto_array.zig
internal functions. Define ForkChoiceError = ProtoArrayError || error{...}
so fork_choice.zig uses the unified superset while proto_array.zig
retains exact error reporting. Zig auto-coerces the subset at call
boundaries.
….zig ForkChoiceError is a fork-choice-level concept, not a proto_array one. Move its definition to fork_choice.zig where it belongs, keeping ProtoArrayError in proto_array.zig for internal precision.
- Rename ProtoArrayStruct -> ProtoArray, ForkChoiceStruct -> ForkChoice - Rename proto_array import alias to pa, struct field pa to proto_array - Add isGloasBlock() method to ProtoBlock and ProtoNode - Hoist block_epoch computation in onBlockInner to avoid redundant computeEpochAtSlot calls across extractAndUpdateCheckpoints and computeAndUpdateUnrealizedCheckpoints
… onAttestation Revert helper function extractions to match TS Lodestar's intentional inline style. Inline validateBlock, extractAndUpdateCheckpoints, extractCheckpointFromState, computeAndUpdateUnrealizedCheckpoints, resolveAttestationPayloadStatus, applyVotesImmediately, and queueVotesForSlot. Remove now-unused CheckpointResult struct. Also port missing TS comments (zero-hash aliasing, unrealized checkpoint optimization, payload status, vote delay, etc.).
…estar - onBlockGloas: remove g.full orelse g.empty fallback, return null when FULL variant missing (matches TS undefined behavior) - applyScoreChanges: remove extra RevertedFinalizedEpoch guard not present in TS - getPayloadStatusTiebreaker: propagate shouldExtendPayload errors with try instead of catch false, update full call chain return types (maybeUpdateBestChildAndDescendant, compareCandidateChild, compareAgainstBestChild, updateBestDescendants) - onExecutionPayload: add execution_status parameter instead of hardcoding .valid, matching TS signature - validateLatestHash: only set irrecoverable_error for InvalidLVHExecutionResponse, ignore other errors (matches TS) - ForkChoice: add lvh_error field to preserve LVH error context when irrecoverable_error is set
|
@codex review |
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40cad0563d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| post_state.castToFork(f), | ||
| &post_cached_state.slashings_cache, | ||
| bt, | ||
| block.castToFork(bt, f), |
There was a problem hiding this comment.
stateTransition dispatches processBlock using the state's fork (f) and then blindly does block.castToFork(bt, f). With Gloas added, a state can still be .fulu at the first Gloas slot (there is no GLOAS upgrade branch in processSlots), so a .full_gloas block reaches this cast with f == .fulu and hits an invalid union field access instead of returning a typed validation error. Add an explicit fork-sequence compatibility check before this cast.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this should be handled in state transition gloas support
| if (comptime fork.gte(.electra) and fork.lt(.gloas)) { | ||
| const execution_requests = &body.inner.execution_requests; |
There was a problem hiding this comment.
This gate skips execution_requests entirely for .gloas, but there is no replacement handler in src/state_transition for ExecutionPayloadEnvelope / SignedExecutionPayloadEnvelope. As written, post-Gloas deposit/withdrawal/consolidation requests are never applied to state, which causes consensus-state divergence once Gloas is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this should be handled in state transition gloas support
| .full_fulu = out, | ||
| }; | ||
| }, | ||
| .gloas => blk: { |
There was a problem hiding this comment.
Adding .gloas block loading here enables Gloas transition fixtures, but TestCaseUtils.getForkPre() still has no .gloas arm and falls through unreachable. The transition runner calls loadPreStatePreFork() for transition tests, so Gloas cases will panic during initialization instead of loading a Fulu pre-state. Add .gloas => .fulu to the pre-fork mapping.
Useful? React with 👍 / 👎.
- Use positive condition `fork.lt(.gloas)` instead of `!fork.gte(.gloas)` - Replace GenesisBlockNotAvailable error with assert (invariant) - Propagate error from getJustifiedBlock in getSafeExecutionBlockHash - Extract AncestorAndNonAncestorBlocks named type for clarity - Force-unwrap parent FULL status (guaranteed by getParentPayloadStatus)
These methods were dead code — proposer boost is set inline in onBlock and cleared inline in onTick, matching the TS Lodestar implementation.
GrapeBaBa
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces a new fork_choice Zig module (plus benches + CI coverage) and extends the codebase to recognize the new gloas fork (ePBS) across config, types, and state-transition helpers.
Changes:
- Add a full
src/fork_choiceimplementation (store/votes/proto-array integration, attestation + block handling) plus benchmarks and CI test target. - Add
gloasfork support acrossconsensus_types,fork_types, spec test runner utilities, and config/fork sequencing. - Update state transition utilities and block processing to account for ePBS behavior (no blinded blocks, payload decoupling, execution request relocation).
Reviewed changes
Copilot reviewed 44 out of 46 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| zbuild.zon | Register fork_choice module + benches |
| test/spec/test_case.zig | Add gloas SSZ test-case loading/deinit |
| test/spec/runner/sanity.zig | Support gloas blocks in runner |
| src/state_transition/utils/execution.zig | ePBS execution-enabled semantics for gloas+ |
| src/state_transition/utils/epoch.zig | Add computeSlotsSinceEpochStart helper |
| src/state_transition/test_utils/generate_state.zig | Add config generation for gloas |
| src/state_transition/state_transition.zig | Disallow blinded blocks for gloas+ |
| src/state_transition/root.zig | Export new epoch/balance Rc helpers |
| src/state_transition/block/slash_validator.zig | Extend Electra slashing constants to gloas |
| src/state_transition/block/process_operations.zig | Skip execution_requests for gloas+ |
| src/state_transition/block/process_block.zig | Skip exec payload processing for gloas+ |
| src/preset/preset.zig | Add ePBS-related preset constants |
| src/fork_types/root.zig | Export new Any* wrapper types |
| src/fork_types/fork_types.zig | Map ForkSeq.gloas to consensus types |
| src/fork_types/beacon_state.zig | Enable fulu -> gloas state upgrade |
| src/fork_types/beacon_block.zig | Enforce no blinded / no payload fields in gloas+ |
| src/fork_types/any_indexed_attestation.zig | New AnyIndexedAttestation wrapper |
| src/fork_types/any_execution_payload.zig | Treat gloas like deneb-header variant |
| src/fork_types/any_beacon_state.zig | Add gloas state + payload availability accessor |
| src/fork_types/any_beacon_block.zig | Add gloas full block variants |
| src/fork_types/any_attester_slashing.zig | Add AnyAttesterSlashing wrapper helpers |
| src/fork_choice/vote_tracker.zig | New SoA votes storage + tests |
| src/fork_choice/store.zig | New fork-choice store + Rc balance handling |
| src/fork_choice/root.zig | Export fork-choice public API surface |
| src/fork_choice/fork_choice.zig | Core fork-choice implementation + tests |
| src/fork_choice/compute_deltas.zig | New delta computation + tests |
| src/consensus_types/root.zig | Export gloas consensus types |
| src/consensus_types/gloas.zig | New gloas SSZ types (ePBS) |
| src/config/networks/sepolia.zig | Add gloas fork fields (disabled epoch) |
| src/config/networks/minimal.zig | Add gloas fork fields + timing params |
| src/config/networks/mainnet.zig | Add gloas fork fields + timing params |
| src/config/networks/hoodi.zig | Add gloas fork fields (disabled epoch) |
| src/config/networks/gnosis.zig | Add gloas fork fields + timing params |
| src/config/networks/chiado.zig | Add gloas fork fields (disabled epoch) |
| src/config/fork_seq.zig | Add ForkSeq.gloas |
| src/config/ChainConfig.zig | Add Gloas + slot timing config fields |
| src/config/BeaconConfig.zig | Add Gloas fork info + timing helpers |
| build.zig | Add fork_choice module, tests, and benches |
| bindings/napi/BeaconStateView.zig | Disallow blinded blocks for gloas+ |
| bench/state_transition/process_block.zig | Skip exec/withdrawals benches for gloas+ |
| bench/fork_choice/util.zig | Shared fork-choice bench initialization |
| bench/fork_choice/update_head.zig | New fork-choice update-head benchmark |
| bench/fork_choice/on_attestation.zig | New fork-choice onAttestation benchmark |
| bench/fork_choice/compute_deltas.zig | New computeDeltas benchmark harness |
| .github/workflows/CI.yml | Run fork-choice tests in CI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Accept vote if it's the first vote (INIT_VOTE_SLOT) or epoch advances. | ||
| if (existing_next_slot == INIT_VOTE_SLOT or computeEpochAtSlot(next_slot) > computeEpochAtSlot(existing_next_slot)) { | ||
| fields.next_indices[validator_index] = @intCast(next_index); | ||
| fields.next_slots[validator_index] = next_slot; | ||
| } | ||
| // else it's an old vote, don't count it. |
There was a problem hiding this comment.
addLatestMessage rejects/accepts votes based on computeEpochAtSlot(next_slot), but this function now stores next_slot (Gloas LatestMessage {slot, root}). Comparing epochs will incorrectly reject newer votes within the same epoch (common case), preventing votes from updating until the epoch boundary. Use a slot comparison (e.g. accept when next_slot > existing_next_slot, plus INIT_VOTE_SLOT special-case) to properly filter stale votes.
| // Accept vote if it's the first vote (INIT_VOTE_SLOT) or epoch advances. | |
| if (existing_next_slot == INIT_VOTE_SLOT or computeEpochAtSlot(next_slot) > computeEpochAtSlot(existing_next_slot)) { | |
| fields.next_indices[validator_index] = @intCast(next_index); | |
| fields.next_slots[validator_index] = next_slot; | |
| } | |
| // else it's an old vote, don't count it. | |
| // Accept vote if it's the first vote (INIT_VOTE_SLOT) or the new vote has a higher slot. | |
| if (existing_next_slot == INIT_VOTE_SLOT or next_slot > existing_next_slot) { | |
| fields.next_indices[validator_index] = @intCast(next_index); | |
| fields.next_slots[validator_index] = next_slot; | |
| } | |
| // else it's an old or equal-slot vote, don't count it. |
| var slot_iter = self.queued_attestations.iterator(); | ||
| while (slot_iter.next()) |entry| { | ||
| const att_slot = entry.key_ptr.*; | ||
| if (att_slot < current_slot) { | ||
| // Process all attestations for this slot. | ||
| var block_iter = entry.value_ptr.iterator(); | ||
| while (block_iter.next()) |block_entry| { | ||
| const block_root = block_entry.key_ptr.*; | ||
| var vote_iter = block_entry.value_ptr.iterator(); | ||
| while (vote_iter.next()) |vote_entry| { | ||
| try self.addLatestMessage( | ||
| allocator, | ||
| vote_entry.key_ptr.*, | ||
| att_slot, | ||
| block_root, | ||
| vote_entry.value_ptr.*, | ||
| ); | ||
| } | ||
|
|
||
| if (att_slot == current_slot - 1) { | ||
| self.queued_attestations_previous_slot += @intCast(block_entry.value_ptr.count()); | ||
| } | ||
| block_entry.value_ptr.deinit(allocator); | ||
| } | ||
| entry.value_ptr.deinit(allocator); | ||
| remove_count += 1; | ||
| } else { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Remove processed slots from front. | ||
| for (0..remove_count) |_| { | ||
| const key = self.queued_attestations.keys()[0]; | ||
| _ = self.queued_attestations.orderedRemove(key); | ||
| } |
There was a problem hiding this comment.
processAttestationQueue assumes queued_attestations.iterator() yields entries in increasing att_slot order (it breaks on the first att_slot >= current_slot and then removes remove_count entries from the front). std.AutoArrayHashMapUnmanaged iteration order is insertion order, not key-sorted, so out-of-order slot insertions can cause eligible past-slot attestations to be skipped and/or the wrong slots to be removed. Iterate all entries and remove by key (or maintain a separate sorted structure / ensure sorted insertion explicitly) instead of relying on front-ordered removal + early break.
| const payload_available = state.state.executionPayloadAvailability( | ||
| checkpoint_slot % preset.SLOTS_PER_HISTORICAL_ROOT, |
There was a problem hiding this comment.
Type mismatch: executionPayloadAvailability expects index: usize (see AnyBeaconState.executionPayloadAvailability), but checkpoint_slot % preset.SLOTS_PER_HISTORICAL_ROOT is a Slot/u64. Zig requires an explicit cast here; as written this should not compile. Cast the modulo result to usize (with a safety assertion if desired) before calling executionPayloadAvailability.
| const payload_available = state.state.executionPayloadAvailability( | |
| checkpoint_slot % preset.SLOTS_PER_HISTORICAL_ROOT, | |
| const payload_index = @intCast(usize, checkpoint_slot % preset.SLOTS_PER_HISTORICAL_ROOT); | |
| const payload_available = state.state.executionPayloadAvailability( | |
| payload_index, |
Add bench_fork_choice_update_head, bench_fork_choice_compute_deltas, and bench_fork_choice_on_attestation to the benchmark build step.
…ests Remove sub-section headers from test areas in proto_array.zig and fork_choice.zig since they tend to become stale as new tests are added. Keep only code/test boundary markers.
wemeetagain
left a comment
wemeetagain
left a comment
There was a problem hiding this comment.
Looks really good. Tracks closely with the typescript version.
- Fix getElement → getFieldRoot for block_roots TreeView API - Fix spurious try on getGloasExtraMetaTyped (returns plain value) - Add 15 onAttestation unit tests: error paths (empty bitfield, future/past epoch, bad target, unknown target/head, future block, invalid target), valid vote application, queuing, zero-hash ignore, vote shifting, epoch advancement, proposer boost, and equivocating validator exclusion
…bugs - Use ForkTypes(fork).BeaconBlock.hashTreeRoot() instead of block wrapper's hashTreeRoot() which doesn't exist on BeaconBlock wrapper - Add try and .* to getFieldRoot() which returns !*const [32]u8 - Fix defer ordering in computeUnrealizedCheckpoints: destroy must run after deinit (Zig defers execute LIFO, so destroy must be declared first)
Merge latest PR head a97a220 and adapt it to Zig 0.16 plus current chain/state-transition interfaces.
- Add .gloas => .fulu to getForkPre() for transition test fixtures - Pass EquivocatingIndices by *const instead of by value in sortEquivocatingKeys - Remove redundant ensureTotalCapacity before resize in Votes.ensureValidatorCount
…ript implementation Rename function parameters and local variables in fork_choice.zig and proto_array.zig to match the naming conventions used in the TypeScript Lodestar codebase: fork_choice.zig: - onBlockInner: unrealized_justified/finalized → unrealized_justified/finalized_checkpoint - onAttestation/processAttestationQueue: att_slot → slot - validateAttestationData: current_epoch → epoch_now - updateCheckpoints: justified/finalized → justified/finalized_checkpoint - updateUnrealizedCheckpoints: same checkpoint suffix alignment - updateHead: head_node → head, score → proposer_boost_score - getDependentRoot: epoch_diff → epoch_difference - getCommonAncestorDepth: prev → prev_block - validateLatestHash: response → exec_response - prune: pruned → pruned_nodes - isDescendant: ancestor_status → ancestor_payload_status proto_array.zig: - validateLatestHash: response → exec_response - isDescendant: ancestor_status → ancestor_payload_status, descendant_status → descendant_payload_status
- Initialize anchor block PTC votes to all-true per spec get_forkchoice_store - Reject same-slot full attestation votes and require FULL variant for index=1 - Add hasPayload to proto_array/fork_choice, refactor isPayloadTimely to use it
Motivation
Lodestar-z currently lacks a fork-choice implementation. This implementation targets Lodestar's unstable branch, which includes the upcoming Gloas/ePBS fork with payload-timeliness and builder-boost mechanics.
Description
proto_array.zig— ProtoArray implementing LMD-GHOST:findHead,applyScoreChanges,onBlock,onAttestation, node viability/best-child comparison,maybePrune,isDescendant, and ePBS payload status trackingfork_choice.zig— ForkChoice orchestrator: checkpoint management,onBlock/onAttestationwith full validation, proposer boost/reorg logic,getHead,updateTime, ancestor queries, and debug/export methodscompute_deltas.zig— Vote-weight delta computation from attestation changesvote_tracker.zig— Per-validator vote tracking (current/next root + epoch)store.zig— ForkChoiceStore holding justified/finalized checkpoints, equivocation tracking, proposer boost state