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

maflcko commented at 3:21 PM on May 18, 2026: member

It is a bit confusing to have some code use the deprecated GetTime, which returns a duration and not a time point, and other code to use NodeClock time points.

Fix all places in net_processing.cpp to properly use time_point types.

DrahtBot added the label Refactoring on May 18, 2026

DrahtBot commented at 3:21 PM on May 18, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35315.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	ryanofsky
Concept ACK	w0xlt
Stale ACK	sedited, seduless

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35561 (net: move some CNodeState fields to Peer by Crypt-iQ)
#35522 (refactor: Extract per-message helpers from SendMessages() (move-only) by pablomartin4btc)
#35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
#34824 (net: encapsulate TxRelay state and replace recursive mutexes by w0xlt)
#34743 (p2p: don't disconnect manual peers for block stalling by willcl-ark)
#34628 (p2p: Replace per-peer transaction rate-limiting with global rate limits by ajtowns)
#34565 (refactor: extract BlockDownloadManager from PeerManagerImpl by w0xlt)
#27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

node::TxDownloadManager txdownloadman{node::TxDownloadOptions{pool, det_rand, true}} in src/test/fuzz/txdownloadman.cpp
node::TxDownloadManagerImpl txdownload_impl{node::TxDownloadOptions{pool, det_rand, true}} in src/test/fuzz/txdownloadman.cpp

2026-07-08 14:11:52

maflcko force-pushed on May 18, 2026

DrahtBot added the label CI failed on May 18, 2026

DrahtBot commented at 3:50 PM on May 18, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/26042826463/job/76558889400 LLM reason (✨ experimental): CI failed because the build stopped with a Clang -Werror error: rpcconsole.cpp has an unused variable (time_now).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

maflcko force-pushed on May 18, 2026

DrahtBot removed the label CI failed on May 18, 2026

maflcko force-pushed on May 29, 2026

w0xlt commented at 12:47 AM on June 14, 2026: contributor

Concept ACK

sedited approved

sedited commented at 9:29 PM on June 24, 2026: contributor

ACK fa14855ff1bd527cbbd56080eb2cfdcd74d780da

seduless commented at 10:14 PM on June 24, 2026: contributor

Tested ACK fa14855ff1bd527cbbd56080eb2cfdcd74d780da

In commit fa1a22927adac877c46645b586ecda8ae1592002, peerman_tests still has a few straightforward migrations left. Non-blocking, happy to send a follow-up if you'd rather not fold it in here.

maflcko commented at 6:16 AM on June 25, 2026: member

Yeah, the goal here is mostly to fix net_processing fully. Not sure what the ideal size of a pull request is, but 7 commits and ~150 lines explicitly touched (and a few more implicitly) seems ok-ish.

Edit: I think the next pull should just remove it fully, in all remaining places?

seduless commented at 3:50 PM on June 25, 2026: contributor

I think the next pull should just remove it fully, in all remaining places?

Perhaps it's useful for the next follow-up to avoid clashing with the scoped clock proposed for the kernel in #35557?

in src/util/time.h:33 in fa10abb295 outdated

  29 | @@ -30,7 +30,7 @@ struct NodeClock : public std::chrono::system_clock {
  30 |      static time_point now() noexcept;
  31 |      static std::time_t to_time_t(const time_point&) = delete; // unused
  32 |      static time_point from_time_t(std::time_t) = delete;      // unused
  33 | -    static constexpr time_point epoch{};
  34 | +    static constexpr std::chrono::time_point<NodeClock, std::chrono::seconds> epoch{};

ryanofsky commented at 1:04 PM on June 29, 2026:

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.

maflcko commented at 3:22 PM on June 30, 2026:

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)

ryanofsky commented at 4:21 PM on June 30, 2026:

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.

ryanofsky commented at 2:25 AM on July 1, 2026:

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.

maflcko commented at 2:37 PM on July 1, 2026:

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?

ryanofsky commented at 4:53 PM on July 7, 2026:

re: #35315 (review)

I think I'm not sure what you are asking here. I have acked this PR. I just think this commit (fa1a22927adac877c46645b586ecda8ae1592002) is bad and unnecessary and suggested several alternatives that would be better:

Not using magic zero value in runtime code but keeping it in serialized formats (runtime code might prefer std::optional, time_point::min or time_point::max values depending on the situation).
Keeping magic zero value in runtime code but deleting (or at least not increasing use of) the nonstandard and error-prone NodeClock::epoch constant by using the standard C++ approach of calling the default time_point{} constructor.
Adding a NODE_UNSET_TIME constant to make it clear that a magic value is being used that needs to be checked for separately, and also discourage code in the style from being written in the future.

maflcko commented at 9:32 AM on July 8, 2026:

make it clear that a magic value is being used that needs to be checked for separately, and also discourage code in the style from being written in the future.

Thx, done in the commit msg and docstring.

maflcko commented at 5:52 AM on July 9, 2026:

Closing thread for now, looks like there is a new thread on the same commit/topic: #35315 (review)

in src/net_processing.cpp:781 in fa1a22927a

 777 | @@ -778,7 +778,7 @@ class PeerManagerImpl final : public PeerManager
 778 |      /** The height of the best chain */
 779 |      std::atomic<int> m_best_height{-1};
 780 |      /** The time of the best chain tip block */
 781 | -    std::atomic<std::chrono::seconds> m_best_block_time{0s};
 782 | +    std::atomic<NodeSeconds> m_best_block_time{NodeClock::epoch};

ryanofsky commented at 1:17 PM on June 29, 2026:

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{}.

maflcko commented at 3:22 PM on June 30, 2026:

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.

ryanofsky commented at 4:39 PM on June 30, 2026:

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.

maflcko commented at 9:32 AM on July 8, 2026:

write std::atomic<NodeSeconds> m_best_block_time{}.

thx, done

maflcko commented at 5:54 AM on July 9, 2026:

Closing thread for now. Continued thread for same commit/topic is #35315 (review)

in src/net_processing.h:169 in fa67c65151 outdated

 165 | @@ -166,7 +166,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
 166 |      virtual void CheckForStaleTipAndEvictPeers() = 0;
 167 |  
 168 |      /** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */
 169 | -    virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;
 170 | +    virtual void UpdateLastBlockAnnounceTime(NodeId node, NodeClock::time_point time) = 0;

ryanofsky commented at 1:24 PM on June 29, 2026:

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.

maflcko commented at 3:21 PM on June 30, 2026:

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.

ryanofsky commented at 4:46 PM on June 30, 2026:

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.

maflcko commented at 3:17 PM on July 1, 2026:

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.

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.

ryanofsky commented at 5:09 PM on July 7, 2026:

re: #35315 (review)

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.

I don't know what happened here, but I'm definitely not asking you to remove refactoring label, or add casts to seconds.

Quoting my original comment "Can commit message be clarified" "commit message should clarify" and "Changing behavior should be ok and probably even an improvement"... and I'm just saying commit messages in this PR would be better if they stated which commits change behavior of the code and which do not.

A concrete reason for asking this is that while you and I have changed time_point code recently and are familiar with these edge-case behavior changes, other reviewers and readers may not be. If a commit only says it is a refactor and swaps out C++ types, reviewers might not know they should be looking out for these cases.

maflcko commented at 9:32 AM on July 8, 2026:

commit message should clarify

Thx, done. Also used ::min here over ::epoch. Also, resolving thread.

in src/test/txrequest_tests.cpp:37 in fae925ff92 outdated

  31 | @@ -32,13 +32,13 @@ struct TxRequestTest : BasicTestingSetup {
  32 |      void TestInterleavedScenarios();
  33 |  };
  34 |  
  35 | -constexpr std::chrono::microseconds MIN_TIME = std::chrono::microseconds::min();
  36 | -constexpr std::chrono::microseconds MAX_TIME = std::chrono::microseconds::max();
  37 | +constexpr NodeClock::time_point MIN_TIME = NodeClock::time_point::min();
  38 | +constexpr NodeClock::time_point MAX_TIME = NodeClock::time_point::max();
  39 |  constexpr std::chrono::microseconds MICROSECOND = std::chrono::microseconds{1};

ryanofsky commented at 1:41 PM on June 29, 2026:

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.

maflcko commented at 3:21 PM on June 30, 2026:

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

maflcko commented at 3:18 PM on July 1, 2026:

(closing thread due to thumbs up)

in src/net.h:197 in fa72cdf956 outdated

 193 | @@ -194,8 +194,8 @@ class CNodeStats
 194 |      NodeId nodeid;
 195 |      NodeClock::time_point m_last_send;
 196 |      NodeClock::time_point m_last_recv;
 197 | -    std::chrono::seconds m_last_tx_time;
 198 | -    std::chrono::seconds m_last_block_time;
 199 | +    NodeClock::time_point m_last_tx_time;

ryanofsky commented at 1:59 PM on June 29, 2026:

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.

maflcko commented at 3:21 PM on June 30, 2026:

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.

maflcko commented at 9:32 AM on July 8, 2026:

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

thx, done, and resolving

in src/net_processing.cpp:6224 in cccc80ec09 outdated

6220 | @@ -6222,6 +6221,6 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6221 |          if (!vGetData.empty())
6222 |              MakeAndPushMessage(node, NetMsgType::GETDATA, vGetData);
6223 |      } // release cs_main
6224 | -    MaybeSendFeefilter(node, peer, current_time);
6225 | +    MaybeSendFeefilter(node, peer, now);

ryanofsky commented at 2:15 PM on June 29, 2026:

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.

maflcko commented at 3:21 PM on June 30, 2026:

(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.

ryanofsky commented at 4:50 PM on June 30, 2026:

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.

maflcko commented at 9:32 AM on July 8, 2026:

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

thx, done, and resolving

in src/net_processing.cpp:6103 in cccc80ec09

6099 | @@ -6101,7 +6100,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6100 |  
6101 |          // Detect whether we're stalling
6102 |          auto stalling_timeout = m_block_stalling_timeout.load();
6103 | -        if (state.m_stalling_since.count() && state.m_stalling_since < current_time - stalling_timeout) {
6104 | +        if (state.m_stalling_since != NodeClock::epoch && state.m_stalling_since < now - stalling_timeout) {

ryanofsky commented at 2:59 PM on June 29, 2026:

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.

maflcko commented at 3:21 PM on June 30, 2026:

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.

ryanofsky commented at 4:54 PM on June 30, 2026:

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.

maflcko commented at 9:31 AM on July 8, 2026:

Make m_stalling_since use std::optional.

Thx done (in a new commit), and resolving thread

in src/net_processing.cpp:6154 in cccc80ec09 outdated

6150 | @@ -6152,13 +6151,13 @@ bool PeerManagerImpl::SendMessages(CNode& node)
6151 |                          // this peer (eventually).
6152 |                          state.fSyncStarted = false;
6153 |                          nSyncStarted--;
6154 | -                        peer.m_headers_sync_timeout = 0us;
6155 | +                        peer.m_headers_sync_timeout = NodeClock::epoch;

ryanofsky commented at 3:12 PM on June 29, 2026:

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.

maflcko commented at 9:31 AM on July 8, 2026:

Would seem more consistent to use NodeClock::time_point::min() here

Sorry, I don't follow here. Using epoch or min is equally irrelevant and wrong here: The value isn't used and trying to imply that a timeout will happen in the past is confusing. Either this should be left as-is (which I've done in this pull request), or this should be rewritten from the ground up, but I think this pull is already large enough. Unless reviewers want me to rewrite this in a separate commit, I'll leave this as-is.

maflcko commented at 2:13 PM on July 8, 2026:

Would seem more consistent to use NodeClock::time_point::min() here

Sorry, I don't follow here. Using epoch or min is equally irrelevant and wrong here:

Ok, I went ahead and pushed a commit to use nullopt instead. The commit is separate, so that it explains why the change is correct and why epoch-zero or min (or any other value) is irrelevant.

ryanofsky approved

ryanofsky commented at 4:29 PM on June 29, 2026: contributor

Code review ACK fa14855ff1bd527cbbd56080eb2cfdcd74d780da. But approach +0.5. NodeClock::time_point is an improvement over using integer or duration types to represent time points. But I don't think it's a good thing to be hardcoding NodeClock::epoch everywhere, especially for things like block times which don't come from the node/system clock. If default-initializing time variables, it seems better to do it the standard way by calling default constructors, instead of inviting inconsistency and preferring to use bitcoin-specific NodeClock::epoch constant. Also, if choosing sentinel time values, it seems better to use min or max values than to treat the epoch time as being special unnecessarily.

I am also not sure it's good hardcode NodeClock::time_point types everywhere. It seems like a lost opportunity to choose to hardcode a platform-dependent clock type that doesn't have a standard precision or representation, when we could use application-specific type aliases like using MempoolTime = NodeClock::time_point;, using NetworkTime = NodeClock::time_point;, using BlockTime = NodeSeconds; to not be tied to the system clock and be able to intentionally chose which precision and representations to use in different areas of the code. For example, it would be nice to define standard ways of serializing each of these types without requiring them all to be serialized the same way.

I left more detailed code review comments below, but I guess my main feedback is I would be happier to see most NodeClock::epoch uses dropped. And I also think it could be a good idea to replace most NodeClock::time_point references here with a networking specific NetworkTime alias.

maflcko commented at 3:21 PM on June 30, 2026: member

using MempoolTime = NodeClock::time_point;, using NetworkTime = NodeClock::time_point;, using BlockTime = NodeSeconds; to not be tied to the system clock and be able to intentionally chose which precision and representations to use in different areas of the code. For example, it would be nice to define standard ways of serializing each of these types without requiring them all to be serialized the same way.

Not sure. Those types are direct aliases, so they are tied to the (mockable) system clock (aka node clock). Also, given that they are type aliases, so anyone can use them interchangeably, which seems confusing.

While I like my approach, I don't think it matters much and I am happy to switch to whatever reviewers prefer.

ryanofsky approved

ryanofsky commented at 5:34 PM on June 30, 2026: contributor

Thanks for the replies. I still think expanded uses of NodeClock::epoch in this PR are bad, and haven't seen a positive case being made for them. So would like to see that addressed by dropping them, improving them, or explaining benefits with some rationale.

re: #35315 (comment)

Those types are direct aliases, so they are tied to the (mockable) system clock (aka node clock). Also, given that they are type aliases, so anyone can use them interchangeably, which seems confusing.

Not sure what is confusing. Type aliases are meant to be interchangeable. They exist to express developer intent and make code more readable and maintainable. Using them would make time variables declarations more self-documenting, allow changing time types without causing churn, and allow adding more type constraints or features like serialization support in the future. I don't have a strong opinion on this. I just think it is a good idea without any downsides that I can see.

maflcko commented at 3:26 PM on July 1, 2026: member

allow changing time types without causing churn, and allow adding more type constraints or features like serialization support in the future. I don't have a strong opinion on this. I just think it is a good idea without any downsides that I can see.

I mostly think this invites bike-shedding, because it is less clear where to draw the line without knowing any of the imaginary future plans. E.g. should network time be the same alias like p2p time, and mempool time, and validation time, or should even different fields in p2p have different named time aliases, ...? [Meta note: Generally it is best to provide each review topic in a new review thread, and not in the global thread. Otherwise, it is harder to follow the global thread, because it mixes different sub-threads]

sedited commented at 9:19 AM on July 4, 2026: contributor

E.g. should network time be the same alias like p2p time, and mempool time, and validation time, or should even different fields in p2p have different named time aliases, ...?

I agree with @maflcko, and see similar things with other aliases that were introduced in the codebase. I think they are a poor tool for being the impetus for more in depth code changes. Sometimes they can help documenting intent, but for the reasons named here I don't think they really make it easier.

DrahtBot added the label Needs rebase on Jul 7, 2026

ryanofsky approved

ryanofsky commented at 5:56 PM on July 7, 2026: contributor

Thanks for the replies. To be clear my only objection to this PR is the expanded use of the NodeClock::epoch constant which I think will lead to bugs and encourage writing bad code. I suggested various specific alternatives in my comments, and also ACKed this PR so these comments can be ignored.

My suggestion to use type aliases instead of hardcoding NodeClock::time_point is less important, because using NodeClock::time_point is definitely better than what current code does, so type aliases would just be an additional improvement. I did list specific reasons I think they are a good idea (making intent clearer by indicating which times need to have the same types and which time do not, making it possible to introduce module-specfic validation and serialization formats without needing to update many call sites) while objections seem more shallow and hand-wavy ("invites bike-shedding", "they are a poor tool") that list no technical downsides. Seems fine to agree to disagree on this, though.

refactor: Allow NodeClock::epoch to be used in NodeSeconds context

Some contexts have only second precision by definition, and it should be
allowed to use NodeClock::epoch as an alias for zero.

The `epoch` constant is only meant to be used in pre-existing code. New
code should neither use the `epoch` constant, nor a default-constructed
time_point. Instead `min`, `max`, or `std::optional` can be used.

fa815059fc

refactor: Use NodeSeconds for m_best_block_time

Previously, it was using a duration type.

fa3d651b11

refactor: Use NodeClock::time_point for m_last_block_announcement

Previously, a raw i64 was used, which also required a cast to seconds.

Review notes:

* The default value was also changed from zero/epoch to min, because
  this is a more sane default value.
* The change of this default value does not affect sorting the worst
  peers.
* However, changing the time points to be more precise may change the
  sorting of the worst peers when they all announced a block in the same
  second. In this case it should be fine and preferable, to use the more
  accurate sorting from the exact block announce time for the worst
  peers, than to fall back to peer with the highest id.

faba251329

refactor: Use NodeClock::time_point in txdownloadman/txrequest fafe118693

refactor: Use NodeClock::time_point instead of std::chrono::seconds in node stats

for:
* ConsiderEviction()
* m_last_tip_update
* m_stale_tip_check_time
* m_last_block_time
* m_last_tx_time

fa4e8efbb8

maflcko force-pushed on Jul 8, 2026

maflcko commented at 9:38 AM on July 8, 2026: member

Thx for the review. Addressed/replied to all review threads while force pushing the rebase. The second-to-last commit was split up and rewritten from scratch.

DrahtBot removed the label Needs rebase on Jul 8, 2026

refactor: Use std::optional<NodeClock::time_point> instead of std::chrono::microseconds in net_processing

This refactor maps a zero-epoch time point to nullopt, to avoid a
special in-band sentinel value for the fields:

* m_stalling_since
* m_next_inv_send_time
* m_next_local_addr_send

m_next_addr_send is updated to a plain time_point, and uses a default of
time_point::min(), because a special sentinel value is not used.

facb9ef75e

refactor: Use std::optional<NodeClock::time_point> instead of std::chrono::microseconds for m_headers_sync_timeout

fSyncStarted gates all uses of the field, so the zero-epoch default is
irrelevant. time_point::max() is the meaningful sentinel here,
representing a disabled timeout after sync has started, so replace that
state with std::nullopt.

fae47ceffe

refactor: Use NodeClock::time_point instead of std::chrono::microseconds in net_processing

Also, use the more natural time_point::min(), not epoch-zero as the
default value for the fields:

* m_downloading_since
* m_next_send_feefilter

fa11a7eeb8

refactor: Use NodeClock for last GetTime call in net_processing.cpp fa314e4330

maflcko force-pushed on Jul 8, 2026

in src/net_processing.cpp:449 in facb9ef75e

 444 | @@ -445,8 +445,8 @@ struct CNodeState {
 445 |      const CBlockIndex* pindexBestHeaderSent{nullptr};
 446 |      //! Whether we've started headers synchronization with this peer.
 447 |      bool fSyncStarted{false};
 448 | -    //! Since when we're stalling block download progress (in microseconds), or 0.
 449 | -    std::chrono::microseconds m_stalling_since{0us};
 450 | +    /// When this peer started stalling block download progress, or unset.
 451 | +    std::optional<NodeClock::time_point> m_stalling_since{};

ryanofsky commented at 1:38 AM on July 9, 2026:

In commit "refactor: Use std::optionalNodeClock::time_point instead of std::chrono::microseconds in net_processing" (facb9ef75ef9c8e264ba1238f864df5627493d13)

Seems like logic for most of these variables would be simplified using min/max constants instead of optional. Like m_stalling_since would be simpler using max as its unset value, m_next_inv_send_time and m_next_local_addr_send would be simpler using min

in src/net_processing.cpp:407 in fae47ceffe

 403 | @@ -404,7 +404,7 @@ struct Peer {
 404 |      std::atomic<bool> m_sent_sendheaders{false};
 405 |  
 406 |      /** When to potentially disconnect peer for stalling headers download */
 407 | -    std::chrono::microseconds m_headers_sync_timeout GUARDED_BY(NetEventsInterface::g_msgproc_mutex){0us};
 408 | +    std::optional<NodeClock::time_point> m_headers_sync_timeout GUARDED_BY(NetEventsInterface::g_msgproc_mutex){};

ryanofsky commented at 1:49 AM on July 9, 2026:

In commit "refactor: Use std::optionalNodeClock::time_point instead of std::chrono::microseconds for m_headers_sync_timeout" (fae47ceffe929bc83647174f9b4cc52814c9b55d)

Also seems like logic this commit would be simpler if it used max instead of std::optional

in src/net_processing.cpp:793 in fa3d651b11

 789 | @@ -790,7 +790,7 @@ class PeerManagerImpl final : public PeerManager
 790 |      /** The height of the best chain */
 791 |      std::atomic<int> m_best_height{-1};
 792 |      /** The time of the best chain tip block */
 793 | -    std::atomic<std::chrono::seconds> m_best_block_time{0s};
 794 | +    std::atomic<NodeSeconds> m_best_block_time{};

ryanofsky commented at 2:02 AM on July 9, 2026:

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

I'm surprised this is using the default constructor instead of setting NodeClock::epoch. I do think using default constructor is better than using epoch. But using std::optional here would seem better than either. Right now ApproximateBestBlockDepth returns a huge nonsense value when this is 0, but it would be better if ApproximateBestBlockDepth returned std::optional as well.

in src/addrman.cpp:551 in fa815059fc

 547 | @@ -548,7 +548,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c
 548 |          const bool currently_online{NodeClock::now() - addr.nTime < 24h};
 549 |          const auto update_interval{currently_online ? 1h : 24h};
 550 |          if (pinfo->nTime < addr.nTime - update_interval - time_penalty) {
 551 | -            pinfo->nTime = std::max(NodeSeconds{0s}, addr.nTime - time_penalty);
 552 | +            pinfo->nTime = std::max(NodeClock::epoch, addr.nTime - time_penalty);

ryanofsky commented at 2:08 AM on July 9, 2026:

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

I need to catch up on your latest comments so sorry if this is explained somewhere, but I still do not understand the reasons behind the choice to prefer NodeClock::epoch over NodeSeconds{0s} and definitely think it would be good to explain why you think using the epoch constant is better in the commit message.

I think using the epoch constant is bad because:

It makes code less less transparent. Previously it was obvious times were being set to 0, now it is less clear what is actually being set.
It make intent less clear. Previously it was clear that a completely arbitrary 0 value was being set and the time point it corresponds to is meaningless. Now it looks like a meaningful date is being set.
Now there are more uses of the epoch constant so it harder to remove the constant. The presence of this constant is bad because it encourages broken, nonstandard code to be written, as I think we both agree.

IMO the PR would be improved dropping the code changes in this commit (I do think the comment added here is helpful).

ryanofsky approved

ryanofsky commented at 2:12 AM on July 9, 2026: contributor

Code review ACK fa314e433020667d1b1171b10083b5a53964d0f2. IMO, this is much improved since the last version, and the last version was already a nice cleanup, so thanks for the updates! I still need to look over the latest review comments, but I reviewed all the code and left a few suggestions. Again feel free to ignore them.

DrahtBot requested review from seduless on Jul 9, 2026

DrahtBot requested review from sedited on Jul 9, 2026

in src/node/eviction.h:23 in fa4e8efbb8

  19 | @@ -19,8 +20,8 @@ struct NodeEvictionCandidate {
  20 |      NodeId id;
  21 |      NodeClock::time_point m_connected;
  22 |      NodeClock::duration m_min_ping_time;
  23 | -    std::chrono::seconds m_last_block_time;
  24 | -    std::chrono::seconds m_last_tx_time;
  25 | +    NodeClock::time_point m_last_block_time;

ryanofsky commented at 2:21 AM on July 9, 2026:

Note: Some commits are still changing behavior without being labeled. For example in fa4e8efbb8b2e3f6460683ee6e1b3aaa3a662d3e logic deciding which peers to evict was based on seconds, now it is based on nanoseconds or whatever precision system_clock uses, which is a minor improvement, but also a potentially observable change.

IMO, it would be better if commits explicitly said "This commit is not changing any changing behavior" "This commit is changing behavior slightly..." so intent behind the changes would be clear.