cerc-io / go-ethereum

Read-only mirror of https://git.vdb.to/cerc-io/go-ethereum (Statediffing-fork of the official Go implementation of the Ethereum protocol)
https://git.vdb.to/cerc-io/go-ethereum
GNU Lesser General Public License v3.0
13 stars 4 forks source link

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

Closed i-norden closed 1 year ago

i-norden commented 1 year ago

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.

  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 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.