From 566818c629ee349fedbe02523f0117a5519324a9 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 17 Oct 2023 09:14:11 +0200 Subject: [PATCH 1/5] trie: avoid panic in stacktrie, return errors instead --- core/state/snapshot/generate.go | 4 +++- trie/stacktrie.go | 25 +++++++++++++++---------- trie/stacktrie_test.go | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/core/state/snapshot/generate.go b/core/state/snapshot/generate.go index 204584c956ea..adeaa1daa01e 100644 --- a/core/state/snapshot/generate.go +++ b/core/state/snapshot/generate.go @@ -230,7 +230,9 @@ func (dl *diskLayer) proveRange(ctx *generatorContext, trieId *trie.ID, prefix [ if origin == nil && !diskMore { stackTr := trie.NewStackTrie(nil) for i, key := range keys { - stackTr.Update(key, vals[i]) + if err := stackTr.Update(key, vals[i]); err != nil { + return nil, err + } } if gotRoot := stackTr.Hash(); gotRoot != root { return &proofResult{ diff --git a/trie/stacktrie.go b/trie/stacktrie.go index 423afdec8816..fd08898b5823 100644 --- a/trie/stacktrie.go +++ b/trie/stacktrie.go @@ -18,6 +18,7 @@ package trie import ( "bytes" + "errors" "sync" "github.com/ethereum/go-ethereum/common" @@ -92,22 +93,26 @@ func NewStackTrie(options *StackTrieOptions) *StackTrie { // Update inserts a (key, value) pair into the stack trie. func (t *StackTrie) Update(key, value []byte) error { - k := keybytesToHex(key) if len(value) == 0 { - panic("deletion not supported") + return errors.New("trying to insert empty (deletion)") } + k := keybytesToHex(key) k = k[:len(k)-1] // chop the termination flag - // track the first and last inserted entries. if t.first == nil { t.first = append([]byte{}, k...) } + if bytes.Compare(t.last, k) >= 0 { + return errors.New("non-ascending key order") + } if t.last == nil { t.last = append([]byte{}, k...) // allocate key slice } else { t.last = append(t.last[:0], k...) // reuse key slice } - t.insert(t.root, k, value, nil) + if err := t.insert(t.root, k, value, nil); err != nil { + return err + } return nil } @@ -189,7 +194,7 @@ func (n *stNode) getDiffIndex(key []byte) int { // Helper function to that inserts a (key, value) pair into // the trie. -func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) { +func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) error { switch st.typ { case branchNode: /* Branch */ idx := int(key[0]) @@ -208,7 +213,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) { if st.children[idx] == nil { st.children[idx] = newLeaf(key[1:], value) } else { - t.insert(st.children[idx], key[1:], value, append(path, key[0])) + return t.insert(st.children[idx], key[1:], value, append(path, key[0])) } case extNode: /* Ext */ @@ -223,8 +228,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) { if diffidx == len(st.key) { // Ext key and key segment are identical, recurse into // the child node. - t.insert(st.children[0], key[diffidx:], value, append(path, key[:diffidx]...)) - return + return t.insert(st.children[0], key[diffidx:], value, append(path, key[:diffidx]...)) } // Save the original part. Depending if the break is // at the extension's last byte or not, create an @@ -282,7 +286,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) { // keys differ, and 3) one leaf for the differentiated // component of each key. if diffidx >= len(st.key) { - panic("Trying to insert into existing key") + return errors.New("trying to insert into existing key") } // Check if the split occurs at the first nibble of the @@ -324,11 +328,12 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) { st.val = value case hashedNode: - panic("trying to insert into hash") + return errors.New("trying to insert into hash") default: panic("invalid type") } + return nil } // hash converts st into a 'hashedNode', if possible. Possible outcomes: diff --git a/trie/stacktrie_test.go b/trie/stacktrie_test.go index 629586e2b1bf..a496638d9982 100644 --- a/trie/stacktrie_test.go +++ b/trie/stacktrie_test.go @@ -26,6 +26,7 @@ import ( "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/trie/testutil" + "github.com/stretchr/testify/assert" "golang.org/x/exp/slices" ) @@ -463,3 +464,24 @@ func TestPartialStackTrie(t *testing.T) { } } } + +func TestStacstaturieErrors(t *testing.T) { + s := NewStackTrie(nil) + // Deletion + if err := s.Update(nil, nil); err == nil { + t.Fatal("expected error") + } + if err := s.Update(nil, []byte{}); err == nil { + t.Fatal("expected error") + } + if err := s.Update([]byte{0xa}, []byte{}); err == nil { + t.Fatal("expected error") + } + // Non-ascending keys (going backwards or repeating) + assert.Nil(t, s.Update([]byte{0xaa}, []byte{0xa})) + assert.NotNil(t, s.Update([]byte{0xaa}, []byte{0xa}), "repeat insert same key") + assert.NotNil(t, s.Update([]byte{0xaa}, []byte{0xb}), "repeat insert same key") + assert.Nil(t, s.Update([]byte{0xab}, []byte{0xa})) + assert.NotNil(t, s.Update([]byte{0x10}, []byte{0xb}), "out of order insert") + assert.NotNil(t, s.Update([]byte{0xaa}, []byte{0xb}), "repeat insert same key") +} From c0ae61fa15dacfe3139c36acc21e16c4e36b668d Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Mon, 23 Oct 2023 18:34:21 +0200 Subject: [PATCH 2/5] trie: do error-checks prior to setting last/first in stacktrie --- trie/stacktrie.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/trie/stacktrie.go b/trie/stacktrie.go index fd08898b5823..42bd31acc6fe 100644 --- a/trie/stacktrie.go +++ b/trie/stacktrie.go @@ -98,21 +98,21 @@ func (t *StackTrie) Update(key, value []byte) error { } k := keybytesToHex(key) k = k[:len(k)-1] // chop the termination flag + if bytes.Compare(t.last, k) >= 0 { + return errors.New("non-ascending key order") + } + if err := t.insert(t.root, k, value, nil); err != nil { + return err + } // track the first and last inserted entries. if t.first == nil { t.first = append([]byte{}, k...) } - if bytes.Compare(t.last, k) >= 0 { - return errors.New("non-ascending key order") - } if t.last == nil { t.last = append([]byte{}, k...) // allocate key slice } else { t.last = append(t.last[:0], k...) // reuse key slice } - if err := t.insert(t.root, k, value, nil); err != nil { - return err - } return nil } From 80768aae8a061c18556d4082ad44a5390629bba2 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 24 Oct 2023 09:15:31 +0200 Subject: [PATCH 3/5] trie: update markers prior to insert (stacktrie) --- trie/stacktrie.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/trie/stacktrie.go b/trie/stacktrie.go index 42bd31acc6fe..a607efbedefc 100644 --- a/trie/stacktrie.go +++ b/trie/stacktrie.go @@ -101,9 +101,6 @@ func (t *StackTrie) Update(key, value []byte) error { if bytes.Compare(t.last, k) >= 0 { return errors.New("non-ascending key order") } - if err := t.insert(t.root, k, value, nil); err != nil { - return err - } // track the first and last inserted entries. if t.first == nil { t.first = append([]byte{}, k...) @@ -113,6 +110,9 @@ func (t *StackTrie) Update(key, value []byte) error { } else { t.last = append(t.last[:0], k...) // reuse key slice } + if err := t.insert(t.root, k, value, nil); err != nil { + return err + } return nil } From f820914bf1a9cadd595333b77b64b270fdd801c5 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 24 Oct 2023 09:17:27 +0200 Subject: [PATCH 4/5] trie: fix typo --- trie/stacktrie_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/trie/stacktrie_test.go b/trie/stacktrie_test.go index a496638d9982..909a77062aab 100644 --- a/trie/stacktrie_test.go +++ b/trie/stacktrie_test.go @@ -465,7 +465,7 @@ func TestPartialStackTrie(t *testing.T) { } } -func TestStacstaturieErrors(t *testing.T) { +func TestStackTrieErrors(t *testing.T) { s := NewStackTrie(nil) // Deletion if err := s.Update(nil, nil); err == nil { From 7642dd8e7a65fb9ffe5b31782dd2158dd57950a8 Mon Sep 17 00:00:00 2001 From: Martin Holst Swende Date: Tue, 24 Oct 2023 14:13:11 +0200 Subject: [PATCH 5/5] trie: undo changes to insert --- trie/stacktrie.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/trie/stacktrie.go b/trie/stacktrie.go index a607efbedefc..f2f5355c49e8 100644 --- a/trie/stacktrie.go +++ b/trie/stacktrie.go @@ -110,9 +110,7 @@ func (t *StackTrie) Update(key, value []byte) error { } else { t.last = append(t.last[:0], k...) // reuse key slice } - if err := t.insert(t.root, k, value, nil); err != nil { - return err - } + t.insert(t.root, k, value, nil) return nil } @@ -194,7 +192,7 @@ func (n *stNode) getDiffIndex(key []byte) int { // Helper function to that inserts a (key, value) pair into // the trie. -func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) error { +func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) { switch st.typ { case branchNode: /* Branch */ idx := int(key[0]) @@ -213,7 +211,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) error { if st.children[idx] == nil { st.children[idx] = newLeaf(key[1:], value) } else { - return t.insert(st.children[idx], key[1:], value, append(path, key[0])) + t.insert(st.children[idx], key[1:], value, append(path, key[0])) } case extNode: /* Ext */ @@ -228,7 +226,8 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) error { if diffidx == len(st.key) { // Ext key and key segment are identical, recurse into // the child node. - return t.insert(st.children[0], key[diffidx:], value, append(path, key[:diffidx]...)) + t.insert(st.children[0], key[diffidx:], value, append(path, key[:diffidx]...)) + return } // Save the original part. Depending if the break is // at the extension's last byte or not, create an @@ -286,7 +285,7 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) error { // keys differ, and 3) one leaf for the differentiated // component of each key. if diffidx >= len(st.key) { - return errors.New("trying to insert into existing key") + panic("Trying to insert into existing key") } // Check if the split occurs at the first nibble of the @@ -328,12 +327,11 @@ func (t *StackTrie) insert(st *stNode, key, value []byte, path []byte) error { st.val = value case hashedNode: - return errors.New("trying to insert into hash") + panic("trying to insert into hash") default: panic("invalid type") } - return nil } // hash converts st into a 'hashedNode', if possible. Possible outcomes: