diff --git a/eth/protocols/snap/handler.go b/eth/protocols/snap/handler.go index 30ebe9864d..34b2a27b7d 100644 --- a/eth/protocols/snap/handler.go +++ b/eth/protocols/snap/handler.go @@ -55,11 +55,13 @@ const ( // maxTrieNodePathLength is the longest a compact-encoded trie path can be // while still addressing a node in any Ethereum state trie. Trie keys are - // Keccak256 hashes (32 bytes = 64 nibbles), and the compact encoding adds - // at most one prefix byte, giving 32+1 = 33 bytes. Anything longer is - // structurally invalid and cannot match a node, so the handler skips it - // without performing a trie traversal. - maxTrieNodePathLength = 33 + // Keccak256 hashes (32 bytes = 64 nibbles). The theoretical compact-encoded + // upper bound is 33 bytes (64 nibbles + 1 prefix byte), but in an MPT the + // value node is always embedded in its parent and is never addressed as a + // standalone node, so the hexary path length is at most 63 nibbles and the + // compact encoding never exceeds 32 bytes. Anything longer is structurally + // invalid and the handler skips it without performing a trie traversal. + maxTrieNodePathLength = 32 ) // Handler is a callback to invoke from an outside runner after the boilerplate diff --git a/eth/protocols/snap/handler_test.go b/eth/protocols/snap/handler_test.go index f44dfd125a..89ea15253a 100644 --- a/eth/protocols/snap/handler_test.go +++ b/eth/protocols/snap/handler_test.go @@ -223,10 +223,12 @@ func TestServiceGetAccessListsQueryByteLimit(t *testing.T) { // TestServiceGetTrieNodesQueryPathLength verifies that ServiceGetTrieNodesQuery // rejects structurally invalid (over-length) compact-encoded paths without // allocating in compactToHex or walking the trie. Trie keys are Keccak256 -// hashes, so the longest valid compact-encoded path is 33 bytes; anything -// longer cannot match a node in any state trie. A peer sending such a path -// (up to the 10MB request size cap) would otherwise force the server to -// allocate 2*len(path)+1 nibbles per entry and start a doomed traversal. +// hashes; value nodes are always embedded in their parent and are never +// addressed as standalone nodes, so the longest compact-encoded path that can +// match a node is 32 bytes. Anything longer cannot match a node in any state +// trie. A peer sending such a path (up to the 10MB request size cap) would +// otherwise force the server to allocate 2*len(path)+1 nibbles per entry and +// start a doomed traversal. func TestServiceGetTrieNodesQueryPathLength(t *testing.T) { t.Parallel() diff --git a/eth/protocols/snap/handlers.go b/eth/protocols/snap/handlers.go index 90fc185027..3d6bc96e1e 100644 --- a/eth/protocols/snap/handlers.go +++ b/eth/protocols/snap/handlers.go @@ -476,15 +476,15 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( // the request, matching the existing behavior for non-existent nodes. if len(accKey) > maxTrieNodePathLength { nodes = append(nodes, nil) - break + } else { + blob, resolved, err := accTrie.GetNode(accKey) + loads += resolved // always account database reads, even for failures + if err != nil { + break + } + nodes = append(nodes, blob) + bytes += uint64(len(blob)) } - blob, resolved, err := accTrie.GetNode(accKey) - loads += resolved // always account database reads, even for failures - if err != nil { - break - } - nodes = append(nodes, blob) - bytes += uint64(len(blob)) default: // Storage slots requested, open the storage trie and retrieve from there @@ -528,18 +528,15 @@ func ServiceGetTrieNodesQuery(chain *core.BlockChain, req *GetTrieNodesPacket) ( // the response stays positionally aligned with the request. if len(path) > maxTrieNodePathLength { nodes = append(nodes, nil) - if bytes > req.Bytes || loads > maxTrieNodeLookups || time.Since(start) > maxTrieNodeTimeSpent { + } else { + blob, resolved, err := stTrie.GetNode(path) + loads += resolved // always account database reads, even for failures + if err != nil { break } - continue + nodes = append(nodes, blob) + bytes += uint64(len(blob)) } - blob, resolved, err := stTrie.GetNode(path) - loads += resolved // always account database reads, even for failures - if err != nil { - break - } - nodes = append(nodes, blob) - bytes += uint64(len(blob)) // Sanity check limits to avoid DoS on the store trie loads if bytes > req.Bytes || loads > maxTrieNodeLookups || time.Since(start) > maxTrieNodeTimeSpent {