test: Run mempool_limit.py even with wallet disabled #20874
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
stackman27
force-pushed
the
diswallet_mempoollimit
branch
4 times, most recently
from
423e609
to
421c3ac
Compare
|
@mjdietzx Thank you very much for the review. I just pushed an update shortening the methods in |
stackman27
force-pushed
the
diswallet_mempoollimit
branch
3 times, most recently
from
47282a0
to
b8344b1
Compare
|
@mjdietzx Thank you very much for the review. Pushed a new update highlighting all the comments and also updated |
stackman27
force-pushed
the
diswallet_mempoollimit
branch
2 times, most recently
from
f24ece5
to
a460244
Compare
stackman27
force-pushed
the
diswallet_mempoollimit
branch
from
a460244
to
f5eb2bc
Compare
stackman27
force-pushed
the
diswallet_mempoollimit
branch
2 times, most recently
from
5873d94
to
42e5251
Compare
stackman27
force-pushed
the
diswallet_mempoollimit
branch
from
42e5251
to
7641348
Compare
stackman27
force-pushed
the
diswallet_mempoollimit
branch
from
2400356
to
18951db
Compare
stackman27
force-pushed
the
diswallet_mempoollimit
branch
3 times, most recently
from
8d7d57b
to
a478bf7
Compare
@glozow Thank you very much for the review! |
| assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex']) | ||
| assert_raises_rpc_error(-26, "mempool min fee not met", miniwallet.send_self_transfer, from_node=node, fee_rate=relayfee) | ||
|
|
||
| def create_large_transactions(self, node, txouts, miniwallet, num, fee, mempool_min_fee): |
There was a problem hiding this comment.
I'm wondering if send_large_transactions would be a better name since it is also broadcasting them? It would be more consistent with send_self_transfer.
There was a problem hiding this comment.
hmm just following the naming convention used here as it's just a copy of that code. But happy to change :)
test/functional/mempool_limit.py
Outdated
| tx.vout.extend(txouts) | ||
| tx_hex = tx.serialize().hex() | ||
| txids = node.sendrawtransaction(tx_hex, 0) | ||
| if node.getmempoolinfo()['mempoolminfee'] == mempool_min_fee: |
There was a problem hiding this comment.
What is the purpose of having this condition in here? As a result, it means that there are transactions which get broadcast and enter the mempool, but are not added to large_txids. I'll add a comment to where it's used to suggest what I think would be a better alternative.
There was a problem hiding this comment.
okay! will add a comment but the basic logic is that i'm making sure that the fees have changed and only include those txs whose fees have gone up. This also supports the assert logic at like 48
stackman27
force-pushed
the
diswallet_mempoollimit
branch
from
a478bf7
to
ed1e869
Compare
|
Thank you @DariusParvin for the review. Added changes as suggested |
|
|
||
| def create_large_transactions(self, node, txouts, miniwallet, num, fee, mempool_min_fee): | ||
| large_txids = [] | ||
| for j in range(num): |
There was a problem hiding this comment.
nit: since j is unused, use _ instead?
| for i in range (3): | ||
| txids.append([]) | ||
| txids[i] = create_lots_of_big_transactions(self.nodes[0], txouts, utxos[30*i:30*i+30], 30, (i+1)*base_fee) | ||
| txids[i] = self.create_large_transactions(node, txouts, miniwallet, 30, (i+1)*base_fee, mempool_min_fee) |
There was a problem hiding this comment.
here num is set to 30 but the test also works with 22 (with only the original tx getting kicked from the mempool). My feeling is that it's more beneficial to have the test run faster than to have some extra leeway with excess txs.
| txids[i] = self.create_large_transactions(node, txouts, miniwallet, 30, (i+1)*base_fee, mempool_min_fee) | |
| txids[i] = self.create_large_transactions(node, txouts, miniwallet, 22, (i+1)*base_fee, mempool_min_fee) |
stackman27
force-pushed
the
diswallet_mempoollimit
branch
from
ed1e869
to
9d2b30c
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Are you still working on this? |
Not at the moment, have been busy with work and classes |
08634e8 fix typos in logging messages (ShubhamPalriwala) d447ded replace: self.nodes[0] with node (ShubhamPalriwala) dddca38 test: use MiniWallet in mempool_limit.py (ShubhamPalriwala) Pull request description: This is a PR proposed in #20078 This PR enables running another non-wallet functional test even when the wallet is disabled thanks to the MiniWallet, i.e. it can be run even when bitcoin-core is compiled with --disable-wallet. It also includes changes in wallet.py in the form of a new method, `create_large_transactions()` for the MiniWallet to create large transactions. Efforts for this feature started in #20874 but were not continued and that PR was closed hence I picked this up. To test this PR locally, compile and build bitcoin-core without the wallet and run: ``` $ test/functional/mempool_limit.py ``` ACKs for top commit: amitiuttarwar: ACK 08634e8, only git changes since last push (and one new line). Zero-1729: ACK 08634e8 🧉 Tree-SHA512: 0f744ad26bf7a5a784aac1ed5077b59c95a36d1ff3ad0087ffd10ac8d5979f7362c63c20c2ce2bfa650fda02dfbcd60b1fceee049a2465c8d221cce51c20369f
This is a PR proposed in #20078.
This PR enables one more of the non-wallet functional tests by running
mempool_limit.pyeven when the wallet is disabled. While restructuring the code, I added an extra method,send_multi_self_transferinMiniWalletclass. This method allows the creation of large transactions (by appending txouts totx.vout) which will be useful in other files likemining_prioritisetransaction.pyas well.While my approach may not be the cleanest or the most efficient, I totally appreciate any suggestion to improve it. Thank you very much!