Add more info about prefix in error message for invalid address #21755
Conversation
src/key_io.cpp
Outdated
There was a problem hiding this comment.
Why not simply print dec.hrp != params.Bech32HRP()?
There was a problem hiding this comment.
You mean use something like this?
error_str = "Invalid prefix for Bech32 address. Details:" + dec.hrp + "!=" + params.Bech32HRP();Which will give this error if wrong prefix for a bech32 address is used:
Looks more confusing for some users.
There was a problem hiding this comment.
Maybe this works better:
error_str = "Invalid prefix `" + dec.hrp + "` for Bech32 address. Suggested prefix: `" + params.Bech32HRP() + "`";
There was a problem hiding this comment.
'Suggested prefix' implies you (the user) can just change it, which obviously isn't the case. I think the error message does need to emphasise the fact you might be using an address from the wrong network.
There was a problem hiding this comment.
I wanted to mention "Wrong network" in this error but I am not sure if wrong network is the only reason why a prefix can be wrong and condition in if statement at L102 becomes true.
There was a problem hiding this comment.
@prayank23 if the decoding succeeded but the HRP is wrong then I think "Wrong network" is a valid assumption. It would either be that or the input was another type of data entirely, using the same checksum, which is much more unlikely and can be safely ignored.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
+ Mention prefix and network
|
@meshcollider @MarcoFalke Let me know if this code and error message looks better Code: L102-L113 in af99798 Error: |
|
I think it's better without these details. At the very least, normal users shouldn't need to see "mainnet". |
ghost
mentioned this pull request
|
Concept ACK
|
|
LGTM. Provides more context as to what went wrong vs the previous message which was somewhat unclear. |
ghost
closed this
|
Please mark up for grabs |
7 participants
Add info about valid prefixes from https://en.bitcoin.it/wiki/List_of_address_prefixes
Mentioned about correct prefix for Base58 encoded address and Bech32 address for mainnet and testnet. Regtest and Signet can be ignored because the error message will become too long and both networks are mostly used by devs who are either aware of prefixes or understand errors or know the right place to ask questions if any doubt.
We have so many prefixes and networks that ideally such error should just have one link which user can open and check all the information about correct prefixes. Although this can also work for time being.
Fixes #21741