trie/concurrent_iterator it.LeafKey() panic #376

New Issue

2023-05-10T22:24:57Z

i-norden commented

2023-05-10 22:24:57 +00:00

It is actually the underlying stock geth nodeIterator that panics, see

panic: can't convert hex key of odd length

goroutine 14 [running]:
github.com/ethereum/go-ethereum/trie.hexToKeyBytes({0x14000278080?, 0x1052eec60?, 0x140000d4150?})
	/Users/iannorden/go/pkg/mod/github.com/cerc-io/go-ethereum@v1.11.5-statediff-5.0.2-alpha/trie/encoding.go:125 +0xf8
github.com/ethereum/go-ethereum/trie.(*nodeIterator).LeafKey(0x1400003c410?)
	/Users/iannorden/go/pkg/mod/github.com/cerc-io/go-ethereum@v1.11.5-statediff-5.0.2-alpha/trie/iterator.go:203 +0x54
github.com/cerc-io/ipld-eth-state-snapshot/pkg/snapshot.(*Service).processStateValueNode(0x14000248000, {0x105328110, 0x140002ae320}, {0x140002f6190, 0x42}, 0x1400007fc40, {0x10592ccf8?, 0x105926bc8?, 0x140000b1868?}, {0x12d26f468, ...})
	/Users/iannorden/go/src/github.com/cerc-io/ipld-eth-state-snapshot/pkg/snapshot/service.go:431 +0x118

But this only occurs when they are being used within the context of our concurrent PrefixBoundIterator. This error hasn't cropped up until now as in v4 we never called it.LeafKey() during statediffing or ipld-eth-state-snapshot processes. What geth calls a "leaf node"- that is, when it.Leaf() returns true- is actually when the iterator is positioned at a "value node" and in v4 we ignored these completely (we processed the values out of the actual leaf nodes, deriving the leaf key from path + partial path of the leaf node instead of calling it.LeafKey()) but due to the changes in v5 to support the indexing of internalized leaf nodes we now consider "value nodes" and call it.LeafKey().

Importantly, it.Leaf() and hasTerm evaluate to true and the iterator is actually positioned at a value node, you can call it.LeafBlob(), when this panic occurs. The issue appears to be: it.LeafKey() has a guard (where the panic is coming from) that checks that the length of the path at the current position of the iterator is even before attempting to compact the path nibbles into their key representation. The problem arises when we create a PrefixBoundIterator with an odd length path prefix. This causes an offset in the path for the underlying subtrie nodeIterator such that its internal path is odd when the full path is even.

This has ramifications for eth-statediff-service and ipld-eth-state-snapshot. This could be fixed at two levels.

Avoid calling it.LeafKey() in these places (aka go back to deriving from path/partial path/path prefix), this is easier for ipld-eth-state-snapshot as it calls it directly whereas eth-statediff-service only calls it indirectly through its dependency on the statediff builder in this repo so it would require a new update and release of geth.
Patch the SubtrieIterators method to ensure we never initialize a subtrie iterator at an odd position in the trie. Also requires a new release.

I don't know that this goes so far as being a bug/issue in the geth nodeIterator, but it is a bit odd that it allows you to initialize an iterator at any position in a trie when some of the methods it exposes will panic if that position happens to be an odd one.

It is actually the underlying stock geth [nodeIterator](https://github.com/cerc-io/go-ethereum/blob/v1.11.6-statediff-v5/trie/iterator.go#L203) that panics, see ``` panic: can't convert hex key of odd length goroutine 14 [running]: github.com/ethereum/go-ethereum/trie.hexToKeyBytes({0x14000278080?, 0x1052eec60?, 0x140000d4150?}) /Users/iannorden/go/pkg/mod/github.com/cerc-io/go-ethereum@v1.11.5-statediff-5.0.2-alpha/trie/encoding.go:125 +0xf8 github.com/ethereum/go-ethereum/trie.(*nodeIterator).LeafKey(0x1400003c410?) /Users/iannorden/go/pkg/mod/github.com/cerc-io/go-ethereum@v1.11.5-statediff-5.0.2-alpha/trie/iterator.go:203 +0x54 github.com/cerc-io/ipld-eth-state-snapshot/pkg/snapshot.(*Service).processStateValueNode(0x14000248000, {0x105328110, 0x140002ae320}, {0x140002f6190, 0x42}, 0x1400007fc40, {0x10592ccf8?, 0x105926bc8?, 0x140000b1868?}, {0x12d26f468, ...}) /Users/iannorden/go/src/github.com/cerc-io/ipld-eth-state-snapshot/pkg/snapshot/service.go:431 +0x118 ``` But this only occurs when they are being used within the context of our concurrent [PrefixBoundIterator](https://github.com/cerc-io/go-ethereum/blob/v1.11.6-statediff-v5/trie/concurrent_iterator/iterator.go). This error hasn't cropped up until now as in v4 we never called `it.LeafKey()` during statediffing or ipld-eth-state-snapshot processes. What geth calls a "leaf node"- that is, when `it.Leaf()` returns true- is actually when the iterator is positioned at a "value node" and in v4 we ignored these completely (we processed the values out of the actual leaf nodes, deriving the leaf key from `path + partial path` of the leaf node instead of calling `it.LeafKey()`) but due to the changes in v5 to support the indexing of internalized leaf nodes we now consider "value nodes" and call `it.LeafKey()`. Importantly, `it.Leaf()` and `hasTerm` evaluate to true and the iterator *is* actually positioned at a value node, you can call `it.LeafBlob()`, when this panic occurs. The issue appears to be: `it.LeafKey()` has a guard (where the panic is coming from) that checks that the length of the path at the current position of the iterator is even before attempting to compact the path nibbles into their key representation. The problem arises when we create a `PrefixBoundIterator` with an odd length path prefix. This causes an offset in the path for the underlying subtrie `nodeIterator` such that its internal path is odd when the full path is even. This has ramifications for eth-statediff-service and ipld-eth-state-snapshot. This could be fixed at two levels. 1. Avoid calling `it.LeafKey()` in these places (aka go back to deriving from path/partial path/path prefix), this is easier for ipld-eth-state-snapshot as it calls it directly whereas eth-statediff-service only calls it indirectly through its dependency on the statediff builder in this repo so it would require a new update and release of geth. 2. Patch the [SubtrieIterators](https://github.com/cerc-io/go-ethereum/blob/v1.11.6-statediff-v5/trie/concurrent_iterator/iterator.go#L133) method to ensure we never initialize a subtrie iterator at an odd position in the trie. Also requires a new release. I don't know that this goes so far as being a bug/issue in the geth `nodeIterator`, but it is a bit odd that it allows you to initialize an iterator at any position in a trie when some of the methods it exposes will panic if that position happens to be an odd one.

Sign in to join this conversation.

Branches Tags

v1.11.6-statediff-v5

v1.10.26-statediff-4.2.2-alpha

jenkins_reborn

v1.10.25-statediff-v4

v1.10.25-statediff-v4-wip

cerc_refactor

release-v1.10.23-statediff-4.2.0-alpha

v1.10.23-statediff-v4

v1.10.23-statediff-v4-wip

v1.10.22-statediff-v4-wip

v1.10.22-statediff-v4

v1.10.21-statediff-v4

linter_updates_wip

release-v1.10-21-statediff-4.1.2-alpha

release-v1.10.21-statediff-4.1.2-alpha

v1.10.20-statediff-v4

v1.10.19-statediff-v4

statediff

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/go-ethereum#376

trie/concurrent_iterator it.LeafKey() panic #376

trie/concurrent_iterator `it.LeafKey()` panic #376