test: Use MiniWallet in mempool_accept.py#22509
test: Use MiniWallet in mempool_accept.py#22509sriramdvt wants to merge 2 commits intobitcoin:masterfrom
Conversation
sriramdvt
force-pushed
the
mini_memaccept
branch
from
aa068ad to
936f56b
Compare
|
Concept ACK |
|
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. |
sriramdvt
force-pushed
the
mini_memaccept
branch
2 times, most recently
from
80008d0 to
48e55a8
Compare
|
@sriramdvt: Your PR was fine as it is -- #22378 is not merged into master yet, so there is no need to resolve a conflict by now; you can remove the second commit. The DrahtBot conflict message above just says that whenever either your PR or #22378 is merged, the other one very likely has to be rebased. |
sriramdvt
force-pushed
the
mini_memaccept
branch
from
48e55a8 to
936f56b
Compare
|
Concept ACK. In the commit message, the |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
Left some review comments below, most concerning variable naming and stylistic improvements. Probably the changes in MiniWallet (comment fix in scan_blocks, returning fee in create_self_transfer) deserve an own commit ahead of the changes in mempool_accept.py.
sriramdvt
force-pushed
the
mini_memaccept
branch
from
936f56b to
7cd06b3
Compare
|
Concept ACK More MiniWallet is better. |
test/functional/mempool_accept.py
Outdated
There was a problem hiding this comment.
| utxo_1 = miniwallet.get_utxo() | |
| miniwallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_1)['txid'] | |
| miniwallet.send_self_transfer(from_node=node)['txid'] |
should be identical
There was a problem hiding this comment.
Also, why the ['txid'] in the end?
There was a problem hiding this comment.
Removed the unnecessary ['txid']. The suggestion is not identical, since miniwallet.send_self_transfer sorts the utxo set by the value, before choosing a transaction.
test/functional/mempool_accept.py
Outdated
There was a problem hiding this comment.
Can also remove this line, because there are no signatures
There was a problem hiding this comment.
tx.vout = [] is necessary since the transaction is serialized in check_mempool_result. Without this, the transaction does not get rejected for the right reasons. Please let me know if I misunderstood the suggestion.
Replaced all wallet functionality in `test/functional/mempool_accept.py` to use `test/functional/test_framework/wallet.py` instead of the wallet built with Bitcoin Core. Co-authored-by: Sishir Giri <[email protected]> Co-authored-by: Sebastian Falbesoner <[email protected]>
sriramdvt
force-pushed
the
mini_memaccept
branch
from
7cd06b3 to
69dda69
Compare
|
Rebased and updated with the suggestions |
|
🐙 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". |
|
Unfortunately this needs significant changes due to changes to the affected tests since. Are you still planning to address this? |
|
@laanwj Thank you for the reminder! I've edited it to be a draft PR in the meanwhile, I will address the changes in a few days and change the status. |
|
mempool_accept.py is already using MiniWallet on master, i.e. this PR should be closed. |
|
Right, see commit 807169e |
7 participants
Replace all wallet-related functionality in
test/functional/mempool_accept.pyto use MiniWallet instead of the wallet built with Bitcoin Core. This allows the test to run even if Bitcoin Core was compiled with--disable-wallet.Work on
mempool_accept.pystarted in #21014, but it has been inactive for some time. This PR also makes use of additional features likescan_blocks()andcreate_self_transfer()that were added to MiniWallet.To test this PR, build Bitcoin Core with(out) the wallet and run: