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

8f0b64f Better error messages for invalid addresses (Bezdrighin)

Pull request description:

  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.

ACKs for top commit:
  kristapsk:
    ACK 8f0b64f
  meshcollider:
    Code review ACK 8f0b64f

Tree-SHA512: ca0f806ab573e96b79e98d9f8c810b81fa99c638d9b5e4d99dc18c8bd2568e6a802ec305fdfb2983574a97a19a46fd53b77645f8078fb77e9deb24ad2a22cf93

Author: meshcollider

src/key_io.cpp

@@ -12,6 +12,9 @@
 #include <assert.h>
 #include <string.h>
 
+/// Maximum witness length for Bech32 addresses.
+static constexpr std::size_t BECH32_WITNESS_PROG_MAX_LEN = 40;
+
 namespace {
 class DestinationEncoder
 {
@@ -65,10 +68,11 @@ class DestinationEncoder
     std::string operator()(const CNoDestination& no) const { return {}; }
 };
 
-CTxDestination DecodeDestination(const std::string& str, const CChainParams& params)
+CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, std::string& error_str)
 {
     std::vector<unsigned char> data;
     uint160 hash;
+    error_str = "";
     if (DecodeBase58Check(str, data, 21)) {
         // base58-encoded Bitcoin addresses.
         // Public-key-hash-addresses have version 0 (or 111 testnet).
@@ -85,10 +89,21 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
             std::copy(data.begin() + script_prefix.size(), data.end(), hash.begin());
             return ScriptHash(hash);
         }
+
+        // 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);
-    if (bech.second.size() > 0 && bech.first == params.Bech32HRP()) {
+    if (bech.second.size() > 0) {
+        error_str = "";
+
+        if (bech.first != params.Bech32HRP()) {
+            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.
@@ -109,18 +124,32 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
                         return scriptid;
                     }
                 }
+
+                error_str = "Invalid Bech32 v0 address data size";
+                return CNoDestination();
+            }
+
+            if (version > 16) {
+                error_str = "Invalid Bech32 address witness version";
                 return CNoDestination();
             }
-            if (version > 16 || data.size() < 2 || data.size() > 40) {
+
+            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);
             unk.length = data.size();
             return unk;
         }
     }
+
+    // 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
@@ -208,14 +237,21 @@ std::string EncodeDestination(const CTxDestination& dest)
     return std::visit(DestinationEncoder(Params()), dest);
 }
 
+CTxDestination DecodeDestination(const std::string& str, std::string& error_msg)
+{
+    return DecodeDestination(str, Params(), error_msg);
+}
+
 CTxDestination DecodeDestination(const std::string& str)
 {
-    return DecodeDestination(str, Params());
+    std::string error_msg;
+    return DecodeDestination(str, error_msg);
 }
 
 bool IsValidDestinationString(const std::string& str, const CChainParams& params)
 {
-    return IsValidDestination(DecodeDestination(str, params));
+    std::string error_msg;
+    return IsValidDestination(DecodeDestination(str, params, error_msg));
 }
 
 bool IsValidDestinationString(const std::string& str)

src/key_io.h

@@ -23,6 +23,7 @@ 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);
 

src/rpc/misc.cpp

@@ -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"},
                         {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"},
                         {RPCResult::Type::NUM, "witness_version", /* optional */ true, "The version number of the witness program"},
                         {RPCResult::Type::STR_HEX, "witness_program", /* optional */ true, "The hex value of the witness program"},
+                        {RPCResult::Type::STR, "error", /* optional */ true, "Error message, if any"},
                     }
                 },
                 RPCExamples{
@@ -54,13 +55,14 @@ static RPCHelpMan validateaddress()
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    CTxDestination dest = DecodeDestination(request.params[0].get_str());
-    bool isValid = IsValidDestination(dest);
+    std::string error_msg;
+    CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg);
+    const bool isValid = IsValidDestination(dest);
+    CHECK_NONFATAL(isValid == error_msg.empty());
 
     UniValue ret(UniValue::VOBJ);
     ret.pushKV("isvalid", isValid);
-    if (isValid)
-    {
+    if (isValid) {
         std::string currentAddress = EncodeDestination(dest);
         ret.pushKV("address", currentAddress);
 
@@ -69,7 +71,10 @@ static RPCHelpMan validateaddress()
 
         UniValue detail = DescribeAddress(dest);
         ret.pushKVs(detail);
+    } else {
+        ret.pushKV("error", error_msg);
     }
+
     return ret;
 },
     };

src/wallet/rpcwallet.cpp

@@ -3820,13 +3820,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);
 

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 = 'bcrt1qtmp74ayg7p24uslctssvjm06q5phz4yrxucgnv'
+BECH32_INVALID_SIZE = 'bcrt1sqqpl9r5c'
+BECH32_INVALID_PREFIX = 'bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4'
+
+BASE58_VALID = 'mipcBbFg9gMiCh81Kj8tqqdgoZub1ZJRfn'
+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

@@ -134,6 +134,7 @@
     'wallet_keypool_topup.py --descriptors',
     'feature_fee_estimation.py',
     '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

@@ -586,7 +586,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("mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
         assert_equal(address_info['address'], "mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ")
         assert_equal(address_info["scriptPubKey"], "76a9144e3854046c7bd1594ac904e4793b6a45b36dea0988ac")