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.

Sign up for GitHub

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

Jump to bottom

config: use default fork config if not specified in config.toml #1654

Merged
merged 2 commits into from
Jun 13, 2023
Merged

config: use default fork config if not specified in config.toml #1654

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27c52b9
config: use default fork config if not specified in config.toml
brilliant-lx May 26, 2023
5c06505
test: use genesis.config for unknown genesis hash
brilliant-lx May 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 101 additions & 7 deletions core/genesis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,21 +246,115 @@ func SetupGenesisBlockWithOverride(db ethdb.Database, genesis *Genesis, override
return newcfg, stored, nil
}

// Hard fork block height specified in config.toml has higher priority, but
// if it is not specified in config.toml, use the default height in code.
func (g *Genesis) configOrDefault(ghash common.Hash) *params.ChainConfig {
var defaultConfig *params.ChainConfig
switch {
case g != nil:
return g.Config
case ghash == params.MainnetGenesisHash:
return params.MainnetChainConfig
defaultConfig = params.MainnetChainConfig
case ghash == params.BSCGenesisHash:
return params.BSCChainConfig
defaultConfig = params.BSCChainConfig
NathanBSC marked this conversation as resolved.
Show resolved Hide resolved
case ghash == params.ChapelGenesisHash:
return params.ChapelChainConfig
defaultConfig = params.ChapelChainConfig
case ghash == params.RialtoGenesisHash:
return params.RialtoChainConfig
defaultConfig = params.RialtoChainConfig
default:
return params.AllEthashProtocolChanges
defaultConfig = params.AllEthashProtocolChanges
}
if g == nil || g.Config == nil {
return defaultConfig
}

// if not set in config.toml, use the default value of each network
if g.Config.ChainID == nil {
g.Config.ChainID = defaultConfig.ChainID
}
if g.Config.HomesteadBlock == nil {
g.Config.HomesteadBlock = defaultConfig.HomesteadBlock
}
if g.Config.EIP150Block == nil {
g.Config.EIP150Block = defaultConfig.EIP150Block
}
if g.Config.EIP155Block == nil {
g.Config.EIP155Block = defaultConfig.EIP155Block
}
if g.Config.EIP158Block == nil {
g.Config.EIP158Block = defaultConfig.EIP158Block
}
if g.Config.ByzantiumBlock == nil {
g.Config.ByzantiumBlock = defaultConfig.ByzantiumBlock
Comment on lines +273 to +290
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think this is the right way to do it, there is something called reflect.VisibleFields in go. Where you could iterate through g.Config struct and in one loop add default value in case is nil. If you change this to use this functionality, then it would automatically update in case of new param in config and no additional change would be required

Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious how could we achieve the goal of automatically update? , it seems that by using reflect.VisibleFields, it still need to use 'if' to compare the fieldName and check its value, and then set the default value (BlockNumber) if nil.

Another minor comments is that reflection seems to be a little hard to maintain as it kind of wrap the type of the field when trying to access it's value, and also the rule of settable. which imbo becomes error-prone for developers when something new is added to ChainConfig.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can achieve that we do not need to add new value here in case of new HF block or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sunny2022da but maybe you are right about settable rule and this just complicates things :/

Copy link
Contributor

@Mister-EA Mister-EA May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@sunny2022da We can compare field names by converting them to string using reflection. Then for each field name of g.Config we look up the value of that field in defaultConfig using the string as a key. Then through reflection we can set the value of the field in g.Config.

Basically we need to create two intermediate maps that allow us to look up block names as keys to the maps.

This is actually similar to how geth maps RPC method names like eth_gasPrice to callbacks such as func GasPrice(..).

Copy link
Contributor

Choose a reason for hiding this comment

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

got you~ So it seems to me the logic is:

  • use reflect.visibleFields to get the list of all `*Block' fields
  • iterate every field = fields[i] , compare the fieldName and the name of HF block, such as HomesteadBlock , then get the value of the field.
  • check if field value is nil, if yes, assign to the defaultConfig value.

it seems to me that the logic is a little bit complicated then the one we currently have.

Moreover, I think the reason that geth use reflection for mapping RPC calls is because there's serialization/de-serialization involved in the RPC case, where reflection is a good choice to get the method name.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are more or less right. You can do it all in one for loop. You iterate over the fields of g.Config, and if the value is nil, you retrieve using the map structure the matching block in defaultConfig, and update the field's value.

If you use this map you don't need to add an if statement for each block name since the key of the map will find you the matching block from defaultConfig.

I think this logic is not that complicated, and even if the code ends up being complicated, the user experience and maintainability are much better as we wouldn't need to keep updating this code every time there is a new hard fork.

Copy link
Contributor

@sunny2022da sunny2022da May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Correct, we just need to maintain the map you mentioned in this way, adding new <k,v> pair when there is new HF added.
So the problem remain to whether we decide to use reflection here, Let hear others~

Thx for explanation!

Copy link
Contributor

Choose a reason for hiding this comment

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

So this PR should add default config values if not specified in config.toml. In this case why reinventing the wheel and not use a config package which is already tested?
ex 1: https://github.com/gookit/config
ex 2: https://pkg.go.dev/github.com/ardanlabs/conf/v3#hdr-Example_Usage

Copy link
Collaborator Author

@brilliant-lx brilliant-lx May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Thank for all your comments, I think the main point is whether to improve the code by reflect.
I would prefer the if/else logic, since:
1.could avoid the complexity of reflection
2.the extra KV map mentioned by @Mister-EA could bring big change to the current HF setup. e.g. the code in: params/config.go

}
if g.Config.ConstantinopleBlock == nil {
g.Config.ConstantinopleBlock = defaultConfig.ConstantinopleBlock
}
if g.Config.PetersburgBlock == nil {
g.Config.PetersburgBlock = defaultConfig.PetersburgBlock
}
if g.Config.IstanbulBlock == nil {
g.Config.IstanbulBlock = defaultConfig.IstanbulBlock
}
if g.Config.MuirGlacierBlock == nil {
g.Config.MuirGlacierBlock = defaultConfig.MuirGlacierBlock
}

// BSC dedicated start
if g.Config.RamanujanBlock == nil {
g.Config.RamanujanBlock = defaultConfig.RamanujanBlock
}
if g.Config.NielsBlock == nil {
g.Config.NielsBlock = defaultConfig.NielsBlock
}
if g.Config.MirrorSyncBlock == nil {
g.Config.MirrorSyncBlock = defaultConfig.MirrorSyncBlock
}
if g.Config.BrunoBlock == nil {
g.Config.BrunoBlock = defaultConfig.BrunoBlock
}
if g.Config.EulerBlock == nil {
g.Config.EulerBlock = defaultConfig.EulerBlock
}
if g.Config.NanoBlock == nil {
g.Config.NanoBlock = defaultConfig.NanoBlock
}
if g.Config.MoranBlock == nil {
g.Config.MoranBlock = defaultConfig.MoranBlock
}
if g.Config.GibbsBlock == nil {
g.Config.GibbsBlock = defaultConfig.GibbsBlock
}
if g.Config.PlanckBlock == nil {
g.Config.PlanckBlock = defaultConfig.PlanckBlock
}
if g.Config.LubanBlock == nil {
g.Config.LubanBlock = defaultConfig.LubanBlock
}
if g.Config.PlatoBlock == nil {
g.Config.PlatoBlock = defaultConfig.PlatoBlock
}
if g.Config.BerlinBlock == nil {
g.Config.BerlinBlock = defaultConfig.BerlinBlock
}
if g.Config.LondonBlock == nil {
g.Config.LondonBlock = defaultConfig.LondonBlock
}
if g.Config.HertzBlock == nil {
g.Config.HertzBlock = defaultConfig.HertzBlock
}

// BSC Parlia set up
if g.Config.Parlia == nil {
g.Config.Parlia = defaultConfig.Parlia
} else {
if g.Config.Parlia.Period == 0 {
g.Config.Parlia.Period = defaultConfig.Parlia.Period
}
if g.Config.Parlia.Epoch == 0 {
g.Config.Parlia.Epoch = defaultConfig.Parlia.Epoch
}
}

return g.Config
}

// ToBlock creates the genesis block and writes state of a genesis specification
Expand Down