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. |
mjdietzx
left a comment
mjdietzx
left a comment
There was a problem hiding this comment.
I took a look at doing this refactoring a week or so ago, and figured I'd do a few easier ones bs this one was so tricky. But looks like you figured it out! Anyways I left a bunch of comments on things I noticed
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
mjdietzx
left a comment
mjdietzx
left a comment
There was a problem hiding this comment.
Nice, I think this is close! If you look at my comment, I think you can do this PR in a way that wallet.py is not changed by bringing in the submit_tx=False change for miniwallet.send_self_transfer
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
Concept ACK. I actually think a cleaner approach would be to make create_lots_of_big_transactions and the other util.py transaction not need wallet. Could reduce the amount of refactoring needed in each of the functional tests?
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! |
DariusParvin
left a comment
DariusParvin
left a comment
There was a problem hiding this comment.
Concept ACK! Just some suggestions that might make it more readable. Also, now that Miniwallet.scan_blocks has been merged, I think you can use it at the start.
test/functional/mempool_limit.py
Outdated
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
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 |
test/functional/mempool_limit.py
Outdated
There was a problem hiding this comment.
nit: since j is unused, use _ instead?
test/functional/mempool_limit.py
Outdated
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
8 participants
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!