refactor: Use NodeClock::time_point in more places #35315

In commit "refactor: Allow NodeClock::epoch to be used in NodeSeconds context" (fa10abb29538fabb4ad97dd70a2e4c709f38fc16)

This change seems kludgy and would seem to encourage writing bad code.

The kludgy part is mixing up different time_point types inside a clock definition which should have one native time type.

The bad code part is encouraging code that shouldn't be tied to the unix epoch to reference it unnecessarily. For example the code using NodeClock::epoch in this commit is storing absolute times as durations which we do not want and should not encourage. And code in later commits uses NodeClock::epoch as an inappropriate sentinel value where time_point::min, time_point::max, or std::nullopt sentinels would be more appropriate.

Would recommend dropping this commit, if not deleting the NodeClock::epoch constant entirely which seems nonstandard and not very helpful.

If default time values are needed, it's more direct and less verbose to call the default time point constructor. Having this second bitcoin-specific way of default initializing time variables just makes code less consistent and more confusing.

For example the code using NodeClock::epoch in this commit is storing absolute times as durations which we do not want and should not encourage.

IIUC addrman is using that to detect corrupt data and also using that as a sentinel. Also, it is used in the addrman serialization. The alternative to serializing just the duration would be to also serialize the type (epoch) somehow, but I don't think a full addrman rewrite with an addrman serialize change is the right call for a simple refactoring change.

I am not sure if there is much value in removing the zero/echo stuff everywhere, but when epoch is used consistently, it is also easier to grep for it and find all places with a single call to git grep.

I like the commit, and I don't think it matters much, so I'd prefer to keep it. But I am happy to drop it, if you think it is a blocker. Also, I am happy to review a different pull removing epoch (assuming that such a pull is doing some more important substantial changes as well)

re: #35315 (review)

I'm not suggesting changing the addrman serializaton format. That would be crazy. I am just asking to not increase usage of the unnecessary, nonstandard, undocumented NodeClock::epoch constant.

but when epoch is used consistently, it is also easier to grep for it and find all places with a single call to git grep.

There is no enforcement NodeClock::epoch is used consistently. Bitcoin core is adding a new, nonstandard clock class member to reference january 1, 1970, and encourage using it as a magical time point without disabling time_point default constructor which also sets this time. If this change included a lint check to prevent the default time_point constructor from being used that could make more sense. But this change provides no extra safety while encouraging bad duration-based code and magic-constant code to be written.

The other changes in this PR using time point more places seem great. But this change extending NodeClock::epoch to places where it doesn't actually make sense to treat January 1, 1970 as special is a step backwards. Better alternatives are using time_point::min, and time_point::max and std::optional in most cases.

But I am happy to drop it, if you think it is a blocker.

I would definitely encourage dropping it but if you don't want to drop it I would like to see some explanation of why it is good to have. It seems like the only use-cases are bad code. NodeClock::epoch is not mentioned in the PR description and no other reviewer has commented on it. It's referenced 10 times before this PR and 44 times after. I gave a code review ACK and half-concept ACK on the PR so this is not a blocker for me, but I would definitely like to see it dropped from the PR or properly explained.

re: #35315 (review)

Another possible approach you can take if you are not comfortable with changing existing code or using the default time_point constructor for its intended purpose would be to define a standalone constant like:

//! Default value assigned to a NodeSeconds time point variable if no explicit
//! value is set. Since C++20 this is guaranteed to be the unix epoch time,
//! 1970-01-01T00:00:00Z.
//! Bitcoin Core code should generally avoid referencing this constant or
//! treating this time point as special. If a time variable is unset it is
//! usually preferable to initialize it with time_point::max or time_point::min
//! values for more natural comparisons, or to use std::optional.
constexpr NodeSeconds NODE_UNSET_TIME{};

This would be a drop-in replacement for the new NodeClock::epoch definition in this PR and avoid the problems I think NodeClock::epoch creates of encouraging incorrect and unsafe code, since it has a scarier and usage comment. It would also avoid adding a nonstandard member to the clock class.

I'm not suggesting changing the addrman serializaton format. That would be crazy. I am just asking to not increase usage of the unnecessary, nonstandard, undocumented NodeClock::epoch constant.

Ok, I just fail to see how to change addrman to avoid this magic value of zero. It is deeply embedded, so any change away from magic-zero means a larger re-write.

I agree with you that epoch is unnecessary, but I think it is self-explanatory that it means the epoch (time-zero). Also there is a static_assert for documentation: static_assert(NodeClock::epoch.time_since_epoch().count() == 0);

Your suggested docstring looks nice. I am happy to modify the first commit to add that docstring to epoch.

I am less sure about moving this to a stand-alone constant. I can see your criticism, but I don't think epoch simply existing is encouraging incorrect and unsafe code (compared to the alternative of having no docstring and just a std-lib default constructor without any docstring/warning). If moving to a stand-alone constant is important, maybe it can be done in a follow-up?

It seems like the only use-cases are bad code.

Btw, I agree. It is just that I don't agree the constant itself makes it worse. This is simply a years-old pre-existing code pattern, and I don't want to expand the scope here too much. (The changes are already 7 commits and 150+ lines changed)

The other reviewers didn't seem to have flagged it either?

In commit "refactor: Use NodeSeconds for m_best_block_time" (fa1a22927adac877c46645b586ecda8ae1592002)

This seems unnecessarily confusing. NodeClock is an arbitrary precision system clock while NodeSeconds explicitly uses integer second values. It doesn't seem helpful to mix up these different types and it is also just shorter to write std::atomic<NodeSeconds> m_best_block_time{}.

This seems unnecessarily confusing.

I don't understand why this is confusing. The epoch of a clock is always the same (a compile time constant), regardless of the the clock's time point duration.

it is also just shorter to write std::atomic<NodeSeconds> m_best_block_time{}.

Yes, it is shorter, but also more confusing. 0s or epoch is used as a sentinel value, so it seems good to be explicit about the special meaning.

I know you mentioned that std::optional can be used instead, which would be cleaner. However, I don't really agree here, because it would make the code a lot more verbose and every call-site would have to safely unwrap the optional one way or another (nested if, value_or, ...)

I like the current commit, so I think I'll keep it, but let me know if this is a blocker.

re: #35315 (review)

I know you mentioned that std::optional can be used instead, which would be cleaner. However, I don't really agree here, because it would make the code a lot more verbose and every call-site would have to safely unwrap the optional one way or another (nested if, value_or, ...)

Looking at net_processing.cpp I see only one access in ApproximateBestBlockDepth which seems buggy and better off using std::optional.

You also claim that calling the default time point constructor (https://en.cppreference.com/cpp/chrono/time_point/time_point) would be "more confusing" without saying what is confusing. The default constructor is part of the standard and well-documented. NodeClock::epoch is nonstandard, undocumented, unjustified, and even more of an oddity after this PR because it now uses a different time type than the rest of the clock.

In commit "refactor: Use NodeClock::time_point for m_last_block_announcement" (fa67c651510db5e571fa82fe692ac33e48c451a4)

Can commit message be clarified to say whether there is any change in behavior here? Presumably if times were previously represented in seconds, and now higher precision times are used, now comparisons between the times may return different values and new bugs and corner cases could be exposed.

Changing behavior should be ok and probably even an improvement, but commit message should clarify whether it is changing or not. Same comment applies to most other commits in this PR as well.

Same comment applies to most other commits in this PR as well.

I use the keyword refactor: in the pull request title and in all commits to indicate that no behavior change is going on. If some weird or obscure behavior change was happening, it would be my burden to point them out in the commit message and reviewers are meant to be encouraged to report undocumented behavior changes.

I like this notation, and I think it is brief and understandable. Also, it is explained in the docs:

CONTRIBUTING.md-126-### Creating the Pull Request
CONTRIBUTING.md-127-
CONTRIBUTING.md-128-The title of the pull request should be prefixed by the component or area that
CONTRIBUTING.md-129-the pull request affects. Valid areas are:
CONTRIBUTING.md-130-
...
CONTRIBUTING.md:137:  - `refactor` for structural changes that do not change behavior

However, if you think it is a blocker, I can modify all the commit messages to say:

This refactor does not change any behavior.

re: #35315 (review)

reviewers are meant to be encouraged to report undocumented behavior changes.

Yes, that's my intent here. Comparisons like state->m_last_block_announcement < oldest_block_announcement that could previously be false when times were seconds may now be true when times are nanoseconds (in case where times were equal). So this change does not seem like a pure refactoring and it would be good to point out any possible behavior changes like this in commit messages.

A similar case where switching to more precise time types caused an observable change in behavior is 77043b0c856f195bda051a1feb2505347f0eddf3. There are also cases where changing time types could lead to overflows, but I don't think that is happening here.

A similar case where switching to more precise time types caused an observable change in behavior is 77043b0.

I don't think the bug was caused by switching to more precise time. The bug fixed there is a rare pre-existing bug, which was made more likely (almost deterministic) by using precise time.

There are also cases where changing time types could lead to overflows, but I don't think that is happening here.

Correct. C++ duration types can deal with year ranges of roughly +-292 years, according to https://en.cppreference.com/cpp/chrono/duration.

I'd presume, mostly there are only overflow issues, when using native time_point ::min() or ::max() in combination with untrusted input seconds (thus injecting a multiplication that overflows).

reviewers are meant to be encouraged to report undocumented behavior changes.

Yes, that's my intent here. Comparisons like state->m_last_block_announcement < oldest_block_announcement that could previously be false when times were seconds may now be true when times are nanoseconds (in case where times were equal).

Correct, but this change in behavior is not reliable or observable from outside. In fact, the tie-breaker behavior on the peer-id seems questionable to begin with. If block announcement happened on a "second-boundary" on master, e.g a block is announced by peer N in second 2.99, but by peer N+1 in second 3.01 (difference 0.02 seconds), then peer N is evicted. However, if the same difference of 0.02 seconds happens some other time (like peer_N announces at 5.20s and peer_N+1 at 5.22s), then peer N+1 is evicted.

After the changes in this commit, there is basically no tie-break anymore (and the code could even be removed), because full-precision comparisons are used, and the slower-to-announce peer is always picked.

Happy to add this to the commit description, or happy to remove the code, but I wouldn't remove the refactor: label, as I don't consider this a change of behavior. Though, I can also add back the cast to seconds explicitly, if you think it makes sense.

In commit "refactor: Use NodeClock::time_point in txdownloadman/txrequest" (fae925ff923f05f34c800b92039e5fd058262b74)

Not important, but since this commit moves away from hardcoding microsecond types everywhere, it would be nice for test code here to stop hardcoding microseconds as well and switch to ticks (NodeClock::duration) instead.

I don't think this is allowed. Changing the fuzz test values to something else will likely change the fuzz input format, which is a behavior change.

Changing the behavior is not allowed in refactor commits, according to CONTRIBUTING.md:137

(closing thread due to thumbs up)

In commit "refactor: Use NodeClock::time_point instead of std::chrono::seconds" (fa72cdf9563a39d6253570199a594b40d14c65dd)

Commit title is very generic. Would be good to add "in node stats" to indicate where the replacement is happening.

I think that makes the commit message larger than the recommended 70chars, but I am happy to push this, if you think it is a blocker.

In commit "refactor: Use NodeClock::time_point instead of std::chrono::microseconds" (cccc80ec090257ad614e54ca3975004282531bd9)

Would seem good to mention net_processing in the commit title since otherwise it is unclear what part of the codebase this commit affects.

(same). I think it is easy enough to call git show --stat to see the area of the codebase, but I am happy to consider changing it if there is a push to this pull.

re: #35315 (review)

Thanks, anything seems fine. It is just nice to have to be have some idea about which code is changing when looking at git log --oneline output.

In commit "refactor: Use NodeClock::time_point instead of std::chrono::microseconds" (cccc80ec090257ad614e54ca3975004282531bd9)

The new code seems to make less sense semantically the old code.

The previous if (state.m_stalling_since.count()) reads like "if a stalling_since value is set".

The new "if (state.m_stalling_since != NodeClock::epoch)" reads like "if not stalling since january 1, 1970"

Number of ways this could be improved:

• Default m_stalling_since to NodeClock::time_point::max instead of NodeClock::epoch
• Make m_stalling_since use std::optional.
• Compare against the NodeClock::time_point{} default value instead referencing the unix epoch.

Default m_stalling_since to NodeClock::time_point::max instead of NodeClock::epoch

Ok, I'll think about those over the next few days, mostly wondering how to minimize review churn on this.

Though, I think that it is nice to make the special value verbosely typed.

re: #35315 (review)

minimize review churn

Note: of the 3 suggestions, comparing against NodeClock::time_point{} would be the smallest change, and would be a relative improvement because it would make code that is using an inappropriate sentinel value look like it using an inappropriate sentinel value. But other alternatives to use an appropriate value or use std::optional do not seem like much work either.

In commit "refactor: Use NodeClock::time_point instead of std::chrono::microseconds" (cccc80ec090257ad614e54ca3975004282531bd9)

Would seem more consistent to use NodeClock::time_point::min() here instead of NodeClock::epoch to indicate we are not syncing, given that NodeClock::time_point::max() is used immediately below to indicate we are done syncing. Epoch time should not be relevant.