From 5f534eb988b903c28b8c2edb1db9a6389aadfdc0 Mon Sep 17 00:00:00 2001
From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com>
Date: Mon, 11 Dec 2023 15:30:20 +0000
Subject: [PATCH] spend: a temporary partial fix for `LowestFee`

This is a temporary partial fix for
https://github.com/bitcoindevkit/coin-select/issues/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.
---
 src/spend.rs        | 22 +++++++++++++++++++++-
 tests/test_spend.py | 19 ++++++++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/src/spend.rs b/src/spend.rs
index 0d7362f48..35936e0c0 100644
--- a/src/spend.rs
+++ b/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
+                })
+            })
         }
     }
 
diff --git a/tests/test_spend.py b/tests/test_spend.py
index 910c9ed2b..94ba1a592 100644
--- a/tests/test_spend.py
+++ b/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