Enforce segwit v0 script validity when creating address.

[nlanson]

Adds a check in Address::from_script() that checks if segwit v0 scripts have a valid length.

Fix: #995

[nlanson]

Related discussion: #648
I believe Address should guarantee validity, so from_script is wrong. It'd be also great to return Result instead of Option but that's for another PR.
Originally posted by @Kixunil in #995 (comment)

I will also do this in a separate PR.

[Eunoia1729]

Was working on the same issue today. Looks good to me!
tACK 971c285

What do you think about adding tests for the same?

[dpc]

It might not be to be done in this PR, but if we have multiple ways of "not turning script into an address", maybe we should make it enum Error { ... list all the ways things can go wrong ... }, so the caller has something better to show to the user than just "didn't turn into address".

[nlanson]

Agreed. Address::from_script() and a few other functions should return Result<_, Error> instead of Option<> but this is for another PR.

[dpc]

#1022 just to not forget.

[nlanson]

What do you think about adding tests for the same?

I have left out tests as I thought the change wasn't so significant and existing tests covered cases where it won't fail, however I am happy to add tests for cases where it is expected to fail and squash it into the commit.

[Kixunil]

tests for cases where it is expected to fail

This would be really great

[apoelstra]

ACK 971c285

[apoelstra]

ACK, though I agree that tests would be greatly appreciated

[nlanson]

6c10d77 Squashed in unit tests and checks to see if scripts are valid in general if witness version is v0.

I also had a play around with trying to add similar checks for v1 scripts for consistency, however adding the checks would fail one case in test_bip173_350_vectors() so I have left it out for now. However, I think that it would be best to be consistent across v0 and v1 scripts and fail if v1 scripts are invalid as well.

The particular case that fails when the v1 check is present is: ("bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kt5nd6y", "5128751e76e8199196d454941c45d1b3a323f1433bd6751e76e8199196d454941c45d1b3a323f1433bd6") .
This fails since the address successfully converts into a script through the Address::from_str() function, but the script cannot be converted back into an address through the Address::from_script() function as the script is not a valid p2tr script. Might be intentional but there is a missing check for v1 witness program lengths in Address::from_str()?

[tcharding]

ACK 6c10d77

[tcharding]

@nlanson I change the issue mention in the PR desciption so that GitHub would pick it up and auto-close the issue when merging. Hope you don't mind.

[sanket1729]

We also need to check the length of witness program when creating a witness variant of Payload.

Accoridng to definition of witness program in BIP141

A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that consists of a 1-byte push opcode (for 0 to 16) followed by a data push between 2 and 40 bytes gets a new special meaning. The value of the first push is called the "version byte". The following byte vector pushed is called the "witness program".

We need to check the between 2 and 40 bytes constraint in the Payload::from_script function too.

[sanket1729]

ACK 6c10d77. Left a comment can be addressed in separate PR.

[apoelstra]

ACK 6c10d77

[tcharding]

We also need to check the length of witness program when creating a witness variant of Payload.

@nlanson do you want this one? Just asking so I dont tread on your toes, cheers.

[nlanson]

We also need to check the length of witness program when creating a witness variant of Payload.

@nlanson do you want this one? Just asking so I dont tread on your toes, cheers.

I believe this is already done: #1022 (comment)