p2p: Use mocktime for ping timeout #23218

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2110-mockPingTime changing 6 files +25 −7

MarcoFalke commented at 11:30 AM on October 7, 2021: member

It is slightly confusing to use mocktime for some times, but not others.

Start fixing that by making the ping timeout use mocktime.

The only downside would be that tests that use mocktime disconnect peers after this patch. However, I don't think this is an issue, as the inactivity check is already disabled for all functional tests after commit 6d76b57ca0cdf6f9c19ce065b9a4a628930a78b5. Only one unit test needed the inactivity check disabled as part of this patch.

A nice side effect of this patch is that the p2p_ping functional test now runs 4 seconds faster.
p2p: Use mocktime for ping timeout fadf1186c8
MarcoFalke added the label Tests on Oct 7, 2021
DrahtBot commented at 5:23 PM on October 7, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #19499 (p2p: Make timeout mockable and type safe, speed up test by MarcoFalke)
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.
laanwj commented at 1:52 PM on October 13, 2021: member

A nice side effect of this patch is that the p2p_ping functional test now runs 4 seconds faster.

Nice! Concept ACK.

in src/net_processing.cpp:4315 in fadf1186c8

4311 | @@ -4312,8 +4312,11 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
4312 |  
4313 |  void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now)
4314 |  {
4315 | -    if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent &&
4316 | +    if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now).count()) &&

laanwj commented at 1:55 PM on October 13, 2021:

Also nice simplification that this simply passes through now instead of indirectly querying GetTimeSeconds deep in the call stack.

laanwj commented at 1:59 PM on October 13, 2021: member

Is the longer term idea here to get rid of unmockable GetTimeSeconds completely? It looks like the only uses after this are

src/bitcoin-cli.cpp:    const int64_t m_time_now{GetTimeSeconds()};
src/net.cpp:            node.nLastSend = GetTimeSeconds();
src/net.cpp:    int64_t now = GetTimeSeconds();
src/net.cpp:    : nTimeConnected(GetTimeSeconds()),
src/qt/rpcconsole.cpp:    const int64_t time_now{GetTimeSeconds()};

The ones in bitcoin-cli and rpcconsole are uncontroversial to replace.

laanwj added the label P2P on Oct 13, 2021
MarcoFalke commented at 4:12 PM on October 15, 2021: member

Yes, some more are handled in #19499, which is not ready for review yet.
naumenkogs commented at 12:17 PM on October 21, 2021: member

Concept ACK
laanwj commented at 5:32 PM on October 21, 2021: member

Code review ACK fadf1186c899f45787a91c28120b0608bdc6c246
laanwj merged this on Oct 21, 2021
laanwj closed this on Oct 21, 2021
MarcoFalke deleted the branch on Oct 21, 2021
sidhujag referenced this in commit 417f64c492 on Oct 21, 2021
Fabcien referenced this in commit ab192b668f on Feb 2, 2022
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

p2p: Use mocktime for ping timeout #23218

Conflicts