test: add mocked Sock that can read/write custom data and/or CNetMessages

[vasild]

trafficstars

Put the generic parts from StaticContentsSock into a separate class ZeroSock so that they can be reused in other mocked Sock implementations.

Add a new DynSock whose Recv() and Send() methods can be controlled after the object is created. To achieve that, the caller/creator of DynSock provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by DynSock::Recv() method and whatever data is written to the socket using DynSock::Send() can later be found in the send-pipe. For convenience there are also two methods to send and receive CNetMessages.

---

This is used in https://github.com/bitcoin/bitcoin/pull/26812 (first two commits from that PR). Extracting as a separate PR suggested here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037.

[DrahtBot]

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors, jonatack, pinheadmz

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

[Sjors]

This seems very useful. I'll try to use it for the (currently very brittle) Sv2Transport tests in #29432, and review it along the way.

[vasild]

@Sjors, I changed the test sv2_connman_tests/client_tests from https://github.com/bitcoin/bitcoin/pull/30332 to use mocked sockets instead of real ones from the operating system. See the top commit from here: https://github.com/vasild/bitcoin/commits/sv2_mocksock/, that compiles, but the test fails at some point. I will try to get it to pass, just sharing this early wip with you.

[Sjors]

@vasild thanks! At first glance these changes make sense, not sure why it's broken. I left some comments on your commit f0dc62f8ab773ef7cbf44ba7119d08af66506428.

[Sjors]

So the Sv2Connman is called, but start() isn't, so the mock BindListenPort doesn't happen.

---

The last commit here gets all the way through the handshake, but then it hangs up with Num bytes received from client id=1: -1. https://github.com/Sjors/bitcoin/commits/2024/07/mockscok/

---

The disconnect seems to happen because has_received_data = socket_it->second.occurred & Sock::RECV; is true when sending a message.

---

I think we need to override ZeroSock::WaitMany to set occured based on what's in m_data (DynSock::WaitMany is not called).

---

Not sure how to fix this. My guess is ListenSock needs its own WaitMany which picks the right entry from m_pipes and then calls DynSock::WaitMany?

[vasild]

@Sjors, thanks for your suggestions at https://github.com/vasild/bitcoin/commit/f0dc62f8ab773ef7cbf44ba7119d08af66506428. I extended DynSock to be able to Accept() new connections, returning a socket that has been provided by the tests.

Now the test sv2_connman_tests/client_tests uses mocked sockets and passes!

See https://github.com/vasild/bitcoin/commits/sv2_mocksock/ which is:

• This PR, modified to handle Accept(). If you like that I will push the mods to this PR.
• Plus https://github.com/bitcoin/bitcoin/pull/30332
• Plus a minor fixup from https://github.com/bitcoin/bitcoin/pull/30332#discussion_r1679318486
• Plus modifications to sv2_connman_tests/client_tests to use mocked sockets and some simplifications because of no real sockets are used now, e.g. the ProcessOurResponse() method is not needed anymore.

[Sjors]

Works like a charm! I'll see if I can add them to the tests in #30332 as well.

---

It worked for the Template Provider as well and was easy to integrate. Thanks again! I'll review this PR soon(tm).

[vasild]

06b21ab6cb...3769f89a78: Extend DynSock to be able to Accept() new connections, returning a socket that has been provided by the tests.

If you like that I will push the mods to this PR.

Done.

[vasild]

3769f89a78...e59097a0a5: rebase to pick CMake

[vasild]

Maybe split 187ba68 into one commit that moves the implementation and another that splits the class.

e59097a0a5...e995ffa5c3: do that ^

The cumulative diff before and after this push is identical:

$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e995ffa5c~3..e995ffa5c)
$

[Sjors]

@vasild it's probably easier to reverse those two commits: first move the implementation out, and then split the class.

[vasild]

@vasild it's probably easier to reverse those two commits: first move the implementation out, and then split the class.

e995ffa5c3...e7cf8e8fc6: done, same as before, the cumulative diff is identical before and after:

$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e995ffa5c~3..e995ffa5c)
$ diff -u <(git diff e59097a0a5~2..e59097a0a5) <(git diff e7cf8e8fc~3..e7cf8e8fc)
$ 

[Sjors]

re-ACK e7cf8e8fc658e4559fcc64279c7bd854773ad6c2

This makes my dimmed zebra very happy for the first commit, and the second one is now much easier to read.

[jonatack]

Concept ACK

[vasild]

e7cf8e8fc6...5766bbefa9: rebase and make sure DynSock::Accept()'s output argument is initialized properly: https://github.com/Sjors/bitcoin/pull/50#issuecomment-2478468037

[pinheadmz]

Concept ACK

Using this in my http rewrite branch and will leave a few review comments next week

[vasild]

5766bbefa9...b448b01494: avoid moving GetRandomNodeEvictionCandidates() as suggested in https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858648731 and https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1745496410

[Sjors]

re-ACK b448b014947093cd217dbde47c8fb9e6c2bc8ba3

[ryanofsky]

Merged this, and one thing I was curious about looking at the code was if it makes sense to replace void* buf, size_t len which is used many places with std::span<std::byte> here? That seems like could make it safer or easier to use

[vasild]

That would be DynSock::Pipe::GetBytes() and DynSock::Pipe::PushBytes(). Sock::Send() and Sock::Recv() also use raw pointer + length, but they were not introduced in this PR and, more importantly, are supposed to mimic 1:1 the send(2) and recv(2) syscalls, in return value, arguments and semantic.

For GetBytes() and PushBytes() the change would be:

diff --git i/src/test/util/net.h w/src/test/util/net.h
index 3e717341d8..a28c442035 100644
--- i/src/test/util/net.h
+++ w/src/test/util/net.h
@@ -17,18 +17,20 @@
 #include <sync.h>
 #include <util/sock.h>
 
 #include <algorithm>
 #include <array>
 #include <cassert>
+#include <cstddef>
 #include <chrono>
 #include <condition_variable>
 #include <cstdint>
 #include <cstring>
 #include <memory>
 #include <optional>
+#include <span>
 #include <string>
 #include <unordered_map>
 #include <vector>
 
 class FastRandomContext;
 
@@ -218,30 +220,29 @@ public:
     class Pipe
     {
     public:
         /**
          * Get bytes and remove them from the pipe.
          * @param[in] buf Destination to write bytes to.
-         * @param[in] len Write up to this number of bytes.
          * @param[in] flags Same as the flags of `recv(2)`. Just `MSG_PEEK` is honored.
          * @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);
+        ssize_t GetBytes(std::span<std::byte> buf, 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);
+        void PushBytes(std::span<const std::byte> buf) 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);
diff --git i/src/test/util/net.cpp w/src/test/util/net.cpp
index ddd96a5064..9f30f6413d 100644
--- i/src/test/util/net.cpp
+++ w/src/test/util/net.cpp
@@ -241,27 +241,27 @@ ssize_t StaticContentsSock::Recv(void* buf, size_t len, int flags) const
 StaticContentsSock& StaticContentsSock::operator=(Sock&& other)
 {
     assert(false && "Move of Sock into StaticContentsSock not allowed.");
     return *this;
 }
 
-ssize_t DynSock::Pipe::GetBytes(void* buf, size_t len, int flags)
+ssize_t DynSock::Pipe::GetBytes(std::span<std::byte> buf, int flags)
 {
     WAIT_LOCK(m_mutex, lock);
 
     if (m_data.empty()) {
         if (m_eof) {
             return 0;
         }
         errno = EAGAIN; // Same as recv(2) on a non-blocking socket.
         return -1;
     }
 
-    const size_t read_bytes{std::min(len, m_data.size())};
+    const size_t read_bytes{std::min(buf.size(), m_data.size())};
 
-    std::memcpy(buf, m_data.data(), read_bytes);
+    std::memcpy(buf.data(), m_data.data(), read_bytes);
     if ((flags & MSG_PEEK) == 0) {
         m_data.erase(m_data.begin(), m_data.begin() + read_bytes);
     }
 
     return read_bytes;
 }
@@ -301,17 +301,17 @@ std::optional<CNetMessage> DynSock::Pipe::GetNetMsg()
     if (reject) {
         return std::nullopt;
     }
     return std::make_optional<CNetMessage>(std::move(msg));
 }
 
-void DynSock::Pipe::PushBytes(const void* buf, size_t len)
+void DynSock::Pipe::PushBytes(std::span<const std::byte> buf)
 {
     LOCK(m_mutex);
-    const uint8_t* b = static_cast<const uint8_t*>(buf);
-    m_data.insert(m_data.end(), b, b + len);
+    const uint8_t* b = reinterpret_cast<const uint8_t*>(buf.data());
+    m_data.insert(m_data.end(), b, b + buf.size());
     m_cond.notify_all();
 }
 
 void DynSock::Pipe::Eof()
 {
     LOCK(m_mutex);
@@ -338,18 +338,18 @@ DynSock::~DynSock()
 {
     m_pipes->send.Eof();
 }
 
 ssize_t DynSock::Recv(void* buf, size_t len, int flags) const
 {
-    return m_pipes->recv.GetBytes(buf, len, flags);
+    return m_pipes->recv.GetBytes({reinterpret_cast<std::byte*>(buf), len}, flags);
 }
 
 ssize_t DynSock::Send(const void* buf, size_t len, int) const
 {
-    m_pipes->send.PushBytes(buf, len);
+    m_pipes->send.PushBytes({reinterpret_cast<const std::byte*>(buf), len});
     return len;
 }
 
 std::unique_ptr<Sock> DynSock::Accept(sockaddr* addr, socklen_t* addr_len) const
 {
     ZeroSock::Accept(addr, addr_len);
@@ -382,14 +382,14 @@ bool DynSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_
                 events.occurred |= Sock::SEND;
                 at_least_one_event_occurred = true;
             }
 
             if ((events.requested & Sock::RECV) != 0) {
                 auto dyn_sock = reinterpret_cast<const DynSock*>(sock.get());
-                uint8_t b;
-                if (dyn_sock->m_pipes->recv.GetBytes(&b, 1, MSG_PEEK) == 1 || !dyn_sock->m_accept_sockets->Empty()) {
+                std::byte b[1];
+                if (dyn_sock->m_pipes->recv.GetBytes(b, MSG_PEEK) == 1 || !dyn_sock->m_accept_sockets->Empty()) {
                     events.occurred |= Sock::RECV;
                     at_least_one_event_occurred = true;
                 }
             }
         }

What do you think? PR? Leave it alone?

[ryanofsky]

re: What do you think? PR? Leave it alone?

Whichever you prefer, I'd happily review it. I wanted to make suggestion more as an idea to keep in mind when updating this code or writing new code, because of benefits described in https://github.com/bitcoin/bitcoin/issues/31272

Also ideally the change would go further IMO: replace std::vector<uint8_t> with std::vector<std::byte>, replace memcpy with std::vector::assign, replace Recv/Send arguments with span instead of void*. I don't think it is good to mimic mimic void pointers in recv/send calls, better to remove all direct recv/send calls and replace them them with simple Recv/Send wrappers that take spans.

[vasild]

This is low prio (for me) and I haven't had the time to PR it for a few months, so I am leaving it as it it. If anybody does PR the diff above or similar, then I would review.

[Sjors]

Feel free to tag me in such a PR.