Add taproot checks to Address::from_script
#1029
Conversation
Recently we added segwit v0 script checks to the `Address::from_script` method, following the principle that we want to provide some safety for users in the 'main' rust-bitcoin API. At the time we did not add taproot checks because doing so broke one of the test vector unit tests. Turns out the unit test was doing roundtripping of an address -> scipt_pubkey() -> address which is not strictly required by BIP-350 (where the test vector comes from). Modify the unit test by adding a boolean flag indicating whether the address should roundtrip and only roundtrip test such addresses. Add taproot script validity check to the `Address::from_script` method.
We recently decided to provide additional safety for users by not creating addresses from certain invalid scripts. Add rustdocs describing the return value and the safety guarantees we are explicitly providing so as not to surprise users. (See GitHub issue rust-bitcoin#955 for more context.)
Creating an address from an invalid taproot script should fail. Add a unit test to verify it does.
|
Maybe in another PR, but I think rust-bitcoin/src/util/address.rs Lines 837 to 840 in 99ae48a |
|
Addresses can come from other places (outside of this library) so I think we still want to support parsing unspendable addresses. @apoelstra can you confirm please? |
There was a problem hiding this comment.
Sorry for the absence. We don't need anything special for the taproot addresses. In fact, removing the non 32-byte witness v1 address would be incorrect.
From BIP341
Any other outputs, including version 1 outputs with lengths other than 32 bytes, or P2SH-wrapped version 1 outputs[3], remain unencumbered.
| @@ -676,6 +676,10 @@ impl Address { | |||
| if script.witness_version() == Some(WitnessVersion::V0) && !(script.is_v0_p2wpkh() || script.is_v0_p2wsh()) { | |||
| return None | |||
| } | |||
|
|
|||
| if script.witness_version() == Some(WitnessVersion::V1) && !script.is_v1_p2tr() { | |||
There was a problem hiding this comment.
This was discussed extensively previously. I will try to dig up references. Doing this is wrong because non-32 bytes WitnessVersion::V1 address types are still available for future usage.
@nlanson, this is incorrect. See the above comment. There is a difference between segwitv0 addresses and taproot addresses. Non 20/32 byte programs segwitv0 programs are invalid. But non-32 byte taproot programs are still valid programs and can used in future upgrades. @tcharding, if we really want to do safe guard the user. We can check whether the 32 byte program is actually a point on the curve. But I am not entirely sure about this because there may be use cases for representing non-curve points as addresses and it is probably not worth the effort. See a related previous attempt #668 and a discussion issue #648 |
|
Thanks for the info @sanket1729. Just reread the relevant part of BIP341 and now understand why this is not necessary for Taproot scripts. It was a detail that completely slipped through my mind. |
|
Agree, the difference is one is anyone can spend, the other is nobody can spend. Adding the check would also conflict with the documentation which describes unspendable addresses but doesn't mention anyone can spend. I wouldn't mind having a method |
|
Closing this one, thanks for the reviews and info. |
Draft because includes #1021
#1021 adds segwit v0 checks to
Address::from_script. This PR adds taproot checks and documentation on why we do the checks.This was done while I was attempting to write a decent issue. Is related to #1021 (obviously) and also to (#995).
cc @nlanson