Improve address decoding errors

[aureleoules]

Attempt to fix #21741.

[sipa]

Concept ACK

[luke-jr]

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]

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]

Thanks @luke-jr I addressed your suggestions.

[willcl-ark]

Concept ACK

[davidgumberg]

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]

@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]

@sipa Thanks! I misunderstood.

[ghost]

utACK 962a093

---

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]

lgtm ACK 962a093

[davidgumberg]

ACK 962a093