Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added electra fields to configuration #120

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

rolfyone
Copy link
Contributor

This includes the electra epoch from #119 so we rely on that merging, or can change it back to MAX_UINT64.

Basically when I was raising a PR for 119, I noticed there were a bunch of electra fields not included.

Also, potentially importantly, assuming we're wanting to switch GOSSIP_MAX_SIZE to MAX_PAYLOAD_SIZE?

@tersec
Copy link
Contributor

tersec commented Jan 28, 2025

Nimbus directly uses this repo as the source of truth for Holesky. I would very much prefer that there be a commit in main which is a minimal-difference commit which does nothing but add the ELECTRA_FORK_EPOCH/ELECTRA_FORK_VERSION.

The Feb 3rd date is already, for the Nimbus release pipeline, and devnet-5 shenanigans, cutting things very close, given the ACDE meeting date. I do not want to add unnecessary risk to this release process. Once that passes, sure.

@rolfyone
Copy link
Contributor Author

rolfyone commented Jan 28, 2025

Nimbus directly uses this repo as the source of truth for Holesky. I would very much prefer that there be a commit in main which is a minimal-difference commit which does nothing but add the ELECTRA_FORK_EPOCH/ELECTRA_FORK_VERSION.

The Feb 3rd date is already, for the Nimbus release pipeline, and devnet-5 shenanigans, cutting things very close, given the ACDE meeting date. I do not want to add unnecessary risk to this release process. Once that passes, sure.

Please note that without the electra fields it's going to be difficult to startup with electra. I raised this because the existing PR won't validate against an electra configuration for teku. AFAIK this is the required configuration to be able to run with Electra enabled, or we'll be missing required configuration.

@rolfyone
Copy link
Contributor Author

rolfyone commented Jan 28, 2025

The only other main change other than electra required things is the renaming from GOSSIP_MAX_SIZE (which include removing MAX_CHUNK_SIZE in the same PR). For some reason this was missing MAX_BLOBS_PER_BLOCK which was introduced in deneb and still a required global field.

@rolfyone
Copy link
Contributor Author

removed change that is in #118 and #119

Electra should require all three of these changes.

Copy link
Contributor

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config looks good to me. Lighthouse is neutral on the issue of merging now vs later, as we don't actually use this repo (see: eth-clients/sepolia#99 (review)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants