test: add P2PK support to MiniWallet

theStack commented at 5:49 pm on May 13, 2021: member

This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, as suggested by MarcoFalke. Using that mode in the test feature_csv_activation.py, all txs submitted to the mempool follow the standard policy, i.e. -acceptnonstdtxn=1 can be removed.

Possible follow-ups:

Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)

DrahtBot added the label Tests on May 13, 2021

in test/functional/feature_csv_activation.py:103 in 6c0a1b7957 outdated

101@@ -103,12 +102,14 @@ def create_self_transfer_from_utxo(self, input_tx):
102     def create_bip112special(self, input, txversion):
103         tx = self.create_self_transfer_from_utxo(input)

reemuru commented at 2:06 am on May 17, 2021:

Hey, I’m hoping to be new contributor someday. Just taking a peek at some PRs. Could these original tx assignments be removed since it is being re-assigned?

0tx = self.create_self_transfer_from_utxo(input)

theStack commented at 11:13 am on May 17, 2021:

Hi reemuru, thanks for taking a look and welcome as new contributor! Note that the tx instance created in this line is used on the right-hand-side of the re-assignment two lines below, i.e. it serves as input for the sign_tx method call: tx = self.miniwallet.sign_tx(tx). The best would probably be if sign_tx modifies the tx in-place and doesn’t return anything, leading to simply self.miniwallet.sign_tx(tx) to have cleaner code and reduce confusion. Will check if that is possible.

in test/functional/feature_csv_activation.py:93 in 6c0a1b7957 outdated

89@@ -90,7 +90,6 @@ def set_test_params(self):
90         self.setup_clean_chain = True
91         self.extra_args = [[
92             '-whitelist=noban@127.0.0.1',
93-            '-acceptnonstdtxn=1',

MarcoFalke commented at 7:16 am on May 17, 2021:

Nice. Thanks for working on this.

I guess an alternative to consider might be to use the option to ignore specific reject reasons, when it is available. #20753

MarcoFalke commented at 7:18 am on May 17, 2021: member

Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)

I think that test also uses the mempool, which doesn’t like raw scripts. I guess an alternative to that is to wrap all scripts into a hashed output first, which is cumbersome :(. Or ignore specific reject reasons, when available.

MarcoFalke deleted a comment on May 17, 2021

michaelfolkson commented at 9:46 am on May 17, 2021: contributor

Concept ACK. A clear improvement to be able to generate P2PK outputs in the functional tests in addition to anyone-can-spend outputs.

theStack commented at 2:14 pm on May 17, 2021: member

Thanks for the reviews! #20753 looks interesting, though at first glance it seems to be discussed controversially. Will look at it more in detail within the next days. Another example where the MiniWallet with P2PK support helps is feature_dersig.py, which used the wallet for creating a DER-signature (i.e. ignoring specific reject reasons doesn’t help in this case).

in test/functional/test_framework/wallet.py:48 in 6c0a1b7957 outdated

45+                self._priv_key.set((1).to_bytes(32, 'big'), True)
46+                pub_key = self._priv_key.get_pubkey()
47+                self._scriptPubKey = bytes(CScript([pub_key.get_bytes(), OP_CHECKSIG]))
48+            else:
49+                self._scriptPubKey = bytes(CScript([OP_TRUE]))
50         else:

MarcoFalke commented at 5:04 pm on May 17, 2021:

would be nice to make this an elif use_p2pk to make it easier to switch to an enum

theStack commented at 6:05 pm on May 17, 2021:

Thanks, done.

MarcoFalke approved

test: MiniWallet: add P2PK support dc7eb64e83

theStack force-pushed on May 17, 2021

theStack commented at 6:07 pm on May 17, 2021: member

Force-pushed with the following changes:

changed MiniWallet.sign_tx() method to not return the tx again, as the signing works in-place by directly modifying the first input’s scriptSig (thanks to @reemuru whose review comment inspired me to do this!)
changed mode selection structure to be more friendlier w.r.t. changing to an enum (thanks to @MarcoFalke for suggesting). The mode selection looks now like the following:

raw_script	use_p2pk	Mode
True	X	raw OP_TRUE output
X	True	raw P2PK ([pubkey] OP_CHECKSIG) output
False	False	bech32 P2WSH address, containing hash of OP_TRUE (default)

test: use P2PK-MiniWallet for feature_csv_activation.py

Using the MiniWallet in P2PK mode, all transactions submitted to the
mempool are following the standard policy now, i.e. the node command
line parameter '-acceptnonstdtxn=1' is not needed anymore.

4bea301692

theStack force-pushed on May 17, 2021

laanwj commented at 7:19 pm on May 18, 2021: member

Code review ACK 4bea30169218e2f21e0c93a059966b41c8edd205

MarcoFalke merged this on May 24, 2021

MarcoFalke closed this on May 24, 2021

sidhujag referenced this in commit 764ff1f002 on May 25, 2021

MarcoFalke referenced this in commit 0a909073dc on May 25, 2021

MarcoFalke referenced this in commit 8600934018 on May 25, 2021

sidhujag referenced this in commit 2a269095ea on May 25, 2021

sidhujag referenced this in commit 2e05ad958f on May 27, 2021

MarcoFalke referenced this in commit 74013641e0 on Jun 21, 2021

sidhujag referenced this in commit fbdbdc0e1d on Jun 24, 2021

theStack deleted the branch on Jul 31, 2021

gwillen referenced this in commit 11548bb33c on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

test: add P2PK support to MiniWallet #21945