Repository navigation
Conversation
`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.
|
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 ... okAlso reviewed the change itself against #2341 independently before seeing this PR, tracing the same path: Two things I checked that aren't in the PR description:
On the open design question in the description (silent skip vs. a surfaced error): I'd lean toward silent skip for now, same as One thing not addressed here that the original issue also asked for: the README/rustdoc still doesn't say what |
|
Thanks for testing it. Both of your checks match what I found. The anchor loop is guarded by 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. |
Description
Fixes #2341.
chain_updateinserts every entry of the server's/blocksresponse into the local checkpoint chain without filtering.CheckPoint::insertasserts 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_genesisisSomeonly forheight == 0, or forheight == 1whenprev_blockhash()is available, andbdk_esplorainserts a bareBlockHash, whose defaultprev_blockhash()isNone.Skipping loses nothing. An entry that agrees with local genesis already hits the
cp.hash() == data.to_blockhash()early-out ininsert, 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
insertalready documents about itself: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'sFilterIterderives the next height fromheader.next_block_hashand so structurally never inserts height 0;bdk_electrum'schain_updateonly 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 whattest_chain_update_wrong_network_errorcovers. The only way to reach a conflicting height-0 entry past that check is a server contradicting its own/blocksresponse.I filtered at the insertion loop rather than at the map, because
fetch_blockmust 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:
/blockspages 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_esplorahas 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 onesplora_client::Error(the crate'sErrorisBox<esplora_client::Error>), which needs an upstream release first; reusingHeaderHashNotFoundwould be misleading. Happy to go that route if you prefer.The regression tests build
latest_blocksso that it already covers the local tip height, meaningchain_updatemakes no HTTP request and needs no regtest backend, hence the unused dummy URL.Changelog notice
bdk_esplora'schain_updatepanicking when a chain source's/blocksresponse contains a height-0 entry that conflicts with the wallet's genesis block.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: