test: fix inconsistency in fundrawtransaction weight limits test #30353
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
rkrux
left a comment
rkrux
left a comment
There was a problem hiding this comment.
tACK 4f571fb
make, test/functional are successful.
Verified that the default value of add_inputs is True in fundrawtransaction that might have caused the inconsistency in the CI: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/spend.cpp#L751
Also verified that the corresponding scenario in wallet_send.py has add_inputs: False passed already here: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_send.py#L600
|
Code review ACK 4f571fb73526b55a83af3c04fbc662ccdd1307ae This looks like the right fix for the observed test failure to me. |
There was a problem hiding this comment.
Concept ACK
I could not recreate this failure locally. Looking closely at the code, 4f571fb73526b55a83af3c04fbc662ccdd1307ae might not solve the issue.
For us to load coins from AutomaticCoinSelection and reach the failure in the C.I.
selection_target in SelectCoins must be greater than 0.
But when selection_target is > 0 and we prevent adding input with m_allow_other_inputs=False, just as it's done in 4f571fb73526b55a83af3c04fbc662ccdd1307ae we will encounter another issue:
Lines 769 to 771 in 9adebe1
I wonder why selection_target is not even positive in this test and cause failure like it did in the C.I because;
The selected inputs values should be 14710000000 sats.
Output value is also same 14710000000 sats.
nTargetValue (output value + fees) should be greater than the selected inputs values making selection_target positive?
I think to fix to prevent this failure, we should add more inputs to the input_weight to cover for the fees and ensure the selection_target is negative.
diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py
index 3c1b2deb1d7..fdc7d8e80a2 100755
--- a/test/functional/wallet_fundrawtransaction.py
+++ b/test/functional/wallet_fundrawtransaction.py
@@ -1326,11 +1326,12 @@ class RawTransactionsTest(BitcoinTestFramework):
self.generate(self.nodes[0], 1)
# 272 WU per input (273 when high-s); picking 1471 inputs will exceed the max standard tx weight.
+ # Add 1 input to cover for fees
rawtx = wallet.createrawtransaction([], [{wallet.getnewaddress(): 0.1 * 1471}])
# 1) Try to fund transaction only using the preset inputs
input_weights = []
- for i in range(1471):
+ for i in range 1472):
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)Or even better, we should fail early without needing to perform all the redundant operations whenever pre-selected inputs weight alone exceed the maximum standard weight.
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index ac2a4826f05..d3c1349553e 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -670,6 +670,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
}
}
+ int total_inputs_weight{0};
if (options.exists("input_weights")) {
for (const UniValue& input : options["input_weights"].get_array().getValues()) {
Txid txid = Txid::FromUint256(ParseHashO(input, "txid"));
@@ -696,10 +697,13 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact
if (weight > MAX_STANDARD_TX_WEIGHT) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, weight cannot be greater than the maximum standard tx weight of %d", MAX_STANDARD_TX_WEIGHT));
}
-
+ total_inputs_weight += weight;
coinControl.SetInputWeight(COutPoint(txid, vout), weight);
}
}
+ if (total_inputs_weight > MAX_STANDARD_TX_WEIGHT) {
+ throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Selected inputs weight cannot be greater than the maximum standard tx weight of %d", MAX_STANDARD_TX_WEIGHT));
+ }
if (recipients.empty())
throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output");
diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py
index 3c1b2deb1d7..727451fb79e 100755
--- a/test/functional/wallet_fundrawtransaction.py
+++ b/test/functional/wallet_fundrawtransaction.py
@@ -1332,7 +1332,7 @@ class RawTransactionsTest(BitcoinTestFramework):
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)
+ assert_raises_rpc_error(-8, "Selected inputs weight cannot be greater than the maximum standard tx weight of 400000", wallet.fundrawtransaction, hexstring=rawtx, input_weights=input_weights)
# 2) Let the wallet fund the transaction
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",
There was a problem hiding this comment.
Nice writeup @ismaelsadeeq!. Thanks for the push, just understood the reason behind it.
The issue is in the inputs selection. They are manually selected from the parent tx without excluding the change output. This output is not from the wallet and helps to surpass the selection amount (otherwise yes, the test would be failing all the time). So, the CI failure arises when the test excludes the change output which only occurs when the change output is at the end of the tx outputs vector.. whose probability is very low.
This also presents an issue inside the wallet. Because it is not detecting that the pre-selected input is not solvable when the user provides the inputs weight. And this has an edge case explanation too.. it is because the wallet can know the not-owned change output internally (which is not so common) when it is an output of a transaction the wallet stores -> because another output(s) of this tx is owned by the wallet. Extra note: This is not a problem for fundrawtransaction() because the command does not sign the transaction. Only for send().
Will fix the test and also this last issue.
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.
furszy
force-pushed
the
2024_test_fix_max_weight_test
branch
from
4f571fb to
00b8e26
Compare
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
Updated per feedback.
The CI failure that originated this PR (#30309 (comment)) can be replicated with the following patch in master:
diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py
--- a/test/functional/wallet_fundrawtransaction.py (revision 9b480f7a25a737c9c4ebc33401e94d66c2da9ec3)
+++ 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)
|
The test failure looks unrelated: |
ismaelsadeeq
left a comment
ismaelsadeeq
left a comment
There was a problem hiding this comment.
Code review and Tested ACK 00b8e26
I was able to recreate the failure with the diff in the OP. I also decoded the transaction and confirmed that it's selecting the change output of the parent transaction with a value of 2.79949465 BTC. That's why the test was not failing.
The commit 00b8e26 fixes this issue.
This is not a problem for
fundrawtransaction()
Yes, it is not. The call will succeed, but when attempting to sign the transaction with signrawtransactionwithkey, the RPC will return an error: "Unable to sign input, invalid stack size (possibly missing key)".
Only for send()
How is this an issue for send()?
Will fix the test and also this last issue.
In this PR? Also, should we consider returning this error early as I suggested?
|
A bit late but here @ismaelsadeeq.
Mainly because of what users would expect from the
Yes sure. thats a possibility too. It does not catch all the scenarios (e.g. max weight can be exceeded with the sum of inputs and outputs sizes) but at least will promptly alert users when they set high custom inputs weights. |
7 participants
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: