kernel, validation: Add btck_chainstate_manager_set_clock

ryanofsky commented at 2:52 AM on June 18, 2026: contributor

The main commit in this PR is the second commit adding a btck_chainstate_manager_set_clock_time API which lets kernel applications run validation code deterministically without depending on the system clock. This is an alternative to #35496, and one of several alternatives discussed in that PR, with some tradeoffs described in #35496 (comment).

The other commits are indirectly related and could be moved to separate PRs (or dropped):

<details><summary>Details</summary>

The first commit fixes a timing race in the chainstatemanager_ibd_exit_after_loading_blocks test that happened because test code and validation code (in UpdateIBDStatus) were both calling NodeClock::now() to get the current clock time, and test relied on the times being the same for tip_recent=true checks. The failure this fixes should be rare, but I noticed it when I experimented with making time comparison in IsTipRecent more precise.
The third commit refactors mempool code to use a consistent way of getting and representing clock times, allowing the fourth commit to enforce that no libbitcoinkernel code uses nondeterministic clock times unintentionally. The third and fourth commits do not need to be part of this PR, but I implemented them to be sure the main commit was complete and all call sites were using the right clock times.

 </details>

DrahtBot commented at 2:52 AM on June 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/35557.

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:

#bitcoin-core/gui/866 (Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence by rebroad)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35511 (RFC: consensus: Make CAmount a class by hodlinator)
#35496 (kernel: add btck_set_mock_time for testing time-dependent paths by stringintech)
#35286 (rpc: add testsubmitpackage for 1p1c test submissions by instagibbs)
#26022 (Add util::ResultPtr class by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values 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.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

AcceptToMemoryPool(chainstate, tx, tx_pool.Now(), true, /*test_accept=*/false) in src/test/fuzz/tx_pool.cpp
StageAddition(tx1_conflict, tx1_fee, {}, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
StageAddition(tx3, tx2_fee, {}, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
StageAddition(tx1_conflict, tx1_fee+1, {}, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
StageAddition(entry4.GetSharedTx(), tx2_fee, {}, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
StageAddition(replacement_tx, 0, {}, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp
StageAddition(replacement_tx, high_fee, {}, 1, 0, false, 4, LockPoints()) in src/test/rbf_tests.cpp

2026-06-18 19:29:04

DrahtBot added the label CI failed on Jun 18, 2026

DrahtBot commented at 4:07 AM on June 18, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/27733525041/job/82045427048 LLM reason (✨ experimental): CI failed because the IWYU (include-what-you-use) check reported changes needed (generated “Failure generated from IWYU”).

<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/node/chainstate.cpp:251 in 9f67c9b269

 247 | @@ -248,7 +248,7 @@ ChainstateLoadResult VerifyLoadedChainstate(ChainstateManager& chainman, const C
 248 |      for (auto& chainstate : chainman.m_chainstates) {
 249 |          if (!is_coinsview_empty(*chainstate)) {
 250 |              const CBlockIndex* tip = chainstate->m_chain.Tip();
 251 | -            if (tip && tip->nTime > GetTime() + MAX_FUTURE_BLOCK_TIME) {
 252 | +            if (tip && tip->nTime > TicksSinceEpoch<std::chrono::seconds>(chainman.Now()) + MAX_FUTURE_BLOCK_TIME) {

maflcko commented at 5:02 AM on June 18, 2026:

9f67c9b269cbc0da459eafa1a5ba357e82f07212: You can use tip->Time(), to avoid the cast here.

Generally, it is better to use the chrono types and let the compiler do the type checks. After compilation, there won't be a difference anyway.

ryanofsky commented at 1:55 PM on June 18, 2026:

re: #35557 (review)

9f67c9b: You can use tip->Time(), to avoid the cast here.

Thanks! That is so much better and I applied this many places

in src/util/time.h:82 in 7d656ac341

  76 | @@ -66,6 +77,13 @@ struct MockableSteadyClock : public std::chrono::steady_clock {
  77 |      static void ClearMockTime();
  78 |  };
  79 |  
  80 | +/** Convert a time point into a duration of specified precision. */
  81 | +template <typename Duration, typename Timepoint>
  82 | +constexpr auto DurationSinceEpoch(Timepoint t)

maflcko commented at 5:12 AM on June 18, 2026:

Seems fine, but this may be promoting ugly code, such as count_seconds(DurationSinceEpoch<std::chrono::seconds>(...

I think generally it is better to use the correct std chrono type and let the compiler do the type checks. Casting a time point to a duration (since epoch) seems like a violation that should be avoided, for the same reasons that GetTime is deprecated.

At call sites you can just replace block->GetBlockTime() with block->Time() to avoid any casts, and this helper.

ryanofsky commented at 1:56 PM on June 18, 2026:

re: #35557 (review)

Seems fine, but this may be promoting ugly code

You're right, dropped this now

in src/validation.h:1020 in 9f67c9b269

1015 | +     * library users can control clock times used for validation.
1016 | +     */
1017 | +    NodeSeconds Now() const
1018 | +    {
1019 | +        const auto clock{m_clock_now_seconds.load(std::memory_order_relaxed)};
1020 | +        return clock != std::chrono::seconds{0} ? NodeSeconds{clock} : std::chrono::time_point_cast<std::chrono::seconds>(NodeClock::now());

maflcko commented at 5:17 AM on June 18, 2026:

Do we really want the cast here? The answer is probably, "doesn't matter", because consensus time checks are seconds-based, and the double-precision rolling update mempool code doesn't care about sub-second precision. Though, I wanted to point it out, that this is intentional and not just something that was most likely to the LLM.

An alternative could be to remove the cast here and only add the cast in call sites that need it (implicilty "upgrading" the call-sites to sub-second precision)

ryanofsky commented at 2:03 PM on June 18, 2026:

re: #35557 (review)

Do we really want the cast here?

I don't think so. It's definitely simpler not to have this and it is dropped now.

Though, I wanted to point it out, that this is intentional and not just something that was most likely to the LLM.

I guess it's fair to blame the LLM for this. Technically I added this cast, but the LLM originally wrote ::Now<NodeSeconds>() which was casting internally. Smarter to avoid it entirely.

ryanofsky commented at 9:52 AM on June 18, 2026: contributor

Thanks @maflcko! I was very dissatisfied with all the casts in the second commit and your tips should be helpful for getting rid of them. I will also implement your suggestion to not cast away precision in ChainstateManager::Now(). (The LLM actually didn't write the cast, but called ::Now<NodeSeconds>() instead and I manually added the cast to try to be more explicit.)

ryanofsky force-pushed on Jun 18, 2026

ryanofsky commented at 2:10 PM on June 18, 2026: contributor

Updated 7d656ac341140dc80f73abcc45adc19f0430eaae -> 94622b041571f73e8e5e661b9bce4213b957a561 (pr/kclock.1 -> pr/kclock.2, compare) adding a new commit to use time points to represent times in mempool code

Updated 94622b041571f73e8e5e661b9bce4213b957a561 -> 27ea72916b71e40859d7d86348eb3d99c4211b34 (pr/kclock.2 -> pr/kclock.3, compare) to fix CI: compile error (duration_cast on time_point in TRACEPOINT calls) and nondeterministic test (clock race in chainstatemanager_ibd_exit_after_loading_blocks); also prepends a standalone test fix commit using only pre-existing SetMockTime API https://github.com/bitcoin/bitcoin/actions/runs/27764264509/job/82146558123

Updated 27ea72916b71e40859d7d86348eb3d99c4211b34 -> 8be500e6bec2e4e664f36dea22c6419237ba9ec2 (pr/kclock.3 -> pr/kclock.4, compare) to fix fuzz UBSan signed integer overflow converting seconds to nanoseconds in ConsumeTxMemPoolEntry https://github.com/bitcoin/bitcoin/actions/runs/15491561394/job/82175291056

Updated 8be500e6bec2e4e664f36dea22c6419237ba9ec2 -> 37fb1b596e2006ffb1af82945f81a27c2bc1777a (pr/kclock.4 -> pr/kclock.5, compare) to fix UBSan signed-integer-overflow: throw in LoadMempool when nTime is out of MempoolTime range; also use ConsumeTime() in ConsumeTxMemPoolEntry https://github.com/bitcoin/bitcoin/actions/runs/27776901912/job/82191293065

test: Fix nondeterministic clock race in chainstatemanager_ibd_exit_after_loading_blocks

The test set up the tip time using Now<NodeSeconds>() and then called
UpdateIBDStatus(), which calls IsTipRecent(), which also calls
Now<NodeSeconds>(). If a second boundary was crossed between the two
calls, the tip_recent=true case would fail spuriously.

Fix by freezing the clock with SetMockTime() before the test runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

f0fea79bda

kernel, validation: scope clock time to ChainstateManager

Add ChainstateManager::Now() as the single injection point for the
current time in all validation paths, replacing direct NodeClock::now()
calls. A new m_clock_now_seconds field (std::atomic<std::chrono::seconds>)
lets callers override the time for a chainstate manager instance.

Affected paths:
- ContextualCheckBlockHeader: future-time check
- UpdateIBDStatus -> IsTipRecent: IBD latch (IsTipRecent gains a now
  parameter so callers control what time is used)
- ProcessNewBlockHeaders / ReportHeadersPresync: IBD progress logging
- GuessVerificationProgress: verification_progress in block tip callbacks
- VerifyLoadedChainstate: future-tip sanity check on startup

The kernel C API gains btck_chainstate_manager_set_clock_time(chainman,
now_seconds), and the C++ wrapper gains a matching SetClockTime() method.
Two concurrent test instances using different chainman objects see
independent clocks; no cleanup of global state is required between tests.

Co-Authored-By: stringintech <stringintech@gmail.com>
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

8a93ea0f29

mempool: Use NodeClock::time_point to represent times

Use NodeClock::time_point instead of integer ticks or std::chrono::seconds
durations to represent time points in mempool code.  Using points is safer and
more precise and tends to simplify time calculations.

This also adds MempoolTime and CTxMemPool::Now() definitions for convenience,
and to make it easier to add more determinism or type-checking to mempool code
in the future without changing it.

Another benefit of this commit is that it gets rid of all GetTime() calls in
the kernel, which have long been deprecated. The integer GetTime() function is
deprecated because it does not return type-safe values. And the template
GetTime() function has been deprecated since the NodeClock struct was added
because it provides a subset of NodeClock functionality and confusingly returns
a duration rather than a time.

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

a839a2c0d8

util/time: Disallow calling now() methods in kernel code

Move NodeClock::now() and NodeClock::now() symbol definitions to a
time_nondet.cpp file so they will not be linked into kernel code and
accidentally called there.

Kernel code should generally try to be deterministic and not rely on
time variables outside of application control. As an escape hatch,
_now_nondet() methods are added to provide nondeterminism in cases where
it is ok.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

37fb1b596e

in src/test/fuzz/tx_pool.cpp:162 in 94622b0415 outdated

 158 | @@ -159,7 +159,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, Cha
 159 |      if (fuzzed_data_provider.ConsumeBool()) {
 160 |          // Try expiry
 161 |          LOCK2(::cs_main, tx_pool.cs);
 162 | -        tx_pool.Expire(GetMockTime() - std::chrono::seconds(fuzzed_data_provider.ConsumeIntegral<uint32_t>()));
 163 | +        tx_pool.Expire(NodeSeconds{GetMockTime() - std::chrono::seconds(fuzzed_data_provider.ConsumeIntegral<uint32_t>())});

maflcko commented at 2:24 PM on June 18, 2026:

probably not a problem in reality, but the call to GetMockTime here looks wrong for several reasons:

It directly access the test-only global of the mock time, which may be 0, and thus may lead to silent bugs if mocktime was not used here?
It side-steps the g_used_system_time runtime sanitizer.
It returns the wrong type (it should be a time point, not a duration)

I guess it would be better to just call Now() or tx_pool.Now() to fix all issues and avoid the NodeSeconds{} cast.

ryanofsky commented at 8:45 PM on June 18, 2026:

re: #35557 (review)

Makes sense, switched GetMockTime() -> Now()

ryanofsky force-pushed on Jun 18, 2026

DrahtBot removed the label CI failed on Jun 18, 2026

ryanofsky marked this as ready for review on Jun 18, 2026

ryanofsky commented at 9:01 PM on June 18, 2026: contributor

Marked ready for review now that CI is passing. Possible questions for reviewers:

Should this PR be split up? I am thinking of dropping everything except the main commit 8a93ea0f2904ec780a690d987a25e5fa113552c9 and moving other commits to a followup PR. But happy to do anything.
Is btck_chainstate_manager_set_clock_time(chainman, now) a good kernel API? I think it is, giving applications control over time in a pretty granular way that doesn't get in the way of adding more control later.
Are the changes to validtion code ok? There's a new ChainstateManager::m_clock_now_seconds field which adds new state, but not much changes otherwise and the changes make code more consistent.

kernel, validation: Add btck_chainstate_manager_set_clock_time #35557

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)