fix: use body hash in blockToHeader + resolve stale TODOs#296
fix: use body hash in blockToHeader + resolve stale TODOs#296lodekeeper-z wants to merge 1 commit intoChainSafe:mainfrom
Conversation
blockToHeader was computing body_root as hash_tree_root(block) instead of hash_tree_root(block.body). This would produce incorrect block headers when used (currently unused but will be needed for fork choice and beacon-node integration). Also resolves stale TODOs in attester_status.zig and block_root.zig with context about when each function is actually needed. 🤖 Generated with AI assistance
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 addresses a functional bug in the block header generation logic and performs a cleanup of technical debt. By ensuring the correct root calculation, the integrity of the BeaconBlockHeader is maintained for future integration. Additionally, the removal of stale TODOs improves code clarity by documenting the intended architectural separation between the state transition function and the beacon-node layer. 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. 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 refactors the block header processing to extract the beacon block body before hashing and replaces several TODO comments with detailed notes regarding future implementations. A review comment suggests chaining method calls in the block header processing to minimize the number of variables in scope, adhering to the repository's style guide.
| const body = block.beaconBlockBody(); | ||
| try body.hashTreeRoot(allocator, &out.body_root); |
There was a problem hiding this comment.
While the current implementation is correct, it can be made more concise by chaining the method calls. This also reduces the number of local variables, adhering to the style guide's principle of minimizing variables in scope.
From the style guide (line 104): "minimize the number of variables in scope, to reduce the probability that variables are misused."
try block.beaconBlockBody().hashTreeRoot(allocator, &out.body_root);
References
- The style guide suggests minimizing the number of variables in scope. The
bodyvariable can be elided by chaining method calls. (link)
| } | ||
|
|
||
| // TODO: getTemporaryBlockHeader | ||
| // Note: getTemporaryBlockHeader is implemented via blockToHeader in process_block_header.zig |
There was a problem hiding this comment.
If it is already implemented, don't need a comment here anymore
| // Note: getTemporaryBlockHeader is implemented via blockToHeader in process_block_header.zig | ||
|
|
||
| // TODO: signedBlockToSignedHeader | ||
| // Note: signedBlockToSignedHeader — implement when needed for beacon-node layer |
There was a problem hiding this comment.
If it isn't implemented, just remain origin TODO comment
| } | ||
|
|
||
| // TODOs: implement missing functions | ||
| // Note: TS lodestar also has parseAttesterFlags, toAttesterFlags, parseParticipationFlags |
There was a problem hiding this comment.
don't need this change IMO
3 participants
Summary
Bug fix:
blockToHeaderwas computingbody_rootashash_tree_root(block)instead ofhash_tree_root(block.body). This would produce incorrectBeaconBlockHeadervalues. The function is currently unused but will be needed for fork choice and beacon-node integration.TODO cleanup: Resolves 3 stale TODOs with actionable notes:
attester_status.zig: Missing functions (parseAttesterFlags, etc.) are for validator monitor, not STF — noted for beacon-node layerblock_root.zig:getTemporaryBlockHeaderalready implemented asblockToHeaderinprocess_block_header.zig;signedBlockToSignedHeaderdeferred to beacon-node layer🤖 Generated with AI assistance