core/state/snapshot: clean up dangling storages in snapshot generation #24665
Conversation
|
Note, this PR is not tested on live network, should be verified first. |
|
It's verified on Mainnet and Goerli, no dangling storage detected though. |
I've read the code a bit, but it's still not quite clear to me what we actually scan. Will this (in total) scan the entire slot key-range, or will it only scan the portions of the keyspace that reside between account ranges? I suspect it's the former. |
|
It will only scan the portions of the keyspace that reside between account ranges. During the snapshot generation process, we will take a batch of accounts as the unit. It's same for dangling storage detector here. A single range detector is constructed for a batch of accounts(by default it's 128 accounts) and all the dangling storages will be detected out in a single database iteration. Why I say |
rjl493456442
force-pushed
the
fix-snap-gen-2
branch
from
8a7dd11 to
592d42f
Compare
|
Sorry, it seems I made this require a rebase? |
| ) | ||
|
|
||
| // DanglingRange describes the range for detecting dangling storages. | ||
| type DanglingRange struct { |
There was a problem hiding this comment.
| type DanglingRange struct { | |
| type DanglingRangeScanner struct { |
perhaps?
| defer iter.Release() | ||
|
|
||
| for iter.Next() { | ||
| account := iter.Key()[len(rawdb.SnapshotStoragePrefix) : len(rawdb.SnapshotStoragePrefix)+common.HashLength] |
There was a problem hiding this comment.
I know that the way you have written it is "more correct", but IMO it's easier to read on the form
account := iter.Key()[1:33] // trim 1-byte prefix, read 32-byte hash
Might be a minority opinion though ...
| if err := drange.cleanup(nil); err != nil { | ||
| return false, nil, err | ||
| } |
There was a problem hiding this comment.
I see that you do a cleanup, but I don't see where you actually ever invoke detect ?
There was a problem hiding this comment.
detect is invoked when the range is constructed.
Let's do it |
| if err := result.forEach(func(key []byte, val []byte) error { return onState(key, val, false, false) }); err != nil { | ||
| var drange *DanglingRange | ||
| if kind != "storage" { | ||
| drange = NewDanglingRange(dl.diskdb, origin, result.last(), false) |
There was a problem hiding this comment.
I wonder if result.last() can be wrong. Example:
The first range is 0x000 - 0x00F. It has the following :
0x001: an account + storage
0x002: an account + storage
0x003: an account + storage
0x00a: an account + storage
Then most of them go away, and we're left with only one account in this range:
0x001: account + storage
Won't result.last be 0x001, making it so we don't scan 0x001 - 0x00F ?
There was a problem hiding this comment.
Why do you say Then most of them go away? If we iterate out four accounts 0x001, 0x002, 0x003, 0x00a, then result.last will be 0x00a. It represents the account snapshot in range 0x001 - 0x00a is checked by generator. And the next range should start from 0x00b.
There was a problem hiding this comment.
Even for these four accounts, three of them are not in trie(dangling account snapshot data) which will be removed from the disk, the result.last should still be 0x00a.
|
Close because of #24811 |
This PR improves the snapshot generation, it will detect the dangling storages in snapshot generation.
Dangling storage means the storage snapshot data is existent while the corresponding account snapshot
data is missing. It's possible to happen in such scenario(text copied from Peter):
During snap sync, we pull in pieces of storage slots, but between two cycles, the contract might get deleted. In that case if the deletion happens in between an interrupt, I think, the old slots will end up on disk, but the old account not... neither the new one which got deleted. So we have storage slots, but no accounts on top.
After snap sync finishes, we need to regenerate the state snapshot. So we use our batched account prover to fix any issues in the accounts. The account range may or may not be correct - and we will fix it - however, we will not care about the deleted account since it's not part of the snapshot account range.
Then after fixing the account range, we verify each of the account's storage slots - each existing one that is, or freshly deleted ones. But we don't ever consider dangling storage data without an account.
This PR fixes this issue by introducing a storage scanner. It will be constructed for each account range which detects all the dangling accounts and caches them in memory(the size is limited, no OOM issue). Constructing scanner is not trivial which might involve lots of disk reads, but it's per account range so it's still acceptable. The detected dangling storages will be wiped at the correct time during the generation.