fix(bdk_electrum_streaming)!: Give the chain and anchors one owner - #13
Merged
Merged
Conversation
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.
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.