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

8f0b64f Better error messages for invalid addresses (Bezdrighin)

Pull request description:

  This PR addresses bitcoin#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

src/key_io.cpp

@@ -12,6 +12,7 @@
 #include <assert.h>
 #include <string.h>
 
+
 namespace {
 class DestinationEncoder
 {
@@ -38,10 +39,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 Dash addresses.
         // Public-key-hash-addresses have version 76 (or 140 testnet).
@@ -58,6 +60,10 @@ 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";
     }
     return CNoDestination();
 }
@@ -148,14 +154,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

@@ -24,6 +24,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

@@ -223,10 +223,11 @@ static UniValue validateaddress(const JSONRPCRequest& request)
                 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 dash 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::STR, "error", /* optional */ true, "Error message, if any"},
                     }
                 },
                 RPCExamples{
@@ -235,13 +236,14 @@ static UniValue validateaddress(const JSONRPCRequest& request)
                 },
             }.Check(request);
 
-    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);
 
@@ -250,7 +252,10 @@ static UniValue validateaddress(const JSONRPCRequest& request)
 
         UniValue detail = DescribeAddress(dest);
         ret.pushKVs(detail);
+    } else {
+        ret.pushKV("error", error_msg);
     }
+
     return ret;
 }
 

src/wallet/rpcwallet.cpp

@@ -3787,13 +3787,19 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
 
     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

@@ -140,6 +140,7 @@
     'feature_fee_estimation.py',
     'interface_zmq_dash.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

@@ -595,7 +595,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("yjQ5gLvGRtmq1cwc4kePLCrzQ8GVCh9Gaz")
         assert_equal(address_info['address'], "yjQ5gLvGRtmq1cwc4kePLCrzQ8GVCh9Gaz")
         assert_equal(address_info["scriptPubKey"], "76a914fd2b4d101724a76374fccbc5b6df7670a75d7cd088ac")