test: Add and use ElapseTime helper #32430

maflcko · 2025-05-06T22:39:14Z

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 a new ElapseTime helper, which resets the mocktime once it goes out of scope. Also, it has a method to advance the mocktime by a delta.

DrahtBot · 2025-05-06T22:39:17Z

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/32430.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	l0rinc
Stale ACK	w0xlt

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33106 (policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee by glozow)
#32941 (p2p: TxOrphanage revamp cleanups by glozow)
#32414 (validation: periodically flush dbcache during reindex-chainstate by andrewtoth)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#28676 (Cluster mempool implementation by sdaftuar)
#17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#16545 (refactor: Implement missing error checking for ArgsManager flags 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.

DrahtBot · 2025-05-06T22:54:38Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/41759458930}
_{LLM reason (✨ experimental): (empty)}

Hints

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.

l0rinc

Concept ACK - was thinking of something similar when reviewing 41479ed (#32414)

maflcko · 2025-05-07T09:47:14Z

Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully 😅

src/test/util/setup_common.h

w0xlt

ACK fae29c5

DrahtBot · 2025-07-08T20:27:38Z

🚧 At least one of the CI tasks failed.
_{Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/45588216821}
_{LLM reason (✨ experimental): The CI failure is caused by a compilation error in src/util/time.cpp due to incorrect usage of std::chrono::duration, which prevents building.}

Hints

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.

maflcko · 2025-07-09T15:04:10Z

Sorry for the force-pushes, but this uncovered pre-existing issues in the fuzz tests that were not using mocktime during init. So I went ahead and fixed those as well in a separate commit. Should be good now, hopefully 😅

Went ahead and cherry-picked out the fuzz-fixes into #32927. Should be easy to (re-)review that one.

maflcko · 2025-07-11T13:44:21Z

rebased to drop merged commits and included some minimal fixups

l0rinc

Thanks for fixing these, left quite a few comments, hope you'll find them useful

src/test/util/setup_common.h

l0rinc · 2025-07-27T04:28:09Z

src/test/fuzz/headerssync.cpp

👍 for using the built-in min instead.

Could we add it to the commit message that this isn't just a simple refactor (and not just about the return value), but as @hodlinator stated in a different PR where he did the same:

That way we can always proceed instead of just exiting the fuzz target, wasting cycles. Also reduces code complexity.

thx, split up into a new commit

l0rinc · 2025-07-27T04:31:13Z

src/test/util/time.h

typo in commit message:

no state is leaked between test cases

l0rinc · 2025-07-27T04:32:34Z

src/test/util/time.h

nit: the code isn't in exact alignment with the comment - and since we are calling it with 0, maybe something like:

Suggested change

Assert(d >= 0s); // Steady time can only move forward.

Assert(d >= 0ms); // Steady time cannot move backward.

(nit2: I know it's the same, but to clarify that we don't just mean that time-jumps cannot be 0.5 seconds, we might want to use 0ms instead)

thx, reworded comment a bit

l0rinc · 2025-07-27T04:33:41Z

src/test/util/time.h

nit: I understand that it's meant to be symmetric with ElapseSteady, but I don't find the comment helpful as it is.

thx, reworded comment

l0rinc · 2025-07-27T05:14:59Z

src/test/denialofservice_tests.cpp

any reason for defining the delta so far away from first usage?

Suggested change

elapse_time(delta);

const auto delta{3 * std::chrono::seconds{m_node.chainman->GetConsensus().nPowTargetSpacing} + 1s};

elapse_time(delta);

Just to make review easier, because you agree that the commit is already large in #32430 (comment). Will leave as-is for now.

src/test/mempool_tests.cpp

l0rinc · 2025-07-27T05:29:15Z

src/test/fuzz/p2p_handshake.cpp

do we still need to keep these constants or is there maybe a cleaner way now?

seems unrelated? But i am happy to review a pull request changing it

l0rinc · 2025-07-27T05:34:27Z

src/test/util/time.h

nit: we might as well make the constructors explicit

l0rinc · 2025-07-27T05:46:20Z

src/bench/util_time.cpp

faaaaac58a447c4f8da67ff55a35f7e85cda369f is quite big...
And some of the changes are non-trivial, if you can, please consider doing it in multiple commits

It is just 50 lines, so it should be manageable. Anyone is free to review it file-by-file, if they want to split it up, but I am not sure about splitting it up too much?

This is less code and also required for the next commit.

This is a bit more type-safe than a raw i64.

This makes it easier to use mock-time in tests. Also, it resets the global mocktime, so that no state is leaked between test cases. Also, it resets the global steady mock-time for the same reason.

This is required in the next commit.

maflcko · 2025-07-31T10:41:23Z

Force pushed with some minor doc-changes and small refactoring in src/test/util/time.h

l0rinc

code review ACK fa80e75

DrahtBot · 2025-08-04T17:58:15Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2025-11-01T01:36:21Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it with one of the labels 'Up for grabs' or 'Insufficient Review', so that it can be picked up in the future.

DrahtBot added the Tests label May 6, 2025

maflcko force-pushed the 2505-elapse-time branch from fa13228 to bbbb2af Compare May 6, 2025 22:54

DrahtBot added the CI failed label May 6, 2025

maflcko force-pushed the 2505-elapse-time branch 5 times, most recently from fa123d4 to fae29c5 Compare May 7, 2025 09:27

l0rinc approved these changes May 7, 2025

View reviewed changes

DrahtBot removed the CI failed label May 7, 2025

DrahtBot mentioned this pull request May 7, 2025

validation: periodically flush dbcache during reindex-chainstate #32414

Merged

w0xlt reviewed May 7, 2025

View reviewed changes

src/test/util/setup_common.h Outdated Show resolved Hide resolved

w0xlt reviewed May 7, 2025

View reviewed changes

DrahtBot requested a review from l0rinc May 7, 2025 18:39

This was referenced May 7, 2025

wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict #29680

Closed

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

DrahtBot mentioned this pull request May 16, 2025

fuzz: Delete wallet_notifications #32526

Merged

DrahtBot added the Needs rebase label May 16, 2025

maflcko force-pushed the 2505-elapse-time branch from fae29c5 to fa67233 Compare July 8, 2025 19:44

DrahtBot added the CI failed label Jul 8, 2025

DrahtBot removed the Needs rebase label Jul 8, 2025

This was referenced Jul 8, 2025

fuzz: Make process_message(s) more deterministic #32822

Merged

p2p: improve TxOrphanage denial of service bounds #31829

Merged

This was referenced Jul 9, 2025

fuzz: Add missing calls to SetMockTime for determinism #32927

Merged

p2p: TxOrphanage revamp cleanups #32941

Merged

maflcko force-pushed the 2505-elapse-time branch 2 times, most recently from fad3690 to fa1dde3 Compare July 11, 2025 13:35

DrahtBot removed Needs rebase CI failed labels Jul 11, 2025

maflcko requested a review from w0xlt July 18, 2025 12:43

DrahtBot added the Needs rebase label Jul 18, 2025

maflcko force-pushed the 2505-elapse-time branch 2 times, most recently from fa55137 to fafcff1 Compare July 24, 2025 14:19

DrahtBot added Needs rebase and removed Needs rebase labels Jul 24, 2025

maflcko force-pushed the 2505-elapse-time branch from fafcff1 to faaaaac Compare July 25, 2025 11:09

DrahtBot removed the Needs rebase label Jul 25, 2025

l0rinc reviewed Jul 27, 2025

View reviewed changes

DrahtBot mentioned this pull request Jul 29, 2025

refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags #17580

Draft

MarcoFalke added 6 commits July 31, 2025 12:19

fuzz: Use min option in ConsumeTime

faaccbf

This is less code and also required for the next commit.

fuzz: Return chrono duration from ConsumeTime()

fa892b8

This is a bit more type-safe than a raw i64.

test: Add ElapseTime context

44440d6

This makes it easier to use mock-time in tests. Also, it resets the global mocktime, so that no state is leaked between test cases. Also, it resets the global steady mock-time for the same reason.

fuzz: Use ElapseTime

faec029

test: Allow time_point in boost checks

fa9d494

This is required in the next commit.

test: Use ElapseTime in more tests

fa80e75

maflcko force-pushed the 2505-elapse-time branch from faaaaac to fa80e75 Compare July 31, 2025 10:22

l0rinc approved these changes Jul 31, 2025

View reviewed changes

This was referenced Jul 31, 2025

policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106

Merged

kernel, refactor: return error status on all fatal errors #29700

Draft

common: Disallow calling IsArgSet() on ALLOW_LIST options #17783

Draft

DrahtBot added the Needs rebase label Aug 4, 2025

	Assert(d >= 0s); // Steady time can only move forward.
	Assert(d >= 0ms); // Steady time cannot move backward.

	elapse_time(delta);
	const auto delta{3 * std::chrono::seconds{m_node.chainman->GetConsensus().nPowTargetSpacing} + 1s};
	elapse_time(delta);

test: Add and use ElapseTime helper #32430

Are you sure you want to change the base?

test: Add and use ElapseTime helper #32430

Uh oh!

Conversation

maflcko commented May 6, 2025

Uh oh!

DrahtBot commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented May 6, 2025

Uh oh!

l0rinc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented May 7, 2025

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 8, 2025

Uh oh!

maflcko commented Jul 9, 2025

Uh oh!

maflcko commented Jul 11, 2025

Uh oh!

l0rinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 31, 2025

Uh oh!

l0rinc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Aug 4, 2025

Uh oh!

DrahtBot commented May 6, 2025 •

edited

Loading

l0rinc left a comment •

edited

Loading

l0rinc Jul 27, 2025 •

edited

Loading

l0rinc Jul 27, 2025 •

edited

Loading

l0rinc left a comment •

edited

Loading