fix: process fulu data columns without waiting for block - #9166
Closed
nflaig wants to merge 2 commits into
Closed
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the getBeaconBlockRootFromFuluDataColumnSidecarSerialized utility to extract beacon block roots from serialized Fulu data column sidecars. It also updates the network processor to handle these sidecars by queuing them for validation without requiring the full block immediately, and includes corresponding unit tests. I have no feedback to provide as the existing review comment was purely explanatory.
![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: ea28fb780c
鈩癸笍 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".
Comment on lines
+560
to
+564
| const blockHeader = ssz.phase0.BeaconBlockHeader.deserialize( | ||
| data.subarray( | ||
| SLOT_BYTES_POSITION_IN_SIGNED_DATA_COLUMN_SIDECAR_PRE_GLOAS, | ||
| SLOT_BYTES_POSITION_IN_SIGNED_DATA_COLUMN_SIDECAR_PRE_GLOAS + BEACON_BLOCK_HEADER_SIZE | ||
| ) |
There was a problem hiding this comment.
getBeaconBlockRootFromFuluDataColumnSidecarSerialized() now calls ssz.phase0.BeaconBlockHeader.deserialize(...) directly during the network pre-processing path, but this helper is expected to be non-throwing (all other extractors return null for invalid input). A malformed sidecar with valid length but invalid header encoding can make this deserialize call throw, and onPendingGossipsubMessage does not catch extractor errors, so untrusted gossip can abort processing instead of being handled as invalid gossip. Please wrap this deserialize in a try/catch and return null on parse failure.
Useful? React with 馃憤聽/ 馃憥.
Member
Author
There was a problem hiding this comment.
I guess this is valid, also calling hashTreeRoot() here might be a concern in general since we wanna avoid expensive work
5 tasks
Contributor
Performance Report鉁旓笍 no performance regression detected Full benchmark results
|
twoeths
reviewed
Apr 3, 2026
| SLOT_BYTES_POSITION_IN_SIGNED_DATA_COLUMN_SIDECAR_PRE_GLOAS + BEACON_BLOCK_HEADER_SIZE | ||
| ) | ||
| ); | ||
| const blockRoot = ssz.phase0.BeaconBlockHeader.hashTreeRoot(blockHeader); |
Member
There was a problem hiding this comment.
so now we have to do a hashTreeRoot() here which is not cheap
Member
Author
There was a problem hiding this comment.
yep same concern I had here #9166 (comment), do we just want the change in network process index file?
I guess this becomes unnecessary if we do not extract the root since it's skipped anyways here
if there is no root extracted
twoeths
reviewed
Apr 3, 2026
twoeths
left a comment
twoeths
left a comment
Member
There was a problem hiding this comment.
now I've just noticed that for fulu DataColumnSidecar, we even don't have a block root there
so it will not reach this https://github.com/ChainSafe/lodestar/pull/9166/changes#diff-d2cbc467ecd1859e4b52649025596463a04f60a06c850c1007a77827f773a903R345
meaning it will not await for block
seems like the current implementation is already as designed
Member
Author
|
we already don't wait for block if there is no extracted root and closing |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
per #9162 (review)