Optimize destroying an account with no storage in snapshot diffToDisk #399
Conversation
core/state/snapshot/snapshot.go
Outdated
| // Skip any account not covered yet by the snapshot | ||
| if base.genMarker != nil && bytes.Compare(hash[:], base.genMarker) > 0 { | ||
| if errors.Is(err, ErrNotCoveredYet) { |
There was a problem hiding this comment.
Though it makes sense, I'd keep the code here unchanged, to minimise diff.
There was a problem hiding this comment.
I don't think there's merge conflict concern here because this code seems to be unchanged upstream until it's removed in ethereum/go-ethereum#30752
core/state/snapshot/snapshot.go
Outdated
| rawdb.DeleteAccountSnapshot(batch, hash) | ||
| base.cache.Set(hash[:], nil) | ||
|
|
||
| if err == nil && (oldAccount == nil || oldAccount.Root == nil) { |
There was a problem hiding this comment.
For the sake of a minimal understandable diff, I'd put here the call to base.account.
Also, either handle err != nil in some way or comment why you don't need to.
There was a problem hiding this comment.
I've added a warning if err != nil and a comment explaining that it falls through to iterating over and removing storage in case there is any
core/state/snapshot/disklayer.go
Outdated
| @@ -76,30 +76,28 @@ func (dl *diskLayer) Stale() bool { | |||
|
|
|||
| // Account directly retrieves the account associated with a particular hash in | |||
| // the snapshot slim data format. | |||
| func (dl *diskLayer) Account(hash common.Hash) (*types.SlimAccount, error) { | |||
| data, err := dl.AccountRLP(hash) | |||
| func (dl *diskLayer) account(hash common.Hash, ignoreStale bool) (*types.SlimAccount, error) { | |||
There was a problem hiding this comment.
thinking alternative names.. not sure they are better, but what do you think about "evenIfStale" ?
There was a problem hiding this comment.
Sounds good to me, renamed
To do this, we get the account info from the disk layer and check if its storage root was empty before trying to iterate over its storage to delete it.