Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: fix invalid parameter error codes for {sign,verify}message RPCs #18466

Conversation

theStack
Copy link
Contributor

RPCs that accept address parameters usually return the intended error code RPC_INVALID_ADDRESS_OR_KEY (-5) if a passed address is invalid. The two exceptions to the rule are signmessage and verifymessage, which return RPC_TYPE_ERROR (-3) in this case instead. Oddly enough verifymessage returns RPC_INVALID_ADDRESS_OR_KEY when the signature was malformed, where RPC_TYPE_ERROR would be more approriate.

This PR fixes these inaccuracies and as well adds tests to rpc_signmessage.py that check the parameter validity and error codes for the related RPCs signmessagewithprivkey, signmessage and verifymessage.

master branch:

$ ./bitcoin-cli signmessage invalid_addr message
error code: -3
error message:
Invalid address
$ ./bitcoin-cli verifymessage invalid_addr dummy_sig message
error code: -3
error message:
Invalid address
$ ./bitcoin-cli verifymessage 12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX invalid_sig message
error code: -5
error message:
Malformed base64 encoding

PR branch:

$ ./bitcoin-cli signmessage invalid_addr message
error code: -5
error message:
Invalid address
$ ./bitcoin-cli verifymessage invalid_addr dummy_sig message
error code: -5
error message:
Invalid address
$ ./bitcoin-cli verifymessage 12c6DSiU4Rq3P4ZxziKxzrL5LmMBrzjrJX invalid_sig message
error code: -3
error message:
Malformed base64 encoding

@maflcko
Copy link
Member

maflcko commented Apr 17, 2020

Do we have guidelines on what error code to use?

@theStack
Copy link
Contributor Author

Do we have guidelines on what error code to use?

Not that I know of, I was just orienting myself on what most other RPCs receiving address parameters use as error codes (see e.g.

git grep "RPC_.*Invalid address"

). Even without guideline I'd assume it's common practice to choose the most specific error code, and only use the more more generic one (like RPC_TYPE_ERROR) if none of the specific ones match.

@laanwj
Copy link
Member

laanwj commented Aug 28, 2020

I think it's good to be consistent here, whether there are guidelines or not. To return the same return values for the same kind of errors over different RPC calls.

I do agree this change needs a release notes mention. Some people depend on specific return values, see e.g. talaia-labs/python-teos#37 (which is not relevant to this specific RPCs).

@theStack theStack force-pushed the 20200329-rpc-improve-signverifymessage-error-codes branch from d208f80 to a5cfb40 Compare August 29, 2020 08:44
@theStack
Copy link
Contributor Author

Added a relase note commit and rebased on master.

@DrahtBot DrahtBot mentioned this pull request Oct 15, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 15, 2020

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

Conflicts

No conflicts as of last run.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a5cfb40

@laanwj
Copy link
Member

laanwj commented Mar 1, 2021

Code review ACK a5cfb40

@laanwj laanwj merged commit 362e901 into bitcoin:master Mar 1, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 1, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
@theStack theStack deleted the 20200329-rpc-improve-signverifymessage-error-codes branch May 4, 2023 21:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants