test: move sync_blocks and sync_mempool functions to test_framework.py #19208

ycshao · 2020-06-08T12:59:45Z

This PR moves sync_blocks and sync_mempool out from test_framework/util.py to test_framework/test_framework.py so they can take contextual information of test framework into account.

Change all reference callers to call functions from test_framework.py
Remove **kwargs which is not used
Take into account of timeout_factor when respecting timeout in function implementations.
Pass all tests by running ./test/functional/test_runner.py

fixes #18930

maflcko · 2020-06-08T13:03:29Z

Nice! Concept ACK

ycshao · 2020-06-08T13:12:50Z

Seems like the style of those files are auto-formatted by autopep8. I'll leave the reviewers to comment about the format.

One question about the implementation. Does it make sense to move assert (all([len(x.getpeerinfo()) for x in rpc_connections])) up to immediately after the while loop starts? It will assert sooner and save some time and operations.

ycshao · 2020-06-08T18:03:07Z

Looks like wallet_avoidreuse.py failed in travis but it works fine on my machine. Is there a way to replay the travis failed tests?

DrahtBot · 2020-06-09T00:06:22Z

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: Check that peers with forcerelay permission are not asked to feefilter #19198 (test: Check that peers with forcerelay permission are not asked to feefilter 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.

ycshao · 2020-06-09T01:26:45Z

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.

Could someone explain what does conflicts mean here? Is there a logical order of PRs mentioned + mine to review and merge?

adamjonas · 2020-06-09T14:03:18Z

@ycshao DrahtBot crawls over open PRs and alerts you when there is a conflict with the main branch that requires a rebase or, in this case, other PRs that may lead to future conflicts indicating that you are working on similar code. DrahtBot is not sentient enough to decide the merge order. It's just raising it to your attention.

adamjonas · 2020-06-09T15:48:13Z

Re the format - autopep8 is making the diff much larger. I think it's easier, for now, to keep the diff as minimal as possible so reviewers can focus on the changes relevant to your patch. At the very least, you can break the formatting changes into their own commit and follow the lead of this reformat.

I'd also suggest squashing your second commit since the first commit doesn't pass the linter without it.

ycshao · 2020-06-09T17:32:18Z

@adamjonas thanks for the answer and suggestion. I'll squash and break formatting into it's own commit.

glozow

Oo first PR? Concept ACK :) I'm guessing "contextual information of test framework" is the timeout_factor?
+1 to Jonas, just appeasing test/lint/lint-python.sh for now would be better.

Does it make sense to move assert (all([len(x.getpeerinfo()) for x in rpc_connections])) up to immediately after the while loop starts? It will assert sooner and save some time and operations.

I had this thought too. But I think it would fail if the nodes happened to all be synced up (despite not being connected right now) before the first iteration of the while loop. I think it's possible...?

glozow · 2020-06-09T19:24:50Z

test/functional/test_framework/test_framework.py

I have a question - it's probably not a big deal to require at least one node to be updated, but would it be appropriate to have an optional blockhash parameter and enforce that hash instead?

I don't see a use case like that in current code. But I think one benefit your idea will give is to shorten test run time if the block we need to sync to is not usually the tip.

ycshao · 2020-06-09T21:07:33Z

Oo first PR? Concept ACK :) I'm guessing "contextual information of test framework" is the timeout_factor?

Yeah for now timeout_factor is easiest to be used. There are other things potentially can be used. I'm open to suggestions.

rpc_timeout - I think rpc_timeout is usually sometime different than timeout people want to configure for syncing operations. Unless there is a factor that can be applied to rpc_timeout to estimate syncing timeout. Open to suggestions.
nodes - We can by default sync all nodes and take in a parameter in sync_blocks and sync_mempool in case subset of nodes need to be synced. This way we get rid of nodes input of those two functions.

+1 to Jonas, just appeasing test/lint/lint-python.sh for now would be better.

lint-python.sh gives me other linting errors which is not related to the file I touched. I guess I'll just make those five files I touched to be pep8 in a separate commit. If it's not necessary, it's easy to take it out.

test/functional/p2p_compactblocks.py:688:43: E741 ambiguous variable name 'l'
test/functional/p2p_compactblocks.py:694:13: E741 ambiguous variable name 'l'
test/functional/p2p_compactblocks.py:697:17: E741 ambiguous variable name 'l'
test/functional/p2p_dos_header_tree.py:37:38: E741 ambiguous variable name 'l'
test/functional/p2p_dos_header_tree.py:42:31: E741 ambiguous variable name 'l'
test/functional/p2p_dos_header_tree.py:43:55: E741 ambiguous variable name 'l'
test/functional/test_framework/messages.py:70:22: E741 ambiguous variable name 'l'
test/functional/test_framework/messages.py:142:16: E741 ambiguous variable name 'l'
test/functional/test_framework/messages.py:161:24: E741 ambiguous variable name 'l'
test/functional/test_framework/messages.py:177:23: E741 ambiguous variable name 'l'
test/functional/wallet_labels.py:149:13: E741 ambiguous variable name 'l'
test/functional/wallet_labels.py:155:13: E741 ambiguous variable name 'l'
test/lint/lint-python.sh: line 105: mypy: command not found

Does it make sense to move assert (all([len(x.getpeerinfo()) for x in rpc_connections])) up to immediately after the while loop starts? It will assert sooner and save some time and operations.

I had this thought too. But I think it would fail if the nodes happened to all be synced up (despite not being connected right now) before the first iteration of the while loop. I think it's possible...?

I guess it's theoretically possible.

test/functional/test_framework/test_framework.py

maflcko

Concept ACK, but please change the commit title to

test: move sync_blocks and sync_mempool functions to test_framework.py

maflcko · 2020-06-18T12:21:55Z

test/functional/test_framework/test_framework.py

why is this changed?

I removed **kwargs which are not used.

I think they might come in handy in the future. Is there a reason they will be unused forever?

I feel it's better to add it when it will be used in instead of putting it there up front just because some time in the future it might be used. What do you think?

test/functional/test_framework/test_framework.py

maflcko · 2020-06-18T17:08:31Z

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits and change the commit title to something more meaningful: #19208 (review)

maflcko · 2020-06-18T20:36:12Z

ACK cc84460 , reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space 💫

Show signature and timestamp

Signature:

Timestamp of file with hash bea4e035a3dba2f2997391d4f35010649fa6ca21167797054925a5e15c237f47 -

ycshao · 2020-06-22T05:05:09Z

@MarcoFalke @gzhao408 @adamjonas thanks for reviewing and merge!

2488ebc test: move sync_blocks and sync_mempool functions to test_framework.py (random-zebra) d076c81 [Test] Fix intermittent sync_blocks failures (random-zebra) f0de789 Move g_is_mempool_loaded into CTxMemPool::m_is_loaded (Ben Woosley) 0169a75 rpc: Expose g_is_mempool_loaded via getmempoolinfo and /rest/mempool/info.json (Ben Woosley) Pull request description: Backport the following PRs: - bitcoin#15323: rpc: Expose g_is_mempool_loaded via getmempoolinfo - bitcoin#18873: test: Fix intermittent sync_blocks failures - bitcoin#19208: test: move sync_blocks and sync_mempool functions to test_framework.py ACKs for top commit: furszy: nice finding!, code review ACK 2488ebc. Fuzzbawls: ACK 2488ebc Tree-SHA512: 4971a0076b4179b78c5338073a10fbf0649310c2cf3907cff1ab5c02f8dd67f5f7bb4bf524d69c554c8317bd98ce46411ff740aff676ed147525c75d83214976

Summary: This is a backport of [[bitcoin/bitcoin#19208 | core#19208]] Test Plan: `ninja check-functional-extended` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9431

fanquake added the Tests label Jun 8, 2020

ycshao force-pushed the issue-18930 branch from 8a7a292 to 45efd83 Compare June 8, 2020 13:01

ycshao marked this pull request as ready for review June 8, 2020 13:12

ycshao changed the title ~~test: move sync_blocks and sync_mempool functions to test_framework.py~~ test: move sync_blocks and sync_mempool functions to test_framework.py Jun 8, 2020

This was referenced Jun 9, 2020

test: Check that peers with forcerelay permission are not asked to feefilter #19198

Merged

test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until #19134

Closed

DrahtBot mentioned this pull request Jun 9, 2020

tests: Update more tests to work with descriptor wallets #18788

Merged

glozow reviewed Jun 9, 2020

View reviewed changes

ycshao force-pushed the issue-18930 branch from 36404ae to 5347a75 Compare June 9, 2020 20:44

This was referenced Jun 10, 2020

test: changing signature of wait_until(). #19048

Closed

test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013

Merged

Add Muhash3072 implementation in Python #19105

Merged

Add hash_type MUHASH for gettxoutsetinfo #19145

Merged

maflcko reviewed Jun 16, 2020

View reviewed changes

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

DrahtBot added the Needs rebase label Jun 16, 2020

ycshao force-pushed the issue-18930 branch from 4563520 to 5347a75 Compare June 18, 2020 02:17

DrahtBot removed the Needs rebase label Jun 18, 2020

maflcko reviewed Jun 18, 2020

View reviewed changes

test: move sync_blocks and sync_mempool functions to test_framework.py

cc84460

ycshao force-pushed the issue-18930 branch from dda75b9 to cc84460 Compare June 18, 2020 17:34

maflcko merged commit 4b5c919 into bitcoin:master Jun 21, 2020

random-zebra mentioned this pull request Mar 19, 2021

[Tests] sync_blocks and sync_mempool improvements PIVX-Project/PIVX#2254

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

test: move sync_blocks and sync_mempool functions to test_framework.py #19208

test: move sync_blocks and sync_mempool functions to test_framework.py #19208

Uh oh!

Conversation

ycshao commented Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 8, 2020

Uh oh!

ycshao commented Jun 8, 2020

Uh oh!

ycshao commented Jun 8, 2020

Uh oh!

DrahtBot commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ycshao commented Jun 9, 2020

Uh oh!

adamjonas commented Jun 9, 2020

Uh oh!

adamjonas commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycshao commented Jun 9, 2020

Uh oh!

glozow left a comment • 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!

glozow Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ycshao Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

ycshao commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

ycshao Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

ycshao Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 18, 2020

Uh oh!

maflcko commented Jun 18, 2020

Uh oh!

ycshao commented Jun 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ycshao commented Jun 8, 2020 •

edited

Loading

DrahtBot commented Jun 9, 2020 •

edited

Loading

adamjonas commented Jun 9, 2020 •

edited

Loading

glozow left a comment •

edited

Loading

ycshao commented Jun 9, 2020 •

edited

Loading