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

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2007-p2pMockTimeout changing 12 files +66 −58
  1. MarcoFalke commented at 2:25 PM on July 12, 2020: member

    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

  2. MarcoFalke added the label P2P on Jul 12, 2020
  3. MarcoFalke added the label Refactoring on Jul 12, 2020
  4. MarcoFalke force-pushed on Jul 12, 2020
  5. DrahtBot commented at 5:13 PM on July 12, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  6. DrahtBot cross-referenced this on Jul 12, 2020 from issue Use std::chrono for the time to rotate destination of addr messages + tests by naumenkogs
  7. practicalswift commented at 11:07 PM on July 13, 2020: contributor

    Concept ACK: mockable is good

  8. DrahtBot cross-referenced this on Jul 28, 2020 from issue Remove the automatic creation and loading of the default wallet by achow101
  9. luke-jr commented at 10:21 PM on July 29, 2020: member

    Doesn't this defeat the purpose of the test?

  10. DrahtBot cross-referenced this on Aug 20, 2020 from issue net: don't try to relay to the address' originator by vasild
  11. DrahtBot cross-referenced this on Aug 20, 2020 from issue net, rpc: expose nLastBlockTime/nLastTXTime as last block/last_transaction in getpeerinfo by jonatack
  12. DrahtBot added the label Needs rebase on Aug 24, 2020
  13. jnewbery commented at 1:08 PM on December 10, 2020: member

    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.

  14. MarcoFalke force-pushed on Sep 29, 2021
  15. DrahtBot removed the label Needs rebase on Sep 29, 2021
  16. DrahtBot cross-referenced this on Sep 29, 2021 from issue net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery
  17. DrahtBot cross-referenced this on Sep 30, 2021 from issue net/net processing: Move tx inventory into net_processing by jnewbery
  18. DrahtBot cross-referenced this on Sep 30, 2021 from issue tests: Run both descriptor and legacy tests within a single test invocation by achow101
  19. DrahtBot cross-referenced this on Oct 7, 2021 from issue p2p: Use mocktime for ping timeout by MarcoFalke
  20. DrahtBot added the label Needs rebase on Oct 21, 2021
  21. MarcoFalke force-pushed on Oct 22, 2021
  22. MarcoFalke force-pushed on Oct 22, 2021
  23. MarcoFalke force-pushed on Oct 22, 2021
  24. MarcoFalke commented at 9:02 AM on October 22, 2021: member

    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.

  25. MarcoFalke commented at 9:03 AM on October 22, 2021: member

    Doesn't this defeat the purpose of the test?

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

  26. DrahtBot removed the label Needs rebase on Oct 22, 2021
  27. MarcoFalke force-pushed on Nov 2, 2021
  28. MarcoFalke added this to the milestone 23.0 on Nov 17, 2021
  29. MarcoFalke removed this from the milestone 23.0 on Nov 17, 2021
  30. scripted-diff: Rename m_last_send and m_last_recv
    -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-
    fa6d5a238d
  31. MarcoFalke force-pushed on Nov 17, 2021
  32. laanwj added this to the "Blockers" column in a project
  33. in src/net.cpp:1335 in faf36165fd outdated 
    1337 |  
1338 |      if (!ShouldRunInactivityChecks(node, now)) return false;
1339 |  
1340 | -    if (node.m_last_recv == 0 || node.m_last_send == 0) {
1341 | -        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());
1342 | +    if (last_recv.count() == 0 || last_send.count() == 0) {
    mzumsande commented at 10:25 PM on December 3, 2021:

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

    MarcoFalke commented at 9:50 AM on December 6, 2021:

    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.

  34. in src/net_processing.cpp:4317 in faf36165fd outdated 
    4311 | @@ -4312,9 +4312,10 @@ 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, std::chrono::duration_cast<std::chrono::seconds>(now).count()) &&
4316 | +    if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now)) &&
4317 |          peer.m_ping_nonce_sent &&
4318 | -        now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
4319 | +        now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL})
    mzumsande commented at 11:29 PM on December 3, 2021: 
            now > peer.m_ping_start.load() + TIMEOUT_INTERVAL)

    TIMEOUT_INTERVAL is of type std::chrono::minutes now.

    MarcoFalke commented at 9:51 AM on December 6, 2021:

    Nice, fixed.

  35. in src/net.cpp:1327 in faf36165fd outdated 
    1327 |  bool CConnman::InactivityCheck(const CNode& node) const
1328 |  {
1329 | -    // Use non-mockable system time (otherwise these timers will pop when we
1330 | -    // use setmocktime in the tests).
1331 | -    int64_t now = GetTimeSeconds();
1332 | +    // Test that see disconnects after using mocktime can start with a large
    mzumsande commented at 11:56 PM on December 3, 2021:

    nit: Tests

    MarcoFalke commented at 9:51 AM on December 6, 2021:

    thx, fixed

  36. mzumsande commented at 12:15 AM on December 4, 2021: contributor

    Concept ACK

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

  37. p2p: Make timeout mockable and type safe, speed up test fadc0c80ae
  38. MarcoFalke force-pushed on Dec 6, 2021
  39. in src/net_processing.cpp:4318 in fadc0c80ae 
    4311 | @@ -4312,9 +4312,10 @@ 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, std::chrono::duration_cast<std::chrono::seconds>(now).count()) &&
4316 | +    if (m_connman.ShouldRunInactivityChecks(node_to, std::chrono::duration_cast<std::chrono::seconds>(now)) &&
4317 |          peer.m_ping_nonce_sent &&
4318 | -        now > peer.m_ping_start.load() + std::chrono::seconds{TIMEOUT_INTERVAL}) {
4319 | +        now > peer.m_ping_start.load() + TIMEOUT_INTERVAL)
4320 | +    {
    laanwj commented at 5:20 PM on December 9, 2021:

    More consistent to keep the { on the same line?

    MarcoFalke commented at 5:47 PM on December 9, 2021:

    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.

    laanwj commented at 6:19 PM on December 9, 2021:

    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.

  40. laanwj commented at 6:21 PM on December 9, 2021: member

    Code review ACK fadc0c80ae4e526fb2b503f7cc02f6122aaf1de5

  41. naumenkogs commented at 8:49 AM on December 10, 2021: member

    ACK fadc0c80ae4e526fb2b503f7cc02f6122aaf1de5

  42. MarcoFalke merged this on Dec 10, 2021
  43. MarcoFalke closed this on Dec 10, 2021
  44. MarcoFalke deleted the branch on Dec 10, 2021
  45. sidhujag referenced this in commit f7136df5b2 on Dec 10, 2021
  46. jonatack cross-referenced this on Dec 11, 2021 from issue Issue in p2p_timeouts: no_send_node.wait_for_disconnect(timeout=1) by jonatack
  47. fanquake removed this from the "Blockers" column in a project
  48. RandyMcMillan referenced this in commit 3f2e8f8b86 on Dec 23, 2021
  49. Fabcien referenced this in commit 5070d1e559 on Feb 2, 2022
  50. bitcoin locked this on Dec 10, 2022
Contributors
MarcoFalkeDrahtBotpracticalswiftluke-jrjnewberymzumsandelaanwjnaumenkogs
Labels
RefactoringP2P
Linked (view graph)
#20654 qa: AssertionError in p2p_timeouts.py#21735 style: How to format multi-line if condition with clang-format?#23739 Issue in p2p_timeouts: no_send_node.wait_for_disconnect(timeout=1)
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC