perf(trie): reuse stored hashes and traversed nodes in Prove

[brbrr]

Bench results based on sepolia snapshot

case	time/op (main)	time/op (PR)	Δ time	B/op (main)	B/op (PR)	Δ B/op	allocs/op (main)	allocs/op (PR)	Δ allocs
classes_16	7.718ms	3.708ms	−51.96%	522.1Ki	374.6Ki	−28.25%	8,270	5,844	−29.33%
contracts_1	1.488ms	628.2µs	−57.79%	75.39Ki	56.84Ki	−24.60%	1,162	869	−25.22%
contracts_16	20.469ms	9.049ms	−55.79%	1041.2Ki	747.4Ki	−28.21%	16,250	11,610	−28.54%
storage_1x16	1.237ms	660.0µs	−46.66%	65.73Ki	50.86Ki	−22.63%	1,033	780	−24.49%
storage_16x4	11.627ms	5.791ms	−50.20%	582.5Ki	400.5Ki	−31.24%	9,788	6,654	−32.02%
mixed	6.117ms	3.146ms	−48.56%	328.2Ki	243.7Ki	−25.75%	5,211	3,827	−26.56%
geomean	5.240ms	2.516ms	−51.99%	283.1Ki	207.1Ki	−26.84%	4,493	3,247	−27.74%

[github-actions]

Claude finished @brbrr's task in 6m 55s —— View job

---

Review: perf(trie): reuse stored hashes and traversed nodes in Prove

• Read PR diff and surrounding code (trie.go, node.go, storage.go, callers)
• Verify hash-reuse equivalence (Edge.Hash vs Node.Hash, node.Value vs Binary.Hash, carried child hashes)
• Check aliasing / nodePool safety
• Check test coverage and callers
• Post findings

The refactor is sound for its intended input: I traced each reuse and they are exact, not approximate.

• Edge.Hash(fn) and Node.Hash(path, fn) compute the same thing (hash(Value, pathFelt) + pathLen), so binaryProofNode's child.Hash(&edgePath, …) is bit-identical to the old rEdge.Hash(…).
• path() strips the branching bit, so a direct (non-edge) child has path.Len() == 0 and Node.Hash == Value — the return child.Value fast path is correct, and it makes the carried LeftHash/RightHash exactly the child's parent-facing hash.
• Node emission and ordering (edge-then-binary, break on binary leaf) match the old branch table, including the non-existent-key/divergent-path case.
• DB reads drop from ~2 per level to 1 (off-path sibling only), plus one less Pedersen hash per level. Real win on the RPC storage-proof path.

Important

• core/trie/proof.go:116-118 — the new "trie must be hashed" precondition is documented but unenforced. Keying the binary node by *sNode.node.Value is only valid after Hash()/Commit(); ancestors of a Put/Delete keep a stale Value until then. The old bottom-up recomputation tolerated a dirty trie; the new code emits a proof that fails verification. No current caller is affected (RPC handlers prove committed head state, and all test helpers Commit() first — which is also why no test would catch a regression). Suggested a cheap t.rootKeyIsDirty || len(t.dirtyNodes) > 0 guard, mirroring trie2.Prove's ErrCommitted. inline

Nits

• proof.go:85-87 — the aliasing comment should also cover the off-path siblings from binaryProofNode (return child.Value), which are pooled nodes too; and note that Prove now retains one pool node per level for the lifetime of the ProofNodeSet. inline
• proof.go:120-130 — the carry is dead when the on-path child isn't an edge, and a future drift in the carry would silently mis-key a proof node. A for (hash, node) in proofSet: node.Hash(Pedersen) == hash assertion would lock in both this invariant and the one above. inline

No concurrency, nil-deref, or resource-leak issues found; the emptyBitArray child lookup and isEdge uint8 underflow on partial/proof tries are pre-existing and unchanged by this diff.

⚠️ I could not run go test ./core/trie/ — the command was blocked by this job's tool permissions, so the equivalence above is from reading the code, not from executing the suite. Worth running TestProve*/TestVerify*/BenchmarkProve locally (and posting the benchmark delta, since the PR has no description).
· branch perf/prove-reuse-stored-hashes

[codecov]

Codecov Report

❌ Patch coverage is 89.28571% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.61%. Comparing base (98ce435) to head (5d0bd91).

Files with missing lines	Patch %	Lines
core/trie/proof.go	89.28%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3945      +/-   ##
==========================================
- Coverage   74.76%   74.61%   -0.15%     
==========================================
  Files         461      461              
  Lines       40915    40921       +6     
==========================================
- Hits        30590    30535      -55     
- Misses       8232     8287      +55     
- Partials     2093     2099       +6     

Flag	Coverage Δ
jsonv2	77.42% <89.28%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Claude finished @brbrr's task in 5m 52s —— View job

---

Re-review after dabdcf82

• Read the new diff (origin/main...HEAD)
• Check whether the stale-Value precondition is now enforced
• Re-verify hash-reuse equivalence after the cleanup commit
• Run go vet ./core/trie/ (clean); go test blocked — see note
• Post findings

All three findings from my earlier pass are addressed, and I re-derived the hash equivalences from scratch rather than trusting the previous review.

The precondition is now real, not just documented. I walked every write path to confirm the guard at proof.go:73 can't be bypassed by Put/Delete: updateLeaf → dirtyNodes (trie.go:382), insertOrUpdateValue → dirtyNodes in both the proof and non-proof branches (trie.go:466, 494) or setRootKey (496), handleEmptyTrie → setRootKey (399), deleteLast → setRootKey/dirtyNodes (764, 782, 797); Hash clears both (813, 833). No exported write op leaves the trie stale-but-clean. The one remaining hole is PutInner, which bypasses dirty tracking entirely — in-tree only buildTrie uses it, and that calls setRootKey first, so nothing is actually exposed.

Hash reuse re-verified. path(child, parent) has length child.len - parent.len - 1, so a direct child has path.Len() == 0 and Node.Hash short-circuits to a copy of Value — making binaryProofNode's return child.Value exactly the parent-facing hash. For an edge child, Node.Hash computes hash(Value, pathFelt) + pathLen, bit-identical to the Edge.Hash it replaces (node.go:23-36 vs proof.go:47-56), and identical to what updateValueIfDirty folds into node.Value at trie.go:~705. nodesFromRoot only ever advances via node.Left/node.Right (trie.go:330-334), so onPathChild is always a genuine child and the carriedHash switch cannot mis-key. Emission order and the binary-leaf break match the old branch table, including the divergent-path/non-membership case.

The two new tests are the right ones. OrderedSet.Keys() and List() are index-aligned (utils/orderedset.go:58-77), so TestProveSetInvariant's node.Hash(Pedersen) == key check is a valid assertion — and it's the guard that locks in both the carry invariant and the stale-Value case, which is exactly the drift I was worried about last time. Including a non-membership key in it was a good call.

Blockers

None.

Important

None.

Nits

• proof.go:73-75 — use a package-level ErrUnhashedWrites sentinel instead of inline errors.New; proof_test.go:872 currently pins the exact message with require.EqualError. Also worth noting the PutInner gap in that method's doc comment. inline
• proof.go:413 — StorageNode{key: childKey} is a zero-node struct built only to reach a helper that reads .key.len; an isEdgeKey(parent, child *BitArray) split removes the throwaway at both call sites. inline

Not defects, just unchanged pre-existing behaviour I re-checked so nobody attributes them to this diff: the emptyBitArray child lookup and the isEdge uint8 underflow on partial/proof tries, and the felt aliasing into nodePool nodes (the old code aliased sNode.node.Value into Edge.Child too — this diff widens the set of aliased nodes but not the hazard, and the comment at proof.go:89-90 now says so).

⚠️ go vet ./core/trie/ passes, so everything compiles including the new tests, but go test ./core/trie/ is still blocked by this job's tool permissions — the equivalence above is from reading the code, not from a green suite. Please run TestProve*/TestVerify*/TestProveSetInvariant locally. The PR still has no description; a BenchmarkProve before/after delta would make the perf claim reviewable, along with a line on the retained-pool-node trade-off.
· branch perf/prove-reuse-stored-hashes

[github-actions]

Claude finished @brbrr's task in 1m 58s —— View job

---

Re-review after 5d0bd912

• Read the new diff (origin/main...HEAD)
• Verify the isEdge/isEdgeKey split is behaviour-preserving
• Re-check hash reuse, carry, and the unhashed-writes guard after the refactor
• Run go vet ./core/trie/ (clean); go test blocked — see note
• Post findings

The only change since my last pass is the mechanical isEdge split, and it's exact:

• isEdgeKey(parent, child *BitArray) carries the identical branch logic (root → child.len != 0, otherwise child.len - parent.len > 1), and isEdge is now a one-line delegation. The old isEdge never read sNode.node, so replacing isEdge(sNode.key, StorageNode{key: childKey}) with isEdgeKey(sNode.key, childKey) at core/trie/proof.go:419 cannot change behaviour — it just drops the throwaway zero-node struct.
• Both helpers are still reachable (isEdge at proof.go:95, isEdgeKey at proof.go:391/419), so no dead code for the linter to flag.
• Nothing else in Prove, edgeHash, or binaryProofNode moved, so the equivalences I derived last round — Node.Hash(path, fn) ≡ the old Edge.Hash, path.Len() == 0 → Hash() == Value for a direct child, the carriedHash switch keyed off node.Left/node.Right, and the emission order including the divergent-path case — all still hold. The t.rootKeyIsDirty || len(t.dirtyNodes) > 0 guard at proof.go:73 is untouched, and TestProveSetInvariant still pins the node.Hash(Pedersen) == key invariant across both membership and non-membership proofs.

The sentinel-error / PutInner-doc nit from my previous pass is marked not desired by @brbrr — treating that as settled and not re-raising it.

Blockers

None.

Important

None.

Nits

None. This diff is clean.

⚠️ go vet ./core/trie/ passes, so everything compiles including the new tests, but go test ./core/trie/ is still blocked by this job's tool permissions — the equivalence above is from reading the code, not from a green suite. Please run TestProve*/TestVerify*/TestProveSetInvariant locally. The PR still has no description; a BenchmarkProve before/after delta would make the perf claim reviewable, plus a line on the retained-pool-node trade-off (Prove keeps one pool node per level alive for the lifetime of the ProofNodeSet).
· branch perf/prove-reuse-stored-hashes