feat: signature verification for reqresp DA #8580
Merged
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
matthewkeil
commented
Oct 28, 2025
Comment on lines
-459
to
-470
| // TODO(fulu) not in use, remove? | ||
| export type ValidateColumnSidecarsProps = Pick< | ||
| FetchByRootAndValidateColumnsProps, | ||
| "config" | "peerMeta" | "blockRoot" | "missing" | ||
| > & { | ||
| slot: number; | ||
| blobCount: number; | ||
| needed?: fulu.DataColumnSidecars; | ||
| needToPublish?: fulu.DataColumnSidecars; | ||
| }; | ||
|
|
||
| // TODO(fulu) not in use, remove? |
Member
Author
There was a problem hiding this comment.
These were unused. Removed them instead of updating the inner function call for validation
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8580 +/- ##
============================================
- Coverage 51.99% 51.96% -0.04%
============================================
Files 848 848
Lines 65857 65928 +71
Branches 4808 4814 +6
============================================
+ Hits 34242 34258 +16
- Misses 31547 31602 +55
Partials 68 68 🚀 New features to boost your workflow:
|
nflaig
reviewed
Oct 31, 2025
| } | ||
|
|
||
| if (chain !== null) { | ||
| const headState = await chain.getHeadState(); |
Member
There was a problem hiding this comment.
I don't think this is correct, need to get block state (same as in gossip validation)
lodestar/packages/beacon-node/src/chain/validation/dataColumnSidecar.ts
Lines 105 to 106 in 5317389
Member
Author
There was a problem hiding this comment.
No, not needed. Only thing that is used to calculate the signing root is the slot of the object being signed. Not sure why we use the state in gossip actually. I think just because it has the keys on the epochCtx
Member
There was a problem hiding this comment.
I am pretty sure in some re-org scenario or when we validate sidecars from a different chain the pubkey2index could be different between the states
this seems unlikely but we also use the state slot to compute the domain which can very likely be wrong around fork boundary if we always use the head state
@twoeths should probably double check this
Member
Author
There was a problem hiding this comment.
The validator/proposer indices changes based on slot-root but the key cache does not to my knowledge
Contributor
There was a problem hiding this comment.
agree with @nflaig that we need to use the state of parent root dialing to block header slot
the context is the same to gossip, we need to use a state of the same "view/branch" of the message to handle reorg scenario. We dial to block header slot to handle fork boundary
using head state is mostly correct for stable network condition but it does not work all the time, and there is no advantage doing that except for more concise code so let's just not use it
Member
Author
There was a problem hiding this comment.
Its not possible with byRoot code is why I switched it. The state transition has not been run yet, thus there is no state cached, when we attempt to verify the signature. But sending the blocks for validation (partial but not has_all_data will need to happen before the full block input goes through the process pipeline
Member
There was a problem hiding this comment.
Its not possible with byRoot code is why I switched it. The state transition has not been run yet, thus there is no state cached, when we attempt to verify the signature.
it works on gossip, why wouldn't it work here?
Contributor
There was a problem hiding this comment.
we need state of parent dialing to block slot (not the state of block itself) so should be able to do that
nflaig
reviewed
Oct 31, 2025
twoeths
reviewed
Nov 3, 2025
twoeths
reviewed
Nov 3, 2025
nflaig
reviewed
Nov 3, 2025
packages/beacon-node/src/chain/errors/dataColumnSidecarError.ts
Outdated
Show resolved
Hide resolved
nflaig
reviewed
Nov 3, 2025
nflaig
reviewed
Nov 3, 2025
nflaig
reviewed
Nov 3, 2025
packages/beacon-node/src/chain/errors/dataColumnSidecarError.ts
Outdated
Show resolved
Hide resolved
nflaig
reviewed
Nov 3, 2025
packages/beacon-node/src/chain/errors/dataColumnSidecarError.ts
Outdated
Show resolved
Hide resolved
nflaig
reviewed
Nov 3, 2025
wemeetagain
approved these changes
Nov 3, 2025
nflaig
approved these changes
Nov 3, 2025
Member
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces signature verification for blob and data column sidecars received over req/resp, aligning with upcoming specification changes. The implementation is well-structured, adding new signature set creation functions and passing the IBeaconChain instance to validation functions where BLS verification is necessary. A key improvement is the more robust validation of sidecars by comparing the full SignedBeaconBlockHeader. The use of an optional chain parameter in validation functions is a good design choice, allowing control over when signature verification is performed, which is particularly useful for testing and historical sync scenarios. My feedback includes a couple of suggestions to improve parameter naming in packages/state-transition/src/signatureSets/proposer.ts for better code clarity and maintainability.
Member
|
🎉 This PR is included in v1.36.0 🎉 |
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
Spec will be updated to have check of signatures via reqresp. Proacative fix inline with Lighthouse and Prysm
sigp/lighthouse#7650