test: NodeClockContext follow-ups #35114

seduless commented at 2:54 PM on April 19, 2026: contributor

Follow-up to #34858

Updates remaining SetMockTime call sites that are clean, mechanical swaps fitting the spirit of the original PR (see: #34858#pullrequestreview-4031647119 and #34858 (comment)). Further updates to SetMockTime are more complex and deserve separate, isolated PRs.

The default constructor for NodeClockContext increments to the next tick, which is a defensive measure to prevent time going backwards on construction. This has caused some confusion (see thread: #34858 (review)) and can be safely removed after updating the only test where this is load-bearing (b3c9bd7f2df230525c8e339394a315a2c500055d) (see: #34858 (review)). The removal also tightens the addrman_tests/addrman_evictionworks test to sit exactly on the ADDRMAN_REPLACEMENT boundary (4h), catching mutations such as:

diff --git a/src/addrman.cpp b/src/addrman.cpp
index d3dae59ae7..d0929c62cb 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -920,3 +920,3 @@ void AddrManImpl::ResolveCollisions_()
                 // Has successfully connected in last X hours
-                if (current_time - info_old.m_last_success < ADDRMAN_REPLACEMENT) {
+                if (current_time - info_old.m_last_success <= ADDRMAN_REPLACEMENT) {
                     erase_collision = true;

The last follow-up item is updating NodeClockContext to FakeNodeClock to make it clear it is intended for testing (motivated by #34858#pullrequestreview-4082110904 and supported in #34858 (comment)).

DrahtBot added the label Tests on Apr 19, 2026

DrahtBot commented at 2:55 PM on April 19, 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/35114.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, sedited
Stale ACK	kevkevinpal, frankomosh

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:

#35414 (iwyu: Fix warnings in src/bench and treat them as error by hebasto)
#35351 (net: Disallow invalid HeadersSyncState due to lagging clock by hodlinator)
#35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)

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.

in src/test/denialofservice_tests.cpp:255 in 4117fd482d

 251 | @@ -252,6 +252,7 @@ BOOST_FIXTURE_TEST_CASE(block_relay_only_eviction, OutboundTest)
 252 |      options.m_max_automatic_connections = DEFAULT_MAX_PEER_CONNECTIONS;
 253 |  
 254 |      connman->Init(options);
 255 | +    NodeClockContext clock_ctx{}; // before peer creation so m_connected is deterministic

maflcko commented at 10:42 AM on April 20, 2026:

nit in the first commit: Maybe move this after NodeId id{0}; in the second line? This makes it clearer, because readers and reviewers don't have to think about why this test case is mixing real time and fake time for the initial part.

seduless commented at 10:39 AM on April 23, 2026:

I agree, that does make it easier to reason about. Moved to after NodeId id{0} and dropped the comment since the placement is self-documenting now.

in src/test/addrman_tests.cpp:101 in a4ec73ba42

  97 | @@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
  98 |  BOOST_AUTO_TEST_CASE(addrman_terrible_many_failures)
  99 |  {
 100 |      auto now = Now<NodeSeconds>();
 101 | -    SetMockTime(now - (ADDRMAN_MIN_FAIL + 24h));
 102 | +    NodeClockContext clock_ctx{now - (ADDRMAN_MIN_FAIL + 24h)};

maflcko commented at 10:46 AM on April 20, 2026:

nit: Should it not be possible to remove the now variable by using:

NodeClockContext clock_ctx{};

....

clock_ctx += ADDRMAN_MIN_FAIL + 24h;

?

seduless commented at 10:39 AM on April 23, 2026:

Nice suggestion, thanks for flagging. I included this change in the latest push and verified that only the relative time gap matters for this test.

in src/test/util/time.h:48 in 068d3a28f4

  43 | @@ -44,8 +44,8 @@ class NodeClockContext
  44 |      /// Initialize with the given time.
  45 |      explicit NodeClockContext(NodeSeconds init_time) { set(init_time); }
  46 |      explicit NodeClockContext(std::chrono::seconds init_time) { set(init_time); }
  47 | -    /// Initialize with current time, using the next tick to avoid going back by rounding to seconds.
  48 | -    explicit NodeClockContext() { set(++Now<NodeSeconds>().time_since_epoch()); }
  49 | +    /// Initialize with current time.
  50 | +    explicit NodeClockContext() { set(Now<NodeSeconds>().time_since_epoch()); }

maflcko commented at 10:59 AM on April 20, 2026:

nit: Was thinking that this could go into a more extreme self-checking direction. It could advance the time on constructions by 100h. This way, it should catch bugs, where the context is constructed too late. For example, it may detect the "strictness bug" fixed in the prior commit.

Diff:

diff --git a/src/test/util/time.h b/src/test/util/time.h
index 8cd9a1dafc..527014f56b 100644
--- a/src/test/util/time.h
+++ b/src/test/util/time.h
@@ -47,3 +47,3 @@ public:
     /// Initialize with current time, using the next tick to avoid going back by rounding to seconds.
-    explicit NodeClockContext() { set(++Now<NodeSeconds>().time_since_epoch()); }
+    explicit NodeClockContext() { set(100h+Now<NodeSeconds>().time_since_epoch()); }

Output:

$ ./bld-cmake/bin/test_bitcoin -t denialofservice_tests
Running 5 test cases...
test/denialofservice_tests.cpp(101): error: in "denialofservice_tests/outbound_slow_chain_eviction": check !to_send.empty() has failed

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

However, this points to another "bug" in 27 - chainstate_write_tests (Failed), which I presume is due to some leftover init state from TestingSetup. I presume it would be possible to fix by providing a -mocktime=.. startup option in this unit test, or by creating an optional NodeClockContext member field inside the TestingSetup, which is set in this unit test, but all of this sounds tedious, so up to you, and this is just a nit.

I only wanted to share some more thoughts on this commit, but anything is fine here: Dropping the commit, and leaving this as-is, Increasing the time with some other fixes, or tolerating the time to go backwards.

seduless commented at 10:39 AM on April 23, 2026:

This is a neat idea. I tried both +100h and -100h and they surface different failures. +100h catches the failures you mentioned, but the hoist fix in the first commit only surfaced with -100h. I'd prefer to keep the constructor simple for now since it's not obvious which direction to pick.

maflcko commented at 10:03 AM on May 16, 2026:

nit in 0756e53e13e0e04308ec4bc3683ac927f0c1430e: If you drop the ++operator, the time_since_epoch can also be dropped. It was only needed to work around a stdlib bug that I fixed in https://github.com/llvm/llvm-project/commit/a17e97e6778b2cd4114052faf6ee25db330ef405

seduless commented at 6:01 PM on June 8, 2026:

Thanks for the context, dropped it.

maflcko approved

maflcko commented at 10:59 AM on April 20, 2026: member

lgtm. I just left some nits, but feel free to ignore those

seduless force-pushed on Apr 23, 2026

seduless commented at 10:42 AM on April 23, 2026: contributor

Thanks for the review! Addressed the placement and addrman feedback and responded to the constructor offset idea in the thread.

kevkevinpal commented at 12:49 PM on April 29, 2026: contributor

ACK a236a46

I reviewed the commits before the scripted diff, and they look good. The scripted diff seems to work well and is a straightforward swap.

One nit is that I think it makes more sense to rename NodeClockContext to MockNodeClock instead of FakeNodeClock since we use SetMockTime within the class and also some of the comments in the class mention mock time.

The word "Fake" feels off to me. Nonetheless, this is not blocking

frankomosh commented at 7:50 AM on May 8, 2026: contributor

tACK a236a4649a02003ce7a374f0f1789c762eb8f7b5. Full test suite runs clean. Also verified the mutation described in the PR description < to <= on addrman.cpp:921, is correctly caught by addrman_evictionworks

DrahtBot added the label CI failed on May 16, 2026

DrahtBot commented at 10:03 AM on May 16, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task previous releases: https://github.com/bitcoin/bitcoin/actions/runs/24830622845/job/76310796568</sub> <sub>LLM reason (✨ experimental): CI failed due to a C++ build error compiling the fuzz target (src/test/fuzz/cmpctblock.cpp), where NodeClockContext/clock_ctx were undefined.</sub>

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

in src/test/rpc_tests.cpp:358 in 99b33f2dc5 outdated

 354 | @@ -355,7 +355,7 @@ BOOST_AUTO_TEST_CASE(rpc_ban)
 355 |      const int64_t ban_duration{o1.find_value("ban_duration").getInt<int64_t>()};
 356 |      const int64_t time_remaining{o1.find_value("time_remaining").getInt<int64_t>()};
 357 |      BOOST_CHECK_EQUAL(adr.get_str(), "127.0.0.0/24");
 358 | -    BOOST_CHECK_EQUAL(banned_until, time_remaining_expected + now.count());
 359 | +    BOOST_CHECK_EQUAL(banned_until, time_remaining_expected + Now<NodeSeconds>().time_since_epoch().count());

maflcko commented at 10:11 AM on May 16, 2026:

nit in 99b33f2dc59dd10695d590b59d1727d1b2969e27: Sorry for missing this earlier, but according to the docs of TicksSinceEpoch , this should use TicksSinceEpoch<...>(NodeClock::now()) instead?

maflcko commented at 10:13 AM on May 16, 2026:

An alternative would be:

    BOOST_CHECK_EQUAL(NodeSeconds{banned_until}, time_remaining_expected + Now<NodeSeconds>());

(The general idea is to avoid count(), where possible)

seduless commented at 6:01 PM on June 8, 2026:

Nice catch, updated to use TicksSinceEpoch.

maflcko approved

maflcko commented at 10:21 AM on May 16, 2026: member

everything lgtm, but I guess this needs rebase.

Also, I've seen the context being misused in other pull requests, so maybe it could make sense to enforce there is only one active one at any point in time? Something like this shoudl work:

diff --git a/src/test/util/time.h b/src/test/util/time.h
index 8cd9a1dafc..bde2258618 100644
--- a/src/test/util/time.h
+++ b/src/test/util/time.h
@@ -10,2 +10,16 @@
 
+/// CRTP Helper to limit a class to at most one at a time.
+template <class T>
+class LimitOne
+{
+public:
+    LimitOne() { Assert(g_T_available) = false; }
+    ~LimitOne() { g_T_available = true; }
+    LimitOne(const LimitOne&) = delete;
+    LimitOne& operator=(const LimitOne&) = delete;
+
+private:
+    static inline bool g_T_available{true};
+};
+
 
@@ -13,3 +27,3 @@
 /// and reset it after use in a test.
-class SteadyClockContext
+class SteadyClockContext : public LimitOne<SteadyClockContext>
 {
@@ -38,3 +52,3 @@ public:
 /// and reset it after use in a test.
-class NodeClockContext
+class NodeClockContext : public LimitOne<NodeClockContext>
 {

Otherwise:

review ACK a236a4649a02003ce7a374f0f1789c762eb8f7b5 🎯

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a236a4649a02003ce7a374f0f1789c762eb8f7b5 🎯
VMm6UpXnOozje+1LiXQg76D5mtMSOmMtSV7WTFSVbERMhetZIMF0QG5uWNVq8RE314nPnR1ck5BrPuUIJf9HCw==

</details>

fanquake closed this on May 18, 2026

fanquake reopened this on May 18, 2026

sedited commented at 9:17 PM on May 21, 2026: contributor

@seduless can you take a look at the CI failures here?

DrahtBot added the label Needs rebase on May 23, 2026

fanquake commented at 2:14 PM on June 1, 2026: member

@seduless are you still working on this?

fanquake marked this as a draft on Jun 1, 2026

seduless commented at 3:07 AM on June 3, 2026: contributor

@fanquake Thanks for the ping, I'll rebase and work through the open comments.

test: Enter mocktime before peer creation in block_relay_only_eviction

This is a follow-up to commit faad08e59c4419e09eb75054bf468ca98a837ca8.
Hoisting the NodeClockContext above peer creation ensures m_connected is
captured under mocktime, making the MINIMUM_CONNECT_TIME check
deterministic regardless of which peer is selected for eviction.

This is a prerequisite for the next commit, which removes the
one-second advance from the NodeClockContext default constructor.

7c2ec3949a

test: Drop ++ from NodeClockContext default constructor

The increment was originally added so that mocked time would not appear
to go backward relative to real-clock timestamps captured before
construction, since Now<NodeSeconds>() rounds the current time down to
a whole second. In practice the tests do not mix real and mocked
timestamps in a way that exposes this, so the increment is unnecessary.

758fea59a8

test: Use NodeClockContext in more call sites

This refactor is a follow-up to commit
faad08e59c4419e09eb75054bf468ca98a837ca8 and does not
change any behavior.

These call sites are clean mechanical swaps. The remaining ones
require non-trivial test refactoring and are left for future
follow-ups.

1e9546fcf4

scripted-diff: Rename NodeClockContext to FakeNodeClock

The previous name did not indicate the type was intended for
testing. Renaming to FakeNodeClock makes this explicit and
allows call sites to drop the ctx suffix on the variable name.

Suggested in #34858 review feedback.

-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- src | xargs sed -i "s/$1/$2/g"; }

s '\<NodeClockContext\>' 'FakeNodeClock'
s '\<clock_ctx\>'        'clock'
-END VERIFY SCRIPT-

55e402ffef

test: Limit clocks to one active instance

SteadyClockContext and FakeNodeClock assume they are the only active
instance. Overlapping them in the same scope would silently clobber
each other.

Add a CRTP base class, LimitOne, that asserts at construction if
another instance already exists.

35a814a045

seduless force-pushed on Jun 8, 2026

DrahtBot removed the label Needs rebase on Jun 8, 2026

DrahtBot removed the label CI failed on Jun 8, 2026

seduless marked this as ready for review on Jun 8, 2026

seduless commented at 6:01 PM on June 8, 2026: contributor

Rebased and addressed all open comments, ready for another round of review.

re: #35114 (comment)

One nit is that I think it makes more sense to rename NodeClockContext to MockNodeClock instead of FakeNodeClock since we use SetMockTime within the class and also some of the comments in the class mention mock time.

Thanks for the review! Since no one feels strongly about the rename I'd prefer to leave it as-is. Happy to update though if others feel strongly.

re: #35114#pullrequestreview-4303514147

Also, I've seen the context being misused in other pull requests, so maybe it could make sense to enforce there is only one active one at any point in time? Something like this shoudl work

Thanks for the diff. I agree that's a useful improvement to remove the footgun and I applied the diff with the FakeNodeClock name and verified it catches misuse at runtime.

re: #35114 (comment)

@seduless can you take a look at the CI failures here?

CI broke because the cmpctblock fuzz harness (#33300) landed on master using NodeClockContext, which the scripted-diff rename didn't catch since that file didn't exist when this branch was pushed. Updating the name during rebase fixes it.

maflcko commented at 9:02 AM on June 9, 2026: member

Only change is addressing nits, rebase, and adding a new commit.

re-ACK 35a814a045fb1b90801ab8781db5da7c39099ea3 🛒

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 35a814a045fb1b90801ab8781db5da7c39099ea3 🛒
9JSPsOX3CkRc76GDj7G17GLJGhnW5z4VfKWCfZIWpvBXnpke5H8x1jr1MynDDe42FPGApkNQFJtH5L0WYFGnBg==

</details>

DrahtBot requested review from frankomosh on Jun 9, 2026

sedited approved

sedited commented at 12:38 PM on June 9, 2026: contributor

ACK 35a814a045fb1b90801ab8781db5da7c39099ea3

sedited merged this on Jun 9, 2026

sedited closed this on Jun 9, 2026

seduless deleted the branch on Jun 9, 2026

maflcko commented at 7:41 PM on June 9, 2026: member

Followed-up for the remaining unit-test calls in https://github.com/bitcoin/bitcoin/pull/35497