fix(bdk_electrum_streaming)!: Give the chain and anchors one owner - #13
Conversation
LLFourn
left a comment
There was a problem hiding this comment.
Thanks for this — the diagnosis in #12 is right, the chain-driven design is the right
answer to it, and the write-up made the review much faster than it would otherwise have
been. I reproduced the bug against a live electrs and confirmed this branch fixes it.
Requesting changes. The first two are defects in the new code, both on the path this PR
exists to fix, and I have a failing test for each; the third is the modelling issue they
share, which I think is the one worth acting on first:
-
state.rs:256— the stale-response guard covers the successful merkle answer but
not the error answer. A server answers from the chain it had when it received the
request; if a reorg has landed since, that error is about a block we have left
behind.give_up_on_anchorblames it on the replacement block and writes two
irreversible records — a permanentfailed_anchorsverdict, and the deletion of the
txid fromtxids_by_height, which is the only record that would let the eviction
path ask again. Sinceadvance_anchorstreats a failed anchor as resolved by
omission, the job reports success with the anchor silently missing and the
transaction stops being canonical for good. Three-line fix. -
anchor_job.rs:55— the refetch depends on the server answering in request order.
A same-height reorg is applied byChainJob's short-circuit without ever caching the
replacement header, so the job asks for the header and the proof together. Proof
first ⇒cancel_jobs⇒anchor_job = None, unrecoverable; the header arriving
second returnsOk(None)without advancing anything, so it cannot rescue it either.
The protocol carries request ids precisely because ordering is not promised.
Underneath both: failed_anchors is a monotonic negative cache, which is only sound
for negatives that were proved. Right now it has two writers — a merkle root mismatch
(a proof) and any JSON-RPC error on get_merkle (not one). Keeping the connection up on
an error is right and well argued; recording a permanent disproof from it is the part I
would drop. Enforcing "only a verified proof writes failed_anchors" would make defect 1
unwriteable rather than fixed.
state.rs:468—failed_anchorsis a monotonic negative cache, and one of its two
writers has not proved anything. A merkle root mismatch is a proof; a JSON-RPC error
is not, and the predicate is the method name alone. So a rate limit, a syncing index or
a daemon hiccup each permanently unconfirm a transaction. Worth stressing that the fix
in (1) does not cover this — that guard closes the stale-chain case, this is a fault
on the current chain. I ran the live unconfirming reorg against electrs to see what
real classification would have to work with: the payload is a bare JSON string,
"tx not found or is unconfirmed", which conflates the fault with the benign case. So
I'd drop the durable write rather than try to classify it, and keep a classifier only
as an optimisation.
The rest is smaller: cancel_jobs(JobId::Anchor) being terminal for the one job nothing
rebuilds; the GetHeader generation guard passing all 5 tests when deleted; the
breaking-change list missing SpkJobStage; txids_by_height retaining far more than the
~21 heights that can ever be read, which I'd like bounded. Details inline.
On testing — tests/env.rs runs fine here (cargo test --test env, 3 passed, 20s), so
the live path was never exercised in either direction. I have written two live tests:
one reproduces #12 end to end (fails on main, passes here), the other pins the
connection-survival fix (fails at 388d60a, passes here). Happy to hand both over along
with the fixes as a patch.
b35e9dd to
a029317
Compare
a029317 to
1542f99
Compare
370ed36 to
ba5805e
Compare
539f5ac to
fa7b73a
Compare
88ba685 to
674a2a3
Compare
Four warnings already on `main`: three needless borrows and a `&mut` handed to a function that only reads. Cleared first so every commit that follows is clean under `--all-targets`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014y3Urq6cX8uoB46Ck4WbQ7
`try_finish` had its two log messages on the wrong branches, reporting "not finished" on completion and vice versa. `elapsed_seconds` subtracted without saturating, so a backwards clock step would panic a log line.
8355c4d to
61079e1
Compare
641b837 to
9fc90ad
Compare
An Electrum status is a hash over txid-height pairs, so a reorg moving a transaction into a different block of the same height leaves it untouched and the server never notifies the script. The anchor keeps pointing at a block no longer in the chain and nothing asks again, so the transaction stops being canonical for good. The chain tip is the one thing that reports it, so repair is driven from there: every affected script gets the notification the server will not send, replayed from the status and history already cached, at no round-trip. Four ways the anchors that produces could still be wrong, each with a test. A reorg landing mid-pass mixed anchors from two chains. A proof was verified against a header that had not arrived yet. A merkle error and a mismatching proof were both read as disproofs of our own block, when neither says anything about it — `Cache::failed_anchors` goes with them, since it could only ever hold an artifact of two chains disagreeing. And a response answered before a reorg was applied after it. `reorg_to_same_height_block_refetches_anchor_live` fails on `main`. BREAKING CHANGE: `Cache::failed_anchors` is removed; `Cache` gains `spk_statuses` and `spk_hashes_by_height`, so struct-literal construction no longer compiles (`Cache::default()` is unaffected). `SpkJobStage::ProcessingTxsAndAnchors` gains an `anchors_resolved` field. `ReqCoord::pop` returns `Option<PoppedRequest>` rather than `Option<(JobRequest, BTreeSet<JobId>)>`, and `ReqCoord::clear` is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014y3Urq6cX8uoB46Ck4WbQ7
Group the server's per-script histories behind `SpkHistories`, holding each status with the history it stands for so the two cannot desync, and keep the height index private since it is derived from them. Move it and `Cache` to `cache.rs` and give both serde impls. The height index is rebuilt on load rather than stored, and `anchors` is written as a sequence because its tuple key cannot be a JSON map key. BREAKING CHANGE: `Cache::spk_histories` keeps its name but changes type from `HashMap<ElectrumScriptStatus, Vec<response::Tx>>` to `SpkHistories`, and `Cache` loses `spk_statuses` and `spk_hashes_by_height` to it. The free `SPK_HASHES_BY_HEIGHT_HORIZON` is now `SpkHistories::HEIGHT_INDEX_HORIZON`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a95d99f to
d3ff1b1
Compare
Anchors lived in `SpkJob`, so every script resolved them against a tip none of them could move, and the chain layer had to synthesise script notifications to drive its own reorg repair. One layer fabricating the other's events to do its own work means the boundary is wrong. `ConfirmationJob` replaces `ChainJob` and owns the chain and the anchors, leaving `SpkJob` the history, its transactions and their prevouts. It is held only until every script has its history, since the heights those name are all it reads, so header and proof fetches no longer queue behind transaction downloads. `State` stages one `Update` that both kinds write into and hands it over once both are finished, so a transaction is never published ahead of the chain that anchors it. Also fixed here, each with a test: - Anchor scope comes from the statuses `Subscriptions` holds, not the jobs that happen to be live, so a reorg re-verifies every script. - A replaced job's in-flight requests are forgotten, and a header batch is checked against the tip that was actually announced. Neither subsumes the other, and both let us settle on a chain the server has left. - Two scripts paid by the same transactions share a status, so a history is dropped only once no script answers to it. - A history that cannot hash to the status a job awaits ends that job instead of being re-asked on every answer, unbounded. - `SpkJob::poll` returns `Result`, retiring an `unimplemented!` that predates this work: a server answering with a mismatched transaction panicked the state thread. - A proof the server refuses cancels the job rather than being recorded against our block, since an error proves nothing about it either way. A script notification builds a job as well as a tip does, so a disagreement below the reorg window recovers without waiting on a block. The tip notification carries its own header, so it is never fetched again, and a batch that does not link up to it is a chain we were never told about. `Cache` splits by who can reconstruct it. `Subscriptions` is persisted, because no wallet stores an Electrum status and a dropped transaction is never mentioned again; `TxCache` is not, because the caller's wallet already holds all of it. Named for what it owns rather than what it produces: it settles where transactions sit in the chain, and only two of an `Update`'s three parts come from it — the rest are the scripts'. `chain_job.rs` becomes `confirmation_job.rs`. Fixes #12. Fixes #19. BREAKING CHANGE: `ChainJob` and `ChainJobOutcome` are replaced by `ConfirmationJob`, `ConfirmationStage` and `ConfirmationProgress`; `JobId::Chain` is `JobId::Confirmation`, and `JobRequest::GetHeader` is removed. `State::advance` is `State::poll`. `SpkJob::poll` takes `&mut self` and returns `anyhow::Result<SpkProgress>`; `SpkJobStage` and `TxsJobStage` are replaced by `SpkStage`, and `SpkJob::take_tx_update` is gone. `ConfirmationStage` gains `Idle`, so exhaustive patterns over it no longer compile; `ConfirmationJob::is_done` is true only for `Done` and `ConfirmationJob::set_idle` is added, while `ConfirmationJob::reset` is removed. `SpkHistories` is `Subscriptions`, keyed by status; its `remove`, `insert`, `get` and `status` are `remove_spk`, `insert_spk`, `spk_history` and `spk_status`, and its height index (`spk_hashes_at_heights`, `prune`, `HEIGHT_INDEX_HORIZON`) is gone. `Cache::{txs, anchors, spk_txids}` are now `Cache::tx_cache::{…}`, and a serialized `Cache` no longer carries them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014y3Urq6cX8uoB46Ck4WbQ7
… asked for A `blockchain.transaction.get` response was filed under the txid the request named, never under the one the transaction actually hashes to. A server answering with a different transaction put it in the cache under an id that is not its own, and every prevout later resolved through it came from the wrong transaction — a caller would see inputs that were never spent. Nothing downstream could catch it. `SpkJob::poll` errors only in the narrow case where the substitute is too short to reach a spent vout, which a server picking any longer transaction sails past. Predates this branch.
The surface this branch changes is breaking in several directions — jobs, stages, `Cache`'s shape and what a serialized one carries — so the minor bump rather than a patch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014y3Urq6cX8uoB46Ck4WbQ7
d3ff1b1 to
0a756eb
Compare
…index per notification 40e0695 fix(bdk_electrum_streaming): Keep the highest last active index per notification (志宇) 0243bbe test(bdk_electrum_streaming): Cover `last_active_indices` emission (志宇) Pull request description: ## Context Follow-up on #3 ("Fix off-by-one error in last_active_index emission"). **The off-by-one from #3 is already fixed.** At the time the issue was filed, `handle_script_status` returned `this_index + 1` and that value was emitted directly as the keychain's last active index. Commit 4447f17 ("fix: Off-by-one `last_active_indices` update", May 2025) changed it to return `this_index`, and every emission site on `main` today reports the index of the spk that actually has history. The first commit here adds a state-level regression test that pins that behaviour so it cannot silently regress. While covering it, I found a live bug with the same consequence (an active spk never gets revealed) — not an off-by-one, and not the one this PR originally described. **This PR was rebased onto `main` after the "Give the chain and anchors one owner" refactor (#13) and the `State::start` refactor (#17) landed.** Those refactors moved `last_active_indices` bookkeeping from `advance_spk_jobs`/an inline notification handler into a unified `on_spk_status` + `poll_spk_jobs`. The original bug this PR fixed (`advance_spk_jobs` folding several job completions into one update via `BTreeMap::extend`, iterated in script-hash order) no longer exists in that form — but the refactor introduced a different bug with the same symptom, described below. ## The bug on current `main` `on_spk_status` stages a keychain's last active index with a plain `BTreeMap::insert(k, i)` whenever a script hash notification (or subscribe response) names history. Electrum notifies each subscribed script hash independently, in an order unrelated to derivation index — a later-derived spk can be notified before an earlier one. The last notification processed wins, so the staged index can end up lower than the highest spk that actually has history. That index is what `reveal_to_target_multi` reveals to, so the higher spk stays unrevealed and the wallet does not recognise its txouts as its own. Separately, `poll_spk_jobs` had its *own* write to the same field, derived from mere job completion rather than from a notification naming history. That made it fire for *every* finished spk job, active or not — including every empty look-ahead spk subscribed alongside the active ones, which is the common case. `on_spk_status` already stages the index for every spk that actually has history, before its job even runs, so this second write was both redundant for active spks and actively wrong for inactive ones (it would tag an empty look-ahead spk as "active" at whatever index it happened to be, clobbering the real one). Removed. ## Changes **`test(bdk_electrum_streaming): Cover last_active_indices emission`** Adds two state-level tests: - `last_active_index_is_index_of_active_spk`: pins the off-by-one fix (passes on `main`). - `last_active_index_is_highest_regardless_of_notification_order`: drives a sync where two spks of the same keychain (indices 3 and 4) each have history, and delivers their script-hash notifications with the *higher* index notified first. Fails on `main` with `left: [("external", 3)], right: [("external", 4)]`. **`fix(bdk_electrum_streaming): Keep the highest last active index per notification`** Makes `on_spk_status`'s insert a max-merge instead of an overwrite, and removes the redundant/harmful write in `poll_spk_jobs`. Passes with both tests above. ## Testing `cargo fmt --check`, `cargo clippy --lib --tests -D warnings`, and `cargo test` (lib + `tests/state.rs`) all pass. `tests/env.rs` could not be run here: its `bdk_testenv` dev-dependency builds `bitcoind`, whose build script downloads Bitcoin Core from `bitcoincore.org`, which this sandbox's egress proxy blocks. That file is untouched by this PR, but it is worth a CI run. --- _Generated by [Claude Code](https://claude.ai/code/session_017FAFd2PPjDZAfgP35zeQNN)_ Top commit has no ACKs. Tree-SHA512: be03114b8b1aa1aa6eb95e911834fb41898eb8b3fde299653566308a36e42f5afcf00df131154b9b70a625a1bf55fca8004091d112d2db61f6aa378f945f449e
Fixes #12. Fixes #19.
A reorg that moves a transaction into a different block of the same height leaves its Electrum script status untouched — the status is a hash over txid-height pairs — so the server never notifies the script hash. The anchor already delivered goes on pointing at a block that is no longer in the chain, nothing ever asks again, and the transaction stops being canonical for good.
Repair has to be driven from the chain tip, which is the one thing that does report the eviction. Doing that showed the split was in the wrong place. Anchors lived in
SpkJob, so every script resolved them against a tip none of them could move — hence the resolve-the-whole-set-each-pass rule — and the chain layer had to synthesise script notifications to drive its own reorg repair. When one layer fabricates the other's events to do its own work, the boundary is wrong.So
ConfirmationJobreplacesChainJoband owns both the chain and the anchors, leavingSpkJobthe history, the transactions in it and their prevouts. Header fetches batch intoblockchain.block.headersinstead of oneblockchain.block.headerper anchor.Statestages a singleUpdatethat both job kinds write into and hands it over once both are finished, so a transaction is never published ahead of the chain that anchors it.ConfirmationJobfinishing is not enough on its own: it keeps offering the update until the last script is done with it. It is held back only until every script has its history, not until the scripts finish: the job works from the heights those histories name, so a script still downloading its own transactions has already told it every block it needs. Waiting for the downloads serialised the header and proof fetches behind them for nothing. Anchor scope comes from the statusesSubscriptionsholds rather than from whichever jobs happen to be live, so a reorg re-verifies every script and not just the last one to notify.Cachesplits by who can reconstruct it.Subscriptions— per-script status and history — is persisted, because a status is a hash Electrum computes over the history it stands for that no wallet stores, and the server reports a history as it stands now, never again mentioning a transaction it dropped.TxCache— transactions, anchors, and which transactions paid a script — is not, because all of it is in the caller's wallet already; seeding it from wallet data is a caller's job, and aTxCachethat starts empty is correct but re-downloads everything.response::TxisDeserialize-only upstream, so histories round-trip through a private mirror until bitcoindevkit/electrum_streaming_client#20 lands.Two ways to settle on a chain the server has left are fixed too, and neither subsumes the other (#19).
ReqCoord::forget_jobdrops the superseded job's in-flight requests, so the replacement's identicalrequest::Headersis not deduplicated against an answer already on its way from the abandoned chain. And a header batch has to link up to the tip that was actually announced, so a batch answered from a chain we were never told about is abandoned rather than adopted — the request there is current, only the answer is not. The notification carries its own header, so that header is never re-fetched and instead sits at the top of the run the batch must join onto; the linkage walk is what makes a mismatch visible.Keying histories by status brought two faults of its own, both fixed here. A self-transfer with change pays two scripts the same transactions, so they share one status; dropping the outgoing status's history whenever one script moved on took it out from under the other, which then has no history and no notification coming, since its own status has not changed. The history is dropped only once no script answers to it. And a job waits for the history its notification's status stands for, so a server answering with a different one — it reorged since notifying, or the script's transactions dropped out entirely — left the job unable to find its own and asking again on every answer, unbounded and without backoff.
SpkJob::awaiting_historycompares the two, and the job is dropped rather than re-asked: the status the server actually holds is already on its way.Breaking changes (→
0.6.0)ChainJobandChainJobOutcomeare gone, replaced byConfirmationJob,ConfirmationStageandConfirmationProgress.JobId::ChainisJobId::Confirmation, andchain_job.rsbecomesconfirmation_job.rs.Update's three parts come from it —chain_updateandtx_update.anchors. The rest are the scripts', andStateassembles them.State::advanceisState::poll.SpkJob::polltakes&mut selfand returnsanyhow::Result<SpkProgress>;SpkJobStageandTxsJobStageare replaced bySpkStagewith an explicitDone. Additive:SpkJob::awaiting_history.SpkJob::pollis fallible so that a server answeringblockchain.transaction.getwith a transaction whose outputs cannot reach an outpoint we know is spent brings the connection down rather than panicking the state thread. Thatunimplemented!is onmain; it and the txid check below are the two fixes here to code this PR does not introduce.ConfirmationStagegainsIdle, so exhaustive patterns over it no longer compile.Donemeans the job resolved everything and still owes the caller an update, which it keeps offering until taken;Idlemeans nothing is owed — the update was taken, or the job was abandoned. One stage for both handed the same update over twice, and handed one over for a job that resolved nothing.ConfirmationJob::is_doneis now true only forDone, andConfirmationJob::set_idleis added.ConfirmationJob::resetis removed — nothing called it.ConfirmationJob's setters areset_tip,set_statusesandresolve_blocks— the first two no-op on an unchanged value, andset_statusesis called with the same set on every poll, soset_says what they do.SpkHistoriesisSubscriptions, keyed by status rather than script hash.remove/insert/get/statusareremove_spk/insert_spk/spk_history/spk_status. Its height index (spk_hashes_at_heights,prune,HEIGHT_INDEX_HORIZON) is gone — nothing read it once repair stopped being driven through script jobs.Cache::{txs, anchors, spk_txids}are nowCache::tx_cache::{…}, and a serializedCacheno longer carries them.Cache::failed_anchorsis removed, and nothing replaces it. A proof the server refuses or contradicts cancels the job rather than being recorded against our block — an error proves nothing about that block either way. A script notification now builds aConfirmationJobas well as a tip does, so a disagreement below the reorg window recovers without waiting on a block. The trade is deliberate: one pair the server will not prove now withholds the whole update rather than publishing the rest without that anchor.JobRequest::GetHeaderis removed; nothing enqueues a single-header request any more.ReqCoord::popreturnsOption<PoppedRequest>;ReqCoord::clearis removed. Additive:bump_chain_generation,forget_job.debug_assert!. Both are reachable only if our own bookkeeping broke, so a debug build now panics where it used to log and continue. Release behaviour is unchanged.blockchain.transaction.getresponse whose transaction does not hash to the requested txid now errors the connection instead of being cached under the wrong id. Behavioural, not a signature change, but a server that was quietly getting away with it no longer will. Predates this branch.New dependency
serde, already in the tree viaserde_json.Testing
cargo test— 24 deterministic (tests/state.rs), 5 live (tests/env.rs), 8 unit. Each fix was reverted in turn to confirm a test catches it.Two exceptions, stated rather than glossed:
ConfirmationStage::Donerestoring itself afterpoll'smem::take, and the consistency guard parking inIdlerather thanDone, are both reasoned rather than demonstrated. Reverting either produces redundant work rather than a wrong update, so no test catches them.Every one of the seven commits passes
cargo fmt --check,cargo check,cargo clippy --all-targetsand the full suite on its own.reorg_to_same_height_block_refetches_anchor_liveconfirms a tracked transaction, then re-mines it into a different block at the same height. Fails onmain, passes here.a_reorg_reanchors_every_script_not_just_the_last_to_notifyis the one the single-script test harness was structurally blind to: the server now serves any number of scripts, matching a transaction to a script by the outputs it pays.a_history_that_cannot_match_the_job_is_not_re_askedis capped at 50 requests so the unbounded re-ask fails the test rather than hanging it.a_transaction_that_is_not_the_one_asked_for_is_rejectedanswers ablockchain.transaction.getwith a different, perfectly valid transaction and requires the connection to come down.a_merkle_error_below_the_reorg_window_is_recovered_by_a_script_notificationrefuses a proof at a height nothing will rewrite, so the tip never moves and the script notification is the whole of the recovery. It fails if a refused proof drops the job without something able to rebuild one.confirmation_job_runs_ahead_but_the_update_waits_for_the_scriptspins both halves of the gate: it answers a script's history but withholds its transaction, asserts the job reaches proof fetching anyway, then answers the proof and asserts nothing is published — and only once the transaction lands does one update arrive carrying both it and its anchor. Re-announcing the same tip afterwards must produce nothing.Known, not fixed here
Update; the trade this PR makes deliberately. Scripts stuck downloading transactions no longer block the confirmation job itself.(txid, height)the server will not prove withholds the whole update, not just that anchor. The job is cancelled and rebuilt from the next notification rather than carrying a record of the refusal, so a server that refuses one proof indefinitely delivers nothing at all. Judged the server's fault to fix, and preferable to reporting a mined transaction as unconfirmed.TxCachehas no seeding constructor, so a reconnect refetches everything.