feat: update networking config to match CL spec #8510
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
Member
Author
|
/gemini review |
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to refactor networking configurations to align with the Consensus Layer (CL) specifications. The changes are extensive and consistently applied across the codebase, including updates to tests, which significantly improves maintainability by centralizing these constants. I've identified one minor point regarding a new configuration that could be clarified. Overall, this is a high-quality contribution.
Member
Author
|
/gemini review |
nflaig
commented
Oct 8, 2025
| // Reject ContributionAndProofs of previous slots | ||
| // for api ContributionAndProofs, we allow them to be added to the pool | ||
| if (!priority && slot < this.clock.slotWithPastTolerance(MAXIMUM_GOSSIP_CLOCK_DISPARITY)) { | ||
| if (!priority && slot < this.clock.slotWithPastTolerance(this.config.MAXIMUM_GOSSIP_CLOCK_DISPARITY / 1000)) { |
Member
Author
There was a problem hiding this comment.
this was wrong before slotWithPastTolerance expects value in seconds
Contributor
There was a problem hiding this comment.
Code Review
This is a great pull request that refactors networking configurations to align with the Consensus Layer (CL) specifications. The changes are well-organized and consistently applied across the codebase, which significantly improves maintainability by centralizing configuration parameters. The removal of hardcoded constants in favor of values from ChainConfig is a solid improvement. All tests have been updated accordingly, and the new constant names and values appear correct. Overall, this is a high-quality contribution.
nflaig
commented
Oct 8, 2025
| export const EPOCHS_PER_SUBNET_SUBSCRIPTION = 256; | ||
| export const ATTESTATION_SUBNET_EXTRA_BITS = 0; | ||
| export const ATTESTATION_SUBNET_PREFIX_BITS = | ||
| Math.ceil(Math.log2(ATTESTATION_SUBNET_COUNT)) + ATTESTATION_SUBNET_EXTRA_BITS; |
Member
Author
There was a problem hiding this comment.
The ATTESTATION_ constants are part of config as per spec but kept here since we use them in ssz containers which makes it hard to move them. We still return them from config/spec now and I don't think anyone wants to configure those.
Member
There was a problem hiding this comment.
If I had to guess, that would be an oversight/bug? Not sure if its worth updating the spec by now, but I don't think config should affect ssz container definitions.
Member
Author
There was a problem hiding this comment.
Yes, I don't see why those are in config and likely an oversight in the spec but it's part of phase0 and I don't really feel like it's worth fixing this as it's simple for us to just keep them as constants.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #8510 +/- ##
============================================
+ Coverage 52.19% 52.22% +0.02%
============================================
Files 852 852
Lines 65054 65082 +28
Branches 4771 4771
============================================
+ Hits 33955 33986 +31
+ Misses 31030 31027 -3
Partials 69 69 🚀 New features to boost your workflow:
|
Contributor
Performance Report✔️ no performance regression detected Full benchmark results
|
wemeetagain
approved these changes
Oct 13, 2025
AbolareRoheemah
pushed a commit
to AbolareRoheemah/lodestar
that referenced
this pull request
Oct 14, 2025
**Motivation** Conform to CL spec **Description** - move networking constants to config - remove constants no longer part of spec - enable "p2p-interface.md" in config test Closes ChainSafe#6351 Closes ChainSafe#7529
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
Conform to CL spec
Description
Closes #6351
Closes #7529