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.

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

crates/chain/src/local_chain.rs

+#[derive(Clone, Debug, PartialEq)]
+pub enum ApplyBlockError {
+    /// Genesis block is missing or would be altered.
+    MissingGenesis,


I would split this error in two to keep a single semantic for each one: MissingGenesis and ConflictingGenesis or similar

nymius · 2026-02-09T19:21:52Z

crates/chain/src/local_chain.rs

            // 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 an actual checkpoint.


What's the definition of "actual checkpoint": one that isn't a Placeholder or one that contains some data?
Why would it be a checkpoint with no data but at the same time is not a Placeholder?
Wouldn't be better to rewrite this comment to not leave to the reader the definition of "actual checkpoint"?

"actual checkpoint" is a checkpoint that is not a placeholder, a.k.a. a checkpoint that actually contains data.

Actual checkpoints are persisted, placeholder checkpoints are not. If this is a placeholder checkpoint, we don't want to persist it as placeholder checkpoints are already implied by actual checkpoints (and there is no data to persist anyway).

Ok, so all Placeholders are "Checkpoints without data" and all "Checkpoints without data" are Placeholders, i.e., there is a one-to-one mapping between the wrapper iter and the actual structs.

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

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

crates/core/tests/test_checkpoint.rs

+    // Verify chain structure
+    assert_eq!(result.height(), 5, "tip should be at height 5");
+    // Should have: checkpoint 5 only
+    assert_eq!(


What's the difference between displaced and purged if 4 is not part of the chain?

Purged is "transitively displaced".

In this test, 5' has a prev_blockhash that displaces 4. Since, the original chain connects 4 to 6, if 4 is displaced, then 6 is purged.

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.

nymius · 2026-02-09T19:25:03Z

crates/core/tests/test_checkpoint.rs

+        blockhash: hash!("block_0"),
+        prev_blockhash: hash!("genesis_parent"),
+    };
+    let block_1 = TestBlock {


Isn't this redundant for this test case?

nymius · 2026-02-09T19:25:14Z

crates/core/tests/test_checkpoint.rs

+        blockhash: hash!("block_0"),
+        prev_blockhash: hash!("genesis_parent"),
+    };
+    let block_1 = TestBlock {


Isn't this redundant for this test case?

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.

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

evanlinjin commented Feb 13, 2026

Uh oh!

nymius commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

evanlinjin commented Feb 5, 2026 •

edited

Loading

evanlinjin Feb 13, 2026 •

edited

Loading

evanlinjin Feb 13, 2026 •

edited

Loading