mempool: check p2wsh standard

[ChrisCho-H]

Change Description

AFAIK, btcd curretly doesn't check standard rule for witness unlike bitcoin core. I add method CheckWitnessStandard to check standard rule for p2wsh, and execute it inside the method validateStandardness for mempool validation.

Steps to Test

If this PR makes sense, I will add test vectors following the reviews.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.

📝 Please see our Contribution Guidelines for further guidance.

[coveralls]

Pull Request Test Coverage Report for Build 15042853022

Details

• 11 of 46 (23.91%) changed or added relevant lines in 2 files are covered.
• 86 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.2%) to 56.539%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
mempool/mempool.go	2	13	15.38%
mempool/policy.go	9	33	27.27%

Files with Coverage Reduction	New Missed Lines	%
btcutil/gcs/gcs.go	1	80.95%
mempool/mempool.go	1	65.87%
rpcclient/infrastructure.go	84	39.79%

Totals
Change from base Build 14871723979:	-0.2%
Covered Lines:	31057
Relevant Lines:	54930

---

💛 - Coveralls

[kcalvinalvin]

No need for the j here or the branch checking for j != len(txIn.Witness)-1.

[ChrisCho-H]

It's to exclude the last element of witness stack, which is witness script.
Core doesn't check the last element either.

[ChrisCho-H]

I refactored a bit not to use j by excluding the last element of the range to iterate(the logic is same).
https://github.com/btcsuite/btcd/pull/2371/files#diff-129399aad69c6063744078caac62b466d7db74e07f9fe7a3bd6dc99f00a8f8fbR436

[kcalvinalvin]

Maybe leave a comment that this should be called after the inputs are checked?

Otherwise you'd get a runtime panic here.

[ChrisCho-H]

Thanks! I added the comment.
https://github.com/btcsuite/btcd/pull/2371/files#diff-129399aad69c6063744078caac62b466d7db74e07f9fe7a3bd6dc99f00a8f8fbR401

[kcalvinalvin]

Verified that the added logic implements just the P2WSH standardness check in Core.

https://github.com/bitcoin/bitcoin/blob/725c9f7780e0def3d79be151c08a36fe7a9dc59c/src/policy/policy.cpp#L264-L275

It does seem like the original commit is very old. Not quite sure about adding it but am indifferent if it does get added. bitcoin/bitcoin#8499

[Roasbeef]

Not quite sure about adding it but am indifferent if it does get added.

FWIW, there're quite a few newer standardness rules that bitcoind has added over the years that we never adopted. The new rules are generally a moving target, and don't have BIP implementations so all we have is the code to go for.

IMO, the only standardness/policy rules that are actually important, are those that directly impact the relay of time-sensitive transactions across the network. One such example is: TRUC and co.

[ChrisCho-H]

Original PR of bitcoind explains the purpose of standardness/policy rules of p2wsh like following

• 3600 bytes witnessScript and 100 stack items are adequate for a n-of-100 multisig using OP_CHECKSIG, OP_ADD, and OP_EQUAL.
• The max size for ECDSA signature is 73 bytes and nothing should use more than that with the current scripting language.
• This is to prevent abuse of witness space, and reduce the risks of DoS attack with some unknown special and big scripts.

I agree that absence of this doesn't directly impact, but what explained in bitcoind also makes sense to mitigate the possible risks.