portlandhodl commented at 10:17 AM on March 15, 2023: none

Improve Address Decoding Error Messages

Issue

DecodeDestination does not properly handle errors when a user inputs a valid address for the wrong network (e.g., a testnet address while running the client on mainnet).

The previous error messages were misleading. For example, "Invalid or unsupported Segwit (Bech32) or Base58 encoding" was displayed for a valid Bech32 address on a different network. This occurred because the is_bech32 variable only checked if the prefix matched the current network's HRP. If it didn't match, the code would fall through to Base58 decoding logic regardless of whether the string was actually a valid Bech32 address.

Solution

Implementation Approach

Decode first, then validate: Instead of checking the prefix before decoding, we now decode the string using bech32::Decode(str) upfront. This takes minimal CPU cycles and is acceptable since address validation is not a frequent operation.
Gather decoding information: After Bech32 decoding, we also attempt DecodeBase58 (with a length of 100) and DecodeBase58Check. This provides enough information to properly diagnose errors.
Provide network-aware error messages: When an address has an invalid prefix, the error message now includes the expected network values.

Changes Made

1. Add Base58 Encoded Prefixes to ChainParams (`src/kernel/chainparams.cpp`, `src/kernel/chainparams.h`)

2. Refactor Address Decoding Logic (`src/key_io.cpp`)

Decode Bech32 upfront using bech32::Decode(str) instead of checking prefix first
Use structured bindings for cleaner code: auto [bech32_encoding, bech32_hrp, bech32_chars] = bech32::Decode(str)
Improved error handling flow with clearer branching

3. Improved Error Messages

Scenario	Previous Error	New Error
Base58 address with wrong prefix	"Invalid or unsupported Base58-encoded address."	"Invalid Base58 address. Expected prefix 3, or 1"
Bech32 address with wrong HRP	"Invalid or unsupported Segwit (Bech32) or Base58 encoding."	"Invalid or unsupported prefix for Segwit (Bech32) address (expected bc, got tb)"
Invalid Base58 checksum	"Invalid or unsupported Segwit (Bech32) or Base58 encoding."	"Invalid checksum or length of Base58 address (P2PKH or P2SH)"
Ambiguous invalid address	"Invalid or unsupported Segwit (Bech32) or Base58 encoding."	"Address is not valid Base58 or Bech32"
Bech32 with checksum error	N/A	"Bech32 address decoded with error: Invalid Bech32 checksum"

4. Test Updates

Added test/functional/data/rpc_validateaddress.json with comprehensive test vectors for both mainnet and signet
Updated test/functional/rpc_validateaddress.py to use data-driven testing across multiple networks
Updated test/functional/rpc_invalid_address_message.py with new error message expectations

Consistency Improvements

Removed inconsistent periods at the end of some error messages
Changed "Bech32 v0" to "SegWit v0" for clarity

Reference

Issue: #26290

DrahtBot commented at 10:17 AM on March 15, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/27260.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Sjors, rkrux, jonatack, l0rinc
Approach ACK	RandyMcMillan

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31974 (Drop testnet3 by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

DecodeBase58Check(str, data, 21) in src/key_io.cpp
DecodeBase58(str, data, 100) in src/key_io.cpp

2026-03-22 15:16:52

DrahtBot added the label RPC/REST/ZMQ on Mar 15, 2023

in test/functional/rpc_invalid_address_message.py:1 in 0b5cd1ef67 outdated

   0 | @@ -1,4 +1,3 @@
   1 | -#!/usr/bin/env python3

fanquake commented at 10:53 AM on March 15, 2023:

Not sure why this is being removed

portlandhodl commented at 10:58 AM on March 15, 2023:

This was removed because the linter was throwing on the file permissions and that was the suggestion. I never deliberately changed any perms or ran anything as sudo.

Edit: I know exactly what happened. This will be resolved. Edit 2: This has been resolved a result of a scp command from one host to another.

fanquake commented at 10:54 AM on March 15, 2023: member

Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?

portlandhodl force-pushed on Mar 15, 2023

portlandhodl commented at 11:14 AM on March 15, 2023: none

Please keep your changes, and "fixes" commits squashed. Looks like your changing the file perms on at least one file, is that intentional?

Will do. No it was not intentional but was a side effect of moving 3 files over scp to a different host. This has been resolved and commits squashed.

portlandhodl force-pushed on Mar 15, 2023

portlandhodl force-pushed on Mar 16, 2023

portlandhodl renamed this:
~~rpc: enhanced error message for invalid network prefix during address parsing.~~
Enhanced error messages for invalid network prefix during address parsing.
on Mar 16, 2023

portlandhodl force-pushed on Mar 18, 2023

portlandhodl force-pushed on Mar 19, 2023

DrahtBot added the label CI failed on May 18, 2023

maflcko commented at 2:19 PM on May 18, 2023: member

Needs rebase on current master, if still relevant

portlandhodl force-pushed on May 18, 2023

portlandhodl force-pushed on May 19, 2023

portlandhodl commented at 6:21 AM on May 19, 2023: none

Needs rebase on current master, if still relevant

I believe this PR is still very relevant because it substantially improves the logic around address decoding and specifically the displaying of errors in the GUI and RPC. With that said I have rebased and this code is passing all tests other than Win64 native [vs2022] which seems to be failing for the majority of PRs that are on master due to warnings.

maflcko commented at 6:25 AM on May 19, 2023: member

Thanks, the reason I mentioned it was the silent merge conflict: key_io.cpp:152:48: error: ‘const class CChainParams’ has no member named ‘NetworkIDString’, which is now fixed

portlandhodl commented at 6:30 AM on May 19, 2023: none

Thanks, the reason I mentioned it was the silent merge conflict: key_io.cpp:152:48: error: ‘const class CChainParams’ has no member named ‘NetworkIDString’, which is now fixed

Is there a good way to detect these silent conflicts earlier? Usually I get notified via email when there are issues that need rebased; this is the first time one has happened without a notification.

Thanks

portlandhodl force-pushed on May 19, 2023

DrahtBot removed the label CI failed on May 20, 2023

DrahtBot added the label Needs rebase on May 24, 2023

in src/key_io.cpp:176 in b7e27c61ab outdated

 198 | +    if ((bech32Encoding == bech32::Encoding::BECH32 || bech32Encoding == bech32::Encoding::BECH32M) && bech32Chars.size() > 0) {
 199 |          // Bech32 decoding
 200 | -        if (dec.hrp != params.Bech32HRP()) {
 201 | -            error_str = strprintf("Invalid or unsupported prefix for Segwit (Bech32) address (expected %s, got %s).", params.Bech32HRP(), dec.hrp);
 202 | +        if (bech32Hrp != params.Bech32HRP()) {
 203 | +            error_str = strprintf("Invalid chain prefix for %s. Expected %s got %s", networkLabel, params.Bech32HRP(), bech32Hrp);

luke-jr commented at 9:09 PM on June 22, 2023:

The prefix does not necessarily indicate a chain. The current error is more correct.

portlandhodl commented at 8:33 AM on June 23, 2023:

This has been resolved by rewording the error message since you are correct addresses have prefixes not chains.

Invalid prefix for regtest address (expected bcrt, got bc)

luke-jr commented at 8:01 PM on July 27, 2023:

Could just as well still be unsupported, not invalid.

portlandhodl commented at 7:13 AM on July 28, 2023:

I have changed this to read Invalid or unsupported prefix... testing my own knowledge; The reason you can have an unsupported but still technically valid address would be in the event of a softfork to a new SegWit version? e.x. a segwit client interpreting a taproot address would say "invalid" but that is not completely true because the address is valid. Just not supported by the client?

This has been resolved in b66e974ac8f21971602051c08df830d14cf3df7e

Sjors commented at 3:07 PM on September 7, 2023:

Doesn't even have to be a soft fork. E.g. a silent payment address (#27827) has a prefix we don't support (yet), but it's not a soft fork. It's also not a different network.

in src/key_io.cpp:101 in b7e27c61ab outdated

 101 | +        decodeState(std::string str, std::string chainName) : bech32DecodeResult(bech32::Decode(str))
 102 | +        {
 103 | +            is_base58 = DecodeBase58(str, base58DataRaw, maxBase58Chars);
 104 | +            is_base58Check = DecodeBase58Check(str, base58DataCheck, maxBase58CheckChars);
 105 | +            is_bech32 = bech32DecodeResult.encoding != bech32::Encoding::INVALID;
 106 | +            networkLabel = (chainName == "main" || chainName == "test") ? chainName + "net" : chainName;

luke-jr commented at 9:10 PM on June 22, 2023:

Bitcoin shouldn't be called "mainnet" in user-facing errors.

portlandhodl commented at 8:31 AM on June 23, 2023:

This has been resolved by adding a function to chaintype.cpp called ChainTypeToDisplayString. This decodes the ChainType Enum into user-facing strings. Including using 'Bitcoin' to reference mainnet

Impoper chain errors are now displayed for example "error": "Invalid chain prefix for Bitcoin. Expected bc got tb"

in src/key_io.cpp:154 in b7e27c61ab outdated

 166 | +                std::equal(pubkey_prefix.begin(), pubkey_prefix.end(), base58Data.begin()))) {
 167 |              error_str = "Invalid length for Base58 address (P2PKH or P2SH)";
 168 |          } else {
 169 | -            error_str = "Invalid or unsupported Base58-encoded address.";
 170 | +            std::string chainPrefixes = params.GetChainTypeString() == "main" ? "1 or 3" : "m, n, or 2";
 171 | +            error_str = strprintf("Invalid or unsupported Base58 %s address. Expected prefix %s", networkLabel, chainPrefixes);

luke-jr commented at 9:12 PM on June 22, 2023:

Feels kind of ugly to hard-code the prefixes here.

portlandhodl commented at 6:55 AM on July 10, 2023:

8f2e548f6276386c27a60336cf12665d5e316fb7 solves this via the addition of a function to deterministically calculate the range set of encoded base58 prefixes that result from a version byte(s). The requirements are the length of bytes that are encoded and the desired prefix bytes as a vector.

As a side note thanks for suggesting this as a review item, it was quite a fun problem to think about and implement a solution for.

https://github.com/bitcoin/bitcoin/blob/8f2e548f6276386c27a60336cf12665d5e316fb7/src/util/base58address.cpp#L10

luke-jr changes_requested

portlandhodl force-pushed on Jun 23, 2023

portlandhodl force-pushed on Jul 9, 2023

DrahtBot removed the label Needs rebase on Jul 9, 2023

DrahtBot added the label CI failed on Jul 9, 2023

portlandhodl force-pushed on Jul 10, 2023

DrahtBot removed the label CI failed on Jul 10, 2023

portlandhodl commented at 7:15 AM on July 10, 2023: none

This PR has had some substantial updates applied since it's first inception.

Deterministically calculated base58 prefixes with the function EncodedPrefixesFromBase58Prefix
This PR now greatly improves the accuracy of error reporting as seen in the updates to 3d0a5c37e9ccedfa4ecfaa48eeeca1ada5b4eec1 where many valid bech32 tests are fed through the base58 section of the DecodeDestination function. This line gives a great example of how this PR addresses this from the current https://github.com/bitcoin/bitcoin/blob/79e8247ddb166f9b980f40249b7372a502402a4d/test/functional/rpc_validateaddress.py#L11-L17 Into a much more accurate representaion. https://github.com/bitcoin/bitcoin/blob/8f2e548f6276386c27a60336cf12665d5e316fb7/test/functional/rpc_validateaddress.py#L11-L17 This follows up even further to the changes made in #27727
A function ChainTypeToDisplayString(ChainType chain) has been added to give user facing display names for the various chains a Bitcoin client can attach themselves to. https://github.com/bitcoin/bitcoin/blob/8f2e548f6276386c27a60336cf12665d5e316fb7/src/util/chaintype.cpp#L11-L23

As a note this PR solves a fundamental issue in the current address validation scheme that if a user inputs anything bech32 with the wrong hrp it's immediately treated as base58 even though it is valid bech32.

portlandhodl requested review from fanquake on Jul 10, 2023

portlandhodl requested review from luke-jr on Jul 10, 2023

portlandhodl force-pushed on Jul 15, 2023

DrahtBot added the label CI failed on Jul 15, 2023

portlandhodl force-pushed on Jul 15, 2023

DrahtBot removed the label CI failed on Jul 15, 2023

portlandhodl force-pushed on Jul 22, 2023

portlandhodl force-pushed on Jul 23, 2023

portlandhodl force-pushed on Jul 25, 2023

DrahtBot added the label CI failed on Jul 25, 2023

portlandhodl force-pushed on Jul 25, 2023

DrahtBot removed the label CI failed on Jul 25, 2023

portlandhodl force-pushed on Jul 28, 2023

DrahtBot added the label Needs rebase on Aug 17, 2023

portlandhodl force-pushed on Sep 6, 2023

DrahtBot removed the label Needs rebase on Sep 6, 2023

portlandhodl force-pushed on Sep 6, 2023

in src/util/base58address.h:5 in 89139d6b51 outdated

   0 | @@ -0,0 +1,13 @@
   1 | +// Copyright (c) 2023 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#ifndef BITCOIN_UTIL_BASE58ADDRESS_H

Sjors commented at 2:52 PM on September 7, 2023:

89139d6b51a544d041a2783526c4744eeca7bc0f: nit, I prefer adding an underscore for new files, i.e. base58_address.h, but I don't think there's a rule for that.

portlandhodl commented at 2:12 PM on September 9, 2023:

Completed in 7458d034917fa0bfe060a24731b6f22cc2c6db9e

in src/util/base58address.h:11 in 89139d6b51 outdated

   6 | +#define BITCOIN_UTIL_BASE58ADDRESS_H
   7 | +
   8 | +#include <cstddef>
   9 | +#include <vector>
  10 | +
  11 | +std::vector<char> Base58PrefixesFromVersionBytes(size_t length, std::vector<unsigned char> base58Prefix);

Sjors commented at 2:56 PM on September 7, 2023:

89139d6b51a544d041a2783526c4744eeca7bc0f: can you add a Doxygen comment here? See e.g. here for inspiration.

Also I assume this returns 1 prefix, so maybe call it Base58PrefixFromVersionBytes. Without reading the implementation I'm confused, because the function name suggests base58Prefix is an output, but the parameter suggests it's an input.

portlandhodl commented at 5:18 AM on September 9, 2023:

This returns all possible address prefixes given a version byte .

Example: A testnet address uses a version byte of 0x6F prepended to 24 bytes of of data. That data when base58 encoded will generate 2 possible prefixes ['m','n']

I have created unit tests that should show and demonstrate how this function works.

https://github.com/bitcoin/bitcoin/blob/303308e3fcf4ba6b5e10fc516bdf591415f80d99/src/test/base58_prefix_decoder_tests.cpp#L13-L31

in src/kernel/chainparams.h:112 in fa0ed7796f outdated

 108 | @@ -109,10 +109,12 @@ class CChainParams
 109 |      uint64_t PruneAfterHeight() const { return nPruneAfterHeight; }
 110 |      /** Minimum free space (in GB) needed for data directory */
 111 |      uint64_t AssumedBlockchainSize() const { return m_assumed_blockchain_size; }
 112 | -    /** Minimum free space (in GB) needed for data directory when pruned; Does not include prune target*/
 113 | +    /** Minimum free space (in GB) needed for data directory when pruned; Does not include prune target */

Sjors commented at 3:00 PM on September 7, 2023:

fa0ed7796fb9553c0458c7bc534f73441a991b0d: nit, don't touch unrelated things (may need to adjust your editor to not automatically do that).

portlandhodl commented at 5:26 AM on September 9, 2023:

This was deliberate. The rest of the file has comments in the form of /** Alice Bob Carol */ and this line didn't since I was working in the area I decided to make it consistent with the rest of the file.

Sjors commented at 3:05 PM on September 7, 2023: member

Thanks for splitting this up into multiple commits.

I left some brief comments that would make review easier, but haven't done a good review yet.

Can you add a test for Base58PrefixesFromVersionBytes?

Sjors commented at 4:50 PM on September 7, 2023: member

You may also want to check if #28246 gets in the way of anything you're doing here.

portlandhodl force-pushed on Sep 8, 2023

portlandhodl force-pushed on Sep 9, 2023

DrahtBot added the label CI failed on Sep 9, 2023

portlandhodl force-pushed on Sep 9, 2023

portlandhodl commented at 6:21 AM on September 9, 2023: none

You may also want to check if #28246 gets in the way of anything you're doing here.

There is a case in #28246 DecodeDestination that is modified, but this pr does not modify any logic that is being replaced. As such should not be a problem.

portlandhodl force-pushed on Sep 9, 2023

portlandhodl commented at 7:12 AM on September 9, 2023: none

Can you add a test for Base58PrefixesFromVersionBytes?

I have added unit tests for Base58PrefixesFromVersionByte. Coverage includes all mainnet and testnet address types. And the the longest continuous segment of unchanging outputs.

portlandhodl force-pushed on Sep 9, 2023

portlandhodl requested review from Sjors on Sep 9, 2023

portlandhodl force-pushed on Sep 9, 2023

in src/bech32.cpp:407 in e6e22bc9c8 outdated

 405 | -        std::iota(error_locations.begin(), error_locations.end(), 90);
 406 | +    static constexpr uint8_t BECH32_MAX_LENGTH = 90;
 407 | +    static constexpr uint8_t BECH32_MIN_LENGTH = 8;
 408 | +
 409 | +    if (str.size() > BECH32_MAX_LENGTH) {
 410 | +        error_locations.resize(str.size() - BECH32_MAX_LENGTH);

Sjors commented at 5:47 PM on September 9, 2023:

e6e22bc9c838be127bbedc8fdeb3c9a40f6e3513: could add a comment about what this is doing (it confused me initially):

// Mark all error_locations >= BECH32_MAX_LENGTH
// (and ignore all potential errors in earlier positions).

portlandhodl commented at 8:30 PM on September 9, 2023:

add in dbe6130764b85c8eb298ba3d236d1507dc930c5d

in src/test/base58_prefix_decoder_tests.cpp:38 in 8fea6d07a1 outdated

  33 | +
  34 | +    /* common base58 address types */
  35 | +    int i = 0;
  36 | +    for (const unsigned version_byte : CASES) {
  37 | +        std::vector<unsigned char> base58_data;
  38 | +        bool valid_base58 = DecodeBase58(base58_address_std, base58_data, 35);

Sjors commented at 5:54 PM on September 9, 2023:

8fea6d07a1bd7b6ad5684da7328443954395023f: nit: could move this out of the loop since it doesn't depend on version_byte

portlandhodl commented at 6:54 PM on September 9, 2023:

You are correct, this has been moved to a single call before tests. Nice catch.

in src/test/base58_prefix_decoder_tests.cpp:49 in 8fea6d07a1 outdated

  44 | +    }
  45 | +
  46 | +    /* test longest segment */
  47 | +    for (unsigned short upper_range = 0x91; upper_range <= 0xFF; upper_range++) {
  48 | +        std::vector<unsigned char> base58_data;
  49 | +        bool valid_base58 = DecodeBase58(base58_address_std, base58_data, 35);

Sjors commented at 5:56 PM on September 9, 2023:

8fea6d07a1bd7b6ad5684da7328443954395023f: this is duplicate with the above test?

portlandhodl commented at 7:43 PM on September 9, 2023:

This test is different since it tests the largest range of same outputs.

You can view the expected outputs at https://en.bitcoin.it/wiki/List_of_address_prefixes, The second table gives the expected results. The number 145-255 should always have a base 58 prefix of ['2'] for a 25 byte encoded length.

portlandhodl commented at 7:17 AM on September 11, 2023:

An additional bonus I just pushed is 100% coverage for the space of 25 byte Base58 strings. 4192ab5fb8e67cde3e31ee54718ec83b2ee1511b

in test/functional/rpc_invalid_address_message.py:108 in 7397518101 outdated

 118 | +
 119 | +        #node = self.nodes[0]
 120 |  
 121 |          # Missing arg returns the help text
 122 | -        assert_raises_rpc_error(-1, "Return information about the given bitcoin address.", node.validateaddress)
 123 | +        #assert_raises_rpc_error(-1, "Return information about the given bitcoin address.", node.validateaddress)

Sjors commented at 6:01 PM on September 9, 2023:

7397518101fc0f8f56afe41b2d7b9f44946ebb70: did you mean to disable these tests?

portlandhodl commented at 7:55 PM on September 9, 2023:

This was an artifact and has been resolved.

Sjors commented at 6:03 PM on September 9, 2023: member

Concept ACK

Commit e6e22bc9c838be127bbedc8fdeb3c9a40f6e3513 looks good to me. Only lightly inspected the other commits.

Also compiled and ran the full test suite for each commit (ending in 7397518101fc0f8f56afe41b2d7b9f44946ebb70).

portlandhodl force-pushed on Sep 9, 2023

DrahtBot removed the label CI failed on Sep 10, 2023

portlandhodl force-pushed on Sep 11, 2023

DrahtBot added the label CI failed on Sep 11, 2023

portlandhodl force-pushed on Sep 11, 2023

DrahtBot removed the label CI failed on Sep 11, 2023

portlandhodl force-pushed on Sep 11, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

portlandhodl force-pushed on Sep 14, 2023

DrahtBot removed the label Needs rebase on Sep 14, 2023

portlandhodl force-pushed on Sep 17, 2023

DrahtBot added the label Needs rebase on Sep 19, 2023

portlandhodl force-pushed on Sep 20, 2023

DrahtBot added the label CI failed on Sep 20, 2023

DrahtBot removed the label Needs rebase on Sep 20, 2023

portlandhodl force-pushed on Sep 20, 2023

DrahtBot removed the label CI failed on Sep 20, 2023

portlandhodl force-pushed on Sep 24, 2023

portlandhodl force-pushed on Oct 1, 2023

RandyMcMillan commented at 5:28 AM on October 1, 2023: contributor

Approach ACK

portlandhodl force-pushed on Oct 1, 2023

DrahtBot added the label CI failed on Jan 13, 2024

portlandhodl force-pushed on Jan 26, 2024

DrahtBot removed the label CI failed on Jan 26, 2024

achow101 requested review from Sjors on Apr 9, 2024

achow101 requested review from achow101 on Apr 9, 2024

in src/bech32.cpp:438 in 000000000c outdated

 434 | +            if(str[search_idx] == '1') {
 435 | +                error_locations.push_back(search_idx);
 436 | +            }
 437 | +        }
 438 | +        return std::make_pair("Multiple separators", std::move(error_locations));
 439 | +    }

achow101 commented at 9:45 PM on April 15, 2024:

In 000000000c5fad4aca360a6e914c7779167838db "Add bech32 error short & multiple separators cases"

This is incorrect. 1 is an allowed character in the human readable part, hence a multiple separators check doesn't make sense. The real separator is simply the last 1 that appears. See https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32

portlandhodl commented at 9:35 AM on September 29, 2024:

Thank you for this insight, the faulty logic has been removed as a part of the rebase/refactor in the most recent commit.

in src/util/base58_address.cpp:10 in 000000000f outdated

   5 | +#include <base58.h>
   6 | +#include <util/base58_address.h>
   7 | +
   8 | +#include <algorithm>
   9 | +
  10 | +std::vector<char> Base58PrefixesFromVersionByte(size_t length, unsigned char version_byte)

achow101 commented at 6:10 PM on April 16, 2024:

In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"

It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.

Merging this code seems like an additional maintenance burden for very little gain.

Also, if you insist on keeping this code, it should be placed into its final location when introduced instead of moved in the last commit.

portlandhodl commented at 7:14 PM on April 16, 2024:

#27260 (review)

The choice to do this was a response to the above comment. I would agree chainparams would be the place to implement a hardcoded (char[], stringview) prefix. Another advantage is that lookup is likely faster; so if a user is frequently getting errors it may save a few them a few cycles.

portlandhodl commented at 9:34 AM on September 29, 2024:

The choice to use chainparams to store the Base58 address prefixes has been utilized in the most recent pull. This solution simplifies the PR a noticeable amount. Long term it would be easier to modify default chain address constants than it would be to maintain the deterministic decoding mechanisms.

DrahtBot added the label Needs rebase on Jun 5, 2024

fanquake marked this as a draft on Jun 25, 2024

fanquake commented at 3:46 PM on June 25, 2024: member

Moved to draft for now, given at least one outstanding review comment and needs a rebase.

portlandhodl commented at 9:13 PM on June 25, 2024: none

Moved to draft for now, given at least one outstanding review comment and needs a rebase.

Sorry for the delay in addressing these changes. Currently my work schedule has completely overran my spare time.

I will carve out some time this week to rebase and refactor based on the review items.

Thanks

portlandhodl commented at 6:57 AM on September 26, 2024: none

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.

Is it no longer relevant? ➡️ Please close.

Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

Will close if I don't get to it by this weekend, sorry for my delays. Would like to make one last run at this.

Edit: I have updates that will be pushed in a bit.

portlandhodl force-pushed on Sep 29, 2024

portlandhodl marked this as ready for review on Sep 29, 2024

portlandhodl force-pushed on Sep 29, 2024

DrahtBot commented at 8:34 AM on September 29, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30813837174

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot added the label CI failed on Sep 29, 2024

portlandhodl commented at 8:37 AM on September 29, 2024: none

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30813837174

[07:54:50.521] + docker run --rm docker.io/amd64/ubuntu:24.04 bash -c 'env | grep --extended-regexp '\''^(HOME|PATH|USER)='\'''
[07:54:50.522] + tee --append /tmp/env-ci_worker_1726594866_672026932-ci_i686_multiprocess
[07:54:50.522] Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
[07:54:50.548] time="2024-09-29T07:54:50Z" level=warning msg="RunRoot is pointing to a path (/run/user/1003/containers) which is not writable. Most likely podman will fail."
[07:54:50.549] Error: creating tmpdir: mkdir /run/user/1003: permission denied
[07:54:50.551] + echo 'Creating docker.io/amd64/ubuntu:24.04 container to run in'
[07:54:50.551] Creating docker.io/amd64/ubuntu:24.04 container to run in
[07:54:50.551] + DOCKER_BUILDKIT=1
[07:54:50.551] + docker build --file /tmp/cirrus-build/ci/test_imagefile --build-arg CI_IMAGE_NAME_TAG=docker.io/amd64/ubuntu:24.04 --build-arg FILE_ENV=./ci/test/00_setup_env_i686_multiprocess.sh --label=bitcoin-ci-test --tag=ci_i686_multiprocess /tmp/cirrus-build
[07:54:50.552] Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
[07:54:50.576] time="2024-09-29T07:54:50Z" level=warning msg="RunRoot is pointing to a path (/run/user/1003/containers) which is not writable. Most likely podman will fail."
[07:54:50.577] Error: creating tmpdir: mkdir /run/user/1003: permission denied

This error seems to stem from the CI instance itself rather than the pull.

DrahtBot removed the label Needs rebase on Sep 29, 2024

DrahtBot removed the label CI failed on Sep 29, 2024

in src/util/chaintype.cpp:52 in 7478ac6af9 outdated

  48 | @@ -49,6 +49,8 @@ std::string ChainTypeToDisplayString(ChainType chain)
  49 |          return "Bitcoin";
  50 |      case ChainType::TESTNET:
  51 |          return "testnet";
  52 | +    case ChainType::TESTNET4:

Sjors commented at 8:46 AM on October 3, 2024:

7478ac6af922c302e292ace7f61e6eaa461da727: this change belongs in the previous commit 07f9733e6022d77d755321914a2a2a1dd7ce0d59

(One way to move it there is by doing git rebase -i HEAD~3, setting "e" for the first commit, and then add these lines. Then you do a git commit -a --amend to update the first commit. Then git rebase --continue, which should automatically adjust the second commit to omit this change.)

Sjors commented at 8:48 AM on October 3, 2024:

@maflcko I'm puzzled why the test each commit job didn't catch this. No test coverage? Shouldn't it use -Werror?

portlandhodl commented at 10:43 PM on October 4, 2024:

7478ac6: this change belongs in the previous commit 07f9733

(One way to move it there is by doing git rebase -i HEAD~3, setting "e" for the first commit, and then add these lines. Then you do a git commit -a --amend to update the first commit. Then git rebase --continue, which should automatically adjust the second commit to omit this change.)

This worked like a charm and I learned a new skill! Thanks

maflcko commented at 10:23 AM on October 7, 2024:

@maflcko I'm puzzled why the test each commit job didn't catch this. No test coverage? Shouldn't it use -Werror?

Yeah, probably, that should be enabled. Ref: https://github.com/bitcoin/bitcoin/actions/runs/11091169274/job/30814507260#step:6:493

/home/runner/work/bitcoin/bitcoin/src/util/chaintype.cpp:47:13: warning: enumeration value 'TESTNET4' not handled in switch [-Wswitch]
   47 |     switch (chain) {
      |             ^~~~~
1 warning generated.

maflcko commented at 10:31 AM on October 7, 2024:

Fixed in https://github.com/bitcoin/bitcoin/pull/31045

Sjors commented at 9:21 AM on October 3, 2024: member

Tested and mostly happy with the first two preparation commits. Note to other reviewers: see discussion in #27260 (review) for why the prefixes are hardcoded instead of derived.

The main commit fd02e7650f6410824a64f1403a98583b7ec6b0dd might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.

Re last commit message, you can leave this out:

A complete refactor of the previous pull to simplify logic and reduce the chance of technical debt.

Once this PR is merged, it will unclear what "previous pull" refers to.

portlandhodl force-pushed on Oct 4, 2024

portlandhodl commented at 10:46 PM on October 4, 2024: none

Re last commit message, you can leave this out:

A complete refactor of the previous pull to simplify logic and reduce the chance of technical debt.

This has been removed.

fanquake referenced this in commit 03696bb1bd on Oct 8, 2024

Sjors commented at 7:28 AM on December 16, 2024: member

This still stands:

The main commit fd02e76 might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.

portlandhodl commented at 7:30 AM on December 16, 2024: none

This still stands:

The main commit fd02e76 might be easier to review if you split it up. For example one commit that improves the base58 vs bech32 detection, another commit that that improves base58 error handling and finally one for bech32.

Thank you! I will have this completed.

portlandhodl force-pushed on Jan 20, 2025

portlandhodl marked this as a draft on Feb 3, 2025

in src/util/chaintype.cpp:49 in 114acf622f outdated

  40 | @@ -41,3 +41,20 @@ std::optional<ChainType> ChainTypeFromString(std::string_view chain)
  41 |          return std::nullopt;
  42 |      }
  43 |  }
  44 | +
  45 | +std::string ChainTypeToDisplayString(ChainType chain)
  46 | +{
  47 | +    switch (chain) {
  48 | +    case ChainType::MAIN:
  49 | +        return "Bitcoin";

rkrux commented at 9:21 AM on February 6, 2025:

Confirming - Is using sentence case for mainnet correct? Rest of them use lower case.

l0rinc commented at 5:34 AM on July 17, 2025:

Yeah, it is kinda' weird

Sjors commented at 7:51 AM on November 11, 2025:

In 055dbaed69a2577b967fae3d9b7646ef3b01a254 Added chaintype user-facing string display:

This is used as follows in later commits:

Invalid Base58 %s address.

In the original code it would not specify the network:

Invalid or unsupported Base58-encoded address.

I don't think it's very useful to add the network name to address. Certainly not for mainnet, because most users have no idea (and no need to know) about test networks.

My suggestion would be drop this helper entirely.

in src/kernel/chainparams.cpp:284 in d965c08485 outdated

 278 | @@ -273,6 +279,12 @@ class CTestNetParams : public CChainParams {
 279 |          base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
 280 |          base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
 281 |  
 282 | +        base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m","n"};
 283 | +        base58EncodedPrefixes[SCRIPT_ADDRESS] = {"2"};
 284 | +        base58EncodedPrefixes[SECRET_KEY] =     {"9","c"};

rkrux commented at 9:22 AM on February 6, 2025:

Nit: I found these multiple spaces after the = a little unusual (first time seeing this), imo it's fine to use just one.

- base58EncodedPrefixes[SECRET_KEY] =     {"9","c"};
+ base58EncodedPrefixes[SECRET_KEY] = {"9","c"};

Sjors commented at 6:53 AM on January 8, 2026:

FWIW I like the vertical alignment, but I'm not a linter :-)

in src/kernel/chainparams.cpp:282 in d965c08485 outdated

 278 | @@ -273,6 +279,12 @@ class CTestNetParams : public CChainParams {
 279 |          base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
 280 |          base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
 281 |  
 282 | +        base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m","n"};

rkrux commented at 9:26 AM on February 6, 2025:

Super nit: Personal preference as I find it congruent with the English language and it's slightly easier on the eyes. Same for all these other newly added constants.

- base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m","n"};
+ base58EncodedPrefixes[PUBKEY_ADDRESS] = {"m", "n"};

l0rinc commented at 5:26 AM on July 17, 2025:

Yes, please format the modified code according to the specified formatter

rkrux commented at 9:59 AM on February 6, 2025: contributor

I have reviewed the first 2 commits only as I found the 3rd commit to be difficult to review. I agree with the idea of splitting the last commit into multiple commits as mentioned previously: #27260 (comment).

Overall I agree with the idea of improving the error messages when the address decoding fails but I don't think I agree with the below statement because from the POV of the network that address in indeed invalid. Ref: https://en.bitcoin.it/wiki/BIP_0173#:~:text=The%20human%2Dreadable%20part%20%22bc%22%5B7%5D%20for%20mainnet%2C%20and%20%22tb%22%5B8%5D%20for%20testnet.

The current error not a valid Bech32 or Base58 encoding for a valid address on a different network is entirely incorrect.

Concept ACK @ 70deb6737903556e2cf030d5a29106f01b43a574

rkrux commented at 2:25 PM on February 22, 2025: contributor

#31603#pullrequestreview-2634807894

Not exactly what this PR is fixing but I unintentionally ended up trying network inconsistent keys in my node and faced a not so detailed error, which took some debugging to figure out. So I'm definitely in favour of better error parsing like this PR intends to do.

in src/util/chaintype.cpp:45 in 70deb67379 outdated

  40 | @@ -41,3 +41,20 @@ std::optional<ChainType> ChainTypeFromString(std::string_view chain)
  41 |          return std::nullopt;
  42 |      }
  43 |  }
  44 | +
  45 | +std::string ChainTypeToDisplayString(ChainType chain)

jonatack commented at 5:03 PM on February 22, 2025:

The existing ChainTypeToString() in the same file above seems to already be used for displaying user-facing messages. Perhaps simpler to use it rather than adding this new one.

l0rinc commented at 5:34 AM on July 17, 2025:

Or maybe only store the difference:

std::string ChainTypeToDisplayString(ChainType chain)
{
    switch (chain) {
    case ChainType::MAIN: return "Bitcoin";
    case ChainType::TESTNET: return "testnet";
    default: return ChainTypeToString(chain);
    }
}

jonatack commented at 5:03 PM on February 22, 2025: member

Concept ACK

portlandhodl force-pushed on Feb 23, 2025

DrahtBot added the label CI failed on Feb 23, 2025

DrahtBot commented at 9:01 AM on February 23, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37667214061

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

portlandhodl force-pushed on Feb 23, 2025

portlandhodl commented at 9:17 AM on February 24, 2025: none

Hey,

Sorry for the delayed updates to this PR, I just force pushed after quash the split Base58 and bech32 decoding portions. Per the request of @Sjors .

There also was a substantial effort to create a proper set of functional tests that actually test properly. Examples would be that now with the correct network decoding some errors became network aware and as such needed to be converted to the correct network. Example would be the bech32 padding issue. test/functional/rpc_validateaddress.py line 55

I will be working on addressing some of the more recent comments in the coming day.

Thanks, Russeree

portlandhodl marked this as ready for review on Feb 25, 2025

maflcko removed the label CI failed on Feb 25, 2025

Sjors commented at 12:04 PM on April 1, 2025: member

I will be working on addressing some of the more recent comments in the coming day.

Is this still in the pipeline, or do you prefer if we review as-is?

portlandhodl commented at 12:03 PM on April 8, 2025: none

I should have done all of the splitting up as well as revised the tests to match for the new address decoding conditions.

portlandhodl marked this as a draft on May 8, 2025

Sjors commented at 9:50 AM on May 15, 2025: member

Any particular reason for marking this draft?

portlandhodl commented at 1:26 AM on May 17, 2025: none

Any particular reason for marking this draft?

Yes, I was going to work on the tests for this PR to enable testing with multiple networks, currently the tests had to be refactored to work with mainnet based addresses because of the network awareness.

portlandhodl force-pushed on Jun 3, 2025

portlandhodl force-pushed on Jun 4, 2025

portlandhodl marked this as ready for review on Jun 4, 2025

portlandhodl commented at 2:57 AM on June 4, 2025: none

https://github.com/bitcoin/bitcoin/pull/27260/commits/2f2470e844c79695df0140103c73d742371554cc

This is the one! with this commit I have enabled multi network functional testing [Signet, Mainnet] this enables proof that under different networks the results will match expectations. This also had a few benefits too!

Previously commented out tests are now valid! A lot of errors are now properly handed on a per network basis!

Thanks, and ready for review!

portlandhodl requested review from Sjors on Jun 4, 2025

portlandhodl requested review from achow101 on Jun 4, 2025

portlandhodl requested review from maflcko on Jun 4, 2025

portlandhodl requested review from jonatack on Jun 4, 2025

portlandhodl requested review from rkrux on Jun 4, 2025

in test/functional/rpc_invalid_address_message.py:71 in 573d81f96b outdated

  75 | -        self.check_invalid(BECH32_ONE_ERROR_CAPITALS, 'Invalid Bech32 checksum', [38])
  76 | -        self.check_invalid(BECH32_NO_SEPARATOR, 'Missing separator')
  77 | -        self.check_invalid(BECH32_INVALID_CHAR, 'Invalid Base 32 character', [8])
  78 | -        self.check_invalid(BECH32_MULTISIG_TWO_ERRORS, 'Invalid Bech32 checksum', [19, 30])
  79 | -        self.check_invalid(BECH32_WRONG_VERSION, 'Invalid Bech32 checksum', [5])
  80 | +        self.check_invalid(BECH32_TOO_LONG, 'Bech32(m) address decoded with error: Bech32 string too long', list(range(90, 108)))

l0rinc commented at 12:00 AM on July 17, 2025:

Q: when do we refer to it as Bech32 and when Bech32(m)?

in test/functional/rpc_invalid_address_message.py:65 in 13447b21f9 outdated

  61 | @@ -62,12 +62,12 @@ def check_invalid(self, addr, error_str, error_locations=None):
  62 |  
  63 |      def test_validateaddress(self):
  64 |          # Invalid Bech32
  65 | -        self.check_invalid(BECH32_INVALID_SIZE, "Invalid Bech32 address program size (41 bytes)")
  66 | -        self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid or unsupported prefix for Segwit (Bech32) address (expected bcrt, got bc)')
  67 | +        self.check_invalid(BECH32_INVALID_SIZE, 'Invalid Bech32 address program size (41 bytes)')

l0rinc commented at 12:03 AM on July 17, 2025:

It would help with the review if commits only had a single focus: separating low risk refactors with higher risk behavioral changes.

in test/functional/rpc_validateaddress.py:118 in 2f2470e844 outdated

 116 |          "0014751e76e8199196d454941c45d1b3a323f1433bd6",
 117 |      ),
 118 | -    # (
 119 | -    #   "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sl5k7",
 120 | -    #   "00201863143c14c5166804bd19203356da136c985678cd4d27a1b8c6329604903262",
 121 | -    # ),

l0rinc commented at 12:05 AM on July 17, 2025:

what's the story behind these, could we add them to the test suite instead of deleting them? Why were they committed like this in the first place?

Sjors commented at 8:03 AM on November 11, 2025:

In f8513e38b369889df0b402f06d67fcf656495d53 [Functional] Introduce muti network testing:

These were disabled in #27727, IIUC in favour the official test vectors. Seems fine to delete them. If they're actually interesting, then they can be restored and added to the BIP.

(nit: typo in commit message)

in test/functional/rpc_validateaddress.py:233 in 2f2470e844 outdated

 229 | +        if network_name == "signet":
 230 | +            INVALID_DATA = INVALID_DATA_SIGNET
 231 | +            VALID_DATA = VALID_DATA_SIGNET
 232 | +        else:
 233 | +            INVALID_DATA = INVALID_DATA_MAINNET
 234 | +            VALID_DATA = VALID_DATA_MAINNET

l0rinc commented at 12:07 AM on July 17, 2025:

could we rather store them in a dictionary above, keyed by network_name?

INVALID_DATA = {
    "mainnet": [
        ("tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty", "Invalid or unsupported prefix for Segwit (Bech32) Bitcoin address (expected bc, got tc)", []),
...
    ],
    "signet": [
        ("tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty", "Invalid or unsupported prefix for Segwit (Bech32) signet address (expected tb, got tc)", []),
...
    ],
}

VALID_DATA = {
    "mainnet": [
        ("BC1QW508D6QEJXTDG4Y5R3ZARVARY0C5XW7KV8F3T4", "0014751e76e8199196d454941c45d1b3a323f1433bd6"),
...
    ],
    "signet": [
        ("tb1pfees9rn5nz", "51024e73"),
...
    ],
}

and

    def test_validateaddress_on_network(self, network_name):
        self.log.info(f"Testing validateaddress on {network_name}")
        for (addr, error, locs) in INVALID_DATA[network_name]:
            self.check_invalid(addr, error, locs)
        for (addr, spk) in VALID_DATA[network_name]:
            self.check_valid(addr, spk)

Sjors commented at 8:11 AM on November 11, 2025:

In f8513e38b369889df0b402f06d67fcf656495d53 [Functional] Introduce muti network testing:

The elegant solution imo is to move these test vectors into a JSON file, see e.g. rpc_getblockstats.py.

But I think the current approach is fine. You could also duplicate the body of test_validateaddress_on_network into test_validateaddress_mainnet and test_validateaddress_signet rather than copying the constants. Each for loop could then be preceded with self.log.debug("Valid mainnet addresses"), etc.

in test/functional/rpc_validateaddress.py:249 in 2f2470e844 outdated

 247 | +        self.nodes.clear()
 248 | +        self.chain = "" # Switch to mainnet
 249 | +        self.extra_args = [["-prune=899"]]
 250 | +        self.setup_chain()
 251 | +        self.setup_network()
 252 | +        self.test_validateaddress_on_network("mainnet")

l0rinc commented at 12:47 AM on July 17, 2025:

Not sure this is the best way to test this, starting it up in signet mode and stopping and clearing and overwriting the args - can we rather make them parameterizable, something like:

class ValidateAddressSignetTest(BaseValidateAddressTest):
    def set_test_params(self):
        super().set_test_params()
        self.network_name = "signet"
        self.chain = "signet"
        self.extra_args = [["-prune=899", "-signet"]]


class ValidateAddressMainnetTest(BaseValidateAddressTest):
    def set_test_params(self):
        super().set_test_params()
        self.network_name = "mainnet"
        self.chain = ""
        self.extra_args = [["-prune=899"]]


if __name__ == "__main__":
    ValidateAddressSignetTest(__file__).main()
    ValidateAddressMainnetTest(__file__).main()

(if this is even possible, haven't tried it, my point is better separation of concerns)

Sjors commented at 8:16 AM on November 11, 2025:

If this works out of the box it's worth taking, but otherwise I would keep the current approach. Address validation is simple enough that I'm not worried about the node being in a confused state.

in src/key_io.cpp:149 in 573d81f96b outdated

 147 | +        if (!is_base58) {
 148 | +            // If bech32 decoding failed due to invalid base32 chars, address format is ambiguous; otherwise, report bech32 error
 149 | +            bool is_validBech32Chars = (bech32_error != "Invalid Base 32 character");
 150 | +            error_str = is_validBech32Chars ? "Bech32(m) address decoded with error: " + bech32_error : "Address is not valid Base58 or Bech32";
 151 |          } else {
 152 | +            if (bech32_error == "Invalid character or mixed case") {

l0rinc commented at 4:36 AM on July 17, 2025:

We should find a way to avoid digging into the internals of LocateErrors - maybe we could return a proper type instead of std::pair<std::string, std::vector<int>> LocateErrors which would return the error category (as enum?) instead of the text directly - the users of LocateErrors should decide the exact error message per error code.

in src/key_io.cpp:144 in 573d81f96b outdated

 142 |      } else if (!is_bech32) {
 143 | +        bool is_base58 = DecodeBase58(str, data, MAX_BASE58_CHARS);
 144 |          // Try Base58 decoding without the checksum, using a much larger max length
 145 | -        if (!DecodeBase58(str, data, 100)) {
 146 | -            error_str = "Invalid or unsupported Segwit (Bech32) or Base58 encoding.";
 147 | +        if (!is_base58) {

l0rinc commented at 4:39 AM on July 17, 2025:

we could invert these conditions to start with the non-negated versions, it flows more naturally to do if (condition) ... else compared to if (!condition) ... else

in src/key_io.cpp:141 in 573d81f96b outdated

 137 | +                base58_address_prefixes += std::string(encoded_prefixes[i]);
 138 | +            }
 139 | +            error_str = strprintf("Invalid Base58 %s address. Expected prefix %s", params.GetChainTypeDisplayString(), base58_address_prefixes);
 140 |          }
 141 |          return CNoDestination();
 142 |      } else if (!is_bech32) {

l0rinc commented at 4:42 AM on July 17, 2025:

I know this was here before as well, but we have already checked is_bech32 in the if condition, we could break up if (!is_bech32 && DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS)) { into an if (!is_bech32) { if (DecodeBase58Check) ... } else { ... } instead - preferably inverting it as well to if (is_bech32) {} else if (DecodeBase58Check...). If you decide to accept these comments, please do it in multiple, focused commits, the changes should guide the reviewer and tell a story.

in src/key_io.cpp:137 in 573d81f96b outdated

 133 | +            for (size_t i = 0; i < encoded_prefixes.size(); ++i) {
 134 | +                if (i > 0) {
 135 | +                    base58_address_prefixes += (i == encoded_prefixes.size() - 1) ? ", or " : ", ";
 136 | +                }
 137 | +                base58_address_prefixes += std::string(encoded_prefixes[i]);
 138 | +            }

l0rinc commented at 4:45 AM on July 17, 2025:

maybe we could just join them via comma instead, something like:

const auto prefixes{util::Join(encoded_prefixes, ", ")};

or combining it with the above:

const auto prefixes{util::Join(Cat(std::vector(params.Base58EncodedPrefix(CChainParams::SCRIPT_ADDRESS)),
                                   params.Base58EncodedPrefix(CChainParams::PUBKEY_ADDRESS)), ", ")};

(untested and we'd likely have to adjust the full texts accordingly)

or combining it with the above, maybe this instead:

std::string prefixes;
for (const auto& prefix : params.Base58EncodedPrefix(CChainParams::SCRIPT_ADDRESS)) {
    if (!prefixes.empty()) prefixes += ", ";
    prefixes += prefix;
}
for (const auto& prefix : params.Base58EncodedPrefix(CChainParams::PUBKEY_ADDRESS)) {
    if (!prefixes.empty()) prefixes += ", ";
    prefixes += prefix;
}

in src/key_io.cpp:100 in 573d81f96b outdated

 100 |  
 101 | -    if (!is_bech32 && DecodeBase58Check(str, data, 21)) {
 102 | +    auto check_base58 = [&]() { return DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS); };
 103 | +    
 104 | +
 105 | +    if (!is_bech32 && check_base58()) {

l0rinc commented at 5:05 AM on July 17, 2025:

as mentioned, if we invert the condition we'd have if (is_bech32) { ... } else if (check_base58()). But I'm not even sure why these two are extracted, I don't find the check_base58oris_bech32` to be a helpful abstraction here - I'd inline them

Also, bech32 and base58 shouldn't be handled in the same method, they're not the same responsibility - could you please extract their handling to smaller, more focused methods?

I'd test and extract them in separate commits as well, I don't see a reason for combining them in any way.

Sjors commented at 8:25 AM on November 11, 2025:

In 04d5d4acab4259505654a506c838fecb29fdb88d Base58 decoding logic + bech32 decoding network awareness: splitting bech32 and base58 handling in an earlier commit might also help to focus the current commit more on base58. I still a lot of lines touching bech32 code.

in src/key_io.cpp:95 in 573d81f96b outdated

  94 | +
  95 | +    std::vector<unsigned char> data, bech32_data;
  96 | +
  97 | +    auto [bech32_encoding, bech32_hrp, bech32_chars] = bech32::Decode(str);
  98 | +    auto [bech32_error, bech32_error_loc] = bech32::LocateErrors(str);
  99 | +    bool is_bech32 = bech32_encoding != bech32::Encoding::INVALID;

l0rinc commented at 5:12 AM on July 17, 2025:

does it even make sense to run bech32::LocateErrors when bech32_encoding != bech32::Encoding::INVALID? Seems a bit all over the place.

in src/key_io.cpp:147 in 573d81f96b outdated

 145 | -        if (!DecodeBase58(str, data, 100)) {
 146 | -            error_str = "Invalid or unsupported Segwit (Bech32) or Base58 encoding.";
 147 | +        if (!is_base58) {
 148 | +            // If bech32 decoding failed due to invalid base32 chars, address format is ambiguous; otherwise, report bech32 error
 149 | +            bool is_validBech32Chars = (bech32_error != "Invalid Base 32 character");
 150 | +            error_str = is_validBech32Chars ? "Bech32(m) address decoded with error: " + bech32_error : "Address is not valid Base58 or Bech32";

l0rinc commented at 5:14 AM on July 17, 2025:

I find it confusing that when we don't have valid bech32 chars we report that it's not a valid base58

in test/functional/rpc_validateaddress.py:126 in 573d81f96b outdated

 111 |      ),
 112 | -    (
 113 | -        "tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vpggkg4j",
 114 | -        "Invalid or unsupported Segwit (Bech32) or Base58 encoding.",  # tb1, Non-zero padding in 8-to-5 conversion
 115 | -        [],
 116 | -    ),

l0rinc commented at 5:15 AM on July 17, 2025:

could you please provide any explanation in the commit message why these were discarded?

in src/kernel/chainparams.cpp:518 in 633bb85b45 outdated

 513 | +        base58EncodedPrefixes[SCRIPT_ADDRESS] = {"2"};
 514 | +        base58EncodedPrefixes[SECRET_KEY] =     {"9","c"};
 515 | +        base58EncodedPrefixes[EXT_PUBLIC_KEY] = {"tpub"};
 516 | +        base58EncodedPrefixes[EXT_SECRET_KEY] = {"tpriv"};
 517 | +
 518 |          bech32_hrp = "tb";

l0rinc commented at 5:27 AM on July 17, 2025:

as mentioned by @Sjors, I think we may want to leave room for Silent Payment prefixes as well: 9219e4f (#28201)

Probably nothing to do yet, just good to know about it - cc: @josibake

Sjors commented at 7:55 AM on November 11, 2025:

In 7b71e6e056f5567152097d5c9a1a8e2f59c1b661 Include Base58 encoded prefixes in chainparams: this commit only adds base58 prefixes, silent payments will use bech32m.

in src/util/chaintype.h:21 in 825f4e934e outdated

  15 | @@ -16,6 +16,8 @@ enum class ChainType {
  16 |      TESTNET4,
  17 |  };
  18 |  
  19 | +std::string ChainTypeToDisplayString(ChainType chain);
  20 | +
  21 |  std::string ChainTypeToString(ChainType chain);

l0rinc commented at 5:36 AM on July 17, 2025:

The header order should ideally be the same as the implementation, unless there's a reason for it:

std::string ChainTypeToString(ChainType chain);
std::string ChainTypeToDisplayString(ChainType chain);

l0rinc changes_requested

l0rinc commented at 5:39 AM on July 17, 2025: contributor

Concept ACK, we should indeed make the error messages more user friendly.

It seems to me the PR is heading in the right direction, but we need to separate handling unrelated code paths - bech32 and base58 are different enough that they deserve their own helper method, not just buried somewhere in an if condition. The functional tests could also use better separation, without favoring one or the other in the default params and the commit messages should explain the context better.

achow101 commented at 3:39 PM on October 22, 2025: member

Are you still working on this?

achow101 requested review from murchandamus on Oct 22, 2025

achow101 removed review request from Sjors on Oct 22, 2025

achow101 requested review from Sjors on Oct 22, 2025

portlandhodl commented at 5:01 PM on October 22, 2025: none

Actually yes! Found some time this week. Please allow for a push today!

Thanks!

portlandhodl force-pushed on Nov 2, 2025

portlandhodl marked this as a draft on Nov 7, 2025

Sjors commented at 7:37 AM on November 11, 2025: member

CI fails with:

Bech32 address decoded with error: Invalid character or mixed case == Bech32(m) address decoded with error: Invalid character or mixed case

in test/functional/rpc_validateaddress.py:170 in f8513e38b3

 166 | @@ -177,7 +167,7 @@
 167 |      ),
 168 |      (
 169 |          "tb1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vq47Zagq",
 170 | -        "Bech32 address decoded with error: Invalid character or mixed case",  # tb1, Mixed case
 171 | +        "Bech32(m) address decoded with error: Invalid character or mixed case",  # tb1, Mixed case

Sjors commented at 8:05 AM on November 11, 2025:

In f8513e38b369889df0b402f06d67fcf656495d53 [Functional] Introduce muti network testing: the extra (m) breaks the test

in src/key_io.cpp:98 in c974f892c7 outdated

  94 | @@ -95,7 +95,6 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
  95 |      bool is_bech32 = bech32_encoding != bech32::Encoding::INVALID;
  96 |  
  97 |      auto check_base58 = [&]() { return DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS); };
  98 | -

Sjors commented at 8:17 AM on November 11, 2025:

Nit for c974f892c7b6a61630c555f08ec201d911d5fd62 Bech32 decoding logic: unrelated whitespace change and random looking asdas in the commit message.

Sjors commented at 8:27 AM on November 11, 2025: member

Thanks for splitting base58 and bech32 stuff into separate commits. I think that 04d5d4acab4259505654a506c838fecb29fdb88d Base58 decoding logic + bech32 decoding network awareness could itself be split, see inline comment from @l0rinc, since it still contains a lot of bech32 changes despite the commit name.

My main other suggestion is to drop the first commit (and don't add the network name to error messages).

See inline comment for how to fix the test.

portlandhodl force-pushed on Nov 16, 2025

portlandhodl commented at 5:20 AM on January 8, 2026: none

My main other suggestion is to drop the first commit (and don't add the network name to error messages).

Okay I, set aside some time to work on this PR tonight. If there is no reason to add the network name to the error messages then this whole PR is kind of a moot point and should be closed. #26290

I am completely okay with this if that is the case.

Edit: I see what you mean remove the network name and only display the prefixes.

portlandhodl force-pushed on Jan 8, 2026

DrahtBot added the label CI failed on Jan 8, 2026

portlandhodl force-pushed on Jan 8, 2026

Sjors commented at 7:05 AM on January 8, 2026: member

Quick note as of 6bcaa46a7640196766922bc1435d96f17a5e751e. IIUC you rebased, and then dropped 055dbaed69a2577b967fae3d9b7646ef3b01a254 Added chaintype user-facing string display, adjusting things where needed. But I guess you're still working on potentially refactoring 870c850b7d7c492e365b3d6b7dec64f7e17a633e Base58 decoding logic + bech32 decoding network awareness and 2df89b6ffdab851ec9e6a5c090305a4f323114ae Bech32 decoding logic based on earlier reviews?

portlandhodl commented at 7:09 AM on January 8, 2026: none

But I guess you're still working on potentially refactoring 870c850 Base58 decoding logic + bech32 decoding network awareness and 2df89b6 Bech32 decoding logic based on earlier reviews?

Absolutely, this is the correct understanding of this, will re-request and pull out of draft when ready. I may respond to a few of the critiques, during this refactor.

Thanks again for the review.

DrahtBot removed the label CI failed on Jan 8, 2026

portlandhodl force-pushed on Jan 8, 2026

DrahtBot added the label CI failed on Jan 8, 2026

DrahtBot commented at 8:37 AM on January 8, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS-cross to arm64: https://github.com/bitcoin/bitcoin/actions/runs/20810552653/job/59773530053 LLM reason (✨ experimental): Build failed due to undefined symbol MAX_BASE58_CHARS in key_io.cpp (use of undeclared MAX_BASE58_CHARS).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

portlandhodl force-pushed on Jan 8, 2026

portlandhodl commented at 10:22 AM on January 8, 2026: none

Quick note as of 6bcaa46. IIUC you rebased, and then dropped 055dbae Added chaintype user-facing string display, adjusting things where needed. But I guess you're still working on potentially refactoring 870c850 Base58 decoding logic + bech32 decoding network awareness and 2df89b6 Bech32 decoding logic based on earlier reviews?

ef5e5170bf39601a25409b2d26e0554576c6bbed I am using this as a setup commit for the more logic like sections that are handled in key_io.cpp

One of the hardest portions seems to be is that at some point this all converges into a case where you have to test that you don't have valid base58 or valid bech32 which inherently will have to run checks for both.

portlandhodl force-pushed on Jan 8, 2026

portlandhodl marked this as ready for review on Jan 8, 2026

portlandhodl requested review from Sjors on Jan 8, 2026

portlandhodl force-pushed on Jan 8, 2026

Sjors commented at 2:28 AM on January 9, 2026: member

The "test max 6 ancestor commits" job fails at eacf78ca650f006742eaf36300e6f4ac1374dec3 General address decoding logic exe order.

This should fix it, by moving later changes into that commit: https://github.com/Sjors/bitcoin/commit/aabb7df204ea46402121badb8ffe14ef068301f2

Some of the commit messages have weird trailing characters (adasda, etc).

Noticed there's still a lot of bech32 related changes in c2c8454cdd413680e61c96dacdb912ca198847aa Base58 decoding logic: despite the commit title, maybe split it further?

portlandhodl force-pushed on Jan 9, 2026

portlandhodl commented at 2:46 AM on January 9, 2026: none

The "test max 6 ancestor commits" job fails at eacf78c General address decoding logic exe order.

This should fix it, by moving later changes into that commit: Sjors@aabb7df

Some of the commit messages have weird trailing characters (adasda, etc).

Noticed there's still a lot of bech32 related changes in c2c8454 Base58 decoding logic: despite the commit title, maybe split it further?

Thank you for the fixup! Applied

Edit- Per the messages, it was getting late and I was flogging the rebase -i and made a mistake

l0rinc commented at 12:55 PM on January 13, 2026: contributor

@portlandhodl, ping us when the PR is ready for review again.

DrahtBot marked this as a draft on Jan 19, 2026

portlandhodl force-pushed on Mar 21, 2026

Include Base58 encoded prefixes in chainparams 95256a0157

Bech32: Errors with expected prefixes 2060fc3bf0

portlandhodl force-pushed on Mar 21, 2026

DrahtBot removed the label CI failed on Mar 21, 2026

portlandhodl force-pushed on Mar 21, 2026

portlandhodl force-pushed on Mar 22, 2026

Base58: Errors with expected prefixes b78373188a

portlandhodl force-pushed on Mar 22, 2026

DrahtBot added the label CI failed on Mar 22, 2026

DrahtBot removed the label CI failed on Mar 22, 2026

portlandhodl marked this as ready for review on Mar 22, 2026

portlandhodl commented at 9:30 PM on March 22, 2026: none

@l0rinc review ready, notable changes here

Fall through base58 logic
bech32 decode first, then base58

Nice things I wish I had bech32::LocateErrors would return an Enum instead of a string.

sedited requested review from stickies-v on Mar 24, 2026

Enhanced error messages for invalid network prefix during address parsing. #27260

Improve Address Decoding Error Messages

Issue

Solution

Implementation Approach

Changes Made

1. Add Base58 Encoded Prefixes to ChainParams (src/kernel/chainparams.cpp, src/kernel/chainparams.h)

2. Refactor Address Decoding Logic (src/key_io.cpp)

3. Improved Error Messages

4. Test Updates

Consistency Improvements

Reference

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

1. Add Base58 Encoded Prefixes to ChainParams (`src/kernel/chainparams.cpp`, `src/kernel/chainparams.h`)

2. Refactor Address Decoding Logic (`src/key_io.cpp`)