feat(witness): generate the storage witness from the world-state scope#12018
feat(witness): generate the storage witness from the world-state scope#12018asdacap wants to merge 7 commits into
Conversation
Move storage-witness generation into IWorldStateScopeProvider so witness RPCs (proof_call / debug_executionWitness) keep the fast FlatDB read path during execution instead of forcing every read through trie traversal. - BeginScope gains a trackWitness flag; IScope exposes a Witness property (ScopeWitness: state-trie/storage-trie node RLPs + touched keys) and ReportRead(Address)/ReportRead(in StorageCell), all with safe defaults. - StateProvider/PersistentStorageProvider call ReportRead at every touch point (gated by a cheap flag) so the scope sees reads/writes its own Get never does (cache hits, writes, zero-value balance early-returns) — exact parity with the old WitnessGeneratingWorldState record set. - The backend scopes build the witness lazily post-execution by walking a fresh read-only view at the base state root (flat: GatherReadOnlySnapshotBundle; trie: read-only trie store) via the shared StorageWitnessCollector + relocated MultiAccountProofCollector. - Rewire the witness env onto CreateResettableWorldState()+GlobalStateReader, slim WitnessGeneratingWorldState to bytecode/header assembly, delete WitnessCapturingTrieStore. - ProofRpcModuleCallTests: parameterize the witness-content tests on both flat and trie backends. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback: keep the storage backends free of witness concerns and drop the IWorldState decorator. - Move all witness bookkeeping (touched keys, contract code, post-execution state walk) out of FlatWorldStateScope / TrieStoreScopeProvider into a single WitnessScopeProvider IScope wrapper. The backends are reverted to clean and produce no witness. - ScopeWitness now carries Codes too (captured via a code-db wrapper), so the scope produces the complete storage witness. - Eliminate the WitnessGeneratingWorldState IWorldState decorator: bytecode is captured in the scope wrapper; RecordBytecodeAccess (EIP-2935 hint) is handled by WorldState gated on trackWitness; the final Witness (adding headers) is built by a small WitnessAssembler used directly by the collectors. - The StateProvider/PersistentStorageProvider ReportRead calls stay: they mirror the old decorator's touch points exactly and are required because those caches sit above the scope and writes never read through it, so an IScope wrapper alone cannot observe every touch. proof_call witness tests pass on both flat and trie backends; State, State.Flat, Consensus, Facade and JsonRpc suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…system untouched Follow-up to review: writes don't need separate interception (ReportRead or the write batch) because the EVM reads every account/slot before writing it — SSTORE reads the current value for gas (EvmInstructions.Storage.cs:501), CREATE reads for collision, balance/nonce ops read. So the WitnessScopeProvider wrapper captures everything purely through the scope's own read path: Get (account), the per-account storage tree Get (slot), and the code-db GetCode (code). A write-batch interception would NOT suffice anyway: proof_call uses CallAndRestore and never commits (no write batch at all), reverted writes never reach it, and the storage commit runs multi-threaded. Result: StateProvider / PersistentStorageProvider / PartialStorageProviderBase are now byte-for-byte unchanged vs master — the witness no longer touches the sensitive storage subsystem. Revert IScope.ReportRead and all the provider instrumentation. The only witness hook left outside the wrapper is WorldState.RecordBytecodeAccess (the EIP-2935 BlockhashStore hint), gated on trackWitness. proof_call witness tests pass on both flat and trie backends (incl. the sstore-then-reverted case, captured via the SSTORE gas read); State, State.Flat, Consensus and Facade suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n a separate wrapper
Per review, narrow the storage-backend witness to the one storage-coupled part —
the trie node RLPs — and produce the rest independently at a higher level.
- IScope.Witness / IWorldState.Witness now return IReadOnlyList<byte[]>? (state-trie
node RLPs). Delete the ScopeWitness {StateNodes,Keys,Codes} bundle.
- WitnessScopeProvider -> TrieWitnessScopeProvider: tracks touched keys only to drive
its post-execution walk and returns the trie nodes; no longer records code.
StorageWitnessCollector returns the deduped node list.
- New AccessWitnessScopeProvider: an independent decorator that records the touched
accounts/slots (TouchedKeys) and contract code (Codes) for the current scope.
- Compose WorldState(AccessWitnessScopeProvider(TrieWitnessScopeProvider(backend))):
Access is outer (its code-db wrapper observes code reads), forwards IScope.Witness
through to the inner trie wrapper, and propagates trackWitness so the inner produces
the nodes.
- The collector combines worldState.Witness (trie RLP) + accessWitness TouchedKeys/Codes
+ headers; WitnessAssembler formats the <addr><slot> Keys.
proof_call witness tests pass on both flat and trie backends (identical witness);
State, State.Flat, Consensus, Facade suites green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nesses A stateless verifier re-applies a block's writes and recomputes the post-state root. When a write deletes a key and collapses a trie branch (SSTORE->0, EIP-158 prune, SELFDESTRUCT), recomputing the root reads the surviving sibling node, which is not on the path to any touched key, so the touched-key proof walk omits it. The verifier then hits MissingTrieNodeException / a wrong root. proof_call never commits, so it could not catch this. Capture the structural nodes read while the block's commit recomputes the root, via WitnessCapturingScopedTrieStore wrapping the state- and storage-trie stores when a scope is opened with trackWitness. The scope Witness is now read-proof walk union commit-captured nodes. The flat backend serves resolved nodes with their children already linked in memory, so navigation reaches a child without calling the resolver, leaving it uncaptured. FindCachedOrUnknown therefore captures the node's RLP and returns an unresolved clone carrying only that RLP; re-parsing yields hash-ref children, forcing every child the trie actually navigates to back through the wrapper (and avoiding a null TryLoadRlp reconstruct on flat). This captures exactly the touched nodes, no over-capture. Adds a round-trip regression test (both flat and trie backends) that deletes a storage slot to collapse a branch, generates the witness, and re-executes it through a stateless world state, asserting the recomputed post-state root matches with no MissingTrieNode. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `PatriciaTrieWitnessGenerator`, a mutation-free, single-trie witness collector that walks the pre-state trie driven by a list of read/written key paths and reports the nodes a stateless verifier needs. It replaces the trie-read-interception approach, which forced execution off the flat-DB fast path (the flat snapshot layer and TrieNodeCache hand back decoded TrieNodes without materializing RLP). The walk mirrors `PatriciaTree.BulkSet` (partial-sort by nibble, bucketize into 16, recurse) and reuses its sort helpers. A recursion returns true when its whole subtree is deleted, which drives the lone-child collapse: when deletions reduce a branch to a single remaining child, the surviving sibling is reported even though it was never on a touched path — the node a plain read-path visitor misses. An optional top-level 16-way parallel mode fans the root subtrees out (children pre-resolved single-threaded so the root is never mutated concurrently); the sink must then be thread-safe. Output goes to a small `IWitnessNodeSink`. Tests assert, across fuzz seeds, BulkSet-derived shapes, and targeted collapse/extension/absent/mixed cases: - the generated node set (sequential and parallel) equals the ground truth obtained by replaying the reads and deletes through a read-capturing store; - the witness alone serves every read and recomputes the post-state root with no missing node. A benchmark compares the new generator against the old capture-during-mutation path; the new path is ~0.3-0.7x of the old time, with the biggest win on delete-heavy workloads. Scope: standalone class + tests + benchmark; not yet wired into the witness pipeline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ssGenerator Replace the two-part witness State producer — the MultiAccountProofCollector touched-key walk plus the commit-time WitnessCapturingScopedTrieStore capture — with a single mutation-free pre-state walk. TrieWitnessScopeProvider now records, per touched account/slot, whether it was read or deleted: reads ride the existing read path; deletes (SSTORE->0, SELFDESTRUCT, EIP-158 prune) are observed by decorating the commit write batch (account Set(null), slot Set(zero), Clear()). After execution it runs PatriciaTrieWitnessGenerator over a fresh read-only view of the base state — once on the state trie and once per touched account's storage trie (base storage root read from the pre-state view; self-destructed accounts skip the storage walk) — collecting the read-proof nodes and the collapse siblings a stateless verifier needs. This keeps witness generation on the flat-DB fast path (the generator never forces RLP materialization mid-execution) and is faster than the capture-during-mutation approach. Remove WitnessCapturingScopedTrieStore, MultiAccountProofCollector, and StorageWitnessCollector, and revert the backend trackWitness->sink wiring (Flat, Trie, Healing); the backends no longer do any witness bookkeeping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
evm
new feature
performance is good
refactoring
rpc
state+storage
trie
labels
|
Claude finished @asdacap's task in 7m 32s —— View job PR Review
The architecture is solid: replacing FindingsHigh
The comment says "monotonic (Read → Delete only)" but the body does Medium
Low
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| { | ||
| Dictionary<UInt256, Access> slots = Touch(address).Slots; | ||
| slots[index] = value.IsZero() ? Access.Delete : Access.Read; | ||
| } |
There was a problem hiding this comment.
High — MarkSlot violates the monotonicity contract, can produce an incomplete witness
The comment on line 80 says "The tag is monotonic (Read → Delete only)", but this implementation does an unconditional overwrite. If a slot is first written to zero (Delete) and then written to non-zero again in the same block — which is a legal EVM sequence: SSTORE(s, 0) in tx1, then SSTORE(s, V) in tx2 — the second call silently downgrades the tag from Delete back to Read. The generator then never pulls in the collapse sibling for that slot, so a stateless verifier applying the delete first (the collapse-maximizing order) will fail.
Compare with MarkAccount (line 83) which correctly only ever upgrades:
if (account is null) acct.Access = Access.Delete;The fix should mirror that pattern:
| } | |
| private void MarkSlot(Address address, in UInt256 index, byte[] value) | |
| { | |
| Dictionary<UInt256, Access> slots = Touch(address).Slots; | |
| if (value.IsZero()) | |
| slots[index] = Access.Delete; // monotonic upgrade only | |
| else | |
| { | |
| ref Access existing = ref CollectionsMarshal.GetValueRefOrAddDefault(slots, index, out bool existed); | |
| if (!existed) existing = Access.Read; | |
| } | |
| } |
| // Walk a fresh read-only view at the base state root — never the live scope, which a block-level | ||
| // witness mutates — so the result reflects the pre-execution state regardless of commits done here. | ||
| IReadOnlyTrieStore roStore = readOnlyTrieStoreFactory(); | ||
| using IDisposable _ = roStore.BeginScope(baseBlock); |
There was a problem hiding this comment.
Medium — roStore is IDisposable but never disposed
IReadOnlyTrieStore extends ITrieStore which extends IDisposable. readOnlyTrieStoreFactory() creates a new store on each call. Only the scope returned by BeginScope is disposed here (via using IDisposable _ = ...), but roStore itself is not. Any native resources held by the store (e.g., a RocksDB snapshot) will leak.
| using IDisposable _ = roStore.BeginScope(baseBlock); | |
| using IReadOnlyTrieStore roStore = readOnlyTrieStoreFactory(); | |
| using IDisposable _ = roStore.BeginScope(baseBlock); |
| } | ||
|
|
||
| private sealed class RlpCollectingSink : PatriciaTrieWitnessGenerator.ISink | ||
| { |
There was a problem hiding this comment.
Medium — RlpCollectingSink is not thread-safe; ISink.Add contract requires thread safety for parallel runs
The ISink docs (in PatriciaTrieWitnessGenerator.cs) state:
"When the generator runs in parallel,
Addmay be called concurrently and must be thread-safe."
RlpCollectingSink uses a plain List<byte[]> with no synchronization, so it violates this contract. BuildWitness() always passes parallelize: false (the default), so this isn't a production bug today. But the ISink.Add contract exists precisely so the generator can be called with parallelize: true in the future without changing the sink — and silently violating it here makes that refactor unsafe.
The minimal fix is a lock or ConcurrentBag. Alternatively, document explicitly that this sink requires parallelize: false, and add an assertion or comment to that effect in BuildWitness().
| trackWitness | ||
| // Propagate trackWitness to the inner wrapper so it produces the storage (trie-node) witness. | ||
| ? _current = new AccessWitnessScope(baseProvider.BeginScope(baseBlock, true)) | ||
| : baseProvider.BeginScope(baseBlock, false); |
There was a problem hiding this comment.
Low — _current is stale when BeginScope(trackWitness: false) is called
When trackWitness is false, _current is not updated. A caller that opens a non-witness scope and then reads TouchedKeys / Codes gets data from the previous witness scope. The class comment documents the single-scope-at-a-time expectation, but this can silently return stale data if the provider is ever used outside that invariant. Consider setting _current = null on the non-witness path to surface misuse as a null-ref rather than a silent data leak.
| if (sibling is not null) | ||
| { | ||
| sibling.ResolveNode(resolver, path); | ||
| if (sibling.Keccak is not null) sink.Add(path, sibling); |
There was a problem hiding this comment.
Low — collapse sibling captured one level deep only
When an untouched sibling's root node is captured here, only that single node's RLP goes into the witness. If the sibling is itself a branch or extension, its internal sub-nodes are not collected. A stateless verifier applying the branch-collapse mutation will try to resolve the sibling's key+value (to construct the merged extension), and will fail with "missing node" if those sub-nodes aren't in the witness.
Whether this matters depends on the verifier's execution model. The existing Witness_alone_serves_reads_and_writes test exercises this code path and currently passes, which suggests the current PatriciaTree implementation only needs the sibling's root node during a collapse (it can reconstruct the new extension from the root's stored key prefix without reading deeper). Please confirm this is the intended verifier contract and add a comment here explaining why the top-level node alone is sufficient — a reader encountering this will otherwise suspect it's an oversight.
There was a problem hiding this comment.
I see you reused the idea i suggested of "simulating" the changes to read only state tree and storage trees.
2 things about this idea:
- pros: it makes the recording logic db-agnostic.
- cons: it requires re-execution of state root recomputation to capture the trie nodes then, which i was trying to do during in-flight execution to avoid any recomputation. EDIT: that said actually, my idea still required to do the account proof collection pass separately causing double trie traversal (1 for collecting proof nodes + 1 for state root recomputation) whereas yours do both at the same time, nice!
But my initial solution had overlapping recordings (which your solution does not have) : during execution (WitnessCapturingTrieStore) + account proof collection (WitnessGeneratingWorldState), which was not ideal but i did so because it was solving 2 issues:
- recording of needed child node during MaybeCombineNode (branch collapse) -- done by WitnessCapturingTrieStore
- reverted txs still need trie nodes recording for the account they touched -- done by account proof collection
It seems with your solution, the latter (reverted txs) would not cause any recording because the scope providers lie a level below the state/storage provider (holding the cached account/slot writes), i mean, the call stack is : world state -> state/storage provider -> AccessWitnessScopeProvider.AccessWitnessScope -> TrieWitnessScopeProvider.TrieWitnessScope -> regular trie store scope.
So, as the cached changes lie in state/storage provider and restored there, then they wouldn't get recorded.
I like your idea because the recording logic is more targeted / concentrated (does not need a touch for each single world state level function) but also EF's reference client doesn't really optimize stuff like nethermind client and therefore they don't really guard state accesses like we tend to do (reverted txs as mentioned, and potentially other cases which i'll be able to confirm once i finish fixing all zkevm tests, after merging this PR). So, Having the recording logic at the world state level allowed us more flexibility to force reads etc. That said, so far, the remaining failing zkevm tests were due to having excess preimages (usually 1-2 excess state access), so, having a stricter solution like yours might actually help. But one thing for sure is reverted txs still need state accesses.
| /// The witness is just a set of node RLPs that a verifier rehashes to rebuild the (partial) trie and re-apply the | ||
| /// block's changes. For wide compatibility nothing is assumed about the order in which the verifier applies those | ||
| /// changes, so the witness must cover every node that <em>any</em> ordering could touch. A recursion therefore | ||
| /// returns <c>true</c> ("treat this subtree as deleted", which drives the parent's lone-child check) whenever | ||
| /// <em>some</em> permutation of the entries could empty the subtree — not only when the net result deletes it. In | ||
| /// particular a keyed node whose key is deleted is treated as deleted even if another entry inserts a sibling that | ||
| /// would refill the slot, because the verifier may apply the delete first and transiently collapse the parent. |
There was a problem hiding this comment.
inserts/updates should happen before any deletions, that's the standard defined by EELS.
(We now sort them in PersistentStorageProvider.PerContractState.ProcessStorageChanges)
So that should simplify this generator
| { | ||
| // A self-destructed account drops its whole storage trie; only the state-trie account-delete | ||
| // collapse (covered above) matters. | ||
| if (t.StorageCleared || t.Slots.Count == 0) continue; |
There was a problem hiding this comment.
couldn't we just check t.Access == Delete here ?
|
|
||
| private sealed class TouchedAccount | ||
| { | ||
| public Access Access; // account-level: Delete iff removed (Set null) |
There was a problem hiding this comment.
Why not just use PatriciaTrieWitnessGenerator.AccessType ? as you also use a type from that class directly for RlpCollectingSink : PatriciaTrieWitnessGenerator.ISink
|
Gonna leave @hudem1 for actual implementation. This branch is just for example. |
2 participants
Changes
Generate the storage execution witness (
debug_executionWitness/proof_*) from theIWorldStateScopeProviderscope abstraction instead of intercepting every trie-node read during execution, so witness generation stays on the flat-DB fast path. Part of the stateless-witness work (#11623).IScope.Witness+BeginScope(trackWitness)and compose two self-contained wrappers:WorldState(AccessWitnessScopeProvider(TrieWitnessScopeProvider(backend))). Access produces the witnessKeys+Codes; TrieWitness produces theStatetrie-node RLPs. The storage backends (Flat / Trie /StateProvider/PersistentStorageProvider) are untouched.TrieWitnessScopeProviderrecords, per touched account/slot, whether it was read (via the scope read path — the EVM reads every account/slot before writing) or deleted (via decorating the commit write batch — accountSet(null), slotSet(zero),Clear(); i.e. SSTORE→0, SELFDESTRUCT, EIP-158 prune). After execution it runs the generator over a fresh read-only view of the base state — once on the state trie and once per touched account's storage trie — collecting the read-proof nodes and the collapse siblings a stateless verifier needs.PatriciaTrieWitnessGenerator: a mutation-free,BulkSet-style single-trie pre-state walk that, given touched paths tagged Read/Delete, reports the witness nodes — including the surviving sibling pulled in when a deletion collapses a branch (the node a plain read-path walk misses). It is read-only (never builds nodes, mutates, or commits), order-independent (covers the delete-first worst case), and faster than capturing during mutation (~0.3–0.7×, biggest win on delete-heavy blocks).WitnessCapturingTrieStore-over-CreateReadOnlyTrieStorepath, which forced execution through trie traversal and lost flat-DB speed.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
ProofRpcModuleCallTests, parameterized on both flat and trie backends, includingBlock_witness_round_trips_through_stateless_reexecution_for_a_storage_deletion: SSTORE→0 collapses a storage branch, the witness is generated, then re-executed through a stateless world state, asserting the recomputed post-state root matches with no missing node.PatriciaTrieWitnessGeneratorTests: across fuzz seeds,BulkSet-derived shapes, and targeted collapse/extension/absent/mixed cases — the sequential and parallel walks both equal a capture-during-mutation oracle, and the witness alone serves every read and recomputes the post-state root.Documentation
Requires documentation update
Requires explanation in Release Notes
Remarks
Draft. Follow-up: add a round-trip test for the account-delete path (SELFDESTRUCT / EIP-158 empty-account prune) — the wiring is symmetric to the storage-slot delete path and the generator's account-removal/collapse logic is already covered by its unit tests, but it lacks a dedicated end-to-end case.
🤖 Generated with Claude Code