test: p2p_feefilter improvements (logging, refactoring, speedup) #19564

theStack · 2020-07-22T09:18:53Z

This PR gives some love to the functional test p2p_feefilter.py by introducing the following changes:

add missing log messages for the test_feefilter subtest (the main one)
deduplicate code by using the utility function wait_until (already using the recently introduced version) instead of a manual condition checking loop with time.sleep
improve naming of the function matchAllInvs (more expressive name, snake_case) and moving it from global namespace to the connection class FeefilterConn
speeding up the test significantly by the good ol' method of activating immediate tx relay (as seen on e.g. test: speedup wallet_backup by whitelisting peers (immediate tx relay) #17121, test: speed up wallet_address_types by whitelisting peers (immediate tx relay) #17124, Tests: speed up fundrawtransaction test #17340, test: speed up wallet_avoidreuse, add logging #17362, ...):

master branch:
$ time ./p2p_feefilter.py
...
real    0m39.367s
user    0m1.227s
sys 0m0.571s

PR branch:
$ time ./p2p_feefilter.py
...
real    0m9.386s
user    0m1.120s
sys 0m0.577s

jonatack

Concept ACK

Edit: verified the test runs significantly more quickly with the noban arg.

test/functional/p2p_feefilter.py

theStack · 2020-07-22T15:13:28Z

Thans a lot for reviewing jonatack! Force-pushed with your suggested changes.

jonatack

LGTM, a couple more comments below.

jonatack · 2020-07-22T16:32:17Z

test/functional/p2p_feefilter.py

I wonder if we can eek some more perf by memoizing (untested):

Suggested change

test_framework.wait_until(lambda: sorted(invs_expected) == sorted(self.txinvs),

expected_txinvs = sorted(invs_expected)

test_framework.wait_until(lambda: sorted(self.txinvs) == expected_txinvs),

Yes, that makes sense to do the sorting of the txinvs to test for only once. Did it with in-place sort of the passed list.

test/functional/p2p_feefilter.py

theStack · 2020-08-04T21:32:08Z

Rebased on master and added suggested changes by jonatack (only sort expected invs once, use correct terminology regarding noban permission terminology).

jonatack

ACK 2d43d84

Restarted the stalled s390x Travis job.

jonatack · 2020-08-05T07:07:09Z

test/functional/p2p_feefilter.py

c063dce9 nit: if you have to retouch, can remove "being" here and in line 87.

DrahtBot · 2020-08-06T19:54:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: test: use throwaway _ variable for unused loop counters #19674 (refactor: test: use throwaway _ variable for unused loop counters by theStack)

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 · 2020-08-11T10:20:56Z

Rebased.

jonatack · 2020-08-12T08:15:07Z

re-ACK ea74a3c per git range-diff ce3bdd0e 2d43d84 ea74a3c

maflcko

Looks fine, some style nits

4fbda7da50d508481d9a94b00e4dc994dad9465d

test/functional/p2p_feefilter.py

additionally: -> rename function with snake_case (s/allInvsMatch/wait_for_invs_to_match) -> move it from global namespace to the class FeefilterConn

…lay) Most of the test time is spent in wait_for_invs() after sending to addresses, i.e. the bottleneck is in relaying transactions. By whitelisting the peers via -whitelist, the inventory is transmissioned immediately rather than on average every 5 seconds, speeding up the test significantly: before: $ time ./p2p_feefilter.py ... real 0m39.367s user 0m1.227s sys 0m0.571s with this commit: $ time ./p2p_feefilter.py ... real 0m9.386s user 0m1.120s sys 0m0.577s

theStack · 2020-08-12T23:48:14Z

Thanks for reviewing Marco. Force-pushed with your suggestions and a small log-message change proposed by jonatack previously (#19564 (comment)).

instagibbs · 2020-08-13T02:18:40Z

code review ACK 9e78943

jonatack · 2020-08-16T08:30:10Z

re-ACK 9e78943 per git range-diff 3ab2582 ea74a3c 9e78943

…toring, speedup) 9e78943 test: speedup p2p_feefilter.py by whitelisting peers (immediate tx relay) (Sebastian Falbesoner) fe3f0cc test: use wait_until for invs matching in p2p_feefilter.py (Sebastian Falbesoner) 6d94192 test: add logging for p2p_feefilter.py (Sebastian Falbesoner) Pull request description: This PR gives some love to the functional test `p2p_feefilter.py` by introducing the following changes: * add missing log messages for the `test_feefilter` subtest (the main one) * deduplicate code by using the utility function `wait_until` (already using the [recently introduced version](bitcoin@12410b1)) instead of a manual condition checking loop with `time.sleep` * improve naming of the function `matchAllInvs` (more expressive name, snake_case) and moving it from global namespace to the connection class `FeefilterConn` * speeding up the test significantly by the good ol' method of activating immediate tx relay (as seen on e.g. bitcoin#17121, bitcoin#17124, bitcoin#17340, bitcoin#17362, ...): ``` master branch: $ time ./p2p_feefilter.py ... real 0m39.367s user 0m1.227s sys 0m0.571s PR branch: $ time ./p2p_feefilter.py ... real 0m9.386s user 0m1.120s sys 0m0.577s ``` ACKs for top commit: instagibbs: code review ACK bitcoin@9e78943 jonatack: re-ACK 9e78943 per `git range-diff 3ab2582 ea74a3c 9e78943` Tree-SHA512: fe21c1c5413df9165fea916b5d5f609d3ba33e7b5c3364b38eb824fcc55d9e6abddf27116cbc0b325913d451a73c44542040fb916aec9c46f805c6e12f6f10cf

Summary: > test: add logging for p2p_feefilter.py bitcoin/bitcoin@6d94192 > test: use wait_until for invs matching in p2p_feefilter.py > > additionally: > -> rename function with snake_case (s/allInvsMatch/wait_for_invs_to_match) > -> move it from global namespace to the class FeefilterConn bitcoin/bitcoin@fe3f0cc > test: speedup p2p_feefilter.py by whitelisting peers (immediate tx relay) > > Most of the test time is spent in wait_for_invs() after sending to addresses, > i.e. the bottleneck is in relaying transactions. By whitelisting the peers via > -whitelist, the inventory is transmissioned immediately rather than on average > every 5 seconds, speeding up the test significantly: bitcoin/bitcoin@9e78943 This is a backport of [[bitcoin/bitcoin#19564 | core#19564]] Test Plan: before: ``` TEST | STATUS | DURATION p2p_feefilter.py | ✓ Passed | 29 s ALL | ✓ Passed | 29 s (accumulated) Runtime: 21 s ``` after: ``` TEST | STATUS | DURATION p2p_feefilter.py | ✓ Passed | 4 s ALL | ✓ Passed | 4 s (accumulated) ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10111

fanquake added the Tests label Jul 22, 2020

jonatack reviewed Jul 22, 2020

View reviewed changes

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

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

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

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

theStack force-pushed the 20200722-test-p2p_feefilter_improvements branch from bde6920 to 302388e Compare July 22, 2020 15:11

jonatack reviewed Jul 22, 2020

View reviewed changes

theStack force-pushed the 20200722-test-p2p_feefilter_improvements branch from 302388e to 2d43d84 Compare August 4, 2020 21:25

jonatack reviewed Aug 5, 2020

View reviewed changes

test/functional/p2p_feefilter.py Outdated

Copy link

Member

jonatack Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c063dce9 nit: if you have to retouch, can remove "being" here and in line 87.

DrahtBot mentioned this pull request Aug 6, 2020

refactor: test: use throwaway _ variable for unused loop counters #19674

Merged

DrahtBot added the Needs rebase label Aug 11, 2020

theStack force-pushed the 20200722-test-p2p_feefilter_improvements branch from 2d43d84 to ea74a3c Compare August 11, 2020 10:20

DrahtBot removed the Needs rebase label Aug 11, 2020

laanwj requested review from instagibbs and maflcko August 12, 2020 18:33

maflcko approved these changes Aug 12, 2020

View reviewed changes

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

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

theStack added 3 commits August 12, 2020 22:21

test: add logging for p2p_feefilter.py

6d94192

test: use wait_until for invs matching in p2p_feefilter.py

fe3f0cc

additionally: -> rename function with snake_case (s/allInvsMatch/wait_for_invs_to_match) -> move it from global namespace to the class FeefilterConn

theStack force-pushed the 20200722-test-p2p_feefilter_improvements branch from ea74a3c to 9e78943 Compare August 12, 2020 23:39

maflcko merged commit a57af89 into bitcoin:master Aug 16, 2020

theStack deleted the 20200722-test-p2p_feefilter_improvements branch December 1, 2020 09:58

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

	test_framework.wait_until(lambda: sorted(invs_expected) == sorted(self.txinvs),
	expected_txinvs = sorted(invs_expected)
	test_framework.wait_until(lambda: sorted(self.txinvs) == expected_txinvs),

test: p2p_feefilter improvements (logging, refactoring, speedup) #19564

test: p2p_feefilter improvements (logging, refactoring, speedup) #19564

Uh oh!

Conversation

theStack commented Jul 22, 2020

Uh oh!

jonatack 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theStack commented Jul 22, 2020

Uh oh!

jonatack 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!

jonatack Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

theStack Aug 4, 2020 • 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!

Uh oh!

theStack commented Aug 4, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 6, 2020

Conflicts

Uh oh!

theStack commented Aug 11, 2020

Uh oh!

jonatack commented Aug 12, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Aug 13, 2020

Uh oh!

jonatack commented Aug 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jonatack left a comment •

edited

Loading

jonatack left a comment •

edited

Loading

theStack Aug 4, 2020 •

edited

Loading

theStack commented Aug 12, 2020 •

edited

Loading