From 51c194c255d64cb46a4a121ca5714b1e4b8307ca Mon Sep 17 00:00:00 2001 From: Buffrr Date: Tue, 4 Aug 2026 11:50:48 +0200 Subject: [PATCH 1/3] fix(subtree): trim non-subtree keys from both ends in SubTree::prove Same bug as the main tree's prove_nodes: keys diverging from an internal node's prefix with a 0 bit sort before the matching band, so the one-sided partition_point dropped every matching key whenever a diverge-left key came first, breaking chained proofs. --- src/subtree.rs | 15 ++++++++++++--- tests/batch_insert_proof.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/subtree.rs b/src/subtree.rs index 9d62c9b..a130dd7 100644 --- a/src/subtree.rs +++ b/src/subtree.rs @@ -425,9 +425,18 @@ impl SubTree { left, right, } => { - // Exclude keys that don't match this prefix - let end = keys.partition_point(|key| key.split_point(depth, *prefix).is_none()); - let keys = &keys[..end]; + // Exclude keys that don't match this prefix. Sorted keys that + // diverge below the prefix come before the matching band, ones + // that diverge above come after — trim both sides. + let start = keys.partition_point(|key| { + key.split_point(depth, *prefix) + .is_some_and(|p| key.direction(depth + p) == Direction::Left) + }); + let end = keys.partition_point(|key| { + key.split_point(depth, *prefix) + .is_none_or(|p| key.direction(depth + p) == Direction::Left) + }); + let keys = &keys[start..end]; // Split keys by direction at current depth let depth = depth + prefix.bit_len(); diff --git a/tests/batch_insert_proof.rs b/tests/batch_insert_proof.rs index 1c95239..d6ad2bc 100644 --- a/tests/batch_insert_proof.rs +++ b/tests/batch_insert_proof.rs @@ -84,3 +84,35 @@ fn proving_more_keys_does_not_reduce_capability() { failed_at.unwrap() ); } + +/// `SubTree::prove` mirrors the tree's prove and had the same one-sided key +/// trimming: chaining a proof from a full subtree must also support inserting +/// the proven keys. +#[test] +fn chained_subtree_proof_supports_inserting_its_own_keys() { + for existing in [2usize, 102, 1000] { + let db = tree_with(existing); + + // Prove every existing key so the subtree carries the full tree. + let all: Vec = (0..existing).map(|i| key("existing", i)).collect(); + let mut snapshot = db.begin_read().expect("read tx"); + let full = snapshot + .prove(&all, ProofType::Standard) + .expect("prove existing keys"); + + let keys: Vec = (0..900).map(|i| key("new", i)).collect(); + let mut proof = full + .prove(&keys, spacedb::subtree::ProofType::Standard) + .expect("subtree prove should succeed"); + + for (i, k) in keys.iter().enumerate() { + assert!( + proof.insert(*k, ValueOrHash::Hash(key("val", i))).is_ok(), + "tree with {} existing keys: chained proof over 900 absent \ + keys cannot insert entry {}", + existing, + i + ); + } + } +} From 48f12c980efe622480328d49320ecf5699792cfb Mon Sep 17 00:00:00 2001 From: Buffrr Date: Tue, 4 Aug 2026 16:31:01 +0200 Subject: [PATCH 2/3] fix(path): out-of-bounds read in unaligned split_point comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The non-aligned branch guarded the next-byte read with i < a.len() while indexing a[i + 1], panicking whenever a comparison scanned to self's last byte — e.g. any lookup in a tree where two keys share a ~255-bit prefix. Leaving the low bits zero is safe: a mismatch they introduce lies beyond max_bit_len and is clamped away. --- src/path.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/path.rs b/src/path.rs index db76cf4..ececb08 100644 --- a/src/path.rs +++ b/src/path.rs @@ -212,8 +212,10 @@ impl> PathUtils for T { for (i, b_byte) in b.iter().enumerate() { // Remove bits we don't care about at the start by shifiting let mut a_byte = a[i] << src_start_bit; - // We made room for some bits from the next byte - if i < a.len() { + // We made room for some bits from the next byte. Past the last + // byte the low bits stay zero; any mismatch they cause lies + // beyond max_bit_len and is clamped below. + if i + 1 < a.len() { a_byte |= a[i + 1] >> (8 - src_start_bit); } @@ -387,6 +389,23 @@ mod tests { ); } + #[test] + fn split_point_unaligned_reaching_last_byte_does_not_panic() { + use crate::path::Path; + + // A 254-bit segment compared from start=1 scans every byte of the + // 32-byte key; the last iteration used to read one byte past the end. + let key = Path([0u8; 32]); + let mut segment = PathSegment([0u8; 33]); + segment.set_len(254); + + assert_eq!( + key.split_point(1, segment), + None, + "matching unaligned segment reaching self's last byte" + ); + } + #[test] fn test_extend_from_byte() { let mut segment = PathSegment([0u8; 33]); From 25276ebf68a37000c2b41c7c3ab166853776d08f Mon Sep 17 00:00:00 2001 From: Buffrr Date: Tue, 4 Aug 2026 16:31:01 +0200 Subject: [PATCH 3/3] fix(subtree): honest error semantics for delete and merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delete now rejects keys diverging from an internal node's prefix with KeyNotFound instead of descending and surfacing IncompleteProof at a hash node. Merge no longer absorbs interior None nodes — a proof that cannot compute its own root must not merge into a valid-looking one. --- src/subtree.rs | 14 +++- tests/subtree_semantics.rs | 132 +++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 2 deletions(-) create mode 100644 tests/subtree_semantics.rs diff --git a/src/subtree.rs b/src/subtree.rs index a130dd7..0b9b448 100644 --- a/src/subtree.rs +++ b/src/subtree.rs @@ -239,6 +239,12 @@ impl SubTree { left, right, } => { + // A key diverging from the prefix is provably absent — + // descending anyway would surface IncompleteProof when the + // walk hits a hash node. + if key.split_point(depth, prefix).is_some() { + return Err(VerifyError::KeyNotFound.into()); + } let depth = depth + prefix.bit_len(); match key.direction(depth) { Direction::Right => { @@ -963,8 +969,12 @@ impl SubTree { fn merge_nodes(a: SubTreeNode, b: SubTreeNode, depth: usize) -> Result { match (a, b) { - // If either is None, return the other - (SubTreeNode::None, other) | (other, SubTreeNode::None) => Ok(other), + // A well-formed proof never contains interior None nodes (empty + // roots are handled in merge()). Absorbing one here would launder + // an unverifiable proof into a valid-looking one. + (SubTreeNode::None, _) | (_, SubTreeNode::None) => { + Err(VerifyError::IncompleteProof.into()) + } // Two hash nodes - if they're equal, keep one; otherwise can't merge (SubTreeNode::Hash(h1), SubTreeNode::Hash(h2)) => { diff --git a/tests/subtree_semantics.rs b/tests/subtree_semantics.rs new file mode 100644 index 0000000..4a9fc7c --- /dev/null +++ b/tests/subtree_semantics.rs @@ -0,0 +1,132 @@ +//! Error-semantics regressions for `SubTree`: +//! - deleting a key that provably isn't in the proof must report +//! `KeyNotFound`, not `IncompleteProof` +//! - merging must reject proofs containing interior `None` nodes instead of +//! silently absorbing them + +use spacedb::db::Database; +use spacedb::subtree::SubTreeNode; +use spacedb::tx::ProofType; +use spacedb::{Error, Hash, NodeHasher, Sha256Hasher, VerifyError}; + +fn key(tag: &str, i: usize) -> Hash { + Sha256Hasher::hash(format!("{}{}", tag, i).as_bytes()) +} + +fn tree_with(existing: usize) -> Database { + let db = Database::memory().expect("memory db"); + let mut tx = db.begin_write().expect("write tx"); + for i in 0..existing { + tx = tx + .insert(key("existing", i), b"v".to_vec()) + .expect("insert"); + } + tx.commit().expect("commit"); + db +} + +/// `base` with every bit from `from_bit` onward inverted: diverges from +/// `base` at exactly `from_bit` and takes the opposite branch at every +/// decision afterwards. +fn probe(base: &Hash, from_bit: usize) -> Hash { + let mut k = *base; + for i in from_bit..256 { + k[i / 8] ^= 0x80 >> (i % 8); + } + k +} + +/// A key the proof shows to be absent (`contains` == false) must delete with +/// `KeyNotFound`. Keys diverging inside an internal node's prefix used to +/// descend anyway and surface `IncompleteProof` when they hit a hash node. +#[test] +fn deleting_provably_absent_key_reports_key_not_found() { + // Crafted keys giving the proven path a 254-bit prefix: proving `proven` + // yields root(prefix ∅) → internal(prefix bits 1..254) → leaf, with hash + // siblings. Probes diverging inside that prefix and routed toward the + // hash sibling exercise the missing prefix check. + let proven = [0u8; 32]; + let mut deep_sibling = [0u8; 32]; + deep_sibling[31] = 0x01; // diverges from `proven` at bit 255 + let mut top_sibling = [0u8; 32]; + top_sibling[0] = 0x80; // diverges from `proven` at bit 0 + + let db = Database::memory().expect("memory db"); + let mut tx = db.begin_write().expect("write tx"); + for k in [proven, deep_sibling, top_sibling] { + tx = tx.insert(k, b"v".to_vec()).expect("insert"); + } + tx.commit().expect("commit"); + + let mut absent_probes = 0; + for from_bit in 1..256 { + let candidate = probe(&proven, from_bit); + + let mut snapshot = db.begin_read().expect("read tx"); + let proof = snapshot + .prove(&[proven], ProofType::Standard) + .expect("prove"); + + // Only probes the proof can decide about: provably absent keys. + if !matches!(proof.contains(&candidate), Ok(false)) { + continue; + } + absent_probes += 1; + + match proof.delete(&candidate) { + Err(Error::Verify(VerifyError::KeyNotFound)) => {} + other => panic!( + "probe diverging at bit {}: proof shows the key is absent, \ + expected KeyNotFound, got {:?}", + from_bit, + other.map(|_| "Ok(subtree)") + ), + } + } + assert!(absent_probes > 0, "no probe was provably absent"); +} + +/// Replaces the first `Hash` node found under `node` with `None`, +/// returning whether one was replaced. +fn null_out_first_hash(node: &mut SubTreeNode) -> bool { + match node { + SubTreeNode::Hash(_) => { + *node = SubTreeNode::None; + true + } + SubTreeNode::Internal { left, right, .. } => { + null_out_first_hash(left) || null_out_first_hash(right) + } + _ => false, + } +} + +/// A proof with an interior `None` node cannot compute a root, so merging it +/// must fail — not silently absorb the `None` and return a valid-looking +/// proof built from the other side. +#[test] +fn merging_proof_with_interior_none_fails() { + let db = tree_with(100); + let proven = key("existing", 0); + + let mut snapshot = db.begin_read().expect("read tx"); + let mut broken = snapshot + .prove(&[proven], ProofType::Standard) + .expect("prove"); + let honest = snapshot + .prove(&[proven], ProofType::Standard) + .expect("prove"); + + assert!( + null_out_first_hash(&mut broken.root), + "expected the proof to contain at least one hash node" + ); + // Sanity: the broken proof is unverifiable on its own. + assert!(broken.compute_root().is_err()); + + assert!( + broken.merge(honest).is_err(), + "merging an unverifiable proof (interior None) must fail instead of \ + laundering it into a valid-looking one" + ); +}