docs: address review feedback on `prev_blockhash` validation

- Clarify `CheckPoint::insert` eviction semantics when `data.prev_blockhash`
  conflicts with the checkpoint at `height - 1`.
- Explain placeholder handling in `merge_chains` conflict branch where
  `u.data()` may legitimately insert `None`.
- Preserve mismatch height in `merge_chains`' fallback so release-mode
  callers get a useful `try_include_height` instead of `0`.
- Reframe the "update displaces invalid block" test comment to make the
  best-effort recovery intent explicit.
- Derive `PartialEq` on `CheckPointEntry` and fix a broken intra-doc link.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

diff --git a/crates/chain/src/local_chain.rs b/crates/chain/src/local_chain.rs
index 56cde203e3b4ce6807939e8ff53ac2cae2952a39..5c938ee47374bf2d87b150d87735e2303871dbfb 100644 (file)
--- a/crates/chain/src/local_chain.rs
+++ b/crates/chain/src/local_chain.rs
@@ -631,10 +631,6 @@ where
     D: ToBlockHash + fmt::Debug + Clone,
 {
     // Apply the changeset to produce the final merged chain.
-    //
-    // `PrevBlockhashMismatch` should never happen because the merge iteration detects
-    // `prev_blockhash` conflicts and resolves them by invalidating conflicting blocks (setting
-    // them to `None` in the changeset) before we reach this point.
     fn finish<D>(
         original_tip: CheckPoint<D>,
         changeset: ChangeSet<D>,
@@ -643,12 +639,24 @@ where
         D: ToBlockHash + fmt::Debug + Clone,
     {
         let new_tip = apply_changeset_to_checkpoint(original_tip, &changeset).map_err(|err| {
-            debug_assert!(
-                matches!(err, ApplyBlockError::MissingGenesis),
-                "PrevBlockhashMismatch should never happen"
-            );
-            CannotConnectError {
-                try_include_height: 0,
+            match err {
+                ApplyBlockError::MissingGenesis => CannotConnectError {
+                    try_include_height: 0,
+                },
+                // The merge iteration is supposed to detect `prev_blockhash` conflicts and resolve
+                // them by invalidating conflicting blocks in the changeset. Reaching this arm means
+                // either the original chain was internally inconsistent or the iteration missed a
+                // case — a bug on our side. Debug builds panic; release builds surface the height
+                // where the mismatch surfaced so the caller at least has a useful pointer.
+                ApplyBlockError::PrevBlockhashMismatch { expected } => {
+                    debug_assert!(
+                        false,
+                        "merge_chains should have resolved prev_blockhash mismatch at {expected:?}",
+                    );
+                    CannotConnectError {
+                        try_include_height: expected.height,
+                    }
+                }
             }
         })?;
         Ok((new_tip, changeset))
@@ -692,7 +700,7 @@ where
         match (curr_orig.as_ref(), curr_update.as_ref()) {
             // Update block that doesn't exist in the original chain
             (o, Some(u)) if Some(u.height()) > o.map(|o| o.height()) => {
-                // Only append to `ChangeSet` when this is an actual checkpoint.
+                // Only append to `ChangeSet` when this is a non-placeholder checkpoint.
                 if let Some(data) = u.data() {
                     changeset.blocks.insert(u.height(), Some(data));
                 }
@@ -757,6 +765,12 @@ where
                     }
                     // We have an invalidation height so we set the height to the updated hash and
                     // also purge all the original chain block hashes above this block.
+                    //
+                    // `u.data()` returns `None` when `u` is a placeholder — in that case we erase
+                    // orig's checkpoint at this height without providing replacement data. The
+                    // implied block is still recoverable via the `prev_blockhash` of the occupied
+                    // checkpoint above it in the update chain (which is handled in its own
+                    // iteration).
                     changeset.blocks.insert(u.height(), u.data());
                     for invalidated_height in potentially_invalidated_heights.drain(..) {
                         changeset.blocks.insert(invalidated_height, None);

diff --git a/crates/chain/tests/test_local_chain.rs b/crates/chain/tests/test_local_chain.rs
index a9af0248b8be35dc0734ce56be73029972d29ba7..bc5151d9c5bb5c96d9d7b2f0db94b2aba2dccc01 100644 (file)
--- a/crates/chain/tests/test_local_chain.rs
+++ b/crates/chain/tests/test_local_chain.rs
@@ -1228,7 +1228,9 @@ fn merge_chains_with_prev_blockhash() {
         // but no B exists. Update introduces A at height 1, which displaces C because
         // C's `prev_blockhash` ("B") doesn't match A's hash ("A").
         //
-        // Note: This can only happen if chains are constructed incorrectly.
+        // Note: This shape of chain isn't reachable via the public API in normal use, but the
+        // merge remains best-effort so a caller with pre-existing bad state (partial persistence,
+        // data loaded from an untrusted source, etc.) can still recover.
         TestLocalChain {
             name: "update displaces invalid block below point of agreement",
             chain: LocalChain::from_blocks(

diff --git a/crates/core/src/checkpoint.rs b/crates/core/src/checkpoint.rs
index 2a4944a51ff73569ec97c45663fcb9e4d2cd6c5f..3d33a268feee541b1ba3dcc69454c94bfb2780a1 100644 (file)
--- a/crates/core/src/checkpoint.rs
+++ b/crates/core/src/checkpoint.rs
@@ -317,8 +317,9 @@ where
     /// Inserts `data` at its `height` within the chain.
     ///
     /// If a checkpoint already exists at `height` with a matching hash, returns `self` unchanged.
-    /// If a checkpoint exists at `height` with a different hash, or if `prev_blockhash` conflicts
-    /// occur, the conflicting checkpoint and all checkpoints above it are removed.
+    /// Otherwise, if the insertion conflicts — either with an existing checkpoint at `height` (by
+    /// hash), or with the checkpoint at `height - 1` (via `data.prev_blockhash`) — every
+    /// checkpoint at or above `height` is removed.
     ///
     /// # Panics
     ///

diff --git a/crates/core/src/checkpoint_entry.rs b/crates/core/src/checkpoint_entry.rs
index b0151ec98f164f1ebd43f06d37835de765ad7686..675a226f97c3916ee42a38bec7d8e4ded4cca5c2 100644 (file)
--- a/crates/core/src/checkpoint_entry.rs
+++ b/crates/core/src/checkpoint_entry.rs
@@ -17,8 +17,8 @@ use bitcoin::BlockHash;
 
 use crate::{BlockId, CheckPoint, ToBlockHash};
 
-/// An entry yielded by [`CheckPointIter`].
-#[derive(Debug, Clone)]
+/// An entry yielded by [`CheckPointEntryIter`].
+#[derive(Debug, Clone, PartialEq)]
 pub enum CheckPointEntry<D> {
     /// A placeholder entry: there is no checkpoint stored at this height,
     /// but the checkpoint one height above links back here via its `prev_blockhash`.