p2p: Use mocktime for ping timeout #23218
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Nice! Concept ACK. |
| @@ -4312,8 +4312,11 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers() | |||
|
|
|||
| void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::microseconds now) | |||
| { | |||
| if (m_connman.ShouldRunInactivityChecks(node_to) && peer.m_ping_nonce_sent && | |||
| if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now).count()) && | |||
There was a problem hiding this comment.
Also nice simplification that this simply passes through now instead of indirectly querying GetTimeSeconds deep in the call stack.
|
Is the longer term idea here to get rid of unmockable 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. |
|
Yes, some more are handled in #19499, which is not ready for review yet. |
|
Concept ACK |
|
Code review ACK fadf118 |
Summary: > It is slightly confusing to use mocktime for some times, but not others. > > Start fixing that by making the ping timeout use mocktime. This is a backport of [[bitcoin/bitcoin#23218 | core#23218]] Depends on D10956 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10957
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 6d76b57. 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_pingfunctional test now runs 4 seconds faster.