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

Add deneb config for Chiado #7886

Merged
merged 12 commits into from
Jan 18, 2024
Merged

Add deneb config for Chiado #7886

merged 12 commits into from
Jan 18, 2024

Conversation

rolfyone
Copy link
Contributor

fixes #7871

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

fixes Consensys#7871

Signed-off-by: Paul Harris <paul.harris@consensys.net>
Signed-off-by: Paul Harris <paul.harris@consensys.net>
@rolfyone
Copy link
Contributor Author

Theres probably a few things to push up stream so that there's less changes in this between the published and our version...

Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

You probably need to add a line like this too https://github.com/ConsenSys/teku/blob/5ac9458e040f900cf61d164ff8084176239d22db/ethereum/networks/src/main/java/tech/pegasys/teku/networks/Eth2NetworkConfiguration.java#L653

Also we could update gnosis deneb settings, but it could be done in separate PR

@rolfyone
Copy link
Contributor Author

Ultimately what i want is to be able to have https://github.com/gnosischain/configs/blob/main/chiado/config.yaml in our chiado.yaml, but its very different currently...
(below the chiado.yaml is the one from the gnosis ref above)

diff chiado.yaml ethereum/spec/src/main/resources/tech/pegasys/teku/spec/config/configs/chiado.yaml
41a42,51
> # Fork Choice
> # ---------------------------------------------------------------
> # 2**3 (= 8)
> SAFE_SLOTS_TO_UPDATE_JUSTIFIED: 8
> # 20%
> REORG_HEAD_WEIGHT_THRESHOLD: 20
> # 160%
> REORG_PARENT_WEIGHT_THRESHOLD: 160
> # `2` epochs
> REORG_MAX_EPOCHS_SINCE_FINALIZATION: 2
48,49d57
< # 2**0 (= 1)
< RANDOM_SUBNETS_PER_VALIDATOR: 1
157a166,167
> # [New in Deneb:EIP7514] 2**3 (= 8)
> MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT: 8
158a169,197
> # Networking
> # ---------------------------------------------------------------
> # `10 * 2**20` (= 10485760, 10 MiB)
> GOSSIP_MAX_SIZE: 10485760
> # `2**10` (= 1024)
> MAX_REQUEST_BLOCKS: 1024
> # `2**8` (= 256)
> EPOCHS_PER_SUBNET_SUBSCRIPTION: 256
> ## `MIN_VALIDATOR_WITHDRAWABILITY_DELAY + CHURN_LIMIT_QUOTIENT // 2`
> MIN_EPOCHS_FOR_BLOCK_REQUESTS: 2304
> # `10 * 2**20` (=10485760, 10 MiB)
> MAX_CHUNK_SIZE: 10485760
> # 5s
> TTFB_TIMEOUT: 5
> # 10s
> RESP_TIMEOUT: 10
> ATTESTATION_PROPAGATION_SLOT_RANGE: 32
> # 500ms
> MAXIMUM_GOSSIP_CLOCK_DISPARITY: 500
> MESSAGE_DOMAIN_INVALID_SNAPPY: 0x00000000
> MESSAGE_DOMAIN_VALID_SNAPPY: 0x01000000
> # 2 subnets per node
> SUBNETS_PER_NODE: 2
> # 2**8 (= 64)
> ATTESTATION_SUBNET_COUNT: 64
> ATTESTATION_SUBNET_EXTRA_BITS: 0
> # ceillog2(ATTESTATION_SUBNET_COUNT) + ATTESTATION_SUBNET_EXTRA_BITS
> ATTESTATION_SUBNET_PREFIX_BITS: 6
  • The fork choice section was in our previous file, so i copied that back in,
  • MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT copied from mainnet which sounds like it was incorrect
  • networking section was in our previous implementation, so I copied that.
    I'm not sure where these fields come from for everyone else, so it's a bit curious. I wonder if @daplion has any input on these...

It does look like I missed RANDOM_SUBNETS_PER_VALIDATOR from the diff above... I'll double check that...

To me the 2 missing sections, and probably MAX_PER_EPOCH_ACTIVATION_CHURN_LIMIT should all be in the config source we copy...

@rolfyone
Copy link
Contributor Author

rolfyone commented Jan 15, 2024

just on RANDOM_SUBNETS_PER_VALIDATOR, that flags as an unknown option:

2024-01-16 06:48:19.441+10:00 | main | INFO  | SpecConfigReader | Ignoring non-configurable constants supplied in network configuration: BLS_WITHDRAWAL_PREFIX, TARGET_AGGREGATORS_PER_COMMITTEE, EPOCHS_PER_RANDOM_SUBNET_SUBSCRIPTION, DOMAIN_BEACON_PROPOSER, DOMAIN_BEACON_ATTESTER, DOMAIN_RANDAO, DOMAIN_DEPOSIT, DOMAIN_VOLUNTARY_EXIT, DOMAIN_SELECTION_PROOF, DOMAIN_AGGREGATE_AND_PROOF, DOMAIN_SYNC_COMMITTEE, DOMAIN_CONTRIBUTION_AND_PROOF, DOMAIN_SYNC_COMMITTEE_SELECTION_PROOF

java.lang.IllegalArgumentException: Unable to load configuration for network "chiado": Detected unknown spec config entries: RANDOM_SUBNETS_PER_VALIDATOR

This looks to be because:
ethereum/consensus-specs#2749 which starts using different network fields, and also explains a lot of the network fields...

@zilm13
Copy link
Contributor

zilm13 commented Jan 15, 2024

this option was outdated with spec changes see #7341

Signed-off-by: Paul Harris <paul.harris@consensys.net>
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

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

@rolfyone
Copy link
Contributor Author

You probably need to add a line like this too https://github.com/ConsenSys/teku/blob/5ac9458e040f900cf61d164ff8084176239d22db/ethereum/networks/src/main/java/tech/pegasys/teku/networks/Eth2NetworkConfiguration.java#L653

yes but now we have a test that will break ;)

Signed-off-by: Paul Harris <paul.harris@consensys.net>
Signed-off-by: Paul Harris <paul.harris@consensys.net>
@dapplion
Copy link

@rolfyone thanks for noticing, the chiado config was not updated to the latest set of keys. Now fixed with

Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@4rgon4ut 4rgon4ut left a comment

Choose a reason for hiding this comment

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

LGTM. Added some minor suggestions according to recent changes in gnosischain/configs#31

@rolfyone
Copy link
Contributor Author

LGTM. Added some minor suggestions according to recent changes in gnosischain/configs#31

oh awesome - that makes sense then... thanks!

rolfyone and others added 4 commits January 19, 2024 07:40
…/configs/chiado.yaml

Co-authored-by: 4rgon4ut <59182467+4rgon4ut@users.noreply.github.com>
…/configs/chiado.yaml

Co-authored-by: 4rgon4ut <59182467+4rgon4ut@users.noreply.github.com>
…/configs/chiado.yaml

Co-authored-by: 4rgon4ut <59182467+4rgon4ut@users.noreply.github.com>
@rolfyone rolfyone merged commit 1f65619 into Consensys:master Jan 18, 2024
15 checks passed
@rolfyone rolfyone deleted the 7871 branch January 18, 2024 22:06
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.

Schedule Chiado testnet Deneb fork (expected on Jan, 31)
5 participants