test: Use NodeClockContext in more tests #34858

maflcko commented at 8:09 AM on March 19, 2026: member

Currently mocktime is written to a global, which may leak between sub-tests (albeit some tests try to reset the mocktime on a best-effort basis). Also, when advancing it, one has to keep a counter variable around.

Fix both issues by using the recently added NodeClockContext, which resets the mocktime once it goes out of scope. Also, it has a method to advance the mocktime by a delta.

DrahtBot renamed this:
~~test: Use NodeClockContext in more tests~~
test: Use NodeClockContext in more tests
on Mar 19, 2026

DrahtBot added the label Tests on Mar 19, 2026

DrahtBot commented at 8:09 AM on March 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.

Type	Reviewers
ACK	seduless, frankomosh, ryanofsky, achow101

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:

#35003 (net_processing: improve block data I/O error handling in P2P paths by furszy)
#34360 (bench: add WalletBalanceManySpent for high-history wallet scenario by w0xlt)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)

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.

test: Allow time_point in boost checks

This is required in the next commit.

fa9f434df8

fuzz: Use NodeClockContext

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

However, it is nice to know that no global mocktime leaks from the fuzz
init step to the first fuzz input, or from one fuzz input execution to
the next.
With the clock context, the global is re-set at the end of the context.

fa8fe0941e

test: Use NodeClockContext in more tests faad08e59c

maflcko force-pushed on Mar 20, 2026

seduless commented at 4:00 PM on March 30, 2026: contributor

Tested ACK faad08e59c4419e09eb75054bf468ca98a837ca8

nit: First commit message says "required in the next commit" but it's the third commit that needs it (the time_point comparison in testnet4_miner_tests).

No need to expand scope, but these are the remaining SetMockTime call sites that seem to fit the spirit of these changes. Happy to pick these up in a follow-up PR:

chainstate_write_tests.cpp:91
addrman_tests.cpp:101,112,135
private_broadcast_tests.cpp:29,120
rpc_tests.cpp:345,347
bench/index_blockfilter.cpp:42
bench/wallet_encrypt.cpp:55

maflcko commented at 8:09 AM on March 31, 2026: member

Happy to pick these up in a follow-up PR:

Ok, I'll leave this as-is. I guess some of them were merged in between I wrote the diff and now.

frankomosh commented at 8:22 AM on April 9, 2026: contributor

Tested ACK faad08e59c4419e09eb75054bf468ca98a837ca8. Ran all relevant tests, all clean. Also verified that the default-constructor call sites in orphanage_tests and addrman_tests behave identically to the explicit {Now<NodeSeconds>()} form.

nit: Can see several of the new NodeClockContext clock_ctx{} call sites only care about relative advancement, not absolute starting value. Do you think it make sense to add a deterministic alternative starting at a fixed non-zero epoch, something like NodeClockContext::from_epoch(), so tests that don't need wall-clock anchoring can express that explicitly?

in src/test/denialofservice_tests.cpp:278 in faad08e59c

 273 | @@ -273,7 +274,8 @@ BOOST_FIXTURE_TEST_CASE(block_relay_only_eviction, OutboundTest)
 274 |      }
 275 |      BOOST_CHECK(vNodes.back()->fDisconnect == false);
 276 |  
 277 | -    SetMockTime(GetTime() + MINIMUM_CONNECT_TIME + 1);
 278 | +    NodeClockContext clock_ctx{};
 279 | +    clock_ctx += MINIMUM_CONNECT_TIME;

ryanofsky commented at 12:01 PM on April 9, 2026:

In commit "test: Use NodeClockContext in more tests" (faad08e59c4419e09eb75054bf468ca98a837ca8)

Looks like +1 has been dropped here. Would seem more conservative to keep unless there is a rationale for dropping it. Even if dropping it doesn't cause test to fail, it could weaken the test and make it not test the condition it was originally trying to check. I didn't look at the test so I'm not sure what the case is.

maflcko commented at 12:51 PM on April 9, 2026:

Thanks for pointing this out, and sorry for mixing this into the commit in that way.

Dropping the +1 makes the test stricter, because the test checks that "The extra peer should only get marked for eviction after MINIMUM_CONNECT_TIME"

The corresponding p2p code is probably:

src/net_processing.cpp-            if (node_state == nullptr ||
src/net_processing.cpp:                (now - pnode->m_connected >= MINIMUM_CONNECT_TIME && node_state->vBlocksInFlight.empty())) {
src/net_processing.cpp-                pnode->fDisconnect = true;
src/net_processing.cpp-                LogDebug(BCLog::NET, "disconnecting extra block-relay-only peer=%d (last block received at time %d)\n",
src/net_processing.cpp-                         pnode->GetId(), count_seconds(pnode->m_last_block_time));
src/net_processing.cpp-                return true;

Happy to restore the +1, or move it into a new commit, but my preference would be to make the test as strict as possible.

seduless commented at 10:42 PM on April 15, 2026:

re: #34858 (review)

Looks like +1 has been dropped here.

Agree with Marco's preference to make the test as strict as possible, however I believe the +1 is still preserved here since we use the default constructor for NodeClockContext, which adds one second to the current time.

https://github.com/bitcoin/bitcoin/blob/faad08e59c4419e09eb75054bf468ca98a837ca8/src/test/util/time.h#L48

maflcko commented at 6:08 AM on April 16, 2026:

NodeClockContext, which adds one second to the current time.

Hmm, nice catch. Though, this is not something to rely on. I added the ++, because I was thinking that the wall clock may proceed to the next second here (or even to the second next second, if the scheduler is really borked). Though, on a second thought, this may be more confusing than helpful, because it likely won't help to trigger any bug. So I guess it could make sense to remove the ++ again.

Also, unrelated from that, to make this test case as strict as possible, the fake node clock context should be created before the connection is created. Otherwise, it may include a non-deterministic and larger delay than intended. Suggested diff:

diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
index 9709eacd8b..875c42ae55 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -266,2 +266,3 @@ BOOST_FIXTURE_TEST_CASE(block_relay_only_eviction, OutboundTest)
 
+    NodeClockContext clock_ctx{};
     // Add an extra block-relay-only peer breaking the limit (mocks logic in ThreadOpenConnections)
@@ -276,3 +277,2 @@ BOOST_FIXTURE_TEST_CASE(block_relay_only_eviction, OutboundTest)
 
-    NodeClockContext clock_ctx{};
     clock_ctx += MINIMUM_CONNECT_TIME;

in src/test/orphanage_tests.cpp:435 in faad08e59c

 431 | @@ -431,9 +432,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
 432 |      FillableSigningProvider keystore;
 433 |      BOOST_CHECK(keystore.AddKey(key));
 434 |  
 435 | -    // Freeze time for length of test
 436 | -    auto now{GetTime<std::chrono::seconds>()};
 437 | -    SetMockTime(now);
 438 | +    NodeClockContext clock_ctx{};

ryanofsky commented at 12:07 PM on April 9, 2026:

In commit "test: Use NodeClockContext in more tests" (faad08e59c4419e09eb75054bf468ca98a837ca8)

Noticed in an earlier test you added a new comment "keep mocktime constant" and here you deleted an existing comment "Freeze time for length of test". Would be good to be consistent. Personally I think the "Freeze time for length of test" comment is helpful and clearer than "keep mocktime constant" but anything seems fine here.

maflcko commented at 12:51 PM on April 9, 2026:

I think they are different. The comment about keeping it constant is only applied to the single place that has a const context: const NodeClockContext clock_ctx{}; // keep mocktime constant.

However, for "normal" use of NodeClockContext clock_ctx{};, there shouldn't be any need for any comment, because ideally the name should be self-explanatory and self-documenting.

Happy to change the name. I see you suggested FakeNodeClock clock_ctx{};. Would that address your concern here?

ryanofsky commented at 1:53 PM on April 9, 2026:

re: #34858 (review)

Thanks for clarifying the difference between the comments, that does makes sense, and I don't have a real opinion or concern here, was just giving a reaction. On naming, I do think FakeNodeClock clock; could be a little clearer and I'd be happy to review any renames, but no strong opinion here either.

maflcko commented at 2:05 PM on April 9, 2026:

Ok, I'll leave as-is for now, here.

ryanofsky approved

ryanofsky commented at 12:24 PM on April 9, 2026: contributor

Code review ACK faad08e59c4419e09eb75054bf468ca98a837ca8 but had a question about dropping +1 in one test below.

Overall NodeClockContext seems like a nice utility and makes tests more readable. Though I don't like the name because there is no indication it is intended for testing. Would have used naming more like FakeNodeClock clock; but probably too late for that.

maflcko commented at 12:51 PM on April 9, 2026: member

nit: Can see several of the new NodeClockContext clock_ctx{} call sites only care about relative advancement, not absolute starting value. Do you think it make sense to add a deterministic alternative starting at a fixed non-zero epoch, something like NodeClockContext::from_epoch(), so tests that don't need wall-clock anchoring can express that explicitly?

Interesting question. I think there are two different answers:

For fuzz tests, I'd say no, because it seems better to explicitly initialize the value in each fuzz test, so that the fuzz test is self-contained and does not depend on a compile-time constant, where it is not immediately clear that this constant value applies to this fuzz test.
For unit test, I'd say no as well. Of course unit tests should also be fully deterministic, and if faking the time to the current wall-clock causes non-determinism, that should be handled for each unit test explicitly. Outside of that, it seems beneficial to use the wall-clock, because then it can also be faked from outside via libfaketime.

This brings me an unrelated idea: In one of the CI boxes, we should advance the time with libfaketime (or otherwise) by a few years, to ensure all tests that are deterministically passing today will likely also be passing deterministically in a few years.

Overall NodeClockContext seems like a nice utility and makes tests more readable. Though I don't like the name because there is no indication it is intended for testing. Would have used naming more like FakeNodeClock clock; but probably too late for that.

I don't think it is too late. I think the merge conflicts mostly stem from the include list changes and otherwise, the unit tests are not merge-conflict heavy. It should be trivial to run a scripted-diff over for this rename. Given that there already is a follow-up queued up, maybe it can be done there?

ryanofsky commented at 1:54 PM on April 9, 2026: contributor

re: #34858#pullrequestreview-4080792268

Do you think it make sense to add a deterministic alternative starting at a fixed non-zero epoch

Agree with Marco a compile-time constant starting point does not seem ideal. But if you are suggesting using a deterministic random starting time (like ConsumeTime() in fuzz tests and RANDOM_CTX_SEED-derived time in unit tests) that does seem better than using the current clock time by default.

libfaketime CI job also sounds like a good idea

achow101 commented at 9:25 PM on April 9, 2026: member

ACK faad08e59c4419e09eb75054bf468ca98a837ca8

achow101 merged this on Apr 9, 2026

achow101 closed this on Apr 9, 2026

maflcko deleted the branch on Apr 10, 2026

maflcko commented at 6:50 AM on April 10, 2026: member

No need to expand scope, but these are the remaining SetMockTime call sites that seem to fit the spirit of these changes. Happy to pick these up in a follow-up PR: ...

(re #34858#pullrequestreview-4031647119)

I think there is at least one more by now, but I haven't grep'd in detail.