Skip to content

Conversation

@0xmrree
Copy link

@0xmrree 0xmrree commented Dec 18, 2025

Issue Addressed

Missing values in /eth/v1/config/spec #8571
- there will be follow up PR for the re org props

Proposed Changes

  1. As per above issue from EF dev ops, I added
"EPOCHS_PER_SUBNET_SUBSCRIPTION": "256",
  "ATTESTATION_SUBNET_COUNT": "64",
  "ATTESTATION_SUBNET_EXTRA_BITS": "0",
  "UPDATE_TIMEOUT": "8192",
  "DOMAIN_BLS_TO_EXECUTION_CHANGE": "0x0a000000"

to /eth/v1/config/spec
2. Had to change the minimal config for UPDATE_TIMEOUT to get currents tests to pass. This is ok given UPDATE_TIMEOUT is not used in lighthouse as this config for light client spec from altair
3. ATTESTATION_SUBNET_PREFIX_BITS is now dynamically calculated and shimmed into the /eth/v1/config/spec output as advised by @michaelsproul

Testing

  1. ran cargo nextest run -p types and passed all tests, see below
...
        PASS [   0.006s] types withdrawal::withdrawal_request::tests::test_ssz_round_trip
        PASS [   0.007s] types withdrawal::withdrawal_request::tests::test_tree_hash_root
        PASS [   0.010s] types::committee_cache default_values
        PASS [   5.431s] types::committee_cache initializes_with_the_right_epoch
        PASS [   5.489s] types::committee_cache fails_without_validators
        PASS [   5.534s] types::state cache_initialization
        PASS [   5.617s] types::state beacon_proposer_index
        PASS [   0.238s] types::state decode_base_and_altair
        PASS [   5.740s] types::committee_cache min_randao_epoch_correct
        PASS [   6.236s] types::committee_cache shuffles_for_the_right_epoch
        PASS [  19.782s] types::state committees::current_epoch_committee_consistency
        PASS [  19.816s] types::state committees::next_epoch_committee_consistency
        PASS [  19.540s] types::state committees::previous_epoch_committee_consistency
────────────
     Summary [  25.263s] 265 tests run: 265 passed, 0 skipped
  1. Started beacon node via ./target/release/lighthouse bn --network sepolia ... and ran
curl http://localhost:5052/eth/v1/config/spec | jq '.data | {
  EPOCHS_PER_SUBNET_SUBSCRIPTION,
  ATTESTATION_SUBNET_COUNT,
  ATTESTATION_SUBNET_EXTRA_BITS,
  ATTESTATION_SUBNET_PREFIX_BITS,
  UPDATE_TIMEOUT,
  DOMAIN_BLS_TO_EXECUTION_CHANGE
}'

which returned

{
  "EPOCHS_PER_SUBNET_SUBSCRIPTION": "256",
  "ATTESTATION_SUBNET_COUNT": "64",
  "ATTESTATION_SUBNET_EXTRA_BITS": "0",
  "ATTESTATION_SUBNET_PREFIX_BITS": "6",
  "UPDATE_TIMEOUT": "8192",
  "DOMAIN_BLS_TO_EXECUTION_CHANGE": "0x0a000000"
}

@0xmrree 0xmrree changed the base branch from stable to unstable December 18, 2025 05:03
@michaelsproul
Copy link
Member

You'll need to rebase on unstable to get CI to pass

@michaelsproul
Copy link
Member

I'll be OOO until Jan 12 but someone might be able to review

@chong-he chong-he added the ready-for-review The code is ready for review label Dec 22, 2025
@mergify
Copy link

mergify bot commented Dec 22, 2025

Some required checks have failed. Could you please take a look @0xmrree? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Dec 22, 2025
Copy link
Member

@chong-he chong-he left a comment

Choose a reason for hiding this comment

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

I have tested the beacon API output and the fields added in this PR are now displayed correctly as the output for mainnet.

Some comments below. Some CIs are failing too, so would be great if you can fix them, thanks.

@0xmrree 0xmrree force-pushed the update_chainspec_p2p_and_etc branch from f8b6209 to 618615c Compare December 23, 2025 06:49
@0xmrree
Copy link
Author

0xmrree commented Dec 23, 2025

@chong-he addressed the above feedback, ran cargo fmt --all, reran cargo nextest run -p types, and re tested api. the ci failures should be gone now that i have added default values for the added config fields. also the code is much simpler as i am not longer changing any values in the config anymore.

@0xmrree 0xmrree requested a review from chong-he December 23, 2025 22:28
Copy link
Member

@chong-he chong-he left a comment

Choose a reason for hiding this comment

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

This is looking much better, just a bit of minor comments and something that I ain't sure about - will need a dev look into this

Thanks!

inactivity_score_bias: 4,
inactivity_score_recovery_rate: 16,
min_sync_committee_participants: 1,
update_timeout: 8192,
Copy link
Member

Choose a reason for hiding this comment

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

This is for the gnosis config. I don't see it in the gnosis config file: https://github.com/gnosischain/configs/blob/main/mainnet/config.yaml so I think we don't have to put this field for the gnosis presets

Copy link
Author

Choose a reason for hiding this comment

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

inactivity_penalty_quotient_altair is not in the gnosis config but yet you still have https://github.com/sigp/lighthouse/blob/unstable/consensus/types/src/core/chain_spec.rs#L1419. Am I missing something? If the attention was so strict to keep unused values out between two cases then there should of been separate structs IMO

Copy link
Member

Choose a reason for hiding this comment

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

That's right. I think we can keep it then.

inactivity_score_bias: 4,
inactivity_score_recovery_rate: 16,
min_sync_committee_participants: 1,
update_timeout: 8192,
Copy link
Member

Choose a reason for hiding this comment

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

Actually I am not sure if we should put update_timeout here, because this belongs to the presets (which I have seen you put in consensus/types/src/core/preset.rs, which is the right place to put).

I see other presets such as INACTIVITY_PENALTY_QUOTIENT_ALTAIR is being put here in the spec, but there are also quite some other presets are not here too. So I am unsure about this. We can leave it here now and wait for a developer's comment on this

Copy link
Author

Choose a reason for hiding this comment

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

My understanding is that those altair presets are in the chain spec but they don't come from them, instead there is just a test that reads the altair presets then asserts using from_chain_spec method in the altair preset class to make sure they align with. I followed same pattern as inactivity_penalty_quotient_altair. That being said that var is for light client anyway so it wont be used ...

Copy link
Member

Choose a reason for hiding this comment

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

yeah having a closer look, chain_spec.rs is the source of truth for the config values (including presets), and also see this in the comment:

/// Contains a mixture of "preset" and "config" values w.r.t to the EF definitions.

so I think we are good

@0xmrree 0xmrree force-pushed the update_chainspec_p2p_and_etc branch from 87f79c9 to d79505b Compare December 24, 2025 04:05
@0xmrree
Copy link
Author

0xmrree commented Dec 24, 2025

addressed feedback and ready for another review

@0xmrree 0xmrree requested a review from chong-he December 24, 2025 04:08
@chong-he
Copy link
Member

addressed feedback and ready for another review

This now looks great to me, the beacon API query for /eth/v1/config/spec now contains the added 5 fields, thanks for the PR.

As this involves spec change, we will wait for a final review from the team in case I miss anything.

@chong-he chong-he added spec_change A change related to the Eth2 spec ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Dec 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

HTTP-API ready-for-review The code is ready for review spec_change A change related to the Eth2 spec

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants