SegwitAddress: include length check for Taproot witness programs

[johnzweng]

Hi @schildbach ! I have another PR, this time a very little change, but a little bit more reasoning behind it. So I'm very curious to hear your feedback. 🙂

Overview

• This PR just adds a size check for Segwit v1 (Taproot) addresses to the SegwitAddress constructor so that it only accepts 32-byte long witness programs (the only valid length for Taproot as defined in BIP 341).
• Adding this check means that AddressParser.parseAddress(...) will in future throw an exception for too short or too long Segwit v1 addresses (as it already does today for Segwit v0 addresses with invalid lengths). Currently too short/long Taproot addresses are accepted as valid by AddressParser.parseAddress(...).

Current state

• Currently in bitcoinJ there is already a check for witness program length for Segwit v0 in the constructor. So it is impossible to instantiate a v0 SegwitAddress with an invalid length.
• This is not the case for Taproot addresses (Segwit v1), so one can instantiate a SegwitAddress instance with too short or too long Taproot addresses without exception.
• I am aware of the fact that there is a semantic difference in v0 an v1 addresses with invalid lengths:
  • Segwit v0: BIP 141 specifies that all v0 witness programs with invalid length (not 20 or 32 bytes) MUST fail ("the script must fail"). This means: any coins sent to such an address will become unspendable.
  • Segwit v1: This is different from v0: BIP 341 specifies that Taproot validation rules only apply to witness programs with a length of 32 bytes. This implies that if the witness program has any other length, then no rules will be applied to v1 witness scripts. In short: sending coins to too long or too short Taproot addresses will become anyonce-can-spend (unless changed in a future soft fork).
  • These BIP descriptions match the code in Bitcoin Core, where invalid length v0 scripts will immediately fail here in the else branch and invalid v1 lengths are just not covered by this if-condition (which includes length) so that they end up below in the default else branch returning always true.
• Nevertheless, v1 addresses with invalid lengths are still covered by transaction policy by marking them as non-standard (the flag SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM is set here in policy.h, which will lead to returning an error on validation here). This makes it more difficult to spend such an invalid Taproot anyone-can-spend address as nodes will not accept such transactions in their mempool by default.

Rationale

I was unsure if this PR is reasonable (and you might still argue about it) as BIP 341 not explicitly marks these scripts as invalid. But from a practical point of view I still would argue that we should treat them as invalid addresses. Implementors of Bitcoin services (ATMs, exchanges) use AddressParser.parseAddress(...) as an easy check to see if a given address is valid. And while invalid-length-v1 addresses technically still might be valid I think in most cases it is not what the user intends.

So if there are buggy wallets out there which create crap addresses (example: v1 addresses with length 20 like v0), it would be nice if such addresses would raise attention (to prevent services from just blindly sending coins there). Many callers of AddressParser.parseAddress(...) may not dive that deep into these semantics and just use it as a simple validation check (without the need for a deeper understanding).

I also understand if you don't agree with my reasoning and don't accept this PR. But then maybe we could add some "dangerous" flag, so that validation fails by default on such addresses (and also currently unused Segwit versions) but instantiation of such addresses can still be enforced by providing the "dangerous" flag (or something similiar).

[johnzweng]

Note: I just force-pushed again, I had overlooked an existing test in SegwitAddressTest which failed with my new check. It should work now.

[schildbach]

Thanks for this PR!

I think the intention of BIP 341 to not strongly forbid non-32-byte programs is extensibility. They want to be able to softfork future script types.

But you're right, with bitcoinj being used as a client (rather than a full bitcoin node), we should probably follow policy more closely than consensus – we've done so in other areas as well. This would mean that if a new script type comes up and a consumer of bitcoinj wants to use it, the consumer would be forced to upgrade to a newer bitcoinj (if it already exists). But I guess that's ok.

So I agree with this PR.

I've added one review comment inline, but it's a really minor one. Thanks specifically for the intensive documentation and rationale – it really helps with reviewing!

[johnzweng]

Thanks specifically for the intensive documentation and rationale – it really helps with reviewing!

You're welcome! 🙂 I know from a reviewer's perspective how valuable this is.
(Also this makes it easier for other readers to follow the reasoning behind changes.)

[schildbach]

Side note: I think segwit v0 used to be extensible too, but they found a vulnerability in the bech32 format. The existing 20 and 32 byte programs were luckily not affected. So the fix was to "close" v0 by restricting it to these 2 usages, and add a new fixed and extensible v1 + bech32m.

[johnzweng]

Side note: I think segwit v0 used to be extensible too, but they found a vulnerability in the bech32 format. The existing 20 and 32 byte programs were luckily not affected. So the fix was to "close" v0 by restricting it to these 2 usages, and add a new fixed and extensible v1 + bech32m.

Interesting learning of Bitcoin history. :) I was aware that bech32m was introduced because of this vulnerability, but didn't know that v0 had been planned to be extensible in the first place.

[schildbach]

Merged! Let's see if someone complains about future script types…

[johnzweng]

Let's see if someone complains about future script types…

Aaaaand here we come: bc1pfeessrawgf entered the chat.. 😄

[schildbach]

bc1pfeessrawgf

https://stacker.news/items/593226
https://mempool.space/tx/b10c0000004da5a9d1d9b4ae32e09f0b3e62d21a5cce5428d4ad714fb444eb5d

[johnzweng]

I am impressed how much love was put into every detail of this specific transaction. 😃

Later on the creator also published a stacker.news article where he reveleaed even more details: https://stacker.news/items/600187

Mindblowing! 😊