Add `prev_blockhash` validation to `CheckPoint` by evanlinjin · Pull Request #2115 · bitcoindevkit/bdk

evanlinjin · 2026-02-05T14:18:50Z

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:

feat(chain)!: make CheckPoint data field optional #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.
feat(chain)!: Add prev_blockhash to ToBlockHash trait #2091 - This second attempt had some good ideas, but was distracted from the goal of Ensure CheckPoint chain methods validate and link via previous blockhash #2021. I mostly reused the CheckPoint::insert implementation of that PR.

Changelog notice

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`.

Checklists

All Submissions:

I followed the contribution guidelines

New Features:

I've added tests for the new feature
I've added docs for the new feature

Additionally, `insert` now panics if the genesis block gets displaced (if it existed in the first place). Co-authored-by: valued mammal <valuedmammal@protonmail.com>

crates/chain/src/local_chain.rs

Make `TestLocalChain` and `ExpectedResult` generic over checkpoint data type `D`, allowing the same test infrastructure to work with both `BlockHash` and `TestBlock` types. Add `merge_chains_with_prev_blockhash` test to verify that `prev_blockhash` correctly invalidates conflicting blocks and connects disjoint chains. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Add comprehensive tests for CheckPoint::push error cases: - Push fails when height is not greater than current - Push fails when prev_blockhash conflicts with self - Push succeeds when prev_blockhash matches Include tests for CheckPoint::insert conflict handling: - Insert with conflicting prev_blockhash - Insert purges conflicting tail - Insert between conflicting checkpoints 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> Co-Authored-By: valued mammal <valuedmammal@protonmail.com>

Use `D: Clone` instead of `D: Copy`.

Introduce `ApplyBlockError` enum with two variants: - `MissingGenesis`: genesis block is missing or would be altered - `PrevBlockhashMismatch`: block's `prev_blockhash` doesn't match expected This replaces `MissingGenesisError` in several `LocalChain` methods: - `from_blocks` - `from_changeset` - `apply_changeset` Also adds test cases for `merge_chains` with `prev_blockhash` scenarios: - Update displaces invalid block below point of agreement - Update fills gap with matching `prev_blockhash` - Cascading eviction through multiple blocks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Explain the purpose of `CheckPointEntry` and its two variants: - `Occupied`: real checkpoint at this height - `Placeholder`: implied by `prev_blockhash` from checkpoint above Also fix typo: "atleast" → "at least" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

evanlinjin · 2026-02-07T11:46:47Z

This PR is actually ready for review now!

nymius

Approach ACK 2c29ff1

I think the "ghost" checkpoint approach is good enough for managing the new "not-really-in-chain" checkpoints.

After detaining myself for a while trying to understand for the first time the inner workings of merge_chain, I think it is hard to read, and it's going to get hairy to maintain in the future.
I will come back with some ideas to address that.

Something similar happens with the CheckPointEntry::prev recursion. Some unit test will come handy in the future.

I liked the format of TestLocalChain, the tables were very useful.

crates/chain/src/local_chain.rs

nymius · 2026-02-09T19:23:09Z

crates/core/src/checkpoint.rs

+            // Continue traversing down (if possible).
+            match cp.prev() {
+                Some(prev) => cp = prev,
+                None => break None,


When we hit None on this iterator, it means we have exhausted CheckPoint so we are beyond genesis block?

No. The original chain may not have a genesis. We will not hit this if the original chain has a genesis block.

The original chain may not have a genesis.

There is some no-buggy reason this could happen?

I haven't encountered it in person, but if there's no reason to error/panic, then we shouldn't do it.

CheckPoint is an update type, chain sources might merge chains together before forming an update (not sure).

crates/core/tests/test_checkpoint.rs

nymius · 2026-02-09T19:24:24Z

crates/core/tests/test_checkpoint.rs

+    assert_eq!(new_cp.height(), 105);
+    assert_eq!(new_cp.hash(), hash!("block_105"));
+
+    // Verify chain structure: 100, 105


This is not precisely verifying chain structure. An explicit check for both CheckPoints will be more accurate.

crates/core/tests/test_checkpoint.rs

nymius · 2026-02-09T19:26:38Z

crates/core/src/checkpoint.rs

-    /// extended.
+    /// height order, or there are any `prev_blockhash` mismatches, then returns an `Err(..)`
+    /// containing the last checkpoint that would have been extended.
    pub fn from_blocks(blocks: impl IntoIterator<Item = (u32, D)>) -> Result<Self, Option<Self>> {


Doesn't this merit an Error and Returns section?

evanlinjin · 2026-02-13T16:08:21Z

After detaining myself for a while trying to understand for the first time the inner workings of merge_chain, I think it is hard to read, and it's going to get hairy to maintain in the future.
I will come back with some ideas to address that.

@nymius thanks for looking into that! For this PR, it's good enough to just understand how merge_chains is meant to function, and ensure our tests covers all cases and edge cases. Just treat it like a black box for now and we can look into it later.

nymius · 2026-02-13T21:15:24Z

@nymius thanks for looking into that! For this PR, it's good enough to just understand how merge_chains is meant to function, and ensure our tests covers all cases and edge cases. Just treat it like a black box for now and we can look into it later.

Yes, not intention to turn this into a refactor. Something for another PR.

- Clarify displaced vs purged terminology in assertion messages - Add explicit checkpoint verification instead of only counting - Remove redundant block_1 from genesis panic tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Prevent `merge_chains` from replacing the genesis block when original and update disagree on the genesis hash. This aligns with `CheckPoint::insert` which already panics on genesis replacement. Also update the "fix blockhash before agreement point" test to operate at a non-genesis height and add a new test for conflicting genesis. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

nymius · 2026-02-20T16:36:11Z

crates/chain/src/local_chain.rs

+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for ApplyBlockError {}


core::error::Error was stabilized in 1.81, as MSRV is 1.85 now, I think we can remove this (and all the others too)

nymius · 2026-02-20T18:27:24Z

crates/chain/src/local_chain.rs

+    // 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>,
+    ) -> 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| {
+            debug_assert!(
+                matches!(err, ApplyBlockError::MissingGenesis),
+                "PrevBlockhashMismatch should never happen"
+            );
+            CannotConnectError {
+                try_include_height: 0,
+            }
+        })?;
+        Ok((new_tip, changeset))
+    }


This is not improving the comprehension in a meaningful way. I would remove this function and add the comment directly on top of the code, once moved to the end.

nymius · 2026-02-20T18:49:18Z

crates/chain/tests/test_local_chain.rs

+        // 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.


If this can only happen if chains are constructed incorrectly, shouldn't the expected behavior be to fail then?

nymius · 2026-02-20T19:09:10Z

crates/core/tests/test_checkpoint.rs

+    assert!(
+        result.unwrap_err().eq_ptr(&cp),
+        "should return self on error"
+    );


To give a full picture of the expected behavior I would check the chain still contains block 100.

nymius

The code is addressing the referenced issues, and the changes correlate with the discussion on them.
I haven't found any conceptual concerns, but would like to hear other voices @ValuedMammal.

P.S.: I wasn't expecting to do that many comments, next time I'll push them in a single review. Sorry

nymius · 2026-02-20T19:11:27Z

crates/core/tests/test_checkpoint.rs

+        "push should succeed when prev_blockhash matches"
+    );
+    let new_cp = result.unwrap();
+    assert_eq!(new_cp.height(), 101);


Check that new_cp.len() is 2.

evanlinjin and others added 4 commits February 5, 2026 08:37

feat(core): Add prev_blockhash method to ToBlockHash trait

b0fadeb

feat(core): Initial work on CheckPointEntry

04b83f3

fix(core): push now errors on prev_blockhash mismatch

820b760

fix(core): Checkpoint::insert now evicts on prev_blockhash mismatch

c2e9ade

Additionally, `insert` now panics if the genesis block gets displaced (if it existed in the first place). Co-authored-by: valued mammal <valuedmammal@protonmail.com>

github-project-automation bot added this to BDK Chain Feb 5, 2026

evanlinjin self-assigned this Feb 5, 2026

evanlinjin added the bug Something isn't working label Feb 5, 2026

evanlinjin requested a review from ValuedMammal February 5, 2026 14:19

evanlinjin moved this to Needs Review in BDK Chain Feb 5, 2026

evanlinjin requested review from nymius and oleonardolima February 5, 2026 14:20

evanlinjin commented Feb 5, 2026

View reviewed changes

crates/chain/src/local_chain.rs Outdated Show resolved Hide resolved

evanlinjin commented Feb 5, 2026

View reviewed changes

crates/chain/src/local_chain.rs Show resolved Hide resolved

evanlinjin marked this pull request as draft February 5, 2026 14:54

evanlinjin and others added 4 commits February 5, 2026 23:57

fix(chain): merge_chains now takes account of prev_blockhashes

696b8da

test(chain): Test apply_update with a single CheckPoint<Header>

e17a7e3

evanlinjin force-pushed the cp_entry branch from 209a4ee to 014c527 Compare February 5, 2026 23:57

evanlinjin and others added 3 commits February 6, 2026 09:37

feat(chain)!: Relax the generic parameter for LocalChain<D>

5a568dc

Use `D: Clone` instead of `D: Copy`.

evanlinjin force-pushed the cp_entry branch from 3f4d8cc to 2c29ff1 Compare February 7, 2026 10:42

evanlinjin marked this pull request as ready for review February 7, 2026 10:49

evanlinjin mentioned this pull request Feb 7, 2026

feat(chain)!: make CheckPoint data field optional #2024

Closed

6 tasks

nymius reviewed Feb 9, 2026

View reviewed changes

nymius reviewed Feb 20, 2026

View reviewed changes

evanlinjin added this to the Chain 0.24.0 milestone Feb 21, 2026

Comments

Conversation

evanlinjin commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

New Features:

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Feb 7, 2026

Uh oh!

nymius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Feb 13, 2026

Uh oh!

nymius commented Feb 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nymius left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

evanlinjin commented Feb 5, 2026 •

edited

Loading