Improve address decoding errors#26514
Conversation
|
Concept ACK |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
NACK on talking about "different networks". While this could in theory be Lightning or sidechains, it's more likely to lead users toward scamcoins.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
aureleoules
force-pushed
the
2022-11-improve-address-decode-errors
branch
2 times, most recently
from
5304de4 to
43e47ad
Compare
|
Thanks @luke-jr I addressed your suggestions. |
aureleoules
force-pushed
the
2022-11-improve-address-decode-errors
branch
from
43e47ad to
962a093
Compare
|
Concept ACK |
|
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();
} |
|
@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). |
|
@sipa Thanks! I misunderstood. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
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.
|
lgtm ACK 962a093 |
|
ACK 962a093 |
8 participants
Attempt to fix #21741.