rpc/jsonrpc, execution/commitment: drop transient-collapse siblings from execution witness#21569
Conversation
…rom execution witness debug_executionWitness computes the post-state commitment by replaying a block's updates in key-sorted order. Deleting a key that reduces a branch to a single child collapses it, and the collapse tracer touches the surviving sibling into the witness. When a later insert in the same block refills that branch, its final form has >=2 children and the sibling is not part of the canonical witness, so the witness carried one extra node. Carry the collapsing branch's prefix through CollapseTracer. After the post-state commitment, read each candidate branch from the in-memory commitment domain and drop the sibling when the branch ended with >=2 children (transient collapse); keep siblings whose branch genuinely collapsed.
9865350 to
4dc8e36
Compare
There was a problem hiding this comment.
Pull request overview
This PR refines debug_executionWitness witness construction by filtering out “transient-collapse siblings” that are only touched due to intermediate trie collapses during key-sorted state replay within a single block, but do not exist in the block’s final canonical post-state trie shape.
Changes:
- Extend the commitment trie collapse tracer to report both the surviving sibling path and the collapsing branch prefix.
- Add a post-commitment filter in
debug_execution_witness.gothat drops collapse siblings when the final post-state branch has>= 2children. - Introduce lightweight helpers to read post-state branch child-count from the commitment domain (
BranchChildCount,BranchData.ChildCount).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
rpc/jsonrpc/debug_execution_witness.go |
Collect collapse candidates with branch prefixes and filter out transient-collapse siblings based on post-state branch child-count. |
execution/commitment/hex_patricia_hashed.go |
Update collapse tracer calls to provide the collapsing branch prefix in addition to sibling path. |
execution/commitment/commitmentdb/commitment_context.go |
Add BranchChildCount helper to query post-compute branch child count from commitment domain. |
execution/commitment/commitment.go |
Add BranchData.ChildCount() helper to compute branch child count from encoded branch data. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yperbasis
left a comment
There was a problem hiding this comment.
Requesting changes:
1. Collapse tracer leaks on the error paths. In detectCollapseSiblings the tracer is cleared only on the success path — the ComputeCommitment-error and root-mismatch returns leave it installed. The trie is then Release()d into the process-global hphPool, and resetForReuse() clears cache/capture/trace but not collapseTracer, so the stale closure (capturing a now-dead candidates slice) can re-enter the pool and fire during normal commitment. Please defer sdCtx.SetCollapseTracer(nil) immediately after setting it (and drop the now-redundant explicit clear), and add hph.collapseTracer = nil to resetForReuse().
2. PR description — "key-sorted order" is inaccurate. The canonical rule is insert-before-delete, not key-sorted; the witness_state_replay_order fixtures are specifically constructed to fail a key-sorted implementation. This PR passes because the post-hoc filter restores canonical membership. Please reword so it's clear the filter is an equivalent-membership shortcut, not a match to the spec's replay ordering.
3. Pre-allocate siblingPaths — siblingPaths = make([][]byte, 0, len(candidates)) (likely a prealloc lint finding).
… filter - defer SetCollapseTracer(nil) so the tracer is cleared on the error and root-mismatch paths, not only on success - clear collapseTracer in resetForReuse() so a pooled trie cannot retain a stale closure - preallocate siblingPaths - reword the filter comment: it restores canonical witness membership, it does not reproduce the spec's insert-before-delete replay order
Conflict in rpc/jsonrpc/debug_execution_witness.go: kept the branch's mode-gated collapse-sibling drop (legacy keeps siblings; canonical drops childCount>=2) over main's unconditional drop (#21569).
Problem
debug_executionWitnessemits one extra state node for blocks that delete and re-insert keys under the same trie branch.The canonical witness follows insert-before-delete ordering, under which such a branch never collapses. Erigon's post-state commitment replays the block's updates in a different order that can transiently reduce the branch to a single child; the collapse tracer then touches the surviving sibling into the witness. The block's net change leaves the branch with >=2 children, so that sibling is not part of the canonical witness.
Surfaced by EEST fixtures
eip8025_optional_proofs/witness_state_replay_order/*, which are constructed to fail a replay that is not insert-before-delete.Fix
This does not change the replay order. It restores canonical node membership with a post-pass filter:
CollapseTracercarries the collapsing branch's nibble prefix alongside the sibling path.detectCollapseSiblingsreads each candidate branch from the in-memory commitment domain and drops the sibling when the branch ended with >=2 children (transient collapse); siblings whose branch genuinely collapsed are kept.BranchData.ChildCount()andSharedDomainsCommitmentContext.BranchChildCount().Producer-only; no consumer changes, no state-root impact.
Testing
Verified against the EEST zkevm witness suite (#21487, which carries the fixtures): the two
witness_state_replay_orderfixtures pass and the fullfor_amsterdamrun shows no new failures.Part of #21307.