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/state: implement fast storage deletion #27955

Merged
merged 7 commits into from
Aug 26, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 107 additions & 27 deletions core/state/statedb.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ import (
"github.com/ethereum/go-ethereum/trie/triestate"
)

const (
// storageDeleteLimit denotes the highest permissible memory allocation
// employed for contract storage deletion.
storageDeleteLimit = 512 * 1024 * 1024
)

type revision struct {
id int
journalIndex int
Expand Down Expand Up @@ -1022,59 +1028,127 @@ func (s *StateDB) clearJournalAndRefund() {
s.validRevisions = s.validRevisions[:0] // Snapshots can be created without journal entries
}

// deleteStorage iterates the storage trie belongs to the account and mark all
// slots inside as deleted.
func (s *StateDB) deleteStorage(addr common.Address, addrHash common.Hash, root common.Hash) (bool, map[common.Hash][]byte, *trienode.NodeSet, error) {
start := time.Now()
// fastDeleteStorage is the function that efficiently deletes the storage trie
// of a specific account. It leverages the associated state snapshot for fast
// storage iteration and constructs trie node deletion markers by creating
// stack trie with iterated slots.
func (s *StateDB) fastDeleteStorage(addrHash common.Hash, root common.Hash) (bool, common.StorageSize, map[common.Hash][]byte, *trienode.NodeSet, error) {
iter, err := s.snaps.StorageIterator(s.originalRoot, addrHash, common.Hash{})
if err != nil {
return false, 0, nil, nil, err
}
defer iter.Release()

var (
size common.StorageSize
nodes = trienode.NewNodeSet(addrHash)
slots = make(map[common.Hash][]byte)
)
stack := trie.NewStackTrie(func(owner common.Hash, path []byte, hash common.Hash, blob []byte) {
nodes.AddNode(path, trienode.NewDeleted())
size += common.StorageSize(len(path))
})
for iter.Next() {
if size > storageDeleteLimit {
return true, size, nil, nil, nil
}
slot := common.CopyBytes(iter.Slot())
size += common.StorageSize(common.HashLength + len(slot))
slots[iter.Hash()] = slot

if err := stack.Update(iter.Hash().Bytes(), slot); err != nil {
return false, 0, nil, nil, err
}
}
if iter.Error() != nil {
return false, 0, nil, nil, err
}
if stack.Hash() != root {
return false, 0, nil, nil, fmt.Errorf("snapshot is not matched, exp %x, got %x", root, stack.Hash())
}
Comment on lines +1069 to +1071
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if stack.Hash() != root {
return false, 0, nil, nil, fmt.Errorf("snapshot is not matched, exp %x, got %x", root, stack.Hash())
}
if it.Err() != nil {
return false, 0, nil, nil, it.Err()
}
if stack.Hash() != root {
return false, 0, nil, nil, fmt.Errorf("snapshot is not matched, exp %x, got %x", root, stack.Hash())
}

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, both it.Next() and it.Hash() can internally error, so we should check it.Err() after each invocation. If we fail to do so, and ignore an error from it.Hash(), and subsequently call it.Next(), then we will be rewarded with panic(fmt.Sprintf("called Next of failed iterator: %v", it.fail))

But I guess it should be enough to have one check after the loop, like my comment, and another check right after slots[iter.Hash()] = slot ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically, we need to check the failure after each iteration, however, i guess it's unnecessary. The cached error won't be clean anyway.

And the reason I don't add the iterator error checking here is: if we encounter any failure in iterator, the stackTrie will produce a different root hash anyway.

But yeah, I will add one check right after the loop. It's cheap anyway and would be helpful to bubble up the "real issue".

Copy link
Contributor

Choose a reason for hiding this comment

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

And the reason I don't add the iterator error checking here is: if we encounter any failure in iterator, the stackTrie will produce a different root hash anyway.

Ah, no, as I pointed out, we won't get a different root, we will encounter a panic crash.

return false, size, slots, nodes, nil
}

// slowDeleteStorage serves as a less-efficient alternative to "fastDeleteStorage,"
// employed when the associated state snapshot is not available. It iterates the
// storage slots along with all internal trie nodes via trie directly.
func (s *StateDB) slowDeleteStorage(addr common.Address, addrHash common.Hash, root common.Hash) (bool, common.StorageSize, map[common.Hash][]byte, *trienode.NodeSet, error) {
tr, err := s.db.OpenStorageTrie(s.originalRoot, addr, root)
if err != nil {
return false, nil, nil, fmt.Errorf("failed to open storage trie, err: %w", err)
return false, 0, nil, nil, fmt.Errorf("failed to open storage trie, err: %w", err)
}
it, err := tr.NodeIterator(nil)
if err != nil {
return false, nil, nil, fmt.Errorf("failed to open storage iterator, err: %w", err)
return false, 0, nil, nil, fmt.Errorf("failed to open storage iterator, err: %w", err)
}
var (
set = trienode.NewNodeSet(addrHash)
slots = make(map[common.Hash][]byte)
stateSize common.StorageSize
nodeSize common.StorageSize
size common.StorageSize
nodes = trienode.NewNodeSet(addrHash)
slots = make(map[common.Hash][]byte)
)
for it.Next(true) {
// arbitrary stateSize limit, make it configurable
if stateSize+nodeSize > 512*1024*1024 {
log.Info("Skip large storage deletion", "address", addr.Hex(), "states", stateSize, "nodes", nodeSize)
if metrics.EnabledExpensive {
slotDeletionSkip.Inc(1)
}
return true, nil, nil, nil
if size > storageDeleteLimit {
return true, size, nil, nil, nil
}
if it.Leaf() {
slots[common.BytesToHash(it.LeafKey())] = common.CopyBytes(it.LeafBlob())
stateSize += common.StorageSize(common.HashLength + len(it.LeafBlob()))
size += common.StorageSize(common.HashLength + len(it.LeafBlob()))
continue
}
if it.Hash() == (common.Hash{}) {
continue
}
nodeSize += common.StorageSize(len(it.Path()))
set.AddNode(it.Path(), trienode.NewDeleted())
size += common.StorageSize(len(it.Path()))
nodes.AddNode(it.Path(), trienode.NewDeleted())
}
if err := it.Error(); err != nil {
return false, 0, nil, nil, err
}
return false, size, slots, nodes, nil
}

// deleteStorage is designed to delete the storage trie of a designated account.
// It could potentially be terminated if the storage size is excessively large,
// potentially leading to an out-of-memory panic. The function will make an attempt
// to utilize an efficient strategy if the associated state snapshot is reachable;
// otherwise, it will resort to a less-efficient approach.
func (s *StateDB) deleteStorage(addr common.Address, addrHash common.Hash, root common.Hash) (bool, map[common.Hash][]byte, *trienode.NodeSet, error) {
var (
start = time.Now()
err error
aborted bool
size common.StorageSize
slots map[common.Hash][]byte
nodes *trienode.NodeSet
)
// The fast approach can be failed if the snapshot is not fully
// generated, or it's internally corrupted. Fallback to the slow
// one just in case.
if s.snap != nil {
aborted, size, slots, nodes, err = s.fastDeleteStorage(addrHash, root)
}
if s.snap == nil || err != nil {
aborted, size, slots, nodes, err = s.slowDeleteStorage(addr, addrHash, root)
}
if err != nil {
return false, nil, nil, err
}
if metrics.EnabledExpensive {
if int64(len(slots)) > slotDeletionMaxCount.Value() {
slotDeletionMaxCount.Update(int64(len(slots)))
if aborted {
slotDeletionSkip.Inc(1)
}
n := int64(len(slots))
if n > slotDeletionMaxCount.Value() {
slotDeletionMaxCount.Update(n)
}
if int64(stateSize+nodeSize) > slotDeletionMaxSize.Value() {
slotDeletionMaxSize.Update(int64(stateSize + nodeSize))
if int64(size) > slotDeletionMaxSize.Value() {
slotDeletionMaxSize.Update(int64(size))
}
slotDeletionTimer.UpdateSince(start)
slotDeletionCount.Mark(int64(len(slots)))
slotDeletionSize.Mark(int64(stateSize + nodeSize))
slotDeletionCount.Mark(n)
slotDeletionSize.Mark(int64(size))
}
return false, slots, set, nil
return aborted, slots, nodes, nil
}

// handleDestruction processes all destruction markers and deletes the account
Expand Down Expand Up @@ -1102,7 +1176,13 @@ func (s *StateDB) deleteStorage(addr common.Address, addrHash common.Hash, root
// In case (d), **original** account along with its storages should be deleted,
// with their values be tracked as original value.
func (s *StateDB) handleDestruction(nodes *trienode.MergedNodeSet) (map[common.Address]struct{}, error) {
// Short circuit if geth is running with hash mode. This procedure can consume
// considerable time and storage deletion isn't supported in hash mode, thus
// preemptively avoiding unnecessary expenses.
incomplete := make(map[common.Address]struct{})
if s.db.TrieDB().Scheme() == rawdb.HashScheme {
return incomplete, nil
}
for addr, prev := range s.stateObjectsDestruct {
// The original account was non-existing, and it's marked as destructed
// in the scope of block. It can be case (a) or (b).
Expand Down
18 changes: 16 additions & 2 deletions core/state/statedb_fuzz_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,12 @@ import (

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/core/rawdb"
"github.com/ethereum/go-ethereum/core/state/snapshot"
"github.com/ethereum/go-ethereum/core/types"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/rlp"
"github.com/ethereum/go-ethereum/trie"
"github.com/ethereum/go-ethereum/trie/triedb/pathdb"
"github.com/ethereum/go-ethereum/trie/triestate"
)

Expand Down Expand Up @@ -179,16 +181,28 @@ func (test *stateTest) run() bool {
storageList = append(storageList, copy2DSet(states.Storages))
}
disk = rawdb.NewMemoryDatabase()
tdb = trie.NewDatabase(disk, &trie.Config{OnCommit: onCommit})
tdb = trie.NewDatabase(disk, &trie.Config{OnCommit: onCommit, PathDB: pathdb.Defaults})
sdb = NewDatabaseWithNodeDB(disk, tdb)
byzantium = rand.Intn(2) == 0
)
defer disk.Close()
defer tdb.Close()

var snaps *snapshot.Tree
if rand.Intn(3) == 0 {
snaps, _ = snapshot.New(snapshot.Config{
CacheSize: 1,
Recovery: false,
NoBuild: false,
AsyncBuild: false,
}, disk, tdb, types.EmptyRootHash)
}
for i, actions := range test.actions {
root := types.EmptyRootHash
if i != 0 {
root = roots[len(roots)-1]
}
state, err := New(root, sdb, nil)
state, err := New(root, sdb, snaps)
if err != nil {
panic(err)
}
Expand Down