Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Separation of Constant, Preset and Configuration variables #2390
Changes from 25 commits
b9ffd8f
7689ebb
7d59016
4941a97
c1f1adb
8148c42
e4593d2
2c7a684
79d0fa0
d0fef3b
cd49470
9030270
ef8d600
6f68913
ccc6679
d3bf218
1e7c5b1
0894125
e8b0c46
f5c647b
a57ff5f
fb82472
925f050
90c4a75
48e1ef1
291168e
ff021da
46bb827
76cc964
c080324
6193c7c
1636a1d
e50d8d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
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?