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: Better error messages for invalid addresses #20832
Conversation
Tagging @instagibbs as requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK, builds cleanly and works. A few suggestions follow.
0f8f334
to
f1c7aea
Compare
Thanks a lot for the review! I've made the proposed changes. In addition, I've realized that DecodeDestination is used in other places too which return a generic 'invalid address' error message in case if fails. Should we also handle those places as part of this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Take or leave the suggested changes, test coverage of various paths is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code review ACK f1c7aea (modulo nitpicks).
65ca93d
to
11de868
Compare
Thanks again for the comments. I think for now I'll let this PR cover only 'getaddressinfo' and 'validateaddress' just to keep it small enough. In the future I will work on the other code paths. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost-ACK 11de868
The new test file test/functional/rpc_invalid_address_message.py
has the wrong file permissions.
ACK f58a4ed |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
reACK f58a4ed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, w/ a few nits
@@ -39,13 +39,14 @@ static RPCHelpMan validateaddress() | |||
RPCResult{ | |||
RPCResult::Type::OBJ, "", "", | |||
{ | |||
{RPCResult::Type::BOOL, "isvalid", "If the address is valid or not. If not, this is the only property returned."}, | |||
{RPCResult::Type::BOOL, "isvalid", "If the address is valid or not"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe split up the two possible return cases like getrawmempool
does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I will try to do this in a future commit where I address the other places where you need to return an error message for invalid address as they probably all require a similar change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f58a4ed
I have reviewed it and it looks OK
4 ACKs, RFM imo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit addresses bitcoin#20809. We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid.
Thanks everyone for the review! I've rebased on the latest master so there are no more merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 8f0b64f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 8f0b64f
+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin#20832 for solution Github-Pull: bitcoin-core/gui#280 Rebased-From: 3bad0b3
+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin#20832 for solution Github-Pull: bitcoin-core/gui#280 Rebased-From: 3bad0b3
This commit addresses bitcoin#20809. We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid. Github-Pull: bitcoin#20832 Rebased-From: 8f0b64f
This commit addresses #20809. We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid. bitcoin/bitcoin#20832 (1/1) ELEMENTS: Merge conflicts resolved based on d6c85c5 (from 22.0 rebase)
This commit addresses #20809. We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid. bitcoin/bitcoin#20832 (1/1) ELEMENTS: Merge conflicts resolved based on d6c85c5 (from 22.0 rebase)
c72c949 blech32: copy ubsan suppression for bech32 to blech32 (Andrew Poelstra) d13fb49 blech32: add test vectors for blech32 and blech32m (Andrew Poelstra) 15a826e blech32: add functional tests for blech32m (Andrew Poelstra) c01e09e blech32: add blech32m format and use it to decode witness v1+ addresses (Andrew Poelstra) 18fcec8 naming nits (Fabian Jahr) 8515f40 Add signet support to gen_key_io_test_vectors.py (Pieter Wuille) b3df66f Use Bech32m encoding for v1+ segwit addresses (Pieter Wuille) 42f43a1 Add Bech32m test vectors (Pieter Wuille) b1d1d94 Implement Bech32m encoding/decoding (Pieter Wuille) c607835 Better error messages for invalid addresses (Bezdrighin) Pull request description: Includes backports of bitcoin/bitcoin#20832 (1 commit) and bitcoin/bitcoin#20861 (5 commits) ACKs for top commit: gwillen: utACK c72c949. Tree-SHA512: af96a6ef31b1cab72b0350197dcb34761e9ffb2ec43685084408b5fafcda0adee1945045003194600372652f7cca65d721bda3ed9b6be7e9543d3199e2cbe145
+ Detailed error messages for invalid address + Used `IsValidDestination` instead of `IsValidDestinationString` + Referred to bitcoin/bitcoin#20832 for solution
This change was part of 22.0, removing "Needs release note". |
This PR addresses #20809.
We add more detailed error messages in case an invalid address is provided inside the 'validateaddress' and 'getaddressinfo' RPC calls. This also covers the case when a user provides an address from a wrong network.
We also add a functional test to test the new error messages.