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

russeree · 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

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	Sjors
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:

#30047 (refactor: Model the bech32 charlimit as an Enum by josibake)
#29607 (refactor: Reduce memory copying operations in bech32 encoding/decoding by paplorinc)
#29473 (refactor: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing by paplorinc)

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

russeree · 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

russeree · 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

russeree · 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

Signed-off-by: russeree <git@qrsnap.io>

This commit addresses an issue in Bitcoin Core where an incorrect bech32 prefix would lead to the address being treated as entirely invalid for both base58 and bech32 encoding, without providing the user with an appropriate error message. Base58 error handling was also improved. Changes: - 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/base58_address.(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. A huge thanks to D++ for the extensive contributions Co-authored-by: D++ <82842780+dplusplus1024@users.noreply.github.com> Signed-off-by: russeree <git@qrsnap.io>

RandyMcMillan · 2023-10-01T05:28:15Z

Approach ACK

Signed-off-by: russeree <git@qrsnap.io>

achow101 · 2024-04-15T21:45:20Z

src/bech32.cpp

+    if (str.find('1') != pos) {
+        for(size_t search_idx = 0; search_idx < str.size(); ++search_idx) {
+            if(str[search_idx] == '1') {
+                error_locations.push_back(search_idx);
+            }
+        }
+        return std::make_pair("Multiple separators", std::move(error_locations));
+    }


In 0000000 "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

achow101 · 2024-04-16T18:10:32Z

src/util/base58_address.cpp

+
+#include <algorithm>
+
+std::vector<char> Base58PrefixesFromVersionByte(size_t length, unsigned char version_byte)


In 0000000 "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.

#27260 (comment)

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.

DrahtBot · 2024-06-05T04:32:37Z

🐙 This pull request conflicts with the target branch and needs rebase.

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

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

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

russeree 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

russeree 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

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

russeree 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

russeree force-pushed the 26290 branch 7 times, most recently from 0000000 to 0000000 Compare October 1, 2023 01:26

Add bech32 error short & multiple separators cases

0000000

Signed-off-by: russeree <git@qrsnap.io>

russeree force-pushed the 26290 branch 4 times, most recently from 1234567 to 0000000 Compare October 1, 2023 02:55

russeree and others added 3 commits September 30, 2023 20:10

Add Base58 Address Prefix Decoder

0000000

Signed-off-by: russeree <git@qrsnap.io>

Added chaintype user-facing string display

0000000

Signed-off-by: russeree <git@qrsnap.io>

refactor base58_address.[cpp/h] into base58[.cpp/h]

0101010

Signed-off-by: russeree <git@qrsnap.io>

russeree force-pushed the 26290 branch from 0000000 to 0000000 Compare October 1, 2023 17:37

DrahtBot added the CI failed label Jan 13, 2024

russeree force-pushed the 26290 branch from 0000000 to 0101010 Compare January 26, 2024 22:00

DrahtBot removed the CI failed label Jan 26, 2024

DrahtBot mentioned this pull request Mar 8, 2024

refactor: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing #29473

Open

achow101 requested review from Sjors and achow101 April 9, 2024 15:32

achow101 reviewed Apr 16, 2024

View reviewed changes

DrahtBot mentioned this pull request May 6, 2024

refactor: Model the bech32 charlimit as an Enum #30047

Merged

DrahtBot mentioned this pull request May 24, 2024

refactor: Reduce memory copying operations in bech32 encoding #29607

Open

DrahtBot added the Needs rebase label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

russeree commented Mar 15, 2023 •

edited

DrahtBot commented Mar 15, 2023 •

edited

fanquake commented Mar 15, 2023

russeree commented Mar 15, 2023

maflcko commented May 18, 2023

russeree commented May 19, 2023

maflcko commented May 19, 2023

russeree commented May 19, 2023

RandyMcMillan commented Oct 1, 2023

achow101 Apr 15, 2024

achow101 Apr 16, 2024

russeree Apr 16, 2024

DrahtBot commented Jun 5, 2024


		#include <algorithm>

		std::vector<char> Base58PrefixesFromVersionByte(size_t length, unsigned char version_byte)

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

russeree commented Mar 15, 2023 • edited

Issue:

Proposed Solution:

Reference

Implementation :

Other Notes

DrahtBot commented Mar 15, 2023 • edited

Code Coverage

Reviews

Conflicts

fanquake commented Mar 15, 2023

russeree commented Mar 15, 2023

maflcko commented May 18, 2023

russeree commented May 19, 2023

maflcko commented May 19, 2023

russeree commented May 19, 2023

RandyMcMillan commented Oct 1, 2023

achow101 Apr 15, 2024

Choose a reason for hiding this comment

achow101 Apr 16, 2024

Choose a reason for hiding this comment

russeree Apr 16, 2024

Choose a reason for hiding this comment

DrahtBot commented Jun 5, 2024

russeree commented Mar 15, 2023 •

edited

DrahtBot commented Mar 15, 2023 •

edited