Merge bitcoindevkit/bdk#1837: bdk_electrum: Handle negative heights properly

161d715 test(electrum): Chained mempool sync (志宇)
7502052 fix(electrum): Handle negative heights properly (志宇)

Pull request description:

  ### Description

  This fixes a problem with `bdk_electrum` making bogus `transaction_get_merkle` calls to the Electrum server.

  In electrum, heights returned alongside txs may be 0 or -1. 0 means the tx is unconfirmed. -1 means the tx is unconfirmed and spends an unconfirmed tx.

  Previously, the codebase assumed that heights cannot be negative and used a `i32 as usize` cast (which would lead to the casted height being 18446744073709551615). Then the codebase will try to call `transaction_get_merkle` with the overflowed height.

  ### Notes to the reviewers

  The test introduced in this PR does not fail with the old codebase. What ends up happening is that `transaction_get_merke` will be called with the overflow height and the Electrum server will return with "merkle not found".

  To see the failure, you can change the `height as usize` casts to use `.try_into().expect()` then you will see a panic.

  These changes should be applied as `1.0.1` and `1.1.1` fixes.

  ### Changelog notice

  * Fix `bdk_electrum` handling of negative spk-history height.

  ### 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:

  * [x] I've added tests to reproduce the issue which are now passing

  ~* [ ] This pull request breaks the existing API~
  ~* [ ] I'm linking the issue being fixed by this PR~

ACKs for top commit:
  LagginTimes:
    ACK 161d715

Tree-SHA512: 18bc5c6ccebd810e253cc88a69554ef6a27168df1961a5d77e2044bf30f4e27d44cb3a2499f5d29f0a3bd0eb349cacd7ea870f9c7225dd09798c7e19091a0f04

Author: ValuedMammal

crates/electrum/src/bdk_electrum_client.rs

@@ -276,7 +276,9 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
 
                 for tx_res in spk_history {
                     tx_update.txs.push(self.fetch_tx(tx_res.tx_hash)?);
-                    self.validate_merkle_for_anchor(tx_update, tx_res.tx_hash, tx_res.height)?;
+                    if let Ok(height) = tx_res.height.try_into() {
+                        self.validate_merkle_for_anchor(tx_update, tx_res.tx_hash, height)?;
+                    }
                 }
             }
         }
@@ -312,7 +314,9 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
                 if !has_residing && res.tx_hash == op_txid {
                     has_residing = true;
                     tx_update.txs.push(Arc::clone(&op_tx));
-                    self.validate_merkle_for_anchor(tx_update, res.tx_hash, res.height)?;
+                    if let Ok(height) = res.height.try_into() {
+                        self.validate_merkle_for_anchor(tx_update, res.tx_hash, height)?;
+                    }
                 }
 
                 if !has_spending && res.tx_hash != op_txid {
@@ -326,7 +330,9 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
                         continue;
                     }
                     tx_update.txs.push(Arc::clone(&res_tx));
-                    self.validate_merkle_for_anchor(tx_update, res.tx_hash, res.height)?;
+                    if let Ok(height) = res.height.try_into() {
+                        self.validate_merkle_for_anchor(tx_update, res.tx_hash, height)?;
+                    }
                 }
             }
         }
@@ -360,7 +366,9 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
                 .into_iter()
                 .find(|r| r.tx_hash == txid)
             {
-                self.validate_merkle_for_anchor(tx_update, txid, r.height)?;
+                if let Ok(height) = r.height.try_into() {
+                    self.validate_merkle_for_anchor(tx_update, txid, height)?;
+                }
             }
 
             tx_update.txs.push(tx);
@@ -374,11 +382,11 @@ impl<E: ElectrumApi> BdkElectrumClient<E> {
         &self,
         tx_update: &mut TxUpdate<ConfirmationBlockTime>,
         txid: Txid,
-        confirmation_height: i32,
+        confirmation_height: usize,
     ) -> Result<(), Error> {
         if let Ok(merkle_res) = self
             .inner
-            .transaction_get_merkle(&txid, confirmation_height as usize)
+            .transaction_get_merkle(&txid, confirmation_height)
         {
             let mut header = self.fetch_header(merkle_res.block_height as u32)?;
             let mut is_confirmed_tx = electrum_client::utils::validate_merkle_proof(

crates/electrum/tests/test_electrum.rs

@@ -5,9 +5,15 @@ use bdk_chain::{
     spk_txout::SpkTxOutIndex,
     Balance, ConfirmationBlockTime, IndexedTxGraph, Indexer, Merge, TxGraph,
 };
+use bdk_core::bitcoin::Network;
 use bdk_electrum::BdkElectrumClient;
-use bdk_testenv::{anyhow, bitcoincore_rpc::RpcApi, TestEnv};
+use bdk_testenv::{
+    anyhow,
+    bitcoincore_rpc::{json::CreateRawTransactionInput, RawTx, RpcApi},
+    TestEnv,
+};
 use core::time::Duration;
+use electrum_client::ElectrumApi;
 use std::collections::{BTreeSet, HashSet};
 use std::str::FromStr;
 
@@ -54,6 +60,63 @@ where
     Ok(update)
 }
 
+/// If an spk history contains a tx that spends another unconfirmed tx (chained mempool history),
+/// the Electrum API will return the tx with a negative height. This should succeed and not panic.
+#[test]
+pub fn chained_mempool_tx_sync() -> anyhow::Result<()> {
+    let env = TestEnv::new()?;
+    let rpc_client = env.rpc_client();
+    let electrum_client = electrum_client::Client::new(env.electrsd.electrum_url.as_str())?;
+
+    let tracked_addr = rpc_client
+        .get_new_address(None, None)?
+        .require_network(Network::Regtest)?;
+
+    env.mine_blocks(100, None)?;
+
+    // First unconfirmed tx.
+    env.send(&tracked_addr, Amount::from_btc(1.0)?)?;
+
+    // Create second unconfirmed tx that spends the first.
+    let utxo = rpc_client
+        .list_unspent(None, Some(0), None, Some(true), None)?
+        .into_iter()
+        .find(|utxo| utxo.script_pub_key == tracked_addr.script_pubkey())
+        .expect("must find the newly created utxo");
+    let tx_that_spends_unconfirmed = rpc_client.create_raw_transaction(
+        &[CreateRawTransactionInput {
+            txid: utxo.txid,
+            vout: utxo.vout,
+            sequence: None,
+        }],
+        &[(
+            tracked_addr.to_string(),
+            utxo.amount - Amount::from_sat(1000),
+        )]
+        .into(),
+        None,
+        None,
+    )?;
+    let signed_tx = rpc_client
+        .sign_raw_transaction_with_wallet(tx_that_spends_unconfirmed.raw_hex(), None, None)?
+        .transaction()?;
+    rpc_client.send_raw_transaction(signed_tx.raw_hex())?;
+
+    env.wait_until_electrum_sees_txid(signed_tx.compute_txid(), Duration::from_secs(5))?;
+
+    let spk_history = electrum_client.script_get_history(&tracked_addr.script_pubkey())?;
+    assert!(
+        spk_history.into_iter().any(|tx_res| tx_res.height < 0),
+        "must find tx with negative height"
+    );
+
+    let client = BdkElectrumClient::new(electrum_client);
+    let request = SyncRequest::builder().spks(core::iter::once(tracked_addr.script_pubkey()));
+    let _response = client.sync(request, 1, false)?;
+
+    Ok(())
+}
+
 #[test]
 pub fn test_update_tx_graph_without_keychain() -> anyhow::Result<()> {
     let env = TestEnv::new()?;