Repository navigation
Conversation
Exposing median-time-past (MTP) on canonical views needs block
timestamps, but the `ChainQuery` interface can't carry them: its
response is `ChainResponse = Option<BlockId>` — only a hash and height.
Change the interface so the driver returns block *data* by height,
generic over a block type `B` (default `BlockHash`, usable as a full
`Header`). The driving call also moves from `next_query() -> Option<…>`
to `poll() -> TaskProgress`, whose enum expresses states `next_query`
couldn't: incremental progress (`Advanced`) and a `Blocked` state that
leaves room for a future streaming driver. This keeps the multi-stage
canonicalization a clean poll loop rather than an internal one.
With block data flowing back, `CanonicalView` gains MTP: per-output on
`CanonicalTxOut::prev_mtp`, and for the tip via `Canonical::tip_mtp`,
both computed only when opted into with `CanonicalViewTask::with_mtp()`.
`prev_mtp` is the MTP of the block *preceding* the output's confirmation
height, not the MTP at that height. That is the reference BIP-68
evaluates relative timelocks against, matching Bitcoin Core's
`CalculateSequenceLocks`:
GetAncestor(std::max(nCoinHeight - 1, 0))->GetMedianTimePast()
so it is computed at `confirmation_height.saturating_sub(1)`, the
`saturating_sub` matching Core's `max(…, 0)`. `tip_mtp` is MTP at the
tip height, which is the BIP-113 reference for absolute timelocks.
MTP is deliberately on `CanonicalTxOut` only, not `CanonicalTx`: a
relative timelock is evaluated against the coin being spent, and a
transaction has no coin. `None` means the value is unknown — MTP not
enabled, the output unconfirmed, its transaction confirmed only
transitively, or an 11-block window the chain could not fully resolve —
and must never be read as an absent or satisfied timelock.
Mechanically: rename `ChainQuery` to `ChainTask`; remove
`ChainRequest`/`ChainResponse`; add a `BlockQueries` helper for
request/resolve tracking; add `LocalChain::run_task` and the
`canonicalize{,_with_mtp}()` convenience methods.
Tests run `canonicalize_with_mtp` over a header chain whose block times
equal their heights, so each MTP is predictable. They cover `prev_mtp` and
`tip_mtp`, saturation at low heights, and `None` for an output whose
transaction is confirmed only transitively, even though the full window
is there.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
Both canonicalization phases checked a tx's anchors against fetched blocks with identical logic: scan for a resolved, matching block; else request the missing heights; else wait on in-flight queries; else give up. The two copies had already drifted (the phase-2 copy still carried the old `has_unresolved_heights` shape with a latent empty-`Query` edge). Extract that logic as `BlockQueries::resolve_candidates`, returning a `BlockCandidateResolution` enum (`Confirmed` / `Query` / `Awaiting` / `NotConfirmed`). It lives on `BlockQueries` since it is an operation on that type, and is decoupled from `Anchor`: callers pass the candidates as `(payload, BlockId)` pairs, so it needs only a `BlockId` per candidate rather than the `Anchor` trait. Each call site now maps the four outcomes onto its own queue handling and `TaskProgress`. Adds unit tests covering all four outcomes, including the `Awaiting` branch that the synchronous driver never hits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both `CheckPoint::median_time_past` and the canonical view task's MTP computation implemented the same BIP-113 median (11-block window, higher-middle value). Extract a `bdk_core::median_time_past(height, block_time)` helper — abstracting the timestamp source via a closure — so the rule lives in one place; both sites now delegate to it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An MTP at height `h` medians the timestamps at `h-10..=h`, so any height
in that window the driver cannot resolve makes the whole result `None` —
silently, since a gap is indistinguishable from a reorged-out block.
This is the normal case for a `LocalChain` built by syncing: electrum and
esplora insert a handful of checkpoints, not every height, so such a
chain yields `None` for both `prev_mtp` and `tip_mtp` until those clients
fetch the preceding window for each confirmed height.
Document the requirement on `with_mtp` and pin the behaviour with a test
over a chain holding only heights {0, 50, 100}, which also shows that the
gaps do not affect canonicalization itself.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
Gaps in the protocol between a task and its driver, one on each side, plus one at the hand-off between the two canonicalization phases. Task side: `resolve_query(height, None)` means "the chain source has no block at this height", and it is terminal — the height leaves the pending set and is never requested again. A driver that passed `None` on an I/O error would permanently record a real block as absent, turning a confirmed transaction into an unconfirmed one with no error surfaced anywhere, i.e. a silently wrong balance. The recovery path already exists: leave the height unresolved and retry it, which is what `unresolved_queries` is for. Driver side: `LocalChain::run_task` rejected `TaskProgress::Blocked` with `unreachable!()`. That was right about the outcome — this driver resolves every height a task announces via `Query` before polling again, so a task that announces what it needs never has anything outstanding — but it gave the reason as a property of the driver rather than of the task, and the panic was undocumented. Reaching that arm means the task is waiting on heights it never announced. Such a task would also hang a streaming driver, which starts fetches only for announced heights, so serving it by draining `unresolved_queries()` would hide the bug behind the one driver everyone tests against. Panic with a message that names the violation, document it under `# Panics`, and cover it with a test. Hand-off: phase 2 (`CanonicalViewTask`) is built from phase 1's `BlockQueries`, which may still hold pending heights, e.g. when phase 1 ran on another driver. Phase 2 would then wait on heights it never announced, the exact violation `run_task` now panics on. Add `BlockQueries::clear_pending` and call it when phase 2 takes over, so it reuses only resolved blocks and requests anything else itself. Cover it with a `BlockQueries` case and a phase-2 test. A streaming driver may still deliver phase 1's in-flight answers after the hand-off, so let `BlockQueries::resolve` accept answers for heights that are no longer pending instead of `debug_assert!`ing that they were. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
`BlockQueries` is half of `CanonicalTask::Output`, so without `Debug` the task's own output tuple isn't `Debug` — `dbg!(chain.run_task(task))` and `.unwrap()` on a `Result` in that position don't compile. Its neighbours `BlockCandidateResolution` and `Canonical` already derive it (Rust API guideline C-DEBUG). Covered by formatting the output tuple in the existing `CanonicalTask` test, so the derive can't be dropped without a test failing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
`ChainPosition::Unconfirmed::last_seen` is documented as `None` only when the transaction "was never seen in the mempool **and** only seen in a conflicting chain". Both arms of the `ObservedIn` match discarded a real sighting that contradicted that. `TxGraph::txids_by_descending_last_seen` filters evicted transactions out, so an evicted transaction never reaches the mempool stage of canonicalization. It can still be marked canonical *transitively*, swept by `mark_canonical` as the ancestor of a canonical descendant — and it arrives with a `last_seen` the stage never got to consider. `ObservedIn::Block` forced `None`. That gave `first_seen: Some(t), last_seen: None` — "first seen at t, never seen" — since `first_seen` is passed through on exactly the same terms one line above. Report `tx_node.last_seen` instead. `ObservedIn::Mempool` used the descendant's sighting alone. That is only a lower bound (a child cannot sit in the mempool without its parent), so it discarded a later direct observation in favour of an inferred one, and could likewise place `last_seen` before `first_seen`. Take the later of the two. Neither changes the ordinary path: a transaction with a sighting that has not been evicted is marked in the mempool stage, which runs newest-first, so a sweep only ever reaches an ancestor whose own sighting is older. That is why both were invisible. Adds a regression test per arm; nothing covered the evicted-ancestor path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NND8BhEmYTYdLRv4dbUUro
`BlockQueries` stored answers by height alone, while each `ChainTask` kept its own `tip`. Now that `resolve` accepts answers for heights that are no longer pending, nothing said which chain those answers belong to: a second answer for a height could come from a different chain (e.g. after a reorg) and silently replace the first, after a task had already acted on it. Make `BlockQueries` carry the tip its answers are relative to (`BlockQueries::new(tip)`, `BlockQueries::tip`), and have `CanonicalTask` and `CanonicalViewTask` read their tip from it instead of storing their own. Every answer is then the block at that height in the chain ending at `tip`, so a height has one right answer however often it is fetched. Drop `Default`, as there is no default tip. `CanonicalTxs::view_task` reuses the `BlockQueries` it is handed only if it is for the same tip as phase 1; otherwise phase 2 starts empty and fetches what it needs, rather than trusting blocks from another chain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`classify_outpoints` and `balance` took an `is_locked` predicate, so every caller had to compare its script's timelocks against the chain itself: the tip height (spendable in block `tip + 1`), `tip_mtp` (BIP-113), `prev_mtp` (BIP-68) and the 512-second units, and remember that an unknown MTP means locked. Take a `timelocks` callback returning the spend path's `Timelocks` (an absolute and/or relative `LockTime`) instead, and do the comparison in the view with the new `CanonicalTxOut::are_timelocks_met`, as `is_mature` already does for coinbase maturity. A lock that needs an unknown MTP is unmet. An unconfirmed output is treated as confirming in the block after the tip, as Bitcoin Core does for mempool coins. Both methods are unreleased, so the signature change is free until the next chain release. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`CanonicalTask` requested block data one anchored tx at a time, so a driver where each query is a network round-trip paid one serial round-trip per tx. When a tx needs block data, also request the anchor heights of every anchored leaf (the undecided anchored txs that are not ancestors of any other), so the stage needs a single `Query` when no anchor is stale. Txs are still processed in the same order, so conflict resolution is unchanged. After a stale anchor, only walk from the txs that failed to sweep their ancestors, which keeps this linear on long stale chains. Add tests covering the batches. Refs bitcoindevkit#2403
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2451 +/- ##
==========================================
+ Coverage 80.66% 83.74% +3.08%
==========================================
Files 31 32 +1
Lines 6236 6538 +302
Branches 293 301 +8
==========================================
+ Hits 5030 5475 +445
+ Misses 1136 997 -139
+ Partials 70 66 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title
feat(chain): batch anchor-height queries in CanonicalTaskDescription
Closes #2403. Depends on #2139. Only the last commit is new here, and I'll rebase once #2139 is merged.
CanonicalTaskrequested block data one anchored transaction at a time, so a driver where each query is a network round-trip (e.g. a Bitcoin Core RPC driver) paid one serial round-trip per transaction.This change batches anchor-height queries for all anchored leaves into a single
Querywhen no anchor is stale.When an anchor becomes stale or a conflict is detected, only the ancestors of the affected transactions are queried in subsequent rounds. This avoids unnecessary queries while keeping the results correct.
Notes to the reviewers
This only changes when heights are fetched. Transactions are still processed in the same order, so
mark_canonicalruns in the same order and conflict resolution remains unchanged.The first
Queryfinds the exact leaf set. After a stale anchor or lost conflict, only the ancestors of those transactions are fetched, starting at the nearest anchored ancestor, so their parents are batched together in the nextQuery.Recomputing the full leaf set every round was O(n²) on long chains of stale anchors (~5.75s vs ~10ms for 4,000 transactions). This approach keeps the process linear.
The trade-off is that a later round may fetch a height that is no longer needed, but it never gives a wrong result.
Tests record every
Querybatch and cover independent leaves, ancestors that are never queried, stale anchors, lost conflicts, and unchanged conflict resolution.Changelog notice
Changed
CanonicalTaskinbdk_chainnow batches anchor-height queries for all anchored leaves into a singleQuery, instead of querying one anchored transaction at a time.Checklists
All Submissions