Skip to content

fix(electrum): keep txs whose merkle proof fails visible as unconfirmed - #2336

Open
Brijesh-Thakkar wants to merge 1 commit into
bitcoindevkit:masterfrom
Brijesh-Thakkar:fix/electrum-failed-proof-seen-at
Open

Brijesh-Thakkar wants to merge 1 commit into
bitcoindevkit:masterfrom
Brijesh-Thakkar:fix/electrum-failed-proof-seen-at

Conversation

@Brijesh-Thakkar

Copy link
Copy Markdown

Description

Fixes #2304.

bdk_electrum queues txs the server lists with height > 0 for merkle-proof validation and records no seen_at for them. If the proof fails (also after the one retry in batch_fetch_anchors), no anchor is produced and the tx is left in tx_update.txs with neither an anchor nor a seen_at. It is then stored but never canonical, so a tx the server reports as confirmed is invisible in the wallet.

The fix, in sync and full_scan: after anchors are fetched, every queued txid with no anchor gets (txid, start_time) in seen_ats, i.e. it is treated as unconfirmed until the proof validates. Txs that are anchored are untouched.

Notes to the reviewers

  • Why this option: electrum: Should validate_merkle_for_anchor throw 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 a seen_at in populate_with_spks for height > 0 (puts a mempool timestamp on every confirmed tx and breaks the exact seen_ats assertion in test_electrum.rs), and adding start_time to batch_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.
  • Already-anchored txs: a tx already anchored from an earlier sync gets a stored seen_at if 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.
  • Test input: the reproduction in the issue builds its tx with TxIn::default(), which has a null previous outpoint and is therefore a coinbase input. The test here uses a non-null previous outpoint.
  • Deliberate limit: a coinbase tx with a failing proof still gets no seen_at, because bdk_chain asserts coinbase txs never have a last_seen (canonical_task.rs).
  • Cargo.toml: adds a [[test]] entry for test_bad_proof with required-features = ["use-rustls"], so the new test is skipped under --no-default-features, as test_electrum is (electrum_client::Client needs a TLS feature).
  • Not addressed: populate_with_txids also 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.
  • Electrum batch responses are paired with zip without 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_electrum no longer leaves a tx without an anchor or seen_at when its merkle proof fails to validate; it is treated as unconfirmed until it does.

Checklists

All Submissions:

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

Testing

crates/electrum/tests/test_bad_proof.rs runs 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 existing bdk_electrum unit 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-targets build.

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>
Copilot AI balanced review requested due to automatic review settings October 1, 2026 09:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Triage

Development

Successfully merging this pull request may close these issues.

Electrum sync leaves a transaction without anchor or seen_at when its merkle proof fails

2 participants