rbf: use largest change output for change index

src/commands/mod.rs

@@ -1014,30 +1014,45 @@ impl DaemonControl {
                 feerate_vb,
             )));
         }
-        let prev_txos = &prev_psbt.unsigned_tx.output;
-        let mut destinations: HashMap<bitcoin::Address, bitcoin::Amount> =
-            HashMap::with_capacity(prev_txos.len());
-        let mut change_index: Option<bip32::ChildNumber> = None;
-        for txo in prev_txos {
-            let address = bitcoin::Address::from_script(
-                &txo.script_pubkey,
-                self.config.bitcoin_config.network,
-            )
-            .map_err(CommandError::Address)?;
-            match db_conn.derivation_index_by_address(&address) {
-                Some((ind, true)) => {
-                    // TODO: check if there is more than one change output?
-                    println!("found change at address {} and index {}", address, ind);
-                    change_index = Some(ind);
-                }
-                _ => {
-                    // Non-change outputs are handled in the same way whether they are ours or not.
-                    if !is_cancel {
-                        destinations.insert(address, bitcoin::Amount::from_sat(txo.value));
-                    }
+        let prev_derivs: Vec<_> = prev_psbt
+            .unsigned_tx
+            .output
+            .iter()
+            .map(|txo| {
+                let address = bitcoin::Address::from_script(
+                    &txo.script_pubkey,
+                    self.config.bitcoin_config.network,
+                )
+                .expect("address already used in mempool transaction");
+                (
+                    address.clone(),
+                    bitcoin::Amount::from_sat(txo.value),
+                    db_conn.derivation_index_by_address(&address),
+                )
+            })
+            .collect();
+        // Set the change index we'll use for the replacement transaction, if any, to that of
+        // the change output with the largest value and then largest index.
+        let change_index = prev_derivs
+            .iter()
+            .filter_map(|(_, amt, deriv)| {
+                if let Some((ind, true)) = &deriv {
+                    Some((*ind, amt))
+                } else {
+                    None
                 }
-            };
-        }
+            })
+            .max_by(|(ind_1, amt_1), (ind_2, amt_2)| amt_1.cmp(amt_2).then(ind_1.cmp(ind_2)))
+            .map(|(ind, _)| ind);
+        // Use all previous outputs as destinations, except for the output corresponding to the change index we found above.
+        let destinations: HashMap<bitcoin::Address, bitcoin::Amount> = prev_derivs
+            .iter()
+            .filter_map(|(addr, amt, deriv)| match deriv {
+                Some((ind, true)) if *ind == change_index.expect("change_index is some here") => None,
+                _ => Some((addr.clone(), *amt)),
+            })
+            .collect();
+
         // If `!is_cancel`, we take the previous coins as mandatory candidates and add confirmed coins as optional.
         // Otherwise, we take the previous coins as optional candidates and let coin selection find the
         // best solution that includes at least one of these. If there are insufficient funds to create the replacement