Skip to content

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

Merged
fanquake merged 2 commits intobitcoin:masterfrom
maflcko:2303-bench-with-steady-clock-🙂
Mar 8, 2023

Hidden character warning

The head ref may contain hidden characters: "2303-bench-with-steady-clock-\ud83d\ude42"
Merged

util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk#27189
fanquake merged 2 commits intobitcoin:masterfrom
maflcko:2303-bench-with-steady-clock-🙂

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 2, 2023 •
edited
Loading

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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2023 •
edited
Loading

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.

@maflcko maflcko changed the title Use steady clock in SeedStrengthen and FindBestImplementation util: Use steady clock in SeedStrengthen and FindBestImplementation Mar 2, 2023
@maflcko maflcko force-pushed the 2303-bench-with-steady-clock- branch from fa29875 to 1111e2f Compare March 2, 2023 13:49
@maflcko maflcko changed the title util: Use steady clock in SeedStrengthen and FindBestImplementation util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk Mar 2, 2023
@DrahtBot DrahtBot mentioned this pull request Mar 2, 2023
Merged
@willcl-ark
Copy link
Member

willcl-ark commented Mar 6, 2023

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.

@maflcko
Copy link
Member Author

maflcko commented Mar 6, 2023

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?

john-moffett
john-moffett approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@john-moffett john-moffett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa1b4e5

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

willcl-ark
willcl-ark approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa1b4e5

@sipa
Copy link
Member

sipa commented Mar 7, 2023

Concept ACK

@fanquake fanquake merged commit 2de0559 into bitcoin:master Mar 8, 2023
@maflcko maflcko deleted the 2303-bench-with-steady-clock-🙂 branch March 8, 2023 14:32
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 8, 2023
@fanquake
Merge bitcoin#27189: util: Use steady clock in SeedStrengthen, FindBe…
24cce59 
…stImplementation, FlushStateToDisk

fa1b4e5 Use steady clock in FlushStateToDisk (MarcoFalke)
1111e2f Use steady clock in SeedStrengthen and FindBestImplementation (MarcoFalke)

Pull request description:

  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.

ACKs for top commit:
  john-moffett:
    ACK fa1b4e5
  willcl-ark:
    ACK fa1b4e5

Tree-SHA512: cc625e796b186accd53222bd64eb57d0512bc7e588312d254349b542bbc5e5daac348ff2b3b3f7dc5ae0bbbae2ec11fdbf3022cf2164211633765a4b0108e83e
@bitcoin bitcoin locked and limited conversation to collaborators Mar 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stickies-v stickies-v Awaiting requested review from stickies-v

2 more reviewers

@willcl-ark willcl-ark willcl-ark approved these changes

@john-moffett john-moffett john-moffett approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @DrahtBot @willcl-ark @sipa @john-moffett @fanquake