eth/protocols/snap: address review — if-else guard, tighten path limit to 32

Per review on #34854:

- Refactor the path-length guard to if-else style (no append-nil-and-break);
  the trie traversal lives in the else branch.
- Tighten maxTrieNodePathLength from 33 to 32 bytes. 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.
- Update test comment to reflect the 32-byte real-world cap.

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
+	// Keccak256 hashes (32 bytes = 64 nibbles). The theoretical compact-encoded
-	// at most one prefix byte, giving 32+1 = 33 bytes. Anything longer is
+	// upper bound is 33 bytes (64 nibbles + 1 prefix byte), but in an MPT the
-	// structurally invalid and cannot match a node, so the handler skips it
+	// value node is always embedded in its parent and is never addressed as a
-	// without performing a trie traversal.
+	// standalone node, so the hexary path length is at most 63 nibbles and the
-	maxTrieNodePathLength = 33
+	// 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

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
+// hashes; value nodes are always embedded in their parent and are never
-// longer cannot match a node in any state trie. A peer sending such a path
+// addressed as standalone nodes, so the longest compact-encoded path that can
-// (up to the 10MB request size cap) would otherwise force the server to
+// match a node is 32 bytes. Anything longer cannot match a node in any state
-// allocate 2*len(path)+1 nibbles per entry and start a doomed traversal.
+// 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()
 

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 {