psbt funding: cannot create 1 in, 1 out tx

joostjager commented 3 years ago

Background

I am trying to fund a psbt by specifying one input and one output without change expected, but it doesn't work.

Your environment

lnd master 3608d36ccaca57b9ac4a99cf36672c6edae0a4a0
linux
bitcoind

Steps to reproduce

Create a 1M utxo

joost@joost-desktop $ lncli-alice listunspent
{
"utxos": [
    {
        "address_type": 0,
        "address": "bcrt1q3sx7ycsz5705mjrjcv0ah2q7qcf8l447zatpgc",
        "amount_sat": 1000000,
        "pk_script": "00148c0de26202a79f4dc872c31fdba81e06127fd6be",
        "outpoint": "cb521cafdd9d57357181be99a4d847b0545ddd7969cae76f5fef20141657a59a:1",
        "confirmations": 1
    },
...

Spend utxo to another wallet address: lncli-alice wallet psbt fund --inputs='["cb521cafdd9d57357181be99a4d847b0545ddd7969cae76f5fef20141657a59a:1"]' --outputs='{"bcrt1qucshph05ztxfvfxzpv758m98lpmz6k4uryf7um":998900}' --sat_per_vbyte 10

This transaction is 110 vbytes. So at 10 sat/b, we expect a fee of 1100 sat. This is exactly the difference between input and output value, so no change is expected.

Expected behaviour

Tx is created.

Actual behaviour

[lncli] rpc error: code = Unknown desc = wallet couldn't fund PSBT: could not add change address to database: fee estimation not successful: insufficient funds available to construct transaction

Root cause

I think the problem is here: https://github.com/btcsuite/btcwallet/blob/0230b97a7b519e7ae9e5bfe7c6a6dec307b3c528/wallet/txauthor/author.go#L97

It always assumes a change output and then returns the insufficientFundsError a few lines later.

Roasbeef commented 2 years ago

ziggie1984 commented 2 years ago

is this issue still free to work on ?

Roasbeef commented 2 years ago

@ziggie1984 yes!

ziggie1984 commented 2 years ago

starting my work on it, will report with my first impression on friday the upcoming week !

ziggie1984 commented 2 years ago

thanks @joostjager for the hint, yes the inaccuracy is exactly there, always adding e.g. 31 bytes for p2wkh default change:

My plan how to fix this issue:

change how the fees are estimated in btcwallet/wallet/txauthor/author.go meaning that before trying with a changeoutput I try without it. And if fees are higher than dustlimit changeoutput is added
after each new fetch of Inputs, we test again whether we can create transaction without change.

In addition it would come handy to add a --dry-run handle to the command lncli wallet psbt fund --dry-run, just to try some combination without locking the inputs?

Thanks in advance for your thoughts.

joostjager commented 2 years ago

I briefly looked at the code and I am not sure if the current implementation is optimal, aside from this issue. The call fetchInputs(amount) may make things more complicated than necessary, because it already needs an amount including fee. Fee is independent on the selected inputs, which makes it a circular problem.

Perhaps you can take inspiration from tx generation in LND: https://github.com/lightningnetwork/lnd/blob/master/sweep/tx_input_set.go

The approach here is different. It just adds inputs one by one and with each input added, it evaluates the need for a change output and determines the fee. More of a one way flow.

ziggie1984 commented 1 year ago

Restarting my work on it!

ziggie1984 commented 1 year ago

Releasing the first attempt on friday!

guggero commented 1 year ago

@ziggie1984 what will your fix include? Some changes to coin selection? I'm not sure the problem still persists with the merge of https://github.com/lightningnetwork/lnd/pull/7348 (which should cause the coin selection to properly estimate the fees to omit a change output in the first place), if I understand things correctly.

ziggie1984 commented 1 year ago

Hmm so I tested with latest version, and I can still not create a transaction which pays exactly 1 sat/vbyte because the change is always included in the calculation and I will have to pay for the change although its not gonna be created.

For the p2tr output I will have to add additional 43 sats for the change, so thats the thing I am going to fix.

Example on signet:

{
    "utxos": [
        {
            "address_type": 4,
            "address": "tb1pgtmc0hspuldjcptkzkswghmg30m2rfw6q5xrms0lfzqx956h7mtq57aahf",
            "amount_sat": 19690,
            "pk_script": "512042f787de01e7db2c057615a0e45f688bf6a1a5da050c3dc1ff488062d357f6d6",
            "outpoint": "1a2417f51a9b21e27e073d899058f3c3e3275723968143f11d53632ba2de7230:1",
            "confirmations": 4174
        }
    ]
}

Now I wanna create a Tx which pays everything except 110 sats for the tx when using 1 sat/vbyte

❯ lncli --network signet wallet psbt fund --sat_per_vbyte 1 --outputs='{"tb1pp9840k0frcun9t4eff96sfuwn3e52xdumfdmejylqw8tgpqz6p6smcglvn":19580}'
[lncli] rpc error: code = Unknown desc = wallet couldn't fund PSBT: error creating funding TX: insufficient funds available to construct transaction

10 sats more does also not work because its an issue of the code which always include the change

❯ lncli --network signet wallet psbt fund --sat_per_vbyte 1 --outputs='{"tb1pp9840k0frcun9t4eff96sfuwn3e52xdumfdmejylqw8tgpqz6p6smcglvn":19570}'
[lncli] rpc error: code = Unknown desc = wallet couldn't fund PSBT: error creating funding TX: insufficient funds available to construct transaction

Adding the additional 43 sats for the changeout it works and the tx is created though thats more then we wanna pay

❯ lncli --network signet wallet psbt fund --sat_per_vbyte 1 --outputs='{"tb1pp9840k0frcun9t4eff96sfuwn3e52xdumfdmejylqw8tgpqz6p6smcglvn":19537}'
{
    "psbt": "cHNidP8BAF4CAAAAATBy3qIrY1Md8UOBliNXJ+PD81iQiT0HfuIhmxr1FyQaAQAAAAD/////AVFMAAAAAAAAIlEgCU9X2ekeOTKuuUpLqCeOnHNFGbzaW7zInwOOtAQC0HUAAAAAAAEBK+pMAAAAAAAAIlEgQveH3gHn2ywFdhWg5F9oi/ahpdoFDD3B/0iAYtNX9tYiBgMOQsOeYoP6NaeFUPvUirD5NaTIQV3zySUEaXMoOfWUaxgAAAAAVgAAgAAAAIAAAACAAQAAAAIAAAAhFg5Cw55ig/o1p4VQ+9SKsPk1pMhBXfPJJQRpcyg59ZRrGQAAAAAAVgAAgAAAAIAAAACAAQAAAAIAAAAAAA==",
    "change_output_index": -1,
    "locks": [
        {
            "id": "ede19a92ed321a4705f8a1cccc1d4f6182545d4bb4fae08bd5937831b7e38f98",
            "outpoint": "1a2417f51a9b21e27e073d899058f3c3e3275723968143f11d53632ba2de7230:1",
            "expiration": 1675876936,
            "pk_script": "512042f787de01e7db2c057615a0e45f688bf6a1a5da050c3dc1ff488062d357f6d6",
            "value": 19690
        }
    ]
}

here the tx : https://mempool.space/signet/tx/9c7ff51a29461e29e7fe883230e6d9310c638eff9dfbe2f3c865d710fec9c660

Torakushi commented 1 year ago

I don't think that https://github.com/lightningnetwork/lnd/pull/7348 will solve the problem.

Indeed, the problem is that in NewUnsignedTransaction we always expect a changeSource (will never be nil).

estimatedSize := txsizes.EstimateVirtualSize(
        0, 0, 1, 0, outputs, changeSource.ScriptSize,
    )

So the estimateFees for transaction without change output will always be overestimated by NewUnsignedTransaction and we will not be able to have an "exact amount" as in the example of @joostjager

The only thing https://github.com/lightningnetwork/lnd/pull/7348 will help is to estimate the fees that NewUnsignedTransaction will calculate before calling psbt (because we will know which address type will be used for the change in NewUnsignedTransaction) and then modify the tx amount accordingly (like @ziggie1984 did with his 43 sats).

guggero commented 1 year ago

Ah yes, I see, it's still taking into account the non-existent change output. The new parameter for the change output type probably only changes the 43 sats overpayment to slightly more if you set it to p2tr, since those outputs are bigger.

lightningnetwork / lnd