Skip to content

fix(chain): make LocalChain canonicalization and request builder exts generic over D - #2439

Open
evanlinjin wants to merge 2 commits into
bitcoindevkit:masterfrom
evanlinjin:dv/replace-2438
Open

evanlinjin wants to merge 2 commits into
bitcoindevkit:masterfrom
evanlinjin:dv/replace-2438

Conversation

@evanlinjin

Copy link
Copy Markdown
Member

Description

Makes the rest of bdk_chain's accidentally BlockHash-only API generic over the checkpoint data D:

  • LocalChain::{is_block_in_chain, chain_tip, canonicalize, canonical_view}: their impl LocalChain<BlockHash> block becomes impl<D> LocalChain<D>, and apply_header* get a BlockHash block of their own. The four only use get/hash/block_id, so a LocalChain<Header> can now canonicalize a TxGraph.
  • SyncRequestBuilderExt and FullScanRequestBuilderExt are implemented for builders with any D, so revealed_spks_from_indexer, unused_spks_from_indexer and spks_from_indexer work with a CheckPoint<Header> tip.

The rest of the BlockHash-only API is that way on purpose: apply_header* build BlockHash checkpoints, the sqlite local_chain::ChangeSet stores hashes, the chain sources fetch hashes, and SyncRequestBuilder<()>::spks stays BlockHash-only so SyncRequest::builder().spks(..) keeps inferring D.

Fixes #2437

Replaces #2438, adding the LocalChain methods and leaving out its unrelated MSRV pin change (#2435 has it).

Notes to the reviewers

None of this API is released (chain-0.23.x has a non-generic LocalChain, core-0.6.x builders have no D), so there's no changelog entry.

This conflicts with #2139, which moves is_block_in_chain into the D: ToBlockHash impl and renames canonicalize/canonical_view. This is the smaller fix; #2139 can rebase onto it.

Against master, this can break type inference where nothing else pins D:

  • A LocalChain built from generic data (e.g. Hash::hash(..)) whose only use was canonicalize/canonical_view. Two tests here needed a LocalChain<BlockHash> annotation.
  • A request built with revealed_spks_from_indexer/spks_from_indexer, without chain_tip, and never passed to a chain source. Passing it to esplora/electrum sync/full_scan still infers D.

Changelog notice

None: fixes unreleased API.

Checklists

All Submissions:

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

🤖 Generated with Claude Code

evanlinjin and others added 2 commits October 26, 2026 02:22
`is_block_in_chain`, `chain_tip`, `canonicalize` and `canonical_view`
only need block hashes, which any checkpoint data provides, but they
were in `impl LocalChain<BlockHash>`, so a `LocalChain<Header>` could
not canonicalize a `TxGraph`. Make that impl block generic over `D`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`SyncRequestBuilderExt` and `FullScanRequestBuilderExt` never touch the
checkpoint data, but were only implemented for `D = BlockHash`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-project-automation github-project-automation Bot moved this to Triage in BDK Chain Oct 26, 2026
@evanlinjin
evanlinjin marked this pull request as ready for review October 26, 2026 02:28
@evanlinjin
evanlinjin requested a review from a team as a code owner October 26, 2026 02:28
@codecov

codecov Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.23%. Comparing base (7b68d28) to head (0b280d9).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2439      +/-   ##
==========================================
+ Coverage   80.66%   81.23%   +0.57%     
==========================================
  Files          31       31              
  Lines        6236     6236              
  Branches      293      294       +1     
==========================================
+ Hits         5030     5066      +36     
+ Misses       1136     1100      -36     
  Partials       70       70              
Flag Coverage Δ
rust 81.23% <ø> (+0.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@reez reez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0b280d9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

chain: SyncRequestBuilderExt/FullScanRequestBuilderExt only work with BlockHash checkpoint data

2 participants