Some kernel paths read the current time (e.g. header validation's future-time check, and the btck_SynchronizationState carried by btck_NotifyBlockTip / btck_NotifyHeaderTip callbacks), which makes them awkward to exercise deterministically in tests. This PR exposes btck_set_mock_time as a wrapper over the existing SetMockTime, mirroring what the node has via the setmocktime RPC.
Prior IRC discussion: https://gnusha.org/bitcoin-kernel/2026-06-04.log
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35496.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | josibake, janb84, sedited |
| Concept ACK | stickies-v |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
<!--85328a0da195eb286784d51f73fa0af9-->
š§ At least one of the CI tasks failed.
<sub>Task No wallet: https://github.com/bitcoin/bitcoin/actions/runs/27215802797/job/80356864067</sub>
<sub>LLM reason (āØ experimental): CI failed due to a build linker error: test_kernel couldnāt link because NodeClock::now() had undefined references.</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>
Interesting, Concept ACK.
Force-pushed: the test now verifies the effect through the kernel API instead of reading Now<NodeSeconds>() directly. Depending on the build configuration, g_mock_time can end up duplicated between libbitcoinkernel and the test binary, so the test's read and the kernel's write hit different globals and the assertion fails even if the link issue is resolved by linking bitcoin_util into test_kernel.
Concept ACK
Force-pushed to lower the upper bound from std::chrono::nanoseconds::max() converted to seconds (~year 2262) to UINT32_MAX (~year 2106).
Previously I matched the setmocktime RPC's upper bound, but actually exercising that maximum can cause signed integer overflow in paths that add to the current time (e.g.ContextualCheckBlockHeader's future-time check).
Looking for a tighter bound that's both safe against such overflows and semantically meaningful, I landed on UINT32_MAX: a natural ceiling since block header nTime is uint32_t, and mocking time beyond that doesn't seem meaningful for anything consensus-related.
I also restructured the unit test so that applying the patch from https://github.com/stringintech/bitcoin/commit/38173fc7f09182104e6e210a8884d2d4e8992c9f surfaces the overflow in CI; e.g. UBSan caught it in https://github.com/stringintech/bitcoin/actions/runs/27306244344/job/80664809704#step:10:4832.
If this bound looks reasonable, a follow-up could apply the same approach to the setmocktime RPC. Open to suggestions if anyone has a better idea!
If this bound looks reasonable, a follow-up could apply the same approach to the
setmocktimeRPC. Open to suggestions if anyone has a better idea!
Yeah, I guess that could make sense. The time may eventually "hop" into the u32 block time context/type, in which case it will overflow.
For reference to reproduce:
diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index bf7c7d117a..51c3d01de4 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -292,3 +292,3 @@ class BlockchainTest(BitcoinTestFramework):
time_2106 = 2**32 - 1
- self.nodes[0].setmocktime(time_2106)
+ self.nodes[0].setmocktime(time_2106+5)
last = self.generate(self.nodes[0], 6)[-1]
Will give:
SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation src/node/miner.cpp:151:21
Concept ACK
To use one of my favourite buzzwords, sans-io! Definitely agree with making time an input. Less excited about a method that allows modifying a global state variable. Using header validation as an example, ContextualCheckBlockHeader() reads NodeClock::now() for the future-time check and ProcessNewBlockHeaders() reads it for progress estimates. Exporting btck_set_mock_time() makes that dependency public and process global instead, so one kernel user can affect all kernel contexts in the process, and callers need to manage cleanup, etc themselves. This could be a problem for testing, for example.
I think this is a good opportunity to start moving the kernel API toward a pattern I've been experimenting with where runtime inputs are passed at operation boundaries, rather than controlled through global state.
One way you could do this incrementally here is keep the existing function as is (using NodeClock::now(), but add an options variant:
typedef struct btck_BlockHeaderValidationOptions btck_BlockHeaderValidationOptions;
btck_BlockHeaderValidationOptions* btck_block_header_validation_options_create();
void btck_block_header_validation_options_destroy(
btck_BlockHeaderValidationOptions* options);
int btck_block_header_validation_options_set_current_time(
btck_BlockHeaderValidationOptions* options,
int64_t current_time);
btck_BlockValidationState*
btck_chainstate_manager_process_block_header_with_options(
btck_ChainstateManager* chainman,
const btck_BlockHeader* header,
const btck_BlockHeaderValidationOptions* options);
..which could grab the current time once at the API boundary via the node clock or use the supplied time when provided, then pass that value down to the future-time check. We get the testing benefit and it gives us a pattern we can reuse for other time inputs as the kernel API grows. Testing is the obvious example but another reason I've been experimenting with this is snapshotting and committing consensus updates, e.g., updating the UTXO set. having the ability control time makes it so much easier to reason through stuff like this.
This is more work than setmocktime, because this would have to be repeated for more functions , of course. But I think this is the right work and moves us to a safer kernel API in general. Whenever something is hard to test, like you found here, thats usually an indication its also hard to write safe, correct code. In an ideal kernel engine world, we would be passing all of these runtime side effects in , rather than relying on the kernel to decide on these things internally.
Thank you for your feedback @josibake! I understand the concern regarding mutating a global (also brought up in the IRC discussion linked in the PR description), and I totally agree that if we decide on an alternative that proves not too invasive, we should just skip this PR and go in that direction.
Regarding the suggested approach, I think passing time as an arg makes perfect sense for purer functions (in the style of the current btck_block_check/btck_transaction_check, if they needed time), but I'm not sure it suits operations that take chainman as input. Passing time to such a function may suggest the operation depends on time only through that one direct read. While this might be true for btck_chainstate_manager_process_block_header, I'm thinking it may not generalize well, since chainman may have absorbed time-dependent state from previous operations (e.g. with btck_chainstate_manager_process_block, the IBD latch is set permanently based on a time read in an earlier call, and the btck_SynchronizationState delivered in a later call's tip notification depends on that latch; in other words, a later call's output depends on time provided to an earlier one). That's why I feel a global mock time, ugly as it is (ideally a per-context clock), might be more honest about what it provides at the current state of things.
Curious what you think; you have a much better view of the bigger picture here, so I may well be missing an angle.
Great callout re: implying the chainman (or calls that take the chainmain as an input) are more pure than they actually are. But I actually think the chainman example you gave is an even stronger motivation for the direction I'd like to go. Let me try to explain:
The north star version of the kernel API I'd like to move toward is one where runtime side effects are captured outside the kernel operation and passed in as values at the operation boundary. This is heavily inspired by the "local reasoning" philosophy, i.e. the API should make the operationās real inputs visible enough that a caller can reason about the state transition locally.
For ChainstateManager, the model is not a pure function like:
validate(header, time) -> resultbut rather has an inout state transition:
process(chainman, block/header, runtime_inputs) -> chainman' + result + notifications..which is an important distinction, imo. ChainstateManager does carry time derived state from one call into later calls, the IBD latch you mentioned being a great example. But I think that argues for explicit runtime inputs, not for exporting mock time. In the explicit input model, if an operation exits IBD based on a supplied time, that becomes part of the resulting ChainstateManager state. A later notification depending on that state is expected. The important part is that the earlier state transition had an explicit time input. This of course should be documented, to avoid the confusion you mentioned above.
A global mock clock makes the relationship less visible: the operation depends on whichever caller last mutated global time. This can be guarded against (e.g., the existing node guardrails), but I think it gets a lot messier with kernel, and also writing a bunch of extra code to make using the existing code safe is smelly. A per context clock as you suggest would be better than global state because it avoids cross context interference. However, I'd argue the kernel API should either accept a stable operation-time value or capture one explicitly before validation starts; otherwise the operationās real input remains hidden in mutable context state and unobservable to the caller.
Concretely, the direction I would like to move toward is:
btck_BlockValidationOptions
current_time
btck_chainstate_manager_process_block_header(..., options)
btck_chainstate_manager_process_block(..., options)
with the implementation translating the C options into an internal invariant-bearing value,
e.g. BlockValidationTime, and then passing that value through the validation path. The
important rule is that once the explicit path is used, operations should not
silently fall back to NodeClock::now(). The supplied time should feed the future-time check,
header progress estimates, and eventually the IBD recency/latching paths touched by block
processing.
Obviously, this is still a north start design. For this PR, I don't think we need to solve the whole thing at once and can instead making a few functions more easily testable in the right way, e.g. the smallest useful step would be to avoid adding btck_set_mock_time and instead add an explicit options variant for the path this PR is testing:
btck_BlockValidationOptions* btck_block_validation_options_create();
int btck_block_validation_options_set_current_time(...);
btck_BlockValidationState*
btck_chainstate_manager_process_block_header_with_options(
btck_ChainstateManager* chainman,
const btck_BlockHeader* header,
const btck_BlockValidationOptions* options);
That would solve the deterministic header validation test without exporting global mutable
time as kernel API, and more importantly it gives us a pattern to extend to process_block / IBD in a
follow-up. The slightly larger but more coherent alternative would beto use the same btck_BlockValidationOptions for both process_block_header and process_block. Ideally we document a design in an issue, so that each subsequent PR has a strong motivation to point to and can also be judged against. I'd be more than happy to collaborate on writing a document if that's interesting to you.
TLDR; skip btck_set_mock_time as the kernel API, introduce explicit options at the kernel boundary, and we can keep the PRs scoped to the small, incremental chunks that move us toward the intended kernel API shape without requiring a full validation refactor.
Obviously, this is still a north start design. For this PR, I don't think we need to solve the whole thing at once and can instead making a few functions more easily testable in the right way, e.g. the smallest useful step would be to avoid adding btck_set_mock_time and instead add an explicit options variant for the path this PR is testing:
There is a tension here between what is and ought to be in these idealized-style API boundaries. Setting the mock time in the option leads to worse local reasoning in the concrete: It communicates that the time is only mocked for this specific instance. We've had a discussion on how we should handle globals in the header a few times, primarily around the logging API. Initially I implemented the API functions such as btck_logging_set_level_category on a logging connection, but reviewers rightly pointed out that this makes reasoning about the function harder. I now tend to lean towards communicating mutating globals with global functions. There is also #34775 which makes the global effects of logging clearer by getting rid of the instantiated logging connection. So I think the function here is fine as is.
This is a dance: We want to have a better API, but we also want better functionality for users of the library that can be used and tested properly without breaking expectations because of a quirk in the implementation code.
1489 | @@ -1488,3 +1490,13 @@ int btck_transaction_check(const btck_Transaction* tx, btck_TxValidationState* v 1490 | const bool ok = CheckTransaction(*btck_Transaction::get(tx), state); 1491 | return ok ? 1 : 0; 1492 | } 1493 | + 1494 | +int btck_set_mock_time(int64_t timestamp) 1495 | +{ 1496 | + constexpr int64_t MAX_BLOCK_TIME_REPRESENTABLE{std::numeric_limits<uint32_t>::max()};
Can you call this the same like in the rpc code you recently added?
Approach ACK
There is a tension here between what is and ought to be in these idealized-style API boundaries. Setting the mock time in the option leads to worse local reasoning in the concrete: It communicates that the time is only mocked for this specific instance
I understand the concern and sympathise. I also agree with the principle that if something mutates global state, the API should communicate that as global. An options API that just called SetMockTime() internally would be terrible. But that's not what I'm advocating for here.
I also donāt accept the only honest API is neccessarily a global time setter. The way to make the API honest is to stop treating this as āmock timeā and instead make it an input that is actually threaded through the relevant code paths.
For the header future-time validation path, that can be made true today, no lies. The path is:
ProcessNewBlockHeaders
-> AcceptBlockHeader
-> ContextualCheckBlockHeader
..and on through to use it in the future-time check:
struct BlockValidationTime {
NodeSeconds now;
};
static bool ContextualCheckBlockHeader(
const CBlockHeader& block,
BlockValidationState& state,
const ChainstateManager& chainman,
const CBlockIndex* pindexPrev,
BlockValidationTime time)
{
// ...
if (block.Time() > time.now + std::chrono::seconds{MAX_FUTURE_BLOCK_TIME}) {
return state.Invalid(BlockValidationResult::BLOCK_TIME_FUTURE,
"time-too-new",
"block timestamp too far in the future");
}
// ...
}
and AcceptBlockHeader() / ProcessNewBlockHeaders() would take and forward the same value, etc. Existing non-explicit paths could still capture NodeClock::now() at the boundary or use a small overload, but the explicit path would not mutate or read g_mock_time.
@stringintechās concern is real for process_block, and I donāt think we should expose the same option there until the time-dependent paths are wired up, as well. For example, IBD latching currently goes through UpdateIBDStatus() and IsTipRecent(), so an explicit process_block path would need to pass the operation time into that and same for progress estimates, etc. This is what I was referring to in response when saying we should really have a cohesive design around time, and other runtime inputs into kernel.
Again, I'm not advocating we add current_time to an options object and leave the relevant operation path reading NodeClock::now() in the background. The argument I am trying to make is the reason this code is hard to test and the reason reviewers tend to keep referring to the only available solution as "ugly" is because it is: it indicates something that should be a locally reasoned input at a boundary is instead a hidden global variable. My concern is adding testing via this path will give a false sense of security on how safe the code is. Instead , I think we should make the code safer while also making it easier to test. For the header future-time check, from what I can tell the code can be safer and more testable today with the approach I'm describing. For process_block, and other areas it would necessarily come with the additional refactoring. I do feel that making the code safer and easier to test, however, is an excellent motivation for refactoring.
The way to make the API honest is to stop treating this as āmock timeā and instead make it an input that is actually threaded through the relevant code paths.
I agree with this conceptually (and it's what I suggested on the IRC discussion linked in OP), and I think your suggested approach of incrementally exposing functions _with_options that purely rely on user-provided time could be a sensible way of getting there.
I think fully implementing that will be orders of magnitude more work to get merged than this PR's approach, and they are not meaningfully at odds with each other. We can still work on improving how we deal with time while also allowing better testing of the current NodeClock::now()-based code.
1360 | +{ 1361 | + if (btck_set_mock_time(timestamp.count()) != 0) { 1362 | + throw std::runtime_error("timestamp out of range"); 1363 | + } 1364 | +} 1365 | +
1/2:
NIT: Add a RAII guard alongside the existing set_mock_time free function. It overrides the clock in its constructor and always calls btck_set_mock_time(0) in its destructor, so global mock time can't leak past the scope even on an exception path
//! RAII guard that overrides the kernel clock for its lifetime and restores
//! the system clock on destruction
class MockTime
{
public:
explicit MockTime(std::chrono::seconds timestamp) { set(timestamp); }
~MockTime() { btck_set_mock_time(0); }
MockTime(const MockTime&) = delete;
MockTime& operator=(const MockTime&) = delete;
void set(std::chrono::seconds timestamp) { set_mock_time(timestamp); }
};
I also thought about adding that, but I'm hesitant to put it in the public C++ API. Setting the mock time and resetting it right away may not be the only usage (a node impl might want to only expose it as an RPC), and an RAII helper tends to grow usage-specific conveniences (like FakeNodeClock), so a single one in the shipped wrapper couldn't fit every consumer. I think it's better downstream users adding their own helper for their unit tests if they want one. For this PR I could inline the helper in the test file, but it felt like overkill for this single test case. Happy to add one in the test file if you think it still helps.
Have to agree that the location was unfortunate, this was not to extend the C++ API purely to add a guard to make sure you leave the system in the same state you left it, even when it throws.
1418 | + BlockValidationState ok_state{chainman->ProcessBlockHeader(header)}; 1419 | + BOOST_CHECK(ok_state.GetValidationMode() == ValidationMode::VALID); 1420 | + BOOST_CHECK(ok_state.GetBlockValidationResult() == BlockValidationResult::UNSET); 1421 | + 1422 | + // Restore the system clock so later tests aren't affected 1423 | + set_mock_time(std::chrono::seconds{0});
2/2:
MockTime mock_time{block_time - std::chrono::hours{3}};
BlockValidationState future_state{chainman->ProcessBlockHeader(header)};
BOOST_CHECK(future_state.GetValidationMode() == ValidationMode::INVALID);
BOOST_CHECK(future_state.GetBlockValidationResult() == BlockValidationResult::TIME_FUTURE);
// At the upper bound the header is far in the past and must be accepted; this also
// confirms the future-time check's "now + 2h" computation doesn't overflow when now is at its max.
mock_time.set(MAX_BLOCK_TIME_REPRESENTABLE);
BlockValidationState ok_state{chainman->ProcessBlockHeader(header)};
BOOST_CHECK(ok_state.GetValidationMode() == ValidationMode::VALID);
BOOST_CHECK(ok_state.GetBlockValidationResult() == BlockValidationResult::UNSET);
I think fully implementing that will be orders of magnitude more work to get merged than this PR's approach, and they are not meaningfully at odds with each other. We can still work on improving how we deal with time while also allowing better testing of the current
NodeClock::now()-based code.
Moving forward solves the testing issue at hand but it also introduces technical debt, a for-testing-only global mutation into the public kernel API which sets a trend and once merged, the function is hard to remove. (yes the API is experimental but there nothing as permanent as temporary)
That being said, if we end up moving forward on this PR, I suggest to add an RAII gaurd.
the function is hard to remove.
The code surface is tiny and self-contained.
(yes the API is experimental but there nothing as permanent as temporary)
There is absolutely no argument to not remove a function in an interface that is marked as experimental and without compatibility guarantees, just for compatibility guarantees. Kernel development is already difficult and constrained enough as it is, we absolutely should not artificially slow ourselves down even more. I will continue to push back very hard on this and hope others will too.
Thanks a lot for elaborating, @josibake. The overall direction you're laying out makes a lot of sense to me. I have a couple of follow-up questions, and likely more once I've connected the dots, including a closer look at where else these validation paths use wall clock time (or monotonic time?) across what's reachable through the kernel. I'd like to explore that a bit more and get back to you; a dedicated issue might be the right home for it. Really appreciate the insights!
Force-pushed to address #35496 (review) by @sedited.
Thanks a lot for elaborating, @josibake. The overall direction you're laying out makes a lot of sense to me. I have a couple of follow-up questions, and likely more once I've connected the dots, including a closer look at where else these validation paths use wall clock time (or monotonic time?) across what's reachable through the kernel. I'd like to explore that a bit more and get back to you; a dedicated issue might be the right home for it. Really appreciate the insights!
Glad it was helpful, @stringintech ! I've been working through a more cohesive design of how time should be passed in as a runtime input to the kernel, and subsequently treated inside the kernel. I'd love to collaborate further on this if there is interest. I think a dedicated issue is a good place, perhaps you can open one with your initial findings?
The code surface is tiny and self-contained.
I'm not sure what your point is here. The callout is not about how many lines of code this is , rather the introduction of a function for testing into the API. This means tests will be written using this function in this repo and downstream repos consuming this API.
As you point out later, kernel development is already difficult and constrained, so asking everyone to rewrite their tests (including us) if/when we decide to change this is not trivial and could be met with the same push back of "don't ask me to rewrite tests , kernel work is difficult / constrained." I get that you want this ability for testing, but your argument that this is trivial and contained feels a bit disingenuous and dismissive of @janb84 pointing out a valid risk.
There is absolutely no argument to not remove a function in an interface that is marked as experimental and without compatibility guarantees, just for compatibility guarantees. Kernel development is already difficult and constrained enough as it is, we absolutely should not artificially slow ourselves down even more. I will continue to push back very hard on this and hope others will too.
Again, I'm not sure what you're arguing against here. It is valid to point out that code/functionality is orders of magnitude harder to remove, regardless of what the present day intentions are. There seems to be general agreement from yourself, the author, and other reviewers that this is not an ideal state and we would like to improve it to the point of hopefully removing this function some day. But that will necessarily be more work if this PR is merged: the work to make time a runtime input into the kernel AND the additional work to remove this code, rewrite our tests and coordinate rewriting tests in bindings / downstream projects that consume this API.
I would have preferred we just started on the runtime work now, but am sympathetic to the argument we can do this now while also tackling the harder work. However, I don't see any reason to "push back very hard" on someone pointing out this is a tradeoff and a risk we are taking by deciding to do things this way.
1385 | @@ -1386,3 +1386,39 @@ BOOST_AUTO_TEST_CASE(btck_transaction_check_tests) 1386 | "ffffffff00ffffffff000100000000000000000000000000000000000000000000000000000000" 1387 | "00000000000000ffffffff010000000000000000015100000000"); 1388 | } 1389 | + 1390 | +BOOST_AUTO_TEST_CASE(btck_set_mock_time_tests)
Instead of the API, you could add a KernelMockTime for the tests, same as we do with the other clock mocks in the unit tests:
index a6184be4a6..5702508e37 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -1387,6 +1387,21 @@ BOOST_AUTO_TEST_CASE(btck_transaction_check_tests)
"00000000000000ffffffff010000000000000000015100000000");
}
+class KernelMockTime
+{
+public:
+ explicit KernelMockTime(std::chrono::seconds timestamp) { set(timestamp); }
+ ~KernelMockTime()
+ {
+ set_mock_time(std::chrono::seconds{0});
+ }
+
+ KernelMockTime(const KernelMockTime&) = delete;
+ KernelMockTime& operator=(const KernelMockTime&) = delete;
+
+ void set(std::chrono::seconds timestamp) { set_mock_time(timestamp); }
+};
+
BOOST_AUTO_TEST_CASE(btck_set_mock_time_tests)
{
// Out-of-range timestamps throw
@@ -1407,18 +1422,15 @@ BOOST_AUTO_TEST_CASE(btck_set_mock_time_tests)
const std::chrono::seconds block_time{header.Timestamp()};
// With the time set 3h before the header, the kernel must see the header as >2h in the future and reject it
- set_mock_time(block_time - std::chrono::hours{3});
+ KernelMockTime mock_time{block_time - std::chrono::hours{3}};
BlockValidationState future_state{chainman->ProcessBlockHeader(header)};
BOOST_CHECK(future_state.GetValidationMode() == ValidationMode::INVALID);
BOOST_CHECK(future_state.GetBlockValidationResult() == BlockValidationResult::TIME_FUTURE);
// At the upper bound the header is far in the past and must be accepted; this also
// confirms the future-time check's "now + 2h" computation doesn't overflow when now is at its max.
- set_mock_time(max_time);
+ mock_time.set(max_time);
BlockValidationState ok_state{chainman->ProcessBlockHeader(header)};
BOOST_CHECK(ok_state.GetValidationMode() == ValidationMode::VALID);
BOOST_CHECK(ok_state.GetBlockValidationResult() == BlockValidationResult::UNSET);
-
- // Restore the system clock so later tests aren't affected
- set_mock_time(std::chrono::seconds{0});
}
I believe this would address the concern of time bleeding from one test to the next, especially in the case of exceptions. I also believe this is much more resilient than expecting test writers and reviewers remembering that the clock needs to be manually reset.
Applied, thanks! Also brought up by @janb84 in #35496 (review) (where I suggested keeping it a test-local helper).
Some kernel paths read the current time (e.g. header validation's future-time check, and the `btck_SynchronizationState` carried by `btck_NotifyBlockTip` / `btck_NotifyHeaderTip` callbacks), which makes them awkward to exercise deterministically in tests. This commit exposes `btck_set_mock_time` as a wrapper over the existing `SetMockTime`, mirroring what the node has via the `setmocktime` RPC.
ACK https://github.com/bitcoin/bitcoin/commit/156f2c6c49d6b2d0a8c1f87110cf1703b52b39bc
LGTM, in the interest of not letting the perfect be the enemy of the good. I do think we should start moving the kernel towards accepting time as a runtime input, starting with the easy examples discussed in this PR.
ACK 156f2c6c49d6b2d0a8c1f87110cf1703b52b39bc
Thanks for incorporating the RAII guard (but in the correct location š ).
Hope to see this removed soon for a more integral solution as suggested by @josibake, given that that change can take a while i'm for merging this (and hope that my concerns where just overcautious)
in the interest of not letting the perfect be the enemy of the good
I haven't read all of the discussion, but from what i can tell, I agree with points Josie is making, and I wish as a project we would more often choose approaches based on technical merit rather than expediency & resource constraints, although I understand there is a tradeoff.
In this case, I don't see why it's very hard to do the right thing. Like would a btck_chainstate_manager_set_mock_time(chainman, now) function not be a good idea for some reason? Implementation in 37adb4a76be482e4ff804f188eb4fe9a08cffa22
In this case, I don't see why it's very hard to do the right thing. Like would a btck_chainstate_manager_set_mock_time(chainman, now) function not be a good idea for some reason? Implementation in https://github.com/bitcoin/bitcoin/commit/37adb4a76be482e4ff804f188eb4fe9a08cffa22
Earlier concerns were raised about a naive implementation that still uses a global mocktime behind the facade. I'd definitely review a pull request that implements something along the lines of this patch. This still falls back to the global mock time if that is set, right? That seems very confusing for internal usage. Maybe we can place it into the node context? Then for the kernel library we can keep in bitcoinkernel.cpp's context class.
Maybe we can place it into the node context?
I think a time option is less confusing and more useful if it's narrowly scoped. The ideal thing would probably be what josie suggested of passing time directly into functions which use it. But because chainman has an IBD latch that depends on the time, and many other functions rely on it, I think just letting chainman have a time option is a good balance of purity and convenience.
But alternately, to see what it would look like, I implemented your suggestion of moving time option to the context level inĀ https://github.com/ryanofsky/bitcoin/commit/fe02261d2dc416a4b9f4be4794f588b02ccc6311 and it doesn't look bad either, though IMO it's not as intuitive or useful as giving chainman a time option. (I would be out of my depth in guessing which kernel applications you intend to support, but naively I would imagine that being able to simulate different times for different chains could beĀ a useful thing.)
I'd be happy to open another pr, or discuss this more or see any suggestions adopted here.Ā
Thanks a lot for the patches @ryanofsky!
As mentioned in #35496 (comment), I also think that a per-context (or per-chainman) mock time would require less work compared to the boundary-passed-time approach, to also work with cases that depend on chainman carrying time-derived state over calls (e.g. the IBD latch). I haven't gone deep into how solving such cases using @josibake's approach would look but I wonder if, in case we do intend to eventually go down that road, some of the internal plumbing we would add for a per-context clock would end up being reverted later; unless the changes are something we would want for our own node impl as well, and not only to serve kernel testing.
I opened #35557 based on previous patches, which I think provides a good solution to the problem of letting applications control clock times. I think this is a better alternative than using a global mock time, but even if this is not the solution we want to go with I think it provides a good basis for comparison. Note: I should probably add stringintech as a co-author to the commit in that PR since even though the PR was initially written by claude I think it was based on stringintech's code and tests.
in case we do intend to eventually go down that road, some of the internal plumbing we would add for a per-context clock would end up being reverted later; unless the changes are something we would want for our own node impl as well, and not only to serve kernel testing.
Yes, I would expect that if we change validation functions to explicitly be passed clock times as inputs we would want to do that in a pure way and remove all NodeClock references, and with my approach in #35557 drop the ChainstateManager::m_clock_now_seconds member. There would not be too much plumbing to rip out though. Also if we wanted we could still keep the kernel API unchanged, by having the kernel pass in clock times if the application doesn't provide them.
ACK 156f2c6c49d6b2d0a8c1f87110cf1703b52b39bc