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

achow101 · 2020-11-30T21:49:23Z

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.

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.

DrahtBot · 2020-11-30T23:11:46Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

decryp2kanon · 2020-12-06T00:50:19Z

Concept ACK

laanwj · 2020-12-17T17:51:22Z

Concept ACK, thanks for adding a test.

instagibbs · 2020-12-24T02:10:38Z

src/wallet/wallet.cpp

@@ -3068,7 +3068,8 @@ bool CWallet::CreateTransactionInternal(
        tx = MakeTransactionRef(std::move(txNew));

        // Limit size
-        if (GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT)
+        if ((sign && GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT) ||
+            (!sign && tx_sizes.second > MAX_STANDARD_TX_WEIGHT))


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

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.

test/functional/rpc_fundrawtransaction.py

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

instagibbs · 2020-12-26T09:51:07Z

Seems strictly better, simple, plus test coverage.

ACK 48a0319

meshcollider

utACK 48a0319

murchandamus

utACK with nits 48a0319

murchandamus · 2021-02-25T23:02:35Z

src/wallet/wallet.cpp

@@ -1594,14 +1594,15 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
    return true;
 }

-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)
+// Returns pair of vsize and weight
+std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, bool use_max_sig)


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?

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.

murchandamus · 2021-02-25T23:17:46Z

src/wallet/wallet.cpp

@@ -1610,13 +1611,16 @@ int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wall
 }

 // txouts needs to be in the order of tx.vin
-int64_t CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)
+std::pair<int64_t, int64_t> CalculateMaximumSignedTxSize(const CTransaction &tx, const CWallet *wallet, const std::vector<CTxOut>& txouts, bool use_max_sig)


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

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

…e funded tx will end up being too large after signing 48a0319 Add a test that selects too large if BnB is used (Andrew Chow) 3e69939 Fail if maximum weight is too large (Andrew Chow) 51e2cd3 Have CalculateMaximumSignedTxSize also compute tx weight (Andrew Chow) Pull request description: 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. ACKs for top commit: instagibbs: ACK bitcoin@48a0319 meshcollider: utACK 48a0319 Xekyo: utACK with nits 48a0319 Tree-SHA512: 1700c60b07f67e2d5c591c5ccd131ac9f1861fab3def961c3c9c4b3281ec1063fe8e4f0f7f1038cac72692340856406bcee8fb45c8104d2ad34357a0ec878ac7

…on_too_large d09120b test: give fundraw more time for test_transaction_too_large (Jon Atack) Pull request description: to hopefully fix timeouts from a new test added in 48a0319 of #20536 merged March 8, 2021 seen locally when running via the test runner ``` File "/home/jon/projects/bitcoin/bitcoin/test/functional/rpc_fundrawtransaction.py", line 927, in test_transaction_too_large raise JSONRPCException({ test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344) ``` and in the CI like https://bitcoinbuilds.org/index.php?ansilog=28537952-2c92-46f2-9871-8918e5ba2738.log#l2398 ``` File "/home/ubuntu/src/test/functional/rpc_fundrawtransaction.py", line 927, in test_transaction_too_large test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 240.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344) ``` Top commit has no ACKs. Tree-SHA512: f11c923439014fe12420f986c640fd301a26282eb41516957d73b9c751087cbae3d0e316f9ccb49bcb424f488540266f70d3f97948633e77c62bd7935df90452

achow101 added 2 commits November 30, 2020 16:38

Have CalculateMaximumSignedTxSize also compute tx weight

51e2cd3

DrahtBot added the Wallet label Nov 30, 2020

This was referenced Dec 1, 2020

Add Single Random Draw as an additional coin selection algorithm #17526

Merged

Use effective values throughout coin selection #17331

Merged

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

instagibbs reviewed Dec 24, 2020

View reviewed changes

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

instagibbs reviewed Dec 24, 2020

View reviewed changes

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

Add a test that selects too large if BnB is used

48a0319

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

achow101 force-pushed the fundtx-error-large-tx branch from 3201572 to 48a0319 Compare December 24, 2020 17:40

meshcollider reviewed Jan 20, 2021

View reviewed changes

This was referenced Feb 17, 2021

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207

Merged

refactor: Make CWalletTx sync state type-safe #21206

Merged

murchandamus reviewed Feb 25, 2021

View reviewed changes

DrahtBot mentioned this pull request Mar 4, 2021

rpc: include_unsafe option for fundrawtransaction #21359

Merged

meshcollider merged commit a8b0892 into bitcoin:master Mar 8, 2021

sidhujag mentioned this pull request Mar 8, 2021

TX size warning syscoin/syscoin#419

Closed

This was referenced Mar 10, 2021

test: increase rpc_timeout for fundrawtx test_transaction_too_large #21410

Merged

Timeouts in a test case in rpc_fundrawtransaction #21452

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

achow101 commented Nov 30, 2020

DrahtBot commented Nov 30, 2020 •

edited

decryp2kanon commented Dec 6, 2020

laanwj commented Dec 17, 2020

instagibbs Dec 24, 2020

achow101 Dec 24, 2020

instagibbs commented Dec 26, 2020

meshcollider left a comment

murchandamus left a comment

murchandamus Feb 25, 2021

achow101 Mar 4, 2021

murchandamus Feb 25, 2021

achow101 Mar 4, 2021

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

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

Conversation

achow101 commented Nov 30, 2020

DrahtBot commented Nov 30, 2020 • edited

Conflicts

decryp2kanon commented Dec 6, 2020

laanwj commented Dec 17, 2020

instagibbs Dec 24, 2020

Choose a reason for hiding this comment

achow101 Dec 24, 2020

Choose a reason for hiding this comment

instagibbs commented Dec 26, 2020

meshcollider left a comment

Choose a reason for hiding this comment

murchandamus left a comment

Choose a reason for hiding this comment

murchandamus Feb 25, 2021

Choose a reason for hiding this comment

achow101 Mar 4, 2021

Choose a reason for hiding this comment

murchandamus Feb 25, 2021

Choose a reason for hiding this comment

achow101 Mar 4, 2021

Choose a reason for hiding this comment

DrahtBot commented Nov 30, 2020 •

edited