Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spend: a temporary partial fix for LowestFee #867

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/spend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
})
}
}

Expand Down
19 changes: 18 additions & 1 deletion tests/test_spend.py
Original file line number Diff line number Diff line change
@@ -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


Expand Down Expand Up @@ -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
Expand Down