test: MiniWallet: support skipping mempool checks (feature_fee_estimation.py performance fix) #24941

pull theStack wants to merge 2 commits into bitcoin:master from theStack:202204-test-MiniWallet-support_creating_txs_without_mempool_checks changing 2 files +9 −10

theStack commented at 1:58 AM on April 22, 2022: member
MiniWallet's core method for creating txs (create_self_transfer) right now always executes the testmempoolaccept RPC to check for mempool validity or invalidity. In some test cases where we use MiniWallet to create a huge number of transactions this can lead to performance issues, in particular feature_fee_estimation.py where the execution time after MiniWallet usage (PR #24817) doubled, see #24828 (comment), #24828 (comment). This PR mitigates this by skipping the mempool check if the parameter mempool_valid is set to False.

As a preparatory commit, the test feature_csv_activation.py has to be adapted w.r.t. to rehashing of transactions, as we now hash all transactions immediately in create_self_transfer in order to get the txid (before we relied on the result of testmempoolaccept).

On my machine, this decreases the execution time quite noticably:

master branch:
```
$ time ./test/functional/feature_fee_estimation.py
real    3m20.771s
user    2m52.360s
sys     0m39.340s
```
PR branch:
```
$ time ./test/functional/feature_fee_estimation.py
real    2m1.386s
user    1m42.510s
sys     0m22.980s
```
Partly fixes #24828 (hopefully).
DrahtBot added the label Tests on Apr 22, 2022

in test/functional/test_framework/wallet.py:266 in 7ca345c16f outdated

 259 | @@ -259,13 +260,17 @@ def create_self_transfer(self, *, fee_rate=Decimal("0.003"), from_node=None, utx
 260 |              tx.wit.vtxinwit = [CTxInWitness()]
 261 |              tx.wit.vtxinwit[0].scriptWitness.stack = [CScript([OP_TRUE]), bytes([LEAF_VERSION_TAPSCRIPT]) + self._internal_key]
 262 |          tx_hex = tx.serialize().hex()
 263 | +        txid = tx.rehash()
 264 | +        wtxid = tx.getwtxid()
 265 | +
 266 | +        if mempool_valid is not None:

MarcoFalke commented at 6:24 AM on April 22, 2022:

I don't think we need the miniwallet to check that mempool_valid=False is equal to tmpa["allowed"].

        if mempool_valid:

theStack commented at 1:15 PM on April 22, 2022:

Good idea, done.

test: MiniWallet: always rehash after signing (P2PK mode)

Also explicitly rehash in the cases where we modify a tx after signing
in feature_csv_activation.py. Parts of this test relied on the fact that
rehashing of transactions is done in the course of calculating a block's
merkle root (`calc_merkle_root`), which only works if no hash was
calculated before due to a caching mechanism.

In the following commit the txid in MiniWallet is calculated via
`rehash()`, i.e. this doesn't work anymore and we always have to
explicitely have the right hash before we calculate the merkle root.

01552e8f67

test: MiniWallet: skip mempool check if `mempool_valid=False`

MiniWallet's core method for creating txs (`create_self_transfer`)
right now always executes the `testmempoolaccept` RPC to check for
mempool validity or invalidity. In some test cases where we use
MiniWallet to create a huge number of transactions this can lead
to performance issues (e.g. feature_fee_estimation.py where the
execution time after MiniWallet usage almost doubled). Providing
the possibility to skip the mempool checks is a mitigation for
this.

master branch:
$ time ./test/functional/feature_fee_estimation.py
real    3m20.771s
user    2m52.360s
sys     0m39.340s

PR branch:
$ time ./test/functional/feature_fee_estimation.py
real    2m1.386s
user    1m42.510s
sys     0m22.980s

a498acce45

theStack force-pushed on Apr 22, 2022
danielabrozzoni approved

danielabrozzoni commented at 9:41 PM on May 2, 2022: contributor

tACK a498acce4514d83d8dafcebaad522a89b6dc70fa

I also experienced faster running time:

On master:

time ./test/functional/feature_fee_estimation.py
real    1m49.060s
user    1m10.307s
sys     0m4.977s

On a498acce4514d83d8dafcebaad522a89b6dc70fa:

time ./test/functional/feature_fee_estimation.py
real    1m27.915s
user    1m4.089s
sys     0m4.600s

MarcoFalke merged this on May 3, 2022
MarcoFalke closed this on May 3, 2022
theStack deleted the branch on May 3, 2022
sidhujag referenced this in commit 1e4a8f3ce3 on May 4, 2022
Fabcien referenced this in commit 4624df9933 on Dec 2, 2022
DrahtBot locked this on May 3, 2023

Contributors

Labels

Linked (view graph)

#24817 test: use MiniWallet for feature_fee_estimation.py #24828 Intermittent failure in feature_fee_estimation.py