wallet: allow psbtbumpfee to work with txs with external inputs #23202

achow101 · 2021-10-06T02:39:04Z

This PR allows psbtbumpfee to return a PSBT for transactions that contain external inputs. This does not work for bumping in the GUI nor bumpfee because these need private keys available to sign and send the transaction. But psbtbumpfee returns a psbt, so it is fine to not be able to sign.

In order to correctly estimate the size of the inputs for coin selection, the fee bumper will use the size of the inputs of the transaction being bumped. Because the sizes of signatures are not guaranteed, for external inputs, the fee bumper will verify the scripts with a special SignatureChecker which will compute the weight of all of the signatures in that input, and compute their weights if those signatures were maximally sized. This allows the fee bumper to obtain a max size estimate for each external input.

Builds on #23201 as it relies on the ability to pass weights in to coin selection.

Closes #23189

DrahtBot · 2021-10-06T11:19:25Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22793 (Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ghost · 2021-10-14T15:41:44Z

Concept ACK

ghost · 2022-01-26T00:55:42Z

Compiling this branch now as its ready for review. Not sure if I will be able to test everything but this is an interesting feature which should exist in Core. Hoping other devs review this without any club.

t-bast · 2022-01-26T13:15:00Z

I would really love to see this land. I can test this end-to-end with eclair whenever it's ready and verify that it lets us bump lightning transactions (I can add tests with various kinds of inputs, confirmed / unconfirmed / unsafe).

How does it impact @xekyo's work on ancestor aware funding (https://github.com/Xekyo/bitcoin/commits/ancestor-aware-funding)? Ideally the feerate computed here should take into account all unconfirmed ancestors, but maybe Xekyo's work would just land after this PR and fix its feerate computation?

achow101 · 2022-01-26T17:11:19Z

How does it impact @xekyo's work on ancestor aware funding (https://github.com/Xekyo/bitcoin/commits/ancestor-aware-funding)? Ideally the feerate computed here should take into account all unconfirmed ancestors, but maybe Xekyo's work would just land after this PR and fix its feerate computation?

I believe that work will require some mempool changes that have not been implemented yet, so it is probable that it will still be some time before that is ready for review.

t-bast · 2022-01-26T17:24:46Z

Thanks for the quick answer, so I guess @xekyo's work will simply improve psbtbumpfee's feerate estimation when there are unconfirmed ancestors and can be added after this PR is merged.

achow101 · 2022-08-16T21:26:02Z

@t-bast @ishaanam @instagibbs @xekyo Re-review?

t-bast · 2022-08-17T08:49:17Z

There seems to be an issue when I provide the fee_rate parameter, I'm getting the following (seemingly unrelated) error:

bitcoind: wallet/wallet.cpp:1540: bool wallet::FillInputToWeight(CTxIn&, int64_t): Assertion `txin.scriptWitness.IsNull()' failed.

I was calling psbtbumpfee on the following regtest tx:

02000000000102b72c3d62593110e0e340e15363b9d12dbe79af9fb9aa2087ba306436d5a51bca0200000000010000003182d6341b973d2f83b9b07a2075a6501e9753831df26427cbe0de6c601b43000100000000fdffffff02bb08000000000000220020352a6494d2541ba95998c1f2881bce71cb4fe420ebda7cba45f66d88222e4ba02858eb0200000000225120996aa742c86fe13934f855a461a3ac23acfca489369a3b80c8829ba5d122cbc005004730440220309709721d51a931c9984ae02c4f6cefc72d947a61b4d46d39f891f0ff24f92402202fd39201ccfa4e201e5f4c6a72fcb248ac985b21946dc07c2d1ccff3f30e45ed83473044022022fe0c3dd42c641e9cd4756360165597333ef24b1aebbe6f1a7f66e7e2288403022040cffadb335097ab36036d18ee18178520bc92b8bac894c430961242fe3e82880120fa64d24aac82ae7f180cd3fb75391a1be94913130d514b75d3d2001018f14f968d76a91458ea2a44f040121f04384be65d29024be6be21a18763ac672102a1af0b6995a260ad47f9438ae5525701c8bdafd3792b8418376384466f1e55777c8201208763a914a451fab97f394a6bb0e74f22992c227e28e62b2f88527c2103faf880218ed6f87580e99a4760857726a74adf878be803567f7babea4166c0a052ae6775022701b175ac6851b2756801401f5c05b6fc4a49e8b5bf4557753e9d861b62b3509bf82c497698328b519bd1f29360b89c095e22b4e1276d8801fe515ed15f46edd8ab74416b241fefef33927d00000000

Its second input is a wallet input and the second output is the change output, and the amount should allow bumping to the feerate I'm expecting.

achow101 · 2022-08-17T15:38:29Z

There seems to be an issue when I provide the fee_rate parameter, I'm getting the following (seemingly unrelated) error:

Good catch. I've added a fix for that and a test case.

src/wallet/feebumper.cpp

t-bast

ACK bea4313, I tried to bump lightning htlc transactions and everything seems to work fine 👍

instagibbs · 2022-08-19T14:29:22Z

ACK bea4313

furszy

Needs to be rebased, b3d33ef was included in #25679.

Instead of calculating the fee by using what is stored in the wallet, calculate it by looking up the UTXOs.

When bumping the fee of a transaction containing external inputs, determine the weights of those inputs. Because signatures can have a variable size, the script is executed with a special SignatureChecker which will compute the total weight of the signatures in the transaction and the weight if they were all maximum size signatures. This allows us to compute the maximum weight of the input for use during coin selection.

The max size calculation expects some inputs to have empty scriptSigs and witnesses, so we need to clear these before doing that calculation.

In some cases, notably psbtbumpfee, it is okay, and potentially desired, to be able to bump the fee of a transaction which contains external inputs.

achow101 · 2022-08-19T15:27:30Z

Needs to be rebased, b3d33ef was already included in #25679.

Rebased

ishaanam

reACK b1f8fa3

src/wallet/test/feebumper_tests.cpp

test/functional/wallet_bumpfee.py

ishaanam · 2022-08-19T19:03:19Z

reACK c3b099a

t-bast

Re-ran my tests agains c3b099a, ACK

…externa…

fanquake added the Wallet label Oct 6, 2021

This was referenced Oct 6, 2021

Fix intermittent failure in wallet_send.py and rpc_fundrawtransaction.py #23200

Merged

doc: Extract FundTxDoc in rpcwallet #23172

Merged

achow101 force-pushed the bumpfee-ext-inputs branch 3 times, most recently from c2d2948 to 11880ff Compare October 6, 2021 18:19

DrahtBot mentioned this pull request Oct 7, 2021

wallet: Introduce SelectionResult for encapsulating a coin selection solution #22019

Merged

achow101 force-pushed the bumpfee-ext-inputs branch 2 times, most recently from a6f2dc9 to 8dbfdc5 Compare October 8, 2021 17:41

DrahtBot mentioned this pull request Oct 19, 2021

wallet: Allow users to specify input weights when funding a transaction #23201

Merged

DrahtBot mentioned this pull request Oct 27, 2021

Optimize coin selection by dropping BnB upper limit #23367

Closed

DrahtBot mentioned this pull request Nov 18, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

This was referenced Nov 27, 2021

wallet: Split stuff from rpcwallet #23602

Merged

MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet #23647

Closed

DrahtBot mentioned this pull request Dec 6, 2021

Split up rpcwallet #23667

Merged

achow101 force-pushed the bumpfee-ext-inputs branch from 8dbfdc5 to 61ae973 Compare December 8, 2021 15:55

DrahtBot added the Needs rebase label Dec 13, 2021

achow101 force-pushed the bumpfee-ext-inputs branch from 61ae973 to 709e433 Compare December 14, 2021 02:52

DrahtBot removed the Needs rebase label Dec 14, 2021

DrahtBot mentioned this pull request Jan 6, 2022

wallet: add config to prioritize a solution that doesn't create change in coin selection #23475

Closed

DrahtBot added the Needs rebase label Jan 25, 2022

achow101 marked this pull request as ready for review January 25, 2022 18:22

achow101 force-pushed the bumpfee-ext-inputs branch from 709e433 to 1689110 Compare January 25, 2022 18:22

DrahtBot removed the Needs rebase label Jan 25, 2022

achow101 force-pushed the bumpfee-ext-inputs branch from 04970f2 to b14e652 Compare August 17, 2022 15:38

instagibbs reviewed Aug 17, 2022

View reviewed changes

src/wallet/feebumper.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the bumpfee-ext-inputs branch 2 times, most recently from ffd6935 to bea4313 Compare August 17, 2022 21:26

t-bast approved these changes Aug 19, 2022

View reviewed changes

fanquake requested review from ishaanam and instagibbs and removed request for ishaanam August 19, 2022 07:33

furszy reviewed Aug 19, 2022

View reviewed changes

achow101 added 4 commits August 19, 2022 11:27

bumpfee: Calculate fee by looking up UTXOs

612f1e4

Instead of calculating the fee by using what is stored in the wallet, calculate it by looking up the UTXOs.

bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size

31dd3dc

The max size calculation expects some inputs to have empty scriptSigs and witnesses, so we need to clear these before doing that calculation.

bumpfee: be able to bump fee of a tx with external inputs

1bc8106

In some cases, notably psbtbumpfee, it is okay, and potentially desired, to be able to bump the fee of a transaction which contains external inputs.

achow101 force-pushed the bumpfee-ext-inputs branch from bea4313 to b1f8fa3 Compare August 19, 2022 15:27

ishaanam reviewed Aug 19, 2022

View reviewed changes

src/wallet/test/feebumper_tests.cpp Outdated Show resolved Hide resolved

test/functional/wallet_bumpfee.py Outdated Show resolved Hide resolved

achow101 added 3 commits August 19, 2022 14:37

test, bumpfee: Check that psbtbumpfee can bump txs with external inputs

ff63832

Make DUMMY_CHECKER availble outside of script/sign.cpp

116a620

wallet, tests: Test bumpfee's max input weight calculation

c3b099a

achow101 force-pushed the bumpfee-ext-inputs branch from b1f8fa3 to c3b099a Compare August 19, 2022 18:37

t-bast approved these changes Aug 22, 2022

View reviewed changes

fanquake merged commit 607d5a4 into bitcoin:master Aug 22, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 22, 2022

Merge bitcoin#23202: wallet: allow psbtbumpfee to work with txs with …

f8e125b

…externa…

bitcoin locked and limited conversation to collaborators Aug 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: allow psbtbumpfee to work with txs with external inputs #23202

wallet: allow psbtbumpfee to work with txs with external inputs #23202

achow101 commented Oct 6, 2021

DrahtBot commented Oct 6, 2021 •

edited

ghost commented Oct 14, 2021

ghost commented Jan 26, 2022

t-bast commented Jan 26, 2022

achow101 commented Jan 26, 2022

t-bast commented Jan 26, 2022

achow101 commented Aug 16, 2022

t-bast commented Aug 17, 2022

achow101 commented Aug 17, 2022

t-bast left a comment

instagibbs commented Aug 19, 2022

furszy left a comment •

edited

achow101 commented Aug 19, 2022

ishaanam left a comment

ishaanam commented Aug 19, 2022

t-bast left a comment •

edited

wallet: allow psbtbumpfee to work with txs with external inputs #23202

wallet: allow psbtbumpfee to work with txs with external inputs #23202

Conversation

achow101 commented Oct 6, 2021

DrahtBot commented Oct 6, 2021 • edited

Conflicts

ghost commented Oct 14, 2021

ghost commented Jan 26, 2022

t-bast commented Jan 26, 2022

achow101 commented Jan 26, 2022

t-bast commented Jan 26, 2022

achow101 commented Aug 16, 2022

t-bast commented Aug 17, 2022

achow101 commented Aug 17, 2022

t-bast left a comment

Choose a reason for hiding this comment

instagibbs commented Aug 19, 2022

furszy left a comment • edited

Choose a reason for hiding this comment

achow101 commented Aug 19, 2022

ishaanam left a comment

Choose a reason for hiding this comment

ishaanam commented Aug 19, 2022

t-bast left a comment • edited

Choose a reason for hiding this comment

DrahtBot commented Oct 6, 2021 •

edited

furszy left a comment •

edited

t-bast left a comment •

edited