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

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


    if (!ShouldRunInactivityChecks(node, now)) return false;

-    if (node.m_last_recv == 0 || node.m_last_send == 0) {
-        LogPrint(BCLog::NET, "socket no message in first %i seconds, %d %d peer=%d\n", m_peer_connect_timeout, node.m_last_recv != 0, node.m_last_send != 0, node.GetId());
+    if (last_recv.count() == 0 || last_send.count() == 0) {


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

laanwj added this to Blockers in High-priority for review Dec 2, 2021

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

fanquake removed this from Blockers in High-priority for review Dec 11, 2021

bitcoin locked and limited conversation to collaborators Dec 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

maflcko commented Jul 12, 2020 •

edited

DrahtBot commented Jul 12, 2020 •

edited

practicalswift commented Jul 13, 2020

luke-jr commented Jul 29, 2020

jnewbery commented Dec 10, 2020

maflcko commented Oct 22, 2021

maflcko commented Oct 22, 2021

mzumsande left a comment

mzumsande Dec 3, 2021

maflcko Dec 6, 2021

laanwj Dec 9, 2021

maflcko Dec 9, 2021

laanwj Dec 9, 2021

laanwj commented Dec 9, 2021

naumenkogs commented Dec 10, 2021

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

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

Conversation

maflcko commented Jul 12, 2020 • edited

DrahtBot commented Jul 12, 2020 • edited

Conflicts

practicalswift commented Jul 13, 2020

luke-jr commented Jul 29, 2020

jnewbery commented Dec 10, 2020

maflcko commented Oct 22, 2021

maflcko commented Oct 22, 2021

mzumsande left a comment

Choose a reason for hiding this comment

mzumsande Dec 3, 2021

Choose a reason for hiding this comment

maflcko Dec 6, 2021

Choose a reason for hiding this comment

laanwj Dec 9, 2021

Choose a reason for hiding this comment

maflcko Dec 9, 2021

Choose a reason for hiding this comment

laanwj Dec 9, 2021

Choose a reason for hiding this comment

laanwj commented Dec 9, 2021

naumenkogs commented Dec 10, 2021

maflcko commented Jul 12, 2020 •

edited

DrahtBot commented Jul 12, 2020 •

edited