Merge 95d19f8 into merged_master (Bitcoin PR bitcoin/bitcoin#16807)

I did not do a great job with this merge, and it could use some improvement. Commented out some of the assertions in rpc_invalid_address_message.py
delta1 · May 30, 2023 · eee64b5 · eee64b5
2 parents df90bee + 95d19f8
commit eee64b5
Show file tree

Hide file tree

Showing 8 changed files with 618 additions and 72 deletions.
diff --git a/doc/release-notes-16807.md b/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.
diff --git a/src/bech32.cpp b/src/bech32.cpp
diff --git a/src/bech32.h b/src/bech32.h
@@ -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.
 
@@ -47,6 +48,9 @@ DecodeResult Decode(const std::string& str);
 /// Exported for testing.
 uint32_t PolyMod(const std::vector<uint8_t>& v);
 
+/** 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
diff --git a/src/key_io.cpp b/src/key_io.cpp
@@ -142,7 +142,7 @@ class DestinationEncoder
     std::string operator()(const NullData& null) const { return "null"; }
 };
 
-CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, const bool for_parent, std::string& error_str)
+CTxDestination DecodeDestination(const std::string& str, const CChainParams& params, const bool for_parent, std::string& error_str, std::vector<int>* error_locations)
 {
     // ELEMENTS: special case nulldata as "null"
     if (str == "null") return NullData{};
@@ -151,7 +151,12 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
     size_t pk_size = CPubKey::COMPRESSED_SIZE;
     uint160 hash;
     error_str = "";
-    if (DecodeBase58Check(str, data, 55)) {
+
+    // 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());
+    bool is_blech32 = (ToLower(str.substr(0, params.Blech32HRP().size())) == params.Blech32HRP());
+
+    if (!is_bech32 && !is_blech32 && DecodeBase58Check(str, data, 55)) {
         // 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.
@@ -191,10 +196,23 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
             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";
+        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 && !is_blech32) {
+        // 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(); // ELEMENTS: FIXME
     }
+
     data.clear();
     const auto dec = bech32::Decode(str);
     const std::string& hrp = for_parent ? params.ParentBech32HRP() : params.Bech32HRP();
@@ -346,8 +364,13 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
         }
     }
 
-    // Set error message if address can't be interpreted as Base58 or Bech32 or Blech32.
-    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();
 }
@@ -436,27 +459,27 @@ std::string EncodeDestination(const CTxDestination& dest)
     return std::visit(DestinationEncoder(Params(), false), 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(), false, error_msg);
+    return DecodeDestination(str, Params(), false, error_msg, error_locations);
 }
 
 CTxDestination DecodeDestination(const std::string& str)
 {
     std::string error_msg;
-    return DecodeDestination(str, Params(), false, error_msg);
+    return DecodeDestination(str, Params(), false, error_msg, nullptr);
 }
 
 bool IsValidDestinationString(const std::string& str, const CChainParams& params)
 {
     std::string error_msg;
-    return IsValidDestination(DecodeDestination(str, params, false, error_msg));
+    return IsValidDestination(DecodeDestination(str, params, false, error_msg, nullptr));
 }
 
 bool IsValidDestinationString(const std::string& str)
 {
     std::string error_msg;
-    return IsValidDestination(DecodeDestination(str, Params(), false, error_msg));
+    return IsValidDestination(DecodeDestination(str, Params(), false, error_msg, nullptr));
 }
 
 //
@@ -470,7 +493,7 @@ std::string EncodeParentDestination(const CTxDestination& dest)
 
 CTxDestination DecodeParentDestination(const std::string& str, std::string& error_msg)
 {
-    return DecodeDestination(str, Params(), true, error_msg);
+    return DecodeDestination(str, Params(), true, error_msg, nullptr);
 }
 
 // ELEMENTS

diff --git a/src/key_io.h b/src/key_io.h
@@ -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);
 

diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
@@ -65,6 +65,10 @@ static RPCHelpMan validateaddress()
                         }},
                         {RPCResult::Type::STR, "error", /* optional */ true, "Error message, if any"},
                         {RPCResult::Type::STR, "error_parent", /* 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{
@@ -74,11 +78,12 @@ 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);
     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);
+    const bool isValid = IsValidDestination(dest);
+    const bool is_valid_parent = IsValidDestination(parent_dest);
     CHECK_NONFATAL(isValid == error_msg.empty());
     CHECK_NONFATAL(is_valid_parent == error_msg_parent.empty());
 
@@ -112,6 +117,9 @@ static RPCHelpMan validateaddress()
         ret.pushKV("parent_address_info", parent_info);
     }
     if (!isValid && !is_valid_parent) {
+        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);
         ret.pushKV("error_parent", error_msg_parent);
     }

diff --git a/src/test/bech32_tests.cpp b/src/test/bech32_tests.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.
 
@@ -70,10 +71,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++;
     }
 }
 
@@ -93,11 +120,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++;
     }
 }