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: resolve scheme from a read-write database #28313

Merged
merged 4 commits into from
Oct 11, 2023

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Oct 11, 2023

This pull request provides a quick solution for fixing issue #28307

This issue can occur when unclean shutdown happens and freezer tables need to be repair. In database read-only mode, the freezer will only be validated but not repair and terminates the node abnormally.

This pull request moves the scheme check into the Ethereum constructor where the read-write database is already initialized. This approach can avoid opening read-only database in very early stage, to prevent this error totally.

@rjl493456442 rjl493456442 marked this pull request as ready for review October 11, 2023 05:42
@@ -64,7 +63,6 @@ var Defaults = Config{
TxLookupLimit: 2350000,
TransactionHistory: 2350000,
StateHistory: params.FullImmutabilityThreshold,
StateScheme: rawdb.HashScheme,
Copy link
Member Author

Choose a reason for hiding this comment

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

Note, it's necessary to remove the default value, otherwise hash mode is always picked if user doesn't specify via cli flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we could leave it, if we do something like this

func ParseStateScheme(provided, default string, disk ethdb.Database) (string, error) {

and call it via

provided = ""
if ctx.IsSet(StateSchemeFlag.Name){ 
  provided = ctx.String(StateSchemeFlag.Name) 
}
scheme, err := rawdb.ParseStateScheme(provided, ctx.String(StateSchemeFlag.Name), disk)

I don't know if it's worth it though

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, perhaps it's better to not have HashScheme set in configs unless teh user explicitly does so. Otherwise someone who doesn't care and just did a dumpconfig will forever be stuck on Hash since it will keep forcing it. Otherwise if they don't care and dumpconfig omits it, they would switch to path when the default flips too.

Copy link
Member

Choose a reason for hiding this comment

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

I mean when they resync. I can imagine people keeping configs around longer than their chain.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Looks good to me (caveat: haven't tested it)

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

SGTM

@karalabe karalabe added this to the 1.13.3 milestone Oct 11, 2023
@karalabe karalabe merged commit eeb5dc3 into ethereum:master Oct 11, 2023
1 check passed
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 12, 2023
* cmd, core: resolve scheme from a read-write database

* cmd, core, eth: move the scheme check in the ethereum constructor

* cmd/geth: dump should in ro mode

* cmd: reverts
tyler-smith pushed a commit to blocknative/go-ethereum that referenced this pull request Oct 16, 2023
* cmd, core: resolve scheme from a read-write database

* cmd, core, eth: move the scheme check in the ethereum constructor

* cmd/geth: dump should in ro mode

* cmd: reverts
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
* cmd, core: resolve scheme from a read-write database

* cmd, core, eth: move the scheme check in the ethereum constructor

* cmd/geth: dump should in ro mode

* cmd: reverts
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request Jan 31, 2024
* cmd, core: resolve scheme from a read-write database

* cmd, core, eth: move the scheme check in the ethereum constructor

* cmd/geth: dump should in ro mode

* cmd: reverts
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