Draft
Conversation
| } | ||
|
|
||
| let multi_proof: MultiProof = array_witness.get_hint(); | ||
| let nomt_witness: NomtWitness = array_witness.get_hint(); |
Member
There was a problem hiding this comment.
This duplicates the information we send to ZK.
NomtWitness contains operations, which has matching data to state_accesses
pub struct NomtWitness {
pub path_proofs: Vec<WitnessedPath>,
pub operations: WitnessedOperations,
}
pub struct WitnessedOperations {
pub reads: Vec<WitnessedRead>,
pub writes: Vec<WitnessedWrite>,
}
pub struct WitnessedPath {
pub inner: PathProof,
pub path: TriePosition,
}
pub struct WitnessedRead {
pub key: KeyPath,
pub value: Option<ValueHash>,
pub path_index: usize,
}| .collect::<anyhow::Result<Vec<_>>>()?; | ||
| verified_paths.sort_by(|a, b| a.0.path().cmp(b.0.path())); | ||
|
|
||
| let mut path_index_by_prefix = BTreeMap::new(); |
Member
There was a problem hiding this comment.
What is this done? This seems like this should be part of nomt
| }) | ||
| .ok_or_else(|| anyhow::anyhow!("No NOMT path proof covers write key {:?}", key))?; | ||
|
|
||
| verified_paths[matching_index].3.push((key, value)); |
Member
There was a problem hiding this comment.
What is verified_paths exactly in the end? What does it represent?
| anyhow::bail!("Missing NOMT write witness entries"); | ||
| } | ||
|
|
||
| let mut verified_paths = nomt_witness |
Member
There was a problem hiding this comment.
Suggested change
| let mut verified_paths = nomt_witness | |
| // Vec<(VerifiedPathProof, usize, KeyPath, Vec<(KeyPath, Option<ValueHash>)>)> | |
| let mut verified_paths = nomt_witness |
Member
There was a problem hiding this comment.
This is pretty complicated variable.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Was Broken
The failing test was showing that native witness generation and guest-side replay produced different public data, specifically a different final state root. The important part is that the STF logic was not the problem. The bug was in how Sovereign eplayed NOMT state updates inside the guest.
Before the fix, Sovereign effectively treated NOMT like:
That is too lossy. NOMT update verification is path-scoped, not just “all writes sorted by key”.
The new test in examples/demo-rollup/tests/prover/sp1_cpu_prover.rs:79 runs the mock SP1 guest for each witness and checks that the guest’s public outputs match the witness:
The helper in examples/demo-rollup/tests/prover/sp1_cpu_prover.rs:160 deserializes public_values from the mock proof so the test compares exactly what the guest computed.
That test is what exposed the NOMT replay bug.
The key prover-side change is in crates/module-system/sov-state/src/nomt/prover_storage.rs:537.
Before this change, the prover took NOMT’s full witness and collapsed it into a MultiProof, discarding:
Now it stores the full nomt::Witness in the Sovereign witness:
That is necessary because the verifier needs more than a flat multiproof if it wants to replay updates correctly.
The main fix is in crates/module-system/sov-state/src/nomt/zk_storage.rs:37.
The flow now is:
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:46
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:69
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:70
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:83
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:108
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:133
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:151
Support function: crates/module-system/sov-state/src/nomt/zk_storage.rs:194
Code: crates/module-system/sov-state/src/nomt/zk_storage.rs:179
Why This Fix Works
The old approach was wrong because it replayed updates as a flat batch. Reads can be checked that way, but writes cannot. NOMT updates need each write attached to a path that actually scopes that key.
The final version fixes that by:
That is why the final_state_root mismatch disappeared.
Why There Was a Second Failure on Larger Data
After the first fix, larger data still hit OpOutOfScope. That showed another issue: you could not safely rely on NOMT’s serialized write-to-path bookkeeping alone for guest replay.
The current code avoids that by deriving write grouping from actual path prefixes instead of trusting the witness path_index. The likely upstream cause is NOMT witness assembly under larger workloads; that part is an inference. The proven part is that
regrouping by prefix scope fixes the failure.
Net Effect
The PR changes the protocol from:
to:
“store full NOMT witness, verify reads globally, verify updates path-by-path”
I have updated
CHANGELOG.mdwith a new entry if my PR makes any breaking changes or fixes a bug. If my PR removes a feature or changes its behavior, I provide help for users on how to migrate to the new behavior.I have carefully reviewed all my
Cargo.tomlchanges before opening the PRs. (Are all new dependencies necessary? Is any module dependency leaked into the full-node (hint: it shouldn't)?)Linked Issues
Testing
Describe how these changes were tested. If you've added new features, have you added unit tests?
Docs
Describe where this code is documented. If it changes a documented interface, have the docs been updated?
Rendered docs are available at
https://sovlabs-ci-rustdoc-artifacts.us-east-1.amazonaws.com/<BRANCH_NAME>/index.html