Skip to content
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

cmd, core, miner: rework genesis setup #30907

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rjl493456442
Copy link
Member

TODO

  • Add waiter if the verkle fork is scheduled with a future time

@fjl
Copy link
Contributor

fjl commented Dec 13, 2024

I think we should extract the chain config setup into its own function like I did in my branch: https://github.com/fjl/go-ethereum/tree/genesis-verkle-config

Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Left a few questions / suggestions but LGTM overall.

return &triedb.Config{
HashDB: hashdb.Defaults,
Preimages: recordPreimage,
IsVerkle: isVerkle,
Copy link
Member

Choose a reason for hiding this comment

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

maybe set it to false here, or just omit it, it looks a bit weird given what happens on L276

}
}
// Get the existing chain configuration.
// Construct the new chain config either from the supplied genesis, or
// the stored chain config if nothing specified.
newcfg := genesis.configOrDefault(stored)
Copy link
Member

Choose a reason for hiding this comment

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

we need a better name now, because it's no longer returning any "default". Maybe getUpdatedConfig ?

@@ -525,6 +531,11 @@ func (c *ChainConfig) IsVerkle(num *big.Int, time uint64) bool {
return c.IsLondon(num) && isTimestampForked(c.VerkleTime, time)
}

// IsVerkleGenesis returns whether the verkle fork is activated at genesis block.
func (c *ChainConfig) IsVerkleGenesis() bool {
return c.IsLondon(common.Big0) && c.VerkleTime != nil && c.EnableVerkleAtGenesis
Copy link
Member

Choose a reason for hiding this comment

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

but if I EnableVerkleAtGenesis is set, does VerkleTime matter at all?

@@ -2224,7 +2224,6 @@ func MakeTrieDatabase(ctx *cli.Context, disk ethdb.Database, preimage bool, read
// ignore the parameter silently. TODO(rjl493456442)
// please config it if read mode is implemented.
config.HashDB = hashdb.Defaults
return triedb.NewDatabase(disk, config)
Copy link
Member

Choose a reason for hiding this comment

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

that means that both config.HashDB and config.PathDB are set, and this is leading to a log.Crit in triedb.NewDatabase. Was this intentional ?

chainConfig, genesisHash, genesisErr := SetupGenesisBlockWithOverride(db, triedb, genesis, overrides)
if _, ok := genesisErr.(*params.ConfigCompatError); genesisErr != nil && !ok {
return nil, genesisErr
chainConfig, genesisHash, compactErr, err := SetupGenesisBlockWithOverride(db, cacheConfig.StateScheme, cacheConfig.Preimages, genesis, overrides)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
chainConfig, genesisHash, compactErr, err := SetupGenesisBlockWithOverride(db, cacheConfig.StateScheme, cacheConfig.Preimages, genesis, overrides)
chainConfig, genesisHash, compatErr, err := SetupGenesisBlockWithOverride(db, cacheConfig.StateScheme, cacheConfig.Preimages, genesis, overrides)

Comment on lines +456 to +459
if compactErr != nil {
log.Warn("Rewinding chain to upgrade configuration", "err", compactErr)
if compactErr.RewindToTime > 0 {
bc.SetHeadWithTimestamp(compactErr.RewindToTime)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if compactErr != nil {
log.Warn("Rewinding chain to upgrade configuration", "err", compactErr)
if compactErr.RewindToTime > 0 {
bc.SetHeadWithTimestamp(compactErr.RewindToTime)
if compatErr != nil {
log.Warn("Rewinding chain to upgrade configuration", "err", compatErr)
if compactErr.RewindToTime > 0 {
bc.SetHeadWithTimestamp(compatErr.RewindToTime)

} else {
bc.SetHead(compat.RewindToBlock)
bc.SetHead(compactErr.RewindToBlock)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bc.SetHead(compactErr.RewindToBlock)
bc.SetHead(compatErr.RewindToBlock)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants