test: remove wallet dependency from feature_nulldummy.py #25364

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?

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

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

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.

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:

diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py
index e860a7fc36..90e0887b08 100755
--- a/test/functional/feature_nulldummy.py
+++ b/test/functional/feature_nulldummy.py
@@ -60,19 +60,13 @@ class NULLDUMMYTest(BitcoinTestFramework):
             '-par=1',  # Use only one script thread to get the exact reject reason for testing
         ]]
 
-    def create_transaction(self, *, txid, inAmount=None, spk=None, rs=None, ws=None, addr, amount, privkey):
+    def create_transaction(self, *, txid, prev_utxo_details=None, addr, amount, privkey):
         input = {"txid": txid, "vout": 0}
-        if inAmount is not None:
-            input["amount"] = inAmount
-        if spk is not None:
-            input["scriptPubKey"] = spk
-        if rs is not None:
-            input["redeemScript"] = rs
-        if ws is not None:
-            input["witnessScript"] = ws
         output = {addr: amount}
         rawtx = self.nodes[0].createrawtransaction([input], output)
-        signedtx = self.nodes[0].signrawtransactionwithkey(rawtx, [privkey], [input])
+        # Details only needed for scripthash or witness spends
+        input = None if not prev_utxo_details else [{**input,**prev_utxo_details}]
+        signedtx = self.nodes[0].signrawtransactionwithkey(rawtx, [privkey], input)
         return tx_from_hex(signedtx["hex"])
 
     def run_test(self):
@@ -101,24 +95,22 @@ class NULLDUMMYTest(BitcoinTestFramework):
 
         self.log.info(f"Test 1: NULLDUMMY compliant base transactions should be accepted to mempool and mined before activation [{COINBASE_MATURITY + 3}]")
         test1txs = [self.create_transaction(txid=coinbase_txid[0], addr=self.ms_address, amount=49,
-                                            spk=self.nodes[0].getrawtransaction(coinbase_txid[0], True, block_hash[0])["vout"][0]["scriptPubKey"]["hex"],
                                             privkey=self.nodes[0].PRIV_KEYS[0][1])]
         txid1 = self.nodes[0].sendrawtransaction(test1txs[0].serialize_with_witness().hex(), 0)
         spk = test1txs[0].vout[0].scriptPubKey.hex()
-        test1txs.append(self.create_transaction(txid=txid1, spk=spk,
-                                                rs=cms["redeemScript"],
+        test1txs.append(self.create_transaction(txid=txid1, prev_utxo_details={"scriptPubKey":spk,
+                                                "redeemScript":cms["redeemScript"]},
                                                 addr=self.ms_address, amount=48,
                                                 privkey=self.privkey))
         txid2 = self.nodes[0].sendrawtransaction(test1txs[1].serialize_with_witness().hex(), 0)
         test1txs.append(self.create_transaction(txid=coinbase_txid[1],
-                                                spk=self.nodes[0].getrawtransaction(coinbase_txid[1], True, block_hash[1])["vout"][0]["scriptPubKey"]["hex"],
                                                 addr=self.wit_ms_address, amount=49,
                                                 privkey=self.nodes[0].PRIV_KEYS[0][1]))
         txid3 = self.nodes[0].sendrawtransaction(test1txs[2].serialize_with_witness().hex(), 0)
         self.block_submit(self.nodes[0], test1txs, accept=True)
 
         self.log.info("Test 2: Non-NULLDUMMY base multisig transaction should not be accepted to mempool before activation")
-        test2tx = self.create_transaction(txid=txid2, spk=spk, rs=cms["redeemScript"],
+        test2tx = self.create_transaction(txid=txid2, prev_utxo_details={"scriptPubKey":spk, "redeemScript":cms["redeemScript"]},
                                           addr=self.ms_address, amount=47,
                                           privkey=self.privkey)
         invalidate_nulldummy_tx(test2tx)
@@ -128,7 +120,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
         self.block_submit(self.nodes[0], [test2tx], accept=True)
 
         self.log.info("Test 4: Non-NULLDUMMY base multisig transaction is invalid after activation")
-        test4tx = self.create_transaction(txid=test2tx.hash, spk=spk, rs=cms["redeemScript"],
+        test4tx = self.create_transaction(txid=test2tx.hash, prev_utxo_details={"scriptPubKey":spk, "redeemScript":cms["redeemScript"]},
                                           addr=self.address, amount=46,
                                           privkey=self.privkey)
         test6txs = [CTransaction(test4tx)]
@@ -137,8 +129,8 @@ class NULLDUMMYTest(BitcoinTestFramework):
         self.block_submit(self.nodes[0], [test4tx], accept=False)
 
         self.log.info("Test 5: Non-NULLDUMMY P2WSH multisig transaction invalid after activation")
-        test5tx = self.create_transaction(txid=txid3, spk=test1txs[2].vout[0].scriptPubKey.hex(),
-                                          inAmount=49, ws=wms["redeemScript"],
+        test5tx = self.create_transaction(txid=txid3, prev_utxo_details={"scriptPubKey":test1txs[2].vout[0].scriptPubKey.hex(),
+                                          "amount":49, "witnessScript":wms["redeemScript"]},
                                           addr=self.wit_address, amount=48,
                                           privkey=self.privkey)
         test6txs.append(CTransaction(test5tx))

This is indeed much cleaner. Made the necessary changes.

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

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.

nit, maybe move address to where it is first used

Same

nit, maybe move address to where it is first used

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

                                            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

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

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?

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"]}