Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: speed up wallet_avoidreuse, add logging #17362

Merged
merged 2 commits into from Nov 7, 2019
Merged

test: speed up wallet_avoidreuse, add logging #17362

merged 2 commits into from Nov 7, 2019

Conversation

jonatack
Copy link
Contributor

@jonatack jonatack commented Nov 4, 2019 •
edited

Inspired by PRs #17340 and #15881.

  • add logging
  • pass -whitelist in set_test_params to speed up transaction relay

wallet_avoidreuse.py is not intended to test P2P transaction relay/timing, so it should be fine to do this here. This reduces test run time variability and speeds up the test by 2-3 times on average.

Test run times in seconds:

  • before: 20, 24, 22, 17, 27, 40, 30

  • after: 10, 10, 8, 9, 10, 7, 8

@jonatack
Copy link
Contributor Author

jonatack commented Nov 4, 2019 •
edited

Test output with logging:

$ test/functional/wallet_avoidreuse.py 
2019-11-04T09:25:24.487000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_5geycz8_
2019-11-04T09:25:25.657000Z TestFramework (INFO): Test wallet files persist avoid_reuse flag
2019-11-04T09:25:26.715000Z TestFramework (INFO): Test immutable wallet flags
2019-11-04T09:25:27.637000Z TestFramework (INFO): Test fund send fund send dirty
2019-11-04T09:25:31.799000Z TestFramework (INFO): Test fund send fund send
2019-11-04T09:25:34.649000Z TestFramework (INFO): Stopping nodes
2019-11-04T09:25:34.955000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_5geycz8_ on exit
2019-11-04T09:25:34.955000Z TestFramework (INFO): Tests successful

@jonatack jonatack force-pushed the wallet_avoidreuse-test-improvements branch from 5ea05c9 to d2edf02 Compare November 4, 2019 09:56
@DrahtBot DrahtBot added the Tests label Nov 4, 2019
jachiang
jachiang suggested changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@jachiang jachiang left a comment •
edited

Choose a reason for hiding this comment

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

Thanks @jonatack!

tACK d2edf02
Reviewed test, also don't see any need for tx propagation delay.

My informal run times confirm improvement.
Before 463eab5 : 31.9 / 22.9 / 35.7 / 29.8 / 34.8 / 29.7 /
With patch d2edf02: 14.7 / 11.9 / 10.7/ 12.9 / 13.7 / 13.7 /12.4

However, I am unsure of the source of variability. I don't see an improvement there.

test/functional/wallet_avoidreuse.py Show resolved Hide resolved
Sjors
Sjors approved these changes Nov 4, 2019
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

ACK d2edf02 on macOS 10.15 with --enable-debug (which is slower, but better at catching issues).

Before: 18-25 seconds
After: 10 seconds

Part of inconsistency can come from caching.

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved
maflcko
maflcko reviewed Nov 4, 2019
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

Just some general notes. I don't see a reason to do most of these changes. Most look like irrelevant character shuffling, which neither the python interpreter should care about nor any human reader of the code.

No need to change them and invalidate the ACK, but something to keep in mind for the future.

test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved
test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved
test/functional/wallet_avoidreuse.py Outdated Show resolved Hide resolved
@jonatack
Copy link
Contributor Author

jonatack commented Nov 4, 2019 •
edited

@MarcoFalke, thank you for reviewing. I empathise with your remarks and understand the increased review burden and issues of churn/git blame/git history. Will minimise the changeset in the future.

@jonatack jonatack force-pushed the wallet_avoidreuse-test-improvements branch from d2edf02 to 2b04c7c Compare November 6, 2019 11:11
@jonatack
test: add logging to wallet_avoidreuse.py
6d50b26
@jonatack
test: speed up wallet_avoidreuse.py
0e7c90e 
Use -whitelist to speed up transaction relay.

The wallet_avoidreuse.py test is not intended to test transaction relay/timing,
so it should be fine to do this here.

This greatly reduces test run time variability and speeds up the test by 2-3
times on average, e.g. on my system from 20-30 seconds down to 8-10 seconds.
@jonatack jonatack force-pushed the wallet_avoidreuse-test-improvements branch from 2b04c7c to 0e7c90e Compare November 7, 2019 09:04
jonatack
jonatack commented Nov 7, 2019
View reviewed changes
Copy link
Contributor Author

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Rebased to minimal changeset, no code changes except the following:

-self.extra_args = [["-whitelist=127.0.0.1"]] * 2
+self.extra_args = [["-whitelist=127.0.0.1"]] * self.num_nodes

@maflcko
Copy link
Member

maflcko commented Nov 7, 2019

Thanks. This patch is a lot more candy to my eyes than the previous one.

@maflcko
Copy link
Member

maflcko commented Nov 7, 2019

ACK 0e7c90e 🐊

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 0e7c90eb37a687158c261ddd1ff9f1028a1e7012 🐊
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjgfgv/c+HAPiENUjhlN/9JBHfsmMEootFcApeIeuXMOontskood9TPTOARVNt+
jOMDycGM8zEcJh+Fb/Oe6UyI82rUKtKfP6bGGxbhZKRGV+qBIQLOijcu+HyFdbKO
WC6Xg7L8l9v0DllSkWuDx13kgGMUmSLjLGLEptguPobmeSDrHdL/0/HZH35aH7zA
1lzEly99JabUDJckc78G1XYXHWXKImfGxsZHmDwZbGqTJaygEiCa0Psab4iI4W17
VUcaRU/kGCA5owhy6n4tYeGo6/bi4oZ5ImPJ9NkhcNAQJyjV18Rt9y1h2B5T1hXE
CeqXvEvfcXMsrKk1cG9cyNY/V6DnR0zp8jEdDu8BO4LHI9Kx5DmVVFS0p5Icpmgj
w0kD2fGy0KDPDECauFhDB0VTL1x1Fh3iet+QXeW/G4fd6/4FEGYoIUWLtwN9hVwT
M4f50hLAjTyr7qKtebkirMXH2u3dhIwXosLsxK8o+QdyB8YnkMl2mHBre18bgLxU
1pv7IBZ7
=u/gW
-----END PGP SIGNATURE-----

Timestamp of file with hash 6cefb31533317d8b2a2f9fe6e2a75f803e92835ac7b9cf4100482a5a252a11c1 -

fanquake
fanquake approved these changes Nov 7, 2019
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 0e7c90e

master (772673d):

time test/functional/test_runner.py wallet_avoidreuse.py
wallet_avoidreuse.py | ✓ Passed  | 38 s
wallet_avoidreuse.py | ✓ Passed  | 14 s
wallet_avoidreuse.py | ✓ Passed  | 27 s
wallet_avoidreuse.py | ✓ Passed  | 22 s

This PR (0e7c90e):

time test/functional/test_runner.py wallet_avoidreuse.py
wallet_avoidreuse.py | ✓ Passed  | 6 s
wallet_avoidreuse.py | ✓ Passed  | 6 s
wallet_avoidreuse.py | ✓ Passed  | 6 s
wallet_avoidreuse.py | ✓ Passed  | 6 s

@jonatack
Copy link
Contributor Author

jonatack commented Nov 7, 2019

Thanks. This patch is a lot more candy to my eyes than the previous one.

I agree -- sorry about that brainfart.

fanquake added a commit that referenced this pull request Nov 7, 2019
@fanquake
Merge #17362: test: speed up wallet_avoidreuse, add logging
2706162 
0e7c90e test: speed up wallet_avoidreuse.py (Jon Atack)
6d50b26 test: add logging to wallet_avoidreuse.py (Jon Atack)

Pull request description:

  Inspired by PRs #17340 and #15881.

  - add logging
  - pass -whitelist in `set_test_params` to speed up transaction relay

  `wallet_avoidreuse.py` is not intended to test P2P transaction relay/timing, so it should be fine to do this here. This reduces test run time variability and speeds up the test by 2-3 times on average.

  Test run times in seconds:

  - before: 20, 24, 22, 17, 27, 40, 30

  - after: 10, 10, 8, 9, 10, 7, 8

ACKs for top commit:
  MarcoFalke:
    ACK 0e7c90e 🐊
  fanquake:
    ACK 0e7c90e

Tree-SHA512: 6d954a0aaf402c9594201626b59d29263479059e68fa5155bb44ed973cd0c3347729dd78b78b4d5a2275e45da365dc1afb4cc7e3293dea33fcc2e3e83a39faf5
@fanquake fanquake merged commit 0e7c90e into bitcoin:master Nov 7, 2019
@jonatack jonatack deleted the wallet_avoidreuse-test-improvements branch November 7, 2019 17:15
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 15, 2020
@fanquake @majcosta
[backport#17362] test: speed up wallet_avoidreuse, add logging
bf0471f 
Summary:
0e7c90eb37a687158c261ddd1ff9f1028a1e7012 test: speed up wallet_avoidreuse.py (Jon Atack)
6d50b2606ea9249627556051637080c3587b1b04 test: add logging to wallet_avoidreuse.py (Jon Atack)

Pull request description:

  Inspired by PRs #17340 and #15881.

  - add logging
  - pass -whitelist in `set_test_params` to speed up transaction relay

  `wallet_avoidreuse.py` is not intended to test P2P transaction relay/timing, so it should be fine to do this here. This reduces test run time variability and speeds up the test by 2-3 times on average.

  Test run times in seconds:

  - before: 20, 24, 22, 17, 27, 40, 30

  - after: 10, 10, 8, 9, 10, 7, 8

---

Backport of Core [[bitcoin/bitcoin#17362 | PR17362]]

Test Plan:
  test_runner.py wallet_avoidreuse

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6588
@theStack theStack mentioned this pull request Jul 22, 2020
Merged
maflcko pushed a commit that referenced this pull request Aug 16, 2020
Merge #19564: test: p2p_feefilter improvements (logging, refactoring,…
a57af89 
… 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](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. #17121, #17124, #17340, #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 9e78943
  jonatack:
    re-ACK 9e78943 per `git range-diff 3ab2582 ea74a3c 9e78943`

Tree-SHA512: fe21c1c5413df9165fea916b5d5f609d3ba33e7b5c3364b38eb824fcc55d9e6abddf27116cbc0b325913d451a73c44542040fb916aec9c46f805c6e12f6f10cf
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 16, 2020
Merge bitcoin#19564: test: p2p_feefilter improvements (logging, refac…
9f7e4e2 
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift left review comments

@jachiang jachiang jachiang requested changes

@Sjors Sjors Sjors approved these changes

@fanquake fanquake fanquake approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jonatack @maflcko @Sjors @fanquake @practicalswift @jachiang @DrahtBot