commands: require change for self-send

[jp1ac4]

This adds a new LowestFeeChangeCondition metric for use in coin selection. It's the same as LowestFee with the option to only find solutions with change.

This option is then used for self-sends to ensure only a solution with change will be returned.

[darosior]

How about making this error:

liana/src/commands/mod.rs

Lines 556 to 578 in ed35a79

	// If necessary, add a change output.
	if change_amount.to_sat() > 0 {
	// Don't forget to update our next change index!
	let next_index = change_index
	.increment()
	.expect("Must not get into hardened territory");
	db_conn.set_change_index(next_index, &self.secp);
	check_output_value(change_amount)?;
	// TODO: shuffle once we have Taproot
	change_txo.value = change_amount.to_sat();
	tx.output.push(change_txo);
	psbt_outs.push(PsbtOut {
	bip32_derivation: change_desc.bip32_derivations(),
	..PsbtOut::default()
	});
	} else if is_self_send {
	return Err(CommandError::InsufficientFunds(
	selected_coins.iter().map(|c| c.amount).sum(),
	None,
	feerate_vb,
	));
	}

A sanity check failure instead? It should never happen now: we would always fail the coin selection above before.

[darosior]

Did you mean to use a binary or here?

Suggested change

	if selector.is_target_met(target, drain) && (drain.is_some() | !must_have_change) {
	if selector.is_target_met(target, drain) && (drain.is_some() || !must_have_change) {

[jp1ac4]

Oops, thanks for spotting. That should be a logical or. I'll change it.

[jp1ac4]

A sanity check failure instead? It should never happen now: we would always fail the coin selection above before.

Ah yes, that's true. I think in that case the sanity check would already catch it, as it checks for tx.output.is_empty(). So it should be enough just to remove the else if is_self_send { ... } check in the snippet above.

[jp1ac4]

I've now made the suggested changes, and also made a small fix to the documentation.

[darosior]

ACK 7a188f2

Looks good to me, but since more concepts from create_spend are bleeding into select_coins_for_spend (the self-send) we should either document it or tweak its interface a bit.

[darosior]

Would have been nice to have a comment above about how change_amount must be positive in case of a self send, and how if this doesn't hold the sanity checks will catch this.

[jp1ac4]

Ah yes, I'll do that.

[darosior]

The doc-comment of the function should be updated to document this.

[darosior]

I think even better would be to pass it as a parameter, so this function does not have any notion of self-send: it just exposes an interface to select coins such as there is always a change. And we set this in create_spend where we already have this notion of self-spend.

[jp1ac4]

The new parameter sounds good, will add that.

[jp1ac4]

I've made the suggested changes and also fixed, in a separate commit, an unrelated part of documentation for select_coins_for_spend.

[darosior]

ACK 6123b87