bitcoindevkit / bitcoindevkit/bdk

Electrum sync forwards unrelated history transactions and its caches never evict

Open
#2,302 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**

`populate_with_spks` (`crates/electrum/src/bdk_electrum_client.rs:333-344`) fetches every transaction listed in a script's history and pushes it to `tx_update.txs`. `fetch_tx` only checks that the returned body matches the requested txid; nothing checks that the transaction pays to or spends from the queried script, which is what the method's doc comment describes. `TxGraph::apply_update` inserts all `txs` unconditionally, so whatever the server lists ends up in the wallet's graph and persisted changeset. With `fetch_prev_txouts = true`, `fetch_prev_txout` (lines 621-647) additionally fetches the parent of every input of every such transaction and adds it as a floating txout. `populate_with_outpoints` (lines 385-432) fetches history entries one by one until it finds a spend; the non-matching ones are left out of the update but stay in the cache.

The client's `tx_cache`, `block_header_cache` and `anchor_cache` (lines 27-31) only ever grow; there is no cap or eviction for the lifetime of a `BdkElectrumClient`. Together this means the amount of data downloaded and retained per sync is determined by the server's history responses rather than by the wallet's own activity. A server that lists many large, unrelated transactions for a single script makes the client download all of them, keep them in memory and forward them to the wallet.

This issue was found by AI.

**To Reproduce**

Add `crates/electrum/tests/test_unrelated_history.rs` and run `cargo test -p bdk_electrum --test test_unrelated_history`. It runs a stub Electrum server in-process; no real server is needed:

```rust
use std::io::{BufRead, BufReader, Write};
use std::net::TcpListener;

use bdk_chain::bitcoin::{
absolute, consensus, hashes::Hash, transaction, Amount, ScriptBuf, Transaction, TxIn, TxOut,
WPubkeyHash,
};
use bdk_chain::spk_client::SyncRequest;
use bdk_electrum::electrum_client::{Client, ConfigBuilder};
use bdk_electrum::BdkElectrumClient;

/// Electrum stub that lists `tx` in the history of every script it is asked about.
fn serve_history_with(listener: TcpListener, tx: Transaction) {
let (txid, raw_tx) = (tx.compute_txid(), consensus::encode::serialize_hex(&tx));
for stream in listener.incoming() {
let mut stream = stream.unwrap();
for request in BufReader::new(stream.try_clone().unwrap()).lines() {
let request = request.unwrap();
let id_start = request.find("\"id\":").unwrap() + 5;
let id: String = request[id_start..].chars().take_while(char::is_ascii_digit).collect();
let result = if request.contains("blockchain.scripthash.get_history") {
format!(r#"[{{"height":0,"tx_hash":"{txid}"}}]"#)
} else if request.contains("blockchain.transaction.get") {
format!(r#""{raw_tx}""#)
} else {
panic!("unexpected request: {request}");
};
writeln!(stream, r#"{{"jsonrpc":"2.0","id":{id},"result":{result}}}"#).unwrap();
}
}
}

#[test]
fn sync_ignores_history_entries_unrelated_to_the_script() {
let wallet_spk = ScriptBuf::new_p2wpkh(&WPubkeyHash::from_byte_array([1; 20]));
let unrelated_tx = Transaction {
version: transaction::Version::TWO,
lock_time: absolute::LockTime::ZERO,
input: vec![TxIn::default()],
output: vec![TxOut {
value: Amount::from_sat(1_000),
script_pubkey: ScriptBuf::new_p2wpkh(&WPubkeyHash::from_byte_array([2; 20])),
}],
};
let unrelated_txid = unrelated_tx.compute_txid();

let listener = TcpListener::bind("127.0.0.1:0").unwrap();
let url = format!("tcp://{}", listener.local_addr().unwrap());
std::thread::spawn(move || serve_history_with(listener, unrelated_tx));

let config = ConfigBuilder::new().retry(0).build();
let client = BdkElectrumClient::new(Client::from_config(&url, config).unwrap());
let request = SyncRequest::builder().spks([wallet_spk]);
let response = client.sync(request, 1, false).unwrap();

let txids: Vec<_> = response.tx_update.txs.iter().map(|tx| tx.compute_txid()).collect();
assert!(!txids.contains(&unrelated_txid), "update contains unrelated tx {unrelated_txid}");
}
```

The assertion fails with `update contains unrelated tx ad20c55e…`: the transaction neither pays to the requested script nor spends from it, yet it is part of the sync update.

**Expected behavior**

The update should only contain transactions that involve the requested scripts or outpoints, and the data the client downloads and retains should not grow without bound based on what the server lists.

Related: #2295 (scan termination also depends only on the server's history responses).

Contributor guide

Open the contributing guide

Research direction

Start in crates/electrum/src/bdk_electrum_client.rs, reading populate_with_spks, populate_with_outpoints, fetch_prev_txout, and the cache fields around lines 27-31. Run crates/electrum/tests/test_unrelated_history.rs with cargo test -p bdk_electrum --test test_unrelated_history, then inspect related sync tests and issue #2295. Done means unrelated history entries are excluded and cache growth is bounded without breaking valid sync results.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.