Merge bitcoin/bitcoin#16807: Let validateaddress locate error in Bech32 address

88cc481 Modify copyright header on Bech32 code (Samuel Dobson)
5599813 Add lots of comments to Bech32 (Samuel Dobson)
2eb5792 Add release notes for validateaddress Bech32 error detection (MeshCollider)
42d6a02 Refactor and add more tests for validateaddress (Samuel Dobson)
c4979f7 Add boost tests for bech32 error detection (MeshCollider)
02a7bde Add error_locations to validateaddress RPC (Samuel Dobson)
b62b67e Add Bech32 error location function (Samuel Dobson)
0b06e72 More detailed error checking for base58 addresses (Samuel Dobson)

Pull request description:

  Addresses (partially) #16779 - no GUI change in this PR

  Adds a LocateError function the bech32 library, which is then called by `validateaddress` RPC, (and then eventually from a GUI tool too, future work). I think modifying validateaddress is nicer than adding a separate RPC for this.
  Includes tests.

  Based on https://github.com/sipa/bech32/blob/master/ecc/javascript/bech32_ecc.js
  Credit to sipa for that code

ACKs for top commit:
  laanwj:
    Code review and manually tested ACK 88cc481
  ryanofsky:
    Code review ACK 88cc481 with caveat that I only checked the new `LocateErrors` code to try to verify it didn't have unsafe or unexpected operations or loop forever or crash. Did not try to verify behavior corresponds to the spec. In the worst case bugs here should just affect error messages not actual decoding of addresses so this seemed ok.
  w0xlt:
    tACK 88cc481

Tree-SHA512: 9c7fe9745bc7527f80a30bd4c1e3034e16b96a02cc7f6c268f91bfad08a6965a8064fe44230aa3f87e4fa3c938f662ff4446bc682c83cb48c1a3f95cf4186688

Author: laanwj

doc/release-notes-16807.md

@@ -0,0 +1,6 @@
+Updated RPCs
+------------
+
+- The `validateaddress` RPC now optionally returns an `error_locations` array, with the indices of
+invalid characters in the address. For example, this will return the locations of up to two Bech32
+errors.

src/bech32.cpp

@@ -1,4 +1,5 @@
 // Copyright (c) 2017, 2021 Pieter Wuille
+// Copyright (c) 2021 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -44,6 +45,9 @@ struct DecodeResult
 /** Decode a Bech32 or Bech32m string. */
 DecodeResult Decode(const std::string& str);
 
+/** Return the positions of errors in a Bech32 string. */
+std::string LocateErrors(const std::string& str, std::vector<int>& error_locations);
+
 } // namespace bech32
 
 #endif // BITCOIN_BECH32_H

src/bech32.h

@@ -76,12 +76,16 @@ class DestinationEncoder
     std::string operator()(const CNoDestination& no) const { return {}; }
 };
 
-CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, std::string& error_str)
+CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, std::string& error_str, std::vector<int>* error_locations)
 {
     std::vector<unsigned char> data;
     uint160 hash;
     error_str = "";
-    if (DecodeBase58Check(str, data, 21)) {
+
+    // Note this will be false if it is a valid Bech32 address for a different network
+    bool is_bech32 = (ToLower(str.substr(0, params.Bech32HRP().size())) == params.Bech32HRP());
+
+    if (!is_bech32 && DecodeBase58Check(str, data, 21)) {
         // base58-encoded Bitcoin addresses.
         // Public-key-hash-addresses have version 0 (or 111 testnet).
         // The data vector contains RIPEMD160(SHA256(pubkey)), where pubkey is the serialized public key.
@@ -98,15 +102,27 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
             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";
+        if (!std::equal(script_prefix.begin(), script_prefix.end(), data.begin()) &&
+            !std::equal(pubkey_prefix.begin(), pubkey_prefix.end(), data.begin())) {
+            error_str = "Invalid prefix for Base58-encoded address";
+        } else {
+            error_str = "Invalid length for Base58 address";
+        }
+        return CNoDestination();
+    } else if (!is_bech32) {
+        // Try Base58 decoding without the checksum, using a much larger max length
+        if (!DecodeBase58(str, data, 100)) {
+            error_str = "Invalid HRP or Base58 character in address";
+        } else {
+            error_str = "Invalid checksum or length of Base58 address";
+        }
+        return CNoDestination();
     }
+
     data.clear();
     const auto dec = bech32::Decode(str);
     if ((dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) && dec.data.size() > 0) {
         // Bech32 decoding
-        error_str = "";
         if (dec.hrp != params.Bech32HRP()) {
             error_str = "Invalid prefix for Bech32 address";
             return CNoDestination();
@@ -168,8 +184,13 @@ 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";
+    // Perform Bech32 error location
+    if (!error_locations) {
+        std::vector<int> dummy_errors;
+        error_str = bech32::LocateErrors(str, dummy_errors);
+    } else {
+        error_str = bech32::LocateErrors(str, *error_locations);
+    }
 
     return CNoDestination();
 }
@@ -258,9 +279,9 @@ std::string EncodeDestination(const CTxDestination& dest)
     return std::visit(DestinationEncoder(Params()), dest);
 }
 
-CTxDestination DecodeDestination(const std::string& str, std::string& error_msg)
+CTxDestination DecodeDestination(const std::string& str, std::string& error_msg, std::vector<int>* error_locations)
 {
-    return DecodeDestination(str, Params(), error_msg);
+    return DecodeDestination(str, Params(), error_msg, error_locations);
 }
 
 CTxDestination DecodeDestination(const std::string& str)
@@ -272,7 +293,7 @@ CTxDestination DecodeDestination(const std::string& str)
 bool IsValidDestinationString(const std::string& str, const CChainParams& params)
 {
     std::string error_msg;
-    return IsValidDestination(DecodeDestination(str, params, error_msg));
+    return IsValidDestination(DecodeDestination(str, params, error_msg, nullptr));
 }
 
 bool IsValidDestinationString(const std::string& str)

src/key_io.cpp

@@ -23,7 +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);
+CTxDestination DecodeDestination(const std::string& str, std::string& error_msg, std::vector<int>* error_locations = nullptr);
 bool IsValidDestinationString(const std::string& str);
 bool IsValidDestinationString(const std::string& str, const CChainParams& params);
 

src/key_io.h

@@ -52,6 +52,10 @@ static RPCHelpMan validateaddress()
                         {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"},
+                        {RPCResult::Type::ARR, "error_locations", "Indices of likely error locations in address, if known (e.g. Bech32 errors)",
+                            {
+                                {RPCResult::Type::NUM, "index", "index of a potential error"},
+                            }},
                     }
                 },
                 RPCExamples{
@@ -61,7 +65,8 @@ static RPCHelpMan validateaddress()
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
     std::string error_msg;
-    CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg);
+    std::vector<int> error_locations;
+    CTxDestination dest = DecodeDestination(request.params[0].get_str(), error_msg, &error_locations);
     const bool isValid = IsValidDestination(dest);
     CHECK_NONFATAL(isValid == error_msg.empty());
 
@@ -77,6 +82,9 @@ static RPCHelpMan validateaddress()
         UniValue detail = DescribeAddress(dest);
         ret.pushKVs(detail);
     } else {
+        UniValue error_indices(UniValue::VARR);
+        for (int i : error_locations) error_indices.push_back(i);
+        ret.pushKV("error_locations", error_indices);
         ret.pushKV("error", error_msg);
     }
 

src/rpc/misc.cpp

@@ -1,4 +1,5 @@
 // Copyright (c) 2017 Pieter Wuille
+// Copyright (c) 2021 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -68,10 +69,36 @@ BOOST_AUTO_TEST_CASE(bech32_testvectors_invalid)
         "1qzzfhee",
         "a12UEL5L",
         "A12uEL5L",
+        "abcdef1qpzrz9x8gf2tvdw0s3jn54khce6mua7lmqqqxw",
     };
+    static const std::pair<std::string, int> ERRORS[] = {
+        {"Invalid character or mixed case", 0},
+        {"Invalid character or mixed case", 0},
+        {"Invalid character or mixed case", 0},
+        {"Bech32 string too long", 90},
+        {"Missing separator", -1},
+        {"Invalid separator position", 0},
+        {"Invalid Base 32 character", 2},
+        {"Invalid separator position", 2},
+        {"Invalid character or mixed case", 8},
+        {"Invalid checksum", -1}, // The checksum is calculated using the uppercase form so the entire string is invalid, not just a few characters
+        {"Invalid separator position", 0},
+        {"Invalid separator position", 0},
+        {"Invalid character or mixed case", 3},
+        {"Invalid character or mixed case", 3},
+        {"Invalid checksum", 11}
+    };
+    int i = 0;
     for (const std::string& str : CASES) {
+        const auto& err = ERRORS[i];
         const auto dec = bech32::Decode(str);
         BOOST_CHECK(dec.encoding == bech32::Encoding::INVALID);
+        std::vector<int> error_locations;
+        std::string error = bech32::LocateErrors(str, error_locations);
+        BOOST_CHECK_EQUAL(err.first, error);
+        if (err.second == -1) BOOST_CHECK(error_locations.empty());
+        else BOOST_CHECK_EQUAL(err.second, error_locations[0]);
+        i++;
     }
 }
 
@@ -91,11 +118,37 @@ BOOST_AUTO_TEST_CASE(bech32m_testvectors_invalid)
         "au1s5cgom",
         "M1VUXWEZ",
         "16plkw9",
-        "1p2gdwpf"
+        "1p2gdwpf",
+        "abcdef1l7aum6echk45nj2s0wdvt2fg8x9yrzpqzd3ryx",
+    };
+    static const std::pair<std::string, int> ERRORS[] = {
+        {"Invalid character or mixed case", 0},
+        {"Invalid character or mixed case", 0},
+        {"Invalid character or mixed case", 0},
+        {"Bech32 string too long", 90},
+        {"Missing separator", -1},
+        {"Invalid separator position", 0},
+        {"Invalid Base 32 character", 2},
+        {"Invalid Base 32 character", 3},
+        {"Invalid separator position", 2},
+        {"Invalid Base 32 character", 8},
+        {"Invalid Base 32 character", 7},
+        {"Invalid checksum", -1},
+        {"Invalid separator position", 0},
+        {"Invalid separator position", 0},
+        {"Invalid checksum", 21},
     };
+    int i = 0;
     for (const std::string& str : CASES) {
+        const auto& err = ERRORS[i];
         const auto dec = bech32::Decode(str);
         BOOST_CHECK(dec.encoding == bech32::Encoding::INVALID);
+        std::vector<int> error_locations;
+        std::string error = bech32::LocateErrors(str, error_locations);
+        BOOST_CHECK_EQUAL(err.first, error);
+        if (err.second == -1) BOOST_CHECK(error_locations.empty());
+        else BOOST_CHECK_EQUAL(err.second, error_locations[0]);
+        i++;
     }
 }
 

src/test/bech32_tests.cpp

@@ -12,79 +12,96 @@
 )
 
 BECH32_VALID = 'bcrt1qtmp74ayg7p24uslctssvjm06q5phz4yrxucgnv'
+BECH32_VALID_CAPITALS = 'BCRT1QPLMTZKC2XHARPPZDLNPAQL78RSHJ68U33RAH7R'
+BECH32_VALID_MULTISIG = 'bcrt1qdg3myrgvzw7ml9q0ejxhlkyxm7vl9r56yzkfgvzclrf4hkpx9yfqhpsuks'
+
 BECH32_INVALID_BECH32 = 'bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqdmchcc'
 BECH32_INVALID_BECH32M = 'bcrt1qw508d6qejxtdg4y5r3zarvary0c5xw7k35mrzd'
 BECH32_INVALID_VERSION = 'bcrt130xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqynjegk'
 BECH32_INVALID_SIZE = 'bcrt1s0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v8n0nx0muaewav25430mtr'
 BECH32_INVALID_V0_SIZE = 'bcrt1qw508d6qejxtdg4y5r3zarvary0c5xw7kqqq5k3my'
 BECH32_INVALID_PREFIX = 'bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx'
+BECH32_TOO_LONG = 'bcrt1q049edschfnwystcqnsvyfpj23mpsg3jcedq9xv049edschfnwystcqnsvyfpj23mpsg3jcedq9xv049edschfnwystcqnsvyfpj23m'
+BECH32_ONE_ERROR = 'bcrt1q049edschfnwystcqnsvyfpj23mpsg3jcedq9xv'
+BECH32_ONE_ERROR_CAPITALS = 'BCRT1QPLMTZKC2XHARPPZDLNPAQL78RSHJ68U32RAH7R'
+BECH32_TWO_ERRORS = 'bcrt1qax9suht3qv95sw33xavx8crpxduefdrsvgsklu' # should be bcrt1qax9suht3qv95sw33wavx8crpxduefdrsvgsklx
+BECH32_NO_SEPARATOR = 'bcrtq049ldschfnwystcqnsvyfpj23mpsg3jcedq9xv'
+BECH32_INVALID_CHAR = 'bcrt1q04oldschfnwystcqnsvyfpj23mpsg3jcedq9xv'
+BECH32_MULTISIG_TWO_ERRORS = 'bcrt1qdg3myrgvzw7ml8q0ejxhlkyxn7vl9r56yzkfgvzclrf4hkpx9yfqhpsuks'
+BECH32_WRONG_VERSION = 'bcrt1ptmp74ayg7p24uslctssvjm06q5phz4yrxucgnv'
 
 BASE58_VALID = 'mipcBbFg9gMiCh81Kj8tqqdgoZub1ZJRfn'
 BASE58_INVALID_PREFIX = '17VZNX1SN5NtKa8UQFxwQbFeFc3iqRYhem'
+BASE58_INVALID_CHECKSUM = 'mipcBbFg9gMiCh81Kj8tqqdgoZub1ZJJfn'
+BASE58_INVALID_LENGTH = '2VKf7XKMrp4bVNVmuRbyCewkP8FhGLP2E54LHDPakr9Sq5mtU2'
 
 INVALID_ADDRESS = 'asfah14i8fajz0123f'
+INVALID_ADDRESS_2 = '1q049ldschfnwystcqnsvyfpj23mpsg3jcedq9xv'
 
 class InvalidAddressErrorMessageTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
 
-    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_INVALID_BECH32)
-        assert not info['isvalid']
-        assert_equal(info['error'], 'Version 1+ witness address must use Bech32m checksum')
-
-        info = node.validateaddress(BECH32_INVALID_BECH32M)
-        assert not info['isvalid']
-        assert_equal(info['error'], 'Version 0 witness address must use Bech32 checksum')
-
-        info = node.validateaddress(BECH32_INVALID_V0_SIZE)
-        assert not info['isvalid']
-        assert_equal(info['error'], 'Invalid Bech32 v0 address data size')
-
-        info = node.validateaddress(BECH32_VALID)
+    def check_valid(self, addr):
+        info = self.nodes[0].validateaddress(addr)
         assert info['isvalid']
         assert 'error' not in info
+        assert 'error_locations' not in info
 
-        info = node.validateaddress(BECH32_INVALID_VERSION)
-        assert not info['isvalid']
-        assert_equal(info['error'], 'Invalid Bech32 address witness version')
-
-        # Base58
-        info = node.validateaddress(BASE58_INVALID_PREFIX)
-        assert not info['isvalid']
-        assert_equal(info['error'], 'Invalid prefix for Base58-encoded address')
+    def check_invalid(self, addr, error_str, error_locations=None):
+        res = self.nodes[0].validateaddress(addr)
+        assert not res['isvalid']
+        assert_equal(res['error'], error_str)
+        if error_locations:
+            assert_equal(res['error_locations'], error_locations)
+        else:
+            assert_equal(res['error_locations'], [])
 
-        info = node.validateaddress(BASE58_VALID)
-        assert info['isvalid']
-        assert 'error' not in info
+    def test_validateaddress(self):
+        # Invalid Bech32
+        self.check_invalid(BECH32_INVALID_SIZE, 'Invalid Bech32 address data size')
+        self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid HRP or Base58 character in address')
+        self.check_invalid(BECH32_INVALID_BECH32, 'Version 1+ witness address must use Bech32m checksum')
+        self.check_invalid(BECH32_INVALID_BECH32M, 'Version 0 witness address must use Bech32 checksum')
+        self.check_invalid(BECH32_INVALID_VERSION, 'Invalid Bech32 address witness version')
+        self.check_invalid(BECH32_INVALID_V0_SIZE, 'Invalid Bech32 v0 address data size')
+        self.check_invalid(BECH32_TOO_LONG, 'Bech32 string too long', list(range(90, 108)))
+        self.check_invalid(BECH32_ONE_ERROR, 'Invalid checksum', [9])
+        self.check_invalid(BECH32_TWO_ERRORS, 'Invalid checksum', [22, 43])
+        self.check_invalid(BECH32_ONE_ERROR_CAPITALS, 'Invalid checksum', [38])
+        self.check_invalid(BECH32_NO_SEPARATOR, 'Missing separator')
+        self.check_invalid(BECH32_INVALID_CHAR, 'Invalid Base 32 character', [8])
+        self.check_invalid(BECH32_MULTISIG_TWO_ERRORS, 'Invalid checksum', [19, 30])
+        self.check_invalid(BECH32_WRONG_VERSION, 'Invalid checksum', [5])
+
+        # Valid Bech32
+        self.check_valid(BECH32_VALID)
+        self.check_valid(BECH32_VALID_CAPITALS)
+        self.check_valid(BECH32_VALID_MULTISIG)
+
+        # Invalid Base58
+        self.check_invalid(BASE58_INVALID_PREFIX, 'Invalid prefix for Base58-encoded address')
+        self.check_invalid(BASE58_INVALID_CHECKSUM, 'Invalid checksum or length of Base58 address')
+        self.check_invalid(BASE58_INVALID_LENGTH, 'Invalid checksum or length of Base58 address')
+
+        # Valid Base58
+        self.check_valid(BASE58_VALID)
 
         # Invalid address format
-        info = node.validateaddress(INVALID_ADDRESS)
-        assert not info['isvalid']
-        assert_equal(info['error'], 'Invalid address format')
+        self.check_invalid(INVALID_ADDRESS, 'Invalid HRP or Base58 character in address')
+        self.check_invalid(INVALID_ADDRESS_2, 'Invalid HRP or Base58 character in address')
 
     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 HRP or Base58 character in 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)
+        assert_raises_rpc_error(-5, "Invalid HRP or Base58 character in address", node.getaddressinfo, INVALID_ADDRESS)
 
     def run_test(self):
         self.test_validateaddress()