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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
1338 | + assert_raises_rpc_error(-4, "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs", 1339 | + wallet.fundrawtransaction, hexstring=rawtx) 1340 | + 1341 | + # 3) Pre-select some inputs and let the wallet fill-up the remaining amount 1342 | + inputs = input_weights[0:1000] 1343 | + 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 72b226882fe2348a9a66aee1d8d21b4e2d275e68. Failed as expected.
ACK 3f5027626593c7af546d3b53fc658c87bb815348 Helpful addition to help cover edge cases. Anything that should be added to unit tests?
798 | @@ -799,6 +799,13 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av 799 | op_selection_result->RecalculateWaste(coin_selection_params.min_viable_change, 800 | coin_selection_params.m_cost_of_change, 801 | coin_selection_params.m_change_fee); 802 | + 803 | + // Verify we haven't exceeded the maximum allowed weight 804 | + int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); 805 | + if (op_selection_result->GetWeight() >= max_inputs_weight) {
The comment above MAX_STANDARD_WEIGHT says it's the maximum weight we are willing to mine: https://github.com/bitcoin/bitcoin/blob/master/src/policy/policy.h#L27
Should the condition here not be strictly greater than?
yes, thx
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.
1323 | + for _ in range(1472): 1324 | + outputs.append({wallet.getnewaddress(address_type="legacy"): 0.1}) 1325 | + txid = self.nodes[0].send(outputs=outputs)["txid"] 1326 | + self.generate(self.nodes[0], 1) 1327 | + 1328 | + # 272 WU per input (273 when high-s); picking 1471 inputs will exceed the max standard tx weight.
From (273 when high-s), I assume this considers a signed input but the docs of createrawtransaction and fundrawtransaction say that these commands don't generate a signed transaction. So this max weight check on picking 1472 inputs of 273 WU each internally seems to be dependent only on the weight parameter passed to fundrawtransaction RPC, which is dependent on the user input.
Is there not any other internal worst-case scenario treatment of the inputs weights that might not be dependent on user input?
yeah, good eye. send() and walletprocesspsbt() will also fail. Will add coverage for one of them.
Thanks for adding a functional test for send() as well.
798 | @@ -799,6 +799,13 @@ util::Result<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& av 799 | op_selection_result->RecalculateWaste(coin_selection_params.min_viable_change, 800 | coin_selection_params.m_cost_of_change, 801 | coin_selection_params.m_change_fee); 802 | + 803 | + // Verify we haven't exceeded the maximum allowed weight 804 | + int max_inputs_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR);
Approach ACK
This also avoids signing all inputs prior to erroring out.
603 | + assert_raises_rpc_error(-4, "The inputs size exceeds the maximum weight. Please try sending a smaller amount or manually consolidating your wallet's UTXOs", 604 | + wallet.send, outputs=[{wallet.getnewaddress(): 0.1 * 1471}]) 605 | + 606 | + # 3) Pre-select some inputs and let the wallet fill-up the remaining amount 607 | + inputs = inputs[0:1000] 608 | + 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 72b226882fe2348a9a66aee1d8d21b4e2d275e68.
re ACK for 72b226882fe2348a9a66aee1d8d21b4e2d275e68
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.
587 | + wallet = self.nodes[1].get_wallet_rpc("test_weight_limits") 588 | + 589 | + # Generate future inputs; 272 WU per input (273 when high-s). 590 | + # Picking 1471 inputs will exceed the max standard tx weight. 591 | + outputs = [] 592 | + for _ in range(1472):
Any specific reason to generate 1472 outputs when 1471 are picked?
Any specific reason to generate 1472 outputs when 1471 are picked?
All are picked actually. The extra output covers the fee.
Oh yeah, makes sense. TY.
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:
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: https://github.com/bitcoin/bitcoin/pull/29523/commits/79f3affa3954ebedcdcb28101723f5ce3fe05880, 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.
utACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
ACK 72b226882fe2348a9a66aee1d8d21b4e2d275e68
1330 | + 1331 | + # 1) Try to fund transaction only using the preset inputs 1332 | + input_weights = [] 1333 | + for i in range(1471): 1334 | + input_weights.append({"txid": txid, "vout": i, "weight": 273}) 1335 | + 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'.