From 2966d1285aeae995357653138b708f71f96125c1 Mon Sep 17 00:00:00 2001 From: Igor Crevar Date: Mon, 23 Oct 2023 15:04:07 +0200 Subject: [PATCH] EVM-864 Snapshot writing error not handled (#1993) --- state/executor.go | 10 +++- state/immutable-trie/copy_trie.go | 40 ++++++++++----- state/immutable-trie/snapshot.go | 18 ++++--- state/immutable-trie/state.go | 4 +- state/immutable-trie/storage.go | 85 ++++++++++++++++++------------- state/state.go | 2 +- state/testing.go | 47 +++++++++++------ state/txn_test.go | 4 +- tests/state_test.go | 3 +- tests/testing.go | 4 +- 10 files changed, 137 insertions(+), 80 deletions(-) diff --git a/state/executor.go b/state/executor.go index 0bcf46510a..8e211e9aed 100644 --- a/state/executor.go +++ b/state/executor.go @@ -116,7 +116,10 @@ func (e *Executor) WriteGenesis( return types.Hash{}, err } - _, root := snap.Commit(objs) + _, root, err := snap.Commit(objs) + if err != nil { + return types.Hash{}, err + } return types.BytesToHash(root), nil } @@ -396,7 +399,10 @@ func (t *Transition) Commit() (Snapshot, types.Hash, error) { return nil, types.ZeroHash, err } - s2, root := t.snap.Commit(objs) + s2, root, err := t.snap.Commit(objs) + if err != nil { + return nil, types.ZeroHash, err + } return s2, types.BytesToHash(root), nil } diff --git a/state/immutable-trie/copy_trie.go b/state/immutable-trie/copy_trie.go index 3f65878a61..afa912187b 100644 --- a/state/immutable-trie/copy_trie.go +++ b/state/immutable-trie/copy_trie.go @@ -17,9 +17,9 @@ import ( var emptyCodeHash = crypto.Keccak256(nil) func getCustomNode(hash []byte, storage Storage) (Node, []byte, error) { - data, ok := storage.Get(hash) - if !ok { - return nil, nil, nil + data, ok, err := storage.Get(hash) + if err != nil || !ok { + return nil, nil, err } // NOTE. We dont need to make copies of the bytes because the nodes @@ -42,24 +42,34 @@ func getCustomNode(hash []byte, storage Storage) (Node, []byte, error) { } func CopyTrie(nodeHash []byte, storage Storage, newStorage Storage, agg []byte, isStorage bool) error { + batchWriter := newStorage.Batch() + + if err := copyTrieHash(nodeHash, storage, batchWriter, agg, isStorage); err != nil { + return err + } + + return batchWriter.Write() +} + +func copyTrieHash(nodeHash []byte, storage Storage, batchWriter Batch, agg []byte, isStorage bool) error { node, data, err := getCustomNode(nodeHash, storage) if err != nil { return err } //copy whole bytes of nodes - newStorage.Put(nodeHash, data) + batchWriter.Put(nodeHash, data) - return copyTrie(node, storage, newStorage, agg, isStorage) + return copyTrieNode(node, storage, batchWriter, agg, isStorage) } -func copyTrie(node Node, storage Storage, newStorage Storage, agg []byte, isStorage bool) error { +func copyTrieNode(node Node, storage Storage, batchWriter Batch, agg []byte, isStorage bool) error { switch n := node.(type) { case nil: return nil case *FullNode: if len(n.hash) > 0 { - return CopyTrie(n.hash, storage, newStorage, agg, isStorage) + return copyTrieHash(n.hash, storage, batchWriter, agg, isStorage) } for i := range n.children { @@ -67,7 +77,7 @@ func copyTrie(node Node, storage Storage, newStorage Storage, agg []byte, isStor continue } - err := copyTrie(n.children[i], storage, newStorage, append(agg, uint8(i)), isStorage) + err := copyTrieNode(n.children[i], storage, batchWriter, append(agg, uint8(i)), isStorage) if err != nil { return err } @@ -76,7 +86,7 @@ func copyTrie(node Node, storage Storage, newStorage Storage, agg []byte, isStor case *ValueNode: //if node represens stored value, then we need to copy it if n.hash { - return CopyTrie(n.buf, storage, newStorage, agg, isStorage) + return copyTrieHash(n.buf, storage, batchWriter, agg, isStorage) } if !isStorage { @@ -85,26 +95,28 @@ func copyTrie(node Node, storage Storage, newStorage Storage, agg []byte, isStor return fmt.Errorf("can't parse account %s: %w", hex.EncodeToString(encodeCompact(agg)), err) } else { if account.CodeHash != nil && bytes.Equal(account.CodeHash, emptyCodeHash) == false { - code, ok := storage.GetCode(types.BytesToHash(account.CodeHash)) + hash := types.BytesToHash(account.CodeHash) + + code, ok := storage.GetCode(hash) if ok { - newStorage.SetCode(types.BytesToHash(account.CodeHash), code) + batchWriter.Put(GetCodeKey(hash), code) } else { return fmt.Errorf("can't find code %s", hex.EncodeToString(account.CodeHash)) } } if account.Root != types.EmptyRootHash { - return CopyTrie(account.Root[:], storage, newStorage, nil, true) + return copyTrieHash(account.Root[:], storage, batchWriter, nil, true) } } } case *ShortNode: if len(n.hash) > 0 { - return CopyTrie(n.hash, storage, newStorage, agg, isStorage) + return copyTrieHash(n.hash, storage, batchWriter, agg, isStorage) } - return copyTrie(n.child, storage, newStorage, append(agg, n.key...), isStorage) + return copyTrieNode(n.child, storage, batchWriter, append(agg, n.key...), isStorage) } return nil diff --git a/state/immutable-trie/snapshot.go b/state/immutable-trie/snapshot.go index 84a06fe1fa..869cc5f759 100644 --- a/state/immutable-trie/snapshot.go +++ b/state/immutable-trie/snapshot.go @@ -2,6 +2,7 @@ package itrie import ( "bytes" + "fmt" "github.com/0xPolygon/polygon-edge/crypto" "github.com/0xPolygon/polygon-edge/state" @@ -73,7 +74,7 @@ func (s *Snapshot) GetCode(hash types.Hash) ([]byte, bool) { return s.state.GetCode(hash) } -func (s *Snapshot) Commit(objs []*state.Object) (state.Snapshot, []byte) { +func (s *Snapshot) Commit(objs []*state.Object) (state.Snapshot, []byte, error) { batch := s.state.storage.Batch() tt := s.trie.Txn(s.state.storage) @@ -96,7 +97,7 @@ func (s *Snapshot) Commit(objs []*state.Object) (state.Snapshot, []byte) { if len(obj.Storage) != 0 { trie, err := s.state.newTrieAt(obj.Root) if err != nil { - panic(err) //nolint:gocritic + return nil, types.ZeroHash[:], fmt.Errorf("snapshot commit failed to create trie: %w", err) } localTxn := trie.Txn(s.state.storage) @@ -122,7 +123,7 @@ func (s *Snapshot) Commit(objs []*state.Object) (state.Snapshot, []byte) { } if obj.DirtyCode { - s.state.SetCode(obj.CodeHash, obj.Code) + batch.Put(GetCodeKey(obj.CodeHash), obj.Code) } vv := account.MarshalWith(arena) @@ -133,14 +134,19 @@ func (s *Snapshot) Commit(objs []*state.Object) (state.Snapshot, []byte) { } } - root, _ := tt.Hash() + root, err := tt.Hash() + if err != nil { + return nil, types.ZeroHash[:], fmt.Errorf("snapshot commit can not retrieve hash: %w", err) + } nTrie := tt.Commit() // Write all the entries to db - batch.Write() + if err := batch.Write(); err != nil { + return nil, types.ZeroHash[:], fmt.Errorf("snapshot commit db write error: %w", err) + } s.state.AddState(types.BytesToHash(root), nTrie) - return &Snapshot{trie: nTrie, state: s.state}, root + return &Snapshot{trie: nTrie, state: s.state}, root, nil } diff --git a/state/immutable-trie/state.go b/state/immutable-trie/state.go index 270a745928..95a9e9905d 100644 --- a/state/immutable-trie/state.go +++ b/state/immutable-trie/state.go @@ -42,8 +42,8 @@ func (s *State) newTrie() *Trie { return NewTrie() } -func (s *State) SetCode(hash types.Hash, code []byte) { - s.storage.SetCode(hash, code) +func (s *State) SetCode(hash types.Hash, code []byte) error { + return s.storage.SetCode(hash, code) } func (s *State) GetCode(hash types.Hash) ([]byte, bool) { diff --git a/state/immutable-trie/storage.go b/state/immutable-trie/storage.go index e1df83378c..cb1f775cad 100644 --- a/state/immutable-trie/storage.go +++ b/state/immutable-trie/storage.go @@ -16,19 +16,25 @@ var parserPool fastrlp.ParserPool var ( // codePrefix is the code prefix for leveldb codePrefix = []byte("code") + + // leveldb not found error message + levelDBNotFoundMsg = "leveldb: not found" ) +// Batch is batch write interface type Batch interface { + // Put puts key and value into batch. It can not return error because actual writing is done with Write method Put(k, v []byte) - Write() + // Write writes all the key values pair previosly putted with Put method to the database + Write() error } // Storage stores the trie type Storage interface { - Put(k, v []byte) - Get(k []byte) ([]byte, bool) + Put(k, v []byte) error + Get(k []byte) ([]byte, bool, error) Batch() Batch - SetCode(hash types.Hash, code []byte) + SetCode(hash types.Hash, code []byte) error GetCode(hash types.Hash) ([]byte, bool) Close() error @@ -49,37 +55,42 @@ func (b *KVBatch) Put(k, v []byte) { b.batch.Put(k, v) } -func (b *KVBatch) Write() { - _ = b.db.Write(b.batch, nil) +func (b *KVBatch) Write() error { + return b.db.Write(b.batch, nil) } -func (kv *KVStorage) SetCode(hash types.Hash, code []byte) { - kv.Put(append(codePrefix, hash.Bytes()...), code) +func (kv *KVStorage) SetCode(hash types.Hash, code []byte) error { + return kv.Put(GetCodeKey(hash), code) } func (kv *KVStorage) GetCode(hash types.Hash) ([]byte, bool) { - return kv.Get(append(codePrefix, hash.Bytes()...)) + res, ok, err := kv.Get(GetCodeKey(hash)) + if err != nil { + return nil, false + } + + return res, ok } func (kv *KVStorage) Batch() Batch { return &KVBatch{db: kv.db, batch: &leveldb.Batch{}} } -func (kv *KVStorage) Put(k, v []byte) { - _ = kv.db.Put(k, v, nil) +func (kv *KVStorage) Put(k, v []byte) error { + return kv.db.Put(k, v, nil) } -func (kv *KVStorage) Get(k []byte) ([]byte, bool) { +func (kv *KVStorage) Get(k []byte) ([]byte, bool, error) { data, err := kv.db.Get(k, nil) if err != nil { - if err.Error() == "leveldb: not found" { - return nil, false - } else { - panic(err) //nolint:gocritic + if err.Error() == levelDBNotFoundMsg { + return nil, false, nil } + + return nil, false, err } - return data, true + return data, true, nil } func (kv *KVStorage) Close() error { @@ -111,41 +122,40 @@ func NewMemoryStorage() Storage { return &memStorage{db: map[string][]byte{}, code: map[string][]byte{}, l: new(sync.Mutex)} } -func (m *memStorage) Put(p []byte, v []byte) { +func (m *memStorage) Put(p []byte, v []byte) error { m.l.Lock() defer m.l.Unlock() buf := make([]byte, len(v)) copy(buf[:], v[:]) m.db[hex.EncodeToHex(p)] = buf + + return nil } -func (m *memStorage) Get(p []byte) ([]byte, bool) { +func (m *memStorage) Get(p []byte) ([]byte, bool, error) { m.l.Lock() defer m.l.Unlock() v, ok := m.db[hex.EncodeToHex(p)] if !ok { - return []byte{}, false + return []byte{}, false, nil } - return v, true + return v, true, nil } -func (m *memStorage) SetCode(hash types.Hash, code []byte) { - m.l.Lock() - defer m.l.Unlock() - - m.code[hash.String()] = code +func (m *memStorage) SetCode(hash types.Hash, code []byte) error { + return m.Put(append(codePrefix, hash.Bytes()...), code) } func (m *memStorage) GetCode(hash types.Hash) ([]byte, bool) { - m.l.Lock() - defer m.l.Unlock() - - code, ok := m.code[hash.String()] + res, ok, err := m.Get(GetCodeKey(hash)) + if err != nil { + return nil, false + } - return code, ok + return res, ok } func (m *memStorage) Batch() Batch { @@ -165,14 +175,15 @@ func (m *memBatch) Put(p, v []byte) { (*m.db)[hex.EncodeToHex(p)] = buf } -func (m *memBatch) Write() { +func (m *memBatch) Write() error { + return nil } // GetNode retrieves a node from storage func GetNode(root []byte, storage Storage) (Node, bool, error) { - data, ok := storage.Get(root) - if !ok || len(data) == 0 { - return nil, false, nil + data, ok, err := storage.Get(root) + if err != nil || !ok || len(data) == 0 { + return nil, false, err } // NOTE. We dont need to make copies of the bytes because the nodes @@ -263,3 +274,7 @@ func decodeNode(v *fastrlp.Value, s Storage) (Node, error) { return nil, fmt.Errorf("node has incorrect number of leafs") } + +func GetCodeKey(hash types.Hash) []byte { + return append(codePrefix, hash.Bytes()...) +} diff --git a/state/state.go b/state/state.go index 010c83726f..f37d6ff255 100644 --- a/state/state.go +++ b/state/state.go @@ -20,7 +20,7 @@ type State interface { type Snapshot interface { readSnapshot - Commit(objs []*Object) (Snapshot, []byte) + Commit(objs []*Object) (Snapshot, []byte, error) } // Account is the account reference in the ethereum state diff --git a/state/testing.go b/state/testing.go index 44d7af1e7d..977b12ebeb 100644 --- a/state/testing.go +++ b/state/testing.go @@ -140,7 +140,9 @@ func testDeleteCommonStateRoot(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(false) require.NoError(t, err) - snap2, _ := snap.Commit(objs) + snap2, _, err := snap.Commit(objs) + require.NoError(t, err) + txn2 := newTxn(snap2) txn2.SetState(addr1, hash0, hash0) @@ -149,7 +151,8 @@ func testDeleteCommonStateRoot(t *testing.T, buildPreState buildPreState) { objs, err = txn2.Commit(false) require.NoError(t, err) - snap3, _ := snap2.Commit(objs) + snap3, _, err := snap2.Commit(objs) + require.NoError(t, err) txn3 := newTxn(snap3) assert.Equal(t, hash1, txn3.GetState(addr1, hash2)) @@ -173,7 +176,8 @@ func testWriteState(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(false) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.Equal(t, hash1, txn.GetState(addr1, hash1)) @@ -192,7 +196,8 @@ func testWriteEmptyState(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(false) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.True(t, txn.Exist(addr1)) @@ -206,7 +211,8 @@ func testWriteEmptyState(t *testing.T, buildPreState buildPreState) { objs, err = txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.False(t, txn.Exist(addr1)) @@ -224,7 +230,8 @@ func testUpdateStateWithEmpty(t *testing.T, buildPreState buildPreState, deleteE objs, err := txn.Commit(deleteEmptyObjects) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.Equal(t, !deleteEmptyObjects, txn.Exist(addr1)) @@ -236,7 +243,8 @@ func testUpdateStateWithEmpty(t *testing.T, buildPreState buildPreState, deleteE objs, err = txn.Commit(deleteEmptyObjects) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.Equal(t, true, txn.Exist(addr1)) @@ -249,7 +257,8 @@ func testUpdateStateWithEmpty(t *testing.T, buildPreState buildPreState, deleteE objs, err = txn.Commit(deleteEmptyObjects) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.Equal(t, true, txn.Exist(addr1)) @@ -267,7 +276,8 @@ func testSuicideAccountInPreState(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.False(t, txn.Exist(addr1)) @@ -288,7 +298,8 @@ func testSuicideAccount(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.False(t, txn.Exist(addr1)) @@ -313,7 +324,8 @@ func testSuicideAccountWithData(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) @@ -339,7 +351,8 @@ func testSuicideCoinbase(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.Equal(t, big.NewInt(10), txn.GetBalance(addr1)) @@ -399,7 +412,8 @@ func testChangePrestateAccountBalanceToZero(t *testing.T, buildPreState buildPre objs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.False(t, txn.Exist(addr1)) @@ -417,7 +431,8 @@ func testChangeAccountBalanceToZero(t *testing.T, buildPreState buildPreState) { objs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(objs) + snap, _, err = snap.Commit(objs) + require.NoError(t, err) txn = newTxn(snap) assert.False(t, txn.Exist(addr1)) @@ -447,7 +462,9 @@ func testSetAndGetCode(t *testing.T, buildPreState buildPreState) { affectedObjs, err := txn.Commit(true) require.NoError(t, err) - snap, _ = snap.Commit(affectedObjs) + snap, _, err = snap.Commit(affectedObjs) + require.NoError(t, err) + assert.Len(t, affectedObjs, 1) code, ok := snap.GetCode(affectedObjs[0].CodeHash) diff --git a/state/txn_test.go b/state/txn_test.go index 955424cea6..0353442f16 100644 --- a/state/txn_test.go +++ b/state/txn_test.go @@ -47,8 +47,8 @@ func (m *mockSnapshot) GetCode(hash types.Hash) ([]byte, bool) { return nil, false } -func (m *mockSnapshot) Commit(objs []*Object) (Snapshot, []byte) { - return nil, nil +func (m *mockSnapshot) Commit(objs []*Object) (Snapshot, []byte, error) { + return nil, nil, nil } func newStateWithPreState(preState map[types.Address]*PreState) Snapshot { diff --git a/tests/state_test.go b/tests/state_test.go index 83760eefa5..3aff28d6a9 100644 --- a/tests/state_test.go +++ b/tests/state_test.go @@ -94,7 +94,8 @@ func RunSpecificTest(t *testing.T, file string, c testCase, name, fork string, i objs, err := txn.Commit(forks.EIP155) require.NoError(t, err) - _, root := snapshot.Commit(objs) + _, root, err := snapshot.Commit(objs) + require.NoError(t, err) // Check block root if !bytes.Equal(root, p.Root.Bytes()) { diff --git a/tests/testing.go b/tests/testing.go index acc0a38290..032837e3e1 100644 --- a/tests/testing.go +++ b/tests/testing.go @@ -211,9 +211,9 @@ func buildState( return nil, nil, types.ZeroHash, err } - snap, root := snap.Commit(objs) + snap, root, err := snap.Commit(objs) - return s, snap, types.BytesToHash(root), nil + return s, snap, types.BytesToHash(root), err } type indexes struct {