fix(electrum): keep txs whose merkle proof fails visible as unconfirmed - #2336
Open
Brijesh-Thakkar wants to merge 1 commit into
Open
Brijesh-Thakkar wants to merge 1 commit into
Brijesh-Thakkar wants to merge 1 commit into
Conversation
In `sync` and `full_scan`, a tx the server lists with height > 0 is only queued for `batch_fetch_anchors` and gets no `seen_at`. If its merkle proof does not validate (even after the one header retry), no anchor is produced, so the tx ends up in `tx_update.txs` with neither an anchor nor a `seen_at`. `TxGraph::apply_update` stores it but canonicalization ignores it, so the wallet shows nothing for a tx the server reports. A failed proof is deliberately not an error (a stale header can cause one), so instead treat such a tx as unconfirmed until the proof validates: after fetching anchors, insert `(txid, start_time)` into `seen_ats` for every queued txid that got no anchor, the same way txs with height <= 0 are handled. Coinbase txs are skipped because `bdk_chain` asserts that they never have a `last_seen`. Txs whose proof validates are unaffected. Add a regression test that runs a stub Electrum server in-process and needs neither bitcoind nor electrs. Like `test_electrum`, it requires the `use-rustls` feature (`electrum_client::Client` needs a TLS feature), so it is skipped under `--no-default-features`. Fixes bitcoindevkit#2304 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Brijesh-Thakkar
requested review from
evanlinjin and
oleonardolima
as code owners
October 1, 2026 09:13
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.
Description
Fixes #2304.
bdk_electrumqueues txs the server lists with height > 0 for merkle-proof validation and records noseen_atfor them. If the proof fails (also after the one retry inbatch_fetch_anchors), no anchor is produced and the tx is left intx_update.txswith neither an anchor nor aseen_at. It is then stored but never canonical, so a tx the server reports as confirmed is invisible in the wallet.The fix, in
syncandfull_scan: after anchors are fetched, every queued txid with no anchor gets(txid, start_time)inseen_ats, i.e. it is treated as unconfirmed until the proof validates. Txs that are anchored are untouched.Notes to the reviewers
validate_merkle_for_anchorthrow an error for missing or invalid proof? #1508 decided a failed proof must not be a returned error (a stale header can cause one), so the sync keeps succeeding. Alternatives considered: always adding aseen_atinpopulate_with_spksfor height > 0 (puts a mempool timestamp on every confirmed tx and breaks the exactseen_atsassertion intest_electrum.rs), and addingstart_timetobatch_fetch_anchors(signature change, wider diff). Fixing at the two call sites changes nothing on the success path and covers the spk, outpoint and txid paths. The ~15-line block is duplicated at the two call sites because a helper would need four parameters and would not make the diff smaller.seen_atif a later proof fails. This is harmless to canonicalization: anchors are never removed, and anchored txs are processed before seen txs, so a confirmed tx can't become unconfirmed.TxIn::default(), which has a null previous outpoint and is therefore a coinbase input. The test here uses a non-null previous outpoint.seen_at, becausebdk_chainasserts coinbase txs never have alast_seen(canonical_task.rs).[[test]]entry fortest_bad_proofwithrequired-features = ["use-rustls"], so the new test is skipped under--no-default-features, astest_electrumis (electrum_client::Clientneeds a TLS feature).populate_with_txidsalso pushes a tx with no temporal context when the server's history doesn't contain it; that is a different case and is left for a separate issue.zipwithout length checks #2305: this merges cleanly with the batch-length-check branch, and the tests pass on the merged tree. A short proof batch fails there before this code runs, so the two changes complement each other.Changelog notice
Fixed:
bdk_electrumno longer leaves a tx without an anchor orseen_atwhen its merkle proof fails to validate; it is treated as unconfirmed until it does.Checklists
All Submissions:
Bugfixes:
Testing
crates/electrum/tests/test_bad_proof.rsruns a stub Electrum server in-process (no bitcoind, no electrs, no network). It fails without the fix with "has neither an anchor nor a seen_at" and passes with it. The existingbdk_electrumunit and integration tests pass, as do fmt, clippy (--all-features --all-targets -D warnings), the feature-set builds and the MSRV 1.85.0--no-default-features --all-targetsbuild.