Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.
Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.
Includes “net: Add optional length checking to CService::SetSockAddr” from #31014 as a prerequisite.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31022.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
21@@ -22,6 +22,15 @@ struct NodeClock : public std::chrono::system_clock {
22 };
23 using NodeSeconds = std::chrono::time_point<NodeClock, std::chrono::seconds>;
24
25+/** Mockable steady clock in the context of tests, otherwise the system steady clock */
26+struct NodeSteadyClock : public std::chrono::steady_clock {
SteadyClock into this struct instead? Having both types could be confusing.
sure, fine with me, agree it’s less confusing (NodeSteadyClock says nothing at all) though i wasn’t sure which of the current usages should be mockable and didn’t want to decide that here
edit: i’ll rename the structure to something clearer like MockableSteadyClock how about that?
i wasn’t sure which of the current usages should be mockable and didn’t want to decide that here
fair enough!
MockableSteadyClock sgtm!
Should be unambigious now, i’ve renamed it and the methods to set/clear the mock time are included on the clock itself.
0/**
1 * Version of SteadyClock that is mockable in the context of tests (set the
2 * current value with SetMockTime), otherwise the system steady clock.
3 */
4struct MockableSteadyClock : public std::chrono::steady_clock {
5 using time_point = std::chrono::time_point<MockableSteadyClock>;
6 /** Return current system time or mocked time, if set */
7 static time_point now() noexcept;
8 static std::time_t to_time_t(const time_point&) = delete; // unused
9 static time_point from_time_t(std::time_t) = delete; // unused
10
11 /** Set mock time for testing. */
12 static void SetMockTime(std::chrono::milliseconds mock_time_in);
13
14 /** Clear mock time, go back to system steady clock. */
15 static void ClearMockTime();
16};
53+};
54+
55+void SetMockSteadyTime(std::chrono::milliseconds mock_time_in)
56+{
57+ Assert(mock_time_in >= 0s);
58+ g_mock_steady_time.store(mock_time_in, std::memory_order_relaxed);
My approach for this would have been to reuse SetMockTime, which would set g_mock_time to the requested mocktime and g_mock_steady_time to max(mock_time_in, g_mock_steady_time) (to ensure it only ever increases).
The benefit of that approach would be that all existing tests that make use of mock time would now also mock steady time (assuming NodeSteadyClock is used throughout).
I never implemented and tested this but I was curious if you considered this as well?
i don’t know.
i like being able to mock them seperately, and millisecond resolultion instead of second resolution is kind of nice here because it tends to be used for deadlines and timeouts, seconds are kind of clunky there.
also conceptually i think “max(mock_time_in, g_mock_steady_time)” is a wrong representation of how monotonic time works, it’s basically a CPU cycle counter (from system startup, usually), it isn’t linked to wall time or updates of that at all.
The reference point generally doesn’t matter, only differences do; in my current tests i in fact reset it in every test case.
in my current tests i in fact reset it in every test case.
Right, this wouldn’t work with my approach as this is a global.
Concept ACK
Left some comments on the approach for the steady mock time.
This adds a NodeSteadyClock, which is a steady_clock that can be mocked
with millisecond precision.
This will be needed for the test harness.
Add a mock for a simple scriptable UDP server, and use this to test
various code paths (including successful mappings, timeouts and errors)
in the PCP and NATPMP implementations.
