Enhanced error messages for invalid network prefix during address parsing.

russeree commented at 10:17 am on March 15, 2023: contributor

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

No conflicts as of last run.

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

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

russeree force-pushed on Mar 15, 2023

russeree commented at 11:14 am on March 15, 2023: contributor

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.

russeree force-pushed on Mar 15, 2023

russeree force-pushed on Mar 16, 2023

russeree 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

russeree force-pushed on Mar 18, 2023

russeree 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

russeree force-pushed on May 18, 2023

russeree force-pushed on May 19, 2023

russeree commented at 6:21 am on May 19, 2023: contributor

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

russeree commented at 6:30 am on May 19, 2023: contributor

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

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

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

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

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

russeree 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

russeree force-pushed on Jun 23, 2023

russeree 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

russeree force-pushed on Jul 10, 2023

DrahtBot removed the label CI failed on Jul 10, 2023

russeree commented at 7:15 am on July 10, 2023: contributor

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.

russeree requested review from fanquake on Jul 10, 2023

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

russeree force-pushed on Jul 15, 2023

DrahtBot added the label CI failed on Jul 15, 2023

russeree force-pushed on Jul 15, 2023

DrahtBot removed the label CI failed on Jul 15, 2023

russeree force-pushed on Jul 22, 2023

russeree force-pushed on Jul 23, 2023

russeree force-pushed on Jul 25, 2023

DrahtBot added the label CI failed on Jul 25, 2023

russeree force-pushed on Jul 25, 2023

DrahtBot removed the label CI failed on Jul 25, 2023

russeree force-pushed on Jul 28, 2023

DrahtBot added the label Needs rebase on Aug 17, 2023

russeree force-pushed on Sep 6, 2023

DrahtBot removed the label Needs rebase on Sep 6, 2023

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

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

russeree 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:108 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).

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

russeree force-pushed on Sep 8, 2023

russeree force-pushed on Sep 9, 2023

DrahtBot added the label CI failed on Sep 9, 2023

russeree force-pushed on Sep 9, 2023

russeree commented at 6:21 am on September 9, 2023: contributor

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.

russeree force-pushed on Sep 9, 2023

russeree commented at 7:12 am on September 9, 2023: contributor

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.

russeree force-pushed on Sep 9, 2023

russeree requested review from Sjors on Sep 9, 2023

russeree force-pushed on Sep 9, 2023

in src/bech32.cpp:409 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):

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

russeree commented at 8:30 pm on September 9, 2023:

add in dbe6130764b85c8eb298ba3d236d1507dc930c5d

in src/test/base58_prefix_decoder_tests.cpp:76 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

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

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

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

russeree 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).

russeree force-pushed on Sep 9, 2023

DrahtBot removed the label CI failed on Sep 10, 2023

russeree force-pushed on Sep 11, 2023

DrahtBot added the label CI failed on Sep 11, 2023

russeree force-pushed on Sep 11, 2023

DrahtBot removed the label CI failed on Sep 11, 2023

russeree force-pushed on Sep 11, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

russeree force-pushed on Sep 14, 2023

DrahtBot removed the label Needs rebase on Sep 14, 2023

russeree force-pushed on Sep 17, 2023

DrahtBot added the label Needs rebase on Sep 19, 2023

russeree 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

russeree force-pushed on Sep 20, 2023

DrahtBot removed the label CI failed on Sep 20, 2023

russeree force-pushed on Sep 24, 2023

russeree force-pushed on Oct 1, 2023

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

Approach ACK

russeree force-pushed on Oct 1, 2023

DrahtBot added the label CI failed on Jan 13, 2024

russeree 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

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

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

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

russeree commented at 9:13 pm on June 25, 2024: contributor

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

russeree commented at 6:57 am on September 26, 2024: contributor

⌛ 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.

russeree force-pushed on Sep 29, 2024

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

russeree 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

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.

DrahtBot added the label CI failed on Sep 29, 2024

russeree commented at 8:37 am on September 29, 2024: contributor

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

 0[07:54:50.521] + docker run --rm docker.io/amd64/ubuntu:24.04 bash -c 'env | grep --extended-regexp '\''^(HOME|PATH|USER)='\'''
 1[07:54:50.522] + tee --append /tmp/env-ci_worker_1726594866_672026932-ci_i686_multiprocess
 2[07:54:50.522] Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
 3[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."
 4[07:54:50.549] Error: creating tmpdir: mkdir /run/user/1003: permission denied
 5[07:54:50.551] + echo 'Creating docker.io/amd64/ubuntu:24.04 container to run in'
 6[07:54:50.551] Creating docker.io/amd64/ubuntu:24.04 container to run in
 7[07:54:50.551] + DOCKER_BUILDKIT=1
 8[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
 9[07:54:50.552] Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
10[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."
11[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?

russeree 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

0/home/runner/work/bitcoin/bitcoin/src/util/chaintype.cpp:47:13: warning: enumeration value 'TESTNET4' not handled in switch [-Wswitch]
1   47 |     switch (chain) {
2      |             ^~~~~
31 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.

russeree force-pushed on Oct 4, 2024

Added chaintype user-facing string display

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

366678ffc6

Include Base58 encoded prefixes in chainparams 749977cbb5

russeree force-pushed on Oct 4, 2024

Improved address decoding error accuracy.

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

13538163c8

russeree force-pushed on Oct 4, 2024

russeree commented at 10:46 pm on October 4, 2024: contributor

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.

russeree commented at 7:30 am on December 16, 2024: contributor

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.

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

Issue:

Proposed Solution:

Reference

Implementation :

Other Notes

Code Coverage & Benchmarks

Reviews

Conflicts