Skip to content

Commit

Permalink
Merge bitcoindevkit#1756: fix(electrum): prevent fetch_prev_txout f…
Browse files Browse the repository at this point in the history
…rom querying coinbase transactions

d4ef266 test(electrum): `fetch_prev_txout` does not process coinbase transactions (Wei Chen)
0944b35 fix(electrum): prevent `fetch_prev_txout` from querying coinbase transactions (Wei Chen)

Pull request description:

  Fixes bitcoindevkit#1698.

  ### Description

  `fetch_prev_txout` should not try to query the prevouts of coinbase transactions, as it may result in the Electrum server returning an error which would cause the overall `sync`/`full_scan` to fail. Without this critical bug fix,  `bdk_electrum` will crash when someone tracks an address being mined to.

  ### Notes to the reviewers

  ### Changelog notice

  * `fetch_prev_txout` no longer queries coinbase transactions.

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### Bugfixes:

  * [ ] This pull request breaks the existing API
  * [x] I've added tests to reproduce the issue which are now passing
  * [x] I'm linking the issue being fixed by this PR

ACKs for top commit:
  evanlinjin:
    ACK d4ef266

Tree-SHA512: 6423f9486e84f204cf756117cabff35ce79ee169efa43a059c1669ad0f7193b58299eee7c5672af35ab070ed8011637b0a1904866ce2f2fa4580ddc3f9f2d982
  • Loading branch information
evanlinjin committed Dec 6, 2024
2 parents 2a1787b + d4ef266 commit bcff89d
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 1 deletion.
44 changes: 43 additions & 1 deletion crates/electrum/src/bdk_electrum_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
) -> Result<(), Error> {
let mut no_dup = HashSet::<Txid>::new();
for tx in &tx_update.txs {
if no_dup.insert(tx.compute_txid()) {
if !tx.is_coinbase() && no_dup.insert(tx.compute_txid()) {
for vin in &tx.input {
let outpoint = vin.previous_output;
let vout = outpoint.vout;
Expand Down Expand Up @@ -531,3 +531,45 @@ fn chain_update(
}
Ok(tip)
}

#[cfg(test)]
mod test {
use crate::{bdk_electrum_client::TxUpdate, BdkElectrumClient};
use bdk_chain::bitcoin::{OutPoint, Transaction, TxIn};
use bdk_core::collections::BTreeMap;
use bdk_testenv::{utils::new_tx, TestEnv};
use std::sync::Arc;

#[cfg(feature = "default")]
#[test]
fn test_fetch_prev_txout_with_coinbase() {
let env = TestEnv::new().unwrap();
let electrum_client =
electrum_client::Client::new(env.electrsd.electrum_url.as_str()).unwrap();
let client = BdkElectrumClient::new(electrum_client);

// Create a coinbase transaction.
let coinbase_tx = Transaction {
input: vec![TxIn {
previous_output: OutPoint::null(),
..Default::default()
}],
..new_tx(0)
};

assert!(coinbase_tx.is_coinbase());

// Test that `fetch_prev_txout` does not process coinbase transactions. Calling
// `fetch_prev_txout` on a coinbase transaction will trigger a `fetch_tx` on a transaction
// with a txid of all zeros. If `fetch_prev_txout` attempts to fetch this transaction, this
// assertion will fail.
let mut tx_update = TxUpdate {
txs: vec![Arc::new(coinbase_tx)],
..Default::default()
};
assert!(client.fetch_prev_txout(&mut tx_update).is_ok());

// Ensure that the txouts are empty.
assert_eq!(tx_update.txouts, BTreeMap::default());
}
}
34 changes: 34 additions & 0 deletions crates/electrum/tests/test_electrum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -525,3 +525,37 @@ fn tx_can_become_unconfirmed_after_reorg() -> anyhow::Result<()> {

Ok(())
}

#[test]
fn test_sync_with_coinbase() -> anyhow::Result<()> {
let env = TestEnv::new()?;
let electrum_client = electrum_client::Client::new(env.electrsd.electrum_url.as_str())?;
let client = BdkElectrumClient::new(electrum_client);

// Setup address.
let spk_to_track = ScriptBuf::new_p2wsh(&WScriptHash::all_zeros());
let addr_to_track = Address::from_script(&spk_to_track, bdk_chain::bitcoin::Network::Regtest)?;

// Setup receiver.
let (mut recv_chain, _) = LocalChain::from_genesis_hash(env.bitcoind.client.get_block_hash(0)?);
let mut recv_graph = IndexedTxGraph::<ConfirmationBlockTime, _>::new({
let mut recv_index = SpkTxOutIndex::default();
recv_index.insert_spk((), spk_to_track.clone());
recv_index
});

// Mine some blocks.
env.mine_blocks(101, Some(addr_to_track))?;
env.wait_until_electrum_sees_block(Duration::from_secs(6))?;

// Check to see if electrum syncs properly.
assert!(sync_with_electrum(
&client,
[spk_to_track.clone()],
&mut recv_chain,
&mut recv_graph,
)
.is_ok());

Ok(())
}

0 comments on commit bcff89d

Please sign in to comment.