Skip to content

Conversation

@RCasatta
Copy link
Contributor

No description provided.

let env = TestEnvBuilder::from_env().with_electrum().build();
let signer = generate_signer();
let view_key = generate_view_key();
let desc = format!("ct({},elwpkh({}/*))", view_key, signer.xpub());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
let desc = format!("ct({},elwpkh({}/*))", view_key, signer.xpub());
let desc = format!("ct({},elwpkh({}/<0;1>/*))", view_key, signer.xpub());

since this PR is affecting the change we have the test generating change addresses
(although for the sake of this PR it should not matter)

let mut pset = wallet
.tx_builder()
.drain_lbtc_wallet()
.add_lbtc_recipient(&node_address, amount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a test where we do the same thing, but sending to an explicit address?

} else {
wollet.addressee_change(
// Track whether we add a change/drain output (needed later for fee adjustment).
// Skip change only when: drain_lbtc is set AND there are explicit recipients AND no drain_to.
Copy link
Contributor

@LeoComandini LeoComandini Jan 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where are we checking checking that there are explicit recipients ?

// Track whether we add a change/drain output (needed later for fee adjustment).
// Skip change only when: drain_lbtc is set AND there are explicit recipients AND no drain_to.
// If there are no recipients (satoshi_out == 0), we always need a change/drain output.
let has_change_output = self.drain_to.is_some() || !self.drain_lbtc || satoshi_out == 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since has_change_output is only used once below, it makes sense to compute it below, and move all of the comments there

@LeoComandini
Copy link
Contributor

current situation

our flow for sending LBTC (ignoring issuance, reissuance, assets) is

  • add all lbtc
  • add all recipients
  • add temp fee output sats 1
  • add change/drain output
  • temp blind, compute fee rate
  • edit change and fee output amount
  • final blinding

Drain/"send all" methods:

  • drain_lbtc_wallet: select all LBTC utxos
  • drain_lbtc_to: send the remaining (tot in - tot recipients - fee) LBTC to this address (otherwise they are sent to a change address)

Note: at the moment, for the sake of simplicity, we always add all LBTC utxos, thus drain_lbtc_wallet does nothing. However we reserve to change this behavior in the future.
We have this since it was the simplest approach to get things done.
If callers do not want to send all utxos, they can do manual coin selection with set_wallet_utxos.

request

The request is to have these 2 PSETs equivalent (equal modulo blinding randomness):

let pset_send_all = wollet
    .tx_builder()
    .drain_lbtc_wallet()
    .drain_lbtc_to(address)
    .finish()?;
let fee = wollet.get_details(&pset_send_all)?.balance.fee;    

let pset = wollet
    .tx_builder()
    // .drain_lbtc_wallet() // REMOVED WRT THE PR
    .add_lbtc_recipient(&node_address, sats_tot + fee)?
    .finish()?;

In other words, we should not add a change if it's "too low".
But that means changing heavily what we're doing now, making sure all edge case works and are covered by tests, etc.

Ideally we don't do the blinding twice, and instead we estimate how many bytes each blinded output adds, use the amount computed to find the fee to match the fee rate, and decide whether or not we should add the change (as it's done by bitcoin wallets incl. GDK).

So we can't use drain_lbtc_wallet in this way,
although it would solve this specific issue,
and we need deeper changes to get the same behavior wrt to other libs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants