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	kevkevinpal, frankomosh, maflcko

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:

#35208 (net: cap future-MTP headers commitments by l0rinc)
#35141 (fuzz: apply node context reset pattern to p2p_handshake by frankomosh)
#35016 (net: deduplicate private broadcast state and snapshot types by takeshikurosawaa)

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

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

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.

b3c9bd7f2d

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.

0756e53e13

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.

99b33f2dc5

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-

a236a4649a

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

 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)

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>