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

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202109-test-add_helper_for_creating_p2pk_scripts changing 7 files +26 −17

theStack commented at 11:55 AM on September 28, 2021: member

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].
fanquake added the label Tests on Sep 28, 2021
in test/functional/test_framework/script_util.py:38 in b62da41633 outdated
```
  34 | @@ -25,6 +35,10 @@
  35 |  DUMMY_P2WPKH_SCRIPT = CScript([b'a' * 21])
  36 |  DUMMY_2_P2WPKH_SCRIPT = CScript([b'b' * 21])
  37 |  
  38 | +def key_to_p2pk_script(key, main = False):
```
MarcoFalke commented at 12:27 PM on September 28, 2021:

what is main?

theStack commented at 12:51 PM on September 28, 2021:

It seems that the main parameter for the script_util helpers is only used in test_framework/address.py where the mainnet boolean parameters is passed-through (for whatever reason...). Since P2PK addresses don't exist, it can be removed. Done.
theStack force-pushed on Sep 28, 2021
DrahtBot commented at 6:13 PM on September 28, 2021: member



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

No conflicts as of last run.
DrahtBot added the label Needs rebase on Sep 29, 2021
test: introduce script_util helper for creating P2PK scripts 429b49378e
theStack force-pushed on Sep 29, 2021
theStack commented at 12:29 PM on September 29, 2021: member

Rebased on master.
DrahtBot removed the label Needs rebase on Sep 29, 2021
in test/functional/test_framework/script_util.py:15 in 429b49378e
```
  14 | +    OP_DUP,
  15 |      OP_EQUAL,
  16 |      OP_EQUALVERIFY,
  17 | +    OP_HASH160,
  18 | +    hash160,
  19 | +    sha256,
```
rajarshimaitra commented at 5:28 PM on October 2, 2021:

Any specific reason for this refactoring?

theStack commented at 6:07 PM on October 2, 2021:

AFAIK, we follow the common practice of keeping the Python imports sorted in lexicographical order. I don't know though if we have written that down anywhere in our docs as guidelines.
rajarshimaitra commented at 5:33 PM on October 2, 2021: contributor

Concept + tACK https://github.com/bitcoin/bitcoin/pull/23118/commits/429b49378ee3a3d73abe276cfd176c1ca08bf9b9
brunoerg commented at 8:02 PM on October 4, 2021: member

Code review ACK 429b49378ee3a3d73abe276cfd176c1ca08bf9b9
laanwj commented at 1:41 PM on October 7, 2021: member

Code review ACK 429b49378ee3a3d73abe276cfd176c1ca08bf9b9
laanwj merged this on Oct 7, 2021
laanwj closed this on Oct 7, 2021
sidhujag referenced this in commit 29ee69d024 on Oct 7, 2021
MarcoFalke referenced this in commit ab25ef8c7f on Oct 27, 2021
sidhujag referenced this in commit a6072b3e2d on Oct 27, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

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

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

Conflicts