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/geth: add check func to validate state scheme #2067

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

sysvm
Copy link
Contributor

@sysvm sysvm commented Dec 14, 2023

Description

Provide explicit for which state scheme geth can use to start or init. And add check function to validate provided state scheme.

Rationale

Users may add StateScheme = "path" in config.toml and want to start geth without persistent state in disk, but geth is started in hash mode. For solving these cases, add a function, obey the following rules:

  1. If config isn't provided, write hash mode to config by default, so in current function, config is nonempty.
  2. If persistent state and cli is empty, use config param.
  3. If persistent state is empty, provide CLI flag and config, choose CLI to return.
  4. If persistent state is nonempty and CLI isn't provided, persistent state should be equal to config.
  5. If all three items are provided: if any two of the three are not equal, return error.

Example

  1. nohup ./geth --config config.toml --cache 8000 --rpc.allow-unprotected-txs --history.transactions 0 >> nohup.out 2>&1& and add StateScheme = "path" in config.toml, geth is started in path mode.

Changes

Notable changes:

  • N/A

@sysvm sysvm added the r4r ready for review label Dec 14, 2023
joeylichang
joeylichang previously approved these changes Dec 14, 2023
@joeylichang
Copy link
Contributor

LGTM

// there is no persistent state data in disk db(e.g. geth init)
if !ctx.IsSet(StateSchemeFlag.Name) {
log.Info("State scheme set by config", "scheme", stateSchemeCfg)
return stateSchemeCfg, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

need judge if the stateSchemeCfg is not setted

Copy link
Contributor Author

@sysvm sysvm Dec 14, 2023

Choose a reason for hiding this comment

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

stateSchemeCfg is impossible to be empty in ResolveStateScheme function, because there is a default value in caller function loadBaseConfig.

}
if !ctx.IsSet(StateSchemeFlag.Name) {
if stored != stateSchemeCfg {
return "", fmt.Errorf("incompatible state scheme, stored: %s, config: %s", stored, stateSchemeCfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

should not return error if stateSchemeCfg is empty

Copy link
Contributor Author

@sysvm sysvm Dec 14, 2023

Choose a reason for hiding this comment

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

ditto. stateSchemeCfg is impossible to be empty in ResolveStateScheme function.

flywukong
flywukong previously approved these changes Dec 15, 2023
Copy link

@RenRick RenRick left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@flywukong flywukong left a comment

Choose a reason for hiding this comment

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

LGTM

@sysvm sysvm merged commit 07c46ab into bnb-chain:develop Dec 18, 2023
5 checks passed
@sysvm sysvm deleted the fix-pbssconfig branch December 18, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r4r ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node can't read StateScheme entries in configuration file
4 participants