fix(electrum): verify anchor heights against current chain in chain_update - #2318
only1dreamgene wants to merge 1 commit into
Conversation
f739d2c to
acb6639
Compare
…pdate `chain_update` inserted a checkpoint using an anchor's own block hash whenever its height fell outside `latest_blocks` (the ~8-block reorg-risk window plus point-of-agreement heights). That hash was fetched separately in `batch_fetch_anchors`, at a different point in the sync, so a merkle proof against it only shows the tx is included in *that* header -- not that the header is still on the chain the current tip represents. Around a reorg, this let an inconsistent or stale header be inserted straight into the checkpoint chain and treated as confirmed. Tighten `chain_update` to only insert checkpoints backed by `latest_blocks`, and add `fill_missing_anchor_heights`, which re-fetches fresh (uncached) headers for anchor heights outside that window right before `chain_update` runs. This closes the staleness gap without regressing historical confirmations on a full scan: every checkpoint reflects the server's current view at commit time, so a reorged-out anchor now resolves to the current (differing) hash instead of being silently trusted. Fixes bitcoindevkit#2312
acb6639 to
5a2772e
Compare
only1dreamgene
changed the title
|
Unfortunately, I think the premise of the issue is incorrect.
Yes, that is the point. If the merkle proof produces an anchor that doesn't point at a block in the update chain (assuming there are no other anchors for that tx), then the wallet won't see that transaction as "confirmed". If a reorg happens mid-sync (assuming that the reorg depth is not deeper than
|
only1dreamgene
left a comment
only1dreamgene
left a comment
There was a problem hiding this comment.
You're right, and thanks for pushing back on this.
For an anchor height with no prior local checkpoint (the normal case for historical transactions on a full scan), the merkle-proof-verified anchor is the only source of truth available -- there's no independent chain to check it against. Re-querying the same server a second time doesn't protect against a dishonest server (it would just repeat the same answer), and against an honest server hit by a genuine reorg, it only narrows a race that's already inside the window CHAIN_SUFFIX_LENGTH is designed to tolerate. Left inline notes on the specific pieces below.
Since this reasoning applies equally to the original report: should #2312 be closed as not-a-bug, or is a docs-only comment on chain_update (explaining why the fallback is safe, no behavior change) still worth adding so this doesn't get flagged again? Happy to close this PR either way -- let me know which you'd prefer.
| tx_update.anchors.iter().cloned(), | ||
| )?), | ||
| Some((chain_tip, mut latest_blocks)) => { | ||
| fill_missing_anchor_heights( |
There was a problem hiding this comment.
Minor correction to the original issue's framing while I'm here: fetch_tip_and_latest_blocks runs before batch_fetch_anchors populates tx_update.anchors later in this function, so latest_blocks is actually the staler of the two reads by the time chain_update runs, not the anchor's hash as the issue assumed.
| None => anchor.block_id.hash, | ||
| }; | ||
| tip = tip.insert(height, hash); | ||
| if let Some(&hash) = latest_blocks.get(&height) { |
There was a problem hiding this comment.
This check doesn't need to be this strict. For anchor heights with no prior local checkpoint, the merkle-proof-verified anchor hash is the only available source of truth -- there's nothing independent to validate it against, so requiring latest_blocks coverage here doesn't add real protection, just complexity.
| return Ok(()); | ||
| } | ||
|
|
||
| let headers = client.batch_block_header(missing_heights.clone())?; |
There was a problem hiding this comment.
This re-queries the same server a second time. Against a dishonest server it adds nothing (it would just repeat the same fabricated answer); against an honest server hit by a genuine reorg, the race this narrows is already inside the window CHAIN_SUFFIX_LENGTH is meant to tolerate. Given the cost -- a batch_block_header call that can cover every historical anchor height on a full scan -- I don't think this function is worth keeping.
|
Having read through the thread, I agree with @evanlinjin original objection: trusting the merkle-proof-verified anchor hash when there's no prior local checkpoint isn't a bug, it's the intended design. CHAIN_SUFFIX_LENGTH already bounds the reorg risk for that case, and there's no independent chain data available at that point to validate the anchor against anyway. Given that, I don't think this should merge as-is: The core premise (that chain_update unsafely trusts anchor hashes) doesn't hold up, and you've already conceded this yourself in the inline comments. So: changes requested, and honestly I'd lean toward closing this PR rather than revising it, since the fix doesn't have a sound justification left once the premise is removed. On your open question : I'd vote for a docs-only comment on chain_update explaining why the anchor-hash fallback is safe (no local checkpoint → nothing to compare against → merkle proof is the trust anchor), rather than closing #2312 with no trace. That gives future readers (and future issue-filers hitting the same "looks like a bug" intuition) something to point to, without touching behavior. Happy to re-review if you want to pivot this into that docs-only version. |
3 participants
Summary
Fixes #2312.
chain_update(crates/electrum/src/bdk_electrum_client.rs) inserted a checkpoint using an anchor's own block hash whenever its height fell outsidelatest_blocks(the lastCHAIN_SUFFIX_LENGTHblocks plus point-of-agreement heights). That hash comes from a header fetched separately inbatch_fetch_anchors, at a different point in the sync — a merkle proof against it only proves the tx is included in that header, not that the header is still on the chain the current tip represents. Around a reorg, a stale or inconsistent header could be inserted straight into the checkpoint chain and treated as confirmed.chain_updatenow only inserts checkpoints backed bylatest_blocks— it no longer falls back to trusting the anchor's own hash.fill_missing_anchor_heightsruns right beforechain_update(in bothsyncandfull_scan) and re-fetches fresh, uncached headers for any anchor height outside the reorg-risk window, merging them intolatest_blocks.This closes the staleness gap without regressing historical confirmations on a full scan/restore: every checkpoint now reflects the server's current view at commit time. A reorged-out anchor resolves to the current (differing) hash instead of being silently trusted, while anchors that are still valid keep their correct confirmed status.
Test plan
cargo test -p bdk_electrum --lib— all 5 tests pass, including a new integration test (fill_missing_anchor_heights_corrects_reorged_anchor) that performs a real reorg viaTestEnvand verifies the checkpoint reflects the corrected chain, not the stale anchor.cargo clippy -p bdk_electrum --lib --tests— clean.