Add unit tests that write data to a mocked socket and inspect what CConnman/PeerManager have written back to the socket, or check the internal state to verify that the behavior is as expected.
This is now possible, after most of #21878 has been merged - we don't do any syscalls (e.g. connect(), recv()) from the high level code and using a mocked socket allows testing the entire networking stack without opening actual network connections.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/26812.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
pragma: export and other improvements by hebasto)src/util and treat them as errors by hebasto)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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):
No other suggestions were found.
<sup>2026-01-27 10:51:04</sup>
69 | + DebugLogHelper debug_log{"getblocks -1 to end"}; 70 | + pipes->recv.PushNetMsg(NetMsgType::GETBLOCKS, block_locator, hash_stop); 71 | + debug_log.EndAndThrowIfNotFound(30s); 72 | +} 73 | + 74 | +BOOST_AUTO_TEST_CASE(ping)
This is inspired by https://github.com/bitcoin/bitcoin/pull/25515
Key difference in #25515 is that it is testing net processing in isolation (at least isolated from CConnman), which imo is something we definitely should be doing. We already have e2e tests i.e. the functional tests, so I don't see the benefit to your approach here (besides the c++ tests being faster than the python ones).
Yes, both testing in isolation and e2e is good to have. The difference between the tests included in this PR and the functional tests is that when the test code runs inside the same process:
I think all those tests are complementary, not mutually exclusive.
The unit tests are what I run locally over and over, as they run so much more quickly. IIRC there was a project idea a few years ago to convert the functional tests in Python to C++. Having better unit test coverage, or shifting coverage from the Python tests to C++ ones, seems beneficial.
38 | + } \ 39 | + } const static g_##msg_type##count_before_main; \ 40 | + FUZZ_TARGET_INIT(netmsg_##msg_type, initialize_netmsg) \ 41 | + { \ 42 | + fuzz_target(buffer, #msg_type); \ 43 | + }
This is duplicated from src/test/fuzz/process_message.cpp. It should be deduplicated by putting it in a common header file, but I wonder if the new tests in fuzz/netmsg.cpp make the tests from fuzz/process_message.cpp and fuzz/process_messages.cpp obsolete and whether the latter should be removed. @MarcoFalke, as the author of process_message[s], what do you think?
Why not de-duplicate by putting it in the existing ./src/test/fuzz/process_message.cpp file?
You mean to put the entire contents of the newly introduced fuzz/netmsg.cpp inside fuzz/process_message.cpp?
yes
Looks plausible. I will try to do that and see how it will look.
Done. This also reduced the size of the diff (the last commit). Thanks!
I put the new e2e tests in a separate new file because this code (which I was trying to avoid duplicating) is gone after https://github.com/bitcoin/bitcoin/pull/27766
52 | + MineBlock(g_setup->m_node, CScript() << OP_TRUE); 53 | + } 54 | + SyncWithValidationInterfaceQueue(); 55 | +} 56 | + 57 | +static void fuzz_target(FuzzBufferType buffer, const std::string& limit_to_message_type)
This target is averaging 2 execs/s on my machine. A good target should average a 1000 execs/s (https://github.com/google/fuzzing/blob/master/docs/good-fuzz-target.md#speed).
I don't quite see the upside of this target over the process_message(s) targets. It might be able to find additional bugs if run with ThreadSanitizer but the current performance is gonna stop it from doing so effectively.
Fuzzing is particularly effective when the targets are kept narrow. For example: fuzzing net, net processing, validation or any of their sub components (e.g. TxRequestTracker or AddrMan) in isolation makes sense but all of it together will be inefficient.
Pushed an improvement that changes it to about 17 exec/s, it tends to increase if the test is left to run for some time.
IMO e2e fuzzing has some value.
IMO e2e fuzzing has some value.
Yes, but I think it should focus on what you are trying to test. If you keep the target to broad the fuzzer is gonna have a hard time finding interesting test cases (especially if the target is slow). One obvious way of focusing is to only fuzz smaller units of code and in the e2e case you probably want to narrow the way you are interpreting the fuzz input so that you are actually testing what you want to test.
The approach here currently seems to be very similar to the process_message(s) targets (which is why you suggested to replace them iiuc), in that you are interpreting most of the fuzz input as a sequence of arbitrary messages, so the questions that I have are:
process_message(s) (or other targets) are not? In other words which areas/interactions is this fuzzing that are previously not covered?
After running it for some time, the maximum exec/s I got was 21 exec/s which is extremely low. It's not all the targets that reach more than 1000 exec/s here but 21 is too slow.
"resolving" this thread because I removed the fuzz tests from this PR
96 | + BlockTransactionsRequest{.blockhash = chainstate.m_chain.Tip()->GetBlockHash(), 97 | + .indexes = {1, UINT16_MAX}}); 98 | + if (pipes->send.GetBytes(buf, sizeof(buf)) == 0) { 99 | + break; 100 | + } 101 | + std::this_thread::sleep_for(50ms);
I suspect this is the main reason for the poor performance, but the additional threading also doesn't help.
It wasn't but I ditched it anyway in 9b7e9dad27...daee83c1c5.
97 | + .indexes = {1, UINT16_MAX}}); 98 | + if (pipes->send.GetBytes(buf, sizeof(buf)) == 0) { 99 | + break; 100 | + } 101 | + std::this_thread::sleep_for(50ms); 102 | + }
I think this is missing a call to SyncWithValidationInterfaceQueue here.
Maybe. I am probably missing something. Can you elaborate why it is missing a call to SyncWithValidationInterfaceQueue()?
If (in theory) it is possible to push an event on the queue, it should be handled before starting the next input. (Not sure if applicable here)
Added.
9b7e9dad27...daee83c1c5: optimize the fuzz test a bit
32ab679f54...7c591c868d: rebase due to conflicts
There may be a silent merge conflict. After rebase to current master at fde224a6610699a6a28cc27e299ac14cbf7e16ca, building with Clang 16 fails at commit test: add a mocked Sock that allows inspecting what has been Send() to it.
<details><summary>build error</summary><p>
test/util/net.cpp:280:12: error: no viable conversion from returned value of type 'const CNetMessage' to function return type 'std::optional<CNetMessage>'
return msg;
^~~
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:736:41: note: candidate constructor not viable: no known conversion from 'const CNetMessage' to 'const optional<CNetMessage> &' for 1st argument
_LIBCPP_INLINE_VISIBILITY constexpr optional(const optional&) = default;
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:737:41: note: candidate constructor not viable: no known conversion from 'const CNetMessage' to 'optional<CNetMessage> &&' for 1st argument
_LIBCPP_INLINE_VISIBILITY constexpr optional(optional&&) = default;
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:738:41: note: candidate constructor not viable: no known conversion from 'const CNetMessage' to 'nullopt_t' for 1st argument
_LIBCPP_INLINE_VISIBILITY constexpr optional(nullopt_t) noexcept {}
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:762:15: note: candidate template ignored: requirement '_CheckOptionalArgsCtor<const CNetMessage &>::__enable_implicit()' was not satisfied [with _Up = const CNetMessage &]
constexpr optional(_Up&& __v)
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:777:35: note: candidate template ignored: could not match 'const optional<_Up>' against 'const CNetMessage'
_LIBCPP_CONSTEXPR_SINCE_CXX20 optional(const optional<_Up>& __v)
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:795:35: note: candidate template ignored: could not match 'optional<_Up>' against 'const CNetMessage'
_LIBCPP_CONSTEXPR_SINCE_CXX20 optional(optional<_Up>&& __v)
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:748:24: note: explicit constructor is not a candidate
constexpr explicit optional(_InPlaceT, _Args&&... __args)
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:769:24: note: explicit constructor is not a candidate
constexpr explicit optional(_Up&& __v)
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:785:44: note: explicit constructor is not a candidate
_LIBCPP_CONSTEXPR_SINCE_CXX20 explicit optional(const optional<_Up>& __v)
^
/opt/homebrew/opt/llvm/bin/../include/c++/v1/optional:803:44: note: explicit constructor is not a candidate
_LIBCPP_CONSTEXPR_SINCE_CXX20 explicit optional(optional<_Up>&& __v)
^
1 error generated.
make[2]: *** [test/util/libtest_util_a-net.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-recursive] Error 1
make: *** [all-recursive] Error 1
</p></details>
$ clang --version
Homebrew clang version 16.0.1
Target: arm64-apple-darwin22.4.0
Thread model: posix
a81fe4ff9b...f559067e27: rebase due to conflicts, thanks for looking into this!
f559067e27...4c867de996: rebase due to conflicts
79 | + LIMITED_WHILE(fuzzed_data_provider.remaining_bytes() > 0, 100) { 80 | + 81 | + const std::string message_type{ 82 | + LIMIT_TO_MESSAGE_TYPE.empty() ? 83 | + fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE) : 84 | + LIMIT_TO_MESSAGE_TYPE};
Changing the fuzz input format with a run-time setting seems fragile and not intuitive when one wants to re-use fuzz inputs or reproduce runs.
Are you suggesting to replace it with this:
const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE)};
if (!LIMIT_TO_MESSAGE_TYPE.empty() && random_message_type != LIMIT_TO_MESSAGE_TYPE) {
continue;
}
I would address your suggestion, but I am not sure I understand it. Is the above what you meant or did I get it wrong?
Yeah, I think you can just copy paste it from the other test, no?
Done. Now it is the same as in process_message.cpp.
It seems inefficient to me - if LIMIT_TO_MESSAGE_TYPE is set to e.g. filterclear the fuzzer has to brute force all possible strings with length 11 to find it? That is 2<sup>88</sup> possibilities.
A modern fuzz engine will read the LIMIT_TO_MESSAGE_TYPE and inject it into the fuzz input, so it shouldn't take more than a few seconds to guess. In any case it shouldn't take 2^88 tries
Marking this as resolved, let me know if you think it is not.
81 | + const std::string message_type{ 82 | + LIMIT_TO_MESSAGE_TYPE.empty() ? 83 | + fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE) : 84 | + LIMIT_TO_MESSAGE_TYPE}; 85 | + 86 | + CDataStream payload{
Could this use DataStream, or drop it completely?
Good point, dropped it!
4c867de996...35fc849412: address a suggestion (remove usage of CDataStream)
35fc849412...bedbdf4a15: address #26812 (review)
bedbdf4a15...b96bd52f3a: ditto, but move the check to the start of the test
b96bd52f3a...4557cc336f: rebase
12 | @@ -13,6 +13,7 @@ 13 | #include <pubkey.h> 14 | #include <random.h> 15 | #include <stdexcept> 16 | +#include <test/util/net.h>
Rather than including everything, it might be good to have a separate compilation unit for code that needs to be in util/setup_common, versus the current util/net code that is only needed for a handful of the tests.
I tried extracting the mocked sockets out from test/util/net.h into a separate test/util/mocked_sockets.h and then compared the re-compilation time if that is modified (e.g. something in DynSock declaration is changed). In both cases recompilation takes ~28 seconds, so it makes no difference (in practice).
Here is the change (on top of this PR):
<details> <summary>mocked_sockets.h</summary>
commit 398a2d5717b6ef6760e48eac92aa92127157a67a (HEAD -> e2e_tests)
Parent: 4557cc336fd8eb321b0db024b70213f46017071c
Author: Vasil Dimov <vd@FreeBSD.org>
AuthorDate: Wed Jun 28 09:10:41 2023 +0200
Commit: Vasil Dimov <vd@FreeBSD.org>
CommitDate: Wed Jun 28 09:10:41 2023 +0200
gpg: Signature made Wed Jun 28 09:11:03 2023 CEST
gpg: using RSA key E64D8D45614DB07545D9CCC154DF06F64B55CBBF
gpg: Good signature from "Vasil Dimov <vd@myforest.net>" [ultimate]
gpg: aka "Vasil Dimov <vd@FreeBSD.org>" [ultimate]
gpg: aka "Vasil Dimov <vasild@gmail.com>" [ultimate]
wip: mocked_sockets.h
diff --git a/src/test/util/net.h b/src/test/util/mocked_sockets.h
similarity index 67%
copy from src/test/util/net.h
copy to src/test/util/mocked_sockets.h
index b5ab7699f0..cdf6cb5d96 100644
--- a/src/test/util/net.h
+++ b/src/test/util/mocked_sockets.h
@@ -1,111 +1,27 @@
-// Copyright (c) 2020-2022 The Bitcoin Core developers
+// Copyright (c) 2023-2023 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-#ifndef BITCOIN_TEST_UTIL_NET_H
-#define BITCOIN_TEST_UTIL_NET_H
+#ifndef BITCOIN_TEST_UTIL_MOCKED_SOCKETS_H
+#define BITCOIN_TEST_UTIL_MOCKED_SOCKETS_H
#include <compat/compat.h>
#include <net.h>
-#include <netaddress.h>
#include <netmessagemaker.h>
-#include <node/eviction.h>
-#include <util/sock.h>
#include <sync.h>
+#include <util/sock.h>
+#include <version.h>
-#include <array>
-#include <cassert>
#include <chrono>
-#include <condition_variable>
-#include <cstring>
+#include <cstdint>
#include <memory>
#include <optional>
#include <string>
#include <vector>
-struct ConnmanTestMsg : public CConnman {
- using CConnman::CConnman;
-
- void SetPeerConnectTimeout(std::chrono::seconds timeout)
- {
- m_peer_connect_timeout = timeout;
- }
-
- void AddTestNode(CNode& node)
- {
- LOCK(m_nodes_mutex);
- m_nodes.push_back(&node);
- }
- void ClearTestNodes()
- {
- LOCK(m_nodes_mutex);
- for (CNode* node : m_nodes) {
- delete node;
- }
- m_nodes.clear();
- }
-
- void Handshake(CNode& node,
- bool successfully_connected,
- ServiceFlags remote_services,
- ServiceFlags local_services,
- int32_t version,
- bool relay_txs)
- EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
-
- void ProcessMessagesOnce(CNode& node) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) { m_msgproc->ProcessMessages(&node, flagInterruptMsgProc); }
-
- void NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const;
-
- bool ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const;
-};
-
-constexpr ServiceFlags ALL_SERVICE_FLAGS[]{
- NODE_NONE,
- NODE_NETWORK,
- NODE_BLOOM,
- NODE_WITNESS,
- NODE_COMPACT_FILTERS,
- NODE_NETWORK_LIMITED,
-};
-
-constexpr NetPermissionFlags ALL_NET_PERMISSION_FLAGS[]{
- NetPermissionFlags::None,
- NetPermissionFlags::BloomFilter,
- NetPermissionFlags::Relay,
- NetPermissionFlags::ForceRelay,
- NetPermissionFlags::NoBan,
- NetPermissionFlags::Mempool,
- NetPermissionFlags::Addr,
- NetPermissionFlags::Download,
- NetPermissionFlags::Implicit,
- NetPermissionFlags::All,
-};
-
-constexpr ConnectionType ALL_CONNECTION_TYPES[]{
- ConnectionType::INBOUND,
- ConnectionType::OUTBOUND_FULL_RELAY,
- ConnectionType::MANUAL,
- ConnectionType::FEELER,
- ConnectionType::BLOCK_RELAY,
- ConnectionType::ADDR_FETCH,
-};
-
-constexpr auto ALL_NETWORKS = std::array{
- Network::NET_UNROUTABLE,
- Network::NET_IPV4,
- Network::NET_IPV6,
- Network::NET_ONION,
- Network::NET_I2P,
- Network::NET_CJDNS,
- Network::NET_INTERNAL,
-};
-
-std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context);
-
/**
* A mocked Sock alternative that succeeds on all operations.
* Returns infinite amount of 0x0 bytes on reads.
*/
class ZeroSock : public Sock
{
@@ -257,7 +173,8 @@ void DynSock::Pipe::PushNetMsg(const std::string& type, Args&&... payload)
LOCK(m_mutex);
m_data.insert(m_data.end(), header.begin(), header.end());
m_data.insert(m_data.end(), msg.data.begin(), msg.data.end());
m_cond.notify_all();
}
-#endif // BITCOIN_TEST_UTIL_NET_H
+#endif // BITCOIN_TEST_UTIL_MOCKED_SOCKETS_H
+
diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
index ac866d7c89..1acdbf7061 100644
--- a/src/test/util/net.cpp
+++ b/src/test/util/net.cpp
@@ -8,12 +8,13 @@
#include <node/eviction.h>
#include <net.h>
#include <net_processing.h>
#include <netmessagemaker.h>
#include <span.h>
#include <sync.h>
+#include <test/util/mocked_sockets.h>
#include <chrono>
#include <optional>
#include <vector>
void ConnmanTestMsg::Handshake(CNode& node,
diff --git a/src/test/util/net.h b/src/test/util/net.h
index b5ab7699f0..7bad1ee4af 100644
--- a/src/test/util/net.h
+++ b/src/test/util/net.h
@@ -100,164 +100,7 @@ constexpr auto ALL_NETWORKS = std::array{
Network::NET_CJDNS,
Network::NET_INTERNAL,
};
std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context);
-/**
- * A mocked Sock alternative that succeeds on all operations.
- * Returns infinite amount of 0x0 bytes on reads.
- */
-class ZeroSock : public Sock
-{
-public:
- ZeroSock();
-
- ~ZeroSock() override;
-
- ssize_t Send(const void*, size_t len, int) const override;
-
- ssize_t Recv(void* buf, size_t len, int flags) const override;
-
- int Connect(const sockaddr*, socklen_t) const override;
-
- int Bind(const sockaddr*, socklen_t) const override;
-
- int Listen(int) const override;
-
- std::unique_ptr<Sock> Accept(sockaddr* addr, socklen_t* addr_len) const override;
-
- int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override;
-
- int SetSockOpt(int, int, const void*, socklen_t) const override;
-
- int GetSockName(sockaddr* name, socklen_t* name_len) const override;
-
- bool SetNonBlocking() const override;
-
- bool IsSelectable() const override;
-
- bool Wait(std::chrono::milliseconds timeout,
- Event requested,
- Event* occurred = nullptr) const override;
-
- bool WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const override;
-
-private:
- ZeroSock& operator=(Sock&& other) override;
-};
-
-/**
- * A mocked Sock alternative that returns a statically contained data upon read and succeeds
- * and ignores all writes. The data to be returned is given to the constructor and when it is
- * exhausted an EOF is returned by further reads.
- */
-class StaticContentsSock : public ZeroSock
-{
-public:
- explicit StaticContentsSock(const std::string& contents);
-
- /**
- * Return parts of the contents that was provided at construction until it is exhausted
- * and then return 0 (EOF).
- */
- ssize_t Recv(void* buf, size_t len, int flags) const override;
-
-private:
- StaticContentsSock& operator=(Sock&& other) override;
-
- const std::string m_contents;
- mutable size_t m_consumed{0};
-};
-
-/**
- * A mocked Sock alternative that allows providing the data to be returned by Recv()
- * and inspecting the data that has been supplied to Send().
- */
-class DynSock : public ZeroSock
-{
-public:
- /**
- * Unidirectional bytes or CNetMessage queue (FIFO).
- */
- class Pipe
- {
- public:
- /**
- * Get bytes and remove them from the pipe.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] buf Destination to write bytes to.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] len Write up to this number of bytes.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] flags Same as the flags of `recv(2)`. Just `MSG_PEEK` is honored.
- * [@return](/bitcoin-bitcoin/contributor/return/) The number of bytes written to `buf`. `0` if `Eof()` has been called.
- * If no bytes are available then `-1` is returned and `errno` is set to `EAGAIN`.
- */
- ssize_t GetBytes(void* buf, size_t len, int flags = 0) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
-
- /**
- * Deserialize a `CNetMessage` and remove it from the pipe.
- * If not enough bytes are available then the function will wait. If parsing fails
- * or EOF is signaled to the pipe, then `std::nullopt` is returned.
- */
- std::optional<CNetMessage> GetNetMsg() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
-
- /**
- * Push bytes to the pipe.
- */
- void PushBytes(const void* buf, size_t len) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
-
- /**
- * Construct and push CNetMessage to the pipe.
- */
- template <typename... Args>
- void PushNetMsg(const std::string& type, Args&&... payload) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
-
- /**
- * Signal end-of-file on the receiving end (`GetBytes()` or `GetNetMsg()`).
- */
- void Eof() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
-
- private:
- Mutex m_mutex;
- std::condition_variable m_cond;
- std::vector<uint8_t> m_data GUARDED_BY(m_mutex);
- bool m_eof GUARDED_BY(m_mutex){false};
- };
-
- struct Pipes {
- Pipe recv;
- Pipe send;
- };
-
- explicit DynSock(std::shared_ptr<Pipes> pipes);
-
- ~DynSock();
-
- ssize_t Recv(void* buf, size_t len, int flags) const override;
-
- ssize_t Send(const void* buf, size_t len, int) const override;
-
- bool Wait(std::chrono::milliseconds timeout,
- Event requested,
- Event* occurred = nullptr) const override;
-
- bool WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_per_sock) const override;
-
-private:
- DynSock& operator=(Sock&&) override;
-
- std::shared_ptr<Pipes> m_pipes;
-};
-
-template <typename... Args>
-void DynSock::Pipe::PushNetMsg(const std::string& type, Args&&... payload)
-{
- auto msg = CNetMsgMaker(INIT_PROTO_VERSION).Make(type, std::forward<Args>(payload)...);
- std::vector<unsigned char> header;
- V1TransportSerializer().prepareForTransport(msg, header);
-
- LOCK(m_mutex);
- m_data.insert(m_data.end(), header.begin(), header.end());
- m_data.insert(m_data.end(), msg.data.begin(), msg.data.end());
- m_cond.notify_all();
-}
-
#endif // BITCOIN_TEST_UTIL_NET_H
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index d3137fa69e..1daac4a57c 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -10,13 +10,13 @@
#include <node/caches.h>
#include <node/context.h> // IWYU pragma: export
#include <primitives/transaction.h>
#include <pubkey.h>
#include <random.h>
#include <stdexcept>
-#include <test/util/net.h>
+#include <test/util/mocked_sockets.h>
#include <util/chaintype.h>
#include <util/check.h>
#include <util/fs.h>
#include <util/string.h>
#include <util/vector.h>
</details>
Or did you mean something else?
(No opinion on the change), just
In both cases recompilation takes ~28 seconds
Seems odd that re-compilation of all tests takes only 28 seconds when mocked_sockets.h is modified, which is included in setup_common.h, which is included in all test files, no?
Compiling everything from scratch (after git clean -fdx) takes about 1m 30sec (including autogen and ./configure which are single-threaded). The CPU on the machine is:
AMD Ryzen 9 7950X 16-Core Processor (4491.70-MHz K8-class CPU)
For a useful benchmark, you'll have to measure the total CPU time, not the CPU time of just the translation unit that takes the longest.
PR:
real 0m29.623s
user 10m58.337s
sys 0m51.187s
real 0m29.829s
user 10m58.359s
sys 0m50.912s
mocked_sockets.h:
real 0m29.528s
user 11m12.208s
sys 1m0.406s
real 0m28.831s
user 10m45.615s
sys 0m49.680s
I guess the "total CPU time" is real+sys from above. It all seems within noise. In both cases all tests would be recompiled, maybe I got @jonatack's comment wrong?
I guess the "total CPU time" is real+sys from above
user+sys, but thanks for re-checking.
Thanks for looking into it. I like the idea of keeping them separate and avoiding including unneeded code, in general and for building with slower, older CPUs. So 👍 for your diff in #26812 (review). Great work so far AFAICT. Will continue reviewing the remaining commits.
In both cases all tests would be recompiled
Yes, but future changes to util/net would not require compiling all the tests.
Alright, so I got it the other way around. So the point is to avoid recompiling all tests if e.g. ALL_NETWORKS is changed (defined in test/util/net.h). That lowers the re-compilation from 28-29 sec to about 10 sec (if src/test/util/net.h is modified).
I will leave it as it is because moving the code around would further increase the size of this PR which suffers from lack of interest from reviewers and I think expanding it may further turn people away.
37 | - banman.GetBanned(entries_read); 38 | - BOOST_CHECK_EQUAL(entries_read.size(), 1); 39 | - } 40 | + // The invalid entries will be dropped, but the valid one remains 41 | + DebugLogHelper debug_log1{"Dropping entry with unparseable address or subnet (aaaaaaaaa) from ban list"}; 42 | + DebugLogHelper debug_log2{"Dropping entry with unknown version (2) from ban list"};
d2f46c7 Verified the behavior improvement in this commit by changing this line to make it fail.
<details><summary>master</summary><p>
$ ./src/test/test_bitcoin -t banman_tests
Running 1 test case...
libc++abi: terminating due to uncaught exception of type std::runtime_error: 'Dropping entry with unknown version (2) rom ban list' not found in debug log
******** errors disabling the alternate stack:
#error:22
Invalid argument
unknown location:0: fatal error: in "banman_tests/file": signal: SIGABRT (application abort requested)
test/banman_tests.cpp:38: last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
</p></details>
<details><summary>this commit</summary><p>
$ ./src/test/test_bitcoin -t banman_tests
Running 1 test case...
unknown location:0: fatal error: in "banman_tests/file": std::runtime_error: 'Dropping entry with unknown version (2) rom ban list' not found in debug log
test/banman_tests.cpp:37: last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
</p></details>
Reviewed/built/tested the first four commits up to https://github.com/bitcoin/bitcoin/commit/d2f46c705540c74c2b6f83a66535c3ead1cb95d4.
112 | @@ -113,6 +113,7 @@ BITCOIN_TESTS =\ 113 | test/net_peer_eviction_tests.cpp \ 114 | test/net_tests.cpp \ 115 | test/netbase_tests.cpp \ 116 | + test/netmsg_tests.cpp \
37cd2f5 naming: maybe call the test file net_msg_tests.cpp (and move this to line 113)
Done.
27 | + // Wait for all messages due to the initial handshake to be Send() to the socket. 28 | + // The FEEFILTER is the last one, so quit when we get that. 29 | + for (;;) { 30 | + auto msg = pipes->send.GetNetMsg(); 31 | + if (msg.has_value()) { 32 | + count_sent_messages[msg->m_type]++;
37cd2f5
<details><summary>non-blocking suggestions: make pipes const, prefix iterator (per our developer notes, even if compilers optimize for it), reduce nesting</summary><p>
@@ -22,20 +22,15 @@ BOOST_FIXTURE_TEST_SUITE(netmsg_tests, NetTestingSetup)
BOOST_AUTO_TEST_CASE(initial_messages_exchange)
{
std::unordered_map<std::string, size_t> count_sent_messages;
- auto pipes = m_sockets_pipes.PopFront();
+ const auto pipes{m_sockets_pipes.PopFront()};
// Wait for all messages due to the initial handshake to be Send() to the socket.
// The FEEFILTER is the last one, so quit when we get that.
for (;;) {
auto msg = pipes->send.GetNetMsg();
- if (msg.has_value()) {
- count_sent_messages[msg->m_type]++;
- if (msg->m_type == NetMsgType::FEEFILTER) {
- break;
- }
- } else {
- break;
- }
+ if (!msg.has_value()) break;
+ ++count_sent_messages[msg->m_type];
+ if (msg->m_type == NetMsgType::FEEFILTER) break;
}
BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::VERSION], 1);
@@ -51,7 +46,7 @@ BOOST_AUTO_TEST_CASE(initial_messages_exchange)
BOOST_AUTO_TEST_CASE(addr)
{
- auto pipes = m_sockets_pipes.PopFront();
+ const auto pipes{m_sockets_pipes.PopFront()};
std::vector<CAddress> addresses{5};
DebugLogHelper debug_log{strprintf("Received addr: %u addresses", addresses.size())};
@@ -61,7 +56,7 @@ BOOST_AUTO_TEST_CASE(addr)
BOOST_AUTO_TEST_CASE(getblocks)
{
- auto pipes = m_sockets_pipes.PopFront();
+ const auto pipes{m_sockets_pipes.PopFront()};
std::vector<uint256> hashes{5};
CBlockLocator block_locator{std::move(hashes)};
uint256 hash_stop;
@@ -73,7 +68,7 @@ BOOST_AUTO_TEST_CASE(getblocks)
BOOST_AUTO_TEST_CASE(ping)
{
- auto pipes = m_sockets_pipes.PopFront();
+ const auto pipes{m_sockets_pipes.PopFront()};
auto WaitForPingStats = [this](std::chrono::microseconds min,
std::chrono::microseconds last,
@@ -173,7 +168,7 @@ BOOST_AUTO_TEST_CASE(ping)
BOOST_AUTO_TEST_CASE(redundant_verack)
{
- auto pipes = m_sockets_pipes.PopFront();
+ const auto pipes{m_sockets_pipes.PopFront()};
</p></details>
Done.
32 | +#include <cstdlib> 33 | +#include <iostream> 34 | +#include <memory> 35 | +#include <string> 36 | +#include <string_view> 37 | +#include <vector>
https://github.com/bitcoin/bitcoin/commit/37cd2f5a358f1d21c11efa8783be65570418c648 and 4557cc3
Have a look at https://cirrus-ci.com/task/4730456604672000 for IWYU suggestions on the new files.
Done, except this:
test/net_msg_tests.cpp should add these lines:
#include <boost/preprocessor/arithmetic/limits/dec_256.hpp>
#include <boost/preprocessor/comparison/limits/not_equal_256.hpp>
#include <boost/preprocessor/control/expr_iif.hpp>
#include <boost/preprocessor/control/iif.hpp>
#include <boost/preprocessor/detail/limits/auto_rec_256.hpp>
#include <boost/preprocessor/logical/compl.hpp>
#include <boost/preprocessor/logical/limits/bool_256.hpp>
#include <boost/preprocessor/repetition/detail/limits/for_256.hpp>
#include <boost/preprocessor/repetition/for.hpp>
#include <boost/preprocessor/seq/limits/elem_256.hpp>
#include <boost/preprocessor/seq/limits/size_256.hpp>
#include <boost/preprocessor/tuple/elem.hpp>
#include <boost/preprocessor/variadic/limits/elem_64.hpp>
#include <boost/test/tools/old/interface.hpp>
#include <boost/test/unit_test_log.hpp>
#include <boost/test/unit_test_suite.hpp>
#include <boost/test/utils/basic_cstring/basic_cstring.hpp>
#include <boost/test/utils/lazy_ostream.hpp>
test/net_msg_tests.cpp should remove these lines:
- #include <boost/test/unit_test.hpp> // lines 18-18
which seems strange.
55 | + 56 | + static const auto testing_setup = MakeNoLogFileContext<NetTestingSetup>( 57 | + /*chain_type=*/ChainType::REGTEST, 58 | + /*extra_args=*/{"-txreconciliation"}); 59 | + g_setup = testing_setup.get(); 60 | + for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
https://github.com/bitcoin/bitcoin/commit/4557cc336fd8eb321b0db024b70213f46017071c
<details><summary>non-blocking suggestions</summary><p>
@@ -57,7 +57,7 @@ void initialize_process_message_e2e()
/*chain_type=*/ChainType::REGTEST,
/*extra_args=*/{"-txreconciliation"});
g_setup = testing_setup.get();
- for (int i = 0; i < 2 * COINBASE_MATURITY; i++) {
+ for (int i = 0; i < 2 * COINBASE_MATURITY; ++i) {
MineBlock(g_setup->m_node, CScript() << OP_TRUE);
}
SyncWithValidationInterfaceQueue();
@@ -75,9 +75,9 @@ FUZZ_TARGET_INIT(process_message_e2e, initialize_process_message_e2e)
SetMockTime(GetMockTime() + std::chrono::minutes{fuzzed_data_provider.ConsumeIntegral<uint8_t>()});
g_setup->m_node.connman->OpenNetworkConnection(
- CAddress{}, false, nullptr, "1.2.3.4:8333", ConnectionType::OUTBOUND_FULL_RELAY);
+ CAddress{}, /*fCountFailure=*/false, /*grantOutbound=*/nullptr, "1.2.3.4:8333", ConnectionType::OUTBOUND_FULL_RELAY);
- auto pipes = g_setup->m_sockets_pipes.PopFront();
+ const auto pipes{g_setup->m_sockets_pipes.PopFront()};
uint8_t buf[1024];
</p></details>
Done.
I changed it to use {} as suggested but I don't see a point to use {} instead of = for initialization when auto is used. I find the {} variant a little bit less readable but see the merit to use it when Type1 x{expresson_of_Type2}; is used to detect incompatibilities between Type1 and Type2. But why do that for auto x{expressoin_of_any_type};?
With C++11, my understanding was to avoid auto with uniform (braced) initialization because in some cases it could have unexpected results. Since C++17, that was mostly fixed and the CPP Guideline is to prefer the {}-initializer syntax as its rules are simpler, more general, less ambiguous, and safer. See also https://ianyepan.github.io/posts/cpp-uniform-initialization/. Though it didn't matter in this case, IIUC, so I didn't mention it explicitly.
I am still not convinced about auto and brace-initialization.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax mentions:
Use = only when you are sure that there can be no narrowing conversions For built-in arithmetic types, use = only with auto
There will not be narrowing conversions with auto x = expression;
One of the examples down there uses auto p = ....
ACK 4557cc336fd8eb321b0db024b70213f46017071c
Happy to re-review for #26812 (review) and any of the suggestions below.
ACK 612ba17fcadc955c142f21f9b1af0b60f0def55f
34 | + 35 | +void DebugLogHelper::EndAndThrowIfNotFound() 36 | +{ 37 | + StopReceivingLog(); 38 | + if (!m_found) { 39 | throw std::runtime_error(strprintf("'%s' not found in debug log\n", m_message));
Not sure about changing this. It just makes it harder to use. If you prefer to not have an exception, this can use an Assert? Also, in the commit description, it would be good to refer to documentation why it is "not allowed", and/or include an example of what happens.
I will look into replacing this with Assert().
"exception from destructor in c++" is a topic well covered all over the internet. The compiler produces a warning for that. Here is an example:
class A
{
public:
~A() {
std::cout << "before throw a\n";
// warning: '~A' has a non-throwing exception specification but can still throw [-Wexceptions]
//171 | throw std::runtime_error("a");
// | ^
throw std::runtime_error("a");
}
};
int main(int, char**)
{
try {
A a;
std::cout << "before throw b\n";
throw std::runtime_error("b");
} catch (const std::exception& e) {
std::cout << "catch\n"; // never reached
}
return 0;
}
$ ./t
before throw b
before throw a
Abort trap (core dumped)
$
I will look into improving the commit message as well.
Yeah, but this being an "Abort trap" is well defined, not "not allowed". And I think the tests aborting on failure is fine.
Changed to use std::abort() in the destructor. Reverted to the original interface:
{
ASSERT_DEBUG_LOG("expected message");
produce log messages;
}
The change was necessary in order to pass the timeout to the "final check or abort" function which was the destructor and there is no way to pass parameters to the destructor. Anyway, now I pass the timeout to the constructor, save it in a member variable and use it in the destructor. With the wait it is now:
{
ASSERT_DEBUG_LOG_WAIT("expected message", 30s);
produce log messages;
} // the destructor will wait 30s for the message to be logged before aborting
Marking this as "resolved" because I reverted to the original interface of ASSERT_DEBUG_LOG() and changed the exception from the destructor to std::abort().
Let me know if you still have concerns (aka if I should "unresolve" it). Thanks, it looks better now!
Not sure about changing the framework.
Also, needs rebase if still relevant.
needs rebase if still relevant.
For the previous releases CI? (I don't find any merge conflicts with current master.)
I don't find any merge conflicts with current master.
Are you sure you ran a full build after the merge?
Ah, the second try did it :)
<details><summary>build log</summary><p>
test/fuzz/process_message_e2e.cpp:59:1: error: a type specifier is required for all declarations
FUZZ_TARGET_INIT(process_message_e2e, initialize_process_message_e2e)
^
test/fuzz/process_message_e2e.cpp:59:18: error: use of undeclared identifier 'process_message_e2e'
FUZZ_TARGET_INIT(process_message_e2e, initialize_process_message_e2e)
^
test/fuzz/process_message_e2e.cpp:59:70: error: expected ';' after top level declarator
FUZZ_TARGET_INIT(process_message_e2e, initialize_process_message_e2e)
^
;
3 errors generated.
make[2]: *** [test/fuzz/fuzz-process_message_e2e.o] Error 1
make[2]: *** Waiting for unfinished jobs....
test/validation_chainstatemanager_tests.cpp:664:9: error: use of undeclared identifier 'ASSERT_DEBUG_LOG'
ASSERT_DEBUG_LOG("failed to validate the -assumeutxo snapshot state");
^
1 error generated.
make[2]: *** [test/test_bitcoin-validation_chainstatemanager_tests.o] Error 1
wallet/test/walletload_tests.cpp:61:20: error: no matching constructor for initialization of 'DebugLogHelper'
DebugLogHelper logHelper("The descriptor ID calculated by the wallet differs from the one in DB", [&](const std::string* s) {
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./test/util/logging.h:23:14: note: candidate constructor not viable: no known conversion from '(lambda at wallet/test/walletload_tests.cpp:61:103)' to 'MatchFn' (aka 'function<bool (const basic_string<char> &)>') for 2nd argument
explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string&){ return true; });
^
./test/util/logging.h:18:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
class DebugLogHelper
^
1 error generated.
</p></details>
612ba17fca...55c84c2d3b: rebase due to conflicts plus reduce the changes to DebugLogHelper: #26812 (review).
67 | + void StopReceivingLog(); 68 | }; 69 | 70 | #define ASSERT_DEBUG_LOG(message) DebugLogHelper UNIQUE_NAME(debugloghelper)(message) 71 | 72 | +#define ASSERT_DEBUG_LOG_WAIT(message, timeout) \
It is already ugly to have a timeout in the functional tests, and use it to sync them. (At least this one is std::optional)
However, my preference would still be to not have this and instead force the caller to sync.
Otherwise this will just lead to brittle code down the line and potentially intermittent test issues when running with libc++ sanitizers in valgrind on arm64, etc.
Your concern is that the timeout will turn out not to be enough in some slow environments?
When you say "force the caller to sync" do you mean something like this:
{
ASSERT_DEBUG_LOG("foo");
dostuff
somehow ensure the code that is supposed to log "foo" has executed
} // fail if "foo" is not in the log as soon as this is reached, like in `master`
?
Yes.
For example, if you are waiting for the scheduler to execute and log a validationinterface event, you can call SyncWithValidationInterfaceQueue() to sync without a timeout.
Thinking about this - when one thread triggers an event which another thread is supposed to pick and do something about it and the first thread wants to confirm that was done, then some waiting must be involved in the first thread.
SyncWithValidationInterfaceQueue() uses https://en.cppreference.com/w/cpp/thread/future/wait which is unlimited, without a timeout. In practice I guess that means a 2h (?) timeout until some guard in the CI kills the waiting test.
Syncing in the test, explicitly, outside of ASSERT_DEBUG_LOG_WAIT() would require putting sync points in various places in the high-level code - where a flag is raised that e.g. "a ping has been send", or "we have received GETBLOCKS message" and then the test waits for a particular flag to be raised. If that is done then ASSERT_DEBUG_LOG* is unnecessary. In the current approach the log messages are used as sync points. I think that is better because it does not require littering the high-level code with test-specific sync-points/flags.
What about changing ASSERT_DEBUG_LOG_WAIT() to wait infinitely, no timeout? Then it would be similar to SyncWithValidationInterfaceQueue() - a test failure due to ~message not logged~ the expected code not executed by the other thread would be designated not by a "timeout: message not found in log" after 30s but by infinite wait/hang of the test.
Not sure about an infinite timeout for ASSERT_DEBUG_LOG. For example, this can easily be hit when the log string is modified by a developer, turning a (doc) change in one place into a hard to debug unit test issue.
I know that it is possible to hit an infinite runtime in any other place, but that would generally mean a major fault or injected bug elsewhere. For example, if the scheduler thread is stopped and one called SyncWithValidationInterfaceQueue(), it would be quite obvious from the diff why the unit test never finishes. Alternatively, SyncWithValidationInterfaceQueue() could be extended with an assert to check the thread is running to map an infinitive runtime to an immediate crash to catch this type of bug, if it is expected to happen to at least once.
It would be good to explain why you need the debug log to sync and why it is not (easily) possible to just sync in this pull.
Not sure about an infinite timeout for
ASSERT_DEBUG_LOG. For example, this can easily be hit when the log string is modified by a developer, turning a (doc) change in one place into a hard to debug unit test issue.
I agree. I will stick to a meaningful message after a timeout instead of infinite wait (without a message).
I know that it is possible to hit an infinite runtime in any other place, but that would generally mean a major fault or injected bug elsewhere. For example, if the scheduler thread is stopped and one called
SyncWithValidationInterfaceQueue()...
I agree with that too. But the parallel with SyncWithValidationInterfaceQueue() is not very close to the tests in this PR because SyncWithValidationInterfaceQueue() does not provide fine-grained sync points, see below what I mean.
It would be good to explain why you need the debug log to sync and why it is not (easily) possible to just sync in this pull.
It is possible to not use the debug log. Here is how it would look like:
<details> <summary>[patch] use fine-grained sync points in high-level code</summary>
diff --git i/src/net_processing.cpp w/src/net_processing.cpp
index 1c73ca4eb4..35301a6a2d 100644
--- i/src/net_processing.cpp
+++ w/src/net_processing.cpp
@@ -3529,12 +3529,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
// At this point, the outgoing message serialization version can't change.
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());
if (msg_type == NetMsgType::VERACK) {
if (pfrom.fSuccessfullyConnected) {
LogPrint(BCLog::NET, "ignoring redundant verack message from peer=%d\n", pfrom.GetId());
+ g_stage_redundant_verack.post();
return;
}
if (!pfrom.IsInboundConn()) {
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s%s (%s)\n",
@@ -3781,12 +3782,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
}
peer->m_addr_processed += num_proc;
peer->m_addr_rate_limited += num_rate_limit;
LogPrint(BCLog::NET, "Received addr: %u addresses (%u processed, %u rate-limited) from peer=%d\n",
vAddr.size(), num_proc, num_rate_limit, pfrom.GetId());
+ g_stage_received_addr.post();
+
m_addrman.Add(vAddrOk, pfrom.addr, 2h);
if (vAddr.size() < 1000) peer->m_getaddr_sent = false;
// AddrFetch: Require multiple addresses to avoid disconnecting on self-announcements
if (pfrom.IsAddrFetchConn() && vAddr.size() > 1) {
LogPrint(BCLog::NET, "addrfetch connection completed peer=%d; disconnecting\n", pfrom.GetId());
@@ -3948,12 +3951,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
// Send the rest of the chain
if (pindex)
pindex = m_chainman.ActiveChain().Next(pindex);
int nLimit = 500;
LogPrint(BCLog::NET, "getblocks %d to %s limit %d from peer=%d\n", (pindex ? pindex->nHeight : -1), hashStop.IsNull() ? "end" : hashStop.ToString(), nLimit, pfrom.GetId());
+ if (pindex == nullptr && hashStop.IsNull()) {
+ g_stage_getblocks_all.post();
+ }
for (; pindex; pindex = m_chainman.ActiveChain().Next(pindex))
{
if (pindex->GetBlockHash() == hashStop)
{
LogPrint(BCLog::NET, " getblocks stopping at %d %s\n", pindex->nHeight, pindex->GetBlockHash().ToString());
break;
@@ -4765,26 +4771,31 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
} else {
// This should never happen
sProblem = "Timing mishap";
}
} else {
// Nonce mismatches are normal when pings are overlapping
- sProblem = "Nonce mismatch";
if (nonce == 0) {
// This is most likely a bug in another implementation somewhere; cancel this ping
bPingFinished = true;
sProblem = "Nonce zero";
+ g_stage_ping_nonce_zero.post();
+ } else {
+ sProblem = "Nonce mismatch";
+ g_stage_ping_nonce_mismatch.post();
}
}
} else {
sProblem = "Unsolicited pong without ping";
+ g_stage_unsolicited_pong_without_ping.post();
}
} else {
// This is most likely a bug in another implementation somewhere; cancel this ping
bPingFinished = true;
sProblem = "Short payload";
+ g_stage_ping_short_payload.post();
}
if (!(sProblem.empty())) {
LogPrint(BCLog::NET, "pong peer=%d: %s, %x expected, %x received, %u bytes\n",
pfrom.GetId(),
sProblem,
@@ -5259,12 +5270,13 @@ void PeerManagerImpl::MaybeSendPing(CNode& node_to, Peer& peer, std::chrono::mic
m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING, nonce));
} else {
// Peer is too old to support ping command with nonce, pong will never arrive.
peer.m_ping_nonce_sent = 0;
m_connman.PushMessage(&node_to, msgMaker.Make(NetMsgType::PING));
}
+ g_stage_sending_ping.post();
}
}
void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::microseconds current_time)
{
// Nothing to do for non-address-relay peers
@@ -5928,6 +5940,15 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
if (!vGetData.empty())
m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
} // release cs_main
MaybeSendFeefilter(*pto, *peer, current_time);
return true;
}
+
+CSemaphore g_stage_received_addr{0};
+CSemaphore g_stage_getblocks_all{0};
+CSemaphore g_stage_sending_ping{0};
+CSemaphore g_stage_ping_short_payload{0};
+CSemaphore g_stage_unsolicited_pong_without_ping{0};
+CSemaphore g_stage_ping_nonce_mismatch{0};
+CSemaphore g_stage_ping_nonce_zero{0};
+CSemaphore g_stage_redundant_verack{0};
diff --git i/src/net_processing.h w/src/net_processing.h
index 1bd2b1e8ca..de4a12705f 100644
--- i/src/net_processing.h
+++ w/src/net_processing.h
@@ -91,7 +91,16 @@ public:
const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
/** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */
virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;
};
+extern CSemaphore g_stage_received_addr;
+extern CSemaphore g_stage_getblocks_all;
+extern CSemaphore g_stage_sending_ping;
+extern CSemaphore g_stage_ping_short_payload;
+extern CSemaphore g_stage_unsolicited_pong_without_ping;
+extern CSemaphore g_stage_ping_nonce_mismatch;
+extern CSemaphore g_stage_ping_nonce_zero;
+extern CSemaphore g_stage_redundant_verack;
+
#endif // BITCOIN_NET_PROCESSING_H
diff --git i/src/test/net_msg_tests.cpp w/src/test/net_msg_tests.cpp
index 49c97c1440..17bd74cb3a 100644
--- i/src/test/net_msg_tests.cpp
+++ w/src/test/net_msg_tests.cpp
@@ -63,25 +63,27 @@ BOOST_AUTO_TEST_CASE(initial_messages_exchange)
BOOST_AUTO_TEST_CASE(addr)
{
const auto pipes{m_sockets_pipes.PopFront()};
std::vector<CAddress> addresses{5};
- ASSERT_DEBUG_LOG_WAIT(strprintf("Received addr: %u addresses", addresses.size()), 30s);
+ BOOST_REQUIRE(!g_stage_received_addr.try_wait());
pipes->recv.PushNetMsg(NetMsgType::ADDRV2, addresses);
+ g_stage_received_addr.wait();
}
BOOST_AUTO_TEST_CASE(getblocks)
{
const auto pipes{m_sockets_pipes.PopFront()};
std::vector<uint256> hashes{5};
CBlockLocator block_locator{std::move(hashes)};
uint256 hash_stop;
- ASSERT_DEBUG_LOG_WAIT("getblocks -1 to end", 30s);
+ BOOST_REQUIRE(!g_stage_getblocks_all.try_wait());
pipes->recv.PushNetMsg(NetMsgType::GETBLOCKS, block_locator, hash_stop);
+ g_stage_getblocks_all.wait();
}
BOOST_AUTO_TEST_CASE(ping)
{
const auto pipes{m_sockets_pipes.PopFront()};
@@ -132,14 +134,16 @@ BOOST_AUTO_TEST_CASE(ping)
}
}
};
auto SendPing = [&]() {
{
- ASSERT_DEBUG_LOG_WAIT("sending ping", 30s);
+ while (g_stage_sending_ping.try_wait())
+ ;
SetMockTime(GetMockTime() + PING_INTERVAL + 1s);
+ g_stage_sending_ping.wait();
}
return GetPingNonceSent();
};
BOOST_TEST_MESSAGE("Ensure initial messages exchange has completed with the sending of a ping "
"with nonce != 0 and the ping stats indicate a pending ping.");
@@ -147,37 +151,41 @@ BOOST_AUTO_TEST_CASE(ping)
auto time_elapsed = 1s;
SetMockTime(GetMockTime() + time_elapsed);
WaitForPingStats(/*min=*/std::chrono::microseconds::max(), /*last=*/0us, /*wait=*/time_elapsed);
BOOST_TEST_MESSAGE("Check that receiving a PONG without nonce cancels our PING");
{
- ASSERT_DEBUG_LOG_WAIT("Short payload", 30s);
+ BOOST_REQUIRE(!g_stage_ping_short_payload.try_wait());
pipes->recv.PushNetMsg(NetMsgType::PONG);
+ g_stage_ping_short_payload.wait();
}
WaitForPingStats(/*min=*/std::chrono::microseconds::max(), /*last=*/0us, /*wait=*/0us);
BOOST_TEST_MESSAGE("Check that receiving an unrequested PONG is logged and ignored");
{
- ASSERT_DEBUG_LOG_WAIT("Unsolicited pong without ping", 30s);
+ BOOST_REQUIRE(!g_stage_unsolicited_pong_without_ping.try_wait());
pipes->recv.PushNetMsg(NetMsgType::PONG, /*nonce=*/uint64_t{0});
+ g_stage_unsolicited_pong_without_ping.wait();
}
BOOST_TEST_MESSAGE("Check that receiving a PONG with the wrong nonce does not cancel our PING");
uint64_t nonce{SendPing()};
{
- ASSERT_DEBUG_LOG_WAIT("Nonce mismatch", 30s);
+ BOOST_REQUIRE(!g_stage_ping_nonce_mismatch.try_wait());
pipes->recv.PushNetMsg(NetMsgType::PONG, nonce + 1);
+ g_stage_ping_nonce_mismatch.wait();
}
time_elapsed = 5s;
SetMockTime(GetMockTime() + time_elapsed);
WaitForPingStats(/*min=*/std::chrono::microseconds::max(), /*last=*/0us, /*wait=*/time_elapsed);
BOOST_TEST_MESSAGE("Check that receiving a PONG with nonce=0 cancels our PING");
{
- ASSERT_DEBUG_LOG_WAIT("Nonce zero", 30s);
+ BOOST_REQUIRE(!g_stage_ping_nonce_zero.try_wait());
pipes->recv.PushNetMsg(NetMsgType::PONG, /*nonce=*/uint64_t{0});
+ g_stage_ping_nonce_zero.wait();
}
WaitForPingStats(/*min=*/std::chrono::microseconds::max(), /*last=*/0us, /*wait=*/0us);
BOOST_TEST_MESSAGE("Check that receiving a PONG with the correct nonce cancels our PING");
nonce = SendPing();
time_elapsed = 5s;
@@ -187,11 +195,12 @@ BOOST_AUTO_TEST_CASE(ping)
}
BOOST_AUTO_TEST_CASE(redundant_verack)
{
const auto pipes{m_sockets_pipes.PopFront()};
- ASSERT_DEBUG_LOG_WAIT("ignoring redundant verack message", 30s);
+ BOOST_REQUIRE(!g_stage_redundant_verack.try_wait());
pipes->recv.PushNetMsg(NetMsgType::VERACK);
+ g_stage_redundant_verack.wait();
}
BOOST_AUTO_TEST_SUITE_END()
</details>
I could be missing some easier alternative. Let me know if you have something better in mind.
better in mind?
Yeah, any reason you can't just call ProcessMessagesOnce. Alternatively, except for the ping/verack case you can just do what the python def sync_with_ping does?
If ProcessMessagesOnce() is called directly, then it will not be an end-to-end test. It will defeat the purpose of this PR.
Sending ping and waiting for pong (sync_with_ping) might be an alternative. It is not as flexible and fine-grained though as waiting for a particular log message to be logged and it involves a timeout too (which could expire too soon on slow platforms). What would be the advantage of a sync_with_ping approach? I have to think about this.
If ProcessMessagesOnce() is called directly, then it will not be an end-to-end test. It will defeat the purpose of this PR.
Makes sense. I guess I misunderstood the goal of this pull. So the goal is to mimic the functional tests that already do the same checks, but do it without spinning up a real socket, but use a mocked socket?
Yes. My idea is to complement the functional tests, not obsolete them.
In general, when the test is run from inside the same process then we have greater control of what happens - we can inspect variables and check state in more detail than a functional test can do from outside of bitcoind - it has to use the RPC. Also, from inside the process we can alter the state by modifying variables or calling certain functions that is not possible from outside of the process.
When one thread waits for another to execute a particular line of code, if that line of code is not executed for some time the waiting thread has to assume a timeout failure. I.e. some waiting is unavoidable, I am leaving the code as it is.
Do you consider this resolved?
Marking this as resolved. Comment if you think it is not.
63 | + const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE).c_str()}; 64 | + if (!LIMIT_TO_MESSAGE_TYPE.empty() && random_message_type != LIMIT_TO_MESSAGE_TYPE) { 65 | + return; 66 | + } 67 | + 68 | + SetMockTime(GetMockTime() + std::chrono::minutes{fuzzed_data_provider.ConsumeIntegral<uint8_t>()});
In 55c84c2d3bff09784ad127aba68a166f3f36f215: Is there any specific reason for this approach - Instead of using a fixed value like process_message does?
You mean SetMockTime(1610000000);? Connman is already started at this point and I thought that the clock going backwards might be a problem, but replacing this with SetMockTime(1610000000); seems to work so I guess either one is fine. Do you think I should change it?
Do you think I should change it?
In any case, there should be a comment to explain why it sets the mock time, no?
Added a comment.
55c84c2d3b...ca7a9983eb: add a comment in the source code, as per #26812 (review)
64 | + if (!LIMIT_TO_MESSAGE_TYPE.empty() && random_message_type != LIMIT_TO_MESSAGE_TYPE) { 65 | + return; 66 | + } 67 | + 68 | + // any time to successfully reset ibd; use a time in the future because Connman is already running 69 | + SetMockTime(GetMockTime() + std::chrono::minutes{fuzzed_data_provider.ConsumeIntegral<uint8_t>()});
ConsumeIntegral can return 0, so this won't be in the future. Can just drop ConsumeIntegral completely?
I stared at this a bit and realized it does not make sense without the ResetIbd() call and further, the point is to jump out of IBD at some point later during the test (by changing the mock time so that it is near or before the chain tip). Added that and some comments.
ca7a9983eb...ae75114975: enter IBD at the start and possibly jump out at some point, like in src/test/fuzz/process_messages.cpp.
ae75114975...d31cb74c44: rebase for CI
Needs rebase if still relevant
d31cb74c44...4bc062fb9c: rebase due to conflicts
4bc062fb9c...8f21993a06: rebase due to conflicts
Will re-review this soon.
test/net_msg_tests.cpp:70:48: error: ‘WithParams’ was not declared in this scope;
Coverage after 1000 CPU hours of fuzzing: https://dergoegge.github.io/bitcoin-coverage/pr26812/fuzz.coverage/index.html
Doesn't reach much in net_processing, so it's probably not doing much in terms of e2e testing.
8f21993a06...2386ef0a54: rebase due to conflicts and drop the newly added fuzz test.
@dergoegge, thanks for looking into this and testing it. The reason for the low coverage is, I guess, that the fuzzer has a hard time generating a valid or "interesting" data to push to the socket. Same as the reason for #28692. According to @maflcko's advice #21387 (review) a dictionary or a custom mutator could resolve this. I am yet to study how to do that. So, I have removed the last commit (the fuzz test) from this PR.
2386ef0a54...0858b0c084: remove duplicate #include <sync.h>
0858b0c084...8b10990aa0: rebase due to conflicts
227 | + /** 228 | + * Deserialize a `CNetMessage` and remove it from the pipe. 229 | + * If not enough bytes are available then the function will wait. If parsing fails 230 | + * or EOF is signaled to the pipe, then `std::nullopt` is returned. 231 | + */ 232 | + std::optional<CNetMessage> GetNetMsg() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
f42e4f3b3b4b3ca1945d1ea298b443f1cecaf2ea: this only works with v1 transport. In both v2 transport and the (proposed) Stratum v2 transport (#29432) bytes on the wire need further processing to reconstruct the underlying message.
For those it's more useful to have a GetBytes(size_t n) helper method that waits until n bytes have been received.
(Specifically for the Stratum v2 I'm also trying to completely avoid a dependency on CNetMessage, but the design for that is still in flux)
Like this?
<details> <summary>Add GetBytesWait()</summary>
commit 2e8674478813e2e672e5eb7dbb87f28ee11a7268
Parent: ad806a0969226806bb33a890356b89e39904c1a8
Author: Vasil Dimov <vd@FreeBSD.org>
AuthorDate: Tue Feb 27 14:59:04 2024 +0100
Commit: Vasil Dimov <vd@FreeBSD.org>
CommitDate: Tue Feb 27 15:48:18 2024 +0100
gpg: Signature made Tue Feb 27 15:48:19 2024 CET
gpg: using RSA key E64D8D45614DB07545D9CCC154DF06F64B55CBBF
gpg: Good signature from "Vasil Dimov <vd@myforest.net>" [ultimate]
gpg: aka "Vasil Dimov <vd@FreeBSD.org>" [ultimate]
gpg: aka "Vasil Dimov <vasild@gmail.com>" [ultimate]
test: add blocking "get bytes" method to DynSock
diff --git a/src/test/util/net.h b/src/test/util/net.h
index 368c84d622..d03674cb23 100644
--- a/src/test/util/net.h
+++ b/src/test/util/net.h
@@ -224,12 +224,20 @@ public:
* [@param](/bitcoin-bitcoin/contributor/param/)[in] flags Same as the flags of `recv(2)`. Just `MSG_PEEK` is honored.
* [@return](/bitcoin-bitcoin/contributor/return/) The number of bytes written to `buf`. `0` if `Eof()` has been called.
* If no bytes are available then `-1` is returned and `errno` is set to `EAGAIN`.
*/
ssize_t GetBytes(void* buf, size_t len, int flags = 0) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+ /**
+ * Wait for `len` bytes and remove them from the pipe.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] buf Destination to write bytes to.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] len Write this number of bytes to `buf` or less if EOF is signaled.
+ * [@return](/bitcoin-bitcoin/contributor/return/) The number of bytes written to `buf`. Less than `len` means EOF was signaled.
+ */
+ size_t GetBytesWait(void* buf, size_t len) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+
/**
* Deserialize a `CNetMessage` and remove it from the pipe.
* If not enough bytes are available then the function will wait. If parsing fails
* or EOF is signaled to the pipe, then `std::nullopt` is returned.
*/
std::optional<CNetMessage> GetNetMsg() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
@@ -243,13 +251,13 @@ public:
* Construct and push CNetMessage to the pipe.
*/
template <typename... Args>
void PushNetMsg(const std::string& type, Args&&... payload) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
/**
- * Signal end-of-file on the receiving end (`GetBytes()` or `GetNetMsg()`).
+ * Signal end-of-file on the receiving end (`GetBytes()`, `GetBytesWait()` or `GetNetMsg()`).
*/
void Eof() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
private:
/**
* Return when there is some data to read or EOF has been signaled.
diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
index 0fee2041f0..2bd4815e89 100644
--- a/src/test/util/net.cpp
+++ b/src/test/util/net.cpp
@@ -256,12 +256,31 @@ ssize_t DynSock::Pipe::GetBytes(void* buf, size_t len, int flags)
m_data.erase(m_data.begin(), m_data.begin() + read_bytes);
}
return read_bytes;
}
+size_t DynSock::Pipe::GetBytesWait(void* buf, size_t len)
+{
+ WAIT_LOCK(m_mutex, lock);
+
+ size_t total_read_bytes{0};
+
+ do {
+ WaitForDataOrEof(lock);
+ if (!m_data.empty()) {
+ const size_t read_bytes{std::min(len - total_read_bytes, m_data.size())};
+ std::memcpy(reinterpret_cast<uint8_t*>(buf) + total_read_bytes, m_data.data(), read_bytes);
+ m_data.erase(m_data.begin(), m_data.begin() + read_bytes);
+ total_read_bytes += read_bytes;
+ }
+ } while (total_read_bytes < len && !m_eof);
+
+ return total_read_bytes;
+}
+
std::optional<CNetMessage> DynSock::Pipe::GetNetMsg()
{
V1Transport transport{NodeId{0}};
{
WAIT_LOCK(m_mutex, lock);
@@ -309,12 +328,22 @@ void DynSock::Pipe::Eof()
{
LOCK(m_mutex);
m_eof = true;
m_cond.notify_all();
}
+void DynSock::Pipe::WaitForDataOrEof(UniqueLock<Mutex>& lock)
+{
+ Assert(lock.mutex() == &m_mutex);
+
+ m_cond.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(m_mutex) {
+ AssertLockHeld(m_mutex);
+ return !m_data.empty() || m_eof;
+ });
+}
+
DynSock::DynSock(std::shared_ptr<Pipes> pipes) : m_pipes{pipes} {}
DynSock::~DynSock()
{
m_pipes->send.Eof();
}
I did not add that method in this PR because it would be unused. Can be added any time later as part of a PR that needs it.
Something like that yes. Maybe it can be included and used a helper function by GetNetMsg?
I do not think so (or maybe I just don't see it). Inside GetNetMsg() we don't know how many bytes are needed. We throw something at transport.ReceivedBytes() and it tells us how much of that it consumed.
size_t len could be an optional argument. Not sure if that otherwise creates a mess though.
You mean to combine the two methods GetBytes() and GetBytesWait()? I considered this but ditched the idea. The bodies of the methods are too different and the len argument cannot be omitted as it tells the size of the buffer.
Reviewed the fist commit bee6bdf0a5084b2d5749ff06ad63c0e77816c733. Added a question for the second.
8b10990aa0...ad806a0969: rebase and move the local lambda WaitForDataOrEof() as a method of the Pipe class so it can be reused in the future, see #26812 (review)
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/22041198321</sub>
c54a458cb9...9a158da46c: rebase due to conflicts
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/23192935231</sub>
Turned into draft for now, due to failing CI. If this is no longer a WIP, you can move it out of draft.
47 | @@ -48,6 +48,7 @@ 48 | #include <test/util/net.h> 49 | #include <test/util/random.h> 50 | #include <test/util/txmempool.h> 51 | +#include <timedata.h>
Didn't look deeply, but with the following diff this branch builds again after rebase to current master ff8c606cf1eaefd0eab9f144561120a and the unit tests pass:
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -45,7 +45,6 @@
#include <test/util/txmempool.h>
-#include <timedata.h>
#include <txdb.h>
@@ -648,10 +647,6 @@ NetTestingSetup::~NetTestingSetup()
fNameLookup = DEFAULT_NAME_LOOKUP;
fListen = true;
CreateSock = CreateSockOS;
- // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state
- // in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset
- // that state as it was before our test was run.
- TestOnlyResetTimeData();
}
Yes, indeed! Fixed, thanks!
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is 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.
Leave a comment here, if you need help tracking down a confusing failure.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/25321293793</sub>
ecba2fbd20...e0bb306436: rebase due to conflicts
CI failure seems unrelated.
f5fe172e85...127c86d91f: pet clang-tidy: use = default instead of {}
6d0c5247eb...752fbab26a: rebase due to conflicts
Light ACK 752fbab26aa67b01249ef5948d5c78e0e5a5244e per git range-diff 489e5aa 612ba17 752fbab since my previous ACK at #26812 (comment) and a quick re-read of the full changes rebased to current master. I like that the changes here are essentially limited to tests only.
The pull descriptions claims to add a fuzz test, but I can't see anything related to fuzzing
The pull descriptions claims to add a fuzz test, but I can't see anything related to fuzzing
I removed the fuzz test due to #26812 (review) and forgot to update the PR description. Updated now, thanks!
I still wonder how useful this is overall. I understand that it is adding a bunch of test-framework code to mimic the functional tests at a lower level and offers a way to write tests this way in C++. However, developers may still prefer to write the tests in Python going forward, because they are used to it, and because it is more flexible, albeit a bit more expensive, because full processes need to be spun up each time. If this is merged, it will add a few redundant test cases (redundant in the sense of not increasing line coverage over the existing one), but may then end up otherwise unused/stale.
Conceptually I like the changes, but it reminds me of my attempt to "port" the functional test to C++, starting by providing functions for each RPC (https://github.com/bitcoin/bitcoin/commit/fa8685d49ed8e52b1220a89f669dac63bef172bc#diff-663b6e7cfa474d453c20efa49be6e3e9f5066f7fed5586e98da98f2b3dea5653R24), but that never took off either.
Often when it comes to testing P2P behavior, you'd want to spin up several peers/nodes anyway, and the functional tests have the full framework for that, so going forward developers will likely still use that for testing.
developers may still prefer to write the tests in Python going forward, because they are used to it, and because it is more flexible
I agree. This PR does not intend to replace the functional testing framework, but complement it. For example, from C++ one has greater control and observability - the test can read any global or member variable and can call arbitrary functions (repeating #26812 (review)).
Also, when used from within C++ one does not have to re-implement the transport code in Python. From C++ we can use the existent code to parse or craft the socket data.
Wrt "how useful is this": the first part of this PR (also isolated in #30205) is already used in Sv2Transport tests.
from C++ one has greater control and observability - the test can read any global or member variable and can call arbitrary functions
Also, when used from within C++ one does not have to re-implement the transport code in Python. From C++ we can use the existent code to parse or craft the socket data.
For these reasons, along with the speed of running the tests and the simplicity and robustness of using the same language for tests and code, I prefer to write test code in C++ when I can, and run the C++ tests much more often locally than the functional tests. I recall there were initiatives to move functional test code to C++ as @maflcko suggests, and would be supportive of these.
Wrt "how useful is this": the first part of this PR (also isolated in #30205) is already used in Sv2Transport tests.
I followed the links but only found a closed pull requests: #30332#event-13580183288
I think it is easy to say that something will be used in the future, but I think it would be easier if the framework commits were added with non-redundant tests at the same time.
53d2c8e0e2...c1c6a056a0: rebase due to conflicts
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task tidy: https://github.com/bitcoin/bitcoin/runs/39128326309</sub>
<sub>LLM reason (✨ experimental): clang-tidy reported explicit errors due to deprecated headers in net_msg_tests.cpp, causing the CI to fail.</sub>
<details><summary>Hints</summary>
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.
</details>
c1c6a056a0...bfe72353c0: rebase and silence a new tidy error about headers - assert.h vs cassert
bfe72353c0...5d7b70161d: rebase due to conflicts
Can this be closed? I think it would be good if this added non-redundant test coverage of any sort, but hasn't since this pull was opened more than 3 years ago? So there does not seem to exist a relevant use-case?
I think it would be good if this added non-redundant test coverage of any sort
It does - see https://corecheck.dev/bitcoin/bitcoin/pulls/26812 under "Lines of code that were not covered by tests in master but are covered in this pull request".
Extend `DebugLogHelper::~DebugLogHelper()` to be able to
optionally wait for messages, possibly logged from another thread, to
arrive before performing the final check.
Add tests that use a mocked socket to similate network IO and cover the
full `CConnman`, `PeerManager` and the interaction between them.
e4e13ce4c0191901b59752f4f000bed7c025a169...6b218f67f726c6775460854a47ff9e5628d05620: rebase due to conflicts
The idea of this is still relevant - it makes it possible to have fine grained control and mock over what gets written and read to/from the sockets. @Sjors, @jonatack, @maflcko, @dergoegge, @brunoerg have commented here before. Worth your review? Or close due to lack of (review) interest?
I think it would be good if this added non-redundant test coverage of any sort
It does - see https://corecheck.dev/bitcoin/bitcoin/pulls/26812 under "Lines of code that were not covered by tests in master but are covered in this pull request".
Sure, but I think it is a lot easier to have similar tests sit in the same place, and follow the same framework. Otherwise, it is unclear which tests cover what part, and having two frameworks that re-implement a (busy?) debug log parsing to check internal state just doesn't seem debug friendly and maintainable.
The two lines of C++ code:
ASSERT_DEBUG_LOG_WAIT("ignoring redundant verack message", 30s);
pipes->recv.PushNetMsg(NetMsgType::VERACK);
can be written in two lines of Python code, in the appropriate pre-existing Functional test.
The functional tests also have the additional benefit to show that this is not just dead code called by an internal unit test, because they are real end-to-end tests.
So apart from the overhead of having to re-implement the framework helpers, the unit test can also incorrectly mark some dead code as covered, when in reality it is unreachable externally.
So to clarify my question: With "non-redundant coverage" I mean conceptually non-redundant coverage that fits into the great picture.
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
Could turn into draft while there is outstanding review feedback, and this needs rebase anyway?
This has been open for more than 3 years.
My opinion is that it is relevant and good to have. However there is lack of review interest plus a push back from @maflcko.
Thus closing. Could revive work on it if there is interest.