spend: warn when overpaying fee

[jp1ac4]

This is a first step towards #811. The GUI will be updated in a follow-up PR.

It adds a warning if there is any change/excess available that has been added to the fee, building on the assumption from bitcoindevkit/coin-select#14 that the decision whether to include change depends on whether the excess is over a threshold. The function select_coins_for_spend() now returns this threshold so that the caller can check if the possible change amount is above it.

I've used an enum for the different warnings, but could use strings directly.

If the approach looks fine, I'll add some tests.

[darosior]

Approach ACK for the interface, i'm just unsure we need more than just the excess value and change amount.

[darosior]

ACK. Good unit test. Got a couple comments mostly on documentation stuff. Needs rebase.

It's always good to have a sanity check of the interface as consumed externally from the functional tests:

diff --git a/tests/test_spend.py b/tests/test_spend.py
index 94ba1a5..446acf7 100644
--- a/tests/test_spend.py
+++ b/tests/test_spend.py
@@ -26,6 +26,9 @@ def test_spend_change(lianad, bitcoind):
     assert len(spend_psbt.o) == 3
     assert len(spend_psbt.tx.vout) == 3
 
+    # Since the transaction contains a change output there is no warning.
+    assert len(res["warnings"]) == 0
+
     # Sign and broadcast this first Spend transaction.
     signed_psbt = lianad.signer.sign_psbt(spend_psbt)
     lianad.rpc.updatespend(signed_psbt.to_base64())
@@ -46,6 +49,7 @@ def test_spend_change(lianad, bitcoind):
     }
     res = lianad.rpc.createspend(destinations, outpoints, 2)
     spend_psbt = PSBT.from_base64(res["psbt"])
+    assert len(res["warnings"]) == 0
 
     # We can sign and broadcast it.
     signed_psbt = lianad.signer.sign_psbt(spend_psbt)
@@ -110,6 +114,7 @@ def test_coin_marked_spent(lianad, bitcoind):
     res = lianad.rpc.createspend(destinations, [outpoint], 6)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
+    assert len(res["warnings"]) == 0
 
     # Spend the second coin without a change output
     outpoint = next(
@@ -123,6 +128,8 @@ def test_coin_marked_spent(lianad, bitcoind):
     res = lianad.rpc.createspend(destinations, [outpoint], 1)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
+    assert len(res["warnings"]) == 1
+    assert res["warnings"][0] == ""
 
     # Spend the third coin to an address of ours, no change
     outpoints = [
@@ -136,6 +143,8 @@ def test_coin_marked_spent(lianad, bitcoind):
     res = lianad.rpc.createspend(destinations, [outpoints[0]], 1)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
+    assert len(res["warnings"]) == 1
+    assert res["warnings"][0] == ""
 
     # Spend the fourth coin to an address of ours, with change
     destinations = {
@@ -144,6 +153,7 @@ def test_coin_marked_spent(lianad, bitcoind):
     res = lianad.rpc.createspend(destinations, [outpoints[1]], 18)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
+    assert len(res["warnings"]) == 0
 
     # Batch spend the fourth and fifth coins
     outpoint = next(
@@ -159,6 +169,7 @@ def test_coin_marked_spent(lianad, bitcoind):
     res = lianad.rpc.createspend(destinations, [outpoints[2], outpoint], 2)
     psbt = PSBT.from_base64(res["psbt"])
     sign_and_broadcast(psbt)
+    assert len(res["warnings"]) == 0
 
     # All the spent coins must have been detected as such
     all_deposits = (deposit_a, deposit_b, deposit_c, deposit_d)

I've left out the error string assertion as "" since the current string is going to change.

[jp1ac4]

I've now made the suggested changes.

[darosior]

So this can only happen if we've got a selection without change such as adding a change output would decrease the feerate below the target? Any other reason for a negative excess here would be a logic error because at this point we do indeed have enough funds?

[jp1ac4]

That's right, the selected coins here have enough value without a change output and so the negative excess would mean that the selected coins don't have enough value to pay the extra weight for the change output at the target feerate.

[darosior]

ACK 5a15c74