Merge #867: spend: a temporary partial fix for LowestFee

5f534eb spend: a temporary partial fix for `LowestFee` (jp1ac4)

Pull request description:

  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.

  The new functional test included here fails without this fix.

ACKs for top commit:
  darosior:
    ACK 5f534eb

Tree-SHA512: f3a0e67434cad005389a938c0833394ff2c242d262f1ed1e6da9d1ce403af221bbce265a6fe67ad72f8fa8b461530faac1387c980f7f539cbb187603371c58d7

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,5 @@
 from fixtures import *
-from test_framework.serializations import PSBT
+from test_framework.serializations import PSBT, uint256_from_str
 from test_framework.utils import wait_for, COIN, RpcError
 
 
@@ -332,6 +332,23 @@ 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.
+    txid_a = uint256_from_str(bytes.fromhex(txid_a)[::-1])
+    assert psbt.tx.vin[0].prevout.hash == txid_a
+
+
 def test_sweep(lianad, bitcoind):
     """
     Test we can leverage the change_address parameter to partially or completely sweep