From b8d59d9c985feed9ec1d8851f65517c7e5c09deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Tue, 16 Feb 2016 12:37:00 +0200 Subject: [PATCH] core/state, trie: switch iterator panics to error fields --- core/state/iterator.go | 43 ++++++++++++++++++++++++++++------------- core/state/sync_test.go | 20 +++++++------------ trie/iterator.go | 26 ++++++++++++++++++------- trie/sync_test.go | 18 ++++++----------- 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.