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

trie/triedb/pathdb: fix initialization flaws #28715

Closed
wants to merge 1 commit into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 21, 2023

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.

@@ -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
Copy link
Contributor Author

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
Copy link
Member

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)

@holiman
Copy link
Contributor Author

holiman commented Dec 21, 2023

Closing in favour of #28718

@holiman holiman closed this Dec 21, 2023
@holiman holiman removed this from the 1.13.8 milestone Dec 22, 2023
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.

2 participants