test: use MiniWallet for mining_prioritisetransaction.py

theStack commented at 8:07 pm on April 12, 2022: member

This PR enables one more of the non-wallet functional tests (mining_prioritisetransaction.py) to be run even with the Bitcoin Code wallet by using the MiniWallet instead, as proposed in #20078. Note that the adapted helper function create_lots_of_big_transactions is currently only used in this test, i.e. there was no need to change any others.

fanquake added the label Tests on Apr 12, 2022

in test/functional/mining_prioritisetransaction.py:137 in 2f7f8fb8ba outdated

133@@ -131,7 +134,11 @@ def run_test(self):
134         self.relayfee = self.nodes[0].getnetworkinfo()['relayfee']
135 
136         utxo_count = 90
137-        utxos = create_confirmed_utxos(self, self.relayfee, self.nodes[0], utxo_count)
138+        # TODO: use helper create_confirmed_utxos as soon at it uses MiniWallet

ayush933 commented at 4:21 pm on April 13, 2022:

nit:

0        # TODO: use helper create_confirmed_utxos as soon as it uses MiniWallet

theStack commented at 10:44 pm on April 13, 2022:

Thanks for your review, fixed.

ayush933 changes_requested

ayush933 commented at 6:44 pm on April 13, 2022: contributor

tACK 2f7f8fb

theStack force-pushed on Apr 13, 2022

in test/functional/mining_prioritisetransaction.py:137 in b33bfe3335 outdated

133@@ -131,7 +134,11 @@ def run_test(self):
134         self.relayfee = self.nodes[0].getnetworkinfo()['relayfee']
135 
136         utxo_count = 90
137-        utxos = create_confirmed_utxos(self, self.relayfee, self.nodes[0], utxo_count)
138+        # TODO: use helper create_confirmed_utxos as soon as it uses MiniWallet

MarcoFalke commented at 11:06 am on April 15, 2022:

I am not a fan of putting TODO in the source code. If (for any reason) they become stale, they will cause more damage/confusion than helping.

It would be better as an issue, so that it can be tracked easier as the code evolves. Also it can be discussed easier and discarded trivially if needed without touching the code.

Moreover, it seems that send_large_txs can be replaced by create_lots_of_big_transactions? Same for mine_large_block?

theStack commented at 7:38 pm on April 16, 2022:

It would be better as an issue, so that it can be tracked easier as the code evolves. Also it can be discussed easier and discarded trivially if needed without touching the code.

I see your point, though I think creating an issue for merely refactoring three lines of code is a bit like taking a sledgehammer to crack a nut (and one also has to think about opening it after the PR is merged which “triggers” the TODO). Removed the TODO for now – replacing create_confirmed_utxos (and feature_dbcrash.py) with MiniWallet is one of the next things I’ll work on anyways and in the course of that I’ll check where it can dedup code.

Moreover, it seems that send_large_txs can be replaced by create_lots_of_big_transactions? Same for mine_large_block?

Good idea, I added a second refactoring commit doing exactly that. For that to work, I had to generalize create_lots_of_big_transactions a bit, in particular the possibility to not pass in any UTXOs (MiniWallet picks one of it’s internal UTXOs then in this case). Also it validates the fee now, like it’s done in send_large_txs, which I think makes a lot of sense.

MarcoFalke approved

test: use MiniWallet for mining_prioritisetransaction.py

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

8973eeb412

theStack force-pushed on Apr 16, 2022

test: refactor: use `create_lots_of_big_transactions` to dedup where possible b167e536d0

theStack force-pushed on Apr 16, 2022

theStack commented at 7:46 pm on April 16, 2022: member

Force-pushed with suggestions from MarcoFalke taken into account. There is now a second commit which takes use of the brand-new shiny MiniWallet-variant of create_lots_of_big_transactions to deduplicate code. This PR affects the functional tests

mining_prioritisetransaction.py (obviously)
mempool_limit.py (send_large_txs is replaced by create_lots_of_big_transactions)
feature_maxuploadtarget.py (calls mine_large_block where also create_lots_of_big_transactions is used to replace part of its function)

fanquake commented at 10:13 am on April 20, 2022: member

@ayush933 want to take another look?

ayush933 approved

ayush933 commented at 11:31 am on April 20, 2022: contributor

tACK b167e53

danielabrozzoni approved

danielabrozzoni commented at 10:26 am on May 3, 2022: contributor

tACK b167e536d0f5ae4c86d0c3da4a63337f9f0448ba

in test/functional/test_framework/util.py:555 in b167e536d0

551@@ -552,38 +552,31 @@ def gen_return_txouts():
552 
553 # Create a spend of each passed-in utxo, splicing in "txouts" to each raw
554 # transaction to make it large.  See gen_return_txouts() above.
555-def create_lots_of_big_transactions(node, txouts, utxos, num, fee):
556-    addr = node.getnewaddress()
557+def create_lots_of_big_transactions(mini_wallet, node, fee, tx_batch_size, txouts, utxos=None):

kouloumos commented at 4:09 pm on June 1, 2022:

nit: create_lots_of_big_transactions is not introduced in this PR, but maybe include “send” in the name, to clearly indicate functionality?

kouloumos approved

kouloumos commented at 4:24 pm on June 1, 2022: member

ACK b167e536d0f5ae4c86d0c3da4a63337f9f0448ba

in test/functional/test_framework/util.py:560 in 8973eeb412 outdated

572-        newtx = tx.serialize().hex()
573-        signresult = node.signrawtransactionwithwallet(newtx, None, "NONE")
574-        txid = node.sendrawtransaction(signresult["hex"], 0)
575-        txids.append(txid)
576+    use_internal_utxos = utxos is None
577+    for _ in range(tx_batch_size):

furszy commented at 1:59 pm on June 10, 2022:

tiny nit: if utxos is not empty, maybe assert that tx_batch_size >= len(utxos).

furszy approved

furszy commented at 2:31 pm on June 10, 2022: member

ACK b167e536

MarcoFalke merged this on Jun 13, 2022

MarcoFalke closed this on Jun 13, 2022

theStack deleted the branch on Jun 13, 2022

sidhujag referenced this in commit 6dbdba18ec on Jun 13, 2022

in test/functional/mining_prioritisetransaction.py:139 in b167e536d0

133@@ -131,7 +134,10 @@ def run_test(self):
134         self.relayfee = self.nodes[0].getnetworkinfo()['relayfee']
135 
136         utxo_count = 90
137-        utxos = create_confirmed_utxos(self, self.relayfee, self.nodes[0], utxo_count)
138+        utxos = self.wallet.send_self_transfer_multi(from_node=self.nodes[0], num_outputs=utxo_count)['new_utxos']
139+        self.generate(self.wallet, 1)
140+        assert_equal(len(self.nodes[0].getrawmempool()), 0)

MarcoFalke commented at 7:31 am on June 14, 2022:

I really wonder if it is worth it to move this to create_confirmed_utxos? Seems 2-3 LOC duplicated everywhere are fine compared to 1 LOC + import + definition in separate file?

Maybe just remove create_confirmed_utxos?

theStack commented at 11:17 pm on June 14, 2022:

Right, I agree that it’s not worth it, calling send_self_transfer_multi with a subsequent generate is simple enough. Opened a follow-up PR: #25374

MarcoFalke referenced this in commit 4c0d1fec16 on Jun 15, 2022

sidhujag referenced this in commit 1630aae6a4 on Jun 15, 2022

DrahtBot locked this on Jun 14, 2023

test: use MiniWallet for mining_prioritisetransaction.py #24839