From: 志宇 Date: Wed, 22 Apr 2026 06:28:04 +0000 (+0000) Subject: docs: address review feedback on `prev_blockhash` validation X-Git-Url: http://internal-gitweb-vhost/?a=commitdiff_plain;h=c486ba75865806bbe1d88c646385ac57df611ef5;p=bdk 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) --- diff --git a/crates/chain/src/local_chain.rs b/crates/chain/src/local_chain.rs index 56cde203..5c938ee4 100644 --- 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( original_tip: CheckPoint, changeset: ChangeSet, @@ -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 a9af0248..bc5151d9 100644 --- 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 2a4944a5..3d33a268 100644 --- 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 b0151ec9..675a226f 100644 --- 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 { /// A placeholder entry: there is no checkpoint stored at this height, /// but the checkpoint one height above links back here via its `prev_blockhash`.