util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk #27189

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2303-bench-with-steady-clock-🙂 changing 6 files +20 −18
  1. maflcko commented at 12:45 pm on March 2, 2023: member

    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.

  2. DrahtBot commented at 12:45 pm on March 2, 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 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.

    Conflicts

    No conflicts as of last run.

  3. maflcko renamed this:
    Use steady clock in SeedStrengthen and FindBestImplementation
    util: Use steady clock in SeedStrengthen and FindBestImplementation
    on Mar 2, 2023
  4. DrahtBot added the label Utils/log/libs on Mar 2, 2023
  5. Use steady clock in SeedStrengthen and FindBestImplementation 1111e2f8b4
  6. maflcko force-pushed on Mar 2, 2023
  7. Use steady clock in FlushStateToDisk fa1b4e5c32
  8. maflcko renamed this:
    util: Use steady clock in SeedStrengthen and FindBestImplementation
    util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk
    on Mar 2, 2023
  9. fanquake requested review from stickies-v on Mar 4, 2023
  10. fanquake requested review from john-moffett on Mar 4, 2023
  11. fanquake requested review from willcl-ark on Mar 4, 2023
  12. willcl-ark commented at 12:57 pm on March 6, 2023: contributor

    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.

  13. maflcko commented at 1:21 pm on March 6, 2023: member

    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 OP
    • FlushStateToDisk should not have any observable change, unless the node crashes, I guess?
  14. john-moffett approved
  15. john-moffett commented at 7:30 pm on March 6, 2023: contributor

    ACK fa1b4e5c3294fc9aec033892a4a4d7b5cfc015aa

    Good conceptual change, as there’s no reason for (eg) an NTP adjustment to affect these things.

  16. willcl-ark approved
  17. willcl-ark commented at 3:02 pm on March 7, 2023: contributor
    ACK fa1b4e5c3
  18. sipa commented at 3:21 pm on March 7, 2023: member
    Concept ACK
  19. fanquake merged this on Mar 8, 2023
  20. fanquake closed this on Mar 8, 2023

  21. maflcko deleted the branch on Mar 8, 2023
  22. sidhujag referenced this in commit 24cce59f04 on Mar 8, 2023
  23. bitcoin locked this on Mar 7, 2024

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: 2024-10-31 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me