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

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.

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

Add bech32 error short & multiple separators cases

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

000000000c

russeree force-pushed on Oct 1, 2023

Add Base58 Address Prefix Decoder

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

000000000f

Added chaintype user-facing string display

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

0000000004

Improved address decoding error accuracy

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>

0000000001

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

Approach ACK

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

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

010101019b

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

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.

DrahtBot added the label Needs rebase on Jun 5, 2024

DrahtBot commented at 4:32 am on June 5, 2024: contributor

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

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

DrahtBot commented at 1:06 am on September 22, 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.

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.

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

Issue:

Proposed Solution:

Reference

Implementation :

Other Notes

Code Coverage

Reviews

Conflicts