diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b564a798c..efbbfe3a7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,5 @@ +# 2026-04-15 +- #2739 Fixes NOMT Verifier storage. # 2026-04-16 - #2746 Removes re-export of `DaSyncState` and `SyncStatus` from sov-modules-api. Please use `sov-rollup-interface` directly - #2744 **Manual intervention might be needed**: Adds `serde(deny_unknown_fields)`, which can fail rollup at startup if genesis config is not tidy. diff --git a/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/jmt.rs b/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/jmt.rs index 6080b23777..c219ca9529 100644 --- a/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/jmt.rs +++ b/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/jmt.rs @@ -19,4 +19,8 @@ fn test_roundtrip_jmt() { run_jmt_test(TestCase::single_read_write_different_key()); run_jmt_test(TestCase::single_read_write_same_key()); run_jmt_test(TestCase::rounds_of_same_key()); + run_jmt_test(TestCase::multi_write_distinct_keys()); + run_jmt_test(TestCase::multi_write_both_namespaces_distinct()); + run_jmt_test(TestCase::mixed_reads_and_multi_writes()); + run_jmt_test(TestCase::multi_round_multi_write()); } diff --git a/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/mod.rs b/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/mod.rs index 65dce1c289..29b67fe506 100644 --- a/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/mod.rs +++ b/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/mod.rs @@ -122,6 +122,215 @@ impl TestCase { ], } } + + pub fn multi_write_distinct_keys() -> Self { + Self { + rounds: vec![StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![], + ordered_writes: vec![ + ( + SlotKey::from_slice(b"key_1"), + Some(SlotValue::from("value_a")), + ), + ( + SlotKey::from_slice(b"key_2"), + Some(SlotValue::from("value_b")), + ), + ( + SlotKey::from_slice(b"key_3"), + Some(SlotValue::from("value_c")), + ), + ( + SlotKey::from_slice(b"key_4"), + Some(SlotValue::from("value_d")), + ), + ( + SlotKey::from_slice(b"key_5"), + Some(SlotValue::from("value_e")), + ), + ], + }, + user: Default::default(), + }], + } + } + + pub fn multi_write_both_namespaces_distinct() -> Self { + Self { + rounds: vec![StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![], + ordered_writes: vec![ + ( + SlotKey::from_slice(b"k_kernel_1"), + Some(SlotValue::from("v_a")), + ), + ( + SlotKey::from_slice(b"k_kernel_2"), + Some(SlotValue::from("v_b")), + ), + ( + SlotKey::from_slice(b"k_kernel_3"), + Some(SlotValue::from("v_c")), + ), + ], + }, + user: OrderedReadsAndWrites { + ordered_reads: vec![], + ordered_writes: vec![ + ( + SlotKey::from_slice(b"k_user_1"), + Some(SlotValue::from("v_x")), + ), + ( + SlotKey::from_slice(b"k_user_2"), + Some(SlotValue::from("v_y")), + ), + ( + SlotKey::from_slice(b"k_user_3"), + Some(SlotValue::from("v_z")), + ), + ], + }, + }], + } + } + + pub fn mixed_reads_and_multi_writes() -> Self { + Self { + rounds: vec![StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![ + (SlotKey::from_slice(b"key_read_a"), None), + (SlotKey::from_slice(b"key_read_b"), None), + (SlotKey::from_slice(b"key_read_c"), None), + ], + ordered_writes: vec![ + ( + SlotKey::from_slice(b"key_write_1"), + Some(SlotValue::from("v_1")), + ), + ( + SlotKey::from_slice(b"key_write_2"), + Some(SlotValue::from("v_2")), + ), + ( + SlotKey::from_slice(b"key_write_3"), + Some(SlotValue::from("v_3")), + ), + ], + }, + user: Default::default(), + }], + } + } + + /// Minimal trigger for the per-path routing bug fixed by commit `1e0fefd0e`. + /// + /// Two rounds, each with **two** kernel writes and zero user writes. All four + /// key/value pairs are captured from the first two rounds of + /// `test_mock_proof_public_data_matches_witnesses` (genesis + block 1). + /// + /// Bisected from a 4-round / 1000+-write capture. The bug's trigger turned out to + /// be surprisingly narrow: + /// - 1 kernel write per round: bug does NOT trigger. + /// - 2 kernel writes per round: bug triggers. + /// - 3 kernel writes per round (using the third captured write): bug does NOT + /// trigger — the witness shape changes again. + /// + /// User writes are not needed to reproduce: the pre-fix + /// `verify_multi_proof_update` diverges from `session.finish().root()` purely on the + /// kernel namespace's path proofs here. User writes were also not sufficient on + /// their own (see `HistoricalStateReader::materialize_values` — any user write + /// requires at least one kernel write, but kernel-only rounds are allowed). + pub fn routing_bug_minimal() -> Self { + Self { + rounds: vec![ + // Round 1 — seed the kernel trie with two leaves. + StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![], + ordered_writes: vec![ + ( + SlotKey::from_slice(&[2, 0]), + Some(SlotValue::from(vec![0u8; 8])), + ), + ( + SlotKey::from_slice(&[2, 7]), + Some(SlotValue::from(vec![0u8; 8])), + ), + ], + }, + user: Default::default(), + }, + // Round 2 — overwrite [2, 0] and write a longer new key [2, 6, ...]. + // The witness for this round is where the pre-fix verifier's + // verify_multi_proof_update routing diverges from session.finish(). + StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![], + ordered_writes: vec![ + ( + SlotKey::from_slice(&[2, 0]), + Some(SlotValue::from(vec![1, 0, 0, 0, 0, 0, 0, 0])), + ), + ( + SlotKey::from_slice(&[2, 6, 1, 0, 0, 0, 0, 0, 0, 0]), + Some(SlotValue::from(vec![1, 0, 0, 0, 0, 0, 0, 0])), + ), + ], + }, + user: Default::default(), + }, + ], + } + } + + pub fn multi_round_multi_write() -> Self { + let key_1 = SlotKey::from_slice(b"key_1"); + let key_2 = SlotKey::from_slice(b"key_2"); + let key_3 = SlotKey::from_slice(b"key_3"); + let value_a = SlotValue::from("value_a"); + let value_b = SlotValue::from("value_b"); + let value_c = SlotValue::from("value_c"); + Self { + rounds: vec![ + // Round 1: write three keys + StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![], + ordered_writes: vec![ + (key_1.clone(), Some(value_a.clone())), + (key_2.clone(), Some(value_b.clone())), + (key_3.clone(), Some(value_c.clone())), + ], + }, + user: Default::default(), + }, + // Round 2: read two of them, overwrite one, delete another + StateAccesses { + kernel: OrderedReadsAndWrites { + ordered_reads: vec![ + ( + key_1.clone(), + Some(NodeLeaf::make_leaf::(&value_a)), + ), + ( + key_3.clone(), + Some(NodeLeaf::make_leaf::(&value_c)), + ), + ], + ordered_writes: vec![ + (key_1, Some(SlotValue::from("value_a2"))), + (key_2, None), + ], + }, + user: Default::default(), + }, + ], + } + } } pub fn run_test( diff --git a/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/nomt.rs b/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/nomt.rs index 44ab7af92c..72807556dd 100644 --- a/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/nomt.rs +++ b/crates/module-system/sov-modules-api/tests/integration/state_tests/compute_state_update/nomt.rs @@ -18,6 +18,11 @@ fn test_roundtrip_nomt() { run_nomt_test(TestCase::single_read_write_different_key()); run_nomt_test(TestCase::single_read_write_same_key()); run_nomt_test(TestCase::rounds_of_same_key()); + run_nomt_test(TestCase::multi_write_distinct_keys()); + run_nomt_test(TestCase::multi_write_both_namespaces_distinct()); + run_nomt_test(TestCase::mixed_reads_and_multi_writes()); + run_nomt_test(TestCase::multi_round_multi_write()); + run_nomt_test(TestCase::routing_bug_minimal()); } /// Add a new read to the first round. diff --git a/crates/module-system/sov-state/src/nomt/prover_storage.rs b/crates/module-system/sov-state/src/nomt/prover_storage.rs index 09fa20da5f..9d9feb8031 100644 --- a/crates/module-system/sov-state/src/nomt/prover_storage.rs +++ b/crates/module-system/sov-state/src/nomt/prover_storage.rs @@ -536,22 +536,17 @@ fn compute_state_update_namespace( tracing::trace!(accesses = accesses.len(), "compute state update"); let mut finished = session.finish(accesses)?; if write_witness { - let nomt_witness = finished.take_witness().expect("Witness cannot be missing"); let nomt::Witness { path_proofs, - operations: nomt::WitnessedOperations { .. }, - } = nomt_witness; - // Note, we discard `p.path`, but maybe there's a way to use to have more efficient verification? - let mut path_proofs_inner = path_proofs.into_iter().map(|p| p.inner).collect::>(); - - // Sort them as required by - // Note that the path proofs produced within a crate::witness::Witness are not guaranteed to be ordered, - // so the input should be sorted lexicographically by the terminal path prior to calling this function. - // https://github.com/thrumdev/nomt/issues/904 - path_proofs_inner.sort_by(|a, b| a.terminal.path().cmp(b.terminal.path())); - - let multi_proof = MultiProof::from_path_proofs(path_proofs_inner); - witness.add_hint(&multi_proof); + operations: _, + } = finished.take_witness().expect("Witness cannot be missing"); + // Store only the `PathProof`s. `WitnessedPath.path` (a `TriePosition`) is derivable + // on the verifier from each `PathProof`'s terminal + siblings.len(), and + // `WitnessedOperations` is not consumed by the verifier (writes originate from the + // trusted execution trace). + let path_proofs: Vec = + path_proofs.into_iter().map(|wp| wp.inner).collect(); + witness.add_hint(&path_proofs); } Ok(finished) } diff --git a/crates/module-system/sov-state/src/nomt/zk_storage.rs b/crates/module-system/sov-state/src/nomt/zk_storage.rs index 26bbf867a1..b035c655fc 100644 --- a/crates/module-system/sov-state/src/nomt/zk_storage.rs +++ b/crates/module-system/sov-state/src/nomt/zk_storage.rs @@ -2,7 +2,7 @@ use std::marker::PhantomData; use nomt_core::hasher::BinaryHasher; -use nomt_core::proof::MultiProof; +use nomt_core::proof::{MultiProof, PathProof, PathUpdate, VerifiedPathProof}; use nomt_core::trie::{KeyPath, LeafData, Node, ValueHash}; #[cfg(all(feature = "test-utils", feature = "native"))] use sov_rollup_interface::common::SlotNumber; @@ -17,6 +17,89 @@ use crate::{ StorageRoot, Witness, }; +#[derive(Clone, Copy)] +struct PathProofRange { + lower: usize, + upper: usize, + path_bit_index: usize, +} + +fn validate_path_proofs_for_multi_proof(path_proofs: &[PathProof]) -> anyhow::Result<()> { + for window in path_proofs.windows(2) { + if window[0].terminal.path() >= window[1].terminal.path() { + anyhow::bail!( + "Malformed NOMT path proof hint: terminal paths must be strictly increasing" + ); + } + } + + if path_proofs.is_empty() { + return Ok(()); + } + + let mut stack = vec![PathProofRange { + lower: 0, + upper: path_proofs.len(), + path_bit_index: 0, + }]; + + while let Some(mut range) = stack.pop() { + while range.lower + 1 < range.upper { + let lower_path = path_proofs[range.lower].terminal.path(); + let upper_path = path_proofs[range.upper - 1].terminal.path(); + + if lower_path.len() <= range.path_bit_index || upper_path.len() <= range.path_bit_index + { + anyhow::bail!( + "Malformed NOMT path proof hint: terminal path ended before range divergence" + ); + } + + if lower_path[range.path_bit_index] != upper_path[range.path_bit_index] { + let mut mid = None; + for (offset, path_proof) in path_proofs[range.lower..range.upper].iter().enumerate() + { + let path = path_proof.terminal.path(); + if path.len() <= range.path_bit_index { + anyhow::bail!( + "Malformed NOMT path proof hint: terminal path ended before range divergence" + ); + } + if mid.is_none() && path[range.path_bit_index] { + mid = Some(range.lower + offset); + } + } + + let Some(mid) = mid else { + anyhow::bail!( + "Malformed NOMT path proof hint: could not bisect path proof range" + ); + }; + + stack.push(PathProofRange { + lower: mid, + upper: range.upper, + path_bit_index: range.path_bit_index + 1, + }); + range.upper = mid; + range.path_bit_index += 1; + continue; + } + + if path_proofs[range.lower].siblings.len() <= range.path_bit_index { + anyhow::bail!( + "Malformed NOMT path proof hint: missing sibling at shared depth {}", + range.path_bit_index + ); + } + + range.path_bit_index += 1; + } + } + + Ok(()) +} + /// A [`Storage`] implementation designed to be used inside the zkVM, based on NOMT. #[derive(Default, derivative::Derivative)] #[derivative(Clone(bound = "S: MerkleProofSpec"), Debug(bound = ""))] @@ -42,7 +125,42 @@ impl NomtVerifierStorage { ordered_writes: state_writes, } = state_accesses; - let multi_proof: MultiProof = array_witness.get_hint(); + // The hint is UNTRUSTED. Every `PathProof` is authenticated against the trusted + // `prev_root` below: per-path via `PathProof::verify` (used for writes) and in + // aggregate via `verify_multi_proof` (used for reads). Write values are hashed + // from the trusted `state_writes` (execution trace) — witness-reported values are + // never used. + let mut path_proofs: Vec = array_witness.get_hint(); + // Sort once: `MultiProof::from_path_proofs` expects terminal-path order, and + // `verify_update` below also expects its paths ascending — same key, one sort. + path_proofs.sort_unstable_by(|a, b| a.terminal.path().cmp(b.terminal.path())); + validate_path_proofs_for_multi_proof(&path_proofs)?; + + // Writes: verify each path against prev_root up front. Doing this first lets us + // hand ownership of `path_proofs` to `MultiProof::from_path_proofs` below without + // cloning — the latter consumes the vec, and cloning a full `Vec` + // (each with its own `Vec` of siblings) is non-trivial proving cost in a + // zkVM. Per nomt_core's `PathProof::verify` contract, the `key_path` argument can + // be any key that looks up this terminal and must be at least `siblings.len()` + // bits long; `pp.terminal.path()` is exactly the bit-path that reached the + // terminal during proof construction and satisfies both conditions. + // + // `path_proofs` is already in ascending path order, so `verified_paths` inherits + // the ordering that `verify_update`'s PathsOutOfOrder check requires below. + type VerifiedPathWithPendingWrites = (VerifiedPathProof, Vec<(KeyPath, Option)>); + let mut verified_paths: Vec = path_proofs + .iter() + .map(|pp| { + let verified = pp + .verify::>(pp.terminal.path(), prev_root) + .map_err(|e| anyhow::anyhow!("Failed to verify path proof: {:?}", e))?; + Ok((verified, Vec::new())) + }) + .collect::>>()?; + + // Reads: rebuild a MultiProof from the path proofs and verify against prev_root, + // then confirm each read via the existing NOMT primitives. + let multi_proof = MultiProof::from_path_proofs(path_proofs); let verified_multi_proof = nomt_core::proof::verify_multi_proof::>( &multi_proof, prev_root, @@ -77,30 +195,54 @@ impl NomtVerifierStorage { } } - let mut updates = state_writes + // Hash and sort writes globally. Disjoint Patricia paths ⇒ each write routes to + // exactly one bucket, and global ascending key order ⇒ each bucket ends up + // locally ascending (OpsOutOfOrder check). + let mut sorted_writes: Vec<(KeyPath, Option)> = state_writes .into_iter() .map(|(key, value)| { - ( - S::Hasher::digest(key.as_ref()).into(), - value.map(|slot_value| { - // Authenticated write is hash of a combination of size and orignal value hash. - S::Hasher::digest(slot_value.combine_val_hash_and_size::()) - .into() - }), - ) + let key_hash: KeyPath = S::Hasher::digest(key.as_ref()).into(); + let value_hash = value.map(|slot_value| { + // Authenticated write is hash of a combination of size and original value hash. + S::Hasher::digest(slot_value.combine_val_hash_and_size::()).into() + }); + (key_hash, value_hash) }) - .collect::)>>(); + .collect(); + sorted_writes.sort_unstable_by(|a, b| a.0.cmp(&b.0)); + + // Route writes with a single forward sweep. Both vectors are globally sorted, and + // each write belongs to exactly one disjoint verified path, so once a key falls + // past a path we never need to revisit it. + let mut path_idx = 0; + for (key_hash, value_hash) in sorted_writes { + while path_idx < verified_paths.len() + && verified_paths[path_idx] + .0 + .confirm_nonexistence(&key_hash) + .is_err() + { + path_idx += 1; + } - // Sort them by key hash, as required by [`nomt_core::proof::verify_multi_proof_update`] - updates.sort_by(|a, b| a.0.cmp(&b.0)); + let Some((_, writes)) = verified_paths.get_mut(path_idx) else { + anyhow::bail!("No NOMT path proof covers write key: {:?}", key_hash); + }; + writes.push((key_hash, value_hash)); + } - nomt_core::proof::verify_multi_proof_update::>( - &verified_multi_proof, - updates, - ) - .map_err(|e| anyhow::anyhow!("Failed to verify update: {:?}", e)) - // Note: we don't check exhaustion of the proof - // because it does not impact the correctness of the guest, only performance. + let mut updates = Vec::new(); + for (verified, writes) in verified_paths { + if !writes.is_empty() { + updates.push(PathUpdate { + inner: verified, + ops: writes, + }); + } + } + + nomt_core::proof::verify_update::>(prev_root, &updates) + .map_err(|e| anyhow::anyhow!("Failed to verify update: {:?}", e)) } } @@ -258,3 +400,76 @@ impl crate::storage::NativeStorage for NomtVerifierStorage; + + let witness = ArrayWitness::default(); + let path_proof = PathProof { + terminal: PathProofTerminal::Terminator(TriePosition::from_path_and_depth( + [0; 32], 256, + )), + siblings: Vec::new(), + }; + witness.add_hint(&vec![path_proof.clone(), path_proof]); + + let err = NomtVerifierStorage::::compute_state_update_namespace( + OrderedReadsAndWrites::default(), + &witness, + nomt_core::trie::TERMINATOR, + ) + .expect_err("malformed path proof hints should return an error"); + + assert!(err + .to_string() + .contains("terminal paths must be strictly increasing")); + } +} diff --git a/examples/demo-rollup/tests/prover/sp1_cpu_prover.rs b/examples/demo-rollup/tests/prover/sp1_cpu_prover.rs index 3c7254dba8..47ce3175af 100644 --- a/examples/demo-rollup/tests/prover/sp1_cpu_prover.rs +++ b/examples/demo-rollup/tests/prover/sp1_cpu_prover.rs @@ -108,6 +108,34 @@ fn default_prover_address() -> ::Address { ::Address::try_from([0u8; 28].as_ref()).unwrap() } +#[tokio::test(flavor = "multi_thread")] +#[ignore = "manual heavy test"] +async fn test_mock_proof_public_data_matches_witnesses() { + // Use the mock prover: CPU proving is far too slow to run in tests. + std::env::set_var("SP1_PROVER", "mock"); + + let host = TestHost::new().await; + let (_genesis_state_root, witnesses) = super::generate_witnesses().await; + let prover_address = ::Address::try_from([0u8; 28].as_ref()).unwrap(); + + for witness in witnesses { + let expected_initial_state_root = witness.initial_state_root; + let expected_final_state_root = witness.final_state_root; + let expected_slot_hash = witness.da_block_header.hash(); + + let public_data = host + .run_mock_public_data(ProofInput { + stf_witness: witness, + prover_address, + }) + .await; + + assert_eq!(public_data.initial_state_root, expected_initial_state_root); + assert_eq!(public_data.final_state_root, expected_final_state_root); + assert_eq!(public_data.slot_hash, expected_slot_hash); + } +} + // The SP1 prover manages its own Tokio runtime, which conflicts with the `tokio::test` runtime. // To avoid this, all blocking work must be executed inside `tokio::task::spawn_blocking`. struct TestHost { @@ -134,6 +162,23 @@ impl TestHost { .await .unwrap() } + + async fn run_mock_public_data( + &self, + data: ProofInput, + ) -> StateTransitionPublicData<::Address, MockDaSpec, ProofStateRoot> { + let mut host = self.host.clone(); + let method_id = host.method_id(); + tokio::task::spawn_blocking(move || { + let proof = host + .add_hint_and_run(&data) + .expect("Mock prover should run successfully"); + + SP1Verifier::verify(&proof, &method_id).expect("Mock proof should verify successfully") + }) + .await + .unwrap() + } } async fn verify(