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

update config with fields required at Electra #99

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rolfyone
Copy link

  • added electra required fields
  • added missing fields from late block reorg which are phase0
  • added missing MAX_BLOBS_PER_BLOCK which should have been in deneb.

Left MAX_PAYLOAD_SIZE and removal of MAX_CHUNK_SIZE for #97
Left the electra fork epoch for #98

the REORG_* fields don't affect some clients as they haven't implemented late block reorg, but they should definitely be included in a config for the testnet. I'm not sure how we got away without MAX_BLOBS_PER_BLOCK for deneb.

Copy link

@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.

Looks good!

I'm not sure how we got away without MAX_BLOBS_PER_BLOCK for deneb.

Just for context: for Lighthouse we maintain our own copies of the testnet configs in our repo. We add the fields we need there, and we also have defaults for all the new fork fields so that we can maintain backwards compatibility with old configs that omit them. So, unless a param is changed from default, we don't really need it.

@rolfyone
Copy link
Author

Just for context: for Lighthouse we maintain our own copies of the testnet configs in our repo. We add the fields we need there, and we also have defaults for all the new fork fields so that we can maintain backwards compatibility with old configs that omit them. So, unless a param is changed from default, we don't really need it.

We're almost ready for late block reorg so not having these fields would definitely be sub optimal, but we can maintain our own config, it just seems pointless to have an incomplete testnet config on a repo supposed to house the testnet config...

@michaelsproul
Copy link

Yeah, I agree we should merge it. Just wanted to give context for why LH isn't broken without these changes

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.

2 participants