wallet: Error with "Transaction too large" if the funded tx will end up being too large after signing

achow101 commented at 9:49 pm on November 30, 2020: member

Currently the Transaction too large is calculated on the transaction that is returned from CreateTransaction. This does not make sense for when CreateTransaction is being used for fundrawtransaction as no signing occurs so the final returned transaction is missing signatures. Thus users may successfully fund a transaction but fail to broadcast it after it has been fully signed.

So instead we should figure out whether the transaction we are funding will be too large after it is signed. We can do this by having CalculateMaximumSignedTxSize also return the transaction weight and then comparing that weight against the maximum weight.

Have CalculateMaximumSignedTxSize also compute tx weight 51e2cd322c

Fail if maximum weight is too large

Our max weight check in CreateTransaction only worked if the transaction
was fully signed. However if we are funding a transaction, it is
possible that the tx weight will be too large for a standard tx. In that
case, we should also fail. So we use the tx weight returned by
CalculateMaximumSignedTxSize and check against the limit for those
transactions.

3e69939b78

DrahtBot commented at 11:11 pm on November 30, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21359 (rpc: include_unsafe option for fundrawtransaction by t-bast)
#21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
#17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
#17331 (Use effective values throughout coin selection by achow101)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs 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.

DrahtBot added the label Wallet on Nov 30, 2020

decryp2kanon commented at 0:50 am on December 6, 2020: contributor

Concept ACK

laanwj commented at 5:51 pm on December 17, 2020: member

Concept ACK, thanks for adding a test.

in src/wallet/wallet.cpp:3072 in 3e69939b78 outdated

3067@@ -3068,7 +3068,8 @@ bool CWallet::CreateTransactionInternal(
3068         tx = MakeTransactionRef(std::move(txNew));
3069 
3070         // Limit size
3071-        if (GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT)
3072+        if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) ||
3073+            (!sign && tx_sizes.second > MAX_STANDARD_TX_WEIGHT))

instagibbs commented at 2:10 am on December 24, 2020:

do we need both the checks because this could end up being -1?

achow101 commented at 5:37 pm on December 24, 2020:

I have both checks here because the estimate is an overestimate, so I would prefer to use the accurate weight when possible, i.e. when the transaction is signed.

in test/functional/rpc_fundrawtransaction.py:929 in 32015724c5 outdated

924+            outputs[recipient.getnewaddress()] = 0.1
925+        wallet.sendmany("", outputs)
926+        self.nodes[0].generate(10)
927+
928+        # Currently, the only solution is to select most of the coins just made, however this will result in a transaction that is too large
929+        assert_raises_rpc_error(-4, "Transaction too large", recipient.fundrawtransaction ,rawtx)

instagibbs commented at 2:13 am on December 24, 2020:

spacing nit: recipient.fundrawtransaction ,rawtx

achow101 commented at 5:40 pm on December 24, 2020:

Fixed.

Add a test that selects too large if BnB is used

If BnB is used, the test will fail because the transaction is too large.

48a0319bab

in test/functional/rpc_fundrawtransaction.py:928 in 32015724c5 outdated

923+        for i in range(0, 1500):
924+            outputs[recipient.getnewaddress()] = 0.1
925+        wallet.sendmany("", outputs)
926+        self.nodes[0].generate(10)
927+
928+        # Currently, the only solution is to select most of the coins just made, however this will result in a transaction that is too large

instagibbs commented at 2:14 am on December 24, 2020:

I find it confusing the test claims it will have a successful knapsack run then doesn’t?

achow101 commented at 5:40 pm on December 24, 2020:

The fallback to knapsack behavior hasn’t been implemented yet, so the only thing that should happen is we get an error. Originally this was part of a branch that did that fallback, but I removed it because I didn’t like how it was implemented. I’ve updated the comment to reflect this.

achow101 force-pushed on Dec 24, 2020

instagibbs commented at 9:51 am on December 26, 2020: member

Seems strictly better, simple, plus test coverage.

ACK https://github.com/bitcoin/bitcoin/pull/20536/commits/48a0319babb409cf486a9eb7c776810f70b06cb2

meshcollider commented at 4:00 am on January 20, 2021: contributor

utACK 48a0319babb409cf486a9eb7c776810f70b06cb2

in src/wallet/wallet.cpp:1598 in 51e2cd322c outdated

1593@@ -1594,14 +1594,15 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1594     return true;
1595 }
1596 
1597-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)
1598+// Returns pair of vsize and weight
1599+std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)

Xekyo commented at 11:02 pm on February 25, 2021:

I have noticed that this codebase often uses unnamed fields in a specific order rather than named fields. Wouldn’t it be advantageous to define transfer objects that have named fields to be more explicit in what is retrieved from response objects?

achow101 commented at 6:19 pm on March 4, 2021:

For something like this, I feel like it is overkill to introduce a struct or class for these, especially since it is used in only one place.

in src/wallet/wallet.cpp:1614 in 51e2cd322c outdated

1610@@ -1610,13 +1611,16 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
1611 }
1612 
1613 // txouts needs to be in the order of tx.vin
1614-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)
1615+std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)

Xekyo commented at 11:17 pm on February 25, 2021:

I’d find it more intuitive if this were two separate methods to get vsize and weight rather than the unnamed pair of values.

achow101 commented at 6:20 pm on March 4, 2021:

I did these as a single function because it’s the same calculation.

Xekyo commented at 11:23 pm on February 25, 2021: member

utACK with nits 48a0319babb409cf486a9eb7c776810f70b06cb2

meshcollider merged this on Mar 8, 2021

meshcollider closed this on Mar 8, 2021

sidhujag referenced this in commit a0a663ddcb on Mar 8, 2021

MarcoFalke referenced this in commit 5ef16038a1 on Mar 16, 2021

DrahtBot locked this on Aug 16, 2022

wallet: Error with “Transaction too large” if the funded tx will end up being too large after signing #20536

Conflicts