test: Speed up mempool_spend

MarcoFalke commented at 7:58 am on April 23, 2021: member

Locally the test will run 4 seconds faster with --valgrind (18s vs 14s)

test: Add MiniWallet.sendrawtransaction

Can be reviewed with --ignore-all-space --color-moved=dimmed-zebra

fa1bedb494

test: Create MiniWallet.create_self_transfer fa085b470a

test: Fix test cache issue

The documentation did not match the implementation, no coins
were mined to the OP_TRUE address.

fa29382ab2

test: Speed up mempool_spend_coinbase.py fa40eb5b6b

fanquake added the label Tests on Apr 23, 2021

DrahtBot commented at 11:18 am on April 23, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21178 (test: run mempool_reorg.py even with wallet disabled by DariusParvin)
#21014 (test: Run mempool_accept.py even with wallet disabled by stackman27)
#20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

mjdietzx commented at 5:02 pm on April 28, 2021: contributor

I really like the implementation of sendrawtransaction in https://github.com/bitcoin/bitcoin/pull/21762/commits/fa1bedb4944b513a3c9184ad549f58bfbe69e20e. I can close #20889 (review) in favor of this, right?
Also like the implementation of create_self_transfer and how it’s used 👍 in https://github.com/bitcoin/bitcoin/pull/21762/commits/fa085b470a9647f3b261f506b46f4e7ca2baf0b0
I’m confused and still trying to understand what’s really going on here https://github.com/bitcoin/bitcoin/pull/21762/commits/fa29382ab23d52b86bfda8a267195b6c51b713c2. I see why the original implementation was really confusing (no coins ever mined to ADDRESS_BCRT1_P2WSH_OP_TRUE although the code makes you expect that to happen). But I don’t really get why the assertion changes from 250 to 248 assuming it wasn’t already failing. And I also don’t understand why it fits into this PR. Any explanation here could help me get to an ACK quicker
Nice speedup results in https://github.com/bitcoin/bitcoin/pull/21762/commits/fa40eb5b6bfd151912c58d61771f6a6528f44e67. Am I correct that wallet.generate(200) is what is slowing these MiniWallet tests down? And that the approach you use here would be the gold standard for other MiniWallet tests going forward?

MarcoFalke commented at 5:10 pm on April 28, 2021: member

But I don’t really get why the assertion changes from 250 to 248 assuming it wasn’t already failing. And I also don’t understand why it fits into this PR. Any explanation here could help me get to an ACK quicker

I don’t know either why the assertion is there, but the value changes because the block size changes due to the scriptPubKey (size) change.

Am I correct that wallet.generate(200) is what is slowing these MiniWallet tests down? And that the approach you use here would be the gold standard for other MiniWallet tests going forward?

Yes, generate will take time under valgrind, so using the cache when possible will help with a speed-up. Though it is not always possible to use the cache.

in test/functional/mempool_spend_coinbase.py:38 in fa40eb5b6b

41-        utxo_101 = wallet.get_utxo(txid=coinbase_txids[0])
42-        utxo_102 = wallet.get_utxo(txid=coinbase_txids[1])
43+        wallet.scan_blocks(start=chain_height - 100 + 1, num=1)
44+        utxo_mature = wallet.get_utxo()
45+        wallet.scan_blocks(start=chain_height - 100 + 2, num=1)
46+        utxo_immature = wallet.get_utxo()

mjdietzx commented at 5:21 pm on April 28, 2021:

I don’t really like this pattern. The method name sync_blocks seems confusing because I wouldn’t expect a state change (self._utxos) by this method. It seems this would have to be used very carefully, with knowledge of MiniWallet’s internals to avoid bugs. For example, if scan_blocks is called multiple times with any overlapping blocks, then we’ll start to get duplicate utxos in self._utxos (maybe this should be a set instead of a list)?

In general I also don’t really like how self._utxos is sorted (in send_self_transfer by value), while here it isn’t sorted but will have an implicit ordering based on when/how scan_blocks is called, and then based on order of blocks/vouts inside scan_blocks. There’s just a lot of different ways it’s ordered and we are always relying on that ordering

I’m gonna think this through, don’t think this should hold up this PR. But at least I can start the discussion and see what you think / if this can be improved.

MarcoFalke commented at 5:24 pm on April 28, 2021:

I agree. Do you have specific suggestions to improve that pass with all existing tests? Feel free to ping me for review.

mjdietzx commented at 5:22 pm on April 28, 2021: contributor

crACK https://github.com/bitcoin/bitcoin/pull/21762/commits/fa40eb5b6bfd151912c58d61771f6a6528f44e67

MarcoFalke merged this on Apr 29, 2021

MarcoFalke closed this on Apr 29, 2021

MarcoFalke deleted the branch on Apr 29, 2021

sidhujag referenced this in commit e18347face on Apr 29, 2021

gwillen referenced this in commit 6b565b9878 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

test: Speed up mempool_spend_coinbase.py #21762

Conflicts