p2p: Make timeout mockable and type safe, speed up test by maflcko · Pull Request #19499 · bitcoin/bitcoin

maflcko · 2020-07-12T14:25:50Z

Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.

This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836

Fixes #20654

DrahtBot · 2020-07-12T17:13:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#21160 (net/net processing: Move tx inventory into net_processing by jnewbery)

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.

practicalswift · 2020-07-13T23:07:38Z

Concept ACK: mockable is good

luke-jr · 2020-07-29T22:21:02Z

Doesn't this defeat the purpose of the test?

jnewbery · 2020-12-10T13:08:40Z

Concept ACK. There's a lot of refactor/tidyup in this PR. I wonder if that can be split out to make this more focused on the functional changes.

maflcko · 2021-10-22T09:02:21Z

Concept ACK. There's a lot of refactor/tidyup in this PR. I wonder if that can be split out to make this more focused on the functional changes.

Good idea. Split up a scripted-diff as the first commit.

maflcko · 2021-10-22T09:03:10Z

Doesn't this defeat the purpose of the test?

This is a bugfix for the test. Currently it intermittently fails, see OP.

-BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; } ren nLastSend m_last_send ren nLastRecv m_last_recv -END VERIFY SCRIPT-

mzumsande

Concept ACK

Reviewed the non-GUI-related parts so far and played with p2p_timeouts.py - looks good to me, just some nits.

mzumsande · 2021-12-03T22:25:24Z

src/net.cpp

Why sometimes .count() and sometimes count_seconds()?

last_recv is a bit like std::optional. I use .count(), when .has_value() would be used and I use count_seconds(), when .value() would be used.

src/net_processing.cpp

src/net.cpp

laanwj · 2021-12-09T17:20:18Z

src/net_processing.cpp

        peer.m_ping_nonce_sent &&
-        now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
+        now > peer.m_ping_start.load() + TIMEOUT_INTERVAL)
+    {


More consistent to keep the { on the same line?

I think (and other seem to agree, see #21735) that multiline if conditions are hard to read because there is no break between condition and body, so changed it here.

Happy to revert, though.

I don't particularly want to get into discussion about code style, it just seemed inconsistent to me (as all other if cases have the { on the same line), if you have a good reason for it it's fine with me.

laanwj · 2021-12-09T18:21:16Z

Code review ACK fadc0c8

naumenkogs · 2021-12-10T08:49:21Z

ACK fadc0c8

…up test

…, speed up test bc55c30 p2p: Make timeout mockable and type safe, speed up test (MarcoFalke) fdc1c9d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke) Pull request description: Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test. This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836 Fixes #20654 ACKs for top commit: laanwj: Code review ACK bc55c30 naumenkogs: ACK bc55c30 Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64

Summary: ``` -BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; } ren nLastSend m_last_send ren nLastRecv m_last_recv -END VERIFY SCRIPT- ``` This is a backport of [[bitcoin/bitcoin#19499 | core#19499]] [1/2] bitcoin/bitcoin@fa6d5a2 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10954

maflcko added P2P Refactoring labels Jul 12, 2020

maflcko force-pushed the 2007-p2pMockTimeout branch from fa64f6a to fa1bfbe Compare July 12, 2020 14:31

DrahtBot mentioned this pull request Jul 12, 2020

Use std::chrono for the time to rotate destination of addr messages + tests #18642

Merged

DrahtBot mentioned this pull request Jul 28, 2020

Remove the automatic creation and loading of the default wallet #15454

Merged

This was referenced Aug 20, 2020

net: don't try to relay to the address' originator #19763

Merged

net, rpc: expose nLastBlockTime/nLastTXTime as last block/last_transaction in getpeerinfo #19731

Merged

DrahtBot added the Needs rebase label Aug 24, 2020

maflcko force-pushed the 2007-p2pMockTimeout branch from fa1bfbe to 7a80cbf Compare September 29, 2021 08:14

DrahtBot removed the Needs rebase label Sep 29, 2021

This was referenced Sep 29, 2021

net processing: Reduce resource usage for inbound block-relay-only connections #22778

Merged

net/net processing: Move tx inventory into net_processing #21160

Merged

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

DrahtBot mentioned this pull request Oct 7, 2021

p2p: Use mocktime for ping timeout #23218

Merged

DrahtBot added the Needs rebase label Oct 21, 2021

maflcko force-pushed the 2007-p2pMockTimeout branch 3 times, most recently from d7d77a6 to fae77a5 Compare October 22, 2021 08:58

DrahtBot removed the Needs rebase label Oct 22, 2021

maflcko force-pushed the 2007-p2pMockTimeout branch from fae77a5 to fa166da Compare November 2, 2021 13:11

maflcko modified the milestone: 23.0 Nov 17, 2021

scripted-diff: Rename m_last_send and m_last_recv

fa6d5a2

-BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; } ren nLastSend m_last_send ren nLastRecv m_last_recv -END VERIFY SCRIPT-

maflcko force-pushed the 2007-p2pMockTimeout branch from fa166da to faf3616 Compare November 17, 2021 07:46

mzumsande reviewed Dec 4, 2021

View reviewed changes

p2p: Make timeout mockable and type safe, speed up test

fadc0c8

maflcko force-pushed the 2007-p2pMockTimeout branch from faf3616 to fadc0c8 Compare December 6, 2021 09:48

laanwj reviewed Dec 9, 2021

View reviewed changes

maflcko merged commit 9f7661c into bitcoin:master Dec 10, 2021

maflcko deleted the 2007-p2pMockTimeout branch December 10, 2021 09:15

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 10, 2021

Merge bitcoin#19499: p2p: Make timeout mockable and type safe, speed …

f7136df

…up test

jonatack mentioned this pull request Dec 11, 2021

Issue in p2p_timeouts: no_send_node.wait_for_disconnect(timeout=1) #23739

Closed

bitcoin locked and limited conversation to collaborators Dec 10, 2022

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

practicalswift commented Jul 13, 2020

Uh oh!

luke-jr commented Jul 29, 2020

Uh oh!

jnewbery commented Dec 10, 2020

Uh oh!

maflcko commented Oct 22, 2021

Uh oh!

maflcko commented Oct 22, 2021

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

mzumsande Dec 3, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laanwj Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj Dec 9, 2021

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 9, 2021

Uh oh!

naumenkogs commented Dec 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

maflcko commented Jul 12, 2020 •

edited

Loading

DrahtBot commented Jul 12, 2020 •

edited

Loading