test: refactor: use `script_util` helpers for creating P2{PKH,SH,WPKH,WSH} scripts #22363

pull theStack wants to merge 4 commits into bitcoin:master from theStack:202106-test-refactor-use_scriptpubkey_helpers changing 10 files +134 −162

theStack commented at 6:55 PM on June 28, 2021: member

PR #18788 (commit 08067aebfd7e838e6ce6b030c31a69422260fc6f) introduced functions to generate output scripts for various types. This PR replaces all manual CScript creations in the P2PKH, P2SH, P2WPKH, P2WSH formats with those helpers in order to increase readability and maintainability over the functional test codebase. The first commit fixes a bug in the wallet_util helper module w.r.t. to P2SH-P2WSH script creation (the result is not used in any test so far, hence it can still be seen as refactoring).

The following table shows a summary of the output script patterns tackled in this PR:

Type	master branch	PR branch
P2PKH	`CScript([OP_DUP, OP_HASH160, hash160(key), OP_EQUALVERIFY, OP_CHECKSIG])`	`key_to_p2pkh_script(key)`
	`CScript([OP_DUP, OP_HASH160, keyhash, OP_EQUALVERIFY, OP_CHECKSIG])`	`keyhash_to_p2pkh_script(keyhash)`
P2SH	`CScript([OP_HASH160, hash160(script), OP_EQUAL])`	`script_to_p2sh_script(script)`
P2WPKH	`CScript([OP_0, hash160(key)])`	`key_to_p2wpkh_script(key)`
P2WSH	`CScript([OP_0, sha256(script)])`	`script_to_p2wsh_script(script)`

Note that the key_to_... helpers can't be used if an invalid key size (not 33 or 65 bytes) is passed, which is the case in some rare instances where the scripts still have to be created manually.

Possible follow-up ideas:

further simplify by identifying P2SH-wrapped scripts and using key_to_p2sh_p2wpkh_script() and script_to_p2sh_p2wsh_script() helpers
introduce and use key_to_p2pk_script() helper for P2PK scripts

test: wallet util: fix multisig P2SH-P2WSH script creation 61b6a017a9
test: use script_util helpers for creating P2PKH scripts b57b633b94
theStack force-pushed on Jun 28, 2021
theStack force-pushed on Jun 28, 2021
DrahtBot added the label Tests on Jun 28, 2021
fanquake commented at 1:30 AM on June 30, 2021: member

Concept ACK - if we've got these helpers for this, we might as well be using them.
practicalswift commented at 7:48 AM on June 30, 2021: contributor

Concept ACK
in test/functional/mempool_accept.py:30 in d7d3cd4f36 outdated
```
  24 | @@ -25,10 +25,12 @@
  25 |      OP_2,
  26 |      OP_3,
  27 |      OP_CHECKMULTISIG,
  28 | -    OP_EQUAL,
  29 |      OP_HASH160,
  30 |      OP_RETURN,
  31 |  )
  32 | +from test_framework.script_util import (
```
glozow commented at 10:47 AM on July 1, 2021:

In d7d3cd4f367a41f5e79e96dc7efa1ea8d493c344 use script_util helpers for creating P2SH scripts

nit: remove the unused import test_framework.script.hash160. You remove it later, but this commit fails the linter.

theStack commented at 12:49 PM on July 2, 2021:

Oops, my lazyness of never running the linter locally backfires ;) thanks for spotting out. Fixed.
glozow commented at 11:03 AM on July 1, 2021: member

Definite Concept ACK to reusing helper functions instead of creating scripts manually. I think we should add some unit tests to wallet_util.py, since it's not outside the realm of possibility for those util functions to have errors.

Good catch, but is the first commit (61b6a017a9f99ef072b2d1980dd547eb20093352 fix multisig P2SH-P2WSH script creation ) necessary? You overwrite it immediately in the next commit. Perhaps it'd be better to add a unit test that hits that case instead?
theStack force-pushed on Jul 2, 2021
theStack commented at 12:51 PM on July 2, 2021: member

Definite Concept ACK to reusing helper functions instead of creating scripts manually. I think we should add some unit tests to wallet_util.py, since it's not outside the realm of possibility for those util functions to have errors.

Agree that unit tests are a good idea here! This way we can also check that the sanity checks in the helpers (i.e. for key/keyhash/scripthash lengths) work.

Good catch, but is the first commit (61b6a01 fix multisig P2SH-P2WSH script creation ) necessary? You overwrite it immediately in the next commit. Perhaps it'd be better to add a unit test that hits that case instead?

The intention was to clearly separate "bugfixing" (in parantheses because the result is not used in any test right now) and refactoring. I thought if that is mixed in a single commit, it could be potentially confusing for reviewers if at this single instance the replacement of CScript doesn't match the P2SH pattern. Will maybe squash it with a mention of the bugfix in the commit message though when I touch again. Adding a unit test is any case a good idea :)
0xB10C commented at 2:20 PM on July 5, 2021: member

Concept ACK!
in test/functional/data/invalid_txs.py:55 in 15e3379a8a outdated
```
  48 | @@ -49,7 +49,10 @@
  49 |      OP_LSHIFT,
  50 |      OP_RSHIFT
  51 |  )
  52 | -basic_p2sh = sc.CScript([sc.OP_HASH160, sc.hash160(sc.CScript([sc.OP_0])), sc.OP_EQUAL])
  53 | +from test_framework.script_util import (
  54 | +    script_to_p2sh_script,
  55 | +)
  56 | +basic_p2sh = script_to_p2sh_script(sc.CScript([sc.OP_0]))
```
rajarshimaitra commented at 2:55 PM on July 5, 2021:

CScript is already imported. So just script_to_p2sh_script(CScript([sc.OP_0])) would work.

So can be done for OP_0 if its imported in the list above.

theStack commented at 6:48 PM on July 5, 2021:

Thanks for reviewing! Good point, got rid now of the redundant import script as sc and extended the list of OP_... imports accordingly, took also the chance to sort them. Should look cleaner now.

rajarshimaitra commented at 6:53 AM on July 6, 2021:

Looks good now.. Thanks for considering.
rajarshimaitra commented at 2:56 PM on July 5, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/22363/commits/15e3379a8ad9f7d0962af7e0a94b0a5e332b8743.

Just one nit.
test: use script_util helpers for creating P2SH scripts 285a65ccfd
test: use script_util helpers for creating P2W{PKH,SH} scripts 905d672b74
theStack force-pushed on Jul 5, 2021
rajarshimaitra commented at 6:54 AM on July 6, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/22363/commits/905d672b743edf31531d095ffe800449eaffec69
LarryRuane commented at 4:53 PM on July 8, 2021: contributor
tACK 905d672b743edf31531d095ffe800449eaffec69

Review club meeting notes and IRC log: https://bitcoincore.reviews/22363

This PR has many simple "substitution" changes that follow the same pattern, and I wasn't confident that my attention span was sufficient to review all of them. So I made the following sort of change (in a semi-automated way) to the substitution changes in this PR (reinstated the old code and asserted that the new code created the same result):
```
-            spk = keyhash_to_p2pkh_script(pubkeyhash)
+            spk = CScript([OP_DUP, OP_HASH160, pubkeyhash, OP_EQUALVERIFY, OP_CHECKSIG])
+            assert_equal(spk, keyhash_to_p2pkh_script(pubkeyhash))
```
With these changes, test_runner.py passes, so I'm pretty confident the changes are all correct. (Actually, I had some timeouts, but I'm pretty sure it's just because my system is too slow.)

I also searched for places where these changes may have been missed and did not find any, so I think this PR is complete. Good PR!
in test/functional/feature_taproot.py:465 in 905d672b74
```
 461 | @@ -461,13 +462,13 @@ def make_spender(comment, *, tap=None, witv0=False, script=None, pkh=None, p2sh=
 462 |              # P2WPKH
 463 |              assert script is None
 464 |              pubkeyhash = hash160(pkh)
 465 | -            spk = CScript([OP_0, pubkeyhash])
 466 | +            spk = key_to_p2wpkh_script(pkh)
```
0xB10C commented at 5:59 PM on July 8, 2021:

Note: I first thought we are passing a public-key-hash to key_to_p2wpkh_script() as the variable is named pkh. Printing pkh shows that it is an uncompressed public key and the function docs say pkh: the public key for P2PKH or P2WPKH spending too. (nothing to do here)
0xB10C commented at 7:48 PM on July 8, 2021: member

ACK 905d672b743edf31531d095ffe800449eaffec69

Tested and reviewed the changes. Searched for missed P2{PKH,SH,WPKH,WSH} constructions by grepping for opcodes and CScript([.

in test/functional/test_framework/wallet_util.py:85 in 285a65ccfd outdated

  81 | @@ -83,7 +82,7 @@ def get_generate_key():
  82 |                 p2pkh_addr=key_to_p2pkh(pubkey),
  83 |                 p2wpkh_script=CScript([OP_0, pkh]).hex(),
  84 |                 p2wpkh_addr=key_to_p2wpkh(pubkey),
  85 | -               p2sh_p2wpkh_script=CScript([OP_HASH160, hash160(CScript([OP_0, pkh])), OP_EQUAL]).hex(),
  86 | +               p2sh_p2wpkh_script=script_to_p2sh_script(CScript([OP_0, pkh])).hex(),

MarcoFalke commented at 8:50 AM on July 9, 2021:

unrelated nit in 285a65ccfde2e811cfe01e916b998c02ee534a97:

Could use script_to_p2sh_script(p2wpkh_script) to avoid duplication and to clarify that p2sh redeem script is equal to the scriptPubKey (OP_0 + witness program)?

in test/functional/p2p_segwit.py:491 in 905d672b74
```
 488 | @@ -488,8 +489,7 @@ def test_v0_outputs_arent_spendable(self):
 489 |  
 490 |          # Create two outputs, a p2wsh and p2sh-p2wsh
 491 |          witness_program = CScript([OP_TRUE])
```
MarcoFalke commented at 9:03 AM on July 9, 2021:

Unrelated comment in 905d672b743edf31531d095ffe800449eaffec69:

This is wrong. This is the preimage to the witness program. According to BIP 141:

... 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".

The proper name is witnessScript:

The last item in the witness (the "witnessScript") is popped off, hashed with SHA256 ...

fanquake commented at 6:13 AM on July 12, 2021:

Being updated in #22429.
MarcoFalke approved