test: MiniWallet: support skipping mempool checks (feature_fee_estimation.py performance fix) #24941
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maflcko
reviewed
Apr 22, 2022
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.
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
theStack
force-pushed
the
202204-test-MiniWallet-support_creating_txs_without_mempool_checks
branch
from
7ca345c to
a498acc
Compare
danielabrozzoni
approved these changes
May 2, 2022
theStack
deleted the
202204-test-MiniWallet-support_creating_txs_without_mempool_checks
branch
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 4, 2022
…s (feature_fee_estimation.py performance fix) a498acc test: MiniWallet: skip mempool check if `mempool_valid=False` (Sebastian Falbesoner) 01552e8 test: MiniWallet: always rehash after signing (P2PK mode) (Sebastian Falbesoner) Pull request description: 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 bitcoin#24817) doubled, see bitcoin#24828 (comment), bitcoin#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 bitcoin#24828 (hopefully). ACKs for top commit: danielabrozzoni: tACK a498acc Tree-SHA512: f20c358ba42b2ded86175f46ff3ff9eaefb84175cbd1c2624f44904c8d8888e67ce64d6dcbb26aabbf07906e6f5bdea40353eba9ae668618cadcfc517ef7201b
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 2, 2022
Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#24941 | core#24941]]. The first commit is not backported because we diverged a lot from the core test and didn't convert it to MiniWallet yet, and the sign_tx function (which is not in our codebase yet) trivially needs the rehash anyway. So only this commit is relevant: bitcoin/bitcoin@a498acc Test Plan: ninja check-functional Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12730
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MiniWallet's core method for creating txs (
create_self_transfer) right now always executes thetestmempoolacceptRPC 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 parametermempool_validis set toFalse.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_transferin order to get the txid (before we relied on the result oftestmempoolaccept).On my machine, this decreases the execution time quite noticably:
master branch:
PR branch:
Partly fixes #24828 (hopefully).