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.
149 }
150 }
151+
152+ // Set error message if address can't be interpreted neither as Base58 nor Bech32.
153+ if (error_str.empty())
154+ error_str = "Invalid address format";
See doc/developer-notes.md:
If an `if` only has a single-statement `then`-clause, it can appear
on the same line as the `if`, without braces. In every other case,
braces are required, and the `then` and `else` clauses must appear
correctly indented on a new line.3830+
3831 // Make sure the destination is valid
3832 if (!IsValidDestination(dest)) {
3833- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
3834+ if (error_msg.empty())
3835+ error_msg = "Invalid address";
39@@ -40,6 +40,7 @@ static RPCHelpMan validateaddress()
40 RPCResult::Type::OBJ, "", "",
41 {
42 {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not. If not, this is the only property returned."},
43+ {RPCResult::Type::STR, "error_msg", "If the address is not valid this property contains a description of the error encountered while decoding the address."},
suggestion
0@@ -40,13 +40,13 @@ static RPCHelpMan validateaddress()
1 RPCResult::Type::OBJ, "", "",
2 {
3 {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not. If not, this is the only property returned."},
4- {RPCResult::Type::STR, "error_msg", "If the address is not valid this property contains a description of the error encountered while decoding the address."},
5 {RPCResult::Type::STR, "address", "The bitcoin address validated"},
6 {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded scriptPubKey generated by the address"},
7 {RPCResult::Type::BOOL, "isscript", "If the key is a script"},
8 {RPCResult::Type::BOOL, "iswitness", "If the address is a witness address"},
9 {RPCResult::Type::NUM, "witness_version", /* optional */ true, "The version number of the witness program"},
10 {RPCResult::Type::STR_HEX, "witness_program", /* optional */ true, "The hex value of the witness program"},
11+ {RPCResult::Type::STR, "error", /* optional */ true, "Error message, if any"},
12 }
39@@ -40,6 +40,7 @@ static RPCHelpMan validateaddress()
40 RPCResult::Type::OBJ, "", "",
41 {
42 {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not. If not, this is the only property returned."},
this description needs to be updated (can remove trailing full stop too, like the other descriptions here)
0 {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not"},
69@@ -69,7 +70,10 @@ static RPCHelpMan validateaddress()
70
71 UniValue detail = DescribeAddress(dest);
72 ret.pushKVs(detail);
73+ } else {
74+ ret.pushKV("error_msg", error_msg);
suggestion
0- ret.pushKV("error_msg", error_msg);
1+ CHECK_NONFATAL(!error_msg.empty());
2+ ret.pushKV("error", error_msg);
90@@ -89,10 +91,19 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
91 std::copy(data.begin() + script_prefix.size(), data.end(), hash.begin());
92 return ScriptHash(hash);
93 }
94+
95+ // Set potential error message.
96+ // This message may be changed in case that the address can also be interpreted as a Bech32 address.
0 // This message may be changed if the address can also be interpreted as a Bech32 address.
147 unk.length = data.size();
148 return unk;
149 }
150 }
151+
152+ // Set error message if address can't be interpreted neither as Base58 nor Bech32.
0 // Set error message if address can't be interpreted as Base58 or Bech32.
138+ if (data.size() < 2 || data.size() > 40) {
139+ error_str = "Invalid Bech32 address data size";
140 return CNoDestination();
141 }
142+
143+
250- return DecodeDestination(str, Params());
251+ std::string error_msg;
252+ return DecodeDestination(str, error_msg);
253 }
254
255+
238@@ -212,14 +239,23 @@ std::string EncodeDestination(const CTxDestination& dest)
239 return boost::apply_visitor(DestinationEncoder(Params()), dest);
240 }
241
242+CTxDestination DecodeDestination(const std::string& str, std::string& error_msg)
243+{
244+ return DecodeDestination(str, Params(), error_msg);
245+}
246+
247+
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?
69@@ -69,7 +70,11 @@ static RPCHelpMan validateaddress()
70
71 UniValue detail = DescribeAddress(dest);
72 ret.pushKVs(detail);
73+ } else {
74+ CHECK_NONFATAL(!error_msg.empty());
nit, might be better like this, just realized I’m doing the same here
0@@ -57,7 +57,8 @@ static RPCHelpMan validateaddress()
1 {
2 std::string error_msg;
3 CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg);
4- bool isValid = IsValidDestination(dest);
5+ const bool isValid = IsValidDestination(dest);
6+ CHECK_NONFATAL(isValid == error_msg.empty());
7
8 UniValue ret(UniValue::VOBJ);
9 ret.pushKV("isvalid", isValid);
10@@ -71,7 +72,6 @@ static RPCHelpMan validateaddress()
11 UniValue detail = DescribeAddress(dest);
12 ret.pushKVs(detail);
13 } else {
14- CHECK_NONFATAL(!error_msg.empty());
15 ret.pushKV("error", error_msg);
16 }
9+from test_framework.util import (
10+ assert_equal,
11+ assert_raises_rpc_error,
12+)
13+
14+bech32_valid = 'bcrt1qtmp74ayg7p24uslctssvjm06q5phz4yrxucgnv'
130 return CNoDestination();
131 }
132- if (version > 16 || data.size() < 2 || data.size() > 40) {
133+
134+ if (version > 16) {
135+ error_str = "Invalid Bech32 address version";
ACK
Take or leave the suggested changes, test coverage of various paths is nice.
134+ if (version > 16) {
135+ error_str = "Invalid Bech32 address version";
136 return CNoDestination();
137 }
138+
139+ if (data.size() < 2 || data.size() > 40) {
WITHNESS_PROG_MAX_LEN) instead of a magic 40
3829+ CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg);
3830+
3831 // Make sure the destination is valid
3832 if (!IsValidDestination(dest)) {
3833- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
3834+ if (error_msg.empty()) error_msg = "Invalid address";
DecodeDestination()?
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.
12@@ -13,6 +13,8 @@
13
14 #include <string>
15
16+const std::size_t WITNESS_PROG_MAX_LEN = 40;
nit if you retouch (and maybe move to key_io.cpp if we only expect to use it there, I’m not sure what is preferred)
0static constexpr std::size_t WITNESS_PROG_MAX_LEN = 40;
Almost-ACK 11de868a3d3e393e1fde4e5f45232ad5b8c94324
The new test file test/functional/rpc_invalid_address_message.py has the wrong file permissions.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
14@@ -15,6 +15,9 @@
15 #include <string.h>
16 #include <algorithm>
17
18+/// Maximum witness length for Bech32 addresses.
19+static constexpr std::size_t WITNESS_PROG_MAX_LEN = 40;
38@@ -39,13 +39,14 @@ static RPCHelpMan validateaddress()
39 RPCResult{
40 RPCResult::Type::OBJ, "", "",
41 {
42- {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not. If not, this is the only property returned."},
43+ {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not"},
getrawmempool does?
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 #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.
