From 55f2115c41d59e1e8507dc5a162a678dcf72fa6c Mon Sep 17 00:00:00 2001 From: jp1ac4 <121959000+jp1ac4@users.noreply.github.com> Date: Tue, 12 Dec 2023 17:03:06 +0000 Subject: [PATCH] commands: include unconfirmed change as candidates --- doc/API.md | 3 +- src/commands/mod.rs | 30 ++++++----- tests/test_spend.py | 118 +++++++++++++++++++++++--------------------- 3 files changed, 81 insertions(+), 70 deletions(-) diff --git a/doc/API.md b/doc/API.md index 35a9b3b93..f01b14cd0 100644 --- a/doc/API.md +++ b/doc/API.md @@ -148,7 +148,8 @@ A coin may have one of the following four statuses: Create a transaction spending one or more of our coins. All coins must exist and not be spent. If no coins are specified in `outpoints`, they will be selected automatically from the set of -confirmed coins (see [`listcoins`](#listcoins) for coin status definitions). +confirmed coins together with any unconfirmed coins that are change outputs +(see [`listcoins`](#listcoins) for coin status definitions). Will error if the given coins are not sufficient to cover the transaction cost at 90% (or more) of the given feerate. If on the contrary the transaction is more than sufficiently funded, it will diff --git a/src/commands/mod.rs b/src/commands/mod.rs index 6f3510b7b..d08ea2097 100644 --- a/src/commands/mod.rs +++ b/src/commands/mod.rs @@ -486,13 +486,20 @@ impl DaemonControl { // the spend from a set of optional candidates. // Otherwise, only the specified coins will be used, all as mandatory candidates. let candidate_coins: Vec = if coins_outpoints.is_empty() { - // We only select confirmed coins for now. Including unconfirmed ones as well would - // introduce a whole bunch of additional complexity. + // We only select confirmed coins and unconfirmed change for now. + // Including unconfirmed external deposits would introduce a whole bunch + // of additional complexity. db_conn - .coins(&[CoinStatus::Confirmed], &[]) + .coins(&[CoinStatus::Unconfirmed, CoinStatus::Confirmed], &[]) .into_values() - .map(|c| { - coin_to_candidate(&c, /*must_select=*/ false, /*sequence=*/ None) + .filter_map(|c| { + if c.block_info.is_some() || c.is_change { + Some(coin_to_candidate( + &c, /*must_select=*/ false, /*sequence=*/ None, + )) + } else { + None + } }) .collect() } else { @@ -1382,8 +1389,8 @@ mod tests { spend_txid: None, spend_block: None, }]); - // If we try to use coin selection, the unconfirmed coin will not be used as a candidate - // and so we get a coin selection error due to insufficient funds. + // If we try to use coin selection, the unconfirmed non-change coin will not be used + // as a candidate and so we get a coin selection error due to insufficient funds. assert!(matches!( control.create_spend(&destinations, &[], 1, None), Err(CommandError::SpendCreation( @@ -1517,7 +1524,7 @@ mod tests { ))) ); - // Add a confirmed unspent coin to be used for coin selection. + // Add an unconfirmed change coin to be used for coin selection. let confirmed_op_1 = bitcoin::OutPoint { txid: dummy_op.txid, vout: dummy_op.vout + 100, @@ -1525,13 +1532,10 @@ mod tests { db_conn.new_unspent_coins(&[Coin { outpoint: confirmed_op_1, is_immature: false, - block_info: Some(BlockInfo { - height: 174500, - time: 174500, - }), + block_info: None, amount: bitcoin::Amount::from_sat(80_000), derivation_index: bip32::ChildNumber::from(42), - is_change: false, + is_change: true, spend_txid: None, spend_block: None, }]); diff --git a/tests/test_spend.py b/tests/test_spend.py index 50675b0fd..41bec53d0 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -224,104 +224,110 @@ def test_send_to_self(lianad, bitcoind): def test_coin_selection(lianad, bitcoind): """We can create a spend using coin selection.""" # Send to an (external) address. - dest_100_000 = {bitcoind.rpc.getnewaddress(): 100_000} + dest_addr_1 = bitcoind.rpc.getnewaddress() # Coin selection is not possible if we have no coins. assert len(lianad.rpc.listcoins()["coins"]) == 0 with pytest.raises( RpcError, match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", ): - lianad.rpc.createspend(dest_100_000, [], 2) + lianad.rpc.createspend({dest_addr_1: 100_000}, [], 2) # Receive a coin in an unconfirmed deposit transaction. - recv_addr = lianad.rpc.getnewaddress()["address"] - deposit = bitcoind.rpc.sendtoaddress(recv_addr, 0.0008) # 80_000 sats + recv_addr_1 = lianad.rpc.getnewaddress()["address"] + deposit_1 = bitcoind.rpc.sendtoaddress(recv_addr_1, 0.0012) # 120_000 sats wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 1) - # There are still no confirmed coins to use as candidates for selection. + # There are still no confirmed coins or unconfirmed change + # to use as candidates for selection. assert len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 0 assert len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 1 + assert lianad.rpc.listcoins(["unconfirmed"])["coins"][0]["is_change"] is False with pytest.raises( RpcError, match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", ): - lianad.rpc.createspend(dest_100_000, [], 2) + lianad.rpc.createspend({dest_addr_1: 100_000}, [], 2) # Confirm coin. - bitcoind.generate_block(1, wait_for_mempool=deposit) + bitcoind.generate_block(1, wait_for_mempool=deposit_1) wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 1) - - # Insufficient funds for coin selection. + # Coin selection now succeeds. + spend_res_1 = lianad.rpc.createspend({dest_addr_1: 100_000}, [], 2) + assert "psbt" in spend_res_1 + # Increase spend amount and we have insufficient funds again even though we + # now have confirmed coins. with pytest.raises( RpcError, match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", ): - lianad.rpc.createspend(dest_100_000, [], 2) + lianad.rpc.createspend({dest_addr_1: 200_000}, [], 2) - # Reduce spend amount. - dest_30_000 = {bitcoind.rpc.getnewaddress(): 30_000} - res = lianad.rpc.createspend(dest_30_000, [], 2) - assert "psbt" in res - - # The transaction must contain a change output. - spend_psbt = PSBT.from_base64(res["psbt"]) - assert len(spend_psbt.o) == 2 - assert len(spend_psbt.tx.vout) == 2 + # The transaction contains a change output. + spend_psbt_1 = PSBT.from_base64(spend_res_1["psbt"]) + assert len(spend_psbt_1.o) == 2 + assert len(spend_psbt_1.tx.vout) == 2 # Sign and broadcast this Spend transaction. - signed_psbt = lianad.signer.sign_psbt(spend_psbt) - lianad.rpc.updatespend(signed_psbt.to_base64()) - spend_txid = signed_psbt.tx.txid().hex() - lianad.rpc.broadcastspend(spend_txid) - + spend_txid_1 = sign_and_broadcast_psbt(lianad, spend_psbt_1) wait_for(lambda: len(lianad.rpc.listcoins()["coins"]) == 2) - coins = lianad.rpc.listcoins()["coins"] # Check that change output is unconfirmed. assert len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 1 + assert lianad.rpc.listcoins(["unconfirmed"])["coins"][0]["is_change"] is True assert len(lianad.rpc.listcoins(["spending"])["coins"]) == 1 - # Check we cannot use coins as candidates if they are spending/spent or unconfirmed. - with pytest.raises( - RpcError, - match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", - ): - lianad.rpc.createspend(dest_30_000, [], 2) - - # Now confirm the Spend. - bitcoind.generate_block(1, wait_for_mempool=spend_txid) - wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 1) - # But its value is not enough for this Spend. - dest_60_000 = {bitcoind.rpc.getnewaddress(): 60_000} + # We can use unconfirmed change as candidate. + dest_addr_2 = bitcoind.rpc.getnewaddress() + spend_res_2 = lianad.rpc.createspend({dest_addr_2: 10_000}, [], 2) + assert "psbt" in spend_res_2 + spend_psbt_2 = PSBT.from_base64(spend_res_2["psbt"]) + # The spend is using the unconfirmed change. + assert spend_psbt_2.tx.vin[0].prevout.hash == uint256_from_str( + bytes.fromhex(spend_txid_1)[::-1] + ) + # Get another coin to check coin selection with more than one candidate. + recv_addr_2 = lianad.rpc.getnewaddress()["address"] + deposit_2 = bitcoind.rpc.sendtoaddress(recv_addr_2, 0.0002) # 20_000 sats + wait_for(lambda: len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 2) + assert ( + len( + [ + c + for c in lianad.rpc.listcoins(["unconfirmed"])["coins"] + if c["is_change"] + ] + ) + == 1 + ) + # As only one unconfirmed coin is change, we have insufficient funds. + dest_addr_3 = bitcoind.rpc.getnewaddress() with pytest.raises( RpcError, match="Coin selection error: 'Insufficient funds. Missing \\d+ sats.'", ): - lianad.rpc.createspend(dest_60_000, [], 2) - - # Get another coin to check coin selection with more than one candidate. - recv_addr = lianad.rpc.getnewaddress()["address"] - deposit = bitcoind.rpc.sendtoaddress(recv_addr, 0.0002) # 20_000 sats - bitcoind.generate_block(1, wait_for_mempool=deposit) + lianad.rpc.createspend({dest_addr_3: 30_000}, [], 2) + # Now confirm both coins. + bitcoind.generate_block(1, wait_for_mempool=deposit_2) wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 2) + spend_res_3 = lianad.rpc.createspend({dest_addr_3: 30_000}, [], 2) + assert "psbt" in spend_res_3 - res = lianad.rpc.createspend(dest_60_000, [], 2) - assert "psbt" in res - - # The transaction must contain a change output. - auto_psbt = PSBT.from_base64(res["psbt"]) - assert len(auto_psbt.o) == 2 - assert len(auto_psbt.tx.vout) == 2 + # The transaction contains a change output. + spend_psbt_3 = PSBT.from_base64(spend_res_3["psbt"]) + assert len(spend_psbt_3.i) == 2 + assert len(spend_psbt_3.o) == 2 + assert len(spend_psbt_3.tx.vout) == 2 # Now create a transaction with manual coin selection using the same outpoints. outpoints = [ - f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in auto_psbt.tx.vin + f"{txin.prevout.hash:064x}:{txin.prevout.n}" for txin in spend_psbt_3.tx.vin ] - res_manual = lianad.rpc.createspend(dest_60_000, outpoints, 2) - manual_psbt = PSBT.from_base64(res_manual["psbt"]) + res_manual = lianad.rpc.createspend({dest_addr_3: 30_000}, outpoints, 2) + psbt_manual = PSBT.from_base64(res_manual["psbt"]) # Recipient details are the same for both. - assert auto_psbt.tx.vout[0].nValue == manual_psbt.tx.vout[0].nValue - assert auto_psbt.tx.vout[0].scriptPubKey == manual_psbt.tx.vout[0].scriptPubKey + assert spend_psbt_3.tx.vout[0].nValue == psbt_manual.tx.vout[0].nValue + assert spend_psbt_3.tx.vout[0].scriptPubKey == psbt_manual.tx.vout[0].scriptPubKey # Change amount is the same (change address will be different). - assert auto_psbt.tx.vout[1].nValue == manual_psbt.tx.vout[1].nValue + assert spend_psbt_3.tx.vout[1].nValue == psbt_manual.tx.vout[1].nValue def test_coin_selection_changeless(lianad, bitcoind):