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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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",
return util::Error... line was commented out in source/wallet/spend.cpp. It did (as expected).
return util::Error...) for 72b226882fe2348a9a66aee1d8d21b4e2d275e68. Failed as expected.
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?
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.
(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?
send() and walletprocesspsbt() will also fail. Will add coverage for one of them.
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);
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",
return util::Error... for 72b226882fe2348a9a66aee1d8d21b4e2d275e68.
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?
All are picked actually. The extra output covers the fee.
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.
