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.

Sign up for GitHub

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

commands: use unconfirmed coins from self for coin selection #1499

Merged
merged 1 commit into from
Dec 16, 2024
Merged

commands: use unconfirmed coins from self for coin selection #1499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
31 changes: 11 additions & 20 deletions lianad/src/commands/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<CandidateCoin> = 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
Expand All @@ -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((
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down Expand Up @@ -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),
Expand Down
20 changes: 16 additions & 4 deletions tests/test_spend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -412,21 +413,32 @@ 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_change"]
if c["is_from_self"]
]
)
== 1
)
assert (
len(
[
c
for c in lianad.rpc.listcoins(["unconfirmed"])["coins"]
if c["is_change"]
]
)
== 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.
Expand Down
Loading