search

sipa / bitcoin

Bitcoin integration/staging tree
http://www.bitcoin.org
MIT License
88 stars 21 forks source link

policy: taproot: check prevout scriptPubKey for P2SH #121

Closed pinheadmz closed 4 years ago

pinheadmz commented 4 years ago

Addresses https://github.com/bitcoin/bitcoin/pull/17977#discussion_r384485907

Tests the actual previous transaction output scriptPubKey for P2SH before applying taproot standardness checks.

Also adds test for version-1-witness-in-P2SH which is NOT taproot and should not be subject to these standardness checks.

This is a hard case to test, because P2SH is correctly checked in interpreter VerifyWitnessProgram() and so the test case will be thrown out as non-standard for spending an unknown witness version.

To test the test, I wrote a commit https://github.com/pinheadmz/bitcoin/commit/ba1ac81d032632412d45fc9438552b7bcd2253f1 which bypasses the check in VerifyWitnessProgram(), allowing the v1-in-P2SH to get caught in policy IsWitnessStandard() which is patched by this PR.

I'm not sure if there's a better way to cover this patch because there's (at least) two standardness checks applied.

jnewbery commented 4 years ago

Yes, I think you're right that checking for !prevScript.IsPayToScriptHash() is a bug, and the change in IsWitnessStandard() seems correct to me. I haven't reviewed the tests yet.

jnewbery commented 4 years ago

Is this fixed by https://github.com/bitcoin/bitcoin/pull/17977#discussion_r396019344 ?

pinheadmz commented 4 years ago

@jnewbery looks like it -- if you're interested in the test I can rebase so the diff is just that? Or just close.

sipa commented 4 years ago

Oh, I missed this PR, sorry about that.

I think this is fixed, but I'll cherry-pick the test.

sipa commented 4 years ago

Done.