rpc: Better error messages for invalid addresses #20832

eilx2 · 2021-01-03T01:12:53Z

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.

eilx2 · 2021-01-03T01:14:48Z

Tagging @instagibbs as requested.

jonatack

Concept ACK, builds cleanly and works. A few suggestions follow.

src/key_io.cpp

src/wallet/rpcwallet.cpp

src/rpc/misc.cpp

src/key_io.cpp

eilx2 · 2021-01-03T23:11:29Z

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?

src/rpc/misc.cpp

instagibbs

ACK

Take or leave the suggested changes, test coverage of various paths is nice.

test/functional/rpc_invalid_address_message.py

src/key_io.cpp

jonasschnelli

code review ACK f1c7aea502494055fd87b9335e786f151564a49c (modulo nitpicks).

src/key_io.cpp

src/wallet/rpcwallet.cpp

eilx2 · 2021-01-04T14:56:01Z

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.

jonatack

Almost-ACK 11de868a3d3e393e1fde4e5f45232ad5b8c94324

The new test file test/functional/rpc_invalid_address_message.py has the wrong file permissions.

src/key_io.h

jonatack · 2021-01-04T17:43:08Z

ACK f58a4ed145f79ea3c62c3929647db6a4ee193a8c

DrahtBot · 2021-01-04T18:03:21Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

BIP 350: Implement Bech32m and use it for v1+ segwit addresses #20861 (Implement Bech32m and use it for v1+ segwit addresses by sipa)

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.

instagibbs · 2021-01-06T00:23:05Z

reACK f58a4ed

luke-jr

utACK, w/ a few nits

src/key_io.cpp

luke-jr · 2021-01-06T16:58:57Z

src/rpc/misc.cpp

Maybe split up the two possible return cases like getrawmempool does?

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

felipsoarez

ACK f58a4ed
I have reviewed it and it looks OK

instagibbs · 2021-01-08T08:02:44Z

4 ACKs, RFM imo

kristapsk

ACK f58a4ed145f79ea3c62c3929647db6a4ee193a8c (I agree with @luke-jr comment about WITNESS_PROG_MAX_LEN constant name, but don't think that's too important). Apart from reviewing code and running test suite, made sure this does not break JoinMarket which uses getaddressinfo RPC.

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.

eilx2 · 2021-01-24T01:52:27Z

Thanks everyone for the review! I've rebased on the latest master so there are no more merge conflicts.

kristapsk

ACK 8f0b64f

meshcollider

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

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)

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

fanquake · 2022-09-15T15:13:12Z

This change was part of 22.0, removing "Needs release note".

DrahtBot added RPC/REST/ZMQ Utils/log/libs Wallet labels Jan 3, 2021

bitcoin deleted a comment Jan 3, 2021

jonatack reviewed Jan 3, 2021

View reviewed changes

eilx2 force-pushed the master branch 4 times, most recently from 0f8f334 to f1c7aea Compare January 3, 2021 22:15

jonatack reviewed Jan 3, 2021

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

fanquake requested a review from instagibbs January 4, 2021 06:18

instagibbs approved these changes Jan 4, 2021

View reviewed changes

test/functional/rpc_invalid_address_message.py Outdated Show resolved Hide resolved

src/key_io.cpp Outdated Show resolved Hide resolved

jonasschnelli approved these changes Jan 4, 2021

View reviewed changes

src/key_io.cpp Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

eilx2 force-pushed the master branch 2 times, most recently from 65ca93d to 11de868 Compare January 4, 2021 12:25

jonatack reviewed Jan 4, 2021

View reviewed changes

src/key_io.h Outdated Show resolved Hide resolved

eilx2 force-pushed the master branch from 11de868 to f58a4ed Compare January 4, 2021 16:39

DrahtBot mentioned this pull request Jan 4, 2021

Replace boost::variant with std::variant #20480

Merged

alizeyni approved these changes Jan 5, 2021

View reviewed changes

bitcoin deleted a comment from alizeyni Jan 5, 2021

DrahtBot mentioned this pull request Jan 5, 2021

BIP 350: Implement Bech32m and use it for v1+ segwit addresses #20861

Merged

luke-jr approved these changes Jan 6, 2021

View reviewed changes

felipsoarez reviewed Jan 7, 2021

View reviewed changes

kristapsk approved these changes Jan 10, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jan 11, 2021

eilx2 force-pushed the master branch from f58a4ed to 8f0b64f Compare January 24, 2021 01:47

DrahtBot removed the Needs rebase label Jan 24, 2021

kristapsk approved these changes Jan 24, 2021

View reviewed changes

meshcollider reviewed Jan 25, 2021

View reviewed changes

meshcollider merged commit 4b15ffe into bitcoin:master Jan 26, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 26, 2021

Merge bitcoin#20832: rpc: Better error messages for invalid addresses

1f9d20b

meshcollider added the Needs release note label Jan 27, 2021

jarolrod mentioned this pull request Feb 26, 2021

Better errors for address use from wrong network #20809

Closed

ghost mentioned this pull request Apr 21, 2021

Error messages for invalid address #21741

Closed

apoelstra mentioned this pull request Aug 5, 2021

Introduce blech32m format for v1+ witness programs ElementsProject/elements#1025

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

fanquake removed the Needs release note label Sep 15, 2022

rpc: Better error messages for invalid addresses #20832

rpc: Better error messages for invalid addresses #20832

Uh oh!

Conversation

eilx2 commented Jan 3, 2021

Uh oh!

eilx2 commented Jan 3, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eilx2 commented Jan 3, 2021

Uh oh!

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eilx2 commented Jan 4, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Jan 4, 2021

Uh oh!

DrahtBot commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs commented Jan 6, 2021

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

eilx2 Jan 24, 2021

Choose a reason for hiding this comment

Uh oh!

felipsoarez left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jan 8, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

eilx2 commented Jan 24, 2021

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Sep 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

DrahtBot commented Jan 4, 2021 •

edited

Loading