fuzz: share a single mocked steady clock across FuzzedSock instances #35536

pull HowHsu wants to merge 1 commits into bitcoin:master from HowHsu:fuzz-fuzzedsock-shared-clock changing 11 files +45 −40

HowHsu commented at 4:05 PM on June 15, 2026: contributor

This is a follow-up of #35478 (comment), inspired by maflcko .

Each FuzzedSock used to own its mocked steady clock and call MockableSteadyClock::SetMockTime() directly. Hold the clock by reference to an externally provided SteadyClockContext instead, so that several FuzzedSock instances sharing a test case (e.g. one per peer, or one created via Accept()) advance a single mocked clock, and the mocking goes through the SteadyClockContext RAII helper that resets mocktime on destruction.

SteadyClockContext is a LimitOne type, so each fuzz target constructs one instance per iteration and passes it to ConsumeSock / ConsumeNode / the FuzzedSock constructor.
DrahtBot added the label Fuzzing on Jun 15, 2026
DrahtBot commented at 4:05 PM on June 15, 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/35536.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK marcofleon, maflcko

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:
- #35482 (fuzz: exercise the transaction-handling path in process_message(s) by HowHsu)
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.
in src/test/fuzz/util/net.h:187 in 45f690bc45
```
 183 | +     * Externally-provided context used to mock the steady clock in methods
 184 | +     * waiting for a given duration. It is a reference (rather than an owned
 185 | +     * member) so that several FuzzedSock instances sharing a test case (e.g.
 186 | +     * one per peer, or one created from Accept()) advance a single mocked
 187 | +     * clock: SteadyClockContext is a LimitOne type and may not be
 188 | +     * instantiated more than once at a time.
```
maflcko commented at 7:11 AM on June 16, 2026:

LimitOne is an implementation detail, and I think the last sentence can be removed. Otherwise, all use-sites can explain what LimitOne means. If you want to explain it further, a better place would be SteadyClockContext itself, so that the docs are only in a single place, instead of spread out.

The underlying reason is that time is read/written from/to a single global variable. Holding several time contexts for different objects and advancing them independently is simply impossible without changing non-test code. LimitOne is just an implementation detail to serve as a linter to avoid violations here and explicitly disallow several time contexts at runtime.

HowHsu commented at 8:38 AM on June 16, 2026:

Updated
maflcko approved
maflcko commented at 7:12 AM on June 16, 2026: member

lgtm, but the new docs could be shortened.
maflcko commented at 7:16 AM on June 16, 2026: member

A bit unrelated, but I wonder if there should be a scripted-diff to rename SteadyClockContext to FakeSteadyClock, so that the naming follows FakeNodeClock. Otherwise, there are two naming schemes for two types of the same concept.
HowHsu force-pushed on Jun 16, 2026
fanquake commented at 9:56 AM on June 17, 2026: member

cc @marcofleon
HowHsu commented at 7:56 AM on June 18, 2026: contributor

A bit unrelated, but I wonder if there should be a scripted-diff to rename SteadyClockContext to FakeSteadyClock, so that the naming follows FakeNodeClock. Otherwise, there are two naming schemes for two types of the same concept.

Checked the code and agree. Submitted a PR: https://github.com/bitcoin/bitcoin/pull/35559
HowHsu requested review from maflcko on Jun 18, 2026
sedited referenced this in commit 27262a2884 on Jun 22, 2026

Type	Reviewers
ACK	marcofleon, maflcko

fuzz: share a single mocked steady clock across FuzzedSock instances

Each FuzzedSock used to own its mocked steady clock and call
MockableSteadyClock::SetMockTime() directly. Hold the clock by reference
to an externally provided FakeSteadyClock instead, so that several
FuzzedSock instances sharing a test case (e.g. one per peer, or one
created via Accept()) advance a single mocked clock, and the mocking goes
through the FakeSteadyClock RAII helper that resets mocktime on
destruction.

FakeSteadyClock is a LimitOne type, so each fuzz target constructs one
instance per iteration and passes it to ConsumeSock / ConsumeNode / the
FuzzedSock constructor.

6fa4132298

HowHsu force-pushed on Jun 23, 2026
HowHsu commented at 2:22 PM on June 23, 2026: contributor

Rebased due to https://github.com/bitcoin/bitcoin/pull/35559
in src/test/fuzz/pcp.cpp:38 in 6fa4132298
```
  31 | @@ -31,11 +32,12 @@ void port_map_target_init()
  32 |  FUZZ_TARGET(pcp_request_port_map, .init = port_map_target_init)
  33 |  {
  34 |      FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
  35 | +    FakeSteadyClock steady_clock;
  36 |  
  37 |      // Create a mocked socket between random (and potentially invalid) client and gateway addresses.
  38 |      CreateSock = [&](int domain, int type, int protocol) {
```
marcofleon commented at 12:28 PM on June 25, 2026:

Bit of an unrelated nit, so feel free to ignore. While touching these targets, could save and restore the global CreateSock like we do in i2p and connman. It gets overwritten before use every iteration so not a problem in practice, but it would be cleaner to not leave dangling references around.

HowHsu commented at 9:12 AM on June 27, 2026:

Thanks, makes sense. Since this is unrelated to the clock change and CreateSock is overwritten before use on every iteration, I’d prefer to leave it out of this PR and keep it as a possible follow-up cleanup.
marcofleon commented at 12:29 PM on June 25, 2026: contributor

crACK 6fa4132298a9da4fb59ea6d90346db467585db40

maflcko commented at 3:12 PM on June 26, 2026: member

review ACK 6fa4132298a9da4fb59ea6d90346db467585db40 🌕

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 6fa4132298a9da4fb59ea6d90346db467585db40   🌕
Ewqamc38vmlyl7B1AvaNnH3MXnst+RaHxVjGqCujhMROBniWI8IcJCaGap9Z3SvX6cXz2SPyqWV+XHfMJBD/Ag==

</details>

maflcko commented at 3:17 PM on June 26, 2026: member

I wonder if there is any meaningful difference in coverage, but I guess this may be too hard to find out due to the havoc effect? edit: Seems rfm either way, as I think it doesn't matter much.
HowHsu commented at 9:17 AM on June 27, 2026: contributor

I wonder if there is any meaningful difference in coverage, but I guess this may be too hard to find out due to the havoc effect? edit: Seems rfm either way, as I think it doesn't matter much.

Thanks for the review ACK. Agree that any coverage comparison here would probably be noisy due to the havoc effect. I may look into running some related tests later when I have some time.
sedited merged this on Jun 27, 2026
sedited closed this on Jun 27, 2026

Contributors

Review Requested
maflcko

Labels

Fuzzing

Linked (view graph)

#35478 fuzz: reset the mockable steady clock between iterations #35559 scripted-diff: Rename SteadyClockContext to FakeSteadyClock #35615 fuzz: restore CreateSock in PCP targets

fuzz: share a single mocked steady clock across FuzzedSock instances #35536

Code Coverage & Benchmarks

Reviews

Conflicts