lockunspent RPC call. The rationale behind this test is to validate the input parameters for the lockunspent function and ensure that the system behaves correctly when given invalid data. By confirming that the error message “Invalid parameter: vout cannot be negative” is returned, we can prevent unexpected behavior or vulnerabilities in the application.
This commit test for when a negative number is passed as the vout
value during lockunspent rpc call. It ensure that the error message:
Invalid parameter, vout cannot be negative, is returned.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31209.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | tdb3 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
ACK e58c4307ef576915200c4eda57d6d6ff10088667
Thank you. Coverage report seems to indicate coverage was added.
Sanity checked the change by executing functional tests (without legacy-wallet, but with wallet_basic.py --descriptors) with the following update.
0diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
1index f1430a3c601..81fd75db0fd 100644
2--- a/src/wallet/rpc/coins.cpp
3+++ b/src/wallet/rpc/coins.cpp
4@@ -320,7 +320,7 @@ RPCHelpMan lockunspent()
5 const Txid txid = Txid::FromUint256(ParseHashO(o, "txid"));
6 const int nOutput = o.find_value("vout").getInt<int>();
7 if (nOutput < 0) {
8- throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout cannot be negative");
9+ throw JSONRPCError(RPC_WALLET_ERROR, "Invalid parameter, vout cannot be negative");
10 }
11
12 const COutPoint outpt(txid, nOutput);
On master (03cff2c1421e5db59963eba1a845ef5dd318c275), wallet_basic did not fail.
On the PR branch the test did fail (at the new line).
Not sure about this. This seems to be adding coverage for code that shouldn’t exist in the first place.
The integer is passed to COutPoint, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.
Not sure about this. This seems to be adding coverage for code that shouldn’t exist in the first place.
The integer is passed to
COutPoint, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.
I did a quick test by commenting the check and you’re right an error was thrown. However, the error is not as descriptive as the current implementation. The error without the check: “Invalid parameter, vout index out of bounds” . Error with the check: “Invalid parameter, vout cannot be negative”.
Not sure about this. This seems to be adding coverage for code that shouldn’t exist in the first place.
The integer is passed to
COutPoint, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.
Good catch. It seems a bit odd that the RPC is trying to interpret the vout arg as a signed int in the first place (const int nOutput = o.find_value("vout").getInt<int>();), then check for negative.
Removing the check for negative would technically change RPC behavior for users (https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2347371722), but do we think anyone’s downstream use case is actually relying on this odd edge case (negative vout arg)?
Removing the check for negative would technically change RPC behavior for users (#30212 (comment)), but do we think anyone’s downstream use case is actually relying on this odd edge case (negative
voutarg)?
Matching errors based on the error string is fragile and considered bad practise, because error strings are considered less stable than error codes or error types. Also, error strings may be verbose and (if truncated, or not) could match errors in other contexts. The RPC version is also explained in https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning . Finally, downstream software should test new releases of updated dependencies (like Bitcoin Core) before deploying them in production.