Skip to content

fix(esplora): don't insert genesis from the server's latest blocks - #2363

Open
luhrhenz wants to merge 1 commit into
bitcoindevkit:masterfrom
luhrhenz:fix/esplora-filter-latest-blocks
Open

luhrhenz wants to merge 1 commit into
bitcoindevkit:masterfrom
luhrhenz:fix/esplora-filter-latest-blocks

Conversation

@luhrhenz

@luhrhenz luhrhenz commented Oct 9, 2026 •

Copy link
Copy Markdown

Description

Fixes #2341.

chain_update inserts every entry of the server's /blocks response into the local checkpoint chain without filtering. CheckPoint::insert asserts that inserted data does not imply a different genesis, so a server returning a height-0 entry whose hash differs from the wallet's genesis panics sync. Since the update is rebuilt from the server response on every sync, the panic recurs forever and the wallet can never sync against that server again.

The fix skips height 0 when inserting, so the loop starts at height 1.

Why skipping rather than erroring

Height 0 is the only reachable trigger here. The assert fires only when traversal reaches a checkpoint at height 0, and implied_genesis is Some only for height == 0, or for height == 1 when prev_blockhash() is available, and bdk_esplora inserts a bare BlockHash, whose default prev_blockhash() is None.

Skipping loses nothing. An entry that agrees with local genesis already hits the cp.hash() == data.to_blockhash() early-out in insert, so inserting it was a provable no-op. An entry that disagrees is the panic. Neither case wants the insert.

This also lines up with what insert already documents about itself:

Although [CheckPoint] isn't structurally required to contain a genesis block, if one is present, it stays immutable and can't be replaced.

So never handing it a height-0 entry from server data is the caller honouring that contract rather than relying on the assert to enforce it.

This also matches what the other backends already do. bdk_bitcoind_rpc's FilterIter derives the next height from header.next_block_hash and so structurally never inserts height 0; bdk_electrum's chain_update only inserts heights at or below the local tip after requiring agreement with it. Neither errors on a contradictory genesis. They simply never insert one.

A server that is genuinely on a different chain is still rejected: the point-of-agreement search above returns HeaderHashNotFound, which is what test_chain_update_wrong_network_error covers. The only way to reach a conflicting height-0 entry past that check is a server contradicting its own /blocks response.

I filtered at the insertion loop rather than at the map, because fetch_block must still be able to serve height 0 so a genesis-only local chain can find a point of agreement.

Notes to the reviewers

A legitimate height-0 entry can occur: /blocks pages roughly ten blocks down from the tip, so a chain shallower than one page (a fresh regtest or signet) will include height 0. In that case the hash matches local genesis and the insert was already a no-op, so this change is behaviour-preserving there.

The skip is silent, with no log and no error. bdk_esplora has no logging dependency to hook into, and the issue's hard requirement is that sync must not panic persistently. If you'd rather surface it, the natural home is a new variant on esplora_client::Error (the crate's Error is Box<esplora_client::Error>), which needs an upstream release first; reusing HeaderHashNotFound would be misleading. Happy to go that route if you prefer.

The regression tests build latest_blocks so that it already covers the local tip height, meaning chain_update makes no HTTP request and needs no regtest backend, hence the unused dummy URL.

Changelog notice

  • Fixed bdk_esplora's chain_update panicking when a chain source's /blocks response contains a height-0 entry that conflicts with the wallet's genesis block.

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

`chain_update` inserted every entry of the server's `/blocks` response into
the local checkpoint chain. `CheckPoint::insert` keeps genesis immutable and
asserts that inserted data does not imply a different one, so a height-0 entry
whose hash disagrees with the wallet's genesis panicked the sync. The update is
rebuilt from the response on every sync, so the panic recurred and the wallet
could never sync against that server again.

Restrict the insert to height 1 and above, as the other chain sources already
do: `FilterIter` only ever inserts the block following an existing checkpoint,
and `bdk_electrum` only inserts at or below the tip it has agreed with. A
height-0 entry matching our genesis is a no-op, so nothing is lost, and one
that does not match is dropped instead of aborting the sync. A server on a
different chain is still rejected by the point-of-agreement search.

`latest_blocks` itself is left untouched so `fetch_block` can keep serving
height 0 while looking for that point of agreement.
@luhrhenz
luhrhenz requested a review from a team as a code owner October 9, 2026 16:44
@Olorunshogo

Copy link
Copy Markdown

Tested ACK 30073d2

Pulled the branch and ran the new tests directly:

    test blocking_ext::test::test_chain_update_ignores_genesis_in_latest_blocks ... ok
    test async_ext::test::test_chain_update_ignores_genesis_in_latest_blocks ... ok

Also reviewed the change itself against #2341 independently before seeing this PR, tracing the same path: chain_update's final loop inserting every latest_blocks entry unconditionally, CheckPoint::insert's documented panic on a conflicting height-0 entry, and the comparison to bdk_electrum/bdk_bitcoind_rpc never reaching height 0 from server data in the first place. The fix (latest_blocks.range(1..)) matches what I'd have proposed.

Two things I checked that aren't in the PR description:

  • The anchor-insertion loop a few lines above (if tip.get(height).is_none() { ... tip.insert(height, hash) }) doesn't need the same guard: genesis is always already present in tip by the time that loop runs, so tip.get(0) is never None there and it can't reach the panic site. This PR isn't missing a second fix location.
  • The "legitimate height-0 entry" case (short regtest/signet chain) is correctly a no-op either way, since it would've hit insert's early-out before this change too.

On the open design question in the description (silent skip vs. a surfaced error): I'd lean toward silent skip for now, same as bdk_electrum/bdk_bitcoind_rpc: this is defending against a provably spec-violating server, and a new esplora_client::Error variant blocked on an upstream release feels like too much ceremony for a case that should never occur. Could always add the error variant later without breaking anything.

One thing not addressed here that the original issue also asked for: the README/rustdoc still doesn't say what bdk_esplora assumes about chain-source response correctness. Not a blocker for this fix, but might be worth a short follow-up note so the trust assumption is documented somewhere.

@luhrhenz

luhrhenz commented Oct 10, 2026 •

Copy link
Copy Markdown
Author

@Olorunshogo

Thanks for testing it.

Both of your checks match what I found. The anchor loop is guarded by tip.get(height).is_none(), and since tip starts from the point of agreement inside the local chain, genesis is always already an ancestor, so that branch can't reach insert at height 0. Worth having in the thread, since "did you miss a second call site" is the obvious first question.

Good point that the error variant can be added later without breaking anything. That makes the silent skip easier to justify as the interim state rather than a decision that locks anything in.

On the docs gap: agreed, and I left it out deliberately to keep this PR to the panic. #2340 already tracks the undocumented trust model for both esplora and electrum, so the chain-source assumptions probably belong there rather than bolted onto this fix.

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.

Esplora: server-supplied height-0 entry in latest_blocks panics sync in CheckPoint::insert

3 participants