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
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.
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.
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 15, 2023
russeree force-pushed
on Mar 15, 2023
russeree force-pushed
on Mar 15, 2023
russeree force-pushed
on Mar 15, 2023
russeree force-pushed
on Mar 16, 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
russeree force-pushed
on Mar 19, 2023
russeree force-pushed
on Mar 19, 2023
russeree force-pushed
on Mar 19, 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
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
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.
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
b7e27c61aboutdated
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);
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.
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
russeree force-pushed
on Jul 15, 2023
DrahtBot added the label
CI failed
on Jul 15, 2023
russeree force-pushed
on Jul 15, 2023
russeree force-pushed
on Jul 15, 2023
russeree force-pushed
on Jul 15, 2023
russeree force-pushed
on Jul 15, 2023
russeree force-pushed
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 22, 2023
russeree force-pushed
on Jul 23, 2023
russeree force-pushed
on Jul 23, 2023
russeree force-pushed
on Jul 23, 2023
russeree force-pushed
on Jul 23, 2023
russeree force-pushed
on Jul 25, 2023
russeree force-pushed
on Jul 25, 2023
russeree force-pushed
on Jul 25, 2023
russeree force-pushed
on Jul 25, 2023
russeree force-pushed
on Jul 25, 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
89139d6b51outdated
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
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
89139d6b51outdated
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.
in
src/kernel/chainparams.h:108
in
fa0ed7796foutdated
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 */
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 8, 2023
russeree force-pushed
on Sep 8, 2023
russeree force-pushed
on Sep 8, 2023
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 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 force-pushed
on Sep 9, 2023
russeree force-pushed
on Sep 9, 2023
russeree force-pushed
on Sep 9, 2023
russeree force-pushed
on Sep 9, 2023
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
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
7397518101outdated
118+
119+ #node = self.nodes[0]
120121 # 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)
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.
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.
Include Base58 encoded prefixes in chainparams7478ac6af9
russeree force-pushed
on Sep 29, 2024
russeree marked this as ready for review
on Sep 29, 2024
Improved address decoding error accuracy.
A complete refactor of the previous pull to simplify logic and reduce the
chance of technical debt.
Co-authored-by: D++ <82842780+dplusplus1024@users.noreply.github.com>
fd02e7650f
russeree force-pushed
on Sep 29, 2024
DrahtBot
commented at 8:34 am on September 29, 2024:
contributor
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
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.)
@maflcko I’m puzzled why the test each commit job didn’t catch this. No test coverage? Shouldn’t it use -Werror?
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.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me