docs: document chunk restore height placeholder safety (#15)

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<S> {
     merk: Merk<S>,
     chunk_id_to_root_hash: BTreeMap<Vec<u8>, CryptoHash>,
@@ -232,7 +267,18 @@ impl<'db, S: StorageContext<'db>> Restorer<S> {
         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<S> {
                         )
                         .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<S> {
         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<MerkSource<'s, S>>,
@@ -525,6 +580,12 @@ impl<'db, S: StorageContext<'db>> Restorer<S> {
     /// 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<Merk<S>, 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<S> {
                 ))
             })?;
 
-        // 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

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 {