wallet: notify when preset + automatic inputs exceed max weight #30309

furszy · 2024-06-19T17:34:46Z

Found it while finishing my review on #29523. This does not interfere with it.

Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight, but we are not doing so when the user-preselected inputs combined with the wallet-selected inputs exceed the maximum weight.
This change avoids signing all inputs before erroring out and introduces test coverage for fundrawtransaction.

DrahtBot · 2024-06-19T17:34:48Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, rkrux, ismaelsadeeq, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

tdb3

ACK 3f50276
Helpful addition to help cover edge cases.
Anything that should be added to unit tests?

tdb3 · 2024-06-20T22:38:31Z

test/functional/wallet_fundrawtransaction.py

+
+        # 3) Pre-select some inputs and let the wallet fill-up the remaining amount
+        inputs = input_weights[0:1000]
+        assert_raises_rpc_error(-4, "The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs",


Tested that this would fail if the return util::Error... line was commented out in source/wallet/spend.cpp. It did (as expected).

Re-ran this test (comment out return util::Error...) for 72b2268. Failed as expected.

furszy · 2024-06-21T15:19:33Z

Anything that should be added to unit tests?

The behavior change is tested on a functional test. No need to duplicate the same test twice.
Functional tests are portable across releases due to our API stability requirement. Unit tests are harder to maintain due to internal code changes. Thus when possible, functional tests are preferred.

rkrux

tACK 3f50276

make, make check, test/functional are successful.

Thanks @furszy for this improvement, and keeping the PR short and to the point, easy to review! Asked few questions for my understanding.

src/wallet/spend.cpp

test/functional/wallet_fundrawtransaction.py

src/wallet/spend.cpp

ismaelsadeeq

Approach ACK

This also avoids signing all inputs prior to erroring out.

tdb3

re ACK for 72b2268
Re-ran tests locally (exercising induced failures and received failures as expected).
test_weight_limits() implementations are almost identical between wallet_fundrawtransaction and wallet_spend. While this feels a bit redundant, these are separate functional tests so an alternative (like combining) wouldn't really make sense to do.

tdb3 · 2024-06-22T01:03:52Z

test/functional/wallet_fundrawtransaction.py

+
+        # 3) Pre-select some inputs and let the wallet fill-up the remaining amount
+        inputs = input_weights[0:1000]
+        assert_raises_rpc_error(-4, "The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs",


Re-ran this test (comment out return util::Error...) for 72b2268. Failed as expected.

tdb3 · 2024-06-22T01:05:47Z

test/functional/wallet_send.py

+
+        # 3) Pre-select some inputs and let the wallet fill-up the remaining amount
+        inputs = inputs[0:1000]
+        assert_raises_rpc_error(-4, "The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs",


Saw that the same expected failure occurs here when commenting out return util::Error... for 72b2268.

rkrux

tACK 72b2268

Reran make and all functional tests. Thanks @furszy for addressing my comments quickly!

Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight

I ended up going through the coin selection algorithms and found all of them honouring the max weight criteria in case of automatic coin selection in coinselection:

test/functional/wallet_fundrawtransaction.py

test/functional/wallet_send.py

rkrux · 2024-06-24T10:20:50Z

tACK 72b2268

Reran make and all functional tests. Thanks @furszy for addressing my comments quickly!

Basically, we are erroring out early when the automatic coin selection process exceeds the maximum weight

I ended up going through the coin selection algorithms and found all of them honouring the max weight criteria in case of automatic coin selection in coinselection:

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coinselection.cpp#L190

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coinselection.cpp#L325

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coinselection.cpp#L538

https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coinselection.cpp#L651

Oh interesting, as per this commit: 79f3aff, this max_weight means the maximum allowed weight of the selected UTXOs in the transaction, not of the whole transaction, which I now believe transitively translates to the whole transaction weight criteria as well.

ismaelsadeeq

utACK 72b2268

achow101 · 2024-06-26T16:01:44Z

ACK 72b2268

maflcko · 2024-06-27T18:26:30Z

test/functional/wallet_fundrawtransaction.py

+        input_weights = []
+        for i in range(1471):
+            input_weights.append({"txid": txid, "vout": i, "weight": 273})
+        assert_raises_rpc_error(-4, "Transaction too large", wallet.fundrawtransaction, hexstring=rawtx, input_weights=input_weights)


https://cirrus-ci.com/task/5319226913718272?logs=ci#L1930

test 2024-06-26T19:28:29.157000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 157, in try_rpc fun(*args, **kwds) File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 146, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs (-4) During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main self.run_test() File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 117, in run_test self.test_weight_limits() File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 1335, in test_weight_limits assert_raises_rpc_error(-4, "Transaction too large", wallet.fundrawtransaction, hexstring=rawtx, input_weights=input_weights) File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 148, in assert_raises_rpc_error assert try_rpc(code, message, fun, *args, **kwds), "No exception raised" File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 163, in try_rpc raise AssertionError( AssertionError: Expected substring not found in error message: substring: 'Transaction too large' error message: 'The combination of the pre-selected inputs and the wallet automatic inputs selection exceeds the transaction maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs'.

…mits test 00b8e26 test: fix inconsistency in fundrawtransaction weight limits test (furszy) Pull request description: Fix #30309 (comment) inconsistency. Currently, the test is passing due to a mistake in the test inputs selection process. We are selecting the parent transaction change output as one of the inputs of the transaction to fund, which helps to surpass the target amount when it shouldn't due to the fee reduction. The failure arises when the test behaves as intended by its coder; that is, when it does not select the change output. In this case, the pre-selected inputs aren't enough to cover the target amount. Fix this by excluding the parent transaction's change output from the inputs selection and including an extra input to cover the tx fee. The CI failure can be replicated with the following patch in master: ```diff diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py --- a/test/functional/wallet_fundrawtransaction.py(revision 9b480f7) +++ b/test/functional/wallet_fundrawtransaction.py(date 1720652934739) @@ -1322,7 +1322,7 @@ outputs = [] for _ in range(1472): outputs.append({wallet.getnewaddress(address_type="legacy"): 0.1}) - txid = self.nodes[0].send(outputs=outputs)["txid"] + txid = self.nodes[0].send(outputs=outputs, change_position=0)["txid"] self.generate(self.nodes[0], 1) # 272 WU per input (273 when high-s); picking 1471 inputs will exceed the max standard tx weight. @@ -1330,7 +1330,7 @@ # 1) Try to fund transaction only using the preset inputs input_weights = [] - for i in range(1471): + for i in range(1, 1472): # skip first output as it is the parent tx change output input_weights.append({"txid": txid, "vout": i, "weight": 273}) assert_raises_rpc_error(-4, "Transaction too large", wallet.fundrawtransaction, hexstring=rawtx, input_weights=input_weights) ``` ACKs for top commit: achow101: ACK 00b8e26 ismaelsadeeq: Code review and Tested ACK 00b8e26 Tree-SHA512: 5ef792961b7fad4999fc30aa03366432103ddf672ca5cbb366c9eab4c2e46d5ae1ab0c073dfc4fbb2b4e63203653bc0e54463c731c5f8655140207ba5f8e542e

DrahtBot added the Wallet label Jun 19, 2024

tdb3 approved these changes Jun 20, 2024

View reviewed changes

rkrux approved these changes Jun 21, 2024

View reviewed changes

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

test/functional/wallet_fundrawtransaction.py Show resolved Hide resolved

src/wallet/spend.cpp Show resolved Hide resolved

ismaelsadeeq reviewed Jun 21, 2024

View reviewed changes

wallet: notify when preset + automatic inputs exceed max weight

72b2268

This also avoids signing all inputs prior to erroring out.

furszy force-pushed the 2024_wallet_max_weight branch from 3f50276 to 72b2268 Compare June 21, 2024 21:15

tdb3 approved these changes Jun 22, 2024

View reviewed changes

DrahtBot requested review from ismaelsadeeq and rkrux June 22, 2024 01:16

rkrux approved these changes Jun 24, 2024

View reviewed changes

test/functional/wallet_fundrawtransaction.py Show resolved Hide resolved

test/functional/wallet_send.py Show resolved Hide resolved

ismaelsadeeq reviewed Jun 25, 2024

View reviewed changes

achow101 merged commit 1d00601 into bitcoin:master Jun 26, 2024
16 checks passed

furszy deleted the 2024_wallet_max_weight branch June 26, 2024 18:47

ismaelsadeeq mentioned this pull request Jun 27, 2024

Wallet: Add max_tx_weight to transaction funding options (take 2) #29523

Merged

maflcko reviewed Jun 27, 2024

View reviewed changes

furszy mentioned this pull request Jun 27, 2024

test: fix inconsistency in fundrawtransaction weight limits test #30353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: notify when preset + automatic inputs exceed max weight #30309

wallet: notify when preset + automatic inputs exceed max weight #30309

furszy commented Jun 19, 2024 •

edited

Loading

DrahtBot commented Jun 19, 2024 •

edited

Loading

tdb3 left a comment

tdb3 Jun 20, 2024

tdb3 Jun 22, 2024

furszy commented Jun 21, 2024 •

edited

Loading

rkrux left a comment

ismaelsadeeq left a comment

tdb3 left a comment

tdb3 Jun 22, 2024

tdb3 Jun 22, 2024

rkrux left a comment

rkrux commented Jun 24, 2024

ismaelsadeeq left a comment

achow101 commented Jun 26, 2024

maflcko Jun 27, 2024

wallet: notify when preset + automatic inputs exceed max weight #30309

wallet: notify when preset + automatic inputs exceed max weight #30309

Conversation

furszy commented Jun 19, 2024 • edited Loading

DrahtBot commented Jun 19, 2024 • edited Loading

Code Coverage

Reviews

tdb3 left a comment

Choose a reason for hiding this comment

tdb3 Jun 20, 2024

Choose a reason for hiding this comment

tdb3 Jun 22, 2024

Choose a reason for hiding this comment

furszy commented Jun 21, 2024 • edited Loading

rkrux left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

tdb3 left a comment

Choose a reason for hiding this comment

tdb3 Jun 22, 2024

Choose a reason for hiding this comment

tdb3 Jun 22, 2024

Choose a reason for hiding this comment

rkrux left a comment

Choose a reason for hiding this comment

rkrux commented Jun 24, 2024

ismaelsadeeq left a comment

Choose a reason for hiding this comment

achow101 commented Jun 26, 2024

maflcko Jun 27, 2024

Choose a reason for hiding this comment

furszy commented Jun 19, 2024 •

edited

Loading

DrahtBot commented Jun 19, 2024 •

edited

Loading

furszy commented Jun 21, 2024 •

edited

Loading