Merge bitcoindevkit#337: fix(wallet): Don't fail in build_fee_bump for missing parent txid

f15582d fix(wallet): Don't fail in `build_fee_bump` for missing parent txid (valued mammal)
992a08f test: Add `test_bump_fee_pay_to_anchor_foreign_utxo` (valued mammal)

Pull request description:

  ### Description

  Previously `build_fee_bump` could error with `UnknownUtxo` if a parent of the tx being fee-bumped wasn't found in the wallet. This made it impossible to use `build_fee_bump` on a transaction created using `add_foreign_utxo`. This PR is a refactor of `build_fee_bump` that manages to avoid the error by instead querying the tx graph for the txout specifically. This is a reasonable assumption because the previous txouts are necessary to compute the fee of the original tx.

  I've included a test in 992a08f to demonstrate the old behavior which now passes.

  may resolve bitcoindevkit#325.

  ### Notes to the reviewers

  ### Changelog notice

  - Fix `Wallet::build_fee_bump` to enable fee-bumping a tx which contains a foreign UTXO.

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

  #### New Features:

  #### 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:
  nymius:
    ACK f15582d
  notmandatory:
    ACK f15582d

Tree-SHA512: 51248984a4e0eec30093aab7283057d50f356827ec267d6ed957f33525157a3ba0e3fdf69d57ae497b635e39f8dbf8bc88b89b01a95267092bdb43f7a9ef3f54

Author: ValuedMammal

Cargo.toml

@@ -18,7 +18,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 
 [dependencies]
 bdk_chain = { version = "0.23.1", features = ["miniscript", "serde"], default-features = false }
-bitcoin = { version = "0.32.6", features = ["serde", "base64"], default-features = false }
+bitcoin = { version = "0.32.7", features = ["serde", "base64"], default-features = false }
 miniscript = { version = "12.3.1", features = ["serde"], default-features = false }
 rand_core = { version = "0.6.0" }
 serde_json = { version = "1" }

src/wallet/mod.rs

@@ -1711,15 +1711,15 @@ impl Wallet {
         &mut self,
         txid: Txid,
     ) -> Result<TxBuilder<'_, DefaultCoinSelectionAlgorithm>, BuildFeeBumpError> {
-        let graph = self.indexed_graph.graph();
+        let tx_graph = self.indexed_graph.graph();
         let txout_index = &self.indexed_graph.index;
         let chain_tip = self.chain.tip().block_id();
-        let chain_positions = graph
+        let chain_positions: HashMap<Txid, ChainPosition<_>> = tx_graph
             .list_canonical_txs(&self.chain, chain_tip, CanonicalizationParams::default())
             .map(|canon_tx| (canon_tx.tx_node.txid, canon_tx.chain_position))
-            .collect::<HashMap<Txid, _>>();
+            .collect();
 
-        let mut tx = graph
+        let mut tx = tx_graph
             .get_tx(txid)
             .ok_or(BuildFeeBumpError::TransactionNotFound(txid))?
             .as_ref()
@@ -1746,73 +1746,62 @@ impl Wallet {
         let fee = self
             .calculate_fee(&tx)
             .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?;
-        let fee_rate = self
-            .calculate_fee_rate(&tx)
-            .map_err(|_| BuildFeeBumpError::FeeRateUnavailable)?;
+        let fee_rate = fee / tx.weight();
 
         // Remove the inputs from the tx and process them.
-        let utxos = tx
+        let utxos: Vec<WeightedUtxo> = tx
             .input
             .drain(..)
             .map(|txin| -> Result<_, BuildFeeBumpError> {
-                graph
-                    // Get previous transaction.
-                    .get_tx(txin.previous_output.txid)
-                    .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))
-                    // Get chain position.
-                    .and_then(|prev_tx| {
+                let outpoint = txin.previous_output;
+                let prev_txout = tx_graph
+                    .get_txout(outpoint)
+                    .cloned()
+                    .ok_or(BuildFeeBumpError::UnknownUtxo(outpoint))?;
+                match txout_index.index_of_spk(prev_txout.script_pubkey.clone()) {
+                    Some(&(keychain, derivation_index)) => {
+                        let txout = prev_txout;
                         let chain_position = chain_positions
-                            .get(&txin.previous_output.txid)
+                            .get(&outpoint.txid)
                             .cloned()
-                            .ok_or(BuildFeeBumpError::UnknownUtxo(txin.previous_output))?;
-                        let prev_txout = prev_tx
-                            .output
-                            .get(txin.previous_output.vout as usize)
-                            .ok_or(BuildFeeBumpError::InvalidOutputIndex(txin.previous_output))
-                            .cloned()?;
-                        Ok((prev_tx, prev_txout, chain_position))
-                    })
-                    .map(|(prev_tx, prev_txout, chain_position)| {
-                        match txout_index.index_of_spk(prev_txout.script_pubkey.clone()) {
-                            Some(&(keychain, derivation_index)) => WeightedUtxo {
-                                satisfaction_weight: self
-                                    .public_descriptor(keychain)
-                                    .max_weight_to_satisfy()
-                                    .unwrap(),
-                                utxo: Utxo::Local(LocalOutput {
-                                    outpoint: txin.previous_output,
-                                    txout: prev_txout.clone(),
-                                    keychain,
-                                    is_spent: true,
-                                    derivation_index,
-                                    chain_position,
-                                }),
-                            },
-                            None => {
-                                let satisfaction_weight = Weight::from_wu_usize(
-                                    serialize(&txin.script_sig).len() * 4
-                                        + serialize(&txin.witness).len(),
-                                );
-                                WeightedUtxo {
-                                    utxo: Utxo::Foreign {
-                                        outpoint: txin.previous_output,
-                                        sequence: txin.sequence,
-                                        psbt_input: Box::new(psbt::Input {
-                                            witness_utxo: prev_txout
-                                                .script_pubkey
-                                                .witness_version()
-                                                .map(|_| prev_txout.clone()),
-                                            non_witness_utxo: Some(prev_tx.as_ref().clone()),
-                                            ..Default::default()
-                                        }),
-                                    },
-                                    satisfaction_weight,
-                                }
-                            }
-                        }
-                    })
+                            .ok_or(BuildFeeBumpError::TransactionNotFound(outpoint.txid))?;
+                        Ok(WeightedUtxo {
+                            satisfaction_weight: self
+                                .public_descriptor(keychain)
+                                .max_weight_to_satisfy()
+                                .expect("descriptor should be satisfiable"),
+                            utxo: Utxo::Local(LocalOutput {
+                                outpoint,
+                                txout,
+                                keychain,
+                                is_spent: true,
+                                derivation_index,
+                                chain_position,
+                            }),
+                        })
+                    }
+                    None => Ok(WeightedUtxo {
+                        satisfaction_weight: Weight::from_wu_usize(
+                            serialize(&txin.script_sig).len() * 4 + serialize(&txin.witness).len(),
+                        ),
+                        utxo: Utxo::Foreign {
+                            outpoint,
+                            sequence: txin.sequence,
+                            psbt_input: Box::new(psbt::Input {
+                                witness_utxo: prev_txout
+                                    .script_pubkey
+                                    .witness_version()
+                                    .map(|_| prev_txout),
+                                non_witness_utxo: tx_graph
+                                    .get_tx(outpoint.txid)
+                                    .map(|tx| tx.as_ref().clone()),
+                                ..Default::default()
+                            }),
+                        },
+                    }),
+                }
             })
-            .collect::<Result<Vec<WeightedUtxo>, BuildFeeBumpError>>()?;
+            .collect::<Result<_, _>>()?;
 
         if tx.output.len() > 1 {
             let mut change_index = None;
@@ -1832,7 +1821,6 @@ impl Wallet {
         }
 
         let params = TxParams {
-            // TODO: figure out what rbf option should be?
             version: Some(tx.version),
             recipients: tx
                 .output

tests/build_fee_bump.rs

@@ -8,8 +8,8 @@ use bdk_wallet::psbt::PsbtUtils;
 use bdk_wallet::test_utils::*;
 use bdk_wallet::KeychainKind;
 use bitcoin::{
-    absolute, transaction, Address, Amount, FeeRate, OutPoint, ScriptBuf, Sequence, Transaction,
-    TxOut,
+    absolute, hashes::Hash, psbt, transaction, Address, Amount, FeeRate, OutPoint, ScriptBuf,
+    Sequence, Transaction, TxOut, Weight,
 };
 
 mod common;
@@ -944,3 +944,51 @@ fn test_legacy_bump_fee_absolute_add_input() {
 
     assert_eq!(fee, Amount::from_sat(6_000));
 }
+
+// Test that we can fee-bump a tx containing a foreign (p2a) utxo.
+#[test]
+fn test_bump_fee_pay_to_anchor_foreign_utxo() {
+    let (mut wallet, _) = get_funded_wallet_wpkh();
+    let drain_spk = wallet
+        .next_unused_address(KeychainKind::External)
+        .script_pubkey();
+
+    let witness_utxo = TxOut {
+        value: Amount::ONE_SAT,
+        script_pubkey: bitcoin::ScriptBuf::new_p2a(),
+    };
+    // Remember to include this as a "floating" txout in the wallet.
+    let outpoint = OutPoint::new(Hash::hash(b"prev"), 1);
+    wallet.insert_txout(outpoint, witness_utxo.clone());
+    let satisfaction_weight = Weight::from_wu(71);
+    let psbt_input = psbt::Input {
+        witness_utxo: Some(witness_utxo),
+        ..Default::default()
+    };
+
+    let mut tx_builder = wallet.build_tx();
+    tx_builder
+        .add_foreign_utxo(outpoint, psbt_input, satisfaction_weight)
+        .unwrap()
+        .only_witness_utxo()
+        .fee_rate(FeeRate::from_sat_per_vb_unchecked(2))
+        .drain_to(drain_spk.clone());
+    let psbt = tx_builder.finish().unwrap();
+    let tx = psbt.unsigned_tx.clone();
+    assert!(tx.input.iter().any(|txin| txin.previous_output == outpoint));
+    let txid1 = tx.compute_txid();
+    wallet.apply_unconfirmed_txs([(tx, 123456)]);
+
+    // Now build fee bump.
+    let mut tx_builder = wallet
+        .build_fee_bump(txid1)
+        .expect("`build_fee_bump` should succeed");
+    tx_builder
+        .set_recipients(vec![])
+        .drain_to(drain_spk)
+        .only_witness_utxo()
+        .fee_rate(FeeRate::from_sat_per_vb_unchecked(5));
+    let psbt = tx_builder.finish().unwrap();
+    let tx = &psbt.unsigned_tx;
+    assert!(tx.input.iter().any(|txin| txin.previous_output == outpoint));
+}