Ensure difficulty/hash/epoch overrides change the ChainSpec
#2798
Conversation
paulhauner
force-pushed
the
kintsugi-overrides
branch
from
754e632 to
4231be8
Compare
paulhauner
force-pushed
the
kintsugi-overrides
branch
from
2bf8505 to
78df587
Compare
There was a problem hiding this comment.
Nice! Just had a couple comments and nits.
General question:
Since we check for complete spec compatibility, both the BN and VC will need the override flags to function. It seems like it'd be very difficult to actually socially coordinate this with things like Infura and BN fallbacks. Is there away to go about this without having strict BN <> VC compatibility on these fields?
consensus/types/src/chain_spec.rs
Outdated
| #[serde(with = "eth2_serde_utils::quoted_u256")] | ||
| pub terminal_total_difficulty: Uint256, | ||
| pub terminal_block_hash: Hash256, | ||
| #[serde(with = "eth2_serde_utils::quoted_u64")] |
There was a problem hiding this comment.
Epoch uses quoted_u64 by default, so this shouldn't be necessary
| Incorrect use of this flag will cause your node to experience a consensus | ||
| failure. Be extremely careful with this flag.") | ||
| .takes_value(true) | ||
| .global(true) |
There was a problem hiding this comment.
This is going to make these flags available in lighthouse account_manager which is a little strange. But I think this is fixable once we upgrade to clap v3 (v3 is still in beta but I implemented the upgrade here: #2808)
Conversation about why it's not yet fixable:
clap-rs/clap#1183
There was a problem hiding this comment.
Yeah, it's indeed a bit unfortunate. However, I think I'm OK with that. The ability to ensure a consistent spec across all components is worth the occasional weirdness. With this setup, at least the account_manager has access to a correct spec, if it turns out it needs it later.
lighthouse/src/main.rs
Outdated
| .long("terminal-block-hash-override") | ||
| .value_name("TERMINAL_BLOCK_HASH") | ||
| .help("Used to coordinate manual overrides to the TERMINAL_BLOCK_HASH parameter. \ | ||
| Accepts a 256-bit decimal integer (not a hex value). \ |
There was a problem hiding this comment.
Seems like this part of the description should be in the terminal-total-difficulty-override description?
This is a good question. There was some discussion on the merge tech chat earlier this week. It seemed to get a consensus from clients that we should change this value. These values are a last resort and they're critical to consensus, since setting them wrong can lead to chain splits. So, if we do need to use them then it's going to be a massive coordination effort anyway. Notably, the LH VC only warns if the spec doesn't match, it doesn't error and exit anymore. So, I was thinking it's probably nice to have a little warning around if it turns out we need to do a big coordination effort on these. |
|
All comments addressed! Thanks for the review @realbigsean |
paulhauner
added
ready-for-review
* Unify loading of eth2_network_config * Apply overrides at lighthouse binary level * Remove duplicate override values * Add merge values to existing net configs * Make override flags global * Add merge fields to testing config * Add one to TTD * Fix failing engine tests * Fix test compile error * Remove TTD flags * Move get_eth2_network_config * Fix warn * Address review comments
* Unify loading of eth2_network_config * Apply overrides at lighthouse binary level * Remove duplicate override values * Add merge values to existing net configs * Make override flags global * Add merge fields to testing config * Add one to TTD * Fix failing engine tests * Fix test compile error * Remove TTD flags * Move get_eth2_network_config * Fix warn * Address review comments
* Unify loading of eth2_network_config * Apply overrides at lighthouse binary level * Remove duplicate override values * Add merge values to existing net configs * Make override flags global * Add merge fields to testing config * Add one to TTD * Fix failing engine tests * Fix test compile error * Remove TTD flags * Move get_eth2_network_config * Fix warn * Address review comments
Issue Addressed
NA
Proposed Changes
Ensures the difficulty/hash/epoch overrides change the
ChainSpec. This will result in changes to the/eth/v1/config/specwhen the overrides are used, I think this is the correct behaviour (@ajsutton agrees, at least).Additional Info
NA