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.
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.
22+ SEND, // Expect send (with optional data)
23+ RECV, // Expect receive (with data)
24+ NOP, // Just delay
25+ } op;
26+ std::vector<uint8_t> data;
27+ int error;
29+ TestOp(std::chrono::milliseconds delay_in, Op op_in, const std::vector<uint8_t> &data_in, int error_in):
30+ delay(delay_in), op(op_in), data(data_in), error(error_in) {}
31+};
32+
33+/// Save the value of CreateSock and restore when the test ends.
34+class PCPTestingSetup : public BasicTestingSetup
final.
This doesn’t work, boost apparently does derive from it.
0.../src/test/pcp_tests.cpp:260:22: error: cannot derive from ‘final’ base ‘PCPTestingSetup’ in derived type ‘pcp_tests::natpmp_ipv4’
1 260 | BOOST_AUTO_TEST_CASE(natpmp_ipv4)
2 | ^~~~~~~~~~~
3.../src/test/pcp_tests.cpp:311:22: error: cannot derive from ‘final’ base ‘PCPTestingSetup’ in derived type ‘pcp_tests::pcp_ipv4’
4 311 | BOOST_AUTO_TEST_CASE(pcp_ipv4)
5 | ^~~~~~~~
73+ const decltype(CreateSock) m_create_sock_orig;
74+};
75+
76+/** Simple scripted UDP server emulation for testing.
77+ */
78+class PCPTestSock : public Sock
final.
217+
218+private:
219+ const std::vector<TestOp> m_script;
220+ mutable size_t m_script_ptr = 0;
221+ mutable std::chrono::milliseconds m_time_left;
222+ mutable std::chrono::milliseconds m_time{1000};
MockableSteadyClock::SetMockTime(m_time) would disable mocking.
Correct. Thinking of it, we could define a constant for this in the MockableClock like INITIAL_MOCK_TIME. This would document it while at the same time as preventing that mistake being made in future tests that use the mockable steady clock.
The alternative was to make the mock time in MockableClock an optional, but this would mean it could no longer simply be an atomic value, so was kind of a hassle compared to starting at non-zero.
545+ * @param[in] paddr Pointer to sockaddr structure
546+ * @param[in] addrlen Length of sockaddr structure in bytes. This will be checked to exactly match the length of
547+ * a socket address of the provided family, unless std::nullopt is passed
548+ * @returns true on success
549+ */
550+ bool SetSockAddr(const struct sockaddr* paddr, std::optional<socklen_t> addrlen);
AddrLenFromFamily function or such), fine with me too.
48@@ -48,6 +49,27 @@ std::chrono::seconds GetMockTime()
49 return g_mock_time.load(std::memory_order_relaxed);
50 }
51
52+MockableSteadyClock::time_point MockableSteadyClock::now() noexcept
53+{
54+ const auto mocktime{g_mock_steady_time.load(std::memory_order_relaxed)};
For the merge conflict, isn’t this missing in the next line?
0 if (!mocktime.count()) {
1 g_used_system_time = true;
2 }
ACK a50e1b5af747134b3e102ef7867ee262ba410700
PROTOCOL_ERROR is the only variant of MappingError which is not covered by the unit test. If you feel that’s worth including it i wrote https://github.com/darosior/bitcoin/commit/be7482c2f6f754cfa4c205cff66ba9c689d426f5 to cover the 3 code paths in which it occurs.
PROTOCOL_ERROR is the only variant of MappingError which is not covered by the unit test. If you feel that’s worth including it i wrote https://github.com/darosior/bitcoin/commit/be7482c2f6f754cfa4c205cff66ba9c689d426f5 to cover the 3 code paths in which it occurs.
Thank you! Will pull that in.
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.
This adds a NodeSteadyClock, which is a steady_clock that can be mocked
with millisecond precision.
This will be needed for the test harness.
156+ if (!service.IsBindAny() && service != m_local_ip) {
157+ return -1;
158+ }
159+ m_bound = service;
160+ }
161+ return 0;
nit in the context of this test but i think you meant:
0 if (service.SetSockAddr(sa, sa_len)) {
1 // Can only bind to one of our local ips
2 if (!service.IsBindAny() && service != m_local_ip) {
3 return -1;
4 }
5 m_bound = service;
6 return 0;
7 }
8 return -1;
Bind when the network address fails to set makes more sense.
95+
96+ ~PCPTestSock() override { m_socket = INVALID_SOCKET; }
97+
98+ PCPTestSock& operator=(Sock&& other) override
99+ {
100+ assert(false && "Move of Sock into MockSock not allowed.");
nit
0 assert(false && "Move of Sock into PCPTestSock not allowed.");
168+ return nullptr;
169+ };
170+
171+ int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override
172+ {
173+ std::memset(opt_val, 0x0, *opt_len);
For sake of completeness. i think it’s uglier to fall through to the parent class, which would do an actual getsockopt call on an invalid socket if in the future the called code would use GetSockOpt.
i kind of wish there was a purely virtual BaseSock and PosixSock and TestSock etc were a subclass of this, so every subclass would be forced to implement everything.
Could replace with an assert(not implemented) stub. But it’s just one line…
673+ }, 0
674+ },
675+ {
676+ 250ms, TestOp::RECV, // 250ms delay before answer
677+ {
678+ 0x02, 0x81, 0x00, 0x42, // version, opcode, result success
nit
0 0x02, 0x81, 0x00, 0x42, // version, opcode, result error
91+ {
92+ ElapseTime(std::chrono::seconds(0)); // start mocking steady time
93+ PrepareOp();
94+ }
95+
96+ ~PCPTestSock() override { m_socket = INVALID_SOCKET; }
~Sock?
m_socket.
i copied this destructor from StaticContentsSock, which i think also does it redundantly.
91+ {
92+ ElapseTime(std::chrono::seconds(0)); // start mocking steady time
93+ PrepareOp();
94+ }
95+
96+ ~PCPTestSock() override { }
clang-tidy complains about this:
[09:51:32.084] [584/666][11.5s] clang-tidy-19 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/pcp_tests.cpp [09:51:32.084] /ci_container_base/src/test/pcp_tests.cpp:96:5: error: use ‘= default’ to define a trivial destructor [modernize-use-equals-default,-warnings-as-errors] [09:51:32.084] 96 | ~PCPTestSock() override { } [09:51:32.084] | ^ ~~~ [09:51:32.084] | = default; [09:51:32.084] 973 warnings generated.
i should probably find a way to run the linter stuff locally
In theory it became a little bit easier after cmake, but it still requires a configure with clang. The you can possibly use the clang-tidy-diff approach explained in the dev notes. Though, for me it is rare enough to hit, that I am just playing the ping-pong. :sweat_smile:
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.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/35719158891
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.