From 091d7728ee33d231fbafb839ac497f86122d0851 Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Fri, 13 Dec 2024 15:07:46 +0000 Subject: [PATCH] commands: use unconfirmed from self for coin selection We previously used unconfirmed change as a best-guess way of identifying those coins from self, but now we have the `is_from_self` field we can use it directly and be sure we only select coins that are fully under our control. --- lianad/src/commands/mod.rs | 31 +++++++++++-------------------- tests/test_spend.py | 20 ++++++++++++++++---- 2 files changed, 27 insertions(+), 24 deletions(-) diff --git a/lianad/src/commands/mod.rs b/lianad/src/commands/mod.rs index 1669da53..d779323a 100644 --- a/lianad/src/commands/mod.rs +++ b/lianad/src/commands/mod.rs @@ -495,7 +495,7 @@ 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() { - // From our unconfirmed coins, we only include those that are change outputs + // From our unconfirmed coins, we only include those that are from self // since unconfirmed external deposits are more at risk of being dropped // unexpectedly from the mempool as they are beyond the user's control. db_conn @@ -504,7 +504,7 @@ impl DaemonControl { .filter_map(|(op, c)| { if c.block_info.is_some() { Some((c, None)) // confirmed coins have no ancestor info - } else if c.is_change && !c.is_immature { + } else if c.is_from_self { // In case the mempool_entry is None, the coin will be included without // any ancestor info. Some(( @@ -1499,7 +1499,7 @@ mod tests { spend_block: None, is_from_self: false, }]); - // If we try to use coin selection, the unconfirmed non-change coin will not be used + // If we try to use coin selection, the unconfirmed not-from-self 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), @@ -1749,7 +1749,7 @@ mod tests { ))) ); - // Add an unconfirmed change coin to be used for coin selection. + // Add an unconfirmed coin from self to be used for coin selection. let confirmed_op_1 = bitcoin::OutPoint { txid: dummy_op.txid, vout: dummy_op.vout + 100, @@ -1760,10 +1760,10 @@ mod tests { block_info: None, amount: bitcoin::Amount::from_sat(80_000), derivation_index: bip32::ChildNumber::from(42), - is_change: true, + is_change: false, spend_txid: None, spend_block: None, - is_from_self: false, + is_from_self: true, }; db_conn.new_unspent_coins(&[unconfirmed_coin]); // Coin selection error due to insufficient funds. @@ -1849,30 +1849,21 @@ mod tests { assert_eq!(auto_input, manual_input); // Now check that the spend created above using auto-selection only works when the unconfirmed coin - // is change and not immature. - // 1. not change and not immature + // is from self, whether or not it is from change. + // 1. not from self and not change db_conn.remove_coins(&[unconfirmed_coin.outpoint]); let mut unconfirmed_coin_2 = unconfirmed_coin; + unconfirmed_coin_2.is_from_self = false; unconfirmed_coin_2.is_change = false; - unconfirmed_coin_2.is_immature = false; // (this is already the case) db_conn.new_unspent_coins(&[unconfirmed_coin_2]); assert!(matches!( control.create_spend(&destinations, &[], 1, None), Ok(CreateSpendResult::InsufficientFunds { .. }), )); - // 2. change and immature + // 2. not from self and change db_conn.remove_coins(&[unconfirmed_coin_2.outpoint]); + unconfirmed_coin_2.is_from_self = false; unconfirmed_coin_2.is_change = true; - unconfirmed_coin_2.is_immature = true; - db_conn.new_unspent_coins(&[unconfirmed_coin_2]); - assert!(matches!( - control.create_spend(&destinations, &[], 1, None), - Ok(CreateSpendResult::InsufficientFunds { .. }), - )); - // 3. not change and immature - db_conn.remove_coins(&[unconfirmed_coin_2.outpoint]); - unconfirmed_coin_2.is_change = false; - unconfirmed_coin_2.is_immature = true; db_conn.new_unspent_coins(&[unconfirmed_coin_2]); assert!(matches!( control.create_spend(&destinations, &[], 1, None), diff --git a/tests/test_spend.py b/tests/test_spend.py index 19081dd8..1c248325 100644 --- a/tests/test_spend.py +++ b/tests/test_spend.py @@ -339,8 +339,9 @@ def test_coin_selection(lianad, bitcoind): # 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 lianad.rpc.listcoins(["unconfirmed"])["coins"][0]["is_from_self"] is True assert len(lianad.rpc.listcoins(["spending"])["coins"]) == 1 - # We can use unconfirmed change as candidate. + # We can use unconfirmed change as candidate as it is from self. # Depending on the feerate, we'll get a warning about paying extra for the ancestor. dest_addr_2 = bitcoind.rpc.getnewaddress() # If feerate is higher than ancestor, we'll need to pay extra. @@ -412,9 +413,20 @@ def test_coin_selection(lianad, bitcoind): ) # Get another coin to check coin selection with more than one candidate. - recv_addr_2 = lianad.rpc.getnewaddress()["address"] + # This coin will be an external deposit to one of our change addresses. + recv_addr_2 = lianad.rpc.listaddresses(10, 1)["addresses"][0]["change"] deposit_2 = bitcoind.rpc.sendtoaddress(recv_addr_2, 30_000 / COIN) wait_for(lambda: len(lianad.rpc.listcoins(["unconfirmed"])["coins"]) == 2) + assert ( + len( + [ + c + for c in lianad.rpc.listcoins(["unconfirmed"])["coins"] + if c["is_from_self"] + ] + ) + == 1 + ) assert ( len( [ @@ -423,10 +435,10 @@ def test_coin_selection(lianad, bitcoind): if c["is_change"] ] ) - == 1 + == 2 ) dest_addr_3 = bitcoind.rpc.getnewaddress() - # As only one unconfirmed coin is change, we have insufficient funds. + # As only one unconfirmed coin is from self, we have insufficient funds. assert "missing" in lianad.rpc.createspend({dest_addr_3: 20_000}, [], 10) # If we include both unconfirmed coins manually, it will succeed.