net: Consistently use GetTimeMicros() for inactivity checks #9606

1. 
   sdaftuar commented at 9:15 pm on January 20, 2017: member

   Fixes #9586

   The use of mocktime in test logic means that comparisons between GetTime() and GetTimeMicros()/1000000 are unreliable since the former can use mocktime values while the latter always gets the system clock; this changes the networking code’s inactivity checks to consistently use the system clock for inactivity comparisons.

   Also remove some hacks from setmocktime() that are no longer needed, now that we’re using the system clock for nLastSend and nLastRecv.

   Please tag for 0.14.

   ping @theuni

2. fanquake added this to the milestone 0.14.0 on Jan 20, 2017
3. fanquake added the label Refactoring on Jan 20, 2017
4. 
   theuni commented at 7:13 am on January 21, 2017: member

   Concept ACK. This is non-obvious, but I think it’s the way to go for 0.14. Going forward, we really want to use a steady_clock for the net, so we’ll need to rethink timing sources altogether.
5. MarcoFalke added the label P2P on Jan 21, 2017
6. 
   laanwj commented at 6:51 am on January 23, 2017: member

   utACK

   Going forward, we really want to use a steady_clock for the net, so we’ll need to rethink timing sources altogether.

   There too it helps to use a single timing function everywhere, as preparation.

   Also the separation between mock-able and nonmock-able time functions should ideally be made clearer. It’s just weird and arbitrary, that GetTime can be mocked and GetTimeMicroseconds is not. I understand why, but it doesn’t reflect in the names of the functions at least.

7. 
   laanwj commented at 7:10 am on January 23, 2017: member

   As I understand it on a higher level we (should) have multiple times:

   • System wallclock time: GetTimeMillis() GetTimeMicroseconds() Plain gettimeofday. Should be used for logging, and presenting time to the user anywhere. This could be mockable, rolling it into the next one.
   • Mockable time: GetTime(). gettimeofday, but mockable for testing. Maybe used in network for ban expiration and such, so that that can be tested.
   • Consensus time: GetAdjustedTime() =Mockable time with offset based on clocks of other nodes. Used for block handling/validation.
   • Steady time: (Not yet implemented) Monotonic time to be used for timeouts, time differences, benchmarking, etc. Absolute value has no meaning, only differences have.

   One step would be to consistently sort these out. Not based on the unit used (microseconds, milliseconds, seconds) but on the semantic. (not in this pull of course)

8. 
   TheBlueMatt commented at 4:40 pm on January 23, 2017: member

   Technically this breaks qt’s rpcconsole.cpp - it subtracts nLastSend/Recv/nTimeConnected from GetTime(), though I’m not sure it matters.

   Add this to the list of things we need to rethink bottom-up for 0.15, but this seems like a sufficient bugfix for 0.14.

9. 
   morcos commented at 9:33 pm on January 23, 2017: member

   utACK modulo addressing Matt’s comment about rpcconsole.cpp. Trivial to just change to GetTimeMicros()/100000 in those 3 locations.
10. 
    laanwj commented at 9:20 am on January 24, 2017: member

    This is repeating ourselves a lot. Why not… hey, let’s add a function to do GetTimeMicros()/100000 ! :-)
11. 
    sdaftuar commented at 2:20 pm on January 25, 2017: member

    Added a fixup commit to address the bug in qt/rpcconsole.cpp, and added a wrapper for GetTimeMicros()/1000000.
12. net: Consistently use GetTimeMicros() for inactivity checks

    The use of mocktime in test logic means that comparisons between
    GetTime() and GetTimeMicros()/1000000 are unreliable since the former
    can use mocktime values while the latter always gets the system clock;
    this changes the networking code's inactivity checks to consistently
    use the system clock for inactivity comparisons.
    
    Also remove some hacks from setmocktime() that are no longer needed,
    now that we're using the system clock for nLastSend and nLastRecv.

    99464bc38e
13. 
    morcos commented at 3:07 pm on January 25, 2017: member

    utACK 33306c2
14. sdaftuar force-pushed on Jan 25, 2017
15. 
    sdaftuar commented at 3:09 pm on January 25, 2017: member

    Squashed 33306c25c1a2b411dc22418cb194eeababfc4a6f -> 99464bc38e9575ff47f8e33223b252dcea2055e3
16. 
    TheBlueMatt commented at 3:50 pm on January 25, 2017: member

    utACK 99464bc38e9575ff47f8e33223b252dcea2055e3
17. 
    theuni commented at 8:49 pm on January 25, 2017: member

    utACK 99464bc38e9575ff47f8e33223b252dcea2055e3
18. laanwj merged this on Jan 26, 2017
19. laanwj closed this on Jan 26, 2017

    ---

20. laanwj referenced this in commit 3f9f9629cc on Jan 26, 2017
21. DrahtBot locked this on Sep 8, 2021

Contributors
sdaftuar theuni laanwj TheBlueMatt morcos

Labels
Refactoring P2P

Milestone
0.14.0