test: Run mempool_updatefromblock even with wallet disabled #21999

reemuru · 2021-05-19T21:30:37Z

This is a PR proposed in #20078

This PR enables one more of the non-wallet functional tests (mempool_updatefromblock.py) to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead.

Overview:

Refactoring and removal of unnecessary imports and arguments to transaction_graph_test()
Improve logging
Decrease test execution time by ~70% (39s => 12s)

Before:

[USER@SERVER functional]$ time ./mempool_updatefromblock.py 
2021-05-19T21:02:10.809000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_42xs1s0g
2021-05-19T21:02:11.154000Z TestFramework (INFO): Creating 100 transactions...
2021-05-19T21:02:15.741000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:15.756000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:02:20.097000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:20.115000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:02:24.687000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:24.705000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:02:28.774000Z TestFramework (INFO): The last batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:02:29.160000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.3855452537536621 seconds.
2021-05-19T21:02:29.161000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
2021-05-19T21:02:50.455000Z TestFramework (INFO): Stopping nodes
2021-05-19T21:02:50.508000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_42xs1s0g on exit
2021-05-19T21:02:50.508000Z TestFramework (INFO): Tests successful

real    0m39.895s
user    0m34.627s
sys     0m2.474s

After:

[USER@SERVER functional]$ time ./mempool_updatefromblock.py 
2021-05-19T21:08:17.190000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__3fuskkh
2021-05-19T21:08:17.493000Z TestFramework (INFO): Creating 100 transactions...
2021-05-19T21:08:17.634000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:17.686000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:17.771000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:17.820000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:17.896000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:17.943000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:18.009000Z TestFramework (INFO): The batch of 25 transactions has been accepted into the mempool.
2021-05-19T21:08:18.058000Z TestFramework (INFO): All of the transactions from the current batch have been mined into a block.
2021-05-19T21:08:18.060000Z TestFramework (INFO): Mempool size pre-invalidation: 0
2021-05-19T21:08:18.080000Z TestFramework (INFO): Mempool size post-invalidation: 100
2021-05-19T21:08:18.082000Z TestFramework (INFO): All of the recently mined transactions have been re-added into the mempool in 0.0179746150970459 seconds.
2021-05-19T21:08:18.082000Z TestFramework (INFO): Checking descendants/ancestors properties of all of the in-mempool transactions...
2021-05-19T21:08:29.441000Z TestFramework (INFO): Stopping nodes
2021-05-19T21:08:29.496000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test__3fuskkh on exit
2021-05-19T21:08:29.496000Z TestFramework (INFO): Tests successful

real    0m12.465s
user    0m5.843s
sys     0m0.800s

DariusParvin

obvious concept ACK.
Great work! I left some suggestions/questions

test/functional/mempool_updatefromblock.py

reemuru · 2021-05-20T22:39:18Z

Updated with ac8b08b. Let me know if I missed anything! Also, should I roll theses commits up into one, or will they get squashed at merge time? Forgot to sign the first few.
Recent changes:

self.wallet.generate(1) to self.wallet.scan_blocks(start=76, num=1)
remove blocks mined block_ids from cluttering logs along with unnecessary math import
put back enumerate for mempool properties checks
added assert before mining block

DariusParvin · 2021-05-21T00:37:22Z

The changes look good to me! I'm relatively new myself but I think at this stage it would make sense to squash everything into one commit.

reemuru · 2021-05-21T02:41:30Z

The changes look good to me! I'm relatively new myself but I think at this stage it would make sense to squash everything into one commit.

Got it, thank you for your assistance!

sriramdvt

tACK df07919caa36d1345590010f5552d9e24db2bbde. The test works properly and checks what it is supposed to check. I strongly recommend making these changes for clarity and to maintain forward compatibility with MiniWallet.

test/functional/mempool_updatefromblock.py

sriramdvt · 2021-07-23T16:51:56Z

test/functional/mempool_updatefromblock.py

nit: Replace self.nodes[0] with node

reemuru

tACK df07919. The test works properly and checks what it is supposed to check. I strongly recommend making these changes for clarity and to maintain forward compatibility with MiniWallet.

Thanks for reviewing! Made some changes based on your suggestions.

test/functional/mempool_updatefromblock.py

josibake · 2021-07-26T13:27:42Z

Concept ACK

Thanks for posting the profiling data! Definitely agree with more miniwallet usage, especially if it increases test execution speed. I compiled and ran the tests and everything passed, but I would definitely recommend running flake8 and making the suggested style changes. you can do this automatically with autopep8 (I did it by with the following steps):

pip install autopep8
autopep8 --in-place mempool_updatefromblock.py

you can double check the changes by running flake8 mempool_updatefromblock.py. you should only see line length warnings, which are fine to ignore

practicalswift · 2021-07-26T16:13:09Z

Concept ACK

reemuru · 2021-07-26T17:33:31Z

pip install autopep8

oh shoots! never used autopep8 before. will use from now on. Thanks for the tip! (^_^) => 0b7dbeb

josibake · 2021-07-26T22:27:56Z

pip install autopep8
oh shoots! never used autopep8 before. will use from now on. Thanks for the tip! (^_^) => 0b7dbeb

looks great! id suggest squashing into one atomic commit per the contributing guide

reemuru · 2021-07-27T02:05:00Z

pip install autopep8
oh shoots! never used autopep8 before. will use from now on. Thanks for the tip! (^_^) => 0b7dbeb
looks great! id suggest squashing into one atomic commit per the contributing guide

Got it, thanks much!

josibake · 2021-07-27T11:06:15Z

ACK 446c06a

built without wallet, ran the test to verify everything is behaving as expected. also code reviewed to verify the logic of the test is unchanged (strictly a refactor)

DrahtBot · 2021-07-27T20:41:56Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567 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.

maflcko · 2021-07-28T12:21:22Z

test/functional/mempool_updatefromblock.py

please don't commit large style changes in the same commit as refactors/features. This makes review harder because it is not clear what is a refactor/style-change/feature

https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches

Also I don't think the change here makes the code easier to read (neither for devs nor for scripts and parsers)

Oh shoot, my bad. 😭 Was using some auto-styling tool and must've finger fudged it. Fixed and squashed into 33865a5

Oh shoot, my bad. sob Was using some auto-styling tool and must've finger fudged it. Fixed and squashed into 33865a5

my fault, @reemuru , i gave bad advice. @MarcoFalke is correct: linting (style changes) should be in separate PR's from refactors/features

josibake · 2021-07-28T15:17:08Z

ACK c1c0768

built without wallet and ran test

maflcko · 2021-07-28T15:26:21Z

test/functional/mempool_updatefromblock.py

here are still style changes mixed in

maflcko · 2021-07-28T15:26:33Z

test/functional/mempool_updatefromblock.py

why is this changed?

Ah ok, apologies for the noise. Will put this into draft mode since some work is still needed. Thanks for review!

Enables the mempool_updatefromblock non-wallet functional test to be run even with the Bitcoin Core wallet disabled by using the new MiniWallet instead. - Refactoring and removal of unnecessary imports and arguments to transaction_graph_test() - Improve logging - Decrease test execution time by ~70% (39s => 12s)

adamjonas · 2021-08-05T23:17:31Z

@reemuru was there a reason this closed? Would you like to mark it up for grabs?

pg156 · 2022-01-12T13:40:59Z

I will start working on this as it is up for grabs. Please let me know if this is done elsewhere or no longer necessary?

maflcko · 2022-01-12T13:50:25Z

You can open the file on the master branch and check if skip_if_no_wallet is still in the file to see if this is still relevant.

pg156 · 2022-01-13T02:43:08Z

Thanks @MarcoFalke. I see skip_if_no_wallet is still in https://github.com/bitcoin/bitcoin/blob/master/test/functional/mempool_updatefromblock.py. I also checked each revision to the file since this PR was created, and don't see any change making this PR irrelevant. Did I miss anything before determining it is useful to work on this?

fanquake added the Tests label May 19, 2021

DariusParvin reviewed May 20, 2021

View reviewed changes

sriramdvt suggested changes Jul 23, 2021

View reviewed changes

reemuru commented Jul 25, 2021

View reviewed changes

test/functional/mempool_updatefromblock.py Outdated Show resolved Hide resolved

This was referenced Jul 27, 2021

Mempool Update Cut-Through Optimization #21464

Merged

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

Closed

maflcko reviewed Jul 28, 2021

View reviewed changes

reemuru marked this pull request as draft July 30, 2021 13:59

reemuru closed this Aug 1, 2021

reemuru deleted the test branch August 1, 2021 02:46

adamjonas added the Up for grabs label Aug 19, 2021

pg156 mentioned this pull request Jan 17, 2022

test: Convert non-wallet tests to use our python MiniWallet #20078

Closed

pg156 mentioned this pull request Jan 28, 2022

test: use MiniWallet for mempool_updatefromblock.py #24183

Closed

maflcko removed the Up for grabs label Mar 22, 2022

bitcoin locked and limited conversation to collaborators Mar 22, 2023

test: Run mempool_updatefromblock even with wallet disabled #21999

test: Run mempool_updatefromblock even with wallet disabled #21999

Uh oh!

Conversation

reemuru commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DariusParvin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

reemuru commented May 20, 2021

Uh oh!

DariusParvin commented May 21, 2021

Uh oh!

reemuru commented May 21, 2021

Uh oh!

sriramdvt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sriramdvt Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

reemuru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

josibake commented Jul 26, 2021

Uh oh!

practicalswift commented Jul 26, 2021

Uh oh!

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

Uh oh!

Uh oh!

josibake commented Jul 26, 2021

Uh oh!

reemuru commented Jul 27, 2021

Uh oh!

josibake commented Jul 27, 2021

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

reemuru Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

josibake Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

josibake commented Jul 28, 2021

Uh oh!

maflcko Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 28, 2021

Choose a reason for hiding this comment

Uh oh!

reemuru Jul 30, 2021

Choose a reason for hiding this comment

Uh oh!

adamjonas commented Aug 5, 2021

Uh oh!

pg156 commented Jan 12, 2022

Uh oh!

maflcko commented Jan 12, 2022

reemuru commented May 19, 2021 •

edited

Loading

reemuru commented Jul 26, 2021 •

edited

Loading

DrahtBot commented Jul 27, 2021 •

edited

Loading

maflcko Jul 28, 2021 •

edited

Loading