spend: a temporary partial fix for LowestFee

This is a temporary partial fix for
bitcoindevkit/coin-select#6 that should be
reverted once the upstream fix has been made.

When calculating the score, the excess should be added to changeless solutions
instead of those with change.

Given a solution has been found, this fix adds or removes the excess to its
incorrectly calculated score as required so that two changeless solutions can
be differentiated if one has higher excess (and therefore pays a higher fee).

Note that the `bound` function is also affected by this bug, which could mean
some branches are not considered when running BnB, but at least this fix will
mean the score for those solutions that are found is correct.

Author: jp1ac4

src/spend.rs

@@ -201,7 +201,27 @@ where
         if drain.is_none() && self.must_have_change {
             None
         } else {
-            self.lowest_fee.score(cs)
+            // This is a temporary partial fix for https://github.com/bitcoindevkit/coin-select/issues/6
+            // until it has been fixed upstream.
+            // TODO: Revert this change once upstream fix has been made.
+            // When calculating the score, the excess should be added to changeless solutions instead of
+            // those with change.
+            // Given a solution has been found, this fix adds or removes the excess to its incorrectly
+            // calculated score as required so that two changeless solutions can be differentiated
+            // if one has higher excess (and therefore pays a higher fee).
+            // Note that the `bound` function is also affected by this bug, which could mean some branches
+            // are not considered when running BnB, but at least this fix will mean the score for those
+            // solutions that are found is correct.
+            self.lowest_fee.score(cs).map(|score| {
+                // See https://github.com/bitcoindevkit/coin-select/blob/29b187f5509a01ba125a0354f6711e317bb5522a/src/metrics/lowest_fee.rs#L35-L45
+                assert!(cs.selected_value() >= self.lowest_fee.target.value);
+                let excess = (cs.selected_value() - self.lowest_fee.target.value) as f32;
+                bdk_coin_select::float::Ordf32(if drain.is_none() {
+                    score.0 + excess
+                } else {
+                    score.0 - excess
+                })
+            })
         }
     }
 

tests/test_spend.py

@@ -1,5 +1,8 @@
 from fixtures import *
-from test_framework.serializations import PSBT
+from test_framework.serializations import (
+    PSBT,
+    PSBT_IN_NON_WITNESS_UTXO,
+)
 from test_framework.utils import wait_for, COIN, RpcError
 
 
@@ -332,6 +335,24 @@ def test_coin_selection(lianad, bitcoind):
     assert auto_psbt.tx.vout[1].nValue == manual_psbt.tx.vout[1].nValue
 
 
+def test_coin_selection_changeless(lianad, bitcoind):
+    """We choose the changeless solution with lowest fee."""
+    # Get two coins with similar amounts.
+    txid_a = bitcoind.rpc.sendtoaddress(lianad.rpc.getnewaddress()["address"], 0.00031)
+    txid_b = bitcoind.rpc.sendtoaddress(lianad.rpc.getnewaddress()["address"], 0.00032)
+    bitcoind.generate_block(1, wait_for_mempool=[txid_a, txid_b])
+    wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 2)
+    # Send an amount that can be paid by just one of our coins.
+    res = lianad.rpc.createspend({bitcoind.rpc.getnewaddress(): 30800}, [], 1)
+    psbt = PSBT.from_base64(res["psbt"])
+    # Only one input needed.
+    assert len(psbt.i) == 1
+    # Coin A is used as input.
+    assert [psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] for psbt_in in psbt.i][
+        0
+    ] == bytes.fromhex(bitcoind.rpc.gettransaction(txid_a)["hex"])
+
+
 def test_sweep(lianad, bitcoind):
     """
     Test we can leverage the change_address parameter to partially or completely sweep