-
Notifications
You must be signed in to change notification settings - Fork 998
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
Separation of Constant, Preset and Configuration variables #2390
Conversation
Just a thought, but is minimal so different from mainnet that it has significant value? Could minimal be discarded entirely without significant impact on testing? The main reductions in minimal compared to mainnet seem to be bulk and periodicity. Bulk relates to the number of validators and related items (committees etc.) and it doesn't seem that mainnet parameters are unreasonable for testing. Periodicity relates to the frequency of changes (slots per epoch, epochs per voting period etc.) and could be more important for transition testing, but are tests in these areas common enough to warrant the complexity of a second config? Note that I haven't done much testing that has required switching to minimal, so it may be that there are very good reasons for having it. Just thought that as the issue is being looked at it's worth asking the question. |
For spec-testing, yes it is of significant value. But outside of spec-testing, not really. This PR tries to address that: we enable deeper customization during compile-time for testing purposes, but remove the clutter and client-inconsistencies from the standard config. The problem with the mainnet configuration is that it increases the state size by a lot, and slows down things by a lot. Bulk and Periodicity indeed. Running days worth of mainnet-state-transitions in CI takes ages, especially in the unoptimized python spec version. The minimal configuration allows us to run through every spec test, on every change to the specs, catching a lot of issues before they hit implementers.
Slight disagree here. We do have some mainnet testing (it's a simple
They may not be common, but I think they are important enough to warrant it. And 1 compile-time variant with different constants is not unreasonable complexity to me. This PR attempts to make the separation with runtime configuration clear, to reduce any complexity leak there. |
Don't think we should include this in the next release yet. I am happy with the new config approach, but the pyspec part should improve more. The goal is to decorate test cases with fork versions / fork boundaries, and although it enables that, I'm not happy with the spec reloading part. |
38d98fc
to
9030270
Compare
Rebased onto latest The current plan is to update the pyspec builder to output a
And, after the spec clarifies the (relatively small) subset of runtime configuration, we can specify runtime-overrides per test case, to properly test different config values, and ease the fork scheduling problems (the stub values for altair fork version should be overridden in block processing tests, so Altair is actually active) |
configs/mainnet_config.yaml
Outdated
# Altair | ||
ALTAIR_FORK_VERSION: 0x01000000 | ||
ALTAIR_FORK_EPOCH: 18446744073709551615 | ||
# Merge | ||
MERGE_FORK_VERSION: 0x02000000 | ||
MERGE_FORK_EPOCH: 18446744073709551615 | ||
# Sharding | ||
SHARDING_FORK_VERSION: 0x03000000 | ||
SHARDING_FORK_EPOCH: 18446744073709551615 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up #2323 (review)
To distinguish between the "configs for test vectors" context and "configs for the network" context, it would be nice to make *_FORK_EPOCH
live in a separate file.
What do you think about having a mainnet_fork.yaml
which is only for *_FORK_EPOCH
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait till further refactoring. I'm updating the test cases etc. to support "overrides" first: just write which piece of runtime-configuration is different, per test case. We can make fork testing much easier this way. And I honestly like the single config file better: if it's not too large, and runtime-only, it's just a lot more ergonomic for testnet configuration work.
62a29c3
to
e8b0c46
Compare
…r config override functionality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! Really ripples deep in there...
Generally okay with the approach. Need to make sure it's easy for client teams to digest wht actually changed here maybe with a cleaned up header comment. The diff isn't particularly useful to them.
I had a few questions about config vs preset on some vars. Check it out. Happy to debate
configs/mainnet_config.yaml
Outdated
# 2**8 (= 256) epochs ~27 hours | ||
MIN_VALIDATOR_WITHDRAWABILITY_DELAY: 256 | ||
# 2**8 (= 256) epochs ~27 hours | ||
SHARD_COMMITTEE_PERIOD: 256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit surprised SHARD_COMMITTEE_PERIOD
made it in the network config values rather than in a preset. Can you explain your reasoning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in phase0 for the validator exit queue, so if a testnet wants to allow quick validator exits, it needs to modify this. I agree that for sharding itself it doesn't make a whole lot of sense to not put it in the preset. Maybe we should split the two config variables? I.e. define VALIDATOR_EXIT_DELAY
, and update phase0 without changing the value?
| `DOMAIN_CONTRIBUTION_AND_PROOF` | `DomainType('0x09000000')` | | ||
| Name | Value | Description | | ||
| - | - | - | | ||
| `INACTIVITY_SCORE_BIAS` | `uint64(4)` | score points per inactive epoch | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain your choice of making these values configurable?
They seem somewhat similar to the penalty values above that are in presets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make the phase0 variants of inactivity vars configurable, but I imagine we'll have "duplicate" config vars then, and they won't matter for testnets that start post-altair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix markdown header
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts, not strongly:
- 👍 on more structural pyspec
- 👍 on making test case variable configurable by decorator. Nice work!
- [❓] A little concerned about maintainability.
- Variables might be moved between config and preset later when we need.
configs/mainnet.yaml
will contain the configuration cross forks, including the ones in active R&D. IIRC, the client devs don’t like unstable config file that includes variables for the future forks.- 👎 on some variables are
spec.VARIABLE_NAME
and some arespec.config.VARIABLE_NAME
in tests. But maybe I will get used to it.
- [❓ ] The specs were written for the client devs mostly, but it also serves general readers to understand Eth2 protocol. This change exposes more implementation details by separating config and preset.
I'd defer to client devs to make the call. 😄
Diff of test generation with
|
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a lot in here but generally supportive of the change!
ultimately defer to various client teams on how to handle this but do like differentiating the more/less statics parts of the "config"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of clearly defining the config file semantics, and I think this PR does a good job of that.
I skimmed through Lighthouse's compile-time config (EthSpec
) and runtime config (ChainSpec
) to see how it matched up and didn't spot any major issues. There are just a few consts/preset-level values in the ChainSpec, which I've documented below.
I don't think we have the appetite for a large refactor at the moment, so if this PR is adopted we'll likely continue mapping standard configs onto Lighthouse's internal representation, and then slowly migrate towards using the standard internally.
Other minor points:
- There's no central definition of constants, which could be useful to have
- I agree with @hwwhww that having clarity about what forks are being parsed from a config file is desirable. There aren't any Altair/merge/sharding params in the config right now, would the intention be to mix them in with the phase0 params, or have them in separate files like the presets?
- Backwards-compatibility for the
/eth/v1/config/spec
HTTP API is also something to consider. Right now Lighthouse & Teku interoperate OK using the current config. If this PR is adopted I think the best chance of achieving backwards compatibility will be to require the API endpoint to returning the union of the preset & config?
ChainSpec differences
Consts in ChainSpec:
- genesis_slot
- far_future_epoch
- base_rewards_per_epoch
- deposit_contract_tree_depth
- min_per_epoch_churn_limit
- churn_limit_quotient
- bls_withdrawal_prefix_byte
- all the domains
Preset params in ChainSpec:
- max_committees_per_slot
- target_committee_size
- shuffle_round_count
- hysteresis_quotient
- hysteresis_downward_multiplier
- hysteresis_upward_multiplier
- proportional_slashing_multiplier
- min_deposit_amount
- max_effective_balance
- ejection_balance
- effective_balance_increment
- min_attestation_inclusion_delay
- min_seed_lookahead
- max_seed_lookahead
- min_epochs_to_inactivity_penalty
- base_reward_factor
- whistleblower_reward_quotient
- proposer_reward_quotient
- inactivity_penalty_quotient
- min_slashing_penalty_quotient
- safe_slots_to_update_justified
Definitely makes sense to me - being able to setup custom testnets more easily is definitely a win. This doesn't really make any difference for Teku as all the config options from the spec are completely dynamic for us anyway (ie minimal and mainnet are both fully supported in all builds, and we actually have a couple of other variants we use to make it possible for acceptance tests to finalise quickly). So the change for us ultimately just boils down to loading the values from more files and supporting the "PRESET_BASE" field to specify which preset file to mix in. @michaelsproul makes a good point about |
While the preset vs config distinction could be formulated at the implementation-level, rather than spec-level, I think including this in the spec gives more clarity about how much certain params are expected to change and will help with more easily (and consistently) setting up testnets across clients. I think this will be a big win for Lodestar, which currently has teku-level of dynamic behavior. We can likely simplify a lot of our code and improve devex once our ssz types are known ahead of time and no longer depend on run-time config. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
putting it in alpha.6. great work!
Feedback welcome. The idea is to make customization more consistent and reliable between clients, without any breaking changes.
Issues
Currently, as someone who configures many local tests, testing and testnets, I face the following issues:
And as an implementer of the Spec, I face the following issues:
Current client behavior
mainnet
andminimal
in the same buildCONFIG_NAME
(recently removed for other reasons) to detect the base configmainnet
andminimal
SSZ valuesNew proposal
Without changing the meaning of any of the configuration variables, we organize them in 3 types of variables:
Constants
We retain all existing constants, and freeze some additional configuration:
DOMAIN_...
variables: signing is already unique thanks to the fork-digest part. Various external tooling/smart-contracts also need stability here.Preset
A preset is defined as a subset of the previous configuration, bundled together statically, intended for different operation modes:
mainnet
: for full mainnet-like operation,minimal
: faster/simplified local testing, etc.A preset:
mainnet
andminimal
PRESET_BASE
as user. The implementation can then simply check if this preset is allowed.BeaconState
orBeaconBlock
types, but still need to be configurable for testing reasons.Configuration
A configuration is a set of runtime-configurable variables that enable users to set up custom local/test networks.
A configuration specifies a preset with
PRESET_BASE
. This can be eithermainnet
,minimal
, or optionally refer to a preset provided by the client implementation.For new testing of forks, we want to start defining this configuration per testcase. Testing the fork-boundaries is really important, and this enables us to do that, without re-compiling the client for every test case.
The configuration contains:
TODO
PRESET_BASE
Update the way configuration is loadedPyspec build outputs for each preseteth2spec
as python packageLater TODOs
Pyspec changes
mainnet.py
andminimal.py
instead of aspec.py
that needed reloading.FOO_BAR
toconfig.FOO_BAR
config
value of a spec module temporarily, to change the config efficiently.with_config_overrides
andspec_configured_state_test
can be used to configure individual spec tests