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

theStack · 2021-06-28T18:55:35Z

PR #18788 (commit 08067ae) 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

fanquake · 2021-06-30T01:30:45Z

Concept ACK - if we've got these helpers for this, we might as well be using them.

practicalswift · 2021-06-30T07:48:47Z

Concept ACK

glozow

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 (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?

test/functional/mempool_accept.py

theStack · 2021-07-02T12:51:53Z

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 · 2021-07-05T14:20:06Z

Concept ACK!

rajarshimaitra

tACK 15e3379.

Just one nit.

test/functional/data/invalid_txs.py

rajarshimaitra · 2021-07-06T06:54:01Z

tACK 905d672

LarryRuane · 2021-07-08T16:53:32Z

tACK 905d672

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!

0xB10C

ACK 905d672

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

0xB10C · 2021-07-08T17:59:05Z

test/functional/feature_taproot.py

            assert script is None
            pubkeyhash = hash160(pkh)
-            spk = CScript([OP_0, pubkeyhash])
+            spk = key_to_p2wpkh_script(pkh)


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)

maflcko

review ACK 905d672 🕹

Show signature and timestamp

Signature:

Timestamp of file with hash 9c7d420ec15f245669577f72ecacc3562260e8ea7e288845b37518b892345567 -

maflcko · 2021-07-09T08:50:37Z

test/functional/test_framework/wallet_util.py

               p2wpkh_script=CScript([OP_0, pkh]).hex(),
               p2wpkh_addr=key_to_p2wpkh(pubkey),
-               p2sh_p2wpkh_script=CScript([OP_HASH160, hash160(CScript([OP_0, pkh])), OP_EQUAL]).hex(),
+               p2sh_p2wpkh_script=script_to_p2sh_script(CScript([OP_0, pkh])).hex(),


unrelated nit in 285a65c:

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)?

maflcko · 2021-07-09T09:03:05Z

test/functional/p2p_segwit.py

        self.disconnect_nodes(0, 2)

        # Create two outputs, a p2wsh and p2sh-p2wsh
        witness_program = CScript([OP_TRUE])


Unrelated comment in 905d672:

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

Being updated in #22429.

…ting …

…s/witness_program/witness_script/) 8a2b58d test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner) Pull request description: This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program) is defined as follows: > 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".** In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](bitcoin/bitcoin#22363 (comment))! Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f3, PR #8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program This was fixed in PR bitcoin/bips#416 later. So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests. ACKs for top commit: josibake: tACK bitcoin/bitcoin@8a2b58d Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b

…s_program/witness_script/) 8a2b58d test: fix segwit terminology (s/witness_program/witness_script/) (Sebastian Falbesoner) Pull request description: This PR fixes wrong uses of the term "witness program", which according to [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Witness_program) is defined as follows: > 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".** In most cases where "witness program" is used in tests (concerns comments, variable names and in one instance even a function name) what we really want to denote is the "witness script". Thanks to [MarcoFalke for pointing this out in a review comment](bitcoin#22363 (comment))! Some historical background: At the time when the P2P segwit tests were first introduced (commit 330b0f3, PR bitcoin#8149), the term "witness program" was not used consistently in BIP141: https://bitcoin.stackexchange.com/questions/46451/what-is-the-precise-definition-of-witness-program This was fixed in PR bitcoin/bips#416 later. So in some way, this PR can be seen as a very late follow-up to the BIP141 fix that also reflects these changes in the tests. ACKs for top commit: josibake: tACK bitcoin@8a2b58d Tree-SHA512: f36bb9e53d1b54b86bfa87ec12f33e3ebca64b5f59d97e9662fe35ba12c25e1c9a4f93a5425d0eaa3879dce9e50368d345555b927bfab76945511f873396892b

…ting P2PK scripts 429b493 test: introduce script_util helper for creating P2PK scripts (Sebastian Falbesoner) Pull request description: This PR is a follow-up to #22363, which took use of already existing `script_util` helpers to get rid of manual CScript for the P2{PKH,SH,WPKH,WSH} output types, in order to increase readability and maintainability of the test code. Here the same is done for P2PK scripts by introducing a helper `key_to_p2pk_script` and using it. Note that the helper only accepts ECDSA pubkeys (i.e. ones with a size of 33 or 65 bytes), hence it can't be used for scripts in the form of [x-only-pubkey, OP_CHECKSIG]. ACKs for top commit: brunoerg: Code review ACK 429b493 laanwj: Code review ACK 429b493 rajarshimaitra: Concept + tACK 429b493 Tree-SHA512: 984aea01eba5f38a328d69905d90a3a36f0a02419ca3e5baf3c8095895fb094e3780c7da16fad5851db3847bdb05ce8cda244ab09b79b8aa9602dfb3c5e0414c

…or creating P2PK scripts 429b493 test: introduce script_util helper for creating P2PK scripts (Sebastian Falbesoner) Pull request description: This PR is a follow-up to bitcoin#22363, which took use of already existing `script_util` helpers to get rid of manual CScript for the P2{PKH,SH,WPKH,WSH} output types, in order to increase readability and maintainability of the test code. Here the same is done for P2PK scripts by introducing a helper `key_to_p2pk_script` and using it. Note that the helper only accepts ECDSA pubkeys (i.e. ones with a size of 33 or 65 bytes), hence it can't be used for scripts in the form of [x-only-pubkey, OP_CHECKSIG]. ACKs for top commit: brunoerg: Code review ACK 429b493 laanwj: Code review ACK 429b493 rajarshimaitra: Concept + tACK bitcoin@429b493 Tree-SHA512: 984aea01eba5f38a328d69905d90a3a36f0a02419ca3e5baf3c8095895fb094e3780c7da16fad5851db3847bdb05ce8cda244ab09b79b8aa9602dfb3c5e0414c

…are multisig scripts 4718897 test: add script_util helper for creating bare multisig scripts (Sebastian Falbesoner) Pull request description: This PR is a follow-up to #22363 and #23118 and introduces a helper `keys_to_multisig_script` for creating bare multisig outputs in the form of ``` OP_K PubKey1 PubKey2 ... PubKeyN OP_N OP_CHECKMULTISIG ``` The function takes a list of pubkeys (both hex- and byte-strings are accepted due to the `script_util.check_key` helper being used internally) and optionally a threshold _k_. If no threshold is passed, a n-of-n multisig output is created, with _n_ being the number of passed pubkeys. ACKs for top commit: shaavan: utACK 4718897 rajarshimaitra: tACK 4718897 Tree-SHA512: b452d8a75b0d17316b66ac4ed4c6893fe59c7c417719931d4cd3955161f59afca43503cd09b83a35b5a252a122eb3f0fbb9da9f0e7c944cf8da572a02219ed9d

…ating bare multisig scripts 4718897 test: add script_util helper for creating bare multisig scripts (Sebastian Falbesoner) Pull request description: This PR is a follow-up to bitcoin#22363 and bitcoin#23118 and introduces a helper `keys_to_multisig_script` for creating bare multisig outputs in the form of ``` OP_K PubKey1 PubKey2 ... PubKeyN OP_N OP_CHECKMULTISIG ``` The function takes a list of pubkeys (both hex- and byte-strings are accepted due to the `script_util.check_key` helper being used internally) and optionally a threshold _k_. If no threshold is passed, a n-of-n multisig output is created, with _n_ being the number of passed pubkeys. ACKs for top commit: shaavan: utACK 4718897 rajarshimaitra: tACK bitcoin@4718897 Tree-SHA512: b452d8a75b0d17316b66ac4ed4c6893fe59c7c417719931d4cd3955161f59afca43503cd09b83a35b5a252a122eb3f0fbb9da9f0e7c944cf8da572a02219ed9d

theStack added 2 commits June 28, 2021 20:14

test: wallet util: fix multisig P2SH-P2WSH script creation

61b6a01

test: use script_util helpers for creating P2PKH scripts

b57b633

theStack force-pushed the 202106-test-refactor-use_scriptpubkey_helpers branch 2 times, most recently from 7494fba to 6c0038d Compare June 28, 2021 19:15

DrahtBot added the Tests label Jun 28, 2021

glozow reviewed Jul 1, 2021

View reviewed changes

test/functional/mempool_accept.py Outdated Show resolved Hide resolved

theStack force-pushed the 202106-test-refactor-use_scriptpubkey_helpers branch from 6c0038d to 15e3379 Compare July 2, 2021 12:48

rajarshimaitra reviewed Jul 5, 2021

View reviewed changes

test/functional/data/invalid_txs.py Outdated Show resolved Hide resolved

theStack added 2 commits July 5, 2021 20:40

test: use script_util helpers for creating P2SH scripts

285a65c

test: use script_util helpers for creating P2W{PKH,SH} scripts

905d672

theStack force-pushed the 202106-test-refactor-use_scriptpubkey_helpers branch from 15e3379 to 905d672 Compare July 5, 2021 18:41

rajarshimaitra mentioned this pull request Jul 6, 2021

test: add tests for bad-txns-prevout-null reject reason #22408

Merged

0xB10C reviewed Jul 8, 2021

View reviewed changes

maflcko approved these changes Jul 9, 2021

View reviewed changes

maflcko merged commit d1e4c56 into bitcoin:master Jul 9, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 10, 2021

Merge bitcoin#22363: test: refactor: use script_util helpers for crea…

b04eeed

…ting …

theStack mentioned this pull request Jul 11, 2021

test: refactor: fix segwit terminology (s/witness_program/witness_script/) #22429

Merged

theStack deleted the 202106-test-refactor-use_scriptpubkey_helpers branch July 31, 2021 20:05

theStack mentioned this pull request Sep 28, 2021

test: refactor: introduce script_util helper for creating P2PK scripts #23118

Merged

theStack mentioned this pull request Oct 18, 2021

test: refactor: add script_util helper for creating bare multisig scripts #23305

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge d1e4c56 into merged_master (Bitcoin PR bitcoin/bitcoin#22363)

2e6ff43

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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

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

Uh oh!

Conversation

theStack commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Jun 30, 2021

Uh oh!

practicalswift commented Jun 30, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented Jul 2, 2021

Uh oh!

0xB10C commented Jul 5, 2021

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rajarshimaitra commented Jul 6, 2021

Uh oh!

LarryRuane commented Jul 8, 2021

Uh oh!

0xB10C left a comment

Choose a reason for hiding this comment

Uh oh!

0xB10C Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

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

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

theStack commented Jun 28, 2021 •

edited

Loading

0xB10C Jul 8, 2021 •

edited

Loading