Better error messages for invalid addresses

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)

src/key_io.cpp

@@ -17,6 +17,9 @@
 #include <string.h>
 #include <algorithm>
 
+/// Maximum witness length for Bech32 addresses.
+static constexpr std::size_t BECH32_WITNESS_PROG_MAX_LEN = 40;
+
 namespace
 {
 class DestinationEncoder : public boost::static_visitor<std::string>
@@ -123,11 +126,12 @@ class DestinationEncoder : public boost::static_visitor<std::string>
     std::string operator()(const NullData& null) const { return {}; }
 };
 
-CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, const bool for_parent)
+CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, const bool for_parent, std::string& error_str)
 {
     std::vector<unsigned char> data;
     size_t pk_size = CPubKey::COMPRESSED_SIZE;
     uint160 hash;
+    error_str = "";
     if (DecodeBase58Check(str, data, 55)) {
         // base58-encoded Bitcoin addresses.
         // Public-key-hash-addresses have version 0 (or 111 testnet).
@@ -167,11 +171,22 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
             std::copy(payload_start + pk_size, data.end(), hash.begin());
             return ScriptHash(CScriptID(hash), pubkey);
         }
+
+        // Set potential error message.
+        // This message may be changed if the address can also be interpreted as a Bech32 address.
+        error_str = "Invalid prefix for Base58-encoded address";
     }
     data.clear();
     auto bech = bech32::Decode(str);
     const std::string& hrp = for_parent ? params.ParentBech32HRP() : params.Bech32HRP();
-    if (bech.second.size() > 0 && bech.first == hrp) {
+    if (bech.second.size() > 0) {
+        error_str = "";
+
+        if (bech.first != hrp) {
+            error_str = "Invalid prefix for Bech32 address";
+            return CNoDestination();
+        }
+
         // Bech32 decoding
         int version = bech.second[0]; // The first 5 bit symbol is the witness version (0-16)
         // The rest of the symbols are converted witness program bytes.
@@ -192,11 +207,21 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
                         return scriptid;
                     }
                 }
+
+                error_str = "Invalid Bech32 v0 address data size";
                 return CNoDestination();
             }
-            if (version > 16 || data.size() < 2 || data.size() > 40) {
+
+            if (version > 16) {
+                error_str = "Invalid Bech32 address witness version";
                 return CNoDestination();
             }
+
+            if (data.size() < 2 || data.size() > BECH32_WITNESS_PROG_MAX_LEN) {
+                error_str = "Invalid Bech32 address data size";
+                return CNoDestination();
+            }
+
             WitnessUnknown unk;
             unk.version = version;
             std::copy(data.begin(), data.end(), unk.program);
@@ -254,6 +279,9 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
         }
     }
 
+    // Set error message if address can't be interpreted as Base58 or Bech32.
+    if (error_str.empty()) error_str = "Invalid address format";
+
     return CNoDestination();
 }
 } // namespace
@@ -341,32 +369,41 @@ std::string EncodeDestination(const CTxDestination& dest)
     return boost::apply_visitor(DestinationEncoder(Params(), false), dest);
 }
 
+CTxDestination DecodeDestination(const std::string& str, std::string& error_msg)
+{
+    return DecodeDestination(str, Params(), false, error_msg);
+}
+
 CTxDestination DecodeDestination(const std::string& str)
 {
-    return DecodeDestination(str, Params(), false);
+    std::string error_msg;
+    return DecodeDestination(str, Params(), false, error_msg);
 }
 
 bool IsValidDestinationString(const std::string& str, const CChainParams& params)
 {
-    return IsValidDestination(DecodeDestination(str, params, false));
+    std::string error_msg;
+    return IsValidDestination(DecodeDestination(str, params, false, error_msg));
 }
 
 bool IsValidDestinationString(const std::string& str)
 {
-    return IsValidDestination(DecodeDestination(str, Params(), false));
+    std::string error_msg;
+    return IsValidDestination(DecodeDestination(str, Params(), false, error_msg));
 }
 
 //
 // ELEMENTS
 
+
 std::string EncodeParentDestination(const CTxDestination& dest)
 {
     return boost::apply_visitor(DestinationEncoder(Params(), true), dest);
 }
 
-CTxDestination DecodeParentDestination(const std::string& str)
+CTxDestination DecodeParentDestination(const std::string& str, std::string& error_msg)
 {
-    return DecodeDestination(str, Params(), true);
+    return DecodeDestination(str, Params(), true, error_msg);
 }
 
 // ELEMENTS

src/key_io.h

@@ -23,11 +23,12 @@ std::string EncodeExtPubKey(const CExtPubKey& extpubkey);
 
 std::string EncodeDestination(const CTxDestination& dest);
 CTxDestination DecodeDestination(const std::string& str);
+CTxDestination DecodeDestination(const std::string& str, std::string& error_msg);
 bool IsValidDestinationString(const std::string& str);
 bool IsValidDestinationString(const std::string& str, const CChainParams& params);
 
 // ELEMENTS
 std::string EncodeParentDestination(const CTxDestination& dest);
-CTxDestination DecodeParentDestination(const std::string& str);
+CTxDestination DecodeParentDestination(const std::string& str, std::string& error_msg);
 
 #endif // BITCOIN_KEY_IO_H

src/rpc/misc.cpp

@@ -43,9 +43,9 @@ 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_parent", "If the address is valid or not for parent chain. Valid or not, no additional details will be appended unlike `isvalid`"},
-                        {RPCResult::Type::STR, "address", "The address validated"},
+                        {RPCResult::Type::BOOL, "isvalid", "If the address is valid or not"},
+                        {RPCResult::Type::BOOL, "isvalid_parent", "If the address is valid or not for parent chain"},
+                        {RPCResult::Type::STR, "address", "The bitcoin address validated"},
                         {RPCResult::Type::STR_HEX, "scriptPubKey", "The hex-encoded scriptPubKey generated by the address"},
                         {RPCResult::Type::BOOL, "isscript", "If the key is a script"},
                         {RPCResult::Type::BOOL, "iswitness", "If the address is a witness address"},
@@ -58,6 +58,8 @@ static RPCHelpMan validateaddress()
                             {RPCResult::Type::STR, "address", ""},
                             {RPCResult::Type::STR_HEX, "scriptPubKey", ""},
                         }},
+                        {RPCResult::Type::STR, "error", /* optional */ true, "Error message, if any"},
+                        {RPCResult::Type::STR, "error_parent", /* optional */ true, "Error message, if any"},
                     }
                 },
                 RPCExamples{
@@ -66,15 +68,19 @@ static RPCHelpMan validateaddress()
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    CTxDestination dest = DecodeDestination(request.params[0].get_str());
-    CTxDestination parent_dest = DecodeParentDestination(request.params[0].get_str());
+    std::string error_msg;
+    CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg);
+    std::string error_msg_parent;
+    CTxDestination parent_dest = DecodeParentDestination(request.params[0].get_str(), error_msg_parent);
     bool isValid = IsValidDestination(dest);
     bool is_valid_parent = IsValidDestination(parent_dest);
+    CHECK_NONFATAL(isValid == error_msg.empty());
+    CHECK_NONFATAL(is_valid_parent == error_msg_parent.empty());
+
     UniValue ret(UniValue::VOBJ);
     ret.pushKV("isvalid", isValid);
     ret.pushKV("isvalid_parent", is_valid_parent);
-    if (isValid)
-    {
+    if (isValid) {
         std::string currentAddress = EncodeDestination(dest);
         ret.pushKV("address", currentAddress);
 
@@ -100,6 +106,11 @@ static RPCHelpMan validateaddress()
         parent_info.pushKVs(blind_detail);
         ret.pushKV("parent_address_info", parent_info);
     }
+    if (!isValid && !is_valid_parent) {
+        ret.pushKV("error", error_msg);
+        ret.pushKV("error_parent", error_msg_parent);
+    }
+
     return ret;
 },
     };

src/wallet/rpcwallet.cpp

@@ -4222,13 +4222,19 @@ RPCHelpMan getaddressinfo()
 
     LOCK(pwallet->cs_wallet);
 
-    UniValue ret(UniValue::VOBJ);
-    CTxDestination dest = DecodeDestination(request.params[0].get_str());
+    std::string error_msg;
+    CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg);
+
     // Make sure the destination is valid
     if (!IsValidDestination(dest)) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
+        // Set generic error message in case 'DecodeDestination' didn't set it
+        if (error_msg.empty()) error_msg = "Invalid address";
+
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error_msg);
     }
 
+    UniValue ret(UniValue::VOBJ);
+
     std::string currentAddress = EncodeDestination(dest);
     ret.pushKV("address", currentAddress);
 
@@ -5492,9 +5498,10 @@ static RPCHelpMan sendtomainchain_base()
 
     EnsureWalletIsUnlocked(pwallet);
 
-    CTxDestination parent_address = DecodeParentDestination(request.params[0].get_str());
+    std::string error_str;
+    CTxDestination parent_address = DecodeParentDestination(request.params[0].get_str(), error_str);
     if (!IsValidDestination(parent_address))
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Invalid Bitcoin address: %s", error_str));
 
     CAmount nAmount = AmountFromValue(request.params[1]);
     if (nAmount <= 0)

test/functional/rpc_invalid_address_message.py

@@ -0,0 +1,78 @@
+#!/usr/bin/env python3
+# Copyright (c) 2020 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test error messages for 'getaddressinfo' and 'validateaddress' RPC commands."""
+
+from test_framework.test_framework import BitcoinTestFramework
+
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+)
+
+BECH32_VALID = 'ert1qtmp74ayg7p24uslctssvjm06q5phz4yr7gdkdv'
+BECH32_INVALID_SIZE = 'ert1sqqpx5djq'
+BECH32_INVALID_PREFIX = 'bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4'
+
+BASE58_VALID = '2dcjQH4DQC3pMcSQkMkSQyPPEr7rZ6Ga4GR'
+BASE58_INVALID_PREFIX = '17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhem'
+
+INVALID_ADDRESS = 'asfah14i8fajz0123f'
+
+class InvalidAddressErrorMessageTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_wallet()
+
+    def test_validateaddress(self):
+        node = self.nodes[0]
+
+        # Bech32
+        info = node.validateaddress(BECH32_INVALID_SIZE)
+        assert not info['isvalid']
+        assert_equal(info['error'], 'Invalid Bech32 address data size')
+
+        info = node.validateaddress(BECH32_INVALID_PREFIX)
+        assert not info['isvalid']
+        assert_equal(info['error'], 'Invalid prefix for Bech32 address')
+
+        info = node.validateaddress(BECH32_VALID)
+        assert info['isvalid']
+        assert 'error' not in info
+
+        # Base58
+        info = node.validateaddress(BASE58_INVALID_PREFIX)
+        assert not info['isvalid']
+        assert_equal(info['error'], 'Invalid prefix for Base58-encoded address')
+
+        info = node.validateaddress(BASE58_VALID)
+        assert info['isvalid']
+        assert 'error' not in info
+
+        # Invalid address format
+        info = node.validateaddress(INVALID_ADDRESS)
+        assert not info['isvalid']
+        assert_equal(info['error'], 'Invalid address format')
+
+    def test_getaddressinfo(self):
+        node = self.nodes[0]
+
+        assert_raises_rpc_error(-5, "Invalid Bech32 address data size", node.getaddressinfo, BECH32_INVALID_SIZE)
+
+        assert_raises_rpc_error(-5, "Invalid prefix for Bech32 address", node.getaddressinfo, BECH32_INVALID_PREFIX)
+
+        assert_raises_rpc_error(-5, "Invalid prefix for Base58-encoded address", node.getaddressinfo, BASE58_INVALID_PREFIX)
+
+        assert_raises_rpc_error(-5, "Invalid address format", node.getaddressinfo, INVALID_ADDRESS)
+
+    def run_test(self):
+        self.test_validateaddress()
+        self.test_getaddressinfo()
+
+
+if __name__ == '__main__':
+    InvalidAddressErrorMessageTest().main()

test/functional/test_runner.py

@@ -158,6 +158,7 @@
     'wallet_keypool_topup.py',
     'wallet_keypool_topup.py --descriptors',
     'interface_zmq.py',
+    'rpc_invalid_address_message.py',
     'interface_bitcoin_cli.py',
     'mempool_resurrect.py',
     'wallet_txn_doublespend.py --mineblock',

test/functional/wallet_basic.py

@@ -567,7 +567,7 @@ def run_test(self):
         assert_equal(total_txs, len(self.nodes[0].listtransactions("*", 99999)))
 
         # Test getaddressinfo on external address. Note that these addresses are taken from disablewallet.py
-        assert_raises_rpc_error(-5, "Invalid address", self.nodes[0].getaddressinfo, "3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy")
+        assert_raises_rpc_error(-5, "Invalid prefix for Base58-encoded address", self.nodes[0].getaddressinfo, "3J98t1WpEZ73CNmQviecrnyiWrnqRhWNLy")
         address_info = self.nodes[0].getaddressinfo("CTEuA2rzWUDTGt6jFwmAtGtWWqtRgP7NwY4qbGGyht8QpgLvpQmbRHtzeF1yTV1rmJ9GiHhJoqnrbUYT")
         assert_equal(address_info['address'], "CTEuA2rzWUDTGt6jFwmAtGtWWqtRgP7NwY4qbGGyht8QpgLvpQmbRHtzeF1yTV1rmJ9GiHhJoqnrbUYT")
         assert_equal(address_info["scriptPubKey"], "76a914dc389a2145ec3cd4fe37bd6a11653456168cfa2c88ac")