fix(lib/trie): record deleted Merkle values fixed (#2873)

- Record Merkle values deleted only if they are NOT 'dirty' (the same as in the last trie snapshot)
- Record Merkle values deleted when one or more nodes are deleted from the trie
- Do NOT record Merkle values of inlined nodes in the deleted set of Merkle values
- Lazy calculate Merkle values if they are missing (especially for delete operations)
- Update tests to have full trie.go coverage again (except hashing error due to the buffer sync pools at global scope)
- Fix preparing branch for mutation only once before for loop (instead of N times)
- Check errors from merkle value computation (useful for lazy loading to handle database errors)

Author: qdm12

dot/state/test_helpers.go

@@ -244,7 +244,8 @@ func generateBlockWithRandomTrie(t *testing.T, serv *Service,
 	rand := time.Now().UnixNano()
 	key := []byte("testKey" + fmt.Sprint(rand))
 	value := []byte("testValue" + fmt.Sprint(rand))
-	trieState.Put(key, value)
+	err = trieState.Put(key, value)
+	require.NoError(t, err)
 
 	trieStateRoot, err := trieState.Root()
 	require.NoError(t, err)

lib/runtime/interfaces.go

@@ -11,22 +11,24 @@ import (
 
 // Storage runtime interface.
 type Storage interface {
-	Put(key []byte, value []byte)
+	Put(key []byte, value []byte) (err error)
 	Get(key []byte) []byte
 	Root() (common.Hash, error)
 	SetChild(keyToChild []byte, child *trie.Trie) error
 	SetChildStorage(keyToChild, key, value []byte) error
 	GetChildStorage(keyToChild, key []byte) ([]byte, error)
-	Delete(key []byte)
-	DeleteChild(keyToChild []byte)
-	DeleteChildLimit(keyToChild []byte, limit *[]byte) (uint32, bool, error)
+	Delete(key []byte) (err error)
+	DeleteChild(keyToChild []byte) (err error)
+	DeleteChildLimit(keyToChild []byte, limit *[]byte) (
+		deleted uint32, allDeleted bool, err error)
 	ClearChildStorage(keyToChild, key []byte) error
 	NextKey([]byte) []byte
 	ClearPrefixInChild(keyToChild, prefix []byte) error
 	GetChildNextKey(keyToChild, key []byte) ([]byte, error)
 	GetChild(keyToChild []byte) (*trie.Trie, error)
-	ClearPrefix(prefix []byte)
-	ClearPrefixLimit(prefix []byte, limit uint32) (uint32, bool)
+	ClearPrefix(prefix []byte) (err error)
+	ClearPrefixLimit(prefix []byte, limit uint32) (
+		deleted uint32, allDeleted bool, err error)
 	BeginStorageTransaction()
 	CommitStorageTransaction()
 	RollbackStorageTransaction()

lib/runtime/storage/trie.go

@@ -5,6 +5,7 @@ package storage
 
 import (
 	"encoding/binary"
+	"fmt"
 	"sort"
 	"sync"
 
@@ -67,11 +68,11 @@ func (s *TrieState) RollbackStorageTransaction() {
 	s.oldTrie = nil
 }
 
-// Put puts thread safely a value at the specified key in the trie.
-func (s *TrieState) Put(key, value []byte) {
+// Put puts a key-value pair in the trie
+func (s *TrieState) Put(key, value []byte) (err error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
-	s.t.Put(key, value)
+	return s.t.Put(key, value)
 }
 
 // Get gets a value from the trie
@@ -97,15 +98,20 @@ func (s *TrieState) Has(key []byte) bool {
 }
 
 // Delete deletes a key from the trie
-func (s *TrieState) Delete(key []byte) {
+func (s *TrieState) Delete(key []byte) (err error) {
 	val := s.t.Get(key)
 	if val == nil {
-		return
+		return nil
 	}
 
 	s.lock.Lock()
 	defer s.lock.Unlock()
-	s.t.Delete(key)
+	err = s.t.Delete(key)
+	if err != nil {
+		return fmt.Errorf("deleting from trie: %w", err)
+	}
+
+	return nil
 }
 
 // NextKey returns the next key in the trie in lexicographical order. If it does not exist, it returns nil.
@@ -116,19 +122,19 @@ func (s *TrieState) NextKey(key []byte) []byte {
 }
 
 // ClearPrefix deletes all key-value pairs from the trie where the key starts with the given prefix
-func (s *TrieState) ClearPrefix(prefix []byte) {
+func (s *TrieState) ClearPrefix(prefix []byte) (err error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
-	s.t.ClearPrefix(prefix)
+	return s.t.ClearPrefix(prefix)
 }
 
 // ClearPrefixLimit deletes key-value pairs from the trie where the key starts with the given prefix till limit reached
-func (s *TrieState) ClearPrefixLimit(prefix []byte, limit uint32) (uint32, bool) {
+func (s *TrieState) ClearPrefixLimit(prefix []byte, limit uint32) (
+	deleted uint32, allDeleted bool, err error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 
-	num, del := s.t.ClearPrefixLimit(prefix, limit)
-	return num, del
+	return s.t.ClearPrefixLimit(prefix, limit)
 }
 
 // TrieEntries returns every key-value pair in the trie
@@ -167,24 +173,29 @@ func (s *TrieState) GetChildStorage(keyToChild, key []byte) ([]byte, error) {
 }
 
 // DeleteChild deletes a child trie from the main trie
-func (s *TrieState) DeleteChild(key []byte) {
+func (s *TrieState) DeleteChild(key []byte) (err error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
-	s.t.DeleteChild(key)
+	return s.t.DeleteChild(key)
 }
 
-// DeleteChildLimit deletes up to limit of database entries by lexicographic order, return number
-//  deleted, true if all delete otherwise false
-func (s *TrieState) DeleteChildLimit(key []byte, limit *[]byte) (uint32, bool, error) {
+// DeleteChildLimit deletes up to limit of database entries by lexicographic order.
+func (s *TrieState) DeleteChildLimit(key []byte, limit *[]byte) (
+	deleted uint32, allDeleted bool, err error) {
 	s.lock.Lock()
 	defer s.lock.Unlock()
 	tr, err := s.t.GetChild(key)
 	if err != nil {
 		return 0, false, err
 	}
+
 	qtyEntries := uint32(len(tr.Entries()))
 	if limit == nil {
-		s.t.DeleteChild(key)
+		err = s.t.DeleteChild(key)
+		if err != nil {
+			return 0, false, fmt.Errorf("deleting child trie: %w", err)
+		}
+
 		return qtyEntries, true, nil
 	}
 	limitUint := binary.LittleEndian.Uint32(*limit)
@@ -194,20 +205,25 @@ func (s *TrieState) DeleteChildLimit(key []byte, limit *[]byte) (uint32, bool, e
 		keys = append(keys, k)
 	}
 	sort.Strings(keys)
-	deleted := uint32(0)
 	for _, k := range keys {
-		tr.Delete([]byte(k))
+		// TODO have a transactional/atomic way to delete multiple keys in trie.
+		// If one deletion fails, the child trie and its parent trie are then in
+		// a bad intermediary state. Take also care of the caching of deleted Merkle
+		// values within the tries, which is used for online pruning.
+		// See https://github.com/ChainSafe/gossamer/issues/3032
+		err = tr.Delete([]byte(k))
+		if err != nil {
+			return deleted, allDeleted, fmt.Errorf("deleting from child trie located at key 0x%x: %w", key, err)
+		}
+
 		deleted++
 		if deleted == limitUint {
 			break
 		}
 	}
 
-	if deleted == qtyEntries {
-		return deleted, true, nil
-	}
-
-	return deleted, false, nil
+	allDeleted = deleted == qtyEntries
+	return deleted, allDeleted, nil
 }
 
 // ClearChildStorage removes the child storage entry from the trie
@@ -230,7 +246,11 @@ func (s *TrieState) ClearPrefixInChild(keyToChild, prefix []byte) error {
 		return nil
 	}
 
-	child.ClearPrefix(prefix)
+	err = child.ClearPrefix(prefix)
+	if err != nil {
+		return fmt.Errorf("clearing prefix in child trie located at key 0x%x: %w", keyToChild, err)
+	}
+
 	return nil
 }
 

lib/runtime/wasmer/helpers.go

@@ -208,7 +208,7 @@ func toWasmMemoryFixedSizeOptional(context wasmer.InstanceContext, data []byte)
 	return toWasmMemory(context, encodedOptionalFixedSize)
 }
 
-func storageAppend(storage GetSetter, key, valueToAppend []byte) error {
+func storageAppend(storage GetSetter, key, valueToAppend []byte) (err error) {
 	// this function assumes the item in storage is a SCALE encoded array of items
 	// the valueToAppend is a new item, so it appends the item and increases the length prefix by 1
 	currentValue := storage.Get(key)
@@ -259,7 +259,16 @@ func storageAppend(storage GetSetter, key, valueToAppend []byte) error {
 		}
 	}
 
-	logger.Debugf("resulting value: 0x%x", value)
-	storage.Put(key, value)
+	err = storage.Put(key, value)
+	if err != nil {
+		return fmt.Errorf("putting key and value in storage: %w", err)
+	}
+
 	return nil
 }
+
+func panicOnError(err error) {
+	if err != nil {
+		panic(err)
+	}
+}

lib/runtime/wasmer/helpers_test.go

@@ -5,6 +5,7 @@ package wasmer
 
 import (
 	"encoding/json"
+	"errors"
 	"os"
 	"path/filepath"
 	"testing"
@@ -66,3 +67,13 @@ func Test_pointerSize(t *testing.T) {
 		})
 	}
 }
+
+func Test_panicOnError(t *testing.T) {
+	t.Parallel()
+
+	err := (error)(nil)
+	assert.NotPanics(t, func() { panicOnError(err) })
+
+	err = errors.New("test error")
+	assert.PanicsWithValue(t, err, func() { panicOnError(err) })
+}

lib/runtime/wasmer/imports.go

@@ -817,7 +817,12 @@ func ext_trie_blake2_256_root_version_1(context unsafe.Pointer, dataSpan C.int64
 	}
 
 	for _, kv := range kvs {
-		t.Put(kv.Key, kv.Value)
+		err := t.Put(kv.Key, kv.Value)
+		if err != nil {
+			logger.Errorf("failed putting key 0x%x and value 0x%x into trie: %s",
+				kv.Key, kv.Value, err)
+			return 0
+		}
 	}
 
 	// allocate memory for value and copy value to memory
@@ -865,7 +870,12 @@ func ext_trie_blake2_256_ordered_root_version_1(context unsafe.Pointer, dataSpan
 			"put key=0x%x and value=0x%x",
 			key, value)
 
-		t.Put(key, value)
+		err = t.Put(key, value)
+		if err != nil {
+			logger.Errorf("failed putting key 0x%x and value 0x%x into trie: %s",
+				key, value, err)
+			return 0
+		}
 	}
 
 	// allocate memory for value and copy value to memory
@@ -1180,7 +1190,8 @@ func ext_default_child_storage_storage_kill_version_1(context unsafe.Pointer, ch
 	storage := ctx.Storage
 
 	childStorageKey := asMemorySlice(instanceContext, childStorageKeySpan)
-	storage.DeleteChild(childStorageKey)
+	err := storage.DeleteChild(childStorageKey)
+	panicOnError(err)
 }
 
 //export ext_default_child_storage_storage_kill_version_2
@@ -1843,7 +1854,8 @@ func ext_storage_clear_version_1(context unsafe.Pointer, keySpan C.int64_t) {
 	key := asMemorySlice(instanceContext, keySpan)
 
 	logger.Debugf("key: 0x%x", key)
-	storage.Delete(key)
+	err := storage.Delete(key)
+	panicOnError(err)
 }
 
 //export ext_storage_clear_prefix_version_1
@@ -1856,7 +1868,8 @@ func ext_storage_clear_prefix_version_1(context unsafe.Pointer, prefixSpan C.int
 	prefix := asMemorySlice(instanceContext, prefixSpan)
 	logger.Debugf("prefix: 0x%x", prefix)
 
-	storage.ClearPrefix(prefix)
+	err := storage.ClearPrefix(prefix)
+	panicOnError(err)
 }
 
 //export ext_storage_clear_prefix_version_2
@@ -1885,7 +1898,12 @@ func ext_storage_clear_prefix_version_2(context unsafe.Pointer, prefixSpan, lim
 	}
 
 	limitUint := binary.LittleEndian.Uint32(limit)
-	numRemoved, all := storage.ClearPrefixLimit(prefix, limitUint)
+	numRemoved, all, err := storage.ClearPrefixLimit(prefix, limitUint)
+	if err != nil {
+		logger.Errorf("failed to clear prefix limit: %s", err)
+		return mustToWasmMemoryNil(instanceContext)
+	}
+
 	encBytes, err := toKillStorageResultEnum(all, numRemoved)
 	if err != nil {
 		logger.Errorf("failed to allocate memory: %s", err)
@@ -2044,7 +2062,8 @@ func ext_storage_set_version_1(context unsafe.Pointer, keySpan, valueSpan C.int6
 	logger.Debugf(
 		"key 0x%x has value 0x%x",
 		key, value)
-	storage.Put(key, cp)
+	err := storage.Put(key, cp)
+	panicOnError(err)
 }
 
 //export ext_storage_start_transaction_version_1

lib/runtime/wasmer/interfaces.go

@@ -17,16 +17,17 @@ type Storage interface {
 	SetChild(keyToChild []byte, child *trie.Trie) error
 	SetChildStorage(keyToChild, key, value []byte) error
 	GetChildStorage(keyToChild, key []byte) ([]byte, error)
-	Delete(key []byte)
-	DeleteChild(keyToChild []byte)
+	Delete(key []byte) (err error)
+	DeleteChild(keyToChild []byte) (err error)
 	DeleteChildLimit(keyToChild []byte, limit *[]byte) (uint32, bool, error)
 	ClearChildStorage(keyToChild, key []byte) error
 	NextKey([]byte) []byte
 	ClearPrefixInChild(keyToChild, prefix []byte) error
 	GetChildNextKey(keyToChild, key []byte) ([]byte, error)
 	GetChild(keyToChild []byte) (*trie.Trie, error)
-	ClearPrefix(prefix []byte)
-	ClearPrefixLimit(prefix []byte, limit uint32) (uint32, bool)
+	ClearPrefix(prefix []byte) (err error)
+	ClearPrefixLimit(prefix []byte, limit uint32) (
+		deleted uint32, allDeleted bool, err error)
 	BeginStorageTransaction()
 	CommitStorageTransaction()
 	RollbackStorageTransaction()
@@ -46,7 +47,7 @@ type Getter interface {
 
 // Putter puts a value for a key.
 type Putter interface {
-	Put(key []byte, value []byte)
+	Put(key []byte, value []byte) (err error)
 }
 
 // BasicNetwork interface for functions used by runtime network state function