Merge #2115: Add prev_blockhash validation to CheckPoint

c486ba7 docs: address review feedback on `prev_blockhash` validation (志宇)
031b30f docs(core): address review feedback on docs and tests (志宇)
ef35b19 fix(chain)!: make genesis immutable in `merge_chains` (志宇)
d8cb41f test(core): address review feedback on checkpoint tests (志宇)
9c0453c docs(core): Add module-level docs for `checkpoint_entry` (志宇)
3ef6ed8 feat(chain)!: Add `ApplyBlockError` for `prev_blockhash` validation (志宇)
bf6541b feat(chain)!: Relax the generic parameter for `LocalChain<D>` (志宇)
19b9006 test(core): add tests for CheckPoint::push and insert methods (志宇)
9971fda test(chain): Test `apply_update` with a single `CheckPoint<Header>` (valued mammal)
3df0609 test(chain): make `TestLocalChain` generic and add `prev_blockhash` test (志宇)
c314b25 fix(chain): `merge_chains` now takes account of `prev_blockhash`es (志宇)
f10ba0e fix(core): `Checkpoint::insert` now evicts on `prev_blockhash` mismatch (志宇)
046a771 fix(core): `push` now errors on `prev_blockhash` mismatch (志宇)
68d1ef4 feat(core): Initial work on `CheckPointEntry` (志宇)
d4bdff0 feat(core): Add `prev_blockhash` method to `ToBlockHash` trait (志宇)

Pull request description:

  Closes #2021
  Related to #2076
  Replaces #2024
  Replaces #2091

  ### Description

  This PR adds `prev_blockhash` awareness to `CheckPoint`, enabling proper chain validation when merging checkpoint chains that store block headers or similar data with previous block hash information.

  ### Notes to the reviewers

  This PR replaces some prior attempts:

  * #2024 - where we made the `CheckPoint::data` optional - however this resulted in internal complexity and an API with annoying edge cases. The tests from this PR were still useful.

  * #2091 - This second attempt had some good ideas, but was distracted from the goal of #2021. I mostly reused the `CheckPoint::insert` implementation of that PR.

  ### Changelog notice

  ```md
  Added:
  - `ToBlockHash::prev_blockhash()` - optional method to expose previous block hash
  - `CheckPointEntry` - new type for iterating with `prev_blockhash` awareness, yielding "placeholder" entries for heights inferred from `prev_blockhash`
  - `ApplyBlockError` - this is a new error type with two variants; `MissingGenesis` and `PrevBlockhashMismatch`. The second variant is a new error case introduced by `prev_blockhash` awareness.

  Changed:
  - `CheckPoint::push` - now errors when `prev_blockhash` conflicts with current tip (contiguous heights)
  - `CheckPoint::insert` - now evicts/displaces checkpoints on `prev_blockhash` conflict
  - `merge_chains` - now validates `prev_blockhash` consistency when merging
  - `LocalChain<D>` generic parameter - relaxed constraint to `D: Clone` instead of `D: Copy`.

  Fixed:
  - `merge_chains` no longer replaces the genesis block.
  ```

  ### Checklists

  #### All Submissions:

  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature

ACKs for top commit:
  evanlinjin:
    self-ACK c486ba7

Tree-SHA512: 5622a8a418034443931c8b5938e708c2222c42c05efbdac71d47a4e0ce1b4f4b0b7bcd2e1e14e2f295cc056e12c03e5750b40b1a1313cdbdf59a009d81d0b8a1

Author: evanlinjin

crates/chain/src/local_chain.rs

@@ -6,18 +6,18 @@ use core::ops::RangeBounds;
 
 use crate::collections::BTreeMap;
 use crate::{BlockId, ChainOracle, Merge};
-use bdk_core::ToBlockHash;
 pub use bdk_core::{CheckPoint, CheckPointIter};
+use bdk_core::{CheckPointEntry, ToBlockHash};
 use bitcoin::block::Header;
 use bitcoin::BlockHash;
 
 /// Apply `changeset` to the checkpoint.
 fn apply_changeset_to_checkpoint<D>(
     mut init_cp: CheckPoint<D>,
     changeset: &ChangeSet<D>,
-) -> Result<CheckPoint<D>, MissingGenesisError>
+) -> Result<CheckPoint<D>, ApplyBlockError>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     if let Some(start_height) = changeset.blocks.keys().next().cloned() {
         // changes after point of agreement
@@ -34,10 +34,10 @@ where
             }
         }
 
-        for (&height, &data) in &changeset.blocks {
+        for (&height, data) in &changeset.blocks {
             match data {
                 Some(data) => {
-                    extension.insert(height, data);
+                    extension.insert(height, data.clone());
                 }
                 None => {
                     extension.remove(&height);
@@ -48,7 +48,11 @@ where
         let new_tip = match base {
             Some(base) => base
                 .extend(extension)
-                .expect("extension is strictly greater than base"),
+                // Since `extension` is in height order, the only failure case is `prev_blockhash`
+                // mismatch.
+                .map_err(|last_cp| ApplyBlockError::PrevBlockhashMismatch {
+                    expected: last_cp.block_id(),
+                })?,
             None => LocalChain::from_blocks(extension)?.tip(),
         };
         init_cp = new_tip;
@@ -234,7 +238,7 @@ impl<D> LocalChain<D> {
 // Methods where `D: ToBlockHash`
 impl<D> LocalChain<D>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
     /// Constructs a [`LocalChain`] from genesis data.
     pub fn from_genesis(data: D) -> (Self, ChangeSet<D>) {
@@ -251,22 +255,27 @@ where
     ///
     /// The [`BTreeMap`] enforces the height order. However, the caller must ensure the blocks are
     /// all of the same chain.
-    pub fn from_blocks(blocks: BTreeMap<u32, D>) -> Result<Self, MissingGenesisError> {
+    pub fn from_blocks(blocks: BTreeMap<u32, D>) -> Result<Self, ApplyBlockError> {
         if !blocks.contains_key(&0) {
-            return Err(MissingGenesisError);
+            return Err(ApplyBlockError::MissingGenesis);
         }
 
-        Ok(Self {
-            tip: CheckPoint::from_blocks(blocks).expect("blocks must be in order"),
-        })
+        CheckPoint::from_blocks(blocks)
+            .map(|tip| Self { tip })
+            .map_err(|err| {
+                let last_cp = err.expect("must have at least one block (genesis)");
+                ApplyBlockError::PrevBlockhashMismatch {
+                    expected: last_cp.block_id(),
+                }
+            })
     }
 
     /// Construct a [`LocalChain`] from an initial `changeset`.
-    pub fn from_changeset(changeset: ChangeSet<D>) -> Result<Self, MissingGenesisError> {
-        let genesis_entry = changeset.blocks.get(&0).copied().flatten();
+    pub fn from_changeset(changeset: ChangeSet<D>) -> Result<Self, ApplyBlockError> {
+        let genesis_entry = changeset.blocks.get(&0).cloned().flatten();
         let genesis_data = match genesis_entry {
             Some(data) => data,
-            None => return Err(MissingGenesisError),
+            None => return Err(ApplyBlockError::MissingGenesis),
         };
 
         let (mut chain, _) = Self::from_genesis(genesis_data);
@@ -310,7 +319,7 @@ where
     }
 
     /// Apply the given `changeset`.
-    pub fn apply_changeset(&mut self, changeset: &ChangeSet<D>) -> Result<(), MissingGenesisError> {
+    pub fn apply_changeset(&mut self, changeset: &ChangeSet<D>) -> Result<(), ApplyBlockError> {
         let old_tip = self.tip.clone();
         let new_tip = apply_changeset_to_checkpoint(old_tip, changeset)?;
         self.tip = new_tip;
@@ -412,7 +421,7 @@ where
             match cur.get(exp_height) {
                 Some(cp) => {
                     if cp.height() != exp_height
-                        || Some(cp.hash()) != exp_data.map(|d| d.to_blockhash())
+                        || Some(cp.hash()) != exp_data.as_ref().map(|d| d.to_blockhash())
                     {
                         return false;
                     }
@@ -485,6 +494,35 @@ impl<D> FromIterator<(u32, D)> for ChangeSet<D> {
     }
 }
 
+/// Error when applying blocks to a local chain.
+#[derive(Clone, Debug, PartialEq)]
+pub enum ApplyBlockError {
+    /// Genesis block is missing.
+    MissingGenesis,
+    /// Block's `prev_blockhash` doesn't match the expected block.
+    PrevBlockhashMismatch {
+        /// The block that `prev_blockhash` should reference.
+        expected: BlockId,
+    },
+}
+
+impl core::fmt::Display for ApplyBlockError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            ApplyBlockError::MissingGenesis => {
+                write!(f, "genesis block is missing")
+            }
+            ApplyBlockError::PrevBlockhashMismatch { expected } => write!(
+                f,
+                "`prev_blockhash` doesn't match block at height {} ({})",
+                expected.height, expected.hash
+            ),
+        }
+    }
+}
+
+impl core::error::Error for ApplyBlockError {}
+
 /// An error which occurs when a [`LocalChain`] is constructed without a genesis checkpoint.
 #[derive(Clone, Debug, PartialEq)]
 pub struct MissingGenesisError;
@@ -590,18 +628,50 @@ fn merge_chains<D>(
     update_tip: CheckPoint<D>,
 ) -> Result<(CheckPoint<D>, ChangeSet<D>), CannotConnectError>
 where
-    D: ToBlockHash + fmt::Debug + Copy,
+    D: ToBlockHash + fmt::Debug + Clone,
 {
+    // Apply the changeset to produce the final merged chain.
+    fn finish<D>(
+        original_tip: CheckPoint<D>,
+        changeset: ChangeSet<D>,
+    ) -> Result<(CheckPoint<D>, ChangeSet<D>), CannotConnectError>
+    where
+        D: ToBlockHash + fmt::Debug + Clone,
+    {
+        let new_tip = apply_changeset_to_checkpoint(original_tip, &changeset).map_err(|err| {
+            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))
+    }
+
     let mut changeset = ChangeSet::<D>::default();
 
-    let mut orig = original_tip.iter();
-    let mut update = update_tip.iter();
+    let mut orig = original_tip.entry_iter();
+    let mut update = update_tip.entry_iter();
 
     let mut curr_orig = None;
     let mut curr_update = None;
 
-    let mut prev_orig: Option<CheckPoint<D>> = None;
-    let mut prev_update: Option<CheckPoint<D>> = None;
+    let mut prev_orig: Option<CheckPointEntry<D>> = None;
+    let mut prev_update: Option<CheckPointEntry<D>> = None;
 
     let mut point_of_agreement_found = false;
 
@@ -630,13 +700,18 @@ 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()) => {
-                changeset.blocks.insert(u.height(), Some(u.data()));
+                // Only append to `ChangeSet` when this is a non-placeholder checkpoint.
+                if let Some(data) = u.data() {
+                    changeset.blocks.insert(u.height(), Some(data));
+                }
                 prev_update = curr_update.take();
             }
             // Original block that isn't in the update
             (Some(o), u) if Some(o.height()) > u.map(|u| u.height()) => {
-                // this block might be gone if an earlier block gets invalidated
-                potentially_invalidated_heights.push(o.height());
+                if !o.is_placeholder() {
+                    // this block might be gone if an earlier block gets invalidated
+                    potentially_invalidated_heights.push(o.height());
+                }
                 prev_orig_was_invalidated = false;
                 prev_orig = curr_orig.take();
 
@@ -667,21 +742,36 @@ where
                     prev_orig_was_invalidated = false;
                     // OPTIMIZATION 2 -- if we have the same underlying pointer at this point, we
                     // can guarantee that no older blocks are introduced.
-                    if o.eq_ptr(u) {
+                    if o.source_checkpoint().eq_ptr(&u.source_checkpoint()) {
                         if is_update_height_superset_of_original {
                             return Ok((update_tip, changeset));
                         } else {
-                            let new_tip = apply_changeset_to_checkpoint(original_tip, &changeset)
-                                .map_err(|_| CannotConnectError {
-                                try_include_height: 0,
-                            })?;
-                            return Ok((new_tip, changeset));
+                            return finish(original_tip, changeset);
+                        }
+                    }
+                    // Update placeholder with real data (if necessary).
+                    if let Some(u_data) = u.data_ref() {
+                        if o.is_placeholder() {
+                            changeset.blocks.insert(u.height(), Some(u_data.clone()));
                         }
                     }
                 } else {
+                    // Genesis block (height 0) cannot be replaced. If the original and
+                    // update disagree on genesis, they belong to different chains.
+                    if o.height() == 0 && !o.is_placeholder() {
+                        return Err(CannotConnectError {
+                            try_include_height: 0,
+                        });
+                    }
                     // 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.
-                    changeset.blocks.insert(u.height(), Some(u.data()));
+                    //
+                    // `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);
                     }
@@ -710,10 +800,5 @@ where
         }
     }
 
-    let new_tip = apply_changeset_to_checkpoint(original_tip, &changeset).map_err(|_| {
-        CannotConnectError {
-            try_include_height: 0,
-        }
-    })?;
-    Ok((new_tip, changeset))
+    finish(original_tip, changeset)
 }