Improve the error message when an address cannot be parsed because it is for a different network

JeremyRubin commented at 3:42 am on October 11, 2022: contributor

Currently, the error reporting when you are pasting an address into a command such as walletcreatefundedpsbt for e.g. signet but the address is a mainnet address is very vague, giving specific feedback if it is a valid address for a different network (& maybe the network of the connected node) would be very helpful.

JeremyRubin added the label Feature on Oct 11, 2022

araujo88 commented at 1:42 pm on October 11, 2022: contributor

Hi. I just started contributing to the Bitcoin Core and I would like to address this feature.

JeremyRubin commented at 3:04 pm on October 11, 2022: contributor

I think this is a great first issue, i only didn’t tag it as such since I don’t know the particulars of how to solve it best.

araujo88 commented at 7:14 pm on October 13, 2022: contributor

@JeremyRubin you are referring to lines 1672-1674

0    if (err != TransactionError::OK) {
1        throw JSONRPCTransactionError(err);
2    }

in the file src/wallet/rpc/spend.cpp?

kouloumos commented at 7:35 pm on October 13, 2022: contributor

@araujo88 I believe the issue is more general than that. I haven’t check the walletcreatefundedpsbt RPC but I was able to observe this behavior by using validateaddress on mainnet for a signet address and on signet for a mainnet address, which gives the error : “Not a valid Bech32 or Base58 encoding” which is vague indeed.

I haven’t look into the specifics, but I agree that it looks like a good first issue.

Also, check Creating a permanent link to a code snippet for when you want to link to existing code on master. In your case you would end up with this: https://github.com/bitcoin/bitcoin/blob/0384b194149305ffc8580ad6947e0471a36446ff/src/wallet/rpc/spend.cpp#L1672-L1674

araujo88 commented at 0:48 am on October 17, 2022: contributor

@araujo88 I believe the issue is more general than that. I haven’t check the walletcreatefundedpsbt RPC but I was able to observe this behavior by using validateaddress on mainnet for a signet address and on signet for a mainnet address, which gives the error : “Not a valid Bech32 or Base58 encoding” which is vague indeed.

I haven’t look into the specifics, but I agree that it looks like a good first issue.

Also, check Creating a permanent link to a code snippet for when you want to link to existing code on master. In your case you would end up with this:

https://github.com/bitcoin/bitcoin/blob/0384b194149305ffc8580ad6947e0471a36446ff/src/wallet/rpc/spend.cpp#L1672-L1674

I have looked into the source code and found that the error you mentioned (Not a valid Bech32 or Base58 encoding) is raised by the following function:

https://github.com/bitcoin/bitcoin/blob/c35b91afdc401696f4b1ee65a876328fd462aec9/src/key_io.cpp#L79

So I looked up the parameter called const CChainParams& params and it has the following method:

https://github.com/bitcoin/bitcoin/blob/c35b91afdc401696f4b1ee65a876328fd462aec9/src/chainparams.h#L111-L112

So I believe it can be used to provide a more insightful error message?

russeree commented at 9:14 am on March 19, 2023: contributor

This was quite fun to work on and I have submitted a PR for this. This was quite a difficult task to do and there could be room for improvement but it would require modifications to bech32.cpp for better error decoding.

My approach with to just create a struct as a part of the DecodeDestination. This struct is responsible for constructing a state where errors can be decoded and displayed to the user precisely.

These are my results.

Base58

Bech32

Some notes about the implementation

It’s hard to deal with a string that contains both valid Base58 chars and Bech32 Chars.
An invalid duplicate separator position in bech32 that is < 7 chars from the end causes Bech32.cpp to read everything before the second separator error as hrp. This causes the invalid character checker to only check the part the follows the last separator for invalid bech32 chars. This is slightly annoying because inserting a second 1 can lead to the condition where it give invalid separator when it’s very clear the intent was a bech32 but the user could have just inserted a 1 on accident.
The fact these error string in the bech32.cpp error checker aren’t stored in some sort of globally accessible variable creates this need to duplicate the strings in the code base.
Previous errors had inconsistencies such as random periods in some errors and not others. Using the word encoded in some errors and not others.
The previous implementation with 100% certainly would generate misleading errors when invalid prefixes were presented.

Items I didn’t know how to handle.

Using a struct creates a need to use dot operators to access members. This looked quite awful so I opted to use direct bindings to create aliases to the members of the state struct.
Use of declaration for the magic numbers in DecodeBase58. I did this for readability.

russeree commented at 0:21 am on August 17, 2023: contributor

Following up to my previous post there have been substantial changes since the last revision.

An incorrect bech32 address error message now presents the expected ChainParams hrp and network to the user.
A Bech32 address with an incorrect ChainParams prefix is not automatically handled as base58, increasing the accuracy and readability of user facing error messages.
An incorrect base58 address prefix now presents the user with the expected prefixes for the current ChainParams, which are deterministically computed in util/base58address.(cpp/h)
Functional tests were refactored to reflect the additional accuracy of error messages.
Added GetChainTypeDisplayString(), which generates a user facing chain type string for “Bitcoin”, “testnet”, “signet”, and “regtest”. This is used for displaying the user’s active chain when displaying errors. As per Luke Dashjr, “mainnet” is not used in user facing messages.
Added Base58PrefixesFromVersionBytes(), which when given a total length in bytes and a base58 prefix will give the user all possible single character prefixes for the encoded base58 string.
Bech32 error decoding now handles the cases where the input has neither base58 or bech32 characters and the inverse where the input has both valid base58 and bech32 characters.
Increased code readability through the use of structured bindings.
Added a multiple separators check to bech32.cpp this is to convert incorrect error messages about a singular separator position into a message that covers multiple separators and their positions.
Added a minimum length check to bech32.cpp and ErrorLocatiosn message with the locations being a vector of the chars positions up to the minimum length of 8 chars.

Thoughts and critique are highly appreciated.

Improve the error message when an address cannot be parsed because it is for a different network #26290