test: Use MiniWallet in mempool_accept.py #22509

sriramdvt · 2021-07-20T18:23:30Z

Replace all wallet-related functionality in test/functional/mempool_accept.py to 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.py started in #21014, but it has been inactive for some time. This PR also makes use of additional features like scan_blocks() and create_self_transfer() that were added to MiniWallet.

To test this PR, build Bitcoin Core with(out) the wallet and run:

$ test/functional/mempool_accept.py

theStack · 2021-07-21T09:29:55Z

Concept ACK

DrahtBot · 2021-07-21T10:39:51Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22788 (scripted-diff: Use generate* from TestFramework by MarcoFalke)

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.

theStack · 2021-07-21T12:23:11Z

@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.

maflcko · 2021-07-21T13:27:00Z

Concept ACK.

In the commit message, the Co-authored-by: needs to be a trailer (last line of the message).
( https://docs.github.com/en/github/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors )

theStack

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.

test/functional/mempool_accept.py

practicalswift · 2021-07-24T19:54:51Z

Concept ACK

More MiniWallet is better.

maflcko

Left some comments

maflcko · 2021-07-30T11:30:56Z

test/functional/mempool_accept.py

+        utxo_1 = miniwallet.get_utxo()
+        miniwallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_1)['txid']


Suggested change

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

Also, why the ['txid'] in the end?

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

maflcko · 2021-07-30T11:37:32Z

test/functional/mempool_accept.py

            maxfeerate=0,
        )

        self.log.info('A transaction with no outputs')
-        tx = tx_from_hex(raw_tx_reference)
+        tx = tx_from_hex(tx_reference['hex'])
        tx.vout = []
        # Skip re-signing the transaction for context independent checks from now on


Can also remove this line, because there are no signatures

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.

I meant the comment line 😅

test/functional/mempool_accept.py

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 <sishirg27@gmail.com> Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

sriramdvt · 2021-09-01T08:37:37Z

Rebased and updated with the suggestions

DrahtBot · 2021-09-09T13:05:07Z

🐙 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".}

laanwj · 2021-12-17T19:04:45Z

Unfortunately this needs significant changes due to changes to the affected tests since. Are you still planning to address this?

sriramdvt · 2021-12-17T19:27:40Z

@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.

theStack · 2022-03-23T15:27:32Z

mempool_accept.py is already using MiniWallet on master, i.e. this PR should be closed.

maflcko · 2022-03-23T15:34:51Z

Right, see commit 807169e

sriramdvt force-pushed the mini_memaccept branch from aa068ad to 936f56b Compare July 20, 2021 18:32

maflcko mentioned this pull request Jul 20, 2021

test: Run mempool_accept.py even with wallet disabled #21014

Closed

DrahtBot added the Tests label Jul 20, 2021

sriramdvt force-pushed the mini_memaccept branch 2 times, most recently from 80008d0 to 48e55a8 Compare July 21, 2021 12:21

sriramdvt force-pushed the mini_memaccept branch from 48e55a8 to 936f56b Compare July 21, 2021 12:25

DrahtBot mentioned this pull request Jul 21, 2021

test: remove confusing MAX_BLOCK_BASE_SIZE #22378

Merged

theStack reviewed Jul 21, 2021

View reviewed changes

sriramdvt force-pushed the mini_memaccept branch from 936f56b to 7cd06b3 Compare July 21, 2021 18:18

DrahtBot mentioned this pull request Jul 28, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

maflcko reviewed Jul 30, 2021

View reviewed changes

DrahtBot added the Needs rebase label Aug 2, 2021

sriramdvt and others added 2 commits September 1, 2021 12:04

Minor changes to MiniWallet

dbf283a

sriramdvt force-pushed the mini_memaccept branch from 7cd06b3 to 69dda69 Compare September 1, 2021 08:26

DrahtBot removed the Needs rebase label Sep 1, 2021

DrahtBot mentioned this pull request Sep 2, 2021

scripted-diff: Use generate* from TestFramework #22788

Merged

DrahtBot added the Needs rebase label Sep 9, 2021

sriramdvt marked this pull request as draft December 17, 2021 19:25

maflcko added the Up for grabs label Mar 15, 2022

fanquake closed this Mar 23, 2022

fanquake removed Up for grabs Needs rebase labels Mar 23, 2022

bitcoin locked and limited conversation to collaborators Mar 23, 2023

		utxo_1 = miniwallet.get_utxo()
		miniwallet.send_self_transfer(from_node=node, utxo_to_spend=utxo_1)['txid']

test: Use MiniWallet in mempool_accept.py #22509

test: Use MiniWallet in mempool_accept.py #22509

Uh oh!

Conversation

sriramdvt commented Jul 20, 2021

Uh oh!

theStack commented Jul 21, 2021

Uh oh!

DrahtBot commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Jul 21, 2021

Uh oh!

maflcko commented Jul 21, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jul 24, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

sriramdvt Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

sriramdvt Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sriramdvt commented Sep 1, 2021

Uh oh!

DrahtBot commented Sep 9, 2021

Uh oh!

laanwj commented Dec 17, 2021

Uh oh!

sriramdvt commented Dec 17, 2021

Uh oh!

theStack commented Mar 23, 2022

Uh oh!

maflcko commented Mar 23, 2022

Uh oh!

Uh oh!

DrahtBot commented Jul 21, 2021 •

edited

Loading