fix: use previous slot to get sync aggregate for block #8494
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Summary of ChangesHello @nflaig, 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 implements a critical fix to the block production logic, specifically concerning the inclusion of sync aggregates. It resolves an issue where using the Highlights
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
|
Contributor
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a potential BLOCK_ERROR_INVALID_SIGNATURE issue that could arise during fork transitions, especially in scenarios with missed slots. The core of the fix is to consistently use the previous slot (slot - 1) for fetching sync aggregates, rather than the parent block's slot. This change ensures that the sync aggregate included in a new block corresponds to the slot expected by the signature verification logic, which also relies on slot - 1 for domain computation.
The implementation is clean and effective. A significant part of the change involves refactoring to remove the parentSlot parameter from дълга верига от извиквания на функции, което опростява кода и премахва ненужна зависимост. Промените са приложени последователно във всички засегнати файлове, включително актуализации на тестовете, за да отразяват новите сигнатури на функциите. Като цяло, това е стабилна корекция, която подобрява надеждността на процеса на производство на блокове.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8494 +/- ##
=========================================
Coverage 52.24% 52.24%
=========================================
Files 852 852
Lines 65005 64991 -14
Branches 4772 4772
=========================================
- Hits 33959 33952 -7
+ Misses 30977 30970 -7
Partials 69 69 🚀 New features to boost your workflow:
|
Contributor
Performance Report🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
Member
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses a critical bug where using the parent slot to fetch sync aggregates could lead to invalid block signatures, especially with missed slots around epoch boundaries. The fix correctly changes this to use the previous slot (slot - 1), aligning with the specification and preventing signature domain mismatches. The changes also include a nice cleanup, removing the now-redundant parentSlot parameter from several function signatures throughout the block production call stack. The implementation is solid and the reasoning is sound. I have no further suggestions.
wemeetagain
approved these changes
Oct 2, 2025
twoeths
approved these changes
Oct 3, 2025
Contributor
twoeths
left a comment
There was a problem hiding this comment.
looks good to me, adding this spec to remind us later
If a validator is in the current sync committee (i.e. is_assigned_to_sync_committee() above returns True), then for every slot in the current sync committee period, the validator should prepare a SyncCommitteeMessage for the previous slot (slot - 1) according to the logic in get_sync_committee_message as soon as they have determined the head block of slot - 1. This means that when assigned to slot a SyncCommitteeMessage is prepared and broadcast in slot-1 instead of slot.
hence it should be previous slot, not slot of parent block
Member
Author
that's a hint too but I think the best evidence is in previous_slot = max(state.slot, Slot(1)) - Slot(1)
domain = get_domain(state, DOMAIN_SYNC_COMMITTEE, compute_epoch_at_slot(previous_slot))since it uses previous slot for getting the domain we need to do the same to pack the block |
Member
|
🎉 This PR is included in v1.35.0 🎉 |
nflaig
added a commit
that referenced
this pull request
Oct 9, 2025
**Motivation** Since #7927 we started to use parent slot (the slot of parent block) to get sync aggregate for block but this can cause `BLOCK_ERROR_INVALID_SIGNATURE` during fork transition as might include sync aggregates into our block that are older than block slot - 1 which is used to compute domain during signature verification. https://github.com/ChainSafe/lodestar/blob/8961b06c11ac67baab79fe774e0a3d1bfb217ea1/packages/state-transition/src/block/processSyncCommittee.ts#L81 This means we will reject our own block due to invalid signature if there are missed slots during the epoch transition. > we currently hard code parent slot as producedSlot - 1 which is incorrect in reorg scenario This was the motivation from the previous change but this doesn't seem right, sync committee messages are produced at latest 4 seconds into the slot so if we re-org the block due to it being weak/late then sync committee votes will also be for parent block root of that block and not the block itself. So irrespective of block being reorged or not we should always include sync committee messages from previous slot. **Description** Use previous slot to get sync aggregate for block to avoid `BLOCK_ERROR_INVALID_SIGNATURE` in case there are missed slots.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Since #7927 we started to use parent slot (the slot of parent block) to get sync aggregate for block but this can cause
BLOCK_ERROR_INVALID_SIGNATUREduring fork transition as might include sync aggregates into our block that are older than block slot - 1 which is used to compute domain during signature verification.lodestar/packages/state-transition/src/block/processSyncCommittee.ts
Line 81 in 8961b06
This means we will reject our own block due to invalid signature if there are missed slots during the epoch transition.
This was the motivation from the previous change but this doesn't seem right, sync committee messages are produced at latest 4 seconds into the slot so if we re-org the block due to it being weak/late then sync committee votes will also be for parent block root of that block and not the block itself.
So irrespective of block being reorged or not we should always include sync committee messages from previous slot.
Description
Use previous slot to get sync aggregate for block to avoid
BLOCK_ERROR_INVALID_SIGNATUREin case there are missed slots.