util: Use steady clock instead of system clock to measure durations #27405

1. 
   maflcko commented at 12:47 pm on April 3, 2023: member

   GetTimeMillis has multiple issues:

   • It doesn’t denote the underlying clock type
   • It isn’t type-safe
   • It is used incorrectly in places that should use a steady clock

   Fix all issues here.

2. 
   DrahtBot commented at 12:47 pm on April 3, 2023: contributor

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

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	willcl-ark, martinus
   Concept ACK	dergoegge, fanquake, RandyMcMillan

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

3. 
   dergoegge commented at 12:48 pm on April 3, 2023: member

   Concept ACK
4. maflcko renamed this:
   Use steady clock instead of system clock to measure durations
   util: Use steady clock instead of system clock to measure durations
   on Apr 3, 2023
5. DrahtBot added the label Utils/log/libs on Apr 3, 2023
6. maflcko force-pushed on Apr 3, 2023
7. in src/netbase.cpp:40 in fafee71641 outdated

   35@@ -36,8 +36,8 @@ static Proxy nameProxy GUARDED_BY(g_proxyinfo_mutex);
   36 int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT;
   37 bool fNameLookup = DEFAULT_NAME_LOOKUP;
   38 
   39-// Need ample time for negotiation for very slow proxies such as Tor (milliseconds)
   40-int g_socks5_recv_timeout = 20 * 1000;
   41+// Need ample time for negotiation for very slow proxies such as Tor
   42+std::chrono::milliseconds g_socks5_recv_timeout{20};
   

   ---

   ---

   

   martinus commented at 5:58 pm on April 3, 2023:

   I think this should be 20s. I guess in this case it might be safer to write

   0std::chrono::milliseconds g_socks5_recv_timeout = 20s;
   

   because just

   0std::chrono::milliseconds g_socks5_recv_timeout = 20;
   

   would give you a compile error

   ---

   

   maflcko commented at 10:35 am on April 4, 2023:

   Thanks, fixed
8. 
   martinus commented at 6:10 pm on April 3, 2023: contributor

   Concept ACK
9. 
   fanquake commented at 9:50 am on April 4, 2023: member

   Concept ACK
10. net: Use steady clock in InterruptibleRecv fa454dcb20
11. test: Use steady clock in index tests fa1d8044ab
12. qt: Use steady clock to throttle GUI notifications fa97621804
13. wallet: Use steady clock to measure scanning duration fa2c099cec
14. wallet: Use steady clock to calculate number of derive iterations fa83fb3161
15. maflcko force-pushed on Apr 4, 2023
16. fanquake requested review from willcl-ark on May 4, 2023
17. 
    willcl-ark commented at 9:42 am on May 5, 2023: member

    ACK fa83fb3161

    This is a nice cleanup to the clocks.

    There are a few time values left as other types in the codebase, but it doesn’t make sense to change them as they are being used as those types by other components (mainly libevent it seems).

18. DrahtBot removed review request from willcl-ark on May 5, 2023
19. 
    RandyMcMillan commented at 1:13 pm on May 5, 2023: contributor

    Concept ACK
20. fanquake requested review from martinus on May 5, 2023
21. DrahtBot removed review request from martinus on May 5, 2023
22. martinus approved
23. 
    martinus commented at 6:32 pm on May 5, 2023: contributor

    Code review ACK https://github.com/bitcoin/bitcoin/commit/fa83fb31619c19a1a30b4181486601a944941b16, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok.
24. fanquake merged this on May 6, 2023
25. fanquake closed this on May 6, 2023

    ---

26. sidhujag referenced this in commit 6f8d674851 on May 7, 2023
27. maflcko deleted the branch on May 8, 2023
28. kwvg referenced this in commit 8d26315415 on Jul 19, 2023
29. kwvg referenced this in commit 8e029497a3 on Jul 19, 2023
30. kwvg referenced this in commit 01511fb681 on Jul 23, 2023
31. kwvg referenced this in commit fbe7679038 on Jul 23, 2023
32. kwvg referenced this in commit bca200ef58 on Jul 24, 2023
33. kwvg referenced this in commit d1d82b9dfc on Jul 28, 2023
34. kwvg referenced this in commit 4652f91b2c on Jul 28, 2023
35. kwvg referenced this in commit c336e4a289 on Jul 30, 2023
36. knst referenced this in commit 6bf586a20c on Aug 1, 2023
37. knst referenced this in commit 560400b6be on Aug 1, 2023
38. kwvg referenced this in commit d06ce9d114 on Aug 1, 2023
39. kwvg referenced this in commit 5c4c4af4b9 on Aug 1, 2023
40. kwvg referenced this in commit b5dd43b606 on Aug 1, 2023
41. PastaPastaPasta referenced this in commit a9e3b9f2db on Aug 2, 2023
42. PastaPastaPasta referenced this in commit 0b8e501b51 on Aug 2, 2023
43. bitcoin locked this on May 7, 2024

Contributors
maflcko DrahtBot dergoegge martinus fanquake willcl-ark RandyMcMillan

Labels
Utils/log/libs