Improve lockunspent validation for vout

Abdulkbk commented at 9:24 am on November 3, 2024: none

This PR adds a check for the vout value passed during the lockunspent RPC call. The code verifies if a floating-point number is provided and returns a descriptive error message.

The primary goal of this change is to enhance the user experience by providing clear feedback on invalid parameters. Currently, when a floating-point value is passed for the vout, it results in a vague “JSON integer out of range” error. This message is not descriptive and can lead to user confusion. By implementing this check, users will immediately understand that vout must be strictly an integer.

DrahtBot commented at 9:24 am on November 3, 2024: contributor

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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31209 (test: cover edge case for lockunspent rpc by Abdulkbk)

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.

DrahtBot added the label CI failed on Nov 3, 2024

DrahtBot commented at 10:59 am on November 3, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/32438621518

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Abdulkbk marked this as a draft on Nov 3, 2024

rpc: check invalid vout value for lockunspent

This commit introduces a validation check for the vout parameter
to ensure it is an integer. If a floating-point number is provided,
an informative error message is returned: "Invalid parameter,
vout must be an integer" instead of the previous generic error
"JSON integer out of range." This change enhances usability by
providing clearer feedback on incorrect input types.

462c68c7c8

Abdulkbk force-pushed on Nov 3, 2024

DrahtBot removed the label CI failed on Nov 4, 2024

Abdulkbk marked this as ready for review on Nov 4, 2024

maflcko commented at 7:46 am on November 4, 2024: member

Can you motivate this a bit better? getInt is used in many RPCs. What makes this call to getInt in this RPC special? What real-world end-user-facing issue is this trying to solve?

Abdulkbk commented at 12:39 pm on November 4, 2024: none

getInt is used in many RPCs. What makes this call to getInt in this RPC special?

Thank you, @maflcko, for your feedback. I believe that getInt in lockunspent is special because the vout param specifically represents transaction output indexes, which are always whole numbers. Since this value comes directly from user input, providing a descriptive error message would be very helpful.

I also updated the PR description to add motivation for this change.

maflcko commented at 1:15 pm on November 4, 2024: member

Every use of getInt represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?

Tend toward NACK for now, unless there is a reason.

Abdulkbk commented at 2:52 pm on November 4, 2024: none

Every use of getInt represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?

Tend toward NACK for now, unless there is a reason.

I am new to the codebase and learned that every use of getInt represents a whole number after you mentioned it.

For the end users facing this issue, one example I can give, those using bitcoin-cli lockunspent, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error “JSON integer out of range.”

I’d like to hear your thoughts about the test in wallet_basic.py because there is currently no test that covers floating-point vout for lockunspent.Is adding that helpful to Bitcoin?

maflcko commented at 3:08 pm on November 4, 2024: member

For the end users facing this issue, one example I can give, those using bitcoin-cli lockunspent, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error “JSON integer out of range.”

If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.

Abdulkbk commented at 4:04 pm on November 4, 2024: none

For the end users facing this issue, one example I can give, those using bitcoin-cli lockunspent, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error “JSON integer out of range.”

If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.

In that case I will convert this PR to draft and work on figuring a way to consistently improve the error message across the RPCs. Thanks for all the feedback.

Abdulkbk closed this on Nov 4, 2024

Improve lockunspent validation for vout #31205

Code Coverage & Benchmarks

Reviews

Conflicts