There may be a theoretical deadlock for the duration of the offset when the system clock is adjusted into a past time while executing SeedStrengthen.
Fix this by using steady clock.
Do the same in FindBestImplementation, which shouldn’t be affected, because it discards outlier measurements. However, doing the same there for consistency seems fine.
Do the same in FlushStateToDisk, which should make the flushes more steady, if the system clock is adjusted by a large offset.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | john-moffett, willcl-ark |
| Concept ACK | sipa |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Concept ACK for moving to monotonic clocks.
Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is additional hardening for a user whose system clock happens to be changed while one of these is executing?
I couldn’t easily find any callsites which might be directly affected by this, only indirectly. i.e. The system clock happens to change during a call to GetRandHash() or GetRandBytes() (or similar) resulting in a long or unbounded deadlock whilse the strengthening is taking place.
The only remaining use of GetTimeMicros is now in src/logging.cpp which seems nice.
Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is additional hardening for a user whose system clock happens to be changed while one of these is executing?
yes, I don’t think the system clock is ever adjusted in tests. Usually, we’d use NodeClock for a mockable clock. faketime may be a way to mock the system clock in tests, but faketime may not be available on Windows.
About the user-facing changes, this should only be an improvement:
RandAddPeriodic is called in the scheduler thread, so it blocking for a long time may also block shutdown, IBD, and other stuff?FindBestImplementation shouldn’t have any observable change, as explained in OPFlushStateToDisk should not have any observable change, unless the node crashes, I guess?
ACK fa1b4e5c3294fc9aec033892a4a4d7b5cfc015aa
Good conceptual change, as there’s no reason for (eg) an NTP adjustment to affect these things.
