core/state: implement fast storage deletion

[rjl493456442]

Deletion of 328239 slots 22.47 MiB statesize in time 812.148619ms
800ms vs 3.6 s

[holiman]

Looking at the old code now.. Ok, so for hashdb, it first iterates the storage trie, collects it into the set var: aborted, slots, set, err := s.deleteStorage(addr, addrHash, prev.Root),

The set are merged into nodes

		if err := nodes.Merge(set); err != nil {
			return nil, err
		}

Other changes are merged into the same nodes, and eventually, we update the triedb:

		if err := s.db.TrieDB().Update(root, origin, block, nodes, triestate.New(s.accountsOrigin, s.storagesOrigin, incomplete)); err != nil {
			return common.Hash{}, err
		}

And, eventually, it reaches Update, and does

	for _, owner := range order {
		subset := nodes.Sets[owner]
		subset.ForEachWithOrder(func(path string, n *trienode.Node) {
			if n.IsDeleted() {
				return // ignore deletion
			}

So it just ignores all deletions in the end. So all the work we did there was just discarded in the end? Or was there some period where it was used in a memory-capacity, and we just discard the actual disk-write?

If indeed we just iterate/collect the storage nodes just to ignore them in hash-mode, it seems that we can optimize that particular pipeline earlier, and ignore the iteration too?

[holiman]

Some more thoughts. Wouldn't this be a non-changing refactoring?

The original code collects the keys first, but it doesn't actually do anything with it, like sort it, just iterates it again. It looks like the only reason for the first collection is to filter out the empty-element, but that can be easily done in the second loop. Am I missing something?

diff --git a/trie/triedb/hashdb/database.go b/trie/triedb/hashdb/database.go
index b3ae54dbe3..e00b082a65 100644
--- a/trie/triedb/hashdb/database.go
+++ b/trie/triedb/hashdb/database.go
@@ -587,18 +587,10 @@ func (db *Database) Update(root common.Hash, parent common.Hash, block uint64, n
 	//
 	// Note, the storage tries must be flushed before the account trie to
 	// retain the invariant that children go into the dirty cache first.
-	var order []common.Hash
-	for owner := range nodes.Sets {
+	for owner, subset := range nodes.Sets {
 		if owner == (common.Hash{}) {
 			continue
 		}
-		order = append(order, owner)
-	}
-	if _, ok := nodes.Sets[common.Hash{}]; ok {
-		order = append(order, common.Hash{})
-	}
-	for _, owner := range order {
-		subset := nodes.Sets[owner]
 		subset.ForEachWithOrder(func(path string, n *trienode.Node) {
 			if n.IsDeleted() {
 				return // ignore deletion
@@ -609,6 +601,12 @@ func (db *Database) Update(root common.Hash, parent common.Hash, block uint64, n
 	// Link up the account trie and storage trie if the node points
 	// to an account trie leaf.
 	if set, present := nodes.Sets[common.Hash{}]; present {
+		set.ForEachWithOrder(func(path string, n *trienode.Node) {
+			if n.IsDeleted() {
+				return // ignore deletion
+			}
+			db.insert(n.Hash, n.Blob)
+		})
 		for _, n := range set.Leaves {
 			var account types.StateAccount
 			if err := rlp.DecodeBytes(n.Blob, &account); err != nil {

[rjl493456442]

So it just ignores all deletions in the end. So all the work we did there was just discarded in the end? Or was there some period where it was used in a memory-capacity, and we just discard the actual disk-write?

Yes, in hash mode, deletion is not supported, and it's totally useless in hash mode, just try to align with path mode.

and ignore the iteration too?

I added this logic in this PR.

[holiman]

I added this logic in this PR.

Right, gotcha! I'm a couple of steps behind here :)

[rjl493456442]

It looks like the only reason for the first collection is to filter out the empty-element, but that can be easily done in the second loop. Am I missing something?

Yep, basically we need to flush storage trie(s) first, and then account trie. I think logically the refactor is OK, but i would prefer to not change it, because last time I changed the logic here and result in a big bug in this part :)

[rjl493456442]

Running a full sync on sepolia to ensure nothing is broken.

[holiman]

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())
	}

[holiman]

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 ?

[rjl493456442]

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".

[holiman]

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.

[holiman]

I added a test aswell -- maybe it was superfluous, I couldn't really tell. Anyway, LGTM!