-
Couldn't load subscription status.
- Fork 1
Use ChainConfig for stateless block validation #20
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
Use ChainConfig for stateless block validation #20
Conversation
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
|
Your PR title doesn't follow the Conventional Commit guidelines. Example of valid titles:
Usage:
Breaking Changes Breaking changes are noted by using an exclamation mark. For example:
Help For more information, follow the guidelines here: https://www.conventionalcommits.org/en/v1.0.0/ |
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
| for (fork, params) in bf_params.iter().rev() { | ||
| if self.hardforks.is_fork_active_at_timestamp(fork.clone(), timestamp) { | ||
| return *params | ||
| return *params; |
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.
Did some fighting to avoid these, but I'm not entirely sure what's the right vscode config for the Reth repo.
| chain: Option<Chain>, | ||
| genesis: Option<Genesis>, | ||
| hardforks: ChainHardforks, | ||
| fill_genesis_config: bool, |
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.
For the ChainSpecBuilder, I added an option fill_genesis_config(enable:bool) to make it optional. I think this might be the best take, since if you build from a correctly crafted genesis (e.g., Hoodi genesis), then you don't need to fill the chain config.
| chain: Some(MAINNET.chain), | ||
| genesis: Some(MAINNET.genesis.clone()), | ||
| hardforks: MAINNET.hardforks.clone(), | ||
| fill_genesis_config: true, |
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.
If you're starting from mainnet() then by default I activate the fill_genesis_config flag.
| self | ||
| } | ||
|
|
||
| /// Enable Dao at genesis. |
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.
You will see I added the DAO and *Glacier forks that were missing.
I realized this when looking at the genesis chainconfigs that fill_chainconfig created, and noticed for these forks it always had mainnet block number instead of 0.
See here for an ordered set of forks. For example, the ChainSpecBuilder generated a chain spec with both Berlin and Istanbul activated at block 0, but MuirGlacier not activated. If you build a ChainSpecBuilder from mainnet(), that means that the underlying ChainConfig would be left with the MuirGlacier original block activation number instead of zero.
I guess this was OK for the testing crate since these are very old forks, and probably there aren't tests reg difficulty bombs delays or similar. I could have avoided adding these forks, but the underlying ChainConfig we would see generated for EEST blocks would have non-zero block number activations for them which looked very weird and seem buggy. This fixes it.
| if self.fill_genesis_config { | ||
| fill_chainconfig(&mut genesis.config, &self.hardforks, self.chain, None); | ||
| } |
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.
If we were asked to "fill" the genesis block chainconfig, we do so.
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.
BTW, note that None as the last argument (i.e., deposit contract address).
I'm honoring what is being done in L1024 below, but I feel this is somewhat wrong.
I'm not entirely sure why EEST tests don't seem to care about this, but I think some days/weeks ago I discovered that even if this is not set, some alloy-crate used mainnet contract address if it was needed and not provided (so I think it is interpreted as an "override" field).
In any case, I'm just accepting what they did before and avoid opinionating more about what I think might be better.
| chain: Some(value.chain), | ||
| genesis: Some(value.genesis.clone()), | ||
| hardforks: value.hardforks.clone(), | ||
| fill_genesis_config: false, |
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 think in this case setting this to false is the most conservative thing to do (i.e., how it worked before). The user can always call fill_genesis_config(true) if wanted.
| pub mod chain_spec; | ||
| /// ForkSpec module | ||
| /// This is needed because ChainSpec is not serializable (neither is genesis) | ||
| /// | ||
| /// Note: There is an exact copy of ForkSpec in `ef-tests` but since ef-tests is not no_std | ||
| /// we cannot pull that in since we need ForkSpec in the guest program. | ||
| /// | ||
| /// We convert the ef-tests version of ForkSpec in the host into the one located in here. | ||
| /// | ||
| /// When we parse execution spec tests, we get back a ForkSpec, that we then pass into | ||
| /// the guest program and convert it into a ChainSpec. If someone is using Hoodi/Mainnet | ||
| /// etc, then this may not be needed, as you can just do ChainSpec::mainnet() in the guest program | ||
| pub mod fork_spec; |
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.
Nuked crates.
| .build() | ||
| } | ||
| } | ||
| .build(); |
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.
Note that we didn't need to call .fill_genesis_config(true) new option since in L317 we're building from mainnet() which activate this by default.
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.
LGTM
0880801
into
kw/zkevm-benchmark-workload-repo
In this PR:
ChainSpecstateless variant.fill_chainconfighelper which fills aChainConfigfromHardforks+ extra chain info. This is necessary since the Reth JSON files for mainnet do not have the chain config field.MAINNETconstruction, usefill_chainconfigso the underlyinggenesis.chain_configis properly set.ChainSpecBuilder, provide an option to fill the genesis config using the helper.ChainSpecBuilder. These missing forks were Glacier-like forks (i.e., only bumping the difficulty bomb) and the DAO fork. More about this in comments.I can provide some examples of how this works from the
zkevm-benchmark-workloadside (e.g., examples of JSON serialized chain configs), but I'll leave that to the upcoming PR I'll create there.Supersede #13