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.

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

P2P: Make peer timeout configurable, speed up very slow test and ensure correct code path tested. #14733

Merged
merged 2 commits into from Dec 4, 2018

Conversation

Projects
None yet
8 participants
@zallarak
Copy link
Contributor

commented Nov 15, 2018

Summary:

  1. Primary: Adds a debug_only=true flag for peertimeout, defaults to 60 sec., the current hard-coded setting.
  2. Secondary: Drastically speeds up p2p_timeout.py test.
  3. Secondary: Tests that the correct code path is being tested by adding log assertions to the test.

Rationale:

  • P2P timeout was hard-coded: make it explicitly specified and configurable, instead of a magic number.
  • Addresses #13518; p2p_timeout.py takes 4 sec. to run instead of 61 sec.
  • Makes p2p_timeout.py more explicit. Previously, we relied on a comment to inform us of the timeout amount being tested. Now it is specified directly in the test via passing in the new arg; -peertimeout=3.
  • Opens us up to testing more P2P scenarios; oftentimes slow tests are the reason we don't test.

Locally verified changes:

With Proposed Change (4.7 sec.):

$ time ./test/functional/p2p_timeouts.py
2018-11-19T00:04:19.077000Z TestFramework (INFO): Initializing test directory /tmp/testhja7g2n7
2018-11-19T00:04:23.479000Z TestFramework (INFO): Stopping nodes
2018-11-19T00:04:23.683000Z TestFramework (INFO): Cleaning up /tmp/testhja7g2n7 on exit
2018-11-19T00:04:23.683000Z TestFramework (INFO): Tests successful

real    0m4.743s

Currently on master (62.8 sec.):

$ time ./test/functional/p2p_timeouts.py
2018-11-19T00:06:10.948000Z TestFramework (INFO): Initializing test directory /tmp/test6mo6k21h
2018-11-19T00:07:13.376000Z TestFramework (INFO): Stopping nodes
2018-11-19T00:07:13.631000Z TestFramework (INFO): Cleaning up /tmp/test6mo6k21h on exit
2018-11-19T00:07:13.631000Z TestFramework (INFO): Tests successful

real    1m2.836s

Error message demonstrated for new argument -peertimeout:

$ ./bitcoind -peertimeout=-5
...
Error: peertimeout cannot be configured with a negative value.

@fanquake fanquake added the P2P label Nov 15, 2018

Show resolved Hide resolved doc/man/bitcoin-qt.1 Outdated
Show resolved Hide resolved src/netbase.cpp Outdated
Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/netbase.h Outdated

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch Nov 16, 2018

@zallarak

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

net.cpp|h was much more relevant than netbase.cpp|h - moved.

I didn't move it into CNode because it didn't seem right to duplicate the timeout information into every peer. CConnman isn't a parameter to InactivityCheck, so having it as a top-level setting seemed to make most sense for now.

Thanks for your prompt comments @MarcoFalke - looking forward to any further thoughts!

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

CConnman isn't a parameter to InactivityCheck

conman is passed as this or self if you prefer python.

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch Nov 16, 2018

@zallarak

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@MarcoFalke great call. I have updated with this per your feedback. Please let me know what you think of the change.

  • Pass in nPeerConnectTimeout through connOptions and add it to CConnman
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14856 ([WIP] net: remove more CConnman globals (theuni) by dongcarl)

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.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 16, 2018

This looks good, although please use snake case even though the surrounding code doesn't :) Also please squash the nit commit

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch Nov 16, 2018

@zallarak

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch Nov 16, 2018

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

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch 2 times, most recently Nov 17, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

utACK 2c7fbed

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch Nov 18, 2018

@zallarak zallarak changed the title p2p: allow p2ptimeout to be configurable, speed up slow test P2P: allow peer timeout to be configurable Nov 19, 2018

@zallarak zallarak changed the title P2P: allow peer timeout to be configurable P2P: Allow peer timeout to be configurable and speed up very slow test. Nov 19, 2018

Show resolved Hide resolved src/net.h Outdated
Show resolved Hide resolved src/net.cpp Outdated
@dongcarl
Copy link
Contributor

left a comment

Question for other reviewers: I'm wondering what the best types are for peer_connect_timeout, nTime, and nTimeConnected.

I think that while it is logical for these things to be unsigned, and the conversion are largely safe given the current codebase, there are a few things to consider:

  1. Other timeout options (such as -timeout) are signed.
  2. There isn't an unsigned variant of GetArg() or GetSystemTimeInSeconds(), meaning that we have to cast/implicitly convert.
  3. If we're trying to make sure that peer_connect_timeout can't have invalid values by making it unsigned, it can still be 0, which is not valid.

Perhaps what should be done is to leave the types alone until we use something like std::chrono.

Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/net.cpp Outdated
Show resolved Hide resolved src/net.h Outdated
@MarcoFalke
Copy link
Member

left a comment

I reviewed this yesterday, but didn't submit. Turns out most of my comments were already raised, but I am dropping this nonetheless.

Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/net.h Outdated
Show resolved Hide resolved src/net.h Outdated
Show resolved Hide resolved src/net.h Outdated
Show resolved Hide resolved src/net.cpp
Show resolved Hide resolved test/functional/p2p_timeouts.py Outdated
@dongcarl

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2018

Overall, I'm very excited that the test that's changed here will be sped up quite a bit! 😄

@zallarak zallarak force-pushed the zallarak:p2ptimeout branch to 8042bbf Nov 29, 2018

@zallarak

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@dongcarl @MarcoFalke - I have resolved every comment you left:

  • Add log assertions to the test to ensure the right code path is being hit -- this greatly improves the tests!
  • Make time values consistent with the rest of the codebase (signed)
  • Made cli helptext more clear
  • Fix log messages that had 60 seconds hard-coded
  • prefix class members with m_

Really appreciate the great comments.

Let me know how things look from here!

@dongcarl

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2018

tACK 8042bbf

@zallarak zallarak changed the title P2P: Allow peer timeout to be configurable and speed up very slow test. P2P: Make peer timeout configurable. Speeds up very slow test and ensures correct code path is tested. Nov 29, 2018

@zallarak zallarak changed the title P2P: Make peer timeout configurable. Speeds up very slow test and ensures correct code path is tested. P2P: Make peer timeout configurable, speed up very slow test and ensure correct code path tested. Nov 29, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

utACK 8042bbf

@MarcoFalke
Copy link
Member

left a comment

Just two minor nits I noticed, feel free to fix them in a separate commit.

Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/init.cpp Outdated
@zallarak

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2018

@MarcoFalke do you mean make a new pull request, or should I add the nit commits to this one? Thank you!

EDIT: I will add the commit here, but not squash it so that the review is preserved. Let me know if I should do otherwise.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

utACK 48b37db

@meshcollider

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

re-utACK 48b37db

@dongcarl
Copy link
Contributor

left a comment

utACK 48b37db

@MarcoFalke MarcoFalke added this to the 0.18.0 milestone Dec 3, 2018

@laanwj laanwj merged commit 48b37db into bitcoin:master Dec 4, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 4, 2018

Merge #14733: P2P: Make peer timeout configurable, speed up very slow…
… test and ensure correct code path tested.

48b37db make peertimeout a debug argument, remove error message translation (Zain Iqbal Allarakhia)
8042bbf p2p: allow p2ptimeout to be configurable, speed up slow test (Zain Iqbal Allarakhia)

Pull request description:

  **Summary:**

  1. _Primary_: Adds a `debug_only=true` flag for peertimeout, defaults to 60 sec., the current hard-coded setting.
  2. _Secondary_: Drastically speeds up `p2p_timeout.py` test.
  3. _Secondary_: Tests that the correct code path is being tested by adding log assertions to the test.

  **Rationale:**

  - P2P timeout was hard-coded: make it explicitly specified and configurable, instead of a magic number.
  - Addresses #13518; `p2p_timeout.py` takes 4 sec. to run instead of 61 sec.
  - Makes `p2p_timeout.py` more explicit. Previously, we relied on a comment to inform us of the timeout amount being tested. Now it is specified directly in the test via passing in the new arg; `-peertimeout=3`.
  - Opens us up to testing more P2P scenarios; oftentimes slow tests are the reason we don't test.

  **Locally verified changes:**

  _With Proposed Change (4.7 sec.):_
  ```
  $ time ./test/functional/p2p_timeouts.py
  2018-11-19T00:04:19.077000Z TestFramework (INFO): Initializing test directory /tmp/testhja7g2n7
  2018-11-19T00:04:23.479000Z TestFramework (INFO): Stopping nodes
  2018-11-19T00:04:23.683000Z TestFramework (INFO): Cleaning up /tmp/testhja7g2n7 on exit
  2018-11-19T00:04:23.683000Z TestFramework (INFO): Tests successful

  real    0m4.743s
  ```

  _Currently  on master (62.8 sec.):_
  ```
  $ time ./test/functional/p2p_timeouts.py
  2018-11-19T00:06:10.948000Z TestFramework (INFO): Initializing test directory /tmp/test6mo6k21h
  2018-11-19T00:07:13.376000Z TestFramework (INFO): Stopping nodes
  2018-11-19T00:07:13.631000Z TestFramework (INFO): Cleaning up /tmp/test6mo6k21h on exit
  2018-11-19T00:07:13.631000Z TestFramework (INFO): Tests successful

  real    1m2.836s
  ```

  _Error message demonstrated for new argument `-peertimeout`:_
  ```
  $ ./bitcoind -peertimeout=-5
  ...
  Error: peertimeout cannot be configured with a negative value.
  ```

Tree-SHA512: ff7a244ebea54c4059407bf4fb86465714e6a79cef5d2bcaa22cfe831a81761aaf597ba4d5172fc2ec12266f54712216fc41b5d24849e5d9dab39ba6f09e3a2a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.