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