test: use MiniWallet for feature_dbcrash.py #25087

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202205-test-use_MiniWallet_for_feature_dbcrash changing 2 files +34 −28

theStack commented at 10:54 PM on May 8, 2022: member

This PR enables one more of the non-wallet functional tests (feature_dbcrash.py) to be run even with the Bitcoin Code wallet by using the MiniWallet instead, as proposed in #20078.
theStack force-pushed on May 8, 2022
DrahtBot added the label Tests on May 9, 2022
ayush933 approved
ayush933 commented at 12:48 PM on May 16, 2022: contributor

tACK aa9fab1.
in test/functional/feature_dbcrash.py:202 in aa9fab18d1 outdated
```
 211 |  
 212 | -            # Sign and send the transaction to get into the mempool
 213 | -            tx_signed_hex = node.signrawtransactionwithwallet(tx.serialize().hex())['hex']
 214 | -            node.sendrawtransaction(tx_signed_hex)
 215 | +            # Send the transaction to get into the mempool
 216 | +            node.sendrawtransaction(tx.serialize().hex(), 0)
```
MarcoFalke commented at 3:11 PM on May 16, 2022:

What about setting this to 0 by default for MiniWallet?

MarcoFalke commented at 3:15 PM on May 16, 2022:

Also, it seems confusing that this needs to be set at all. Why would a fee of 1000 sat be ever too high?

theStack commented at 3:58 PM on May 19, 2022:

maxfeerate=0 is set here only for performance reasons, to skip this part in the course of the sendrawtransaction RPC: https://github.com/bitcoin/bitcoin/blob/0de36941eca1bff91420dd878eb097db2b1a596c/src/node/transaction.cpp#L71-L80

With the fee-check enabled, the iterations take about twice as long (~2mins instead of ~1min on my machine). Added a comment to clarify.

in test/functional/feature_dbcrash.py:264 in aa9fab18d1 outdated

 262 |              self.log.debug(f"Syncing {len(block_hashes)} new blocks...")
 263 |              self.sync_node3blocks(block_hashes)
 264 | -            utxo_list = self.nodes[3].listunspent()
 265 | -            self.log.debug(f"Node3 utxo count: {len(utxo_list)}")
 266 | +            self.wallet.rescan_utxos()
 267 | +            utxo_list = self.wallet._utxos

MarcoFalke commented at 3:23 PM on May 16, 2022:

Not sure about using the private member here. What about def get_utxos(self, *, mark_as_spent=True): return copy.copy(self._utxos) or so?

theStack commented at 3:59 PM on May 19, 2022:

Agree, accessing private members should be avoided. Thanks, done.

MarcoFalke approved
MarcoFalke commented at 3:23 PM on May 16, 2022: member

lgtm

test: use MiniWallet for feature_dbcrash.py

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

1da5e45725

theStack force-pushed on May 19, 2022
theStack commented at 4:00 PM on May 19, 2022: member

Force-pushed with suggestions from MarcoFalke (https://github.com/bitcoin/bitcoin/pull/25087#discussion_r873860561, #25087 (review)) taken into account.
laanwj referenced this in commit 6db7fbd812 on May 31, 2022
laanwj commented at 2:13 PM on June 1, 2022: member

Code review ACK 1da5e45725a49a867f7ce16fb37b138ad329d132
brunoerg approved
brunoerg commented at 2:17 PM on June 1, 2022: member

crACK 1da5e45725a49a867f7ce16fb37b138ad329d132
MarcoFalke merged this on Jun 1, 2022
MarcoFalke closed this on Jun 1, 2022
theStack deleted the branch on Jun 1, 2022
sidhujag referenced this in commit a7ad5d6dfa on Jun 1, 2022
MarcoFalke referenced this in commit 1c7ef0abd1 on Jun 1, 2022
sidhujag referenced this in commit 72ad43cbf4 on Jun 2, 2022
MarcoFalke referenced this in commit 4c0d1fec16 on Jun 15, 2022
sidhujag referenced this in commit 1630aae6a4 on Jun 15, 2022
MarcoFalke locked this on Oct 11, 2022
fanquake deleted a comment on Oct 11, 2022

Contributors

Labels

Linked (view graph)

#20078 test: Convert non-wallet tests to use our python MiniWallet #25262 test: Set maxfeerate=0 in MiniWallet sendrawtransaction()#25374 test: remove unused `create_confirmed_utxos` helper