bitcoindevkit / bitcoindevkit/bdk

Electrum batch responses are paired with `zip` without length checks

Open
#2,305 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Rust
Stars
1.1k
Forks
483
Avg merge
20d 3h
Merged PRs (30d)
3

Description

**Describe the bug**

`BdkElectrumClient` pairs every batch request with its response using `Iterator::zip` and never checks that the response has as many entries as the request:

- `populate_with_spks`: `batch_script_get_history` (`crates/electrum/src/bdk_electrum_client.rs:305-309`)
- `populate_with_outpoints`: `batch_script_get_history` (lines 377-381)
- `populate_with_txids`: `batch_script_get_history` (lines 495-499)
- `batch_fetch_anchors`: `batch_block_header` (lines 548-549) and `batch_transaction_get_merkle` (lines 571-574)

With a short response the trailing items are silently skipped: scripts at the end of a batch are neither scanned nor get their expected txids marked as evicted, trailing requested txids are dropped from the update, and trailing anchors are never built, while the sync still reports success. For `batch_block_header`, a short response leaves `height_to_hash` incomplete and the indexing at line 561 (`height_to_hash[&h]`) panics with `no entry found for key`.

The stock `electrum_client::Client` cannot return a short vector (its `batch_call` waits for one response per request id), so this concerns the other `ElectrumApi` implementations that `BdkElectrumClient` is generic over (custom transports, proxies, mocks).

This issue was found by AI.

**To Reproduce**

Implement `ElectrumApi` for a transport whose `batch_block_header_raw` returns one header fewer than requested, and `sync` a script whose history holds transactions confirmed at two different heights:

```rust
let client = BdkElectrumClient::new(transport);
let result = client.sync(SyncRequest::builder().spks([spk]), 10, false);
```

This panics inside the library:

```
panicked at crates/electrum/src/bdk_electrum_client.rs:561:42:
no entry found for key
```

With the same transport returning one entry fewer from `batch_script_get_history`, a sync of two scripts returns `Ok` although the second script was never queried and its `expected_spk_txids` were not marked evicted.

Full test (crates/electrum/tests/test_short_batch.rs; needs serde_json = "1" in bdk_electrum's dev-dependencies for the raw_call/batch_call signatures)

```rust
use std::borrow::Borrow;

use bdk_chain::bitcoin::{
absolute, consensus, hashes::Hash, transaction, Amount, Script, ScriptBuf, Transaction, TxIn,
TxOut, Txid, WPubkeyHash,
};
use bdk_chain::spk_client::SyncRequest;
use bdk_electrum::electrum_client::{
Batch, ElectrumApi, Error, GetBalanceRes, GetHeadersRes, GetHistoryRes, GetMerkleRes,
ListUnspentRes, Param, RawHeaderNotification, ScriptStatus, ServerFeaturesRes, TxidFromPosRes,
};
use bdk_electrum::BdkElectrumClient;

/// Transport that answers every request but returns one header fewer than asked for, and
/// optionally one history fewer.
struct ShortBatchTransport {
txs: Vec<(Transaction, usize)>,
short_history: bool,
}

impl ElectrumApi for ShortBatchTransport {
fn batch_script_get_history<'s, I>(&self, scripts: I) -> Result>, Error>
where
I: IntoIterator + Clone,
I::Item: Borrow<&'s Script>,
{
let history = self
.txs
.iter()
.map(|(tx, height)| GetHistoryRes { height: *height as i32, tx_hash: tx.compute_txid(), fee: None })
.collect::>();
let mut out: Vec<_> = scripts.into_iter().map(|_| history.clone()).collect();
if self.short_history { out.pop(); }
Ok(out)
}
fn transaction_get_raw(&self, txid: &Txid) -> Result, Error> {
let (tx, _) = self.txs.iter().find(|(tx, _)| tx.compute_txid() == *txid).unwrap();
Ok(consensus::serialize(tx))
}
fn batch_block_header_raw(&self, heights: I) -> Result>, Error>
where
I: IntoIterator + Clone,
I::Item: Borrow,
{
let requested = heights.into_iter().count();
Ok(vec![vec![0u8; 80]; requested.saturating_sub(1)])
}
fn batch_transaction_get_merkle(&self, txids_and_heights: I) -> Result, Error>
where
I: IntoIterator + Clone,
I::Item: Borrow<(Txid, usize)>,
{
Ok(txids_and_heights
.into_iter()
.map(|item| GetMerkleRes { block_height: item.borrow().1, pos: 0, merkle: vec![] })
.collect())
}

// Not exercised by `sync`.
fn raw_call(&self, _: &str, _: impl IntoIterator) -> Result { unimplemented!() }
fn batch_call(&self, _: &Batch) -> Result, Error> { unimplemented!() }
fn block_headers_subscribe_raw(&self) -> Result { unimplemented!() }
fn block_headers_pop_raw(&self) -> Result, Error> { unimplemented!() }
fn block_header_raw(&self, _: usize) -> Result, Error> { unimplemented!() }
fn block_headers(&self, _: usize, _: usize) -> Result { unimplemented!() }
fn estimate_fee(&self, _: usize) -> Result { unimplemented!() }
fn relay_fee(&self) -> Result { unimplemented!() }
fn script_subscribe(&self, _: &Script) -> Result, Error> { unimplemented!() }
fn batch_script_subscribe<'s, I>(&self, _: I) -> Result>, Error> where I: IntoIterator + Clone, I::Item: Borrow<&'s Script> { unimplemented!() }
fn script_unsubscribe(&self, _: &Script) -> Result { unimplemented!() }
fn script_pop(&self, _: &Script) -> Result, Error> { unimplemented!() }
fn script_get_balance(&self, _: &Script) -> Result { unimplemented!() }
fn batch_script_get_balance<'s, I>(&self, _: I) -> Result, Error> where I: IntoIterator + Clone, I::Item: Borrow<&'s Script> { unimplemented!() }
fn script_get_history(&self, _: &Script) -> Result, Error> { unimplemented!() }
fn script_list_unspent(&self, _: &Script) -> Result, Error> { unimplemented!() }
fn batch_script_list_unspent<'s, I>(&self, _: I) -> Result>, Error> where I: IntoIterator + Clone, I::Item: Borrow<&'s Script> { unimplemented!() }
fn batch_transaction_get_raw<'t, I>(&self, _: I) -> Result>, Error> where I: IntoIterator + Clone, I::Item: Borrow<&'t Txid> { unimplemented!() }
fn batch_estimate_fee(&self, _: I) -> Result, Error> where I: IntoIterator + Clone, I::Item: Borrow { unimplemented!() }
fn transaction_broadcast_raw(&self, _: &[u8]) -> Result { unimplemented!() }
fn transaction_get_merkle(&self, _: &Txid, _: usize) -> Result { unimplemented!() }
fn txid_from_pos(&self, _: usize, _: usize) -> Result { unimplemented!() }
fn txid_from_pos_with_merkle(&self, _: usize, _: usize) -> Result { unimplemented!() }
fn server_features(&self) -> Result { unimplemented!() }
fn ping(&self) -> Result<(), Error> { unimplemented!() }
}

fn tx_paying(spk: &ScriptBuf, lock_time: u32) -> Transaction {
Transaction {
version: transaction::Version::TWO,
lock_time: absolute::LockTime::from_consensus(lock_time),
input: vec![TxIn::default()],
output: vec![TxOut { value: Amount::from_sat(1_000), script_pubkey: spk.clone() }],
}
}

#[test]
fn short_header_batch_is_reported_as_an_error() {
let spk = ScriptBuf::new_p2wpkh(&WPubkeyHash::from_byte_array([1; 20]));
let transport = ShortBatchTransport {
txs: vec![(tx_paying(&spk, 1), 10), (tx_paying(&spk, 2), 20)],
short_history: false,
};
let client = BdkElectrumClient::new(transport);

let result = client.sync(SyncRequest::builder().spks([spk]), 10, false);
assert!(result.is_err(), "sync accepted a short header batch");
}

#[test]
fn short_history_batch_does_not_skip_scripts() {
let spk_a = ScriptBuf::new_p2wpkh(&WPubkeyHash::from_byte_array([1; 20]));
let spk_b = ScriptBuf::new_p2wpkh(&WPubkeyHash::from_byte_array([2; 20]));
let expected_txid = Txid::from_byte_array([9; 32]);
let transport = ShortBatchTransport { txs: vec![(tx_paying(&spk_a, 1), 0)], short_history: true };
let client = BdkElectrumClient::new(transport);

let request = SyncRequest::builder()
.spks([spk_a, spk_b.clone()])
.expected_spk_txids([(spk_b, expected_txid)]);
let response = client.sync(request, 10, false).unwrap();
let evicted = response.tx_update.evicted_ats.iter().any(|(t, _)| *t == expected_txid);
assert!(evicted, "second script was never scanned: {:?}", response.tx_update);
}
```

The first test panics at `bdk_electrum_client.rs:561:42` (`no entry found for key`); the second fails with `second script was never scanned` and an update that only contains the first script's transaction.

**Expected behavior**

A batch response whose length does not match the request should not silently truncate the sync result or panic.

Contributor guide

Open the contributing guide

Research direction

Start in crates/electrum/src/bdk_electrum_client.rs at the batch_script_get_history, batch_block_header, and batch_transaction_get_merkle call sites listed in the issue. Review the short-batch reproduction in crates/electrum/tests/test_short_batch.rs, then run or add its two tests. Done means mismatched response lengths produce an error rather than silently skipping results or panicking.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.