Merge bitcoindevkit#1756: fix(electrum): prevent fetch_prev_txout f…

…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

crates/electrum/src/bdk_electrum_client.rs

@@ -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;
@@ -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());
+    }
+}

crates/electrum/tests/test_electrum.rs

@@ -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(())
+}