Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35557. ReviewsSee the guideline and AI policy for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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.
2026-09-08 18:14:27 |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
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 |
There was a problem hiding this comment.
Updated 7d656ac -> 94622b0 (pr/kclock.1 -> pr/kclock.2, compare) adding a new commit to use time points to represent times in mempool code
Updated 94622b0 -> 27ea729 (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 27ea729 -> 8be500e (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 8be500e -> 37fb1b5 (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
8be500e to
37fb1b5
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
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 8a93ea0 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_secondsfield which adds new state, but not much changes otherwise and the changes make code more consistent.
My preference is larger pull requests usually, so I would keep this in one piece, but I would go a bit further. Can we use this new clock in one of the tests (maybe one of the utxo* fuzz tests, haven't looked too closely yet).
If we set it in the options instead, we can fake the time once per instance, if we set it like here, it can be moved any number of times. I think I prefer that, as it seems to overlap more closely with the current global mechanics. The only reservation I have with this is that there might be a case where the clock needs to be set during chainman construction. I don't think that is the case right now, but that might change in the future.
I would feel more confident if something (a test, a utility, etc.) was actually exercising the various instances where the chainman-scoped fake time is used now. |
This could get a bit annoying and potentially dangerous when users need to start managing clocks across multiple places. Perhaps a more ergonomic alternative could be to add a That way, a single clock can be passed multiple times, and updated in a single place. Of course, it comes with its costs of increased API surface and lifetime concerns that a simple seconds setter doesn't have. |
| // Freeze the clock to avoid a race where Now<NodeSeconds>() in test setup | ||
| // and Now<NodeSeconds>() inside IsTipRecent() straddle a second boundary, | ||
| // causing a spurious failure in the tip_recent=true case. | ||
| SetMockTime(Now<NodeSeconds>()); |
There was a problem hiding this comment.
In commit "test: Fix nondeterministic clock race in chainstatemanager_ibd_exit_after_loading_blocks" (f0fea79)
Nice fix, thanks for cleaning this up.
Instead of bare SetMockTime calls, this is a good candidate for the RAII FakeNodeClock.
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index b0176f641c..3c357844cd 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -18,2 +18,3 @@
#include <test/util/setup_common.h>
+#include <test/util/time.h>
#include <test/util/validation.h>
@@ -22,3 +23,2 @@
#include <util/result.h>
-#include <util/time.h>
#include <util/vector.h>
@@ -178,3 +178,3 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_ibd_exit_after_loading_blocks, ChainTe
// causing a spurious failure in the tip_recent=true case.
- SetMockTime(Now<NodeSeconds>());
+ FakeNodeClock clock{};
@@ -214,3 +214,2 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_ibd_exit_after_loading_blocks, ChainTe
}
- SetMockTime(0);
}There was a problem hiding this comment.
re: #35557 (comment)
Instead of bare
SetMockTimecalls, this is a good candidate for the RAIIFakeNodeClock.
Thanks! Applied your patch and switched to FakeNodeClock
github-project-automation
Bot
moved this to Architecture & Performance
in The libbitcoinkernel Project
sedited
moved this from Architecture & Performance
to API Development
in The libbitcoinkernel Project
Replace FakeNodeClock (global mock time) with chainman.m_clock_now_seconds so the fuzzed time is set directly on the chainstate manager instance rather than via a global. This exercises the IBD and tip-age checks through the new per-chainman clock path. Byte consumption order from fuzzed_data_provider is unchanged so existing corpus entries remain valid. Idea for using clock time in utxo fuzz test from sedited in: bitcoin#35557 (comment) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
That one was merged, so is this here still relevant? Also, this conflicts with #35906 (comment) |
Replace FakeNodeClock (global mock time) with chainman.m_clock_now_seconds so the fuzzed time is set directly on the chainstate manager instance rather than via a global. This exercises the IBD and tip-age checks through the new per-chainman clock path. Byte consumption order from fuzzed_data_provider is unchanged so existing corpus entries remain valid. Idea for using clock time in utxo fuzz test from sedited in: bitcoin#35557 (comment) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…fter_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 FakeNodeClock before the test runs. Co-Authored-By: seduless <[email protected]> Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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. Note: this commit slightly changes behavior of the IsCurrentForFeeEstimation and GuessVerificationProgress functions because these function previously did time calculations with 1-second precision, and use with system clock level precision (typicaly nanoseconds). No other behavior outside of these two functions is changing. Co-Authored-By: stringintech <[email protected]> Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^[email protected]> Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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 <*~=`'#}+{/-|&$^[email protected]>
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 <[email protected]>
…oading_blocks Replace SetMockTime() with chainman.m_clock_now_seconds so the clock frozen for the race-prevention matches the same clock that UpdateIBDStatus/IsTipRecent() reads. This also avoids touching any global state and lets the test more directly exercise the new chainman-scoped clock path. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Replace FakeNodeClock (global mock time) with chainman.m_clock_now_seconds so the fuzzed time is set directly on the chainstate manager instance rather than via a global. This exercises the IBD and tip-age checks through the new per-chainman clock path. Byte consumption order from fuzzed_data_provider is unchanged so existing corpus entries remain valid. Idea for using clock time in utxo fuzz test from sedited in: bitcoin#35557 (comment) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
7 participants
The main commit in this PR is the second commit adding a
btck_chainstate_manager_set_clock_timeAPI 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
The first commit fixes a timing race in the
chainstatemanager_ibd_exit_after_loading_blockstest that happened because test code and validation code (inUpdateIBDStatus) were both callingNodeClock::now()to get the current clock time, and test relied on the times being the same fortip_recent=truechecks. The failure this fixes should be rare, but I noticed it when I experimented with making time comparison inIsTipRecentmore 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.