test: remove wallet dependency from feature

ayush933 commented at 5:46 pm on June 13, 2022: contributor

This PR enables one of the non-wallet functional tests (feature_nulldummy.py) to be run even with the Bitcoin Core wallet disabled.

Commit 1: removes wallet dependency and test_runner.py is edited to make sure the test only runs once. Commit 2: the functions create_transaction() and create_raw_transaction() in blocktools.py are no longer needed and hence removed.

fanquake added the label Tests on Jun 13, 2022

in test/functional/feature_nulldummy.py:89 in b02411d2dd outdated

81@@ -82,31 +82,59 @@ def run_test(self):
82         self.lastblocktime = int(time.time()) + self.lastblockheight
83 
84         self.log.info(f"Test 1: NULLDUMMY compliant base transactions should be accepted to mempool and mined before activation [{COINBASE_MATURITY + 3}]")
85-        test1txs = [create_transaction(self.nodes[0], coinbase_txid[0], self.ms_address, amount=49)]
86+        input = {"txid": coinbase_txid[0], "vout": 0}
87+        output = {self.ms_address: 49}
88+        rawtx = self.nodes[0].createrawtransaction([input], output)
89+        signedtx = self.nodes[0].signrawtransactionwithkey(hexstring=rawtx, privkeys=[self.nodes[0].PRIV_KEYS[0][1]])
90+        test1txs = [tx_from_hex(signedtx["hex"])]

MarcoFalke commented at 8:22 am on June 14, 2022:

I am wondering if it helps if there was a local create_transaction helper? As this is creating a simple chain of txs, the vout could be hardcoded to 0 in the helper, also the node could be hardcoded to self.nodes[0], as there is only one node.

So you’d call self.create_transaction(coinbase_txid[0], self.ms_address, amount=49, privkey=self.nodes[0].PRIV_KEYS[0][1]) here.

The benefit would be less code and less symbols in this scope.

Thoughts?

ayush933 commented at 1:20 pm on June 14, 2022:

Agree that a lot of symbols are reused and the code could be minimised. Added a helper function for creating transactions.

MarcoFalke approved

MarcoFalke commented at 8:23 am on June 14, 2022: member

Looks good. Left an idea

laanwj commented at 9:40 am on June 14, 2022: member

Concept ACK, thanks for working on disentangling more tests from the wallet.

ayush933 force-pushed on Jun 14, 2022

in test/functional/feature_nulldummy.py:96 in a8fe321bae outdated

106+        block_hash = []
107         for i in self.coinbase_blocks:
108-            coinbase_txid.append(self.nodes[0].getblock(i)['tx'][0])
109+            block = self.nodes[0].getblock(i)
110+            coinbase_txid.append(block['tx'][0])
111+            block_hash.append(block["hash"])

MarcoFalke commented at 1:25 pm on June 14, 2022:

Is this needed? In the previous commit you didn’t have to pass in the scriptPubKey for coinbase spends with the deterministic keys.

ayush933 commented at 3:06 pm on June 14, 2022:

In the previous commit I didn’t have to pass [input] to signrawtransactionwithkey for coinbase spends but here I have to. I get Missing scriptPubKey (-3) if I don’t pass the scriptPubKey.

MarcoFalke commented at 7:33 am on June 15, 2022:

Oh I see. This is due to passing the input even though it is not needed. What do you think about passing None in this case?

See this draft diff:

 0diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py
 1index e860a7fc36..90e0887b08 100755
 2--- a/test/functional/feature_nulldummy.py
 3+++ b/test/functional/feature_nulldummy.py
 4@@ -60,19 +60,13 @@ class NULLDUMMYTest(BitcoinTestFramework):
 5             '-par=1',  # Use only one script thread to get the exact reject reason for testing
 6         ]]
 7 
 8-    def create_transaction(self, *, txid, inAmount=None, spk=None, rs=None, ws=None, addr, amount, privkey):
 9+    def create_transaction(self, *, txid, prev_utxo_details=None, addr, amount, privkey):
10         input = {"txid": txid, "vout": 0}
11-        if inAmount is not None:
12-            input["amount"] = inAmount
13-        if spk is not None:
14-            input["scriptPubKey"] = spk
15-        if rs is not None:
16-            input["redeemScript"] = rs
17-        if ws is not None:
18-            input["witnessScript"] = ws
19         output = {addr: amount}
20         rawtx = self.nodes[0].createrawtransaction([input], output)
21-        signedtx = self.nodes[0].signrawtransactionwithkey(rawtx, [privkey], [input])
22+        # Details only needed for scripthash or witness spends
23+        input = None if not prev_utxo_details else [{**input,**prev_utxo_details}]
24+        signedtx = self.nodes[0].signrawtransactionwithkey(rawtx, [privkey], input)
25         return tx_from_hex(signedtx["hex"])
26 
27     def run_test(self):
28@@ -101,24 +95,22 @@ class NULLDUMMYTest(BitcoinTestFramework):
29 
30         self.log.info(f"Test 1: NULLDUMMY compliant base transactions should be accepted to mempool and mined before activation [{COINBASE_MATURITY + 3}]")
31         test1txs = [self.create_transaction(txid=coinbase_txid[0], addr=self.ms_address, amount=49,
32-                                            spk=self.nodes[0].getrawtransaction(coinbase_txid[0], True, block_hash[0])["vout"][0]["scriptPubKey"]["hex"],
33                                             privkey=self.nodes[0].PRIV_KEYS[0][1])]
34         txid1 = self.nodes[0].sendrawtransaction(test1txs[0].serialize_with_witness().hex(), 0)
35         spk = test1txs[0].vout[0].scriptPubKey.hex()
36-        test1txs.append(self.create_transaction(txid=txid1, spk=spk,
37-                                                rs=cms["redeemScript"],
38+        test1txs.append(self.create_transaction(txid=txid1, prev_utxo_details={"scriptPubKey":spk,
39+                                                "redeemScript":cms["redeemScript"]},
40                                                 addr=self.ms_address, amount=48,
41                                                 privkey=self.privkey))
42         txid2 = self.nodes[0].sendrawtransaction(test1txs[1].serialize_with_witness().hex(), 0)
43         test1txs.append(self.create_transaction(txid=coinbase_txid[1],
44-                                                spk=self.nodes[0].getrawtransaction(coinbase_txid[1], True, block_hash[1])["vout"][0]["scriptPubKey"]["hex"],
45                                                 addr=self.wit_ms_address, amount=49,
46                                                 privkey=self.nodes[0].PRIV_KEYS[0][1]))
47         txid3 = self.nodes[0].sendrawtransaction(test1txs[2].serialize_with_witness().hex(), 0)
48         self.block_submit(self.nodes[0], test1txs, accept=True)
49 
50         self.log.info("Test 2: Non-NULLDUMMY base multisig transaction should not be accepted to mempool before activation")
51-        test2tx = self.create_transaction(txid=txid2, spk=spk, rs=cms["redeemScript"],
52+        test2tx = self.create_transaction(txid=txid2, prev_utxo_details={"scriptPubKey":spk, "redeemScript":cms["redeemScript"]},
53                                           addr=self.ms_address, amount=47,
54                                           privkey=self.privkey)
55         invalidate_nulldummy_tx(test2tx)
56@@ -128,7 +120,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
57         self.block_submit(self.nodes[0], [test2tx], accept=True)
58 
59         self.log.info("Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation")
60-        test4tx = self.create_transaction(txid=test2tx.hash, spk=spk, rs=cms["redeemScript"],
61+        test4tx = self.create_transaction(txid=test2tx.hash, prev_utxo_details={"scriptPubKey":spk, "redeemScript":cms["redeemScript"]},
62                                           addr=self.address, amount=46,
63                                           privkey=self.privkey)
64         test6txs = [CTransaction(test4tx)]
65@@ -137,8 +129,8 @@ class NULLDUMMYTest(BitcoinTestFramework):
66         self.block_submit(self.nodes[0], [test4tx], accept=False)
67 
68         self.log.info("Test 5: Non-NULLDUMMY P2WSH multisig transaction invalid after activation")
69-        test5tx = self.create_transaction(txid=txid3, spk=test1txs[2].vout[0].scriptPubKey.hex(),
70-                                          inAmount=49, ws=wms["redeemScript"],
71+        test5tx = self.create_transaction(txid=txid3, prev_utxo_details={"scriptPubKey":test1txs[2].vout[0].scriptPubKey.hex(),
72+                                          "amount":49, "witnessScript":wms["redeemScript"]},
73                                           addr=self.wit_address, amount=48,
74                                           privkey=self.privkey)
75         test6txs.append(CTransaction(test5tx))

ayush933 commented at 11:23 am on June 15, 2022:

This is indeed much cleaner. Made the necessary changes.

ayush933 force-pushed on Jun 15, 2022

in test/functional/feature_nulldummy.py:22 in 9a783a9fd7 outdated

18@@ -19,9 +19,11 @@
19     NORMAL_GBT_REQUEST_PARAMS,
20     add_witness_commitment,
21     create_block,
22-    create_transaction,

MarcoFalke commented at 11:42 am on June 15, 2022:

Is it possible to remove this function now that it is unused?

in test/functional/feature_nulldummy.py:73 in 9a783a9fd7 outdated

82-        self.wit_ms_address = wmulti.addmultisigaddress(1, [self.pubkey], '', 'p2sh-segwit')['address']
83-        if not self.options.descriptors:
84-            # Legacy wallets need to import these so that they are watched by the wallet. This is unnecessary (and does not need to be tested) for descriptor wallets
85-            wmulti.importaddress(self.ms_address)
86-            wmulti.importaddress(self.wit_ms_address)
87+        self.address = getnewdestination()[2]

MarcoFalke commented at 11:46 am on June 15, 2022:

I think you can just inline this. Previously it was used to add a privkey to the wallet and derive the pubkey from the address. Now it is just used as a dummy address.

kouloumos commented at 7:05 pm on June 19, 2022:

nit, maybe move address to where it is first used

in test/functional/feature_nulldummy.py:81 in 9a783a9fd7 outdated

90+        self.privkey = bytes_to_wif(eckey.get_bytes())
91+        self.pubkey = eckey.get_pubkey().get_bytes().hex()
92+        cms = self.nodes[0].createmultisig(1, [self.pubkey])
93+        wms = self.nodes[0].createmultisig(1, [self.pubkey], 'p2sh-segwit')
94+        self.ms_address = cms["address"]
95+        self.wit_address = getnewdestination(address_type='p2sh-segwit')[2]

MarcoFalke commented at 11:48 am on June 15, 2022:

Same

kouloumos commented at 8:23 pm on June 19, 2022:

nit, maybe move address to where it is first used

in test/functional/feature_nulldummy.py:95 in 9a783a9fd7 outdated

90@@ -82,31 +91,45 @@ def run_test(self):
91         self.lastblocktime = int(time.time()) + self.lastblockheight
92 
93         self.log.info(f"Test 1: NULLDUMMY compliant base transactions should be accepted to mempool and mined before activation [{COINBASE_MATURITY + 3}]")
94-        test1txs = [create_transaction(self.nodes[0], coinbase_txid[0], self.ms_address, amount=49)]
95+        test1txs = [self.create_transaction(txid=coinbase_txid[0], addr=self.ms_address, amount=49,
96+                                            privkey=self.nodes[0].PRIV_KEYS[0][1])]

kouloumos commented at 7:08 pm on June 19, 2022:

I did not find any other examples of accessing the private key like this, maybe use

0                                            privkey=self.nodes[0].get_deterministic_priv_key().key)]

also same suggestion for L105

Or alternatively you could use that as the default value for privkey in create_transaction

in test/functional/feature_nulldummy.py:97 in 9a783a9fd7 outdated

90@@ -82,31 +91,45 @@ def run_test(self):
91         self.lastblocktime = int(time.time()) + self.lastblockheight
92 
93         self.log.info(f"Test 1: NULLDUMMY compliant base transactions should be accepted to mempool and mined before activation [{COINBASE_MATURITY + 3}]")
94-        test1txs = [create_transaction(self.nodes[0], coinbase_txid[0], self.ms_address, amount=49)]
95+        test1txs = [self.create_transaction(txid=coinbase_txid[0], addr=self.ms_address, amount=49,
96+                                            privkey=self.nodes[0].PRIV_KEYS[0][1])]
97         txid1 = self.nodes[0].sendrawtransaction(test1txs[0].serialize_with_witness().hex(), 0)
98-        test1txs.append(create_transaction(self.nodes[0], txid1, self.ms_address, amount=48))
99+        spk = test1txs[0].vout[0].scriptPubKey.hex()

kouloumos commented at 7:40 pm on June 19, 2022:

As this spk value is not only used for Test 1, maybe consider assigning it before the start of the tests? You could do something like spk = script_to_p2sh_script(cms['redeemScript']).hex() or spk = self.nodes[0].validateaddress(self.ms_address)["scriptPubKey"]

in test/functional/feature_nulldummy.py:68 in 9a783a9fd7 outdated

65+    def create_transaction(self, *, txid, prev_utxo_details=None, addr, amount, privkey):
66+        input = {"txid": txid, "vout": 0}
67+        output = {addr: amount}
68+        rawtx = self.nodes[0].createrawtransaction([input], output)
69+        # Details only needed for scripthash or witness spends
70+        input = None if not prev_utxo_details else [{**input, **prev_utxo_details}]

kouloumos commented at 7:53 pm on June 19, 2022:

nit, for readability maybe a better name for prev_utxo_details could be input_details/input_unlocking_script to match better with the logic here?

in test/functional/feature_nulldummy.py:99 in 9a783a9fd7 outdated

 96+                                            privkey=self.nodes[0].PRIV_KEYS[0][1])]
 97         txid1 = self.nodes[0].sendrawtransaction(test1txs[0].serialize_with_witness().hex(), 0)
 98-        test1txs.append(create_transaction(self.nodes[0], txid1, self.ms_address, amount=48))
 99+        spk = test1txs[0].vout[0].scriptPubKey.hex()
100+        test1txs.append(self.create_transaction(txid=txid1, prev_utxo_details={"scriptPubKey": spk,
101+                                                "redeemScript": cms["redeemScript"]},

kouloumos commented at 8:07 pm on June 19, 2022:

nit: As the same prev_utxo_details are used more than once, I think it would be beneficial to the test’s readability if you assigned them at the beginning of the test with something like

ms_address_unlock_details = {"scriptPubKey": spk,
 "redeemScript": cms["redeemScript"]}

kouloumos commented at 8:23 pm on June 19, 2022: member

ACK 9a783a9fd76ee5350d64ed1ed5b658c530913653 with some minor comments that i believe could increase readability.

I was also thinking that using the MiniWallet could potentially simplify the test and I tried to validate my intuition, but it seems that the changes needed do not justify the idea.

DrahtBot commented at 0:33 am on June 21, 2022: member

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

Conflicts

No conflicts as of last run.

test: remove wallet dependency from feature_nulldummy.py

This test can now be run even with the Bitcoin Core wallet disabled.

eec23dad1e

ayush933 force-pushed on Jun 24, 2022

remove unused functions

the functions `create_transaction()` and `create_raw_transaction()` were no longer used hence removed.

50ba6697f3

ayush933 commented at 12:46 pm on June 24, 2022: contributor

Thanks a lot @MarcoFalke and @kouloumos for the thorough reviews. Addressed the reviews and made the necessary changes.

kouloumos commented at 4:13 pm on June 28, 2022: member

re-ACK 50ba6697f33b44e475ed65137f7ff0444f6c4ca9, all comments have been addressed.

MarcoFalke merged this on Jun 30, 2022

MarcoFalke closed this on Jun 30, 2022

sidhujag referenced this in commit 1a3a295030 on Jun 30, 2022

DrahtBot locked this on Jun 30, 2023

test: remove wallet dependency from feature_nulldummy.py #25364

Conflicts