From 7d25dfe6f8f7cf30151e85b83e43a5d76a7082e3 Mon Sep 17 00:00:00 2001 From: Roy Crihfield Date: Wed, 2 Aug 2023 20:49:14 +0800 Subject: [PATCH 1/7] node iterator seek: compare paths without terminator byte --- trie/iterator.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/trie/iterator.go b/trie/iterator.go index 83ccc0740f3a..214db1c2929b 100644 --- a/trie/iterator.go +++ b/trie/iterator.go @@ -311,7 +311,7 @@ func (it *nodeIterator) seek(prefix []byte) error { return errIteratorEnd } else if err != nil { return seekError{prefix, err} - } else if bytes.Compare(path, key) >= 0 { + } else if reachedPath(path, key) { return nil } it.push(state, parentIndex, path) @@ -554,6 +554,15 @@ func (it *nodeIterator) pop() { it.putInPool(last) } +// reachedPath normalizes a path by truncating a terminator if present, and returns true if it is +// greater than or equal to the target. +func reachedPath(path, target []byte) bool { + if hasTerm(path) { + path = path[:len(path)-1] + } + return bytes.Compare(path, target) >= 0 +} + func compareNodes(a, b NodeIterator) int { if cmp := bytes.Compare(a.Path(), b.Path()); cmp != 0 { return cmp From 18e4da21de31143209ae19a61f78d6f5a4d54758 Mon Sep 17 00:00:00 2001 From: Roy Crihfield Date: Thu, 3 Aug 2023 17:18:12 +0800 Subject: [PATCH 2/7] unit test for seeking prefixed key --- trie/iterator_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/trie/iterator_test.go b/trie/iterator_test.go index 9679b49ca7db..5540c907cd00 100644 --- a/trie/iterator_test.go +++ b/trie/iterator_test.go @@ -227,6 +227,12 @@ func TestIteratorSeek(t *testing.T) { if err := checkIteratorOrder(nil, it); err != nil { t.Fatal(err) } + + // Seek to a key for which a prefixing key exists. + it = NewIterator(trie.MustNodeIterator([]byte("food"))) + if err := checkIteratorOrder(testdata1[5:], it); err != nil { + t.Fatal(err) + } } func checkIteratorOrder(want []kvs, it *Iterator) error { From c0ca60b3f4c1d5266a116bc503dad629065589bf Mon Sep 17 00:00:00 2001 From: Roy Crihfield Date: Thu, 3 Aug 2023 13:57:32 +0800 Subject: [PATCH 3/7] yield value nodes pre-order --- trie/iterator.go | 42 ++++++++++++++++++++++++++++++++++-------- trie/iterator_test.go | 14 +++++++------- 2 files changed, 41 insertions(+), 15 deletions(-) diff --git a/trie/iterator.go b/trie/iterator.go index 214db1c2929b..5338aed8e7d0 100644 --- a/trie/iterator.go +++ b/trie/iterator.go @@ -449,7 +449,7 @@ func (it *nodeIterator) findChild(n *fullNode, index int, ancestor common.Hash) state *nodeIteratorState childPath []byte ) - for ; index < len(n.Children); index++ { + for ; index < len(n.Children); index = nextChildIndex(index) { if n.Children[index] != nil { child = n.Children[index] hash, _ := child.cache() @@ -471,8 +471,8 @@ func (it *nodeIterator) nextChild(parent *nodeIteratorState, ancestor common.Has switch node := parent.node.(type) { case *fullNode: // Full node, move to the first non-nil child. - if child, state, path, index := it.findChild(node, parent.index+1, ancestor); child != nil { - parent.index = index - 1 + if child, state, path, index := it.findChild(node, nextChildIndex(parent.index), ancestor); child != nil { + parent.index = prevChildIndex(index) return state, path, true } case *shortNode: @@ -498,23 +498,23 @@ func (it *nodeIterator) nextChildAt(parent *nodeIteratorState, ancestor common.H switch n := parent.node.(type) { case *fullNode: // Full node, move to the first non-nil child before the desired key position - child, state, path, index := it.findChild(n, parent.index+1, ancestor) + child, state, path, index := it.findChild(n, nextChildIndex(parent.index), ancestor) if child == nil { // No more children in this fullnode return parent, it.path, false } // If the child we found is already past the seek position, just return it. if bytes.Compare(path, key) >= 0 { - parent.index = index - 1 + parent.index = prevChildIndex(index) return state, path, true } // The child is before the seek position. Try advancing for { - nextChild, nextState, nextPath, nextIndex := it.findChild(n, index+1, ancestor) + nextChild, nextState, nextPath, nextIndex := it.findChild(n, nextChildIndex(index), ancestor) // If we run out of children, or skipped past the target, return the // previous one if nextChild == nil || bytes.Compare(nextPath, key) >= 0 { - parent.index = index - 1 + parent.index = prevChildIndex(index) return state, path, true } // We found a better child closer to the target @@ -541,7 +541,7 @@ func (it *nodeIterator) push(state *nodeIteratorState, parentIndex *int, path [] it.path = path it.stack = append(it.stack, state) if parentIndex != nil { - *parentIndex++ + *parentIndex = nextChildIndex(*parentIndex) } } @@ -563,6 +563,32 @@ func reachedPath(path, target []byte) bool { return bytes.Compare(path, target) >= 0 } +func prevChildIndex(index int) int { + switch index { + case 0: + return 16 + case 16: + return -1 + case 17: + return 15 + default: + return index - 1 + } +} + +func nextChildIndex(index int) int { + switch index { + case -1: + return 16 + case 15: + return 17 + case 16: + return 0 + default: + return index + 1 + } +} + func compareNodes(a, b NodeIterator) int { if cmp := bytes.Compare(a.Path(), b.Path()); cmp != 0 { return cmp diff --git a/trie/iterator_test.go b/trie/iterator_test.go index 5540c907cd00..47dc18c9dbe0 100644 --- a/trie/iterator_test.go +++ b/trie/iterator_test.go @@ -182,14 +182,14 @@ func testNodeIteratorCoverage(t *testing.T, scheme string) { type kvs struct{ k, v string } var testdata1 = []kvs{ + {"bar", "b"}, {"barb", "ba"}, {"bard", "bc"}, {"bars", "bb"}, - {"bar", "b"}, {"fab", "z"}, + {"foo", "a"}, {"food", "ab"}, {"foos", "aa"}, - {"foo", "a"}, } var testdata2 = []kvs{ @@ -218,7 +218,7 @@ func TestIteratorSeek(t *testing.T) { // Seek to a non-existent key. it = NewIterator(trie.MustNodeIterator([]byte("barc"))) - if err := checkIteratorOrder(testdata1[1:], it); err != nil { + if err := checkIteratorOrder(testdata1[2:], it); err != nil { t.Fatal(err) } @@ -230,7 +230,7 @@ func TestIteratorSeek(t *testing.T) { // Seek to a key for which a prefixing key exists. it = NewIterator(trie.MustNodeIterator([]byte("food"))) - if err := checkIteratorOrder(testdata1[5:], it); err != nil { + if err := checkIteratorOrder(testdata1[6:], it); err != nil { t.Fatal(err) } } @@ -317,16 +317,16 @@ func TestUnionIterator(t *testing.T) { all := []struct{ k, v string }{ {"aardvark", "c"}, + {"bar", "b"}, {"barb", "ba"}, {"barb", "bd"}, {"bard", "bc"}, {"bars", "bb"}, {"bars", "be"}, - {"bar", "b"}, {"fab", "z"}, + {"foo", "a"}, {"food", "ab"}, {"foos", "aa"}, - {"foo", "a"}, {"jars", "d"}, } @@ -517,7 +517,7 @@ func testIteratorContinueAfterSeekError(t *testing.T, memonly bool, scheme strin rawdb.WriteTrieNode(diskdb, common.Hash{}, barNodePath, barNodeHash, barNodeBlob, triedb.Scheme()) } // Check that iteration produces the right set of values. - if err := checkIteratorOrder(testdata1[2:], NewIterator(it)); err != nil { + if err := checkIteratorOrder(testdata1[3:], NewIterator(it)); err != nil { t.Fatal(err) } } From 37494126046bf265ec066ac4e5b619a4a8312fc9 Mon Sep 17 00:00:00 2001 From: Roy Crihfield Date: Thu, 3 Aug 2023 18:33:51 +0800 Subject: [PATCH 4/7] reorder funcs, and add comment --- trie/iterator.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/trie/iterator.go b/trie/iterator.go index 5338aed8e7d0..ae4a1956f4bf 100644 --- a/trie/iterator.go +++ b/trie/iterator.go @@ -563,6 +563,9 @@ func reachedPath(path, target []byte) bool { return bytes.Compare(path, target) >= 0 } +// A value embedded in a fullNode occupies its last child slot, but we want to visit it last to +// produce a pre-order traversal. These two functions handle the ordering by mapping index 0 to the +// value, and shifting the indices of children over by 1. func prevChildIndex(index int) int { switch index { case 0: From eb8f7ac0de335962b2815a736b71bf3906e917dd Mon Sep 17 00:00:00 2001 From: Roy Crihfield Date: Fri, 11 Aug 2023 19:17:40 +0800 Subject: [PATCH 5/7] expand commentary --- trie/iterator.go | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/trie/iterator.go b/trie/iterator.go index ae4a1956f4bf..2b47a7328523 100644 --- a/trie/iterator.go +++ b/trie/iterator.go @@ -555,7 +555,8 @@ func (it *nodeIterator) pop() { } // reachedPath normalizes a path by truncating a terminator if present, and returns true if it is -// greater than or equal to the target. +// greater than or equal to the target. Using this, the path of a value node embedded a full node +// will compare less than the full node's children. func reachedPath(path, target []byte) bool { if hasTerm(path) { path = path[:len(path)-1] @@ -563,31 +564,36 @@ func reachedPath(path, target []byte) bool { return bytes.Compare(path, target) >= 0 } -// A value embedded in a fullNode occupies its last child slot, but we want to visit it last to -// produce a pre-order traversal. These two functions handle the ordering by mapping index 0 to the -// value, and shifting the indices of children over by 1. +// A value embedded in a full node occupies the last slot (16) of the array of children. In order to +// produce a pre-order traversal when iterating children, we jump to this last slot first, then go +// back iterate the child nodes (and skip the last slot at the end): + +// prevChildIndex returns the index of a child in a full node which precedes the given index when +// performing a pre-order traversal. func prevChildIndex(index int) int { switch index { - case 0: + case 0: // We jumped back to iterate the children, from the value slot return 16 - case 16: + case 16: // We jumped to the embedded value slot at the end, from the placeholder index return -1 - case 17: + case 17: // We skipped the value slot after iterating all the children return 15 - default: + default: // We are iterating the children in sequence return index - 1 } } +// nextChildIndex returns the index of a child in a full node which follows the given index when +// performing a pre-order traversal. func nextChildIndex(index int) int { switch index { - case -1: + case -1: // Jump from the placeholder index to the embedded value slot return 16 - case 15: + case 15: // Skip the value slot after iterating the children return 17 - case 16: + case 16: // From the embedded value slot, jump back to iterate the children return 0 - default: + default: // Iterate children in sequence return index + 1 } } From bbc0c64074d0547f4f3baf736286d741b7c2ab7c Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Tue, 12 Dec 2023 16:10:43 +0800 Subject: [PATCH 6/7] trie: improve comment --- trie/iterator.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/trie/iterator.go b/trie/iterator.go index 2b47a7328523..9a52a1a9b764 100644 --- a/trie/iterator.go +++ b/trie/iterator.go @@ -145,7 +145,7 @@ type nodeIterator struct { err error // Failure set in case of an internal error in the iterator resolver NodeResolver // optional node resolver for avoiding disk hits - pool []*nodeIteratorState // local pool for iteratorstates + pool []*nodeIteratorState // local pool for iterator states } // errIteratorEnd is stored in nodeIterator.err when iteration is done. @@ -550,13 +550,14 @@ func (it *nodeIterator) pop() { it.path = it.path[:last.pathlen] it.stack[len(it.stack)-1] = nil it.stack = it.stack[:len(it.stack)-1] - // last is now unused - it.putInPool(last) + + it.putInPool(last) // last is now unused } -// reachedPath normalizes a path by truncating a terminator if present, and returns true if it is -// greater than or equal to the target. Using this, the path of a value node embedded a full node -// will compare less than the full node's children. +// reachedPath normalizes a path by truncating a terminator if present, and +// returns true if it is greater than or equal to the target. Using this, +// the path of a value node embedded a full node will compare less than the +// full node's children. func reachedPath(path, target []byte) bool { if hasTerm(path) { path = path[:len(path)-1] @@ -564,12 +565,13 @@ func reachedPath(path, target []byte) bool { return bytes.Compare(path, target) >= 0 } -// A value embedded in a full node occupies the last slot (16) of the array of children. In order to -// produce a pre-order traversal when iterating children, we jump to this last slot first, then go -// back iterate the child nodes (and skip the last slot at the end): +// A value embedded in a full node occupies the last slot (16) of the array of +// children. In order to produce a pre-order traversal when iterating children, +// we jump to this last slot first, then go back iterate the child nodes (and +// skip the last slot at the end): -// prevChildIndex returns the index of a child in a full node which precedes the given index when -// performing a pre-order traversal. +// prevChildIndex returns the index of a child in a full node which precedes +// the given index when performing a pre-order traversal. func prevChildIndex(index int) int { switch index { case 0: // We jumped back to iterate the children, from the value slot @@ -583,8 +585,8 @@ func prevChildIndex(index int) int { } } -// nextChildIndex returns the index of a child in a full node which follows the given index when -// performing a pre-order traversal. +// nextChildIndex returns the index of a child in a full node which follows +// the given index when performing a pre-order traversal. func nextChildIndex(index int) int { switch index { case -1: // Jump from the placeholder index to the embedded value slot From 2e3e4ecda5774e6de096c5caf0038394530165a8 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Tue, 12 Dec 2023 16:44:38 +0800 Subject: [PATCH 7/7] trie: fix more path comparison --- trie/iterator.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/trie/iterator.go b/trie/iterator.go index 9a52a1a9b764..fa016110636a 100644 --- a/trie/iterator.go +++ b/trie/iterator.go @@ -135,7 +135,7 @@ type nodeIteratorState struct { node node // Trie node being iterated parent common.Hash // Hash of the first full ancestor node (nil if current is the root) index int // Child to be processed next - pathlen int // Length of the path to this node + pathlen int // Length of the path to the parent node } type nodeIterator struct { @@ -304,6 +304,7 @@ func (it *nodeIterator) seek(prefix []byte) error { // The path we're looking for is the hex encoded key without terminator. key := keybytesToHex(prefix) key = key[:len(key)-1] + // Move forward until we're just before the closest match to key. for { state, parentIndex, path, err := it.peekSeek(key) @@ -339,7 +340,6 @@ func (it *nodeIterator) peek(descend bool) (*nodeIteratorState, *int, []byte, er // If we're skipping children, pop the current node first it.pop() } - // Continue iteration to the next child for len(it.stack) > 0 { parent := it.stack[len(it.stack)-1] @@ -372,7 +372,6 @@ func (it *nodeIterator) peekSeek(seekKey []byte) (*nodeIteratorState, *int, []by // If we're skipping children, pop the current node first it.pop() } - // Continue iteration to the next child for len(it.stack) > 0 { parent := it.stack[len(it.stack)-1] @@ -453,12 +452,14 @@ func (it *nodeIterator) findChild(n *fullNode, index int, ancestor common.Hash) if n.Children[index] != nil { child = n.Children[index] hash, _ := child.cache() + state = it.getFromPool() state.hash = common.BytesToHash(hash) state.node = child state.parent = ancestor state.index = -1 state.pathlen = len(path) + childPath = append(childPath, path...) childPath = append(childPath, byte(index)) return child, state, childPath, index @@ -504,7 +505,7 @@ func (it *nodeIterator) nextChildAt(parent *nodeIteratorState, ancestor common.H return parent, it.path, false } // If the child we found is already past the seek position, just return it. - if bytes.Compare(path, key) >= 0 { + if reachedPath(path, key) { parent.index = prevChildIndex(index) return state, path, true } @@ -513,7 +514,7 @@ func (it *nodeIterator) nextChildAt(parent *nodeIteratorState, ancestor common.H nextChild, nextState, nextPath, nextIndex := it.findChild(n, nextChildIndex(index), ancestor) // If we run out of children, or skipped past the target, return the // previous one - if nextChild == nil || bytes.Compare(nextPath, key) >= 0 { + if nextChild == nil || reachedPath(nextPath, key) { parent.index = prevChildIndex(index) return state, path, true }