perf: improve state reader with error handling and committed flag #27428

XDCx/tradingstate/XDCx_trie.go

@@ -186,8 +186,37 @@ func (t *XDCXTrie) Copy() *XDCXTrie {
 // NodeIterator returns an iterator that returns nodes of the underlying trie. Iteration
 // starts at the key after the given start key.
 func (t *XDCXTrie) NodeIterator(start []byte) trie.NodeIterator {
-	return t.trie.NodeIterator(start)
+	trieIt, err := t.trie.NodeIterator(start)
+	if err != nil {
+		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
+		return errNodeIterator{err: err}
+	}
+	return trieIt
+}
+
+// errNodeIterator is a safe, non-nil iterator that reports an error and yields no nodes.
+// It prevents nil dereferences when callers don't check for a nil iterator.
+type errNodeIterator struct {
+	err error
+}
+
+func (it errNodeIterator) Next(bool) bool { return false }
+func (it errNodeIterator) Error() error   { return it.err }
+func (it errNodeIterator) Hash() common.Hash {
+	return common.Hash{}
+}
+func (it errNodeIterator) Parent() common.Hash {
+	return common.Hash{}
+}
+func (it errNodeIterator) Path() []byte     { return nil }
+func (it errNodeIterator) NodeBlob() []byte { return nil }
+func (it errNodeIterator) Leaf() bool       { return false }
+func (it errNodeIterator) LeafKey() []byte  { return nil }
+func (it errNodeIterator) LeafBlob() []byte { return nil }
+func (it errNodeIterator) LeafProof() [][]byte {
+	return nil
 }
+func (it errNodeIterator) AddResolver(trie.NodeResolver) {}
 
 // hashKey returns the hash of key as an ephemeral buffer.
 // The caller must not hold onto the return value because it will become

XDCxlending/lendingstate/XDCx_trie.go

@@ -182,8 +182,37 @@ func (t *XDCXTrie) Copy() *XDCXTrie {
 // NodeIterator returns an iterator that returns nodes of the underlying trie. Iteration
 // starts at the key after the given start key.
 func (t *XDCXTrie) NodeIterator(start []byte) trie.NodeIterator {
-	return t.trie.NodeIterator(start)
+	trieIt, err := t.trie.NodeIterator(start)
+	if err != nil {
+		log.Error(fmt.Sprintf("Unhandled trie error: %v", err))
+		return errNodeIterator{err: err}
+	}
+	return trieIt
+}
+
+// errNodeIterator is a safe, non-nil iterator that reports an error and yields no nodes.
+// It prevents nil dereferences when callers don't check for a nil iterator.
+type errNodeIterator struct {
+	err error
+}
+
+func (it errNodeIterator) Next(bool) bool { return false }
+func (it errNodeIterator) Error() error   { return it.err }
+func (it errNodeIterator) Hash() common.Hash {
+	return common.Hash{}
+}
+func (it errNodeIterator) Parent() common.Hash {
+	return common.Hash{}
+}
+func (it errNodeIterator) Path() []byte     { return nil }
+func (it errNodeIterator) NodeBlob() []byte { return nil }
+func (it errNodeIterator) Leaf() bool       { return false }
+func (it errNodeIterator) LeafKey() []byte  { return nil }
+func (it errNodeIterator) LeafBlob() []byte { return nil }
+func (it errNodeIterator) LeafProof() [][]byte {
+	return nil
 }
+func (it errNodeIterator) AddResolver(trie.NodeResolver) {}
 
 // hashKey returns the hash of key as an ephemeral buffer.
 // The caller must not hold onto the return value because it will become

cmd/XDC/dbcmd.go

@@ -390,8 +390,12 @@ func dbDumpTrie(ctx *cli.Context) error {
 	if err != nil {
 		return err
 	}
+	trieIt, err := theTrie.NodeIterator(start)
+	if err != nil {
+		return err
+	}
 	var count int64
-	it := trie.NewIterator(theTrie.NodeIterator(start))
+	it := trie.NewIterator(trieIt)
 	for it.Next() {
 		if max > 0 && count == max {
 			fmt.Printf("Exiting after %d values\n", count)

core/state/database.go

@@ -113,8 +113,9 @@ type Trie interface {
 	Commit(collectLeaf bool) (common.Hash, *trienode.NodeSet)
 
 	// NodeIterator returns an iterator that returns nodes of the trie. Iteration
-	// starts at the key after the given start key.
-	NodeIterator(startKey []byte) trie.NodeIterator
+	// starts at the key after the given start key. An error will be returned
+	// if fails to create node iterator.
+	NodeIterator(startKey []byte) (trie.NodeIterator, error)
 
 	// Prove constructs a Merkle proof for key. The result contains all encoded nodes
 	// on the path to the value at key. The value itself is also included in the last

core/state/dump.go

@@ -139,7 +139,11 @@ func (s *StateDB) DumpToCollector(c DumpCollector, conf *DumpConfig) (nextKey []
 	log.Info("Trie dumping started", "root", s.trie.Hash())
 	c.OnRoot(s.trie.Hash())
 
-	it := trie.NewIterator(s.trie.NodeIterator(conf.Start))
+	trieIt, err := s.trie.NodeIterator(conf.Start)
+	if err != nil {
+		return nil
+	}
+	it := trie.NewIterator(trieIt)
 	for it.Next() {
 		var data types.StateAccount
 		if err := rlp.DecodeBytes(it.Value, &data); err != nil {
@@ -177,7 +181,12 @@ func (s *StateDB) DumpToCollector(c DumpCollector, conf *DumpConfig) (nextKey []
 				log.Error("Failed to load storage trie", "err", err)
 				continue
 			}
-			storageIt := trie.NewIterator(tr.NodeIterator(nil))
+			trieIt, err := tr.NodeIterator(nil)
+			if err != nil {
+				log.Error("Failed to create trie iterator", "err", err)
+				continue
+			}
+			storageIt := trie.NewIterator(trieIt)
 			for storageIt.Next() {
 				_, content, _, err := rlp.Split(storageIt.Value)
 				if err != nil {

core/state/iterator.go

@@ -74,8 +74,12 @@ func (it *nodeIterator) step() error {
 		return nil
 	}
 	// Initialize the iterator if we've just started
+	var err error
 	if it.stateIt == nil {
-		it.stateIt = it.state.trie.NodeIterator(nil)
+		it.stateIt, err = it.state.trie.NodeIterator(nil)
+		if err != nil {
+			return err
+		}
 	}
 	// If we had data nodes previously, we surely have at least state nodes
 	if it.dataIt != nil {
@@ -113,7 +117,10 @@ func (it *nodeIterator) step() error {
 	if err != nil {
 		return err
 	}
-	it.dataIt = dataTrie.NodeIterator(nil)
+	it.dataIt, err = dataTrie.NodeIterator(nil)
+	if err != nil {
+		return err
+	}
 	if !it.dataIt.Next(true) {
 		it.dataIt = nil
 	}

core/state/state_test.go

@@ -43,7 +43,8 @@ func newStateTest() *stateTest {
 
 func TestDump(t *testing.T) {
 	db := rawdb.NewMemoryDatabase()
-	sdb, _ := New(types.EmptyRootHash, NewDatabaseWithConfig(db, &trie.Config{Preimages: true}))
+	tdb := NewDatabaseWithConfig(db, &trie.Config{Preimages: true})
+	sdb, _ := New(types.EmptyRootHash, tdb)
 	s := &stateTest{db: db, state: sdb}
 
 	// generate a few entries
@@ -57,9 +58,10 @@ func TestDump(t *testing.T) {
 	// write some of them to the trie
 	s.state.updateStateObject(obj1)
 	s.state.updateStateObject(obj2)
-	s.state.Commit(0, false)
+	root, _ := s.state.Commit(0, false)
 
 	// check that DumpToCollector contains the state objects that are in trie
+	s.state, _ = New(root, tdb)
 	got := string(s.state.Dump(nil))
 	want := `{
     "root": "71edff0130dd2385947095001c73d9e28d862fc286fca2b922ca6f6f3cddfdd2",
@@ -95,7 +97,8 @@ func TestDump(t *testing.T) {
 
 func TestIterativeDump(t *testing.T) {
 	db := rawdb.NewMemoryDatabase()
-	sdb, _ := New(types.EmptyRootHash, NewDatabaseWithConfig(db, &trie.Config{Preimages: true}))
+	tdb := NewDatabaseWithConfig(db, &trie.Config{Preimages: true})
+	sdb, _ := New(types.EmptyRootHash, tdb)
 	s := &stateTest{db: db, state: sdb}
 
 	// generate a few entries
@@ -111,7 +114,8 @@ func TestIterativeDump(t *testing.T) {
 	// write some of them to the trie
 	s.state.updateStateObject(obj1)
 	s.state.updateStateObject(obj2)
-	s.state.Commit(0, false)
+	root, _ := s.state.Commit(0, false)
+	s.state, _ = New(root, tdb)
 
 	b := &bytes.Buffer{}
 	s.state.IterativeDump(nil, json.NewEncoder(b))

core/state/statedb.go

@@ -45,8 +45,14 @@ type revision struct {
 // StateDB structs within the ethereum protocol are used to store anything
 // within the merkle trie. StateDBs take care of caching and storing
 // nested states. It's the general query interface to retrieve:
+//
 // * Contracts
 // * Accounts
+//
+// Once the state is committed, tries cached in stateDB (including account
+// trie, storage tries) will no longer be functional. A new state instance
+// must be created with new root and updated database for accessing post-
+// commit states.
 type StateDB struct {
 	db   Database
 	trie Trie
@@ -678,7 +684,11 @@ func (s *StateDB) ForEachStorage(addr common.Address, cb func(key, value common.
 	if err != nil {
 		return err
 	}
-	it := trie.NewIterator(tr.NodeIterator(nil))
+	trieIt, err := tr.NodeIterator(nil)
+	if err != nil {
+		return err
+	}
+	it := trie.NewIterator(trieIt)
 
 	for it.Next() {
 		key := common.BytesToHash(s.trie.GetKey(it.Key))

core/state/statedb_test.go

@@ -19,6 +19,7 @@ package state
 import (
 	"bytes"
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"math"
 	"math/big"
@@ -33,6 +34,7 @@ import (
 	"github.com/XinFinOrg/XDPoSChain/core/rawdb"
 	"github.com/XinFinOrg/XDPoSChain/core/tracing"
 	"github.com/XinFinOrg/XDPoSChain/core/types"
+	"github.com/XinFinOrg/XDPoSChain/trie"
 )
 
 // Tests that updating a state trie does not leak any database writes prior to
@@ -514,6 +516,51 @@ func TestTouchDelete(t *testing.T) {
 	}
 }
 
+// TestCommitCopy tests the copy from a committed state is not functional.
+func TestCommitCopy(t *testing.T) {
+	state, _ := New(types.EmptyRootHash, NewDatabase(rawdb.NewMemoryDatabase()))
+
+	// Create an account and check if the retrieved balance is correct
+	addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe")
+	skey := common.HexToHash("aaa")
+	sval := common.HexToHash("bbb")
+
+	state.SetBalance(addr, big.NewInt(42), tracing.BalanceChangeUnspecified) // Change the account trie
+	state.SetCode(addr, []byte("hello"))                                     // Change an external metadata
+	state.SetState(addr, skey, sval)                                         // Change the storage trie
+
+	if balance := state.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 {
+		t.Fatalf("initial balance mismatch: have %v, want %v", balance, 42)
+	}
+	if code := state.GetCode(addr); !bytes.Equal(code, []byte("hello")) {
+		t.Fatalf("initial code mismatch: have %x, want %x", code, []byte("hello"))
+	}
+	if val := state.GetState(addr, skey); val != sval {
+		t.Fatalf("initial non-committed storage slot mismatch: have %x, want %x", val, sval)
+	}
+	if val := state.GetCommittedState(addr, skey); val != (common.Hash{}) {
+		t.Fatalf("initial committed storage slot mismatch: have %x, want %x", val, common.Hash{})
+	}
+	// Copy the committed state database, the copied one is not functional.
+	state.Commit(0, true)
+	copied := state.Copy()
+	if balance := copied.GetBalance(addr); balance.Cmp(big.NewInt(0)) != 0 {
+		t.Fatalf("unexpected balance: have %v", balance)
+	}
+	if code := copied.GetCode(addr); code != nil {
+		t.Fatalf("unexpected code: have %x", code)
+	}
+	if val := copied.GetState(addr, skey); val != (common.Hash{}) {
+		t.Fatalf("unexpected storage slot: have %x", val)
+	}
+	if val := copied.GetCommittedState(addr, skey); val != (common.Hash{}) {
+		t.Fatalf("unexpected storage slot: have %x", val)
+	}
+	if !errors.Is(copied.Error(), trie.ErrCommitted) {
+		t.Fatalf("unexpected state error, %v", copied.Error())
+	}
+}
+
 func TestStateDBAccessList(t *testing.T) {
 	// Some helpers
 	addr := func(a string) common.Address {
@@ -777,7 +824,8 @@ func TestCopyOfCopy(t *testing.T) {
 //
 // See https://github.com/ethereum/go-ethereum/issues/20106.
 func TestCopyCommitCopy(t *testing.T) {
-	state, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase()))
+	tdb := NewDatabase(rawdb.NewMemoryDatabase())
+	state, _ := New(types.EmptyRootHash, tdb)
 
 	// Create an account and check if the retrieved balance is correct
 	addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe")
@@ -814,20 +862,6 @@ func TestCopyCommitCopy(t *testing.T) {
 	if val := copyOne.GetCommittedState(addr, skey); val != (common.Hash{}) {
 		t.Fatalf("first copy pre-commit committed storage slot mismatch: have %x, want %x", val, common.Hash{})
 	}
-
-	copyOne.Commit(0, false)
-	if balance := copyOne.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 {
-		t.Fatalf("first copy post-commit balance mismatch: have %v, want %v", balance, 42)
-	}
-	if code := copyOne.GetCode(addr); !bytes.Equal(code, []byte("hello")) {
-		t.Fatalf("first copy post-commit code mismatch: have %x, want %x", code, []byte("hello"))
-	}
-	if val := copyOne.GetState(addr, skey); val != sval {
-		t.Fatalf("first copy post-commit non-committed storage slot mismatch: have %x, want %x", val, sval)
-	}
-	if val := copyOne.GetCommittedState(addr, skey); val != sval {
-		t.Fatalf("first copy post-commit committed storage slot mismatch: have %x, want %x", val, sval)
-	}
 	// Copy the copy and check the balance once more
 	copyTwo := copyOne.Copy()
 	if balance := copyTwo.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 {
@@ -839,8 +873,23 @@ func TestCopyCommitCopy(t *testing.T) {
 	if val := copyTwo.GetState(addr, skey); val != sval {
 		t.Fatalf("second copy non-committed storage slot mismatch: have %x, want %x", val, sval)
 	}
-	if val := copyTwo.GetCommittedState(addr, skey); val != sval {
-		t.Fatalf("second copy post-commit committed storage slot mismatch: have %x, want %x", val, sval)
+	if val := copyTwo.GetCommittedState(addr, skey); val != (common.Hash{}) {
+		t.Fatalf("second copy committed storage slot mismatch: have %x, want %x", val, sval)
+	}
+	// Commit state, ensure states can be loaded from disk
+	root, _ := state.Commit(0, false)
+	state, _ = New(root, tdb)
+	if balance := state.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 {
+		t.Fatalf("state post-commit balance mismatch: have %v, want %v", balance, 42)
+	}
+	if code := state.GetCode(addr); !bytes.Equal(code, []byte("hello")) {
+		t.Fatalf("state post-commit code mismatch: have %x, want %x", code, []byte("hello"))
+	}
+	if val := state.GetState(addr, skey); val != sval {
+		t.Fatalf("state post-commit non-committed storage slot mismatch: have %x, want %x", val, sval)
+	}
+	if val := state.GetCommittedState(addr, skey); val != sval {
+		t.Fatalf("state post-commit committed storage slot mismatch: have %x, want %x", val, sval)
 	}
 }
 
@@ -849,7 +898,7 @@ func TestCopyCommitCopy(t *testing.T) {
 //
 // See https://github.com/ethereum/go-ethereum/issues/20106.
 func TestCopyCopyCommitCopy(t *testing.T) {
-	state, _ := New(common.Hash{}, NewDatabase(rawdb.NewMemoryDatabase()))
+	state, _ := New(types.EmptyRootHash, NewDatabase(rawdb.NewMemoryDatabase()))
 
 	// Create an account and check if the retrieved balance is correct
 	addr := common.HexToAddress("0xaffeaffeaffeaffeaffeaffeaffeaffeaffeaffe")
@@ -900,19 +949,6 @@ func TestCopyCopyCommitCopy(t *testing.T) {
 	if val := copyTwo.GetCommittedState(addr, skey); val != (common.Hash{}) {
 		t.Fatalf("second copy pre-commit committed storage slot mismatch: have %x, want %x", val, common.Hash{})
 	}
-	copyTwo.Commit(0, false)
-	if balance := copyTwo.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 {
-		t.Fatalf("second copy post-commit balance mismatch: have %v, want %v", balance, 42)
-	}
-	if code := copyTwo.GetCode(addr); !bytes.Equal(code, []byte("hello")) {
-		t.Fatalf("second copy post-commit code mismatch: have %x, want %x", code, []byte("hello"))
-	}
-	if val := copyTwo.GetState(addr, skey); val != sval {
-		t.Fatalf("second copy post-commit non-committed storage slot mismatch: have %x, want %x", val, sval)
-	}
-	if val := copyTwo.GetCommittedState(addr, skey); val != sval {
-		t.Fatalf("second copy post-commit committed storage slot mismatch: have %x, want %x", val, sval)
-	}
 	// Copy the copy-copy and check the balance once more
 	copyThree := copyTwo.Copy()
 	if balance := copyThree.GetBalance(addr); balance.Cmp(big.NewInt(42)) != 0 {
@@ -924,7 +960,7 @@ func TestCopyCopyCommitCopy(t *testing.T) {
 	if val := copyThree.GetState(addr, skey); val != sval {
 		t.Fatalf("third copy non-committed storage slot mismatch: have %x, want %x", val, sval)
 	}
-	if val := copyThree.GetCommittedState(addr, skey); val != sval {
+	if val := copyThree.GetCommittedState(addr, skey); val != (common.Hash{}) {
 		t.Fatalf("third copy committed storage slot mismatch: have %x, want %x", val, sval)
 	}
 }