feat: update configs to be able to support Deneb on all networks #6328
Conversation
This file contains 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
nflaig
commented
Jan 19, 2024
| PROPOSER_SCORE_BOOST: false, // Ignored as it's changing https://github.com/ethereum/consensus-specs/pull/2895 | ||
|
|
||
| // Deposit contract | ||
| DEPOSIT_CHAIN_ID: false, // Non-critical | ||
| DEPOSIT_NETWORK_ID: false, // Non-critical | ||
| DEPOSIT_CONTRACT_ADDRESS: true, | ||
|
|
||
| // Networking | ||
| MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS: false, |
There was a problem hiding this comment.
@g11tech Based on my understanding of this value it is not critical to verify it here as it is only relevant for the beacon node and as part of networking can't result in a consensus failure.
Maybe you could confirm this, and should we add a comment here?
There was a problem hiding this comment.
yes it doesn't affect consensus, adding with false is ok
There was a problem hiding this comment.
I added a comment in e94355b
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6328 +/- ##
=============================================
- Coverage 76.60% 60.13% -16.47%
=============================================
Files 248 407 +159
Lines 25908 46451 +20543
Branches 1448 1534 +86
=============================================
+ Hits 19846 27935 +8089
- Misses 6032 18486 +12454
Partials 30 30 |
Performance Report✔️ no performance regression detected Full benchmark results
|
wemeetagain
reviewed
Jan 19, 2024
nflaig
changed the title
wemeetagain
approved these changes
Jan 22, 2024
g11tech
approved these changes
Jan 23, 2024
|
🎉 This PR is included in v1.15.0 🎉 |
jeluard
pushed a commit
to jeluard/lodestar
that referenced
this pull request
Jan 30, 2024
…inSafe#6328) * Move `MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTS` from constants to config * Align structure / comments with consensus spec * Add references to preset / config files * Rename directory and files that contain config values * Add comment to networking params
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
Currently it is not possible to support Deneb on other networks like Gnosis as we define values that need to be part of configuration as constants and hence those can't be customized.
Description
MIN_EPOCHS_FOR_BLOB_SIDECARS_REQUESTSfrom constants to config, this value is defined as part of configuration in the spec (specs/deneb/p2p-interface.md?plain=1#L66)Support all network configs
After reviewing the spec values I noticed we are still missing plenty of network values which should be part of the config (configs/mainnet.yaml#L113-L150). We either don't support them or those are defined as constants.
I looked into refactoring this to move all of the constants into config but not all parts of the code that use those values can get access to configs easily, for example values used in
rateLimitQuotaslodestar/packages/beacon-node/src/network/reqresp/rateLimit.ts
Line 11 in 2db7120
While it should be possible to refactor the code to make this work, I think we should avoid large changes before the Deneb hard fork as current configuration is well tested already and there is no specific requirements to move those values from constants to config.
BREAKING CHANGE
Mainnet and minimal configs are now exported from
/configssub path instead of/presets.The following change needs to be done if you are directly importing these from the
@lodestar/configpackage