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	stickies-v
Concept ACK	w0xlt
Stale ACK	sedited, seduless, ryanofsky

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.

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.

maflcko commented at 6:01 AM on July 9, 2026:

Resolving thread for now. A new thread seems to have been started about m_headers_sync_timeout in #35315 (review)

ryanofsky commented at 3:34 PM on July 16, 2026:

re: #35315 (review)

The value isn't used

Yes, it wasn't obvious the value was ignored and is gated on fSyncStarted.

It looked like the code was trying to represent 3 states: not syncing, syncing, and done syncing, but fSyncStarted means the first state is never actually checked.

Next question might be why there is an fSyncStarted variable instead of using std::optional, but in any case use of min is probably not advisable here.

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.

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

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

maflcko commented at 3:43 PM on July 9, 2026:

min/max instead of optional

Sure, done.

stickies-v commented at 8:48 PM on July 20, 2026:

Seems like logic for most of these variables would be simplified using min/max constants instead of optional.

I find relying on sentinels (if (State(staller)->m_stalling_since == NodeClock::time_point::max())) to be an antipattern, and would have found it more clear if this were a std::optional.

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 78c6002ede..b4e6909ab2 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -445,8 +445,8 @@ struct CNodeState {
     const CBlockIndex* pindexBestHeaderSent{nullptr};
     //! Whether we've started headers synchronization with this peer.
     bool fSyncStarted{false};
-    /// When this peer started stalling block download progress, or max() if not stalling.
-    NodeClock::time_point m_stalling_since{NodeClock::time_point::max()};
+    /// When this peer started stalling block download progress.
+    std::optional<NodeClock::time_point> m_stalling_since{};
     std::list<QueuedBlock> vBlocksInFlight;
     //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty.
     NodeClock::time_point m_downloading_since{NodeClock::time_point::min()};
@@ -1239,7 +1239,7 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional<Node
             // Last validated block on the queue for this peer was received.
             m_peers_downloading_from--;
         }
-        state.m_stalling_since = NodeClock::time_point::max();
+        state.m_stalling_since.reset();
 
         range.first = mapBlocksInFlight.erase(range.first);
     }
@@ -6205,7 +6205,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
 
         // Detect whether we're stalling
         auto stalling_timeout = m_block_stalling_timeout.load();
-        if (state.m_stalling_since < now - stalling_timeout) {
+        if (state.m_stalling_since && *state.m_stalling_since < now - stalling_timeout) {
             // Stalling only triggers when the block download window cannot move. During normal steady state,
             // the download window should be much larger than the to-be-downloaded set of blocks, so disconnection
             // should only happen during initial block download.
@@ -6302,7 +6302,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
                     pindex->nHeight, node.GetId());
             }
             if (state.vBlocksInFlight.empty() && staller != -1) {
-                if (State(staller)->m_stalling_since == NodeClock::time_point::max()) {
+                if (!State(staller)->m_stalling_since) {
                     State(staller)->m_stalling_since = now;
                     LogDebug(BCLog::NET, "Stall started peer=%d\n", staller);
                 }

</details>

Generally, I think ::min() and ::max() are natural choices when we can use them without relying on them as sentinel values. Once that no longer holds, I think std::optional is more clear.

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 78c6002ede..0c5214c2b0 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -309,8 +309,8 @@ struct Peer {
          *  NODE_BLOOM. See BIP35. */
         bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
         /** The next time after which we will send an `inv` message containing
-         *  transaction announcements to this peer. */
-        NodeClock::time_point m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){NodeClock::time_point::min()};
+         *  transaction announcements to this peer. Unset until version handshake completes. */
+        std::optional<NodeClock::time_point> m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){};
         /** The mempool sequence num at which we sent the last `inv` message to this peer.
          *  Can relay txs with lower sequence numbers than this (see CTxMempool::info_for_relay). */
         uint64_t m_last_inv_sequence GUARDED_BY(m_tx_inventory_mutex){1};
@@ -366,8 +366,8 @@ struct Peer {
     mutable Mutex m_addr_send_times_mutex;
     /** Time point to send the next ADDR message to this peer. */
     NodeClock::time_point m_next_addr_send GUARDED_BY(m_addr_send_times_mutex){NodeClock::time_point::min()};
-    /** Time point to possibly re-announce our local address to this peer. */
-    NodeClock::time_point m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){NodeClock::time_point::min()};
+    /** Time point to possibly re-announce our local address to this peer. Unset before first announcement. */
+    std::optional<NodeClock::time_point> m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){};
     /** Whether the peer has signaled support for receiving ADDRv2 (BIP155)
      *  messages, indicating a preference to receive ADDRv2 instead of ADDR ones. */
     std::atomic_bool m_wants_addrv2{false};
@@ -445,11 +445,11 @@ struct CNodeState {
     const CBlockIndex* pindexBestHeaderSent{nullptr};
     //! Whether we've started headers synchronization with this peer.
     bool fSyncStarted{false};
-    /// When this peer started stalling block download progress, or max() if not stalling.
-    NodeClock::time_point m_stalling_since{NodeClock::time_point::max()};
+    /// When this peer started stalling block download progress.
+    std::optional<NodeClock::time_point> m_stalling_since{};
     std::list<QueuedBlock> vBlocksInFlight;
     //! When the first entry in vBlocksInFlight started downloading. Don't care when vBlocksInFlight is empty.
-    NodeClock::time_point m_downloading_since{NodeClock::time_point::min()};
+    std::optional<NodeClock::time_point> m_downloading_since{};
     //! Whether we consider this a preferred download peer.
     bool fPreferredDownload{false};
     /** Whether this peer wants invs or cmpctblocks (when possible) for block announcements. */
@@ -1231,7 +1231,7 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional<Node
 
         if (state.vBlocksInFlight.begin() == list_it) {
             // First block on the queue was received, update the start download time for the next one
-            state.m_downloading_since = std::max(state.m_downloading_since, NodeClock::now());
+            state.m_downloading_since = std::max(Assert(state.m_downloading_since).value(), NodeClock::now());
         }
         state.vBlocksInFlight.erase(list_it);
 
@@ -1239,7 +1239,7 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional<Node
             // Last validated block on the queue for this peer was received.
             m_peers_downloading_from--;
         }
-        state.m_stalling_since = NodeClock::time_point::max();
+        state.m_stalling_since.reset();
 
         range.first = mapBlocksInFlight.erase(range.first);
     }
@@ -2286,7 +2286,7 @@ void PeerManagerImpl::InitiateTxBroadcastToAll(const Txid& txid, const Wtxid& wt
         // otherwise at risk of leaking to a spy, if the spy is able to
         // distinguish transactions received during the handshake from the rest
         // in the announcement.
-        if (tx_relay->m_next_inv_send_time == NodeClock::time_point::min()) continue;
+        if (!tx_relay->m_next_inv_send_time) continue;
 
         const uint256& hash{peer.m_wtxid_relay ? wtxid.ToUint256() : txid.ToUint256()};
         if (!tx_relay->m_tx_inventory_known_filter.contains(hash)) {
@@ -3892,7 +3892,7 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
             Assume(WITH_LOCK(
                 tx_relay->m_tx_inventory_mutex,
                 return tx_relay->m_tx_inventory_to_send.empty() &&
-                       tx_relay->m_next_inv_send_time == NodeClock::time_point::min()));
+                       !tx_relay->m_next_inv_send_time));
         }
 
         if (pfrom.IsPrivateBroadcastConn()) {
@@ -5500,19 +5500,19 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, NodeClock::time_poi
     LOCK(peer.m_addr_send_times_mutex);
     // Periodically advertise our local address to the peer.
     if (fListen && !m_chainman.IsInitialBlockDownload() &&
-        peer.m_next_local_addr_send < current_time) {
+        (!peer.m_next_local_addr_send || *peer.m_next_local_addr_send < current_time)) {
         // If we've sent before, clear the bloom filter for the peer, so that our
         // self-announcement will actually go out.
         // This might be unnecessary if the bloom filter has already rolled
         // over since our last self-announcement, but there is only a small
         // bandwidth cost that we can incur by doing this (which happens
         // once a day on average).
-        if (peer.m_next_local_addr_send != NodeClock::time_point::min()) {
+        if (peer.m_next_local_addr_send) {
             peer.m_addr_known->reset();
         }
         if (std::optional<CService> local_service = GetLocalAddrForPeer(node)) {
             CAddress local_addr{*local_service, peer.m_our_services, Now<NodeSeconds>()};
-            if (peer.m_next_local_addr_send == NodeClock::time_point::min()) {
+            if (!peer.m_next_local_addr_send) {
                 // Send the initial self-announcement in its own message. This makes sure
                 // rate-limiting with limited start-tokens doesn't ignore it if the first
                 // message ends up containing multiple addresses.
@@ -6091,7 +6091,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
                 LOCK(tx_relay->m_tx_inventory_mutex);
                 // Check whether periodic sends should happen
                 bool fSendTrickle = node.HasPermission(NetPermissionFlags::NoBan);
-                if (tx_relay->m_next_inv_send_time < now) {
+                if (!tx_relay->m_next_inv_send_time || *tx_relay->m_next_inv_send_time < now) {
                     fSendTrickle = true;
                     if (node.IsInboundConn()) {
                         tx_relay->m_next_inv_send_time = NextInvToInbounds(now, INBOUND_INVENTORY_BROADCAST_INTERVAL, node.m_network_key);
@@ -6205,7 +6205,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
 
         // Detect whether we're stalling
         auto stalling_timeout = m_block_stalling_timeout.load();
-        if (state.m_stalling_since < now - stalling_timeout) {
+        if (state.m_stalling_since && *state.m_stalling_since < now - stalling_timeout) {
             // Stalling only triggers when the block download window cannot move. During normal steady state,
             // the download window should be much larger than the to-be-downloaded set of blocks, so disconnection
             // should only happen during initial block download.
@@ -6227,7 +6227,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
         if (state.vBlocksInFlight.size() > 0) {
             QueuedBlock &queuedBlock = state.vBlocksInFlight.front();
             int nOtherPeersWithValidatedDownloads = m_peers_downloading_from - 1;
-            if (now > state.m_downloading_since + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) {
+            if (now > Assert(state.m_downloading_since).value() + std::chrono::seconds{consensusParams.nPowTargetSpacing} * (BLOCK_DOWNLOAD_TIMEOUT_BASE + BLOCK_DOWNLOAD_TIMEOUT_PER_PEER * nOtherPeersWithValidatedDownloads)) {
                 LogInfo("Timeout downloading block %s, %s", queuedBlock.pindex->GetBlockHash().ToString(), node.DisconnectMsg());
                 node.fDisconnect = true;
                 return true;
@@ -6302,7 +6302,7 @@ bool PeerManagerImpl::SendMessages(CNode& node)
                     pindex->nHeight, node.GetId());
             }
             if (state.vBlocksInFlight.empty() && staller != -1) {
-                if (State(staller)->m_stalling_since == NodeClock::time_point::max()) {
+                if (!State(staller)->m_stalling_since) {
                     State(staller)->m_stalling_since = now;
                     LogDebug(BCLog::NET, "Stall started peer=%d\n", staller);
                 }

</details>

Not a blocker, I don't want these style preferences to get in the way of the real improvements.

ryanofsky commented at 1:32 AM on July 21, 2026:

re: #35315 (review)

I haven't checked recently but last time I looked min and max were not used as sentinels, but meaningful values that make comparisons simpler and bugs harder to introduce.

If the claim is that types like optional or variant should be used whenever an individual value is checked for anywhere in the code, even when the value is meaningful, and even when using these types would complicate other comparisons, I'd question that claim. Not to say it's wrong, but just to say it should be judged on how it simplifies code or prevents bugs or has some concrete benefit in the specific situation. Not just that it's "more clear" or avoids an "antipattern".

IMO needing to add Asserts that would otherwise be unnecessary, and write fragile comparisons like if (!x || *x < y) and if (x && x < y) are reasons not to take this approach in this situation.

maflcko commented at 6:22 AM on July 21, 2026:

Right, optional was used in an earlier commit here (looks like GitHub purged facb9ef75ef9c8e264ba1238f864df5627493d13 already). The downsides were:

optional has "converting" compare operators (https://en.cppreference.com/cpp/utility/optional/operator_cmp), so it would be unclear how to write code: if (!x || *x < y) or if (!x || x < y) or if (x<y). I mean, handling the nullopt sentinel value specifically seems clearer, but the compiler/stdlib doesn't enforce that, so maybe just use if (x<y)? But then, one may as well use ::min()/::max() as equally good sentinel value without the code and logic overhead from optional?
dereferencing optional is UB, or calling value() on nullopt will throw. Sure, those are indications of software bugs, but in P2P code we seem to be using Assume instead of Assert. See for example the Assume on m_next_inv_send_time. So instead of trying to avoid nullptr-crash or a throw with optional and somehow still get Assume semanticts (crash in Debug build, fallback in Release), one might as well just use ::min()/::max() as perfectly fine sentinel values that support Assume without a risk of crash/throw.
Existing code uses epoch as sentinel value, similar to ::min(). Bitcoin Core doesn't really allow times at epoch, or before, so epoch-zero and ::min() are equivalent sentinel values.
For the cases where epoch-zero as sentinel is slightly confusing (e.g. timeouts), it seems a smaller diff and suitable cleanup to just switch them to ::max().

stickies-v commented at 1:47 PM on July 21, 2026:

... on how it simplifies code or prevents bugs or has some concrete benefit in the specific situation. Not just that it's "more clear" or avoids an "antipattern".

That's fair. In the case of m_next_inv_send_time, I think using ::min() to indicate that we should send an inv asap, or ::max() that we shouldn't send one is natural and clear and doesn't assign any special meaning to ::min() or ::max(). In InitiateTxBroadcastToAll, however, ::min() does now not just guarantee that we're definitely at some later point, it also adds the additional meaning that a version handshake has not yet been completed. That has absolutely nothing to do with time. The reader now has to be aware of all the usage of the variable, and/or it has to be completely documented.

When m_next_inv_send_time is std::optional, the type makes it perfectly clear that there is a discontinuity to be aware of. That's why I think it's more clear.

Now, probably the most proper solution would be to just stop using m_next_inv_send_time as a proxy for a completed version handshake, but I haven't looked into how feasible that is here.

For other variables, like m_stalling_since, I think it is better to have a sensible (std::nullopt) vs a nonsensical (::max(): a "since" in the future can never make sense) value.

IMO needing to add Asserts that would otherwise be unnecessary

My diff added 2 Asserts that could equally well be omitted, but they're just making the invariant explicit whereas the current code implicitly assumes them. I think explicit is better.

and write fragile comparisons like if (!x || *x < y) and if (x && x < y) are reasons not to take this approach in this situation.

It's more verbose (which can be improved with value_or(), but I'm not sure fragile is the appropriate term for making the discontinuity/semantic overload more explicit?

Sure, those are indications of software bugs, but in P2P code we seem to be using Assume instead of Assert.

Perhaps it's not suitable for the p2p code, but we could still adopt patterns like Assume(some_var).value_or(::min())? It's more verbose, but I think it is helpful to be more explicit?

I mean, handling the nullopt sentinel value specifically seems clearer, but the compiler/stdlib doesn't enforce that

I agree we can't enforce clean code here, but with std::optional we can at least allow it.

Existing code uses epoch as sentinel value, similar to ::min(). Bitcoin Core doesn't really allow times at epoch, or before, so epoch-zero and ::min() are equivalent sentinel values.

I agree it's not worse than epoch.

For the cases where epoch-zero as sentinel is slightly confusing (e.g. timeouts), it seems a smaller diff and suitable cleanup to just switch them to ::max().

I agree, I think ::max() for a timeout is very natural, even when it has not explicitly been set.

maflcko commented at 2:36 PM on July 21, 2026:

Not sure what to do here. This pull is already 10 commits, and switching to optional with Assert or Assume(some_var).value_or(::min()/max()) feels like it is going to explode the review scope here.

I'd say the review scope here should restrict itself to compile-time type-changes only (modulo precision changes).

changing a few sentinel values from epoch-zero to min/max seems fine, but if reviewers can't agree on them for now, it seems best to postpone the two commits (fa8a149c2923d8075fc127dae39653142f6211fa & fa58d8f4fb36e9028503f6bb9415b5c7c1c4dfa1) and just fully restore the initial (and reviewed) version of this pull request (https://github.com/bitcoin/bitcoin/pull/35315#issuecomment-4990553024)

stickies-v commented at 10:47 AM on July 22, 2026:

I'm happy to keep it as-is, it's not a regression vs master and the PR has too many other benefits to be spending too much time/energy on this rather small issue. I just wanted to share my view that I think we should ideally have less ::min() ::max() sentinels, not more.

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

maflcko commented at 3:43 PM on July 9, 2026:

max instead of std::optional

Thx, done

in src/net_processing.cpp:793 in fa3d651b11 outdated

 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.

maflcko commented at 6:41 AM on July 9, 2026:

I'm surprised this is using the default constructor instead of setting NodeClock::epoch.

Why are you surprised that I literally addressed your review feedback? #35315 (review) says:

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

But using std::optional here would seem better ... it would be better if ApproximateBestBlockDepth returned std::optional as well.

It is not allowed to call ApproximateBestBlockDepth when the value is not set. Also, there is no code path where it is not set. I don't think it makes sense to bubble up errors that can never happen. In any case, I am not changing any behavior in this commit, so changing ApproximateBestBlockDepth seems out of scope.

I am not sure what the best way is to catch errors that can never happen here. Historically, in C++17, one could use uninitialized memory for atomic durations. However, in C++20, this is no longer possible after https://wg21.link/p0883r2. In any case, it was never possible with time_points, because they have a user-provided ctor. See also https://godbolt.org/z/a73hvs8rr

ryanofsky commented at 4:10 PM on July 16, 2026:

re: #35315 (review)

Why are you surprised that I literally addressed your review feedback?

Yes I was surprised you took the suggestion here and nowhere else.

It is not allowed to call ApproximateBestBlockDepth when the value is not set. Also, there is no code path where it is not set. I don't think it makes sense to bubble up errors that can never happen. In any case, I am not changing any behavior in this commit, so changing ApproximateBestBlockDepth seems out of scope.

Thanks, I didn't realize there was no code path where m_best_block_time could be read before it is set. When I made my comment I was assuming that is was intended for ApproximateBestBlockDepth to return a really large value in this case so NETWORK_LIMITED would not be used.

Suggestion: The fact that m_best_block_time does always get set to a different value is not obvious, and I think there should at least be a comment here like "// default value is never used" to be clear that it's a bug if this value does get used.

I also think it could be safer to make this field std::optional and assert that it is set before it's used, but absent that, just having a comment would be helpful for making this not look like it is trying to return garbage.

maflcko commented at 7:45 PM on July 16, 2026:

I don't disagree, but this pull request is already too large, and this commit is a pure refactor (after compilation the asm will likely be identical before and after this commit), so I don't want to jam in more unrelated fixups.

Also, peerman isn't even used before connman is started, so simply constructing peerman with the correct value avoids the use of optional (and comment), but again, this pull is already too large to add more changes to it.

in src/addrman.cpp:551 in fa815059fc outdated

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

maflcko commented at 6:42 AM on July 9, 2026:

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.

I am confident that everyone knows that epoch == zero == default ctor. If they don't, it would be faster to look it up, than it takes me to write this comment.

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.

No, the intent is not clearer, and zero is not arbitrary here. zero is embedded into the code and into the serialization format. Idk, I'd prefer to leave this as-is, but I can introduce an ADDRMAN_MAGIC_ZERO = NodeSeconds{0s}; // used for internal corruption checks.

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.

No, the docstring says that a magic zero should not be used, so the presence of the constant discourages use of the magic zero. Also, it is not harder to remove the constant: It is a 3-line scripted diff before and after the changes here to remove it, if anyone wanted to do that.

ryanofsky commented at 4:44 PM on July 16, 2026:

re: #35315 (review)

I am confident that everyone knows that epoch == zero == default ctor. If they don't, it would be faster to look it up, than it takes me to write this comment.

I'm not sure how they would know that. epoch is a formerly undocumented, nonstandard constant that is set in one clock type but not others. And even though people can figure out epoch means 0, It is much more obvious that 0 means 0.

No, the intent is not clearer, and zero is not arbitrary here. zero is embedded into the code and into the serialization format. Idk, I'd prefer to leave this as-is, but I can introduce an ADDRMAN_MAGIC_ZERO = NodeSeconds{0s}; // used for internal corruption checks.

Again using 0 to represent 0 would be recommended.

No, the docstring says that a magic zero should not be used, so the presence of the constant discourages use of the magic zero.

Yeah we are talking past each other here. If 0 needs to be used, a literal 0 is a good way to represent it because it looks like a magic value and is a magic value.

Fortunately 0 does not need to be used most places, and new code should avoid using it by using min/max/optional instead. The one exception might be serialization code, but in that case the 0 can be written in the serialization logic and does not need to affect the way times are represented in memory.

An epoch constant makes bad code look less bad and encourages more code based on it to be written. More uses of the constant also make the constant harder to remove, even with a scripted diff.

Suggestion: Obviously my preferred alternative would be to use 0 instead of epoch in the code, but I at least think you should update the commit to contain some positive explanation of why you think the constant is good to use, not just say it "should be allowed" and "Code may reference this constant." When you are allowing the constant to be used more places, and using it more places, it seems like you should be able to describe what motivated you to do these things.

maflcko commented at 7:45 PM on July 16, 2026:

Looks like a third thread was started to bike-shed the color of zero. Closing this one for now, and let's move discussion there: #35315 (review)

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 outdated

  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.

maflcko commented at 3:43 PM on July 9, 2026:

Sure, modified the commit messages a bit.

maflcko force-pushed on Jul 9, 2026

maflcko commented at 3:44 PM on July 9, 2026: member

Thx for the review, replied to all threads and force pushed.

maflcko force-pushed on Jul 9, 2026

DrahtBot added the label CI failed on Jul 9, 2026

DrahtBot commented at 4:10 PM on July 9, 2026: contributor

🚧 At least one of the CI tasks failed. Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/29028565223/job/86154858915 LLM reason (✨ experimental): CI failed due to a C++ build error: net_processing.cpp fails to compile because current_time is an undeclared identifier (clang use of undeclared identifier).

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

DrahtBot removed the label CI failed on Jul 9, 2026

stickies-v commented at 7:25 PM on July 15, 2026: contributor

Approach ACK for improved type safety in time handling, this makes the code easier to read and harder to make mistakes.

I think the epoch approach is sensible, as it is now documented that it should not be used in new code, and I personally find that (with the docstring) it makes things more clear (as someone not super familiar with p2p).

meta-nit: I think the current commit structure is reasonable, but I would have preferred having separate commits for refactoring changes (i.e. changing the type) and for behaviour changes (i.e. changing the default, using min/max etc). Not a blocker, it's doable as-is, but it would speed the (re-)review cycle up for me.

meta-nit: faba25132928b8bdce526957d3b311fca3e75faf should not have refactor in its title. This new behaviour looks benign to me too, but I think we should highlight that it has behaviour change.

maflcko force-pushed on Jul 16, 2026

maflcko commented at 9:57 AM on July 16, 2026: member

Thanks for the approach review! I've:

Restored the initial version of this pull request from three weeks ago, which had three code-acks.
I removed the minor log behavior change Use time_point::min() over epoch-zero for m_last_block_announcement. This pull is meant to be a refactor.
I've pushed the min/max refactoring commits on top. This should make (re-)review easier. Though, happy to drop them, and submit them later.
Replaced the overly broad "refactor:" with "p2p:" in the two seconds->nanoseconds changes.

in src/util/time.h:38 in fa3ed0cf09 outdated

  34 | +
  35 | +    /// Default value assigned to a time point. Since C++20 this is guaranteed
  36 | +    /// to be the unix epoch time.
  37 | +    ///
  38 | +    /// Code may reference this constant and treat it as special, if an unset
  39 | +    /// variable should be represented as zero. For example, when the time

ryanofsky commented at 12:27 PM on July 16, 2026:

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

This seems like bad advice and the previous comment in fa815059fc1a4eabbd81d586b8e515315eba5ec3 was better. The problem is:

if an unset variable should be represented as zero

An unset variable should not be represented as zero. That is a footgun and a cause of bugs. If you think there are cases where this is appropriate I'd like to know what they are, and this comment should be specific about what they are.

Log messages and RPCs should not be using this constant if they want to display or return zero when literal zeroes would be clearer. Also they are probably better off writing "unset" or null than using zeroes at all. C++ code is better off using std::optional, or min/max times when the goal is to represent the last time something happened, or the next time it should happen.

I still believe that epoch is a non-standard, error-prone constant should be removed entirely. Zeroes are not a good way represent unset times. In cases where they can't be avoided, literal zeros can be used and would preferable for transparency, and to make intent clear because the internal clock epoch time should not be relevant to application logic. Alternately, if there is some reason we don't want to write zeroes literally, we could introduce a constant like NODE_UNSET_TIME which would also express intent more clearly and also make bad code stand out.

Suggestion would be to drop this commit (preferred) or to improve the comment. I think earlier suggested comment could fit well here.

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

maflcko commented at 7:46 PM on July 16, 2026:

I think all other reviewers prefer the constant, so I won't be dropping it. I can add the words Old code ... and New code ..., or some other wording, if I have to re-touch.

About the RPC/logs: Yes it is possible to change

TicksSinceEpoch<seconds>(field_with_zero_default)

to

field_with_min_default == NodeClock::min() ? 0 : TicksSinceEpoch<seconds>(field_with_min_default)

or to

TicksSinceEpoch<seconds>(opt_field.value_or(NodeClock::epoch))

, but this would have to be separate commit and this pull is already too large to include that here.

Also, breaking behavior changes like switching 0 to null or "unset" in RPC are not allowed in this refactor-only PR.

I am happy to review a later pull, if someone creates one.

maflcko commented at 7:43 AM on July 21, 2026:

to improve the comment

Thx, minimally improved docstring.

in src/net_processing.cpp:5402 in fa6f2e9927 outdated

5398 | @@ -5399,7 +5399,8 @@ void PeerManagerImpl::EvictExtraOutboundPeers(NodeClock::time_point now)
5399 |                  // block from.
5400 |                  CNodeState &state = *State(pnode->GetId());
5401 |                  if (now - pnode->m_connected > MINIMUM_CONNECT_TIME && state.vBlocksInFlight.empty()) {
5402 | -                    LogDebug(BCLog::NET, "disconnecting extra outbound peer=%d (last block announcement received at time %d)\n", pnode->GetId(), oldest_block_announcement);
5403 | +                    LogDebug(BCLog::NET, "disconnecting extra outbound peer=%d (last block announcement received at time %d)",

ryanofsky commented at 3:10 PM on July 16, 2026:

In commit "p2p: Use NodeClock::time_point for m_last_block_announcement" (fa6f2e992785b88670a9c6d78078fc73c866e02e)

Not important, but this is also dropping \n, and I think it's good for commit messages to note when they make unrelated changes, so it's clear they are intentional. There is also another \n kept immediately below.

maflcko commented at 7:47 PM on July 16, 2026:

The \n is not needed by the logger. May adjust the commit message if I have to re-touch.

maflcko commented at 7:43 AM on July 21, 2026:

thx, changed commit msg

in src/net_processing.cpp:830 in cccc523550 outdated

 826 | @@ -827,7 +827,7 @@ class PeerManagerImpl final : public PeerManager
 827 |  
 828 |      uint32_t GetFetchFlags(const Peer& peer) const;
 829 |  
 830 | -    std::map<uint64_t, std::chrono::microseconds> m_next_inv_to_inbounds_per_network_key GUARDED_BY(g_msgproc_mutex);
 831 | +    std::map<uint64_t, NodeClock::time_point> m_next_inv_to_inbounds_per_network_key GUARDED_BY(g_msgproc_mutex);

ryanofsky commented at 3:17 PM on July 16, 2026:

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

Could mention this field in commit message

maflcko commented at 7:47 PM on July 16, 2026:

May adjust the commit message if I have to re-touch.

maflcko commented at 7:43 AM on July 21, 2026:

thx, changed commit msg

in src/net_processing.cpp:1355 in fa3c6bae01

1351 | @@ -1352,7 +1352,7 @@ bool PeerManagerImpl::TipMayBeStale()
1352 |  
1353 |  int64_t PeerManagerImpl::ApproximateBestBlockDepth() const
1354 |  {
1355 | -    return (GetTime<std::chrono::seconds>() - m_best_block_time.load()).count() / m_chainparams.GetConsensus().nPowTargetSpacing;
1356 | +    return (Now<NodeSeconds>() - m_best_block_time.load()).count() / m_chainparams.GetConsensus().nPowTargetSpacing;

stickies-v commented at 5:14 PM on July 16, 2026:

in fa3c6bae01bdf472404612d492c81219f6c4f796:

orthogonal, but while touching, would be good to use TicksSeconds here to guard against errors when m_best_block_time changes its duration type?

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index bc7f807ea5..9efe02fa69 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1352,7 +1352,7 @@ bool PeerManagerImpl::TipMayBeStale()
 
 int64_t PeerManagerImpl::ApproximateBestBlockDepth() const
 {
-    return (Now<NodeSeconds>() - m_best_block_time.load()).count() / m_chainparams.GetConsensus().nPowTargetSpacing;
+    return TicksSeconds(Now<NodeSeconds>() - m_best_block_time.load()) / m_chainparams.GetConsensus().nPowTargetSpacing;
 }
 
 bool PeerManagerImpl::CanDirectFetch()

</details>

maflcko commented at 7:43 AM on July 21, 2026:

Thx, done, but with a different patch

ryanofsky approved

ryanofsky commented at 5:50 PM on July 16, 2026: contributor

Code review ACK fa146ffed41a5355d1374b4c4f912f3701e0beec. Nice improvement using the time point type to represent times, instead of duration types or integers.

Unfortunately, it looks like uses of the epoch constant are still increasing from 10 to 26 in this pull, but this is better than the 44 uses in previous versions.

I left some new suggestions, but as always feel free to ignore them. Thanks for all the updates!

DrahtBot requested review from stickies-v on Jul 16, 2026

in src/net_processing.cpp:5930 in fa146ffed4

stickies-v commented at 4:36 PM on July 20, 2026:

nit: "maintain" would now require nanoseconds. No practical implications either way, but might be slightly less confusion to not have a different duration here?

// Convert HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER to nanoseconds before scaling
// to maintain precision
std::chrono::nanoseconds{HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER} *

maflcko commented at 7:43 AM on July 21, 2026:

I mean this is pretty pointless, the base timeout is 15 minutes and the per-header timeout-diff is 1ms, so even with a 1 week-old best header, we are bike-shedding about 15min1.008sec vs 15min1.007sec.

My preference would be to just remove this useless "precision" code, or use SecondsDouble for "precision".

edit: Miners can (and do?) "wiggle" the time here by more than one pow-target-spacing anyway, so implying a greater precision here seems confusing?

stickies-v commented at 10:35 AM on July 21, 2026:

My preference would be to just remove this useless "precision" code

I agree, it seems unnecessary and confusing.

stickies-v commented at 8:57 PM on July 20, 2026: contributor

Reviewed fa146ffed41a5355d1374b4c4f912f3701e0beec

Code LGTM but want to give it another review round to double check potential behaviour change. Left a few comments, but no blockers.

DrahtBot requested review from stickies-v on Jul 20, 2026

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.

Also, add a docstring.

fa562e4c3d

refactor: Use NodeSeconds for m_best_block_time

Previously, it was using a duration type.

fa208c5b24

p2p: Use NodeClock::time_point for m_last_block_announcement

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

Externally, this is a refactor. Internally, this is a minimal behavior
improvement: The value tracks the last *new* block announcement. It
should be rare for more than one new block to be announced in the same
second. Even if several new blocks were announced in the same second,
the value is only used in EvictExtraOutboundPeers. 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.

Nit note: The removed \n char in the log is not needed (refactor).

fafd1e3312

refactor: Use NodeClock::time_point in txdownloadman/txrequest

This may minimally increase the precision from µs to ns, but the
behavior does not change in this refactor.

fa593c2e6e

maflcko force-pushed on Jul 21, 2026

DrahtBot added the label CI failed on Jul 21, 2026

DrahtBot removed the label CI failed on Jul 21, 2026

p2p: Use NodeClock::time_point instead of std::chrono::seconds in node stats and eviction

Externally, this is a refactor, because stats reported by RPC do not
change.

Internally, this is a minimal behavior improvement: With the precision
increase from seconds to nanoseconds, SelectNodeToEvict will be more
precise picking the protected peers with CompareNodeBlockTime and
CompareNodeTXTime.

fab33fdd92

refactor: Drop HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER "precision"

The base timeout is 15 minutes, so trying to imply that a rounding error
of 1ms is relevant seems confusing.

Fix the confusion by using the integer division truncation to round down
to the next ms.

fa4d22d06c

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

for fields:

* m_headers_sync_timeout
* m_downloading_since
* m_next_send_feefilter
* m_next_addr_send
* m_next_local_addr_send
* m_next_inv_send_time
* m_next_inv_to_inbounds_per_network_key
* m_stalling_since

This patch may minimally increase the precision from µs to ns, but the
behavior does not change in this refactor.

fa42ebd0d1

refactor: Use time_point::max() for m_headers_sync_timeout

This refactor does not change any behavior. It uses a single natural
special in-band sentinel values instead of two different ones:
epoch-zero and time_point::max().

Review help:

fSyncStarted gates all uses of the field, so the zero-epoch default is
irrelevant. time_point::max() used below is the meaningful sentinel
here, representing a disabled timeout after sync has started, so use
that value consistently.

fa8a149c29

refactor: Use time_point::min()/max() in net_processing

This refactor does not change any behavior. It uses a more natural
special in-band sentinel values instead of epoch-zero.

Fields with time_point::min() as new sentinel value:
* m_downloading_since
* m_next_send_feefilter
* m_next_addr_send
* m_next_inv_send_time
* m_next_local_addr_send

Fields with time_point::max() as new sentinel value:
* m_stalling_since

fa58d8f4fb

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

maflcko force-pushed on Jul 21, 2026

in src/net_processing.cpp:313 in fab4bd7f2b

 309 | @@ -310,7 +310,7 @@ struct Peer {
 310 |          bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
 311 |          /** The next time after which we will send an `inv` message containing
 312 |           *  transaction announcements to this peer. */
 313 | -        std::chrono::microseconds m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){0};
 314 | +        NodeClock::time_point m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){NodeClock::time_point::min()};

stickies-v commented at 1:52 PM on July 23, 2026:

Would be useful to document all sentinels:

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index d1564766fb..903f6dadda 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -309,7 +309,8 @@ struct Peer {
          *  NODE_BLOOM. See BIP35. */
         bool m_send_mempool GUARDED_BY(m_tx_inventory_mutex){false};
         /** The next time after which we will send an `inv` message containing
-         *  transaction announcements to this peer. */
+         *  transaction announcements to this peer, or time_point::min() when
+         *  the version handshake is not yet completed. */
         NodeClock::time_point m_next_inv_send_time GUARDED_BY(m_tx_inventory_mutex){NodeClock::time_point::min()};
         /** The mempool sequence num at which we sent the last `inv` message to this peer.
          *  Can relay txs with lower sequence numbers than this (see CTxMempool::info_for_relay). */
@@ -366,7 +367,8 @@ struct Peer {
     mutable Mutex m_addr_send_times_mutex;
     /** Time point to send the next ADDR message to this peer. */
     NodeClock::time_point m_next_addr_send GUARDED_BY(m_addr_send_times_mutex){NodeClock::time_point::min()};
-    /** Time point to possibly re-announce our local address to this peer. */
+    /** Time point to possibly re-announce our local address to this peer, or
+     *  time_point::min() if no self-announcement was sent to this peer yet. */
     NodeClock::time_point m_next_local_addr_send GUARDED_BY(m_addr_send_times_mutex){NodeClock::time_point::min()};
     /** Whether the peer has signaled support for receiving ADDRv2 (BIP155)
      *  messages, indicating a preference to receive ADDRv2 instead of ADDR ones. */
@@ -403,7 +405,8 @@ struct Peer {
     /** Whether we've sent our peer a sendheaders message. **/
     std::atomic<bool> m_sent_sendheaders{false};
 
-    /** When to potentially disconnect peer for stalling headers download */
+    /** When to potentially disconnect peer for stalling headers download, or
+     *  time_point::max() if this peer is exempt from the timeout. */
     NodeClock::time_point m_headers_sync_timeout GUARDED_BY(NetEventsInterface::g_msgproc_mutex){NodeClock::time_point::max()};
 
     /** Whether this peer wants invs or headers (when possible) for block announcements */
@@ -482,7 +485,8 @@ struct CNodeState {
       * drop the outbound one that least recently announced us a new block.
       */
     struct ChainSyncTimeoutState {
-        //! A timeout used for checking whether our peer has sufficiently synced
+        //! A timeout used for checking whether our peer has sufficiently synced,
+        //! or NodeClock::epoch when unset
         NodeClock::time_point m_timeout{NodeClock::epoch};
         //! A header with the work we require on our peer's chain
         const CBlockIndex* m_work_header{nullptr};
@@ -959,7 +963,7 @@ private:
     typedef std::multimap<uint256, std::pair<NodeId, std::list<QueuedBlock>::iterator>> BlockDownloadMap;
     BlockDownloadMap mapBlocksInFlight GUARDED_BY(cs_main);
 
-    /** When our tip was last updated. */
+    /** When our tip was last updated, or NodeClock::epoch for no update. */
     std::atomic<NodeClock::time_point> m_last_tip_update{NodeClock::epoch};
 
     /** Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). */

</details>

in src/net_processing.cpp:1356 in fab4bd7f2b

1355 |  }
1356 |  
1357 |  int64_t PeerManagerImpl::ApproximateBestBlockDepth() const
1358 |  {
1359 | -    return (GetTime<std::chrono::seconds>() - m_best_block_time.load()).count() / m_chainparams.GetConsensus().nPowTargetSpacing;
1360 | +    return (Now<NodeSeconds>() - m_best_block_time.load()) / m_chainparams.GetConsensus().PowTargetSpacing();

stickies-v commented at 4:55 PM on July 23, 2026:

nit: any reason we use Node<NodeSeconds>() here instead of NodeClock::now()?

stickies-v approved

stickies-v commented at 6:09 PM on July 23, 2026: contributor

ACK fab4bd7f2bf8eb78efff00a6d1abcb28187e0063

DrahtBot requested review from ryanofsky on Jul 23, 2026

DrahtBot added the label Needs rebase on Jul 25, 2026

DrahtBot commented at 12:14 PM on July 25, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.