From 0243bbe0e77614dfee5d98d1bebdf0e0f38d1a2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 1 Sep 2026 12:35:54 +0000 Subject: [PATCH 1/2] test(bdk_electrum_streaming): Cover `last_active_indices` emission Pin two properties of `last_active_indices` with state-level tests: - The emitted index must be the derivation index of the spk that has history, not the index after it (the off-by-one issue #3 reported). Already fixed on `main`; this pins it against a regression. - The keychain's last active index must be the *highest* of any spks that have history, no matter what order the server notifies their statuses in. Electrum notifies per-script-hash independently of our derivation order, so a later-derived spk can be reported before an earlier one. `state.rs`'s `on_spk_status` currently overwrites the staged index with whichever notification lands last, so this second test fails on `main`: two notifications for indices 4 then 3 leave the keychain's index at 3, not 4. Reporting a lower index than the actual highest leaves the higher spk unrevealed, so the wallet does not recognise its txouts as its own. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017FAFd2PPjDZAfgP35zeQNN --- bdk_electrum_streaming/tests/state.rs | 138 ++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/bdk_electrum_streaming/tests/state.rs b/bdk_electrum_streaming/tests/state.rs index 5bb8571..7bc28a0 100644 --- a/bdk_electrum_streaming/tests/state.rs +++ b/bdk_electrum_streaming/tests/state.rs @@ -399,6 +399,144 @@ fn descriptor_inserted_mid_connection_is_subscribed() -> anyhow::Result<()> { Ok(()) } +/// `last_active_indices` must name the derivation index of the spk that has history, not the +/// index after it. Emitting `index + 1` reveals one spk too many on every sync, permanently +/// skipping an unused address. +#[test] +fn last_active_index_is_index_of_active_spk() -> anyhow::Result<()> { + const ACTIVE_INDEX: u32 = 3; + const LOOKAHEAD: u32 = 5; + + let descriptor = Descriptor::::from_str(&format!("wpkh({XPUB}/0/*)"))?; + let spk = descriptor + .at_derivation_index(ACTIVE_INDEX)? + .script_pubkey(); + let tx = tx_paying(&spk, 50_000); + let txid = tx.compute_txid(); + let (genesis, header_1) = base_headers(); + let header_2 = block_with_tx(&header_1, txid, 200, 0); + + let mut spk_tracker = DerivedSpkTracker::new(LOOKAHEAD); + spk_tracker.insert_descriptor("external", descriptor, 0); + let mut state = BlockingState::new( + ReqCoord::default(), + Cache::default(), + spk_tracker, + CheckPoint::new(BlockId { + height: 0, + hash: genesis.block_hash(), + }), + ); + let mut queue = ReqQueue::new(); + let server = Server { + headers: vec![genesis, header_1, header_2], + txs: vec![(tx, 2)], + merkle_proof: (Vec::new(), 0), + }; + + state.start(&mut queue); + let updates = drain_requests(&mut state, &mut queue, &server); + + let emitted = updates + .iter() + .flat_map(|update| &update.last_active_indices) + .map(|(&k, &i)| (k, i)) + .collect::>(); + assert_eq!( + emitted, + vec![("external", ACTIVE_INDEX)], + "only the spk with history is active, at its own index" + ); + Ok(()) +} + +/// Two spks of the same keychain can each have history, and the server notifies their statuses +/// independently, in an order that has nothing to do with derivation index — a later-derived spk +/// can be notified first. The keychain's last active index must still end up as the *highest* of +/// the two. Reporting a lower one leaves the higher spk unrevealed, so the wallet does not +/// recognise its txouts as its own. +#[test] +fn last_active_index_is_highest_regardless_of_notification_order() -> anyhow::Result<()> { + const LOOKAHEAD: u32 = 5; + + let descriptor = Descriptor::::from_str(&format!("wpkh({XPUB}/0/*)"))?; + let spk_3 = descriptor.at_derivation_index(3)?.script_pubkey(); + let spk_4 = descriptor.at_derivation_index(4)?.script_pubkey(); + let spk_hash_3 = ElectrumScriptHash::new(&spk_3); + let spk_hash_4 = ElectrumScriptHash::new(&spk_4); + let tx_3 = tx_paying(&spk_3, 10_000); + let tx_4 = tx_paying(&spk_4, 20_000); + let txid_3 = tx_3.compute_txid(); + let txid_4 = tx_4.compute_txid(); + let (genesis, header_1) = base_headers(); + // Each in its own single-tx block, so the block's merkle root is the txid and no real proof + // construction is needed. + let header_2 = block_with_tx(&header_1, txid_3, 200, 0); + let header_3 = block_with_tx(&header_2, txid_4, 300, 0); + + let mut spk_tracker = DerivedSpkTracker::new(LOOKAHEAD); + spk_tracker.insert_descriptor("external", descriptor, 0); + let mut state = BlockingState::new( + ReqCoord::default(), + Cache::default(), + spk_tracker, + CheckPoint::new(BlockId { + height: 0, + hash: genesis.block_hash(), + }), + ); + let mut queue = ReqQueue::new(); + let mut server = Server { + headers: vec![genesis, header_1, header_2, header_3], + txs: Vec::new(), + merkle_proof: (Vec::new(), 0), + }; + + // Sync with neither spk active yet, so both are only ever reached through the notifications + // sent below, not through the initial subscribe responses. + state.start(&mut queue); + drain_requests(&mut state, &mut queue, &server); + + server.txs = vec![(tx_3, 2), (tx_4, 3)]; + let status_3 = + ElectrumScriptStatus::from_history(&server.history(&json!(spk_hash_3.to_string()))) + .expect("history is not empty"); + let status_4 = + ElectrumScriptStatus::from_history(&server.history(&json!(spk_hash_4.to_string()))) + .expect("history is not empty"); + + // The higher derivation index is notified first. + state.poll( + &mut queue, + raw_msg(json!({ + "jsonrpc": "2.0", + "method": "blockchain.scripthash.subscribe", + "params": [spk_hash_4.to_string(), status_4.to_string()], + })), + )?; + state.poll( + &mut queue, + raw_msg(json!({ + "jsonrpc": "2.0", + "method": "blockchain.scripthash.subscribe", + "params": [spk_hash_3.to_string(), status_3.to_string()], + })), + )?; + + let updates = drain_requests(&mut state, &mut queue, &server); + let emitted = updates + .iter() + .flat_map(|update| &update.last_active_indices) + .map(|(&k, &i)| (k, i)) + .collect::>(); + assert_eq!( + emitted, + vec![("external", 4)], + "the highest active index must survive being notified before the lower one" + ); + Ok(()) +} + /// A reorg can move a transaction into a different block at the *same* height. An Electrum /// script status is a hash over txid-height pairs, so it does not change and the server has no /// reason to send a script hash notification. The anchor we already delivered now points at a From 40e0695d028f4c412473c152a188bf73567de25d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 1 Sep 2026 12:36:14 +0000 Subject: [PATCH 2/2] fix(bdk_electrum_streaming): Keep the highest last active index per notification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `on_spk_status` stages a keychain's last active index with a plain `BTreeMap::insert` whenever a script hash notification (or subscribe response) names history. Electrum notifies each subscribed script hash independently, in an order unrelated to derivation index, so a spk derived later can be notified before one derived earlier. The last notification processed wins, so the staged index can end up lower than the highest spk that actually has history. That index is what `reveal_to_target_multi` reveals to, so the higher spk stays unrevealed and the wallet does not recognise its txouts as its own. Merge on the maximum instead. `poll_spk_jobs` had its own write to the same field, derived from mere job completion rather than from a notification naming history. That made it fire for every finished spk job, active or not — including every empty look-ahead spk subscribed alongside the active ones, which is the more common case. `on_spk_status` already stages the index for every spk that actually has history, before its job even runs, so this second write was both redundant for active spks and actively wrong for inactive ones. Removed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017FAFd2PPjDZAfgP35zeQNN --- bdk_electrum_streaming/src/state.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/bdk_electrum_streaming/src/state.rs b/bdk_electrum_streaming/src/state.rs index b2cbb4c..648e925 100644 --- a/bdk_electrum_streaming/src/state.rs +++ b/bdk_electrum_streaming/src/state.rs @@ -435,7 +435,15 @@ impl State { .queuer(req_queue, JobId::Spk(script_hash)) .enqueue(request::ScriptHashSubscribe { script_hash }); } - self.staged.last_active_indices.insert(k, i); + // Notifications for spks of the same keychain can arrive in any order (it is + // unrelated to derivation order), so keep the highest index seen rather than the + // last one notified. Overwriting would under-report the last active index and leave + // a higher spk unrevealed. + self.staged + .last_active_indices + .entry(k) + .and_modify(|last| *last = (*last).max(i)) + .or_insert(i); } self.spk_jobs @@ -477,7 +485,6 @@ impl State { ) -> anyhow::Result<()> { // Borrowed field by field so a job is polled where it sits, not lifted out and put back. let Self { - spk_tracker, coord, cache, spk_jobs, @@ -501,10 +508,14 @@ impl State { spk_hash = spk_hash.to_string(), "Spk job finished" ); + // `last_active_indices` is not touched here: `on_spk_status` already + // records it, gated on the spk actually having history, at the moment + // the notification/subscription response names that history — not on job + // completion. Every spk we ever poll here (active or not, from the + // initial subscribe or a later notification) resolves to a finished job, + // so deriving the index from mere completion would tag every unused + // look-ahead spk as active too. staged.tx_update.extend(tx_update); - staged - .last_active_indices - .extend(spk_tracker.index_of_spk_hash(spk_hash)); break; } }