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 (4117fd482d2d584d07430524eb486c4a742e5a6d) (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)).

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.

4117fd482d

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.

068d3a28f4

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.

a4ec73ba42

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-

1da1a309cf

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.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)
#33421 (node: add BlockTemplateCache by ismaelsadeeq)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)

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.

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;

?

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.

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