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

crates/chain/src/local_chain.rs

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

crates/core/src/checkpoint.rs

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.

crates/chain/src/local_chain.rs

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.

I'd prefer to keep finish — it's called from two places and contains a debug_assert! that documents an important invariant. Inlining would mean duplicating the assert + error mapping at both call sites. The named function also communicates intent — "we're done iterating, apply the result."

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?

If the original chain is already in a bad state, erroring would prevent recovery. The merge should be best-effort.

crates/core/tests/test_checkpoint.rs

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

crates/core/tests/test_checkpoint.rs

ValuedMammal · 2026-03-04T18:38:19Z

I'd prefer if we moved the checkpoint entry feature to a separate PR and just focus this one on validating prev_blockhash connections.

evanlinjin · 2026-03-05T05:36:50Z

I'd prefer if we moved the checkpoint entry feature to a separate PR and just focus this one on validating prev_blockhash connections.

@ValuedMammal CheckPointEntry is actually the mechanism through which prev_blockhash validation works in merge_chains — it fills in gaps between checkpoints using prev_blockhash, allowing the merge logic to detect conflicts at heights where no checkpoint exists. These two features are tightly coupled, so splitting them would mean either rewriting merge_chains without CheckPointEntry or landing CheckPointEntry first as a dependency PR.

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>

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>

- 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

Address suggestions by @nymius: - Add `Returns` and `Errors` doc sections to `CheckPoint::from_blocks` - Add test assertions for chain integrity after failed push and chain length after successful push Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.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 force-pushed the cp_entry branch 2 times, most recently 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

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

nymius reviewed Feb 20, 2026

View reviewed changes

crates/core/tests/test_checkpoint.rs Show resolved Hide resolved

nymius reviewed Feb 20, 2026

View reviewed changes

crates/core/tests/test_checkpoint.rs Show resolved Hide resolved

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

evanlinjin and others added 4 commits March 5, 2026 05:40

feat(core): Add prev_blockhash method to ToBlockHash trait

33c81e0

feat(core): Initial work on CheckPointEntry

cd4bc52

fix(core): push now errors on prev_blockhash mismatch

2282fed

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

ea04f83

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>

evanlinjin and others added 9 commits March 5, 2026 05:40

fix(chain): merge_chains now takes account of prev_blockhashes

6d2bc3c

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

9623de5

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

e61fa30

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

evanlinjin force-pushed the cp_entry branch from 7dfb4f2 to 254a6cd Compare March 5, 2026 05:42

evanlinjin mentioned this pull request Mar 5, 2026

Use core::error::Error #2132

Closed

evanlinjin requested a review from nymius March 5, 2026 06:19

evanlinjin mentioned this pull request Mar 5, 2026

When does LocalChain::apply_update invalidate checkpoints? #2095

Open

7 tasks

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evanlinjin commented Feb 13, 2026

Uh oh!

nymius commented Feb 13, 2026

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!

nymius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ValuedMammal commented Mar 4, 2026

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

evanlinjin commented Feb 5, 2026 •

edited

Loading

evanlinjin commented Mar 5, 2026 •

edited

Loading