Fix the fee_amount calculation when drain_val...

...should be negative

When we calculate drain_val we use saturating_sub,
which means that when the operation is supposed to be negative,
we have 0 as a result instead. This means we can't calculate the
fee amount as `fee_amount += drain_val`.
A possible fix could have been making drain_val signed, but instead
I went for a slight refactor of the fee calculation, to make it
clearer.
Fixes #660

src/wallet/mod.rs

@@ -813,9 +813,12 @@ where
             }
         };
 
-        fee_amount += fee_rate.fee_vb(serialize(&drain_output).len());
-
-        let drain_val = (coin_selection.selected_amount() - outgoing).saturating_sub(fee_amount);
+        // sum(inputs) - sum(outputs)
+        let delta = coin_selection.selected_amount() - outgoing;
+        // Fee to be paid if we add the drain_output
+        let drain_fee = fee_rate.fee_vb(serialize(&drain_output).len());
+        // Value of the potential drain output
+        let drain_val = delta.saturating_sub(fee_amount).saturating_sub(drain_fee);
 
         if tx.output.is_empty() {
             // Uh oh, our transaction has no outputs.
@@ -838,12 +841,13 @@ where
         }
 
         if drain_val.is_dust(&drain_output.script_pubkey) {
-            fee_amount += drain_val;
+            fee_amount = delta;
         } else {
             drain_output.value = drain_val;
             if self.is_mine(&drain_output.script_pubkey)? {
                 received += drain_val;
             }
+            fee_amount = delta - drain_val;
             // TODO: We should pay attention when adding a new output: this might increase
             // the lenght of the "number of vouts" parameter by 2 bytes, potentially making
             // our feerate too low
@@ -3969,6 +3973,45 @@ pub(crate) mod test {
         builder.finish().unwrap();
     }
 
+    #[test]
+    fn test_fee_amount_negative_drain_val() {
+        // While building the transaction, bdk would calculate the drain_value
+        // as
+        // current_delta - fee_amount - drain_fee
+        // using saturating_sub, meaning that if the result would end up negative,
+        // it'll remain to zero instead.
+        // This caused a bug in master where we would calculate the wrong fee
+        // for a transaction.
+        let (wallet, descriptors, _) = get_funded_wallet(get_test_wpkh());
+        let send_to = Address::from_str("tb1ql7w62elx9ucw4pj5lgw4l028hmuw80sndtntxt").unwrap();
+        let incoming_txid = crate::populate_test_db!(
+            wallet.database.borrow_mut(),
+            testutils! (@tx ( (@external descriptors, 0) => 8859 ) (@confirmations 1)),
+            Some(100),
+        );
+
+        let mut builder = wallet.build_tx();
+        builder
+            .add_recipient(send_to.script_pubkey(), 8630)
+            .add_utxo(OutPoint::new(incoming_txid, 0))
+            .unwrap()
+            .enable_rbf()
+            .fee_rate(FeeRate::from_sat_per_vb(2.01));
+        let (psbt, details) = builder.finish().unwrap();
+
+        assert!(psbt.inputs.len() == 1);
+        let fee_amount = psbt
+            .inputs
+            .iter()
+            .fold(0, |acc, i| acc + i.witness_utxo.as_ref().unwrap().value)
+            - psbt
+                .unsigned_tx
+                .output
+                .iter()
+                .fold(0, |acc, o| acc + o.value);
+        assert_eq!(fee_amount, details.fee.unwrap());
+    }
+
     #[test]
     fn test_sign_single_xprv() {
         let (wallet, _, _) = get_funded_wallet("wpkh(tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)");