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

portlandhodl · 2023-03-15T10:17:16Z

Issue:

Simply DecodeDestination does not handle errors where the user inputs a valid address for the wrong network. e.x. testnet while running client on mainnet

The current error not a valid Bech32 or Base58 encoding for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of DecodeDestination logic only checks that the prefix matches. If it doesn't it just starts running everything as DecodeBase58Check regardless if the Bech32 String was actually valid.

Proposed Solution:

Base58 Addresses with invalid prefixes will now display network and expected values.

previous: Invalid or unsupported Base58-encoded address.
27260 Invalid or unsupported Base58 testnet address. Expected prefix m, n, or 2

Bech32 Addresses with invalid prefixes will now display network and expected values. The current from of the error is completely incorrect when the user passes valid Bech32 for the wrong network.

previous: Invalid or unsupported Segwit (Bech32) or Base58 encoding.
27260: Invalid chain prefix for testnet. Expected tb got bc

Reference

#26290

Implementation :

Simply put, don't delay the attempt to decode the string as Bech32 using Bech32::Decode(str). This takes a minimal amount CPU cycles to perform and is acceptable to do since this operation is not performed often.

Once you get the decoding status of the string as bech32, do the same with DecodeBase58 using a len of 100 and DecodeBase58Check. This gives you enough information to start properly decoding errors.

After you have decoded the address in various formats run though the logic and display errors for invalid addresses with the network names and expected values when the user just misses the prefix.

Update 1: #27260 (comment)

Other Notes

Previous errors had inconsistencies such as random periods in some errors and not others. Using the word encoded in some errors and not others. This has been resolved.

DrahtBot · 2023-03-15T10:17:19Z

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 react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31974 (Drop testnet3 by Sjors)
#28122 (Silent Payments: Implement BIP352 by josibake)

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.

fanquake · 2023-03-15T10:54:01Z

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

test/functional/rpc_invalid_address_message.py

portlandhodl · 2023-03-15T11:14:21Z

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.

maflcko · 2023-05-18T14:19:53Z

Needs rebase on current master, if still relevant

portlandhodl · 2023-05-19T06:21:45Z

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 · 2023-05-19T06:25:04Z

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 · 2023-05-19T06:30:10Z

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

DrahtBot · 2025-02-23T09:01:36Z

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

Hints

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.

portlandhodl · 2025-02-24T09:17:22Z

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

Sjors · 2025-04-01T12:04:49Z

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 · 2025-04-08T12:03:40Z

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

Sjors · 2025-05-15T09:50:24Z

Any particular reason for marking this draft?

portlandhodl · 2025-05-17T01:26:52Z

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.

Co-authored-by: D++ <82842780+dplusplus1024@users.noreply.github.com>

portlandhodl · 2025-06-04T02:57:37Z

2f2470e

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!

l0rinc

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.

l0rinc · 2025-07-17T00:00:02Z

test/functional/rpc_invalid_address_message.py

-        self.check_invalid(BECH32_INVALID_CHAR, 'Invalid Base 32 character', [8])
-        self.check_invalid(BECH32_MULTISIG_TWO_ERRORS, 'Invalid Bech32 checksum', [19, 30])
-        self.check_invalid(BECH32_WRONG_VERSION, 'Invalid Bech32 checksum', [5])
+        self.check_invalid(BECH32_TOO_LONG, 'Bech32(m) address decoded with error: Bech32 string too long', list(range(90, 108)))


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

l0rinc · 2025-07-17T00:03:57Z

test/functional/rpc_invalid_address_message.py

@@ -62,12 +62,12 @@ def check_invalid(self, addr, error_str, error_locations=None):

    def test_validateaddress(self):
        # Invalid Bech32
-        self.check_invalid(BECH32_INVALID_SIZE, "Invalid Bech32 address program size (41 bytes)")
-        self.check_invalid(BECH32_INVALID_PREFIX, 'Invalid or unsupported prefix for Segwit (Bech32) address (expected bcrt, got bc)')
+        self.check_invalid(BECH32_INVALID_SIZE, 'Invalid Bech32 address program size (41 bytes)')


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

l0rinc · 2025-07-17T00:05:38Z

test/functional/rpc_validateaddress.py

-    # (
-    #   "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sl5k7",
-    #   "00201863143c14c5166804bd19203356da136c985678cd4d27a1b8c6329604903262",
-    # ),


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?

l0rinc · 2025-07-17T00:07:52Z

test/functional/rpc_validateaddress.py

+        if network_name == "signet":
+            INVALID_DATA = INVALID_DATA_SIGNET
+            VALID_DATA = VALID_DATA_SIGNET
+        else:
+            INVALID_DATA = INVALID_DATA_MAINNET
+            VALID_DATA = VALID_DATA_MAINNET


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)

l0rinc · 2025-07-17T00:47:12Z

test/functional/rpc_validateaddress.py

+        # Switch to mainnet tests
+        self.stop_nodes()
+        self.nodes.clear()
+        self.chain = "" # Switch to mainnet
+        self.extra_args = [["-prune=899"]]
+        self.setup_chain()
+        self.setup_network()
+        self.test_validateaddress_on_network("mainnet")


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)

l0rinc · 2025-07-17T05:26:05Z

src/kernel/chainparams.cpp

@@ -273,6 +279,12 @@ class CTestNetParams : public CChainParams {
        base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
        base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};

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


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

l0rinc · 2025-07-17T05:27:24Z

src/kernel/chainparams.cpp

+        base58EncodedPrefixes[SECRET_KEY] =     {"9","c"};
+        base58EncodedPrefixes[EXT_PUBLIC_KEY] = {"tpub"};
+        base58EncodedPrefixes[EXT_SECRET_KEY] = {"tpriv"};
+
        bech32_hrp = "tb";


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

l0rinc · 2025-07-17T05:34:14Z

src/util/chaintype.cpp

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


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); } }

l0rinc · 2025-07-17T05:34:27Z

src/util/chaintype.cpp

+    case ChainType::MAIN:
+        return "Bitcoin";


Yeah, it is kinda' weird

l0rinc · 2025-07-17T05:36:01Z

src/util/chaintype.h

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


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

Suggested change

std::string ChainTypeToDisplayString(ChainType chain);

std::string ChainTypeToString(ChainType chain);

std::string ChainTypeToString(ChainType chain);

std::string ChainTypeToDisplayString(ChainType chain);

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

fanquake reviewed Mar 15, 2023

View reviewed changes

test/functional/rpc_invalid_address_message.py Outdated Show resolved Hide resolved

portlandhodl force-pushed the 26290 branch from 0b5cd1e to d9654cf Compare March 15, 2023 11:07

portlandhodl force-pushed the 26290 branch 7 times, most recently from c98f7cc to 911c8b6 Compare March 16, 2023 00:10

portlandhodl changed the title ~~rpc: enhanced error message for invalid network prefix during address parsing.~~ Enhanced error messages for invalid network prefix during address parsing. Mar 16, 2023

portlandhodl force-pushed the 26290 branch 6 times, most recently from 3c0fcda to aff7635 Compare March 19, 2023 08:50

DrahtBot added the CI failed label May 18, 2023

portlandhodl force-pushed the 26290 branch 2 times, most recently from ef46a30 to 5ff2490 Compare May 19, 2023 03:09

portlandhodl force-pushed the 26290 branch from 5ff2490 to 56908f1 Compare May 19, 2023 23:07

DrahtBot removed the CI failed label May 20, 2023

DrahtBot mentioned this pull request May 23, 2023

rpc: Fix invalid bech32 address handling #27727

Merged

DrahtBot added the CI failed label Feb 23, 2025

portlandhodl force-pushed the 26290 branch from a3b1733 to f974d46 Compare February 23, 2025 19:18

portlandhodl marked this pull request as ready for review February 25, 2025 10:50

maflcko removed the CI failed label Feb 25, 2025

DrahtBot mentioned this pull request Mar 3, 2025

Drop testnet3 #31974

Draft

portlandhodl marked this pull request as draft May 8, 2025 14:27

portlandhodl force-pushed the 26290 branch from f974d46 to b7de615 Compare June 3, 2025 23:27

portlandhodl and others added 4 commits June 3, 2025 16:28

Added chaintype user-facing string display

825f4e9

Co-authored-by: D++ <82842780+dplusplus1024@users.noreply.github.com>

Include Base58 encoded prefixes in chainparams

633bb85

Base58 decoding logic + bech32 decoding network awareness

573d81f

Bech32 decoding logic

13447b2

portlandhodl force-pushed the 26290 branch from b7de615 to 13447b2 Compare June 3, 2025 23:28

[Functional] Introduce muti network testing

2f2470e

portlandhodl force-pushed the 26290 branch from 5dfcea4 to 2f2470e Compare June 4, 2025 02:52

portlandhodl marked this pull request as ready for review June 4, 2025 02:55

portlandhodl requested review from Sjors, achow101, maflcko, jonatack and rkrux June 4, 2025 02:57

DrahtBot mentioned this pull request Jul 14, 2025

Silent Payments: Receiving #32966

Draft

l0rinc suggested changes Jul 17, 2025

View reviewed changes

DrahtBot mentioned this pull request Aug 9, 2025

BIP360: Includes the following: #33163

Closed

		std::string ChainTypeToDisplayString(ChainType chain);

		std::string ChainTypeToString(ChainType chain);

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

Are you sure you want to change the base?

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

Conversation

portlandhodl commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue:

Proposed Solution:

Reference

Implementation :

Other Notes

Uh oh!

DrahtBot commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

fanquake commented Mar 15, 2023

Uh oh!

Uh oh!

portlandhodl commented Mar 15, 2023

Uh oh!

maflcko commented May 18, 2023

Uh oh!

portlandhodl commented May 19, 2023

Uh oh!

maflcko commented May 19, 2023

Uh oh!

portlandhodl commented May 19, 2023

Uh oh!

DrahtBot commented Feb 23, 2025

Uh oh!

portlandhodl commented Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Apr 1, 2025

Uh oh!

portlandhodl commented Apr 8, 2025

Uh oh!

Sjors commented May 15, 2025

Uh oh!

portlandhodl commented May 17, 2025

Uh oh!

portlandhodl commented Jun 4, 2025

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

portlandhodl commented Mar 15, 2023 •

edited

Loading

DrahtBot commented Mar 15, 2023 •

edited

Loading

portlandhodl commented Feb 24, 2025 •

edited

Loading

l0rinc Jul 17, 2025 •

edited

Loading

l0rinc Jul 17, 2025 •

edited

Loading