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

core, cmd, trie: fix the condition of pathdb initialization #28718

Merged
merged 6 commits into from
Dec 21, 2023

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Dec 21, 2023

Alternative to #28715


Fix for #28713

Original problem was caused by #28595 . In this PR, we made it so that as soon as we start to sync, we delete the root of the disk layer. That is not wrong per se, but, another part of the code uses the "presence of the root" as an init-check for the pathdb. And, since the init-check now failed, it tried to initialize it, which failed since a sync was already ongoing.

Impact: after a state-sync has begun, if the node is restarted, it will refuse to restart, with the error message: Fatal: Failed to register the Ethereum service: waiting for sync.

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.

Minor nits

cmd/geth/dbcmd.go Outdated Show resolved Hide resolved
trie/triedb/pathdb/database.go Outdated Show resolved Hide resolved
rjl493456442 and others added 4 commits December 21, 2023 21:46
Co-authored-by: Martin HS <martin@swende.se>
Co-authored-by: Martin HS <martin@swende.se>
return nil
}

// removeFolder deletes all files (not folders) inside the directory 'dir' (but not files in subfolders).
func removeFolder(dir string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a more apt name?

Suggested change
func removeFolder(dir string) {
func removeFilesInFolder(dir string) {

@holiman holiman added this to the 1.13.8 milestone Dec 21, 2023
@arcivanov
Copy link

Assuming this is accepted, can the fix be released ASAP?

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.

LGTM

@holiman holiman merged commit cca9479 into ethereum:master Dec 21, 2023
2 of 3 checks passed
arcivanov pushed a commit to arcivanov/go-ethereum that referenced this pull request Dec 22, 2023
…#28718)

Original problem was caused by ethereum#28595, where we made it so that as soon as we start to sync, the root of the disk layer is deleted. That is not wrong per se, but another part of the code uses the "presence of the root" as an init-check for the pathdb. And, since the init-check now failed, the code tried to re-initialize it which failed since a sync was already ongoing.

The total impact being: after a state-sync has begun, if the node for some reason is is shut down, it will refuse to start up again, with the error message: `Fatal: Failed to register the Ethereum service: waiting for sync.`. 

This change also modifies how `geth removedb` works, so that the user is prompted for two things: `state data` and `ancient chain`. The former includes both the chaindb aswell as any state history stored in ancients. 

---------

Co-authored-by: Martin HS <martin@swende.se>
Doozers pushed a commit to kilnfi/pgeth that referenced this pull request Dec 22, 2023
…#28718)

Original problem was caused by ethereum#28595, where we made it so that as soon as we start to sync, the root of the disk layer is deleted. That is not wrong per se, but another part of the code uses the "presence of the root" as an init-check for the pathdb. And, since the init-check now failed, the code tried to re-initialize it which failed since a sync was already ongoing.

The total impact being: after a state-sync has begun, if the node for some reason is is shut down, it will refuse to start up again, with the error message: `Fatal: Failed to register the Ethereum service: waiting for sync.`. 

This change also modifies how `geth removedb` works, so that the user is prompted for two things: `state data` and `ancient chain`. The former includes both the chaindb aswell as any state history stored in ancients. 

---------

Co-authored-by: Martin HS <martin@swende.se>
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
…#28718)

Original problem was caused by ethereum#28595, where we made it so that as soon as we start to sync, the root of the disk layer is deleted. That is not wrong per se, but another part of the code uses the "presence of the root" as an init-check for the pathdb. And, since the init-check now failed, the code tried to re-initialize it which failed since a sync was already ongoing.

The total impact being: after a state-sync has begun, if the node for some reason is is shut down, it will refuse to start up again, with the error message: `Fatal: Failed to register the Ethereum service: waiting for sync.`. 

This change also modifies how `geth removedb` works, so that the user is prompted for two things: `state data` and `ancient chain`. The former includes both the chaindb aswell as any state history stored in ancients. 

---------

Co-authored-by: Martin HS <martin@swende.se>
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