core/state, trie: switch iterator panics to error fields

4 files changed, 62 insertions, 45 deletions

diff --git a/core/state/iterator.go b/core/state/iterator.go
index a051e5e01..9d8a69b7c 100644
--- a/core/state/iterator.go
+++ b/core/state/iterator.go
@@ -41,6 +41,8 @@ type NodeIterator struct {
 	Hash   common.Hash // Hash of the current entry being iterated (nil if not standalone)
 	Entry  interface{} // Current state entry being iterated (internal representation)
 	Parent common.Hash // Hash of the first full ancestor node (nil if current is the root)
+
+	Error error // Failure set in case of an internal error in the iterator
 }
 
 // NewNodeIterator creates an post-order state node iterator.
@@ -51,17 +53,26 @@ func NewNodeIterator(state *StateDB) *NodeIterator {
 }
 
 // Next moves the iterator to the next node, returning whether there are any
-// further nodes.
+// further nodes. In case of an internal error this method returns false and
+// sets the Error field to the encountered failure.
 func (it *NodeIterator) Next() bool {
-	it.step()
+	// If the iterator failed previously, don't do anything
+	if it.Error != nil {
+		return false
+	}
+	// Otherwise step forward with the iterator and report any errors
+	if err := it.step(); err != nil {
+		it.Error = err
+		return false
+	}
 	return it.retrieve()
 }
 
 // step moves the iterator to the next entry of the state trie.
-func (it *NodeIterator) step() {
+func (it *NodeIterator) step() error {
 	// Abort if we reached the end of the iteration
 	if it.state == nil {
-		return
+		return nil
 	}
 	// Initialize the iterator if we've just started
 	if it.stateIt == nil {
@@ -70,23 +81,29 @@ func (it *NodeIterator) step() {
 	// If we had data nodes previously, we surely have at least state nodes
 	if it.dataIt != nil {
 		if cont := it.dataIt.Next(); !cont {
+			if it.dataIt.Error != nil {
+				return it.dataIt.Error
+			}
 			it.dataIt = nil
 		}
-		return
+		return nil
 	}
 	// If we had source code previously, discard that
 	if it.code != nil {
 		it.code = nil
-		return
+		return nil
 	}
 	// Step to the next state trie node, terminating if we're out of nodes
 	if cont := it.stateIt.Next(); !cont {
+		if it.stateIt.Error != nil {
+			return it.stateIt.Error
+		}
 		it.state, it.stateIt = nil, nil
-		return
+		return nil
 	}
 	// If the state trie node is an internal entry, leave as is
 	if !it.stateIt.Leaf {
-		return
+		return nil
 	}
 	// Otherwise we've reached an account node, initiate data iteration
 	var account struct {
@@ -95,13 +112,12 @@ func (it *NodeIterator) step() {
 		Root     common.Hash
 		CodeHash []byte
 	}
-	err := rlp.Decode(bytes.NewReader(it.stateIt.LeafBlob), &account)
-	if err != nil {
-		panic(err)
+	if err := rlp.Decode(bytes.NewReader(it.stateIt.LeafBlob), &account); err != nil {
+		return err
 	}
 	dataTrie, err := trie.New(account.Root, it.state.db)
 	if err != nil {
-		panic(err)
+		return err
 	}
 	it.dataIt = trie.NewNodeIterator(dataTrie)
 	if !it.dataIt.Next() {
@@ -111,10 +127,11 @@ func (it *NodeIterator) step() {
 		it.codeHash = common.BytesToHash(account.CodeHash)
 		it.code, err = it.state.db.Get(account.CodeHash)
 		if err != nil {
-			panic(fmt.Sprintf("code %x: %v", account.CodeHash, err))
+			return fmt.Errorf("code %x: %v", account.CodeHash, err)
 		}
 	}
 	it.accountHash = it.stateIt.Parent
+	return nil
 }
 
 // retrieve pulls and caches the current state entry the iterator is traversing.
diff --git a/core/state/sync_test.go b/core/state/sync_test.go
index 727a276a5..a2a1edbdb 100644
--- a/core/state/sync_test.go
+++ b/core/state/sync_test.go
@@ -18,7 +18,6 @@ package state
 
 import (
 	"bytes"
-	"fmt"
 	"math/big"
 	"testing"
 
@@ -97,28 +96,23 @@ func checkStateAccounts(t *testing.T, db ethdb.Database, root common.Hash, accou
 	}
 }
 
-// checkStateConsistency checks that all nodes in a state trie and indeed present.
-func checkStateConsistency(db ethdb.Database, root common.Hash) (failure error) {
-	// Capture any panics by the iterator
-	defer func() {
-		if r := recover(); r != nil {
-			failure = fmt.Errorf("%v", r)
-		}
-	}()
+// checkStateConsistency checks that all nodes in a state trie are indeed present.
+func checkStateConsistency(db ethdb.Database, root common.Hash) error {
 	// Remove any potentially cached data from the test state creation or previous checks
 	trie.ClearGlobalCache()
 
 	// Create and iterate a state trie rooted in a sub-node
 	if _, err := db.Get(root.Bytes()); err != nil {
-		return
+		return nil // Consider a non existent state consistent
 	}
 	state, err := New(root, db)
 	if err != nil {
-		return
+		return err
 	}
-	for it := NewNodeIterator(state); it.Next(); {
+	it := NewNodeIterator(state)
+	for it.Next() {
 	}
-	return nil
+	return it.Error
 }
 
 // Tests that an empty state is not scheduled for syncing.
diff --git a/trie/iterator.go b/trie/iterator.go
index d43b77698..ceef52ec8 100644
--- a/trie/iterator.go
+++ b/trie/iterator.go
@@ -169,6 +169,8 @@ type NodeIterator struct {
 	Parent   common.Hash // Hash of the first full ancestor node (nil if current is the root)
 	Leaf     bool        // Flag whether the current node is a value (data) node
 	LeafBlob []byte      // Data blob contained within a leaf (otherwise nil)
+
+	Error error // Failure set in case of an internal error in the iterator
 }
 
 // NewNodeIterator creates an post-order trie iterator.
@@ -180,29 +182,38 @@ func NewNodeIterator(trie *Trie) *NodeIterator {
 }
 
 // Next moves the iterator to the next node, returning whether there are any
-// further nodes.
+// further nodes. In case of an internal error this method returns false and
+// sets the Error field to the encountered failure.
 func (it *NodeIterator) Next() bool {
-	it.step()
+	// If the iterator failed previously, don't do anything
+	if it.Error != nil {
+		return false
+	}
+	// Otherwise step forward with the iterator and report any errors
+	if err := it.step(); err != nil {
+		it.Error = err
+		return false
+	}
 	return it.retrieve()
 }
 
 // step moves the iterator to the next node of the trie.
-func (it *NodeIterator) step() {
+func (it *NodeIterator) step() error {
 	// Abort if we reached the end of the iteration
 	if it.trie == nil {
-		return
+		return nil
 	}
 	// Initialize the iterator if we've just started, or pop off the old node otherwise
 	if len(it.stack) == 0 {
 		it.stack = append(it.stack, &nodeIteratorState{node: it.trie.root, child: -1})
 		if it.stack[0].node == nil {
-			panic(fmt.Sprintf("root node missing: %x", it.trie.Root()))
+			return fmt.Errorf("root node missing: %x", it.trie.Root())
 		}
 	} else {
 		it.stack = it.stack[:len(it.stack)-1]
 		if len(it.stack) == 0 {
 			it.trie = nil
-			return
+			return nil
 		}
 	}
 	// Continue iteration to the next child
@@ -239,13 +250,14 @@ func (it *NodeIterator) step() {
 
 			node, err := it.trie.resolveHash(hash, nil, nil)
 			if err != nil {
-				panic(err)
+				return err
 			}
 			it.stack = append(it.stack, &nodeIteratorState{hash: common.BytesToHash(hash), node: node, parent: ancestor, child: -1})
 		} else {
 			break
 		}
 	}
+	return nil
 }
 
 // retrieve pulls and caches the current trie node the iterator is traversing.
diff --git a/trie/sync_test.go b/trie/sync_test.go
index ce098d5a0..a81f7650e 100644
--- a/trie/sync_test.go
+++ b/trie/sync_test.go
@@ -18,7 +18,6 @@ package trie
 
 import (
 	"bytes"
-	"fmt"
 	"testing"
 
 	"github.com/ethereum/go-ethereum/common"
@@ -80,25 +79,20 @@ func checkTrieContents(t *testing.T, db Database, root []byte, content map[strin
 	}
 }
 
-// checkTrieConsistency checks that all nodes in a trie and indeed present.
-func checkTrieConsistency(db Database, root common.Hash) (failure error) {
-	// Capture any panics by the iterator
-	defer func() {
-		if r := recover(); r != nil {
-			failure = fmt.Errorf("%v", r)
-		}
-	}()
+// checkTrieConsistency checks that all nodes in a trie are indeed present.
+func checkTrieConsistency(db Database, root common.Hash) error {
 	// Remove any potentially cached data from the test trie creation or previous checks
 	globalCache.Clear()
 
 	// Create and iterate a trie rooted in a subnode
 	trie, err := New(root, db)
 	if err != nil {
-		return
+		return nil // // Consider a non existent state consistent
 	}
-	for it := NewNodeIterator(trie); it.Next(); {
+	it := NewNodeIterator(trie)
+	for it.Next() {
 	}
-	return nil
+	return it.Error
 }
 
 // Tests that an empty trie is not scheduled for syncing.