#27260 Enhanced error messages for invalid network prefix during address parsing.
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #27260 Enhanced error messages for invalid network prefix during address parsing. | Author silent 68d | 0 | Mild | 221 + 369 tests | ||||||
|
|
|
|
| ||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | ||||||||||
Goal
Refactors destination decoding in `key_io` to decode Bech32/Bech32m addresses upfront before attempting Base58 decoding, eliminating prefix-based guessing. Converts `bech32::LocateErrors()` to return a structured error enum, and introduces `CChainParams::Base58PrefixText()` to expose expected human-readable Base58 prefixes across networks. Uses these mechanisms to return precise, network-aware error messages when valid addresses from another network or specific malformed strings are passed into RPCs.
Problem: When a user or client passes a valid address formatted for another network (such as entering a signet `tb1...` address on mainnet, or a mainnet Base58 address on testnet), `DecodeDestination` returns confusing, generic errors such as "Invalid or unsupported Segwit (Bech32) or Base58 encoding." Because the previous decoder dispatched to Bech32 only if the input matched the active chain's HRP, valid Bech32 addresses from other chains fell through to the Base58 decoder.
Evidence of importance:
- Fixes issue #26290: 'Improve the error message when an address cannot be parsed because it is for a different network'.
- PR description explains that 'Because the Bech32-vs-Base58 dispatch guessed the encoding from the address prefix, a valid Bech32 address for a different network fell through to the Base58 path and reported "Invalid or unsupported Segwit (Bech32) or Base58 encoding."'
- Commit e1bc3a928f points out that Base58 addresses with valid checksums but wrong-network version bytes gave 'no hint about what went wrong', which this PR replaces with messages like 'Invalid Base58 address. Expected prefix 2, m or n'.
Scope: Modifies public interfaces in `src/bech32.h` (changing `LocateErrors` from returning an error string to returning an `enum class Error`) and adds a new `base58PrefixText` table across all chain params in `src/kernel/chainparams.cpp`. Also replaces the inline test data in `test/functional/rpc_validateaddress.py` with a data-driven JSON vector file tested across multiple network node restarts.
Reviewability
Stale · Author silent 68d
No author activity for 68 days; a review may wait a long time for a response.
Agreement
Mild
objection that needs a change was answered, no reply from the objector (l0rinc: Bundling large refactors, moves, deletions, and behavior changes across commits impedes review and risks regressions.); objection that needs an answer has none yet (l0rinc)
Thread notes: Initial record population. While there is broad Concept support, the PR is held in draft pending author rework to address l0rinc's request to separate behavioral changes from refactors into reviewable commits.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| luke-jr (contributor) | objection, correctness | Error message incorrectly stated that address prefixes indicate a chain. | fixed · fix pushed · v1, not yet re-checked | changed | yes | 2023-06-22: 'The prefix does not necessarily indicate a chain. The current error is more correct.' Status from: 2023-06-23: portlandhodl: 'This has been resolved by rewording the error message since you are correct addresses have prefixes not chains.' |
| luke-jr (contributor) | objection, interface | User-facing errors exposed internal naming by referring to Bitcoin as mainnet. | fixed · fix pushed · v1, not yet re-checked | changed | yes | 2023-06-22: 'Bitcoin shouldn't be called "mainnet" in user-facing errors.' Status from: 2023-06-23: portlandhodl: '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' |
| luke-jr (contributor) | objection, correctness | Prefixes not recognized by the client were labeled invalid rather than unsupported. | fixed · fix pushed · v1, not yet re-checked | answered | yes | 2023-07-27: 'Could just as well still be unsupported, not invalid.' Status from: 2023-07-28: portlandhodl: 'I have changed this to read Invalid or unsupported prefix... This has been resolved in b66e974ac8f21971602051c08df830d14cf3df7e' |
| achow101 (member) | objection, correctness | Rejecting Bech32 strings with multiple '1's breaks valid addresses because '1' is allowed in the HRP. | fixed · fix pushed · v1, not yet re-checked | changed | yes | 2024-04-15: 'This is incorrect. 1 is an allowed character in the human readable part, hence a multiple separators check doesn't make sense.' Status from: 2024-09-29: portlandhodl: 'Thank you for this insight, the faulty logic has been removed as a part of the rebase/refactor in the most recent commit.' |
| achow101 (member) | objection, maintenance | Dynamically calculating Base58 prefixes from version bytes creates unnecessary maintenance burden when static chainparams suffice. | fixed · fix pushed · v1, not yet re-checked | changed | yes | 2024-04-16: '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... Merging this code seems like an additional maintenance burden for very little gain.' Status from: 2024-09-29: portlandhodl: '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.' |
| l0rinc (member) | objection, maintenance | Bundling large refactors, moves, deletions, and behavior changes across commits impedes review and risks regressions. | answered · reply on record; standing not assessed yet · v1, not yet re-checked | changed | yes | 2026-04-20: 'my biggest concern (unrelated changes intertwined in big commits and commit messages not explaining the change) is still unaddressed. I have a hard time reviewing this meaningfully - without redoing everything locally, which I would like to avoid.' |
| l0rinc (member) | objection, safety | Using std::string_view instead of std::string in chainparams risks lifetime bugs. | open · v1, not yet re-checked | answered | no | 2026-04-20: 'SSO should already make these cheap, let's use std::string instead. That's consistent with the rest of CChainParams and removes the potential lifetime footgun.' |
| l0rinc (member) | objection, approach | Reparsing the input string during validation causes redundant decoding passes. | open · v1, not yet re-checked | answered | no | 2026-04-20: 'Do we really need to reparse str here? Could we instead have a single path that lowers up front to avoid duplicated work?' |
Support:
- Sjors (member) (Concept ACK, 2023-09-09): Looks good to improve address decoding errors; inspected and tested commit range.
- RandyMcMillan (contributor) (Approach ACK, 2023-10-01): Approach ACK [not substantive]
- rkrux (member) (Concept ACK, 2025-02-06): Agrees with improving error messages when address decoding fails.
- rkrux (member) (2025-02-22): In favour of better address error parsing after experiencing confusing errors when debugging network-inconsistent keys.
- jonatack (member) (Concept ACK, 2025-02-22): Concept ACK [not substantive]
- l0rinc (member) (Concept ACK, 2025-07-17): We should indeed make the error messages more user friendly.
Participants:
- fanquake (member, neutral, 3 statements 2023-03-15 to 2024-06-25): asked about squashing commits and file permission changes; moved PR to draft
- maflcko (member, neutral, 4 statements 2023-05-18 to 2024-10-07): noted rebase needs and silent merge conflict; investigated CI warning
- luke-jr (contributor, objection, 5 statements 2023-06-22 to 2023-07-27): requested changes on chain terminology, prefix handling, and unsupported vs invalid error wording
- Sjors (member, support, 30 statements 2023-09-07 to 2026-01-09): Concept ACK; tested individual commits, provided a fixup commit, and advised on commit structure
- RandyMcMillan (contributor, support, 1 statements 2023-10-01 to 2023-10-01): Approach ACK
- achow101 (member, objection, 3 statements 2024-04-15 to 2025-10-22): objected to invalid Bech32 separator logic and to the maintenance burden of dynamically computing Base58 prefixes
- rkrux (member, support, 5 statements 2025-02-06 to 2025-02-22): Concept ACK; noted real-world debugging issues from unclear address/key network mismatch errors
- jonatack (member, support, 2 statements 2025-02-22 to 2025-02-22): Concept ACK
- l0rinc (member, objection, 29 statements 2025-07-17 to 2026-08-25): Concept ACK on intent, but requested changes blocking merge until commits are split into focused, reviewable refactors and behavior changes
- sedited (member, neutral, 1 statements 2026-04-20 to 2026-04-20): pinged reviewers for updates
Files
603 lines under test/bench/ci.
- src/key_io.cpp +154/-103
- test/functional/data/rpc_validateaddress.json +245/-0
- test/functional/rpc_validateaddress.py +28/-176
- src/test/bech32_tests.cpp +38/-38
- src/test/key_io_tests.cpp +37/-3
- test/functional/rpc_invalid_address_message.py +20/-16
- src/kernel/chainparams.cpp +30/-0
- src/bech32.cpp +14/-13
- src/bech32.h +20/-2
- src/kernel/chainparams.h +3/-0
- test/functional/wallet_basic.py +1/-1
Card
The short text the ranking pass compares across PRs.
RPC address validation and destination decoding return misleading, generic error messages when passed valid addresses intended for a different network, such as using a testnet address on mainnet. This occurred because Bitcoin Core dispatched between Bech32 and Base58 parsing using the active network's human-readable prefix, causing other networks' Bech32 addresses to fall through into the Base58 parser. This pull request modifies `DecodeDestination` to decode Bech32 upfront, changes `bech32::LocateErrors` to return structured error codes, and populates expected Base58 prefix text across chain parameters. As a result, users receive clear error messages detailing expected versus actual network prefixes as well as explicit diagnoses for mixed-case and checksum issues.
Data
thread read through 2026-08-25 (122 statements), head 5f7fdc05f6
Discussion read under schema 1 rules: suggestions and questions may be missing, and an objection may combine several harms (schema history).
labels: RPC/REST/ZMQ, Needs rebase · draft
Categories
RPC / REST / ZMQ
P3 · user request (ranking pass: Moved down 5: diagnostic message quality, below the additive fields that unblock tooling workarounds.)
Improves user-facing RPC error messages when addresses from different networks are supplied, resolving issue #26290. While clearer diagnostic messages prevent user confusion, the underlying address validation failure is not an operational breakage or critical bug.
Membership: Carries the RPC/REST/ZMQ label and directly modifies RPC address validation and error reporting behavior tested in `rpc_validateaddress.py` and `rpc_invalid_address_message.py`.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0
Utilities (logging, arguments, libraries)
P3 · cleanup (ranking pass: Moved up: misleading cross-network address errors are a genuine user-facing confusion, not just an interface cleanup.)
Refactors Bech32 error location into a structured `enum class Error` and removes prefix-guessing heuristics in `key_io`. This provides a cleaner error-reporting interface across address parsing utilities.
Membership: Touches shared address decoding and encoding utilities in `src/key_io.cpp` and `src/bech32.h`.
Factors: security/stability 0, bug 1, performance 0, user value 1, leverage 0