Repository navigation
Refactor ChainQuery to be more flexible - #2139
evanlinjin wants to merge 9 commits into
Conversation
6faf825 to
c1ad161
Compare
bdd7608 to
45bf03a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2139 +/- ##
==========================================
+ Coverage 80.66% 80.85% +0.19%
==========================================
Files 31 32 +1
Lines 6236 6471 +235
Branches 293 296 +3
==========================================
+ Hits 5030 5232 +202
- Misses 1136 1173 +37
+ 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:
|
|
@evanlinjin the PR dependency is solved, #2038 landed on master! 🚀 🚀 |
2f5d193 to
5dc4b51
Compare
|
@oleonardolima We should have done this in one go in #2038. Now reviewers need to review 2 large refactor PRs. 😢 |
Oh, agree! Though #2038 was long overdue and getting too convoluted, I think having this follow-up won't be that bad. I'll make this review my top priority, let me know when it's ready. |
|
@oleonardolima ready for review |
noahjoeris
left a comment
noahjoeris
left a comment
There was a problem hiding this comment.
Looking good overall. I left a few minor comments
| CanonicalReason::ObservedIn { observed_in, .. } => ChainPosition::Unconfirmed { | ||
| first_seen: tx_node.first_seen, | ||
| last_seen: match observed_in { | ||
| ObservedIn::Block(_) => tx_node.last_seen, |
There was a problem hiding this comment.
It should be ObservedIn::Block(_) => None right?
If it re-enters into mempool it will be ObservedIn::Mempool
There was a problem hiding this comment.
That's a good point and worth discussing now that you've mentioned it. The PR's behavior is the same behavior that is currently on master. Let's create a ticket and discuss there.
Edit: Looks like I did change the behavior.
There was a problem hiding this comment.
Split out to #2300, as discussed.
While writing it up I checked whether this actually differed from master, and it did: master has an explicit last_seen: None here, and this PR had silently changed it to tx_node.last_seen. That was unintended in a refactor, so I have reverted the arm to None — the PR is now behaviour-preserving and the semantic question lives in #2300.
Worth noting for that discussion: the two are not always equal. txids_by_descending_last_seen filters out evicted txs, so an evicted tx with a last_seen never reaches the SeenTxs stage, but it can still be swept transitively as an ancestor of a leftover tx and land in this arm with tx_node.last_seen == Some(_). So your reasoning holds for the ordinary path, with that one hole.
There was a problem hiding this comment.
Following up — I dug into this properly and your instinct pointed at something real, but the conclusion lands the other way. tx_node.last_seen is correct and I have kept it, with a test.
The field's own doc decides it:
This value will be
Noneif the chain data was never seen in the mempool and only seen in a conflicting chain.
Two conditions, joined by "and". You are right that a tx which re-entered the mempool would be ObservedIn::Mempool — for the ordinary path ObservedIn::Block does mean "no mempool sighting". The hole is eviction: TxGraph::txids_by_descending_last_seen filters evicted txs out, so an evicted tx never reaches the mempool stage, yet it can still be swept transitively by mark_canonical as the ancestor of a stale-anchored tx. It then lands in this arm while genuinely having a last_seen.
Forcing None there gave first_seen: Some(t), last_seen: None — "first seen at t, never seen" — because first_seen is passed through on exactly the same terms one line above. That inconsistency is on master today; it is just unreachable without the evicted-ancestor path, which nothing tested.
So this is now a deliberate fix in its own commit rather than a silent change inside the refactor, with test_evicted_ancestor_of_stale_anchored_tx_keeps_last_seen covering it. #2300 is closed with the reasoning. Thanks for pushing on it — the bug would not have been found otherwise.
2d45483 to
f532064
Compare
|
do you want to resolve the conflicts before another review? |
f532064 to
0d79f61
Compare
@noahjoeris I've resolved the conflicts. However, I would like to make a few changes before another |
191de5a to
4ebeab8
Compare
|
@noahjoeris It's ready! |
3b6a49e to
f807286
Compare
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>
f807286 to
df7bdd7
Compare
Dmenec
left a comment
Dmenec
left a comment
There was a problem hiding this comment.
tACK df7bdd7
Looks good to me :) I saw rust-bitcoin 0.33 has is_satisfied_by for lock times using chain data, so maybe are_timelocks_met could use it in the future.
The description is missing the last two commits. Maybe also link #2199 and #2200 as the follow-ups?
nit: would it be possible to squash the follow-up commits into the ones they amend? It'd make it easier for other reviewers to go commit by commit.
| ("rel time unmet", &confirmed, time(3), Some(MTP), false), | ||
| ("rel time, no tip mtp", &confirmed, time(0), None, false), | ||
| ("no prev mtp", &no_prev_mtp, time(0), Some(MTP), false), | ||
| ("rel time 0, mempool", &mempool, time(0), Some(MTP), true), |
There was a problem hiding this comment.
non-blocking: only time(0) is checked for a mempool output.
| ("rel time 0, mempool", &mempool, time(0), Some(MTP), true), | |
| ("rel time 0, mempool", &mempool, time(0), Some(MTP), true), | |
| ("rel time 1, mempool", &mempool, time(1), Some(MTP), false), |
| absolute: None, | ||
| relative: Some(relative::LockTime::from_512_second_intervals(n)), | ||
| }; | ||
|
|
There was a problem hiding this comment.
non-blocking: maybe cover a transitively confirmed output, Something like:
let transitive = txout(
ChainPosition::Confirmed {
anchor: ConfirmationBlockTime {
block_id: BlockId { height: 90, hash: hash!("90") },
confirmation_time: 0,
},
transitively: Some(hash!("child")),
},
None,
);
// ...
("rel blocks met, transitive", &transitive, blocks(11), None, true),
("rel blocks unmet, transitive", &transitive, blocks(12), None, false),| ( | ||
| "both, one unmet", | ||
| &confirmed, | ||
| Timelocks { | ||
| absolute: Some(absolute::LockTime::from_consensus(100)), | ||
| relative: Some(relative::LockTime::from_height(12)), | ||
| }, | ||
| None, | ||
| false, | ||
| ), |
There was a problem hiding this comment.
non-blocking: there's no case where both locks are met
| ( | |
| "both, one unmet", | |
| &confirmed, | |
| Timelocks { | |
| absolute: Some(absolute::LockTime::from_consensus(100)), | |
| relative: Some(relative::LockTime::from_height(12)), | |
| }, | |
| None, | |
| false, | |
| ), | |
| ( | |
| "both, one unmet", | |
| &confirmed, | |
| Timelocks { | |
| absolute: Some(absolute::LockTime::from_consensus(100)), | |
| relative: Some(relative::LockTime::from_height(12)), | |
| }, | |
| None, | |
| false, | |
| ), | |
| ( | |
| "both met", | |
| &confirmed, | |
| Timelocks { | |
| absolute: Some(absolute::LockTime::from_consensus(100)), | |
| relative: Some(relative::LockTime::from_height(11)), | |
| }, | |
| None, | |
| true, | |
| ), |
| ("rel time met", &confirmed, time(2), Some(MTP), true), | ||
| ("rel time unmet", &confirmed, time(3), Some(MTP), false), | ||
| ("rel time, no tip mtp", &confirmed, time(0), None, false), | ||
| ("no prev mtp", &no_prev_mtp, time(0), Some(MTP), false), |
There was a problem hiding this comment.
nit: follow previous structure
| ("no prev mtp", &no_prev_mtp, time(0), Some(MTP), false), | |
| ("rel time, no prev mtp", &no_prev_mtp, time(0), Some(MTP), false), |
| // no sighting of its own. Ordinarily they agree — the mempool stage runs | ||
| // newest-first, so a sweep only reaches an ancestor whose own sighting is | ||
| // older — but evicted txs are filtered out of that stage. | ||
| ObservedIn::Mempool(seen_at) => Some(*seen_at).max(tx_node.last_seen), |
There was a problem hiding this comment.
A test covering the common case would be nice, an ancestor with no last_seen of its own swept by a descendant in the mempool
Description
Adds median-time-past (MTP) to canonical views. MTP needs block timestamps, which the current
bdk_core::ChainQueryinterface can't provide: it requests aVec<BlockId>and gets back only the first in-chain one (Option<BlockId>).This PR replaces
ChainQuerywith a poll-basedChainTask:B.LocalChain<Header>can return headers, which carry timestamps.pollreturns aTaskProgress:Advanced(one unit of work done),Query(heights)(fetch these),Blocked(waiting on outstanding data) orDone. This lets a task do one step per call instead of looping internally.Canonical views can then report MTP, opt-in via
with_mtp():CanonicalTxOut::prev_mtp: MTP of the block before the output's confirmation height. This is what BIP-68 relative timelocks are checked against (Core'sGetAncestor(nCoinHeight - 1)).Canonical::tip_mtp: MTP at the tip, used for BIP-113 absolute timelocks.Both are
Option<u32>.Nonemeans unknown, not "unlocked": MTP needs all 11 blocks of the window. A syncedLocalChainusually lacks them until electrum/esplora fetch them (follow-up PRs), and transitively confirmed txs have no known confirmation height.Notes to the reviewers
LocalChain::canonicalizebecomesrun_task, and the helperLocalChain::canonical_viewbecomescanonicalize.CheckPoint::median_time_paststays. It computes MTP from checkpoints you hold. The canonical view fetches blocks throughChainTask, so it works with any driver. Both use the newbdk_core::median_time_pasthelper.TaskProgress::Blockedhas no in-tree user yet. It is for a future non-blocking driver (e.g. Bitcoin Core RPC).LocalChain::run_taskresolves every queried height before polling again, so it panics onBlocked: that means the task is waiting on a height it never asked for.BlockQueries::resolve_candidatesreturns the first in-chain anchor without tie-breaking. This assumes Drop the "anchor may be a descendant" framing; introduceAnchor::confirmation_height#2298 (at most one anchor of a tx is in the best chain). If that doesn't hold, it must resolve all candidates and pick one deterministically.Changelog notice
bdk_coreChainQueryis replaced by the poll-basedChainTasktrait with aTaskProgressenum (Advanced/Query/Blocked/Done), generic over block typeB(defaultBlockHash). Drivers fetch block data by height instead of resolving candidateBlockIds.BlockQueries, a request/resolve tracker, withresolve_candidatesand theBlockCandidateResolutionenum.median_time_past(height, block_time), a BIP-113 MTP helper taking the timestamp source as a closure.CheckPoint::median_time_pastand the canonical-view MTP both use it.ChainRequest/ChainResponsetype aliases.bdk_chainCanonicalTxOut::prev_mtp(BIP-68 MTP atconfirmation_height - 1) andCanonical::tip_mtp(BIP-113 MTP at the tip), opt-in viaCanonicalViewTask::with_mtp()andLocalChain::canonicalize_with_mtp().CanonicalTaskandCanonicalViewTaskimplementChainTask(previouslyChainQuery) and take a block-type parameterB.LocalChain::canonicalizeis renamed toLocalChain::run_task, andLocalChain::canonical_viewtoLocalChain::canonicalize.ChainPosition::Unconfirmed::last_seenis now reported for an evicted tx swept canonical as the ancestor of another tx. It wasNone(or the descendant's older timestamp), givingfirst_seen: Some(_), last_seen: None.Checklists
All Submissions:
New Features:
Bugfixes: