-
Notifications
You must be signed in to change notification settings - Fork 20.1k
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
trie/triedb/pathdb: fix initialization flaws #28715
Conversation
@@ -576,8 +576,10 @@ func truncateFromHead(db ethdb.Batcher, freezer *rawdb.ResettableFreezer, nhead | |||
return 0, err | |||
} | |||
// Ensure that the truncation target falls within the specified range. | |||
if ohead < nhead || nhead < otail { | |||
return 0, fmt.Errorf("out of range, tail: %d, head: %d, target: %d", otail, ohead, nhead) | |||
if nhead != 0 { // truncating to zero is always possible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not a sufficient fix. The code further down:
// Load the meta objects in range [nhead+1, ohead]
blobs, err := rawdb.ReadStateHistoryMetaList(freezer, nhead+1, ohead-nhead)
if err != nil {
return 0, err
}
batch := db.NewBatch()
for _, blob := range blobs {
var m meta
if err := m.decode(blob); err != nil {
return 0, err
}
rawdb.DeleteStateID(batch, m.root)
}
The call to ReadStateHistoryMetaList
will read the entire chunk of data into memory, since it internally does
return db.AncientRange(stateHistoryMeta, start-1, count, 0)
using 0
as the maxbytes ( if maxBytes is not specified, 'count' items will be returned if they are present
).
Also, I'm not sure how this would behave if we try to load 5M, but 0-4M are already gone.
A better fix might be to something like this:
if nhead != 0 { // truncating to zero is always possible
ohead, err = freezer.TruncateHead(0)
if err != nil {
return 0, err
}
return int(ohead - nhead), nil
}
But if resetting to 0
, we should also set back the tail
to zero. Hm... @rjl493456442 thoughts?
@@ -576,8 +576,10 @@ func truncateFromHead(db ethdb.Batcher, freezer *rawdb.ResettableFreezer, nhead | |||
return 0, err | |||
} | |||
// Ensure that the truncation target falls within the specified range. | |||
if ohead < nhead || nhead < otail { | |||
return 0, fmt.Errorf("out of range, tail: %d, head: %d, target: %d", otail, ohead, nhead) | |||
if nhead != 0 { // truncating to zero is always possible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix is not correct.
The leftover state history can be in range of [tail, head].
The reset to 0 won't work because the state segment before tail is not longer accessible, but we try to load them here blobs, err := rawdb.ReadStateHistoryMetaList(freezer, nhead+1, ohead-nhead)
Closing in favour of #28718 |
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
.Fix courtesy of @rjl493456442 : we now extend the init-check, so that detect that "yes, it's already initalized, and a sync is ongoing".
There's an additional fix also, which I ran into when doing a guerilla partial-wipe by keeping the ancient folder but deleting chaindata. It would not allow pruning the state histories back to zero.