From 4844fa4a91125c355abdce6fe8b67eecb16cffa0 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 4 May 2021 09:44:34 +0200 Subject: [PATCH 01/11] Merge bitcoin/bitcoin#21825: net: add I2P hardcoded seeds 142e2da4401aa8c0643f7fc473ffaceafbbf90c3 net: add I2P seeds to chainparamsseeds (Jon Atack) e01f173fb9b9d35729c700199ba6cf1c028fcaaf contrib: add a few I2P seed nodes (Jon Atack) ea269c7ef1a65960d680f405de21708ba477ecce contrib: parse I2P addresses in generate-seeds.py (Jon Atack) Pull request description: Follow-up to #21560 that updated the fixed seeds infra for BIP155 addresses and then added Tor v3 ones: - Update contrib/generate-seeds.py to parse I2P addresses - Add a few I2P nodes to contrib/seeds/nodes_main.txt - Run generate-seeds.py and add the I2P seeds to chainparamsseeds.h Reviewers, see contrib/seeds/README.md for more info and feel free to use the following CLI one-liner to check for and propose additional seeds for contrib/seeds/nodes_main.txt. You can also see how many I2P peers your node knows with cli -addrinfo. ```rake bitcoin-cli getnodeaddresses 0 | jq '.[] | (select(.address | contains(".b32.i2p"))) | .address' | sort ``` I verified the I2P addresses are correctly BIP155-serialized/deserialized by building with all seeds removed from chainparamsseeds.h except those added here, restarting with `-datadir=newdir -dnsseed=0` and running rpc ` getnodeaddresses 0` that initially returns only the new I2P addresses. ACKs for top commit: laanwj: ACK 142e2da4401aa8c0643f7fc473ffaceafbbf90c3 vasild: ACK 142e2da4401aa8c0643f7fc473ffaceafbbf90c3 Tree-SHA512: 040576012d5f1f034e2bd566ad654a6fdfd8ff7f6b12fa40c9fda1e948ebf8417fcea64cfc14938a41439370aa4669bab3e97274f9d4f9a6906fa9520afa9cf8 Co-authored-by: W. J. van der Laan --- contrib/seeds/generate-seeds.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/contrib/seeds/generate-seeds.py b/contrib/seeds/generate-seeds.py index 2d92718ba73f..05ad6e6021c0 100755 --- a/contrib/seeds/generate-seeds.py +++ b/contrib/seeds/generate-seeds.py @@ -16,6 +16,7 @@ : []: .onion: + .b32.i2p: The output will be two data structures with the peers in binary format: @@ -52,6 +53,12 @@ def name_to_bip155(addr): return (BIP155Network.TORV2, vchAddr) else: raise ValueError('Invalid onion %s' % vchAddr) + elif addr.endswith('.b32.i2p'): + vchAddr = b32decode(addr[0:-8] + '====', True) + if len(vchAddr) == 32: + return (BIP155Network.I2P, vchAddr) + else: + raise ValueError(f'Invalid I2P {vchAddr}') elif '.' in addr: # IPv4 return (BIP155Network.IPV4, bytes((int(x) for x in addr.split('.')))) elif ':' in addr: # IPv6 or CJDNS From 79a32d0664abb590997894e8c8aff45a51105085 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 24 Jun 2021 12:47:04 +0200 Subject: [PATCH 02/11] refactor: follow-up dashify for Merge bitcoin/bitcoin#22257 --- test/functional/rpc_verifyislock.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/functional/rpc_verifyislock.py b/test/functional/rpc_verifyislock.py index 40aff04c162b..efdb3d73f9d1 100755 --- a/test/functional/rpc_verifyislock.py +++ b/test/functional/rpc_verifyislock.py @@ -3,7 +3,7 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. -from test_framework.messages import CTransaction, from_hex, hash256, ser_compact_size, ser_string +from test_framework.messages import tx_from_hex, hash256, ser_compact_size, ser_string from test_framework.test_framework import DashTestFramework from test_framework.util import assert_equal, assert_raises_rpc_error, satoshi_round @@ -23,7 +23,7 @@ def set_test_params(self): self.set_dash_test_params(6, 5, [["-whitelist=127.0.0.1"], [], [], [], [], []]) def get_request_id(self, tx_hex): - tx = from_hex(CTransaction(), tx_hex) + tx = tx_from_hex(tx_hex) request_id_buf = ser_string(b"islock") + ser_compact_size(len(tx.vin)) for txin in tx.vin: From 4a0b67e277f899a74c046499826b51c477e2302e Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 22 Nov 2021 12:37:41 +0100 Subject: [PATCH 03/11] Merge bitcoin/bitcoin#16807: Let validateaddress locate error in Bech32 address 88cc4810926e4f5af6757ee1b0eed61abda3d746 Modify copyright header on Bech32 code (Samuel Dobson) 5599813b80e53a1539c66625b4320ab1b4fb4848 Add lots of comments to Bech32 (Samuel Dobson) 2eb5792ec7bbeaf7138420b6c85c5cd0a0404946 Add release notes for validateaddress Bech32 error detection (MeshCollider) 42d6a029e57a32f2d1d829ff7718b6d40d58b9d1 Refactor and add more tests for validateaddress (Samuel Dobson) c4979f77c1264f0099d1dfa278b1d9c18340b5f9 Add boost tests for bech32 error detection (MeshCollider) 02a7bdee429ae307a5e57832727fed789e2e04fb Add error_locations to validateaddress RPC (Samuel Dobson) b62b67e06cc406fdad68da4c091168fb5f11c1d4 Add Bech32 error location function (Samuel Dobson) 0b06e720c0182dee8b560d2e8d3891b036f63ea7 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 88cc4810926e4f5af6757ee1b0eed61abda3d746 ryanofsky: Code review ACK 88cc4810926e4f5af6757ee1b0eed61abda3d746 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 Dash note: this is the remaining, Dash-side part of the backport. The Bech32 library part (src/bech32.{cpp,h}, src/test/bech32_tests.cpp) was backported earlier in ce6d7cbd03, already including the #23577 follow-ups (bd1186e9c5), so bech32::LocateErrors is used here in its final pair-returning form instead of the out-arg form this PR introduced upstream. Dash L1 has no Bech32 addresses: a string using the Bech32m HRP ("dash" on mainnet, "tdash" on the test chains) can only be a DIP-18 Dash Platform address, which never encodes an L1 destination. The upstream witness version/size checks are therefore replaced by DecodePlatformDestination(), so that a well-formed Platform address, or a Bech32m string with a broken DIP-18 payload, is reported with a specific reason rather than an empty error (validateaddress requires isvalid == error.empty()). The functional test drops the SegWit-only cases for the same reason and covers the DIP-18 equivalents instead; its Bech32m constants are built from the DIP-0018 test vectors with the helpers in test_framework/segwit_addr.py. Co-authored-by: W. J. van der Laan --- doc/release-notes-16807.md | 6 ++ src/key_io.cpp | 46 +++++++-- src/key_io.h | 2 +- src/rpc/output_script.cpp | 10 +- .../functional/rpc_invalid_address_message.py | 97 ++++++++++++++++--- 5 files changed, 137 insertions(+), 24 deletions(-) create mode 100644 doc/release-notes-16807.md diff --git a/doc/release-notes-16807.md b/doc/release-notes-16807.md new file mode 100644 index 000000000000..5027550a9955 --- /dev/null +++ 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. \ No newline at end of file diff --git a/src/key_io.cpp b/src/key_io.cpp index b90c08bb13dd..73774ad11321 100644 --- a/src/key_io.cpp +++ b/src/key_io.cpp @@ -39,12 +39,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* error_locations) { std::vector 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.Bech32PlatformHRP().size())) == params.Bech32PlatformHRP()); + + if (!is_bech32 && DecodeBase58Check(str, data, 21)) { // base58-encoded Dash addresses. // Public-key-hash-addresses have version 76 (or 140 testnet). // The data vector contains RIPEMD160(SHA256(pubkey)), where pubkey is the serialized public key. @@ -61,12 +65,36 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par return ScriptHash(hash); } - // Set potential error message. - 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(); } - // Set error message if address can't be interpreted as Base58. - if (error_str.empty()) error_str = "Invalid address format"; + // Dash has no Bech32m encoding for L1 destinations: a string using the Platform + // HRP can only be a DIP-18 Platform address, which never encodes an L1 destination. + // Decode it anyway to tell the user why exactly it got rejected. + std::string platform_error_str; + const bool is_platform = IsValidPlatformDestination(DecodePlatformDestination(str, params, platform_error_str)); + + // Perform Bech32 error location + auto res = bech32::LocateErrors(str); + error_str = res.first; + if (error_locations) *error_locations = std::move(res.second); + if (error_str.empty()) { + error_str = is_platform ? "This is a Dash Platform address, not a Dash Core address" : platform_error_str; + } return CNoDestination(); } } // namespace @@ -156,9 +184,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* error_locations) { - return DecodeDestination(str, Params(), error_msg); + return DecodeDestination(str, Params(), error_msg, error_locations); } CTxDestination DecodeDestination(const std::string& str) @@ -170,7 +198,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) diff --git a/src/key_io.h b/src/key_io.h index 11bbe5cd6fb1..0b12df251f5e 100644 --- a/src/key_io.h +++ b/src/key_io.h @@ -24,7 +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); +CTxDestination DecodeDestination(const std::string& str, std::string& error_msg, std::vector* error_locations = nullptr); bool IsValidDestinationString(const std::string& str); bool IsValidDestinationString(const std::string& str, const CChainParams& params); diff --git a/src/rpc/output_script.cpp b/src/rpc/output_script.cpp index 2ad98f709d38..540dca2437dd 100644 --- a/src/rpc/output_script.cpp +++ b/src/rpc/output_script.cpp @@ -39,6 +39,10 @@ static RPCHelpMan validateaddress() {RPCResult::Type::STR_HEX, "scriptPubKey", /*optional=*/true, "The hex-encoded scriptPubKey generated by the address"}, {RPCResult::Type::BOOL, "isscript", /*optional=*/true, "If the key is a script"}, {RPCResult::Type::STR, "error", /*optional=*/true, "Error message, if any"}, + {RPCResult::Type::ARR, "error_locations", /*optional=*/true, "Indices of likely error locations in address, if known (e.g. Bech32 errors)", + { + {RPCResult::Type::NUM, "index", "index of a potential error"}, + }}, } }, RPCExamples{ @@ -48,7 +52,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 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()); @@ -64,6 +69,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); } diff --git a/test/functional/rpc_invalid_address_message.py b/test/functional/rpc_invalid_address_message.py index 2f6df8570c94..d1a815e846b3 100755 --- a/test/functional/rpc_invalid_address_message.py +++ b/test/functional/rpc_invalid_address_message.py @@ -6,16 +6,51 @@ from test_framework.test_framework import BitcoinTestFramework +from test_framework.segwit_addr import ( + DIP18_TYPE_P2PKH, + Encoding, + bech32_encode, + convertbits, +) from test_framework.util import ( assert_equal, assert_raises_rpc_error, ) +PLATFORM_HRP = 'tdash' +PLATFORM_KEYHASH = bytes.fromhex('f7da0a2b5cbd4ff6bb2c4d89b67d2f3ffeec0525') + + +def platform_address(encoding, type_byte, payload): + return bech32_encode(encoding, PLATFORM_HRP, convertbits([type_byte] + list(payload), 8, 5)) + + +# DIP-18 Platform addresses (test vectors of DIP-0018): valid Bech32m, but never Dash Core addresses +BECH32_VALID = 'tdash1krma5z3ttj75la4m93xcndna9ullamq9y5fzq2j7' +BECH32_VALID_CAPITALS = 'TDASH1KRMA5Z3TTJ75LA4M93XCNDNA9ULLAMQ9Y5FZQ2J7' +BECH32_VALID_P2SH = 'tdash1sppl5xpu70aka8nacc4kj2htflydspzkxc8jtru5' + +# Well-formed Bech32(m) strings whose DIP-18 payload is invalid +BECH32_INVALID_ENCODING = platform_address(Encoding.BECH32, DIP18_TYPE_P2PKH, PLATFORM_KEYHASH) +BECH32_INVALID_TYPE_BYTE = platform_address(Encoding.BECH32M, 0x00, PLATFORM_KEYHASH) +BECH32_INVALID_SIZE = platform_address(Encoding.BECH32M, DIP18_TYPE_P2PKH, PLATFORM_KEYHASH[:-1]) + +BECH32_INVALID_PREFIX = 'bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx' +BECH32_TOO_LONG = 'tdash1krma5z3ttj75la4m93xcndna9ullamq9y5fzq2j7krma5z3ttj75la4m93xcndna9ullamq9y5fzq2j7krma5z3ttj75la4m93xcndna9ullamq9y5fzq2j7' +BECH32_ONE_ERROR = 'tdash1krma4z3ttj75la4m93xcndna9ullamq9y5fzq2j7' +BECH32_ONE_ERROR_CAPITALS = 'TDASH1KRMA5Z3TTJ75LA4M93XCNDNA9ULLAMQ9Y4FZQ2J7' +BECH32_TWO_ERRORS = 'tdash1krma4z3ttj75la4m93xcndna8ullamq9y5fzq2j7' # should be tdash1krma5z3ttj75la4m93xcndna9ullamq9y5fzq2j7 +BECH32_P2SH_TWO_ERRORS = 'tdash1sppl5xpu70aka8nacd4kj2htflydspzkxc8jtrs5' # should be tdash1sppl5xpu70aka8nacc4kj2htflydspzkxc8jtru5 +BECH32_NO_SEPARATOR = 'tdashkrma5z3ttj75la4m93xcndna9ullamq9y5fzq2j7' +BECH32_INVALID_CHAR = 'tdash1krmo5z3ttj75la4m93xcndna9ullamq9y5fzq2j7' BASE58_VALID = 'yjQ5gLvGRtmq1cwc4kePLCrzQ8GVCh9Gaz' BASE58_INVALID_PREFIX = 'XpG61qAVhdyN7AqVZQsHfJL7AEk4dPVinc' +BASE58_INVALID_CHECKSUM = 'yjQ5gLvGRtmq1cwc4kePLCrzQ8GVCh9Gaa' +BASE58_INVALID_LENGTH = '2VKf7XKMrp4bVNVmuRbyCewkP8FhGLP2E54LHDPakr9Sq5mtU2' INVALID_ADDRESS = 'asfah14i8fajz0123f' +INVALID_ADDRESS_2 = '1q049ldschfnwystcqnsvyfpj23mpsg3jcedq9xv' class InvalidAddressErrorMessageTest(BitcoinTestFramework): def add_options(self, parser): @@ -25,28 +60,64 @@ def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 1 - def test_validateaddress(self): - node = self.nodes[0] - - # 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) + 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 + + 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'], []) + + def test_validateaddress(self): + # Invalid Bech32 + self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid HRP or Base58 character in address') + self.check_invalid(BECH32_TOO_LONG, 'Bech32 string too long', list(range(90, len(BECH32_TOO_LONG)))) + self.check_invalid(BECH32_ONE_ERROR, 'Invalid Bech32m checksum', [10]) + self.check_invalid(BECH32_TWO_ERRORS, 'Invalid Bech32m checksum', [10, 30]) + self.check_invalid(BECH32_ONE_ERROR_CAPITALS, 'Invalid Bech32m checksum', [39]) + self.check_invalid(BECH32_NO_SEPARATOR, 'Missing separator') + self.check_invalid(BECH32_INVALID_CHAR, 'Invalid Base 32 character', [9]) + self.check_invalid(BECH32_P2SH_TWO_ERRORS, 'Invalid Bech32m checksum', [23, 44]) + + # Bech32 strings with a valid checksum but an invalid DIP-18 Platform payload + self.check_invalid(BECH32_INVALID_ENCODING, 'DIP-18 Platform addresses require bech32m checksum') + self.check_invalid(BECH32_INVALID_TYPE_BYTE, 'Unknown DIP-18 type byte') + self.check_invalid(BECH32_INVALID_SIZE, 'Invalid Platform address payload length') + + # Valid Bech32m: Platform addresses are not Dash Core addresses + self.check_invalid(BECH32_VALID, 'This is a Dash Platform address, not a Dash Core address') + self.check_invalid(BECH32_VALID_CAPITALS, 'This is a Dash Platform address, not a Dash Core address') + self.check_invalid(BECH32_VALID_P2SH, 'This is a Dash Platform address, not a Dash Core address') + + # 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 Platform address payload length", node.getaddressinfo, BECH32_INVALID_SIZE) + + 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() From a802b97674b63b2d17bdf87455a41e12b3b566c4 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 6 Dec 2021 10:37:53 +0100 Subject: [PATCH 04/11] Merge bitcoin/bitcoin#23577: Follow-ups to Bech32 error detection a4fe70171b6fa570eda71d86b59d0fb24c2f0614 Make Bech32 LocateErrors return error list rather than using out-arg (Samuel Dobson) 2fa4fd196176160a5ad0a25da173ff93252b8103 Use std::iota instead of manually pushing range (Samuel Dobson) 405c96fc9fd909ccc461f10d55dfdd822b76f5bf Use bounds-checked array lookups in Bech32 error detection code (Samuel Dobson) 28d9c2857f1c430069bffe0547d12800c84ed9ec Simplify encoding of e in GF(1024) tables to (1,0) (Samuel Dobson) 14358a029def2334ac60d6eb630c60db6dc06f9d Replace GF1024 tables and syndrome constants with compile-time generated constexprs. (Samuel Dobson) 63f7b6977989b93e13c3afd8dfd22b524842b9d7 Update release note for bech32 error detection (Samuel Dobson) c8b9a224e70f70ccc638b2c4200a505cdf024efd Report encoding type in bech32 error message (Samuel Dobson) 92f0cafdca11a9463b6f04229c1c47805c97c1b5 Improve Bech32 boost tests (Samuel Dobson) bb4d3e9b970be2a8de3e146623801fc8cbbeb0c7 Address review comments for Bech32 error validation (Samuel Dobson) Pull request description: A number of follow-ups and improvements to the bech32 error location code, introduced in #16807. Notably, this removes the hardcoded GF1024 tables in favour of constexpr table generation. ACKs for top commit: laanwj: Re-ACK a4fe70171b6fa570eda71d86b59d0fb24c2f0614 Tree-SHA512: 6312373c20ebd6636f5797304876fa0d70fa777de2f6c507245f51a652b3d1224ebc55b236c9e11e6956c1e88e65faadab51d53587078efccb451455aa2e2276 Dash note: this is the remaining, Dash-side part of the backport; the src/bech32.{cpp,h} and src/test/bech32_tests.cpp changes were backported earlier in bd1186e9c5. The "Make Bech32 LocateErrors return error list rather than using out-arg" hunk for src/key_io.cpp is not needed either, because Dash never had the intermediate out-arg API. "Not a valid Bech32 or Base58 encoding" is spelled "Not a valid Bech32m or Base58 encoding" here: Bech32m (DIP-18 Platform addresses) is the only Bech32-family encoding Dash accepts for addresses. Co-authored-by: W. J. van der Laan --- doc/release-notes-16807.md | 10 +++++++--- src/key_io.cpp | 13 ++++++++----- test/functional/rpc_invalid_address_message.py | 10 +++++----- 3 files changed, 20 insertions(+), 13 deletions(-) diff --git a/doc/release-notes-16807.md b/doc/release-notes-16807.md index 5027550a9955..3cdd0b36af5a 100644 --- a/doc/release-notes-16807.md +++ b/doc/release-notes-16807.md @@ -1,6 +1,10 @@ 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. \ No newline at end of file +- The `validateaddress` RPC now returns an `error_locations` array for invalid +addresses, with the indices of invalid character locations in the address (if +known). For example, this will attempt to locate up to two Bech32 errors, and +return their locations if successful. Success and correctness are only guaranteed +if fewer than two substitution errors have been made. +The error message returned in the `error` field now also returns more specific +errors when decoding fails. \ No newline at end of file diff --git a/src/key_io.cpp b/src/key_io.cpp index 73774ad11321..0962759f6020 100644 --- a/src/key_io.cpp +++ b/src/key_io.cpp @@ -65,17 +65,20 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par return ScriptHash(hash); } - 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 { + // If the prefix of data matches either the script or pubkey prefix, the length must have been wrong + if ((data.size() >= script_prefix.size() && + 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"; + } else { + error_str = "Invalid prefix for Base58-encoded 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"; + error_str = "Not a valid Bech32m or Base58 encoding"; } else { error_str = "Invalid checksum or length of Base58 address"; } diff --git a/test/functional/rpc_invalid_address_message.py b/test/functional/rpc_invalid_address_message.py index d1a815e846b3..59e9980aaada 100755 --- a/test/functional/rpc_invalid_address_message.py +++ b/test/functional/rpc_invalid_address_message.py @@ -77,7 +77,7 @@ def check_invalid(self, addr, error_str, error_locations=None): def test_validateaddress(self): # Invalid Bech32 - self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid HRP or Base58 character in address') + self.check_invalid(BECH32_INVALID_PREFIX, 'Not a valid Bech32m or Base58 encoding') self.check_invalid(BECH32_TOO_LONG, 'Bech32 string too long', list(range(90, len(BECH32_TOO_LONG)))) self.check_invalid(BECH32_ONE_ERROR, 'Invalid Bech32m checksum', [10]) self.check_invalid(BECH32_TWO_ERRORS, 'Invalid Bech32m checksum', [10, 30]) @@ -105,19 +105,19 @@ def test_validateaddress(self): self.check_valid(BASE58_VALID) # 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') + self.check_invalid(INVALID_ADDRESS, 'Not a valid Bech32m or Base58 encoding') + self.check_invalid(INVALID_ADDRESS_2, 'Not a valid Bech32m or Base58 encoding') def test_getaddressinfo(self): node = self.nodes[0] assert_raises_rpc_error(-5, "Invalid Platform address payload length", node.getaddressinfo, BECH32_INVALID_SIZE) - assert_raises_rpc_error(-5, "Invalid HRP or Base58 character in address", node.getaddressinfo, BECH32_INVALID_PREFIX) + assert_raises_rpc_error(-5, "Not a valid Bech32m or Base58 encoding", 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 HRP or Base58 character in address", node.getaddressinfo, INVALID_ADDRESS) + assert_raises_rpc_error(-5, "Not a valid Bech32m or Base58 encoding", node.getaddressinfo, INVALID_ADDRESS) def run_test(self): self.test_validateaddress() From da7dbee22c8644c93aa8f7a3ab96546357a807e6 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 8 Feb 2022 13:19:37 +0000 Subject: [PATCH 05/11] Merge bitcoin/bitcoin#24259: test: Remove unused valgrind suppressions fa4b61911d54840e9a24bfcabafec159f013ee9a test: Remove unused valgrind suppressions (MarcoFalke) faccb2d7fe3c03f8d9c8a9078d1608948b4f62b0 test: Exclude broken feature_init for now (MarcoFalke) fa086d891b912d30fd4b8748ef4fd816ffad51d7 test: Properly skip feature_syscall_sandbox in valgrind (MarcoFalke) Pull request description: ACKs for top commit: fanquake: ACK fa4b61911d54840e9a24bfcabafec159f013ee9a Tree-SHA512: 5be1a8f288182d386531a033ae7258f753dd655dfa1746a52b65622a0359c2b7143a25b49c0747538308eed606a691847d2f59a5a0382b7751b8de7172adf0d3 Co-authored-by: fanquake --- ci/test/00_setup_env_native_valgrind.sh | 2 +- ci/test/00_setup_env_s390x.sh | 2 +- contrib/valgrind.supp | 21 ++------------------- 3 files changed, 4 insertions(+), 21 deletions(-) diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh index 478a347d8c9d..70957cee0269 100755 --- a/ci/test/00_setup_env_native_valgrind.sh +++ b/ci/test/00_setup_env_native_valgrind.sh @@ -9,7 +9,7 @@ export LC_ALL=C.UTF-8 export PACKAGES="valgrind clang llvm libclang-rt-dev python3-zmq libevent-dev bsdmainutils libboost-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev" export USE_VALGRIND=1 export NO_DEPENDS=1 -export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra --timeout-factor=4" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 +export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra --timeout-factor=4" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 export GOAL="install" # Temporarily pin dwarf 4, until valgrind can understand clang's dwarf 5 export BITCOIN_CONFIG="--enable-zmq --with-incompatible-bdb --with-gui=no CC=clang-19 CXX=clang++-19 CFLAGS='-gdwarf-4' CXXFLAGS='-gdwarf-4'" # TODO enable GUI diff --git a/ci/test/00_setup_env_s390x.sh b/ci/test/00_setup_env_s390x.sh index 2bb4d5e57562..bd343888f923 100755 --- a/ci/test/00_setup_env_s390x.sh +++ b/ci/test/00_setup_env_s390x.sh @@ -19,7 +19,7 @@ fi # Use debian to avoid 404 apt errors export CONTAINER_NAME=ci_s390x export RUN_UNIT_TESTS=true -export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 +export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 export RUN_FUNCTIONAL_TESTS=true export GOAL="install" export BITCOIN_CONFIG="--enable-reduce-exports" diff --git a/contrib/valgrind.supp b/contrib/valgrind.supp index 20e03849e15d..4f97adc538b9 100644 --- a/contrib/valgrind.supp +++ b/contrib/valgrind.supp @@ -13,8 +13,8 @@ # # Note that suppressions may depend on OS and/or library versions. # Tested on: -# * aarch64 (Ubuntu 20.04 system libs, without gui) -# * x86_64 (Ubuntu 18.04 system libs, without gui) +# * aarch64 (Ubuntu 22.04 system libs, clang, without gui) +# * x86_64 (Ubuntu 22.04 system libs, clang, without gui) { Suppress libstdc++ warning - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65434 Memcheck:Leak @@ -91,12 +91,6 @@ ... fun:GetCoin } -{ - Suppress wcsnrtombs glibc SSE4 warning (could be related: https://stroika.atlassian.net/browse/STK-626) - Memcheck:Addr16 - fun:__wcsnlen_sse4_1 - fun:wcsnrtombs -} { Suppress boost warning Memcheck:Leak @@ -107,17 +101,6 @@ fun:_ZN5boost9unit_test14unit_test_mainEPFbvEiPPc fun:main } -{ - Suppress boost still reachable memory warning - Memcheck:Leak - match-leak-kinds: reachable - fun:_Znwm - ... - fun:_M_construct_aux - fun:_M_construct - fun:basic_string - fun:path -} { Suppress LogInstance still reachable memory warning Memcheck:Leak From 3bec7635c614a4c876136bad0d5afd421d7ed8a3 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Wed, 23 Feb 2022 09:18:54 +0000 Subject: [PATCH 06/11] Merge bitcoin/bitcoin#24420: doc: Update bips.md for 23.x 03bc08e16325f43905f6e6f8d5e0ce69aa8a30a4 doc: Mention missing BIP157 in bips.md (laanwj) e97e3ded69ba1341154bbbea0c75bfe6c09c02e0 doc: Update bips.md for 23.x (laanwj) Pull request description: As far as I know, there have been no new bips implemented in this major release. Update `bips.md` accordingly. (if there are, please post below) ACKs for top commit: jonatack: ACK 03bc08e16325f43905f6e6f8d5e0ce69aa8a30a4 prayank23: ACK https://github.com/bitcoin/bitcoin/pull/24420/commits/03bc08e16325f43905f6e6f8d5e0ce69aa8a30a4 Tree-SHA512: d671c37d1aab9f700f9688dabec056acfd2503c83bd3e1612ed1cee4ba92b99db002e34b6f1100b98543fe60f8b757ea2bedcc2ff020243cf30e27b4dd73d04c Co-authored-by: fanquake --- doc/bips.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/bips.md b/doc/bips.md index b2db8e8637fb..4ab4e807b255 100644 --- a/doc/bips.md +++ b/doc/bips.md @@ -38,7 +38,8 @@ Versions and PRs are relevant to Bitcoin's core if not mentioned other. * [`BIP 147`](https://github.com/bitcoin/bips/blob/master/bip-0147.mediawiki): NULLDUMMY softfork as of **v0.13.1** ([PR 8636](https://github.com/bitcoin/bitcoin/pull/8636) and [PR 8937](https://github.com/bitcoin/bitcoin/pull/8937)), *buried* since **v0.19.0** ([PR #16060](https://github.com/bitcoin/bitcoin/pull/16060)). * [`BIP 152`](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki): Compact block transfer and related optimizations are used as of **v0.13.0** ([PR 8068](https://github.com/bitcoin/bitcoin/pull/8068)). * [`BIP 155`](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki): The 'addrv2' and 'sendaddrv2' messages which enable relay of Tor V3 addresses (and other networks) are supported as of **v18.0** ([PR 19954](https://github.com/bitcoin/bitcoin/pull/19954)). -* [`BIP 158`](https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki): Compact Block Filters for Light Clients can be indexed as of **Dash Core v18.0** ([PR dash#4314](https://github.com/dashpay/dash/pull/4314), [PR #14121](https://github.com/bitcoin/bitcoin/pull/14121)). +* [`BIP 157`](https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki) + [`158`](https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki): Compact Block Filters for Light Clients can be indexed as of **Dash Core v18.0** ([PR dash#4314](https://github.com/dashpay/dash/pull/4314)), ([PR #14121](https://github.com/bitcoin/bitcoin/pull/14121)) and served to peers on the P2P network as of **v0.21.0** ([PR #16442](https://github.com/bitcoin/bitcoin/pull/16442)). * [`BIP 159`](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki): The `NODE_NETWORK_LIMITED` service bit is signalled as of **v0.16.0** ([PR 11740](https://github.com/bitcoin/bitcoin/pull/11740)), and such nodes are connected to as of **v0.17.0** ([PR 10387](https://github.com/bitcoin/bitcoin/pull/10387)). * [`BIP 174`](https://github.com/bitcoin/bips/blob/master/bip-0174.mediawiki): RPCs to operate on Partially Signed Bitcoin Transactions (PSBT) are present as of **v18.0** ([PR 13557](https://github.com/bitcoin/bitcoin/pull/13557)). * [`BIP 324`](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki): The v2 transport protocol specified by BIP324 and the associated `NODE_P2P_V2` service bit are supported as of **v22.0**, but off by default ([PR 28331](https://github.com/bitcoin/bitcoin/pull/28331)). On by default as of **v22.1** ([PR 29347](https://github.com/bitcoin/bitcoin/pull/29347)). From 5e7dff5fc93e9c1870de0c639e82aacc07d7b6bd Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Fri, 4 Mar 2022 09:33:15 +0000 Subject: [PATCH 07/11] Merge bitcoin/bitcoin#24441: fuzz: Limit script_format to 100kB bbbbeaf9c87030eb6b033b6a22002ca8d6635d51 fuzz: Limit script_format to 100kB (MarcoFalke) Pull request description: The target is still one of the slowest ones, but doesn't seem incredibly important. Especially for sizes larger than the standard tx size. Fix that by limiting the script size. ACKs for top commit: fanquake: ACK bbbbeaf9c87030eb6b033b6a22002ca8d6635d51 Tree-SHA512: b6cf7248753909ef2f21d8824f187e7c05732dd3b99619c0067f862f3c2b0f9a87779d4ddbbd3a7a4bae5c794280e2f0a223bf835d6bc6ccaba01817d69479a2 Co-authored-by: fanquake --- src/test/fuzz/script_format.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/fuzz/script_format.cpp b/src/test/fuzz/script_format.cpp index 69c8e7331630..eecfc8960df3 100644 --- a/src/test/fuzz/script_format.cpp +++ b/src/test/fuzz/script_format.cpp @@ -3,7 +3,9 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include +#include #include