test: cover edge case for lockunspent rpc #31209

This PR tests the scenario where a negative number is passed as the vout value during the 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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31209.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See 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.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

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.

diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index f1430a3c601..81fd75db0fd 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -320,7 +320,7 @@ RPCHelpMan lockunspent()
         const Txid txid = Txid::FromUint256(ParseHashO(o, "txid"));
         const int nOutput = o.find_value("vout").getInt<int>();
         if (nOutput < 0) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout cannot be negative");
+            throw JSONRPCError(RPC_WALLET_ERROR, "Invalid parameter, vout cannot be negative");
         }
 
         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".

If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and maintaining the error message in a single RPC (or each RPC individually) seems inconsistent (or brittle and verbose).

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.

https://github.com/bitcoin/bitcoin/blob/65b194193661e27cf2d9c0e0d7e3b627a379513a/src/wallet/rpc/coins.cpp#L320-L324

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 vout arg)?

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.

Closing for now. Doesn't seem to be agreement on the change.

I am happy to review a pull request with the suggested fix in #31209 (comment), but I don't have the time to work on this myself.