Skip to content

fix(electrum): fix stale anchor hash on reorg #2011

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LagginTimes
Copy link
Contributor

Description

Fixes an issue in batch_fetch_anchors() in bdk_electrum where, if a stale block header was detected and a fresh header was fetched, the confirmation anchor inserted into anchor_cache still used the hash from the original stale header instead of the new one.

Changelog notice

  • batch_fetch_anchors() no longer uses a potentially stale hash as the anchor.

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

@LagginTimes LagginTimes self-assigned this Aug 12, 2025
@LagginTimes LagginTimes marked this pull request as draft August 12, 2025 12:13
@notmandatory notmandatory moved this to In Progress in BDK Chain Aug 12, 2025
@notmandatory notmandatory added the bug Something isn't working label Aug 12, 2025
@LagginTimes LagginTimes marked this pull request as ready for review August 15, 2025 07:54
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

utACK c8a3326

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

In general, this looks like a solid simplification and likely fixes the problem in the PR description. Can we add a test that triggers a reorg after fetch_tip_and_latest_blocks to confirm?

While reviewing, I noticed the chain-update logic is a bit convoluted, which makes this (and other bdk_electrum changes) tricky to follow:

  • block_header_cache is keyed by height (not block hash), so it’s unclear how we’re ensuring the right header.
  • The !valid block section updates block_header_cache on reorgs after fetch_tip_and_latest_blocks, meaning headers from different chains can co-exist. Anchors from batch_fetch_anchors may also not match the same chain.
  • In chain_update, we use blockhashes from anchors to craft the CheckPoint update. Because inconsistencies are possible, we also pass in lastest_blocks, assuming reorgs aren’t deeper than CHAIN_SUFFIX_LENGTH.

I suggest we merge this PR ASAP (since it’s an important fix).

Then, in a follow-up PR, we could simplify the codebase:

  1. Key block_header_cache by blockhash instead of height. Fetch by height through the CheckPoint chain.
  2. Remove the !valid block section, since headers will always match the chain. Reorgs will naturally invalidate anchors and re-fetch them.
  3. Let fetch_tip_and_latest_blocks() just fetch blocks. Move chain-update logic into chain_update(), updated to take heights_we_are_interested_in instead of anchors. Call chain_update() after batch_fetch_anchors.

This may slightly slow down bdk_electrum (extra header lookups via CheckPoint), but IO is the real bottleneck. Once the skip-list is in, CheckPoint lookups will be fast.


On second though... maybe we should only do bug fixes and focus effort in bdk_electrum_streaming?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants