Skip to content

Commit

Permalink
Improved address decoding error accuracy.
Browse files Browse the repository at this point in the history 
A complete refactor of the previous pull to simplify logic and reduce the
chance of technical debt.

Co-authored-by: D++ <[email protected]>
  • Loading branch information
@russeree @dplusplus1024
russeree and dplusplus1024 committed Oct 4, 2024
1 parent 749977c commit a0d2f72
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 73 deletions.
104 changes: 66 additions & 38 deletions src/key_io.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,21 @@ class DestinationEncoder

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 = "";

// 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());
static const uint8_t MAX_BASE58_CHARS = 100;
static const uint8_t MAX_BASE58_CHECK_CHARS = 21;

std::vector<unsigned char> data, bech32_data;

auto [bech32_encoding, bech32_hrp, bech32_chars] = bech32::Decode(str);
auto [bech32_error, bech32_error_loc] = bech32::LocateErrors(str);

bool is_bech32 = bech32_encoding != bech32::Encoding::INVALID;
bool is_base58_check = DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS);

if (!is_bech32 && DecodeBase58Check(str, data, 21)) {
if (!is_bech32 && is_base58_check) {
uint160 hash;
// 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.
Expand All @@ -112,76 +119,98 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
std::equal(script_prefix.begin(), script_prefix.end(), data.begin())) ||
(data.size() >= pubkey_prefix.size() &&
std::equal(pubkey_prefix.begin(), pubkey_prefix.end(), data.begin()))) {
error_str = "Invalid length for Base58 address (P2PKH or P2SH)";
error_str = "Invalid length for Base58 address";
} else {
error_str = "Invalid or unsupported Base58-encoded address.";
const std::vector<std::string_view>& pubkey_prefixes = params.Base58EncodedPrefix(CChainParams::PUBKEY_ADDRESS);
const std::vector<std::string_view>& script_prefixes = params.Base58EncodedPrefix(CChainParams::SCRIPT_ADDRESS);

std::vector<std::string_view> encoded_prefixes;
encoded_prefixes.insert(encoded_prefixes.end(), script_prefixes.begin(), script_prefixes.end());
encoded_prefixes.insert(encoded_prefixes.end(), pubkey_prefixes.begin(), pubkey_prefixes.end());

std::string base58_address_prefixes;
for (size_t i = 0; i < encoded_prefixes.size(); ++i) {
if (i > 0) {
base58_address_prefixes += (i == encoded_prefixes.size() - 1) ? ", or " : ", ";
}
base58_address_prefixes += std::string(encoded_prefixes[i]); // Convert string_view to string
}

error_str = strprintf("Invalid Base58 %s address. Expected prefix %s", params.GetChainTypeDisplayString(), base58_address_prefixes);
}
return CNoDestination();
} else if (!is_bech32) {
bool is_base58 = DecodeBase58(str, data, MAX_BASE58_CHARS);
bool is_validBech32Chars = (bech32_error != "Invalid Base 32 character" &&
bech32_error != "Invalid character or mixed case");
// Try Base58 decoding without the checksum, using a much larger max length
if (!DecodeBase58(str, data, 100)) {
error_str = "Invalid or unsupported Segwit (Bech32) or Base58 encoding.";
if (!is_base58) {
// If bech32 decoding failed due to invalid base32 chars, address format is ambiguous; otherwise, report bech32 error
error_str = is_validBech32Chars ? "Bech32(m) address decoded with error: " + bech32_error : "Address is not valid Base58 or Bech32";
} else {
error_str = "Invalid checksum or length of Base58 address (P2PKH or P2SH)";
// This covers the case where an address is encoded as valid base58 and invalid bech32(m) due to a non base32 error
error_str = is_validBech32Chars ? "Invalid Base58 or Bech32(m) address" : "Invalid checksum or length of Base58 address";
}

if (is_validBech32Chars && error_locations) {
*error_locations = std::move(bech32_error_loc);
}

return CNoDestination();
}

data.clear();
const auto dec = bech32::Decode(str);
if (dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) {
if (dec.data.empty()) {
if (bech32_encoding == bech32::Encoding::BECH32 || bech32_encoding == bech32::Encoding::BECH32M) {
if (bech32_chars.empty()) {
error_str = "Empty Bech32 data section";
return CNoDestination();
}
// Bech32 decoding
if (dec.hrp != params.Bech32HRP()) {
error_str = strprintf("Invalid or unsupported prefix for Segwit (Bech32) address (expected %s, got %s).", params.Bech32HRP(), dec.hrp);
if (bech32_hrp != params.Bech32HRP()) {
error_str = strprintf("Invalid or unsupported prefix for %s address (expected %s, got %s)", params.GetChainTypeDisplayString(), params.Bech32HRP(), bech32_hrp);
return CNoDestination();
}
int version = dec.data[0]; // The first 5 bit symbol is the witness version (0-16)
if (version == 0 && dec.encoding != bech32::Encoding::BECH32) {
int version = bech32_chars[0]; // The first 5 bit symbol is the witness version (0-16)
if (version == 0 && bech32_encoding != bech32::Encoding::BECH32) {
error_str = "Version 0 witness address must use Bech32 checksum";
return CNoDestination();
}
if (version != 0 && dec.encoding != bech32::Encoding::BECH32M) {
if (version != 0 && bech32_encoding != bech32::Encoding::BECH32M) {
error_str = "Version 1+ witness address must use Bech32m checksum";
return CNoDestination();
}
// The rest of the symbols are converted witness program bytes.
data.reserve(((dec.data.size() - 1) * 5) / 8);
if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end())) {

std::string_view byte_str{data.size() == 1 ? "byte" : "bytes"};
bech32_data.reserve(((bech32_chars.size() - 1) * 5) / 8);
if (ConvertBits<5, 8, false>([&](unsigned char c) { bech32_data.push_back(c); }, bech32_chars.begin() + 1, bech32_chars.end())) {
std::string_view byte_str{bech32_data.size() == 1 ? "byte" : "bytes"};

if (version == 0) {
{
WitnessV0KeyHash keyid;
if (data.size() == keyid.size()) {
std::copy(data.begin(), data.end(), keyid.begin());
if (bech32_data.size() == keyid.size()) {
std::copy(bech32_data.begin(), bech32_data.end(), keyid.begin());
return keyid;
}
}
{
WitnessV0ScriptHash scriptid;
if (data.size() == scriptid.size()) {
std::copy(data.begin(), data.end(), scriptid.begin());
if (bech32_data.size() == scriptid.size()) {
std::copy(bech32_data.begin(), bech32_data.end(), scriptid.begin());
return scriptid;
}
}

error_str = strprintf("Invalid Bech32 v0 address program size (%d %s), per BIP141", data.size(), byte_str);
error_str = strprintf("Invalid SegWit v0 address program size (%d %s), per BIP141", bech32_data.size(), byte_str);
return CNoDestination();
}

if (version == 1 && data.size() == WITNESS_V1_TAPROOT_SIZE) {
if (version == 1 && bech32_data.size() == WITNESS_V1_TAPROOT_SIZE) {
static_assert(WITNESS_V1_TAPROOT_SIZE == WitnessV1Taproot::size());
WitnessV1Taproot tap;
std::copy(data.begin(), data.end(), tap.begin());
std::copy(bech32_data.begin(), bech32_data.end(), tap.begin());
return tap;
}

if (CScript::IsPayToAnchor(version, data)) {
if (CScript::IsPayToAnchor(version, bech32_data)) {
return PayToAnchor();
}

Expand All @@ -190,22 +219,21 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
return CNoDestination();
}

if (data.size() < 2 || data.size() > BECH32_WITNESS_PROG_MAX_LEN) {
error_str = strprintf("Invalid Bech32 address program size (%d %s)", data.size(), byte_str);
if (bech32_data.size() < 2 || bech32_data.size() > BECH32_WITNESS_PROG_MAX_LEN) {
error_str = strprintf("Invalid Bech32 address program size (%d %s)", bech32_data.size(), byte_str);
return CNoDestination();
}

return WitnessUnknown{version, data};
return WitnessUnknown{version, bech32_data};
} else {
error_str = strprintf("Invalid padding in Bech32 data section");
return CNoDestination();
}
}

// Perform Bech32 error location
auto res = bech32::LocateErrors(str);
error_str = res.first;
if (error_locations) *error_locations = std::move(res.second);
// Return results of Bech32(m) error location
error_str = bech32_error;
if (error_locations) *error_locations = std::move(bech32_error_loc);
return CNoDestination();
}
} // namespace
Expand Down
40 changes: 21 additions & 19 deletions test/functional/rpc_invalid_address_message.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@
BECH32_INVALID_VERSION = 'bcrt130xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqynjegk'
BECH32_INVALID_SIZE = 'bcrt1s0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v8n0nx0muaewav25430mtr'
BECH32_INVALID_V0_SIZE = 'bcrt1qw508d6qejxtdg4y5r3zarvary0c5xw7kqqq5k3my'
BECH32_INVALID_PREFIX = 'bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx'
BECH32_INVALID_PREFIX_WRONG_CHAIN = 'bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx'
BECH32_INVALID_PREFIX_NO_HRP = '1mk9q5tvu03k4qnfv6j2xjycx2ceah2zw5nek7czepflkd2a2xxrqk00j84'
BECH32_TOO_LONG = 'bcrt1q049edschfnwystcqnsvyfpj23mpsg3jcedq9xv049edschfnwystcqnsvyfpj23mpsg3jcedq9xv049edschfnwystcqnsvyfpj23m'
BECH32_ONE_ERROR = 'bcrt1q049edschfnwystcqnsvyfpj23mpsg3jcedq9xv'
BECH32_ONE_ERROR_CAPITALS = 'BCRT1QPLMTZKC2XHARPPZDLNPAQL78RSHJ68U32RAH7R'
Expand Down Expand Up @@ -65,19 +66,20 @@ def check_invalid(self, addr, error_str, error_locations=None):
def test_validateaddress(self):
# Invalid Bech32
self.check_invalid(BECH32_INVALID_SIZE, "Invalid Bech32 address program size (41 bytes)")
self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid or unsupported Segwit (Bech32) or Base58 encoding.')
self.check_invalid(BECH32_INVALID_PREFIX_WRONG_CHAIN, 'Invalid or unsupported prefix for regtest address (expected bcrt, got bc)')
self.check_invalid(BECH32_INVALID_PREFIX_NO_HRP, 'Bech32(m) address decoded with error: Invalid separator position',[0])
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 program size (21 bytes), per BIP141")
self.check_invalid(BECH32_TOO_LONG, 'Bech32 string too long', list(range(90, 108)))
self.check_invalid(BECH32_ONE_ERROR, 'Invalid Bech32 checksum', [9])
self.check_invalid(BECH32_TWO_ERRORS, 'Invalid Bech32 checksum', [22, 43])
self.check_invalid(BECH32_ONE_ERROR_CAPITALS, 'Invalid Bech32 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 Bech32 checksum', [19, 30])
self.check_invalid(BECH32_WRONG_VERSION, 'Invalid Bech32 checksum', [5])
self.check_invalid(BECH32_INVALID_V0_SIZE, "Invalid SegWit v0 address program size (21 bytes), per BIP141")
self.check_invalid(BECH32_TOO_LONG, 'Bech32(m) address decoded with error: Bech32 string too long', list(range(90, 108)))
self.check_invalid(BECH32_ONE_ERROR, 'Bech32(m) address decoded with error: Invalid Bech32 checksum', [9])
self.check_invalid(BECH32_TWO_ERRORS, 'Bech32(m) address decoded with error: Invalid Bech32 checksum', [22, 43])
self.check_invalid(BECH32_ONE_ERROR_CAPITALS, 'Invalid Base58 or Bech32(m) address', [38])
self.check_invalid(BECH32_NO_SEPARATOR, 'Bech32(m) address decoded with error: Missing separator')
self.check_invalid(BECH32_INVALID_CHAR, 'Address is not valid Base58 or Bech32', [])
self.check_invalid(BECH32_MULTISIG_TWO_ERRORS, 'Bech32(m) address decoded with error: Invalid Bech32 checksum', [19, 30])
self.check_invalid(BECH32_WRONG_VERSION, 'Bech32(m) address decoded with error: Invalid Bech32 checksum', [5])

# Valid Bech32
self.check_valid(BECH32_VALID)
Expand All @@ -86,16 +88,16 @@ def test_validateaddress(self):
self.check_valid(BECH32_VALID_MULTISIG)

# Invalid Base58
self.check_invalid(BASE58_INVALID_PREFIX, 'Invalid or unsupported Base58-encoded address.')
self.check_invalid(BASE58_INVALID_CHECKSUM, 'Invalid checksum or length of Base58 address (P2PKH or P2SH)')
self.check_invalid(BASE58_INVALID_LENGTH, 'Invalid checksum or length of Base58 address (P2PKH or P2SH)')
self.check_invalid(BASE58_INVALID_PREFIX, 'Invalid Base58 regtest address. Expected prefix 2, m, or n')
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
self.check_invalid(INVALID_ADDRESS, 'Invalid or unsupported Segwit (Bech32) or Base58 encoding.')
self.check_invalid(INVALID_ADDRESS_2, 'Invalid or unsupported Segwit (Bech32) or Base58 encoding.')
self.check_invalid(INVALID_ADDRESS, 'Bech32(m) address decoded with error: Invalid separator position', [14])
self.check_invalid(INVALID_ADDRESS_2, 'Bech32(m) address decoded with error: Invalid separator position', [0])

node = self.nodes[0]

Expand All @@ -108,9 +110,9 @@ def test_getaddressinfo(self):
node = self.nodes[0]

assert_raises_rpc_error(-5, "Invalid Bech32 address program size (41 bytes)", node.getaddressinfo, BECH32_INVALID_SIZE)
assert_raises_rpc_error(-5, "Invalid or unsupported Segwit (Bech32) or Base58 encoding.", node.getaddressinfo, BECH32_INVALID_PREFIX)
assert_raises_rpc_error(-5, "Invalid or unsupported Base58-encoded address.", node.getaddressinfo, BASE58_INVALID_PREFIX)
assert_raises_rpc_error(-5, "Invalid or unsupported Segwit (Bech32) or Base58 encoding.", node.getaddressinfo, INVALID_ADDRESS)
assert_raises_rpc_error(-5, "Invalid or unsupported prefix for regtest address (expected bcrt, got bc)", node.getaddressinfo, BECH32_INVALID_PREFIX_WRONG_CHAIN)
assert_raises_rpc_error(-5, "Invalid Base58 regtest address. Expected prefix 2, m, or n", node.getaddressinfo, BASE58_INVALID_PREFIX)
assert_raises_rpc_error(-5, "Bech32(m) address decoded with error: Invalid separator position", node.getaddressinfo, INVALID_ADDRESS)
assert "isscript" not in node.getaddressinfo(BECH32_VALID_UNKNOWN_WITNESS)

def run_test(self):
Expand Down
30 changes: 15 additions & 15 deletions test/functional/rpc_validateaddress.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@
# BIP 173
(
"tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # Invalid hrp
"Invalid or unsupported prefix for Bitcoin address (expected bc, got tc)", # Invalid hrp
[],
),
("bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t5", "Invalid Bech32 checksum", [41]),
("bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t5", "Bech32(m) address decoded with error: Invalid Bech32 checksum", [41]),
(
"BC13W508D6QEJXTDG4Y5R3ZARVARY0C5XW7KN40WF2",
"Version 1+ witness address must use Bech32m checksum",
Expand All @@ -33,18 +33,18 @@
),
(
"BC1QR508D6QEJXTDG4Y5R3ZARVARYV98GJ9P",
"Invalid Bech32 v0 address program size (16 bytes), per BIP141",
"Invalid SegWit v0 address program size (16 bytes), per BIP141",
[],
),
(
"tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sL5k7",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # tb1, Mixed case
"Address is not valid Base58 or Bech32", # tb1, Mixed case
[],
),
(
"BC1QW508D6QEJXTDG4Y5R3ZARVARY0C5XW7KV8F3t4",
"Invalid character or mixed case", # bc1, Mixed case, not in BIP 173 test vectors
[40],
"Address is not valid Base58 or Bech32", # bc1, Mixed case, not in BIP 173 test vectors
[],
),
(
"bc1zw508d6qejxtdg4y5r3zarvaryvqyzf3du",
Expand All @@ -53,14 +53,14 @@
),
(
"tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3pjxtptv",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # tb1, Non-zero padding in 8-to-5 conversion
"Invalid or unsupported prefix for Bitcoin address (expected bc, got tb)", # tb1, Non-zero padding in 8-to-5 conversion
[],
),
("bc1gmk9yu", "Empty Bech32 data section", []),
# BIP 350
(
"tc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq5zuyut",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # Invalid human-readable part
"Invalid or unsupported prefix for Bitcoin address (expected bc, got tc)", # Invalid human-readable part
[],
),
(
Expand All @@ -70,7 +70,7 @@
),
(
"tb1z0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqglt7rf",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # tb1, Invalid checksum (Bech32 instead of Bech32m)
"Invalid or unsupported prefix for Bitcoin address (expected bc, got tb)", # tb1, Invalid checksum (Bech32 instead of Bech32m)
[],
),
(
Expand All @@ -85,13 +85,13 @@
),
(
"tb1q0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq24jc47",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # tb1, Invalid checksum (Bech32m instead of Bech32)
"Invalid or unsupported prefix for Bitcoin address (expected bc, got tb)", # tb1, Invalid checksum (Bech32m instead of Bech32)
[],
),
(
"bc1p38j9r5y49hruaue7wxjce0updqjuyyx0kh56v8s25huc6995vvpql3jow4",
"Invalid Base 32 character", # Invalid character in checksum
[59],
"Address is not valid Base58 or Bech32", # Invalid character in checksum
[],
),
(
"BC130XLXVLHEMJA6C4DQV22UAPCTQUPFHLXM9H8Z3K2E72Q4K9HCZ7VQ7ZWS8R",
Expand All @@ -106,12 +106,12 @@
),
(
"BC1QR508D6QEJXTDG4Y5R3ZARVARYV98GJ9P",
"Invalid Bech32 v0 address program size (16 bytes), per BIP141",
"Invalid SegWit v0 address program size (16 bytes), per BIP141",
[],
),
(
"tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq47Zagq",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # tb1, Mixed case
"Address is not valid Base58 or Bech32", # tb1, Mixed case
[],
),
(
Expand All @@ -121,7 +121,7 @@
),
(
"tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vpggkg4j",
"Invalid or unsupported Segwit (Bech32) or Base58 encoding.", # tb1, Non-zero padding in 8-to-5 conversion
"Invalid or unsupported prefix for Bitcoin address (expected bc, got tb)", # tb1, Non-zero padding in 8-to-5 conversion
[],
),
("bc1gmk9yu", "Empty Bech32 data section", []),
Expand Down
Loading

0 comments on commit a0d2f72

Please sign in to comment.