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

achow101 · 2021-10-05T23:15:15Z

When funding a transaction with external inputs, instead of providing solving data, a user may want to just provide the maximum signed size of that input. This is particularly useful in cases where the input is nonstandard as our dummy signer is unable to handle those inputs.

The input weight can be provided to any input regardless of whether it belongs to the wallet and the provided weight will always be used regardless of any calculated input weight. This allows the user to override the calculated input weight which may overestimate in some circumstances due to missing information (e.g. if the private key is not known, a maximum size signature will be used, but the actual signer may be doing additional work which reduces the size of the signature).

For send and walletcreatefundedpsbt, the input weight is specified in a weight field in an input object. For fundrawtransaction, a new input_weights field is added to the options object. This is an array of objects consisting of a txid, vout, and weight.

Closes #23187

achow101 · 2021-10-05T23:22:16Z

Note that bumpfee does not take this into account yet, but it's next on my list.

meshcollider · 2021-10-05T23:23:46Z

Concept ACK

fanquake · 2021-10-06T00:26:25Z

cc @t-bast maybe @apoelstra

t-bast · 2021-10-06T06:46:25Z

Concept ACK, I'll test this today with lightning transactions.
Should this PR update the release notes?

src/wallet/rpcwallet.cpp

t-bast · 2021-10-06T09:14:01Z

I tested 5c2ff25 with lightning transactions, and I'm correctly able to specify input weights for non-standard inputs. Thanks @achow101!

DrahtBot · 2021-10-06T11:23:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23545 (scripted-diff: Use clang-tidy syntax for C++ named arguments by MarcoFalke)
#23475 (wallet: add config to prioritize a solution that doesn't create change in coin selection by brunoerg)
#23202 (wallet: allow psbtbumpfee to work with txs with external inputs by achow101)
#19602 (wallet: Migrate legacy wallets to descriptor wallets 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.

src/wallet/rpcwallet.cpp

test/functional/rpc_fundrawtransaction.py

yanmaani

utACK d059229. Here's some nits.

src/wallet/coincontrol.h

src/wallet/rpcwallet.cpp

src/wallet/spend.cpp

test/functional/rpc_fundrawtransaction.py

src/wallet/rpcwallet.cpp

rajarshimaitra

Initial Concept ACK, will do a more detail pass later.

Below are some minor nits (ignorable) and questions I had.

rajarshimaitra · 2021-10-09T12:14:12Z

src/wallet/wallet.cpp

+    // a boundary, the size will be split so that 2/3rds will be in one stack element, and
+    // the remaining 1/3rd in another. Using 3rds allows us to avoid additional boundaries.
+    // 10 bytes is used because that accounts for the maximum size. This does not need to be super precise.
+    if ((add_weight >= 253 && add_weight < 263)


Assuming this numbers are denoting 8 bit boundaries, why its not 255 and 265?

I was confused about this as well since I had never come across compact size uints before. It helped me to read serialize.h and about tx serialization (bip141 and this doc).

This is just saying that when the stack size goes from 252 to 253, you need more bytes to encode the size in the serialized transaction. It's not the same as a byte boundary since (I assume) you need to reserve a few values for codifying how many bytes it is.

test/functional/rpc_fundrawtransaction.py

src/wallet/coincontrol.h

rajarshimaitra

tACK 52d8d44

Verified all tests are covering expected behavior.

Below are few non blocking nits and comments.

src/wallet/rpc/spend.cpp

src/wallet/wallet.h

test/functional/rpc_fundrawtransaction.py

rajarshimaitra · 2022-01-17T10:44:49Z

test/functional/rpc_psbt.py

        self.generate(self.nodes[0], 6)
        ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0]

        # An external input without solving data should result in an error
-        assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[1].walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 10 + ext_utxo['amount']}, 0, {'add_inputs': True})
+        assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 15})


Same regarding "Insufficient funds" error here.

test/functional/wallet_send.py

maflcko · 2022-01-17T11:34:41Z

Is this for 0.23?

test/functional/rpc_fundrawtransaction.py

src/wallet/rpc/spend.cpp

src/wallet/coincontrol.h

glozow

Is this for 0.23?

I hope so!

I've reviewed up until 8a1ea6a. I have some concerns about the argument sanitization, but otherwise looks good to me. This also needs release notes, yes?

glozow · 2022-01-17T15:10:10Z

src/wallet/spend.cpp

+        // If available, override calculated size with coin control specified size
+        if (coin_control.HasInputWeight(outpoint)) {
+            input_bytes = GetVirtualTransactionSize(coin_control.GetInputWeight(outpoint), 0, 0);
+        }


I don't think that's right. If !coin_control.GetExternalOutput, then we can't do coin selection at all and would have returned at an earlier point.

Reviewed code and I agree with this. If it's an external input, we must have not found it in mapWallet. If we also didn't find it in GetExternalOutput(), then we already returned nullopt on line 461.

src/wallet/wallet.cpp

glozow · 2022-01-17T16:01:37Z

src/wallet/wallet.cpp

+    // a boundary, the size will be split so that 2/3rds will be in one stack element, and
+    // the remaining 1/3rd in another. Using 3rds allows us to avoid additional boundaries.
+    // 10 bytes is used because that accounts for the maximum size. This does not need to be super precise.
+    if ((add_weight >= 253 && add_weight < 263)


I was confused about this as well since I had never come across compact size uints before. It helped me to read serialize.h and about tx serialization (bip141 and this doc).

This is just saying that when the stack size goes from 252 to 253, you need more bytes to encode the size in the serialized transaction. It's not the same as a byte boundary since (I assume) you need to reserve a few values for codifying how many bytes it is.

src/wallet/rpc/spend.cpp

src/wallet/wallet.cpp

glozow

reviewed the tests

test/functional/rpc_fundrawtransaction.py

test/functional/wallet_send.py

glozow

ACK db4efde

I'm happy with FillInputToWeight() now, thanks for addressing comments. The unit tests seem comprehensive. I tried every input from -1 to 65536 and no crashes.
Minimum input of 165 = 4 * (32 + 4 + 4 + 1) + 1 makes more sense to me than 160.
Test coverage for walletcreatefundedpsbt, fundrawtransaction, and send look good to me, error messages and resulting feerates are tested and the weights used are correct now.

instagibbs

ACK db4efde

src/wallet/wallet.cpp

src/wallet/rpc/spend.cpp

In order to allow coin selection to take weights from the user, CCoinControl needs to be able to set and get them.

If the user specifies an input size, allow it to override any input size calculations during coin selection.

Coin selection requires knowing the weight of a transaction so that fees can be estimated. However for external inputs, the weight may not be avialble, and solving data may not be enough as the input could be one that we do not support. By allowing users to specify input weights, those external inputs can be included in the transaction. Additionally, if the weight for an input is specified, that value will always be used, regardless of whether the input is in the wallet or solving data is available. This allows us to account for scenarios where the wallet may be more conservative and estimate a larger input than may actually be created. For example, we assume the maximum DER signature size, but an external input may be signed by a wallet which does nonce grinding in order to get a smaller signature. In that case, the user can specify the smaller input weight to avoid overpaying transaction fees.

Added tests to rpc_fundrawtransaction, wallet_send, and rpc_psbt that test that external inputs can be spent when input weight is provided. Also tested that the input weight overrides any calculated weight. Additionally, rpc_psbt's external inputs test is cleaned up a bit to be more similar to rpc_fundrawtransaction's and avoid potential pitfalls due to non-deterministic coin selection behavior.

instagibbs · 2022-01-24T23:40:04Z

reACK 3866272

fanquake · 2022-01-25T02:50:13Z

@t-bast want to take another look here?

t-bast

ACK 3866272

I re-run tests from a lightning node: I'm correctly able to use this new parameter to make bitcoind fund then sign transactions that contain inputs that aren't managed by the bitcoind wallet (in my case, HTLC outputs from a lightning commitment transaction).

A big thanks to everyone who contributed to this PR!

glozow · 2022-01-25T10:11:00Z

reACK 3866272 via range-diff

Changes since last review:

Changed FillInputToWeight() to return a bool for information about whether filling happened or if the input was invalid, changed DummySigns and tests to reflect that.
assertions in FillInputToWeight() that scriptSig and witness start out empty
changed the 165 input minimum to be calculated using size of empty input instead of arithmetic, added a CHECK_NONFATAL that it matches 165

…n fundi…

…l inputs c3b099a wallet, tests: Test bumpfee's max input weight calculation (Andrew Chow) 116a620 Make DUMMY_CHECKER availble outside of script/sign.cpp (Andrew Chow) ff63832 test, bumpfee: Check that psbtbumpfee can bump txs with external inputs (Andrew Chow) 1bc8106 bumpfee: be able to bump fee of a tx with external inputs (Andrew Chow) 31dd3dc bumpfee: Clear scriptSigs and scriptWitnesses before calculated max size (Andrew Chow) a0c3afb bumpfee: extract weights of external inputs when bumping fee (Andrew Chow) 612f1e4 bumpfee: Calculate fee by looking up UTXOs (Andrew Chow) Pull request description: 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 ACKs for top commit: ishaanam: reACK c3b099a t-bast: Re-ran my tests agains c3b099a, ACK Tree-SHA512: 40016ec52d351430977579cfa2694c7e6764f42c9ce09d3a6f1753b767f86053f296d9de988248df033be6d725d67badbf2a5ef82c8ace23c61487729b7691e5

achow101 force-pushed the ext-input-weight branch 2 times, most recently from dc756fd to fd6fde8 Compare October 5, 2021 23:16

achow101 force-pushed the ext-input-weight branch from fd6fde8 to 5c2ff25 Compare October 5, 2021 23:24

DrahtBot added RPC/REST/ZMQ Wallet labels Oct 5, 2021

fanquake requested a review from instagibbs October 6, 2021 00:26

achow101 mentioned this pull request Oct 6, 2021

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

Merged

maflcko reviewed Oct 6, 2021

View reviewed changes

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

DrahtBot mentioned this pull request Oct 6, 2021

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

Merged

DrahtBot added the Needs rebase label Oct 6, 2021

DrahtBot mentioned this pull request Oct 6, 2021

doc: Extract FundTxDoc in rpcwallet #23172

Merged

achow101 force-pushed the ext-input-weight branch 2 times, most recently from 0bada4a to 675309b Compare October 6, 2021 15:32

DrahtBot removed the Needs rebase label Oct 6, 2021

achow101 force-pushed the ext-input-weight branch 2 times, most recently from f37d4e6 to 7121427 Compare October 6, 2021 17:25

instagibbs reviewed Oct 6, 2021

View reviewed changes

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

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

achow101 force-pushed the ext-input-weight branch from 7121427 to a19169a Compare October 6, 2021 23:33

DrahtBot mentioned this pull request Oct 7, 2021

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

Merged

DrahtBot added the Needs rebase label Oct 7, 2021

achow101 force-pushed the ext-input-weight branch from a19169a to d059229 Compare October 7, 2021 20:34

DrahtBot removed the Needs rebase label Oct 7, 2021

yanmaani reviewed Oct 8, 2021

View reviewed changes

maflcko reviewed Oct 8, 2021

View reviewed changes

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

achow101 force-pushed the ext-input-weight branch from 2996f66 to 52d8d44 Compare January 14, 2022 20:09

DrahtBot mentioned this pull request Jan 15, 2022

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

rajarshimaitra reviewed Jan 15, 2022

View reviewed changes

rajarshimaitra reviewed Jan 17, 2022

View reviewed changes

glozow reviewed Jan 17, 2022

View reviewed changes

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

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/coincontrol.h Show resolved Hide resolved

glozow reviewed Jan 17, 2022

View reviewed changes

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

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

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

achow101 force-pushed the ext-input-weight branch 3 times, most recently from a995803 to db4efde Compare January 17, 2022 18:09

glozow reviewed Jan 18, 2022

View reviewed changes

instagibbs approved these changes Jan 20, 2022

View reviewed changes

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

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

achow101 added 4 commits January 24, 2022 11:23

wallet: add input weights to CCoinControl

4060c50

In order to allow coin selection to take weights from the user, CCoinControl needs to be able to set and get them.

wallet: Allow user specified input size to override

808068e

If the user specifies an input size, allow it to override any input size calculations during coin selection.

achow101 force-pushed the ext-input-weight branch from db4efde to 3866272 Compare January 24, 2022 16:29

fanquake requested a review from glozow January 25, 2022 02:49

t-bast approved these changes Jan 25, 2022

View reviewed changes

glozow approved these changes Jan 25, 2022

View reviewed changes

laanwj merged commit b94d0c7 into bitcoin:master Jan 25, 2022

t-bast mentioned this pull request Jan 27, 2022

Add support for funding txs with non-wallet inputs ACINQ/eclair#2151

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022

Merge bitcoin#23201: wallet: Allow users to specify input weights whe…

4d48ec3

…n fundi…

t-bast mentioned this pull request Feb 28, 2022

Question: PSBT and RBF #19481

Closed

bitcoin locked and limited conversation to collaborators Jan 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented Oct 5, 2021

achow101 commented Oct 5, 2021

meshcollider commented Oct 5, 2021

fanquake commented Oct 6, 2021

t-bast commented Oct 6, 2021

t-bast commented Oct 6, 2021

DrahtBot commented Oct 6, 2021 •

edited

Loading

yanmaani left a comment

rajarshimaitra left a comment

rajarshimaitra Oct 9, 2021 •

edited

Loading

glozow Jan 17, 2022

rajarshimaitra left a comment

rajarshimaitra Jan 17, 2022

maflcko commented Jan 17, 2022

glozow left a comment

glozow Jan 17, 2022

glozow Jan 17, 2022

glozow left a comment

glozow left a comment

instagibbs left a comment

instagibbs commented Jan 24, 2022

fanquake commented Jan 25, 2022

t-bast left a comment

glozow commented Jan 25, 2022

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

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

Conversation

achow101 commented Oct 5, 2021

achow101 commented Oct 5, 2021

meshcollider commented Oct 5, 2021

fanquake commented Oct 6, 2021

t-bast commented Oct 6, 2021

t-bast commented Oct 6, 2021

DrahtBot commented Oct 6, 2021 • edited Loading

Conflicts

yanmaani left a comment

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Oct 9, 2021 • edited Loading

Choose a reason for hiding this comment

glozow Jan 17, 2022

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

rajarshimaitra Jan 17, 2022

Choose a reason for hiding this comment

maflcko commented Jan 17, 2022

glozow left a comment

Choose a reason for hiding this comment

glozow Jan 17, 2022

Choose a reason for hiding this comment

glozow Jan 17, 2022

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

instagibbs left a comment

Choose a reason for hiding this comment

instagibbs commented Jan 24, 2022

fanquake commented Jan 25, 2022

t-bast left a comment

Choose a reason for hiding this comment

glozow commented Jan 25, 2022

DrahtBot commented Oct 6, 2021 •

edited

Loading

rajarshimaitra Oct 9, 2021 •

edited

Loading