diff --git a/merk/src/merk/restore.rs b/merk/src/merk/restore.rs index 99fbface4..576ef08f0 100644 --- a/merk/src/merk/restore.rs +++ b/merk/src/merk/restore.rs @@ -57,6 +57,41 @@ use crate::{ /// Restorer handles verification of chunks and replication of Merk trees. /// Chunks can be processed randomly as long as their parent has been processed /// already. +/// +/// # Height safety during restoration (audit finding #15, 2026-03-09) +/// +/// During chunk processing, `Link::Reference` entries are written with +/// child-height values derived from the proof tree structure within each +/// chunk. For `Node::Hash` boundaries between chunks, these heights are +/// placeholders (typically `(0, 0)`) because the actual subtree behind a +/// hash node has not yet been received. This means that between +/// `process_chunk` calls, the on-disk tree may contain links with +/// inaccurate height metadata. +/// +/// **This is safe by design for three reasons:** +/// +/// 1. **Exclusive ownership.** The `Restorer` takes full ownership of the +/// `Merk` instance. No external queries, inserts, or balancing +/// operations are served from the partially-restored tree. At the +/// GroveDB level, the Merk is wrapped inside `SubtreeStateSyncInfo` +/// within a `MultiStateSyncSession`, which is inaccessible to normal +/// database operations. +/// +/// 2. **No structural mutations.** During restoration the tree is only +/// being populated, never rebalanced. Heights are used for AVL +/// balancing decisions during inserts/deletes, but those operations +/// never occur on a restoring tree. The intermediate height values +/// therefore have no effect on correctness. +/// +/// 3. **Mandatory finalization.** Callers must invoke [`Restorer::finalize`] +/// to obtain the restored `Merk`. `finalize` runs `verify_height` +/// which performs a full recursive height audit, and if any +/// discrepancy is found, `rewrite_heights` traverses the entire tree +/// bottom-up to compute correct heights from the actual structure. +/// After height correction, `finalize` additionally calls +/// `merk.verify()` to validate the full tree integrity before +/// returning. A `Merk` with incorrect heights can never escape the +/// `Restorer`. pub struct Restorer { merk: Merk, chunk_id_to_root_hash: BTreeMap, CryptoHash>, @@ -232,7 +267,18 @@ impl<'db, S: StorageContext<'db>> Restorer { Ok(tree) } - /// Write the verified chunk to storage + /// Write the verified chunk to storage. + /// + /// Note on child heights: `Child::as_link()` produces `Link::Reference` + /// entries whose `child_heights` are derived from the proof tree built + /// by `execute()`. For KV children within the same chunk these heights + /// are correct (computed bottom-up by `Tree::attach`). For `Node::Hash` + /// children -- which represent chunk boundaries -- the heights are + /// placeholders (`(0, 0)`) because the referenced subtree has not been + /// received yet. These placeholder heights are harmless: the Restorer + /// owns the Merk exclusively (no queries or balancing occur), and + /// `finalize()` will verify and rewrite all heights before returning + /// the Merk. See the struct-level doc comment for full rationale. fn write_chunk( &mut self, chunk_tree: ProofTree, @@ -257,7 +303,9 @@ impl<'db, S: StorageContext<'db>> Restorer { ) .unwrap(); - // update tree links + // Update tree links. Heights in these links may be + // placeholders for Hash-node children (chunk + // boundaries); corrected by finalize(). *tree.slot_mut(LEFT) = proof_node.left.as_ref().map(Child::as_link); *tree.slot_mut(RIGHT) = proof_node.right.as_ref().map(Child::as_link); @@ -420,9 +468,16 @@ impl<'db, S: StorageContext<'db>> Restorer { Ok(()) } - /// Each nodes height is not added to state as such the producer could lie - /// about the height values after replication we need to verify the - /// heights and if invalid recompute the correct values + /// Recomputes all child heights bottom-up from the actual tree structure. + /// + /// Heights are not part of the cryptographic commitment (they do not + /// affect the Merkle root hash), so a chunk producer could supply + /// arbitrary height values. Additionally, during multi-chunk + /// restoration, heights for chunk-boundary links are stored as + /// placeholders. This method walks the entire tree, computes true + /// heights from the leaves up, and rewrites every node with the + /// correct values. Called by `finalize()` when `verify_height` detects + /// a discrepancy. fn rewrite_heights(&mut self, grove_version: &GroveVersion) -> Result<(), Error> { fn rewrite_child_heights<'s, 'db, S: StorageContext<'db>>( mut walker: RefWalker>, @@ -525,6 +580,12 @@ impl<'db, S: StorageContext<'db>> Restorer { /// Consumes the `Restorer` and returns a newly created, fully populated /// Merk instance. This method will return an error if called before /// processing all chunks. + /// + /// This is the only way to obtain the restored `Merk`. It guarantees + /// that all height metadata is correct before returning, regardless of + /// any placeholder heights stored during intermediate chunk processing. + /// See the struct-level doc comment on [`Restorer`] for the full safety + /// argument. pub fn finalize(mut self, grove_version: &GroveVersion) -> Result, Error> { // ensure all chunks have been processed if !self.chunk_id_to_root_hash.is_empty() || !self.parent_keys.is_empty() { @@ -546,7 +607,8 @@ impl<'db, S: StorageContext<'db>> Restorer { )) })?; - // if height values are wrong, rewrite height + // Heights written during chunk processing may be placeholders (see + // write_chunk doc comment). Verify them here, and rewrite if needed. if self.verify_height(grove_version).is_err() { self.rewrite_heights(grove_version)?; // update the root node after height rewrite diff --git a/merk/src/proofs/tree.rs b/merk/src/proofs/tree.rs index e8f1987ab..02a8f4ee8 100644 --- a/merk/src/proofs/tree.rs +++ b/merk/src/proofs/tree.rs @@ -42,6 +42,14 @@ pub struct Child { impl Child { /// Converts this child into a `Link::Reference` for use during tree /// reconstruction. + /// + /// **Height caveat (audit finding #15):** The `child_heights` stored in + /// the resulting `Link::Reference` are taken directly from this proof + /// tree node. For `Node::Hash` nodes (chunk boundaries during + /// restoration), these will be `(0, 0)` -- not the true heights of the + /// referenced subtree. This is safe because the `Restorer` owns the + /// Merk exclusively and `finalize()` verifies/rewrites all heights + /// before the Merk is returned. See `Restorer` struct-level docs. #[cfg(feature = "minimal")] pub fn as_link(&self) -> Link { let (key, aggregate_data) = match &self.tree.node {