fix(bdk_electrum_streaming): Key anchors by block hash so a same-height reorg is recoverable - #18
fix(bdk_electrum_streaming): Key anchors by block hash so a same-height reorg is recoverable#18LLFourn wants to merge 1 commit into
Conversation
…ht reorg is recoverable Fixes evanlinjin#12. A reorg that moves a tx into a different block at the same height leaves the Electrum script status untouched — it is a hash over `txid:height:` pairs — so the server has no reason to notify, and no subscription can be arranged that would tell us. The protocol carries no signal; the chain dropping the block is the only one there is. The old code keyed pending anchors by `(height, txid)` and re-resolved that height through the chain on every pass. A height is a mutable key and a block hash is not, so a reorg silently retargeted a proof onto the wrong block. An anchor is now established from the header fetched for its height and nothing else — header first, then proof, checked against whatever header is held at check time — and keyed by the block's hash. That makes it a fact rather than a claim about the chain: true forever once established, and merely inert when its block leaves, because bdk scans every anchor a tx has and takes whichever one is in the chain. `failed_anchors` goes, because a non-match cannot distinguish 'not in that block' from 'that header is from a chain the server has left', so it is a retry and never a verdict. Eviction is detected by diffing the tip across a chain update rather than by asking the chain job, so `chain_job.rs` is untouched and the trigger works whatever the chain job is. The height-to-txids record survives a re-ask that finds nothing, because a tx can be reorged out and back in at the same height with the status identical either side, and a client that misses the middle is never told again. Answers are filed against the chain they were asked about. `ReqQueuer::enqueue` merges a re-ask into an identical request already in flight, so a burst of reorgs is answered once — from whichever chain the server held when it read the first request — and the anchor settles there while every later re-ask is absorbed. Anchoring the first block of a burst was never wrong; settling there is, because that anchor is the only one no longer in the chain. An eviction counter stamps outstanding requests, and an answer older than the counter is dropped rather than stored, so the next ask reaches the wire. The counter moves on evictions rather than on chain updates, so an ordinary new block costs nothing. Known limit, now stated in the crate docs rather than discovered: anchoring puts a checkpoint at every confirmation height, those checkpoints are single unverified server answers, and nothing revisits them — a reorg deeper than the chain pass's window is never noticed, and anchoring cannot compensate for that. Both stale-answer guards are pinned rather than merely present, after LLFourn's review of evanlinjin#13 flagged the same class there: an error answer predating a reorg must not be recorded as a refusal, and a header predating one must not be spliced into the chain. The second needs a sparse chain forked below the anchor's height and deeper than the chain pass's window to reach at all, which is why deleting its guard passed every other test. Two caches are bounded. `anchored_at` is read only by an eviction, and an eviction cannot reach below the window the chain pass rewrites — the gap fill never conflicts, because it only writes where the chain holds nothing — so everything below that was ballast. `headers_at` was worse: a tip notification records a header for every announced block, so it gained an entry per block for the life of the process. Both prune to `EVICTION_HORIZON`, leaving alone any slot an outstanding request owns. Breaking, so 0.6.0: `Cache` swaps `failed_anchors`/`headers` for `headers_at`, `proofs`, `anchored_at` and `eviction_epoch`; `JobId` gains `Anchor`; and `SpkJob::advance` takes `&mut Cache` and no longer takes a `CheckPoint`. `Cache::default()`, `State::new` and `Update` are unaffected. Additive: `Observed`, `AnchorStep`, `resolve_anchor`, `EVICTION_HORIZON`. Claude-Session: https://claude.ai/code/session_017kyDF7XatJ5fQE14v3ydeq
|
Two reproducible liveness bugs on this branch. In both a transaction stays permanently unconfirmed after a one-block reorg that the chain pass itself resolves correctly — so neither is covered by the "Known limitation" section, which is about forks deeper than the chain pass window. Both repros are 1–2 blocks deep and the chain ends up holding the right block at the reorged height; only the anchor never catches up. Both use the existing Bug 1 — an anchor whose proof is in flight when a reorg lands is abandoned
The job then re-asks for the proof, gets a correct one for the new block, checks it against the old cached header, and takes the mismatch branch at Observed: Bug 2 — an anchor established below the tip is never re-asked
match before.get(height) {
Some(was) => now.get(height).map(|cp| cp.hash()) != Some(was.hash()),
None => false,
}
In the repro the chain ends up holding Is this architectural?I think so, yes — and worth saying plainly, because I tried the obvious local patch and it is not sufficient. Keying anchors by block hash gives correctness: an anchor can never become false, and a stale one is inert. But it supplies no liveness, and liveness is what issue #12 is actually about. Liveness here is carried by a separate mechanism bolted alongside the anchors — The re-ask trigger's domain is strictly narrower than the anchor's domain. An anchor may be established at any height, at any time, without reading the chain. A re-ask fires only when (a) the anchor already resolved successfully, and (b) the chain held that exact height before the update and holds a different block there now. Anything outside that intersection is unrecoverable. Bug 1 is the (a) gap; bug 2 is the (b) gap. Both funnel into The root of it is that the trigger is edge-triggered: recovery depends on observing a transition. Miss the edge — the answer was in flight, or the chain had not reached that height yet — and the state is never revisited. I tried the minimal fix for bug 1 — move the What removes the class is making the trigger level-triggered — a function of current state rather than of observed transitions. On every chain update, for each Concretely, the trigger currently asks "did a block leave the chain?" when the question that matters is "does every transaction I believe confirmed have an anchor in the chain I now hold?". The second question is answerable from state you already keep, and answering it is what makes the fact-based anchor model complete rather than only sound. ReprosAppend to fn on_top_of(prev: block::Header, time: u32) -> block::Header {
block::Header {
merkle_root: TxMerkleNode::all_zeros(),
prev_blockhash: prev.block_hash(),
time,
..prev
}
}
/// BUG 1: an anchor whose proof is still in flight when a reorg lands is never asked for again.
#[test]
fn repro_1_in_flight_anchor_is_orphaned_by_a_reorg() -> anyhow::Result<()> {
let mut f = AnchorFixture::new(|_, _| {})?;
f.sync();
// A second tx, under a script the client does not track, so it changes what block 2 contains
// without changing the tracked script's history. The replacement block therefore needs a
// different merkle proof for our tx.
let other = tx_paying(ScriptBuf::from_hex("0014000000000000000000000000000000000000dead")?, 7);
let other_txid = other.compute_txid();
let mut both = vec![f.txid, other_txid];
both.sort();
let header_2c = block::Header {
merkle_root: merkle_root_of(&both),
time: 260,
..f.header_2
};
let header_3c = on_top_of(header_2c, 360);
let header_4c = on_top_of(header_3c, 460);
// The chain reaches height 3 via the bulk header fetch, so height 2 is a height the chain
// holds and nothing has cached a `headers_at` entry for it.
f.server.headers.push(f.header_2);
f.server.headers.push(f.header_3);
f.notify_tip(f.header_3, 3)?;
f.drain()?;
// The tx confirms at height 2. Stop once its header has landed, leaving the proof in flight.
f.notify_confirmed_at(2)?;
loop {
match f.answer_next()? {
Some(m) if m == "blockchain.block.header" => break,
Some(_) => continue,
None => panic!("header never asked for"),
}
}
// The server reads the proof request now, against the chain it has now.
let held = f.capture_in_flight();
assert!(!held.is_empty(), "the proof must be in flight");
assert!(
f.state.cache().anchored_at.is_empty(),
"nothing is recorded at the height while the anchor is unresolved",
);
// Height 2 is replaced by a block that also holds the tx, and the chain grows.
f.server.wallet.push((ElectrumScriptHash::new(&other.output[0].script_pubkey), vec![(other, 2)]));
f.server.headers[2] = header_2c;
f.server.headers[3] = header_3c;
f.server.headers.push(header_4c);
f.notify_tip(header_4c, 4)?;
f.drain()?;
// The held answer, describing the chain the server has since left.
let mut updates = f.deliver(held)?;
let (asked, more) = f.drain()?;
updates.extend(more);
assert!(
updates
.iter()
.any(|u| u.tx_update.anchors.contains(&(f.anchor_in(header_2c, 2), f.txid))),
"the replacement block must be anchored, got: {asked:?}; anchors held: {:?}",
f.state.cache().anchors,
);
Ok(())
}
/// BUG 2: an anchor established while the chain was behind is never asked for again, even
/// though the chain later holds the replacement block at that very height.
#[test]
fn repro_2_anchor_made_below_the_tip_is_never_re_asked() -> anyhow::Result<()> {
let mut f = AnchorFixture::new(|_, _| {})?;
f.sync();
let header_3b = on_top_of(f.header_2b, 350);
// The tip stays at 1; the anchor resolves from the header fetched for height 2 alone.
f.server.headers.push(f.header_2);
f.notify_confirmed_at(2)?;
f.drain()?;
assert!(
f.state
.cache()
.anchors
.contains_key(&(f.txid, f.header_2.block_hash())),
"the anchor must resolve before the reorg",
);
assert!(
f.state.cache().anchored_at.get(&2).is_some(),
"and is recorded at its height",
);
// Height 2 is replaced and the chain grows over it.
f.server.headers[2] = f.header_2b;
f.server.headers.push(header_3b);
f.notify_tip(header_3b, 3)?;
let (asked, updates) = f.drain()?;
assert!(
updates
.iter()
.any(|u| u.tx_update.anchors.contains(&(f.anchor_in(f.header_2b, 2), f.txid))),
"the replacement block must be anchored, got: {asked:?}",
);
Ok(())
} |
2 participants
Fixes #12. An alternative to #13, which fixes the same bug — see the comparison
below. Based on
main(2250918). One commit.The bug, and the shape of the fix
A reorg that moves a transaction into a different block of the same height leaves its Electrum
script status untouched — the status is a hash over
txid:height:pairs — so the server has noreason to notify. No subscription can be arranged that would tell us. The chain dropping the block
is the only signal the protocol carries.
The current code keys pending anchors by
(height, txid)(spk_job.rs:22) and re-resolves thatheight through the chain on every pass (
spk_job.rs:270-299). A height is a mutable key; a blockhash is not. That single choice is what lets a reorg silently retarget a proof onto the wrong
block.
So the fix is to stop treating an anchor as a claim about the chain and start treating it as what it
is: a fact about a block. "This txid is in this block", established by checking a merkle proof
against the header fetched for that height, and keyed by that block's hash. Facts do not become
false. A reorg only makes one inert —
LocalChainconsults its own tip,canonical_iterscansevery anchor a transaction has and takes whichever is in the chain, so a stale anchor is ignored
rather than harmful. Anchors and chain updates therefore need neither agree nor travel together.
Three things fall out of that rather than being arranged:
nothing else, so it works whatever the tip is doing.
an_anchor_resolves_without_the_chain_catching_uppins this with a tip that never moves at all.
failed_anchorsgoes. A non-match cannot distinguish "not in that block" from "that header isfrom a chain the server has left", so it is a retry and never a durable verdict. The type that
could record a permanent disproof no longer exists.
chain_job.rsis untouched. Eviction is detected by diffing the tip across a chain update —CheckPointisArc-backed, so cloning it first is free — rather than by asking the chain jobwhat it forked at. The trigger asks the question it actually means and works whatever the chain
job is.
The one thing that must be arranged is the re-ask, because nothing else will prompt it. A
(height, txid)record survives a re-ask that comes back empty, because a transaction can bereorged out and back in at the same height with the status identical either side — a client that
misses the middle is otherwise never told again.
Answers are filed against the chain they were asked about
ReqQueuer::enqueuemerges a request into an identical one already in flight, andreq_to_jobholds that entry until the response lands. So a re-ask raised during a burst of same-height reorgs
puts nothing on the wire, and the one answer that does arrive describes whichever chain the server
held when it read the first request.
Anchoring that first block is not wrong — the transaction really was in it, and it is inert once its
block is gone. The failure is settling there: no anchor for the last block is ever fetched and
the transaction reads unconfirmed. The property is liveness, not correctness:
Observed::Awaiting(epoch)stamps an outstanding request with the eviction counter it went outunder.
recordis the single choke point every answer passes through — success and error alike —and an answer stamped older than the counter is dropped rather than stored, so the next pass's ask
reaches the wire because
pophas just released the request it would have merged into.Two things it deliberately is not. A
stale: HashSet<height>that discards the first answer afterany eviction burns a round trip whenever nothing was in flight. A generation bumped on every tip
change re-fetches on every ordinary block — the counter moves on evictions only, and
a_new_block_does_not_invalidate_an_answer_in_flightis the only test standing between it and thatanti-pattern.
Relationship to #13
#13 diagnoses #12 correctly and its chain-driven trigger is the right instinct. Several conclusions
here were reached independently and agree exactly: fetch the header before the proof; a response
must advance whatever asked for it; a
get_merkleerror is an answer, not a reason to drop theconnection; a stale answer must be discarded; and
CHAIN_SUFFIX_LENGTHis the real reorg horizoneither way. Where they differ:
(height, txid), re-resolved against the local chain each passtip.get(height)) and cannot resolve a height the chain lacksSpkJobby raising the script-hash notification locally, fromspk_statuses+spk_hashes_by_heightreanchorset of(height, txid)pairs, advanced directlyfailed_anchorskept, narrowed to a verified mismatchReqCoord, keyed by request idreq.rs,spk_job.rs,state.rsspk_job.rs,state.rsThe trade is real in both directions and worth being straight about. #13's stamp placement is the
better one: a generation recorded against the request id is stamped exactly when the request hits
the wire, which makes the merged-re-ask case correct by construction. Here the stamp lives on the
cache slot and only
entry().or_insert()keeps a merged re-ask from refreshing it — the sameproperty, held by a smaller margin. Both designs handle the burst.
What this buys instead is that the anchor stops depending on the chain at all, which is what
removes the class rather than the instance: there is no height to re-resolve, no negative cache to
get wrong, no job to replay, and no reason for
chain_job.rsorreq.rsto change.What #13's review flagged that also applied here
Reviewing the two together turned up three items from #13's review that landed on this branch too.
Two were guards that existed but that nothing exercised, and an unexercised guard is one somebody
deletes:
state.rs:256). Covered here byconstruction, since
recordhandles both arms — but not pinned. It is the branch where beingwrong costs more: a recorded refusal is consumed on read and abandons the re-ask, so the anchor
stays on a block that left the chain and only a further reorg at that height would try again.
Now
an_error_that_predates_a_reorg_is_not_taken_as_a_refusal.state.rs:289, where deleting itpassed all 5 tests). Deleting it here passed all 13. Reaching it needs a sparse chain forked below
the anchor's height and deeper than the chain pass's window, which no fixture had. Now
a_header_fetched_before_a_reorg_is_not_spliced_into_the_chain.state.rs:670).anchored_atis the direct analogue ofspk_hashes_by_heightand the same argument bounds it: an eviction cannot reach below the windowthe chain pass rewrites, and the gap fill never conflicts because it only writes where the chain
holds nothing.
headers_atwas worse — a tip notification records a header for every announcedblock, so it gained an entry per block for the life of the process. Both now prune to
EVICTION_HORIZON, leaving alone any slot an outstanding request owns.One flag does not carry over:
ReqCoord::clearis still dead code, but this branch does not touchreq.rs, so the "already extending it" argument for fixing it here does not apply.Testing
cargo test -p bdk_electrum_streaming— 15 deterministic (tests/state.rs), 5 live(
tests/env.rs, ~39s againstbdk_testenv's bitcoind + electrs), 4 unit.cargo fmt --all --checkclean;
cargo clippy --all-targetsat the same 4 warnings asmain.Two live tests cover this end to end, and were written independently of #13's:
reorg_to_same_height_block_refetches_anchor_live— confirms a tracked tx, then re-mines it into adifferent block at the same height. Asserts the transaction comes back confirmed via the
replacement block, and that the anchor to the abandoned block is still held and simply ignored.
reorg_unconfirming_a_tx_keeps_the_connection_alive— invalidates the confirming block and minestwo empty blocks so it cannot be re-mined, then requires the connection to keep serving.
Three cases cannot be reached live at any effort, because they need control over the order responses
come back in: the mid-fetch reorg, the proof held before its header, and the reorg burst.
Mutation table
Each guard removed in turn, against
cargo test --test state:issued < epocharm fromrecorda_burst_of_reorgs…— and nothing elsea_new_block_does_not_invalidate_an_answer_in_flight— and nothing elsea_header_fetched_before_a_reorg_is_not_spliced_into_the_chainan_error_that_predates_a_reorg_is_not_taken_as_a_refusala_reorg_landing_mid_fetch_anchors_the_block_the_proof_provesThe last one is worth flagging as a difference from #13, whose table records the same mutation as
failing nothing. Here it is load-bearing rather than an optimisation: the announced header is the
observation a proof in flight during a reorg gets checked against.
The burst test runs at three speeds, where speed is how many reorgs the server folds into one
announcement —
[1; 11],[4, 1, 6],[11]. The folded ones are invisible to the client, so a fastburst is not a slow one sped up, it is one the client is told less about, and a client that saw a
single chain change could have been right by accident. Each schedule was checked to fail on its own
with the guard removed.
Breaking changes (→
0.6.0)From a diff of the public surface against
2250918. No version bump here — this repo cuts releasesin their own PRs.
Cacheswapsfailed_anchorsandheadersforheaders_at,proofs,anchored_atandeviction_epoch. Struct-literal construction and exhaustive destructuring break;Cache::default()is unaffected, which is how every consumer builds it.JobIdgainsAnchor— exhaustive patterns break.SpkJob::advancetakes&mut Cacheand no longer takes a&CheckPoint.Additive:
Observed,AnchorStep,resolve_anchor,EVICTION_HORIZON.State::new,Update,ReqCoordandChainJobare unchanged —git diffon their declarations against2250918isempty.
Known limitation, stated rather than discovered
Anchoring necessarily plants a checkpoint at every confirmation height it establishes:
LocalChain::is_block_in_chainanswers unknown — not false — for a height the chain does not hold,and
canonical_itermarks a transaction canonical only onSome(true), so without those checkpointsconfirmed transactions read as unconfirmed.
Each of those checkpoints is a single unverified server answer, and nothing revisits it. The
chain pass fetches a fixed window near the tip, so a gap filled below that window is never checked
again, and a fork deeper than the window is never noticed at all — leaving an anchor pointing at a
block no longer in the best chain while the transaction still reads confirmed. Anchoring cannot
compensate for that, and this PR does not claim to: how deep a reorg can be and still be seen is
decided by the chain pass alone. It is documented on the crate root rather than left to be
rediscovered.
https://claude.ai/code/session_017kyDF7XatJ5fQE14v3ydeq