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 Values to Match With the Latest Spec #5

Merged
merged 3 commits into from
Mar 17, 2025

Conversation

nisdas
Copy link
Contributor

@nisdas nisdas commented Mar 17, 2025

In ethereum/consensus-specs#4045 , both GOSSIP_MAX_SIZE and MAX_CHUNK_SIZE were replaced with MAX_PAYLOAD_SIZE . This PR brings the hoodi config up to parity with the latest spec as of v1.5.0-beta.3

Copy link

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM

@arnetheduck
Copy link

hm, the bootnode consistency check is .. unrelated. Can you take a quick look regardless?

@barnabasbusa
Copy link

Could you please also adjust

# `2**14` (= 16,384)
MIN_GENESIS_ACTIVE_VALIDATOR_COUNT: 16384

So we are as close to mainnet as possible.

@nisdas
Copy link
Contributor Author

nisdas commented Mar 17, 2025

hm, the bootnode consistency check is .. unrelated. Can you take a quick look regardless?

I am assuming this is a flake, not sure why it would fail with the current set of changes. A new run has been triggered for it

@parithosh
Copy link
Member

hm, the bootnode consistency check is .. unrelated. Can you take a quick look regardless?

I am assuming this is a flake, not sure why it would fail with the current set of changes. A new run has been triggered for it

I think there's a newline causing the fail?

@@ -93,15 +93,13 @@ DEPOSIT_CONTRACT_ADDRESS: 0x00000000219ab540356cBB839Cbe05303d7705Fa
# Networking
# ---------------------------------------------------------------
# `10 * 2**20` (= 10485760, 10 MiB)

Choose a reason for hiding this comment

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

Suggested change
# `10 * 2**20` (= 10485760, 10 MiB)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we want to remove this ?

@@ -5,7 +5,7 @@ CONFIG_NAME: hoodi
# Genesis
# ---------------------------------------------------------------
# `2**14` (= 16,384)
MIN_GENESIS_ACTIVE_VALIDATOR_COUNT: 1000000
MIN_GENESIS_ACTIVE_VALIDATOR_COUNT: 16384
Copy link

Choose a reason for hiding this comment

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

why are we changing this now? we already cut a rc yesterday which people might be running with the current value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just following this:
#5 (comment)

Copy link

Choose a reason for hiding this comment

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

Ok maybe it does not matter, looks like the only thing we do in Lodestar with MIN_GENESIS_ACTIVE_VALIDATOR_COUNT is "waiting for genesis validators" but since both values 16384 and 1000000 are fulfilled it should be fine to run with previous config

Copy link
Member

Choose a reason for hiding this comment

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

thank you! approving and merging the PR then

@parithosh parithosh merged commit 828c2c9 into eth-clients:main Mar 17, 2025
1 check failed
nflaig added a commit to ChainSafe/lodestar that referenced this pull request Mar 17, 2025
…etwork config (#7594)

This value will be updated by
eth-clients/hoodi#5, this should be part of our
next rc but it's a inconsequential change as both
`MIN_GENESIS_ACTIVE_VALIDATOR_COUNT` values `1000000` vs. `16384` are
already fulfilled by genesis state so "waiting for genesis validators"
will not happen.

this is the code I am talking about

https://github.com/ChainSafe/lodestar/blob/f55be17bd3158f207402648da278db9560be7e91/packages/beacon-node/src/chain/genesis/genesis.ts#L140
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.

6 participants