feat: mix in blob schedule when calculating fork digest #7954
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
ensi321
added
the
status-do-not-merge
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
twoeths
reviewed
Jun 13, 2025
twoeths
reviewed
Jun 13, 2025
twoeths
reviewed
Jun 13, 2025
| unsubscribeSubnetsFromPrevFork(prevFork: ForkName): void { | ||
| this.logger.info("Unsubscribing to random attnets from prev fork", {prevFork}); | ||
| unsubscribeSubnetsBeforeBoundary(boundary: SubscribeBoundary): void { | ||
| this.logger.info("Unsubscribing to random attnets from prev fork", boundary); |
Contributor
There was a problem hiding this comment.
Suggested change
| this.logger.info("Unsubscribing to random attnets from prev fork", boundary); | |
| this.logger.info("Unsubscribing to random attnets from prev boundary", boundary); |
twoeths
reviewed
Jun 13, 2025
|
|
||
| export type MultiaddrStr = string; | ||
| // Boundary of network subscription. We subscribe/unsubscribe during fork and blob schedule transitions | ||
| export type SubscribeBoundary = {fork: ForkName; epoch?: Epoch} | ({fork: ForkName} & BlobScheduleEntry); |
Contributor
There was a problem hiding this comment.
suggest to make it more specific
Suggested change
| export type SubscribeBoundary = {fork: ForkName; epoch?: Epoch} | ({fork: ForkName} & BlobScheduleEntry); | |
| export type SubscribeBoundary = {fork: ForkPreFulu; epoch: Epoch} | ({fork: ForkPostFulu} & BlobScheduleEntry); |
then in both cases we have epoch, then for a lot of interfaces we only need epoch like forkName2ForkDigestHex(epoch): ForkDigestHex
here I suppose we only need to cache by epoch in out config as commented here
Contributor
Author
There was a problem hiding this comment.
here I suppose we only need to cache by epoch in out config as commented #7954 (comment)
We can cache epoch in genesis config but not here.
Reason being SubscribeBoundary resides in GossipTopic, and we need fork info to do something like getGossipSSZType. Sometimes it is hard to get fork info from epoch because a lot of places don't have access to config. It would need some non-trivial refactoring for some places to have access to config
Contributor
Author
There was a problem hiding this comment.
here I suppose we only need to cache by epoch in out config as commented #7954 (comment)
We can cache epoch in genesis config but not here.
Reason being
SubscribeBoundaryresides inGossipTopic, and we need fork info to do something likegetGossipSSZType. Sometimes it is hard to get fork info from epoch because a lot of places don't have access toconfig. It would need some non-trivial refactoring for some places to have access toconfig
There is also one issue with SubscribeBoundary being epoch only, is that when we are in the fulu fork, but before the first BPO fork, fork digest is calculated using ELECTRA_FORK_EPOCH here, but the ssz types still use fulu's. So we need fork info to indicate which fork we are in to derive the ssz type, and epoch to indicate 1) where the boundary is 2) calculate fork digest
twoeths
reviewed
Jun 13, 2025
packages/reqresp/src/types.ts
Outdated
| | { | ||
| type: ContextBytesType.ForkDigest; | ||
| forkDigestContext: ForkDigestContext; | ||
| fork: ForkName; |
Contributor
There was a problem hiding this comment.
same to the other comment, we only need to add an epoch here to simplify, no need fork and blobSchedule
Contributor
Author
There was a problem hiding this comment.
This has been moved to ResponseOutgoing
twoeths
reviewed
Jun 13, 2025
| forkName2ForkDigest(forkName: ForkName): ForkDigest; | ||
| forkName2ForkDigestHex(forkName: ForkName): ForkDigestHex; | ||
| forkName2ForkDigest(forkName: ForkName, blobSchedule: BlobScheduleEntry | null): ForkDigest; | ||
| forkName2ForkDigestHex(forkName: ForkName, blobSchedule: BlobScheduleEntry | null): ForkDigestHex; |
Contributor
There was a problem hiding this comment.
with this enhancement this name is not suitable anymore, maybe just go with getForkDigest(epoch: Epoch): ForkDigest; instead
nflaig
reviewed
Jun 21, 2025
|
|
||
| function computeForkDigest(currentVersion: Version, genesisValidatorsRoot: Root): ForkDigest { | ||
| return computeForkDataRoot(currentVersion, genesisValidatorsRoot).slice(0, 4); | ||
| function computeForkDigest( |
Member
There was a problem hiding this comment.
there are tests for this on the spec test_compute_fork_digest.py, but does not look like we have wired up the spec iterator for those yet
Co-authored-by: Nico Flaig <[email protected]>
Co-authored-by: Nico Flaig <[email protected]>
Contributor
Author
|
Superseded by #8022 |
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.
ResponseOutgoingto calculate context bytesContext bytes calculation is now based on fork and blob schedule fromProtocolinstead of from chunkSpec: ethereum/consensus-specs#4354