debug_executionWitness: follow up#20345
Conversation
There was a problem hiding this comment.
Pull request overview
Follow-up improvements to the debug_executionWitness RPC implementation (from #20205), focusing on determinism, safer context propagation, reduced debug noise, and test/global-state hygiene.
Changes:
- Make witness output more deterministic by sorting
Keysand de-duplicating/sortingCodes. - Propagate request
ctxto commitment seeking and tighten/adjust witness verification behavior (optionally disable via env var). - Reduce debug prints in commitment/witness code and fix test/global-state side effects and minor test-chain cleanup.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/jsonrpc/debug_execution_witness.go | Deterministic ordering for keys/codes; ctx propagation to commitment seeking; optional stateless verification; stricter header handling; remove stray debug print. |
| rpc/jsonrpc/debug_api_test.go | Avoid leaking global statecfg.Schema changes across tests; adjust one modified-accounts assertion. |
| execution/commitment/hex_patricia_hashed.go | Gate witness root-node trace output behind hph.trace and route to logger. |
| execution/commitment/commitment.go | Prevent empty hashed key from being collected (avoids panics / invalid indexing). |
| cmd/rpcdaemon/rpcdaemontest/test_util.go | Remove unused token contract address/path computation in chain generator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return nil, fmt.Errorf("failed to load header for accessed block number %d: %w", bn, err) | ||
| } | ||
| if blockHeader == nil { | ||
| return nil, fmt.Errorf("missing header for accessed block number %d", bn) |
There was a problem hiding this comment.
ExecutionWitness now errors out when an accessed BLOCKHASH header is missing (blockHeader == nil). However the EVM GetHash provider used during execution returns a zero hash when the header is nil, and the stateless verifier does the same when a header isn't present in the witness. To avoid turning a previously-valid execution path into an RPC failure, consider skipping nil headers (or only recording headers when GetHash returned a non-zero hash) while still returning on actual DB errors.
| return nil, fmt.Errorf("missing header for accessed block number %d", bn) | |
| // Missing historical headers are treated as unavailable by the EVM GetHash | |
| // provider and by stateless verification, so do not fail witness generation. | |
| continue |
| n2 = rpc.BlockNumber(12) | ||
| _, err = api.GetModifiedAccountsByNumber(m.Ctx, rpc.BlockNumber(11), &n2) | ||
| result, err = api.GetModifiedAccountsByNumber(m.Ctx, rpc.BlockNumber(11), &n2) | ||
| require.NoError(t, err) | ||
| require.Len(t, result, 3) | ||
| require.NotEmpty(t, result) |
There was a problem hiding this comment.
This test assertion was weakened from an exact expected length to only require.NotEmpty, which can hide regressions in GetModifiedAccountsByNumber. If the exact count is no longer stable, consider asserting a deterministic subset instead (e.g., that specific known addresses are present for the 11→12 range), or keep the deployed Token address from the chain generator so you can assert it is included.
address CodeReview comments in #20205