Improve address decoding errors #26514

pull aureleoules wants to merge 1 commits into bitcoin:master from aureleoules:2022-11-improve-address-decode-errors changing 3 files +15 −15

aureleoules commented at 6:33 PM on November 16, 2022: member

Attempt to fix #21741.
sipa commented at 7:26 PM on November 16, 2022: member

Concept ACK

in src/key_io.cpp:112 in 7d3944dac6 outdated

 106 | @@ -107,17 +107,17 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
 107 |                  std::equal(script_prefix.begin(), script_prefix.end(), data.begin())) ||
 108 |              (data.size() >= pubkey_prefix.size() &&
 109 |                  std::equal(pubkey_prefix.begin(), pubkey_prefix.end(), data.begin()))) {
 110 | -            error_str = "Invalid length for Base58 address";
 111 | +            error_str = "Invalid length for Base58 address (P2PKH or P2SH)";
 112 |          } else {
 113 | -            error_str = "Invalid prefix for Base58-encoded address";
 114 | +            error_str = strprintf("Invalid prefix for Base58-encoded address on %s. This may be an address for a different network. Valid address starts with `1` or `3` (mainnet) and `m` or `n` or `2` (testnet).", params.NetworkIDString());

luke-jr commented at 9:17 PM on November 19, 2022:

            error_str = strprintf("Invalid or unsupported Base58-encoded address.", params.NetworkIDString());

in src/key_io.cpp:118 in 7d3944dac6 outdated

 116 |          return CNoDestination();
 117 |      } else if (!is_bech32) {
 118 |          // Try Base58 decoding without the checksum, using a much larger max length
 119 |          if (!DecodeBase58(str, data, 100)) {
 120 | -            error_str = "Not a valid Bech32 or Base58 encoding";
 121 | +            error_str = "Not a valid Segwit (Bech32) or Base58 encoding. This may be an address for a different network.";

luke-jr commented at 9:18 PM on November 19, 2022:

            error_str = "Invalid or unsupported Segwit (Bech32) or Base58 encoding.";

in src/key_io.cpp:130 in 7d3944dac6 outdated

 126 | @@ -127,7 +127,8 @@ CTxDestination DecodeDestination(const std::string& str, const CChainParams& par
 127 |      if ((dec.encoding == bech32::Encoding::BECH32 || dec.encoding == bech32::Encoding::BECH32M) && dec.data.size() > 0) {
 128 |          // Bech32 decoding
 129 |          if (dec.hrp != params.Bech32HRP()) {
 130 | -            error_str = "Invalid prefix for Bech32 address";
 131 | +            error_str = strprintf("Invalid prefix for Segwit (Bech32) address on %s (expected %s, got %s). This may be an address for a different network.",

luke-jr commented at 9:18 PM on November 19, 2022:

            error_str = strprintf("Invalid or unsupported prefix for Segwit (Bech32) address on %s (expected %s, got %s).",

aureleoules commented at 7:42 AM on November 24, 2022:

I also removed the on %s (network)

luke-jr changes_requested
luke-jr commented at 9:22 PM on November 19, 2022: member

NACK on talking about "different networks". While this could in theory be Lightning or sidechains, it's more likely to lead users toward scamcoins.
DrahtBot commented at 9:22 PM on November 19, 2022: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK 1440000bytes, MarcoFalke, davidgumberg

Concept ACK sipa, willcl-ark

Ignored review luke-jr

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.
aureleoules force-pushed on Nov 24, 2022
aureleoules force-pushed on Nov 24, 2022
aureleoules commented at 7:37 AM on November 24, 2022: member

Thanks @luke-jr I addressed your suggestions.
DrahtBot added the label Needs rebase on Jan 17, 2023
Improve address decoding errors 962a0930e6
aureleoules force-pushed on Jan 17, 2023
DrahtBot removed the label Needs rebase on Jan 17, 2023
willcl-ark commented at 7:38 PM on March 9, 2023: contributor

Concept ACK

Type	Reviewers
ACK	1440000bytes, MarcoFalke, davidgumberg
Concept ACK	sipa, willcl-ark
Ignored review	luke-jr

davidgumberg commented at 1:49 PM on March 11, 2023: contributor

Maybe add a message for not-yet-understood witness versions and a test case?

  if (version == 1 && data.size() == WITNESS_V1_TAPROOT_SIZE) {
      static_assert(WITNESS_V1_TAPROOT_SIZE == WitnessV1Taproot::size());
      WitnessV1Taproot tap;
      std::copy(data.begin(), data.end(), tap.begin());
      return tap;
  }
+
+ if (version > 1 && version <= 16) {
+     error_str =  "Invalid or unsupported Bech32 address witness version."
+     return CNoDestination();
+ }
+
  if (version > 16) {
      error_str = "Invalid Bech32 address witness version";
      return CNoDestination();
  }

sipa commented at 1:51 PM on March 11, 2023: member

@davidgumberg That would be incorrect; sending to future witness versions is perfectly legal. It might make sense in the UI to give a warning, but we can't outlaw it (as it'd break future upgradability).
davidgumberg commented at 2:02 PM on March 11, 2023: contributor

@sipa Thanks! I misunderstood.
unknown approved
unknown commented at 3:31 PM on March 11, 2023: none

utACK https://github.com/bitcoin/bitcoin/pull/26514/commits/962a0930e699b74b3c4d019427df6e2b3af5c831

NACK on talking about "different networks". While this could in theory be Lightning or sidechains, it's more likely to lead users toward scamcoins.

Ideally it should have been 'chain' instead of network as networks are ipv4, ipv6, tor, i2p etc. although we should not be concerned about scammers if they ever use this software but use correct technical terms in errors IMO.

The comment is anyways addressed by author and I think PR improves error messages.
maflcko commented at 3:32 PM on March 13, 2023: member

lgtm ACK 962a0930e699b74b3c4d019427df6e2b3af5c831
davidgumberg commented at 3:40 PM on March 13, 2023: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26514/commits/962a0930e699b74b3c4d019427df6e2b3af5c831
glozow merged this on Mar 13, 2023
glozow closed this on Mar 13, 2023
aureleoules deleted the branch on Mar 13, 2023
sidhujag referenced this in commit 39cd5ddd7d on Mar 14, 2023
sidhujag referenced this in commit fc3998666a on Mar 15, 2023
bitcoin locked this on Mar 12, 2024

Contributors

Linked (view graph)

#21741 Error messages for invalid address

Improve address decoding errors #26514

Reviews

Conflicts