test: use MiniWallet for mining_prioritisetransaction.py #24839

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202204-test-use_MiniWallet_for_mining_prioritisetransaction changing 3 files +44 −57

theStack commented at 8:07 PM on April 12, 2022: contributor

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:

        # 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: contributor
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
shommel cross-referenced this on Apr 22, 2022 from issue test: Convert non-wallet tests to use our python MiniWallet by MarcoFalke
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: contributor

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
theStack cross-referenced this on Jun 14, 2022 from issue test: remove unused `create_confirmed_utxos` helper by theStack
MarcoFalke referenced this in commit 4c0d1fec16 on Jun 15, 2022
sidhujag referenced this in commit 1630aae6a4 on Jun 15, 2022
bitcoin locked this on Jun 14, 2023

Contributors

Labels

Linked (view graph)

#20078 test: Convert non-wallet tests to use our python MiniWallet #25374 test: remove unused `create_confirmed_utxos` helper