test: FakeNodeClock follow-ups in unit tests #35497

maflcko commented at 5:48 PM on June 9, 2026: member

This switches the remaining cases in the unit tests from SetMockTime to FakeNodeClock for clarity, as explained in the commit messages.

DrahtBot added the label Tests on Jun 9, 2026

DrahtBot commented at 5:48 PM on June 9, 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/35497.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	frankomosh, sedited, w0xlt

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:

#35173 (util: shorten thread names to avoid Linux truncation by l0rinc)

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: Add FakeNodeClock m_clock to TestChain100Setup

Currently, all test cases using TestChain100Setup or a derived class
like BuildChainTestingSetup are using mocktime by default due to the
SetMockTime call in the TestChain100Setup ctor.

This is confusing, because test cases using mocktime explicitly seem to
imply that before they set the mocktime, real time was used.

E.g. index_reorg_crash claimed in a comment to "Enable mock time".

Fix this issue by adding a FakeNodeClock m_clock field to
TestChain100Setup. Then, use the m_clock instead of explicit calls to
SetMockTime or to a (now) shadowing local FakeNodeClock variable.

fae9623c8d

test: Use FakeNodeClock in more places

The context is easier to reason about: E.g.,

* in TestBasicMining it allows to drop manual SetMockTime(0) calls,
* in connections_desirable_service_flags it allows to drop manual calls
  to SetMockTime(GetTime<std::chrono::seconds>() + _n_) and replace them
  by operator+=(_n_)
* in wallet_tests it clarifies that the mocktime does not persist
  outside the AddTx function

fa3716c439

test: Use SteadyClockContext in pcp_tests

This is easier to reason about, because it will automatically take care
of properly setting INITIAL_MOCK_TIME in the ctor. Also, it allows to
drop the ElapseTime and replace it with a call to operator+=()

fa03852e9c

maflcko force-pushed on Jun 9, 2026

DrahtBot added the label CI failed on Jun 9, 2026

DrahtBot removed the label CI failed on Jun 9, 2026

in src/test/util/setup_common.h:230 in fa03852e9c

 226 | @@ -226,6 +227,7 @@ struct TestChain100Setup : public TestingSetup {
 227 |       */
 228 |      std::vector<CTransactionRef> PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit);
 229 |  
 230 | +    FakeNodeClock m_clock{std::chrono::seconds{1598887952}};

pablomartin4btc commented at 8:09 PM on June 9, 2026:

Could any fixed timestamp work here? If not, adding a brief comment (and/ or a named constant) indicating the UTC date corresponding to 1598887952 might make the intent clearer.

    static constexpr auto TEST_TIME{std::chrono::seconds{1598887952}};
    FakeNodeClock m_clock{TEST_TIME};

maflcko commented at 8:14 PM on June 9, 2026:

The constant was added in 31d225274ff1a4b245aea0a69f0e5224b0e64ca2 and is the AuthorDate of that commit. Any other constant would probably also work.

w0xlt commented at 9:55 PM on June 10, 2026:

nit:

    FakeNodeClock m_clock{std::chrono::seconds{1598887952}}; // 2020-08-31, arbitrary

seduless commented at 11:35 AM on June 12, 2026:

The mocktime here sets nTime for the mined blocks, so this value is coupled to the hardcoded tip block hash assertion.

https://github.com/bitcoin/bitcoin/blob/fa03852e9cc4f5aa5f62f55f61986663286dddb7/src/test/util/setup_common.cpp#L428-L431

frankomosh commented at 8:09 AM on June 10, 2026: contributor

crACK fa03852e9cc4f5aa5f62f55f61986663286dddb7. This PR continues the FakeNodeClock migration from #35114.

sedited approved

sedited commented at 7:57 PM on June 10, 2026: contributor

ACK fa03852e9cc4f5aa5f62f55f61986663286dddb7

w0xlt commented at 9:56 PM on June 10, 2026: contributor

ACK fa03852e9cc4f5aa5f62f55f61986663286dddb7

sedited merged this on Jun 11, 2026

sedited closed this on Jun 11, 2026

maflcko deleted the branch on Jun 11, 2026

rustaceanrob referenced this in commit 3f1dd488b8 on Jun 12, 2026

seduless commented at 11:35 AM on June 12, 2026: contributor

Tested ACK fa03852e9cc4f5aa5f62f55f61986663286dddb7

Commit fae9623c8db703f8277755b606420400a6cb0d3c is the first place FakeNodeClock is passed by reference to a separate thread, but FakeNodeClock::m_t isn't thread safe. I don't think there's a race here, since main thread writes to m_clock happen before StartBackgroundSync and the dtor write happens after Stop(). This may be a footgun if future tests copy this pattern, e.g. a test that calls mineBlocks() while the index thread is still syncing would race, since both threads would then increment m_clock. Perhaps it's worth updating FakeNodeClock to make it thread safe?