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

1. 
   vasild commented at 3:49 pm on May 30, 2024: contributor

   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 #26812 (first two commits from that PR). Extracting as a separate PR suggested here: #30043 (review).

2. 
   DrahtBot commented at 3:49 pm on May 30, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30205.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	jonatack
   Stale ACK	Sjors

   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.

3. DrahtBot added the label Tests on May 30, 2024
4. 
   Sjors commented at 4:31 pm on May 30, 2024: member

   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.
5. in src/test/util/net.h:240 in 06b21ab6cb outdated

   230+        /**
   231+         * Deserialize a `CNetMessage` and remove it from the pipe.
   232+         * If not enough bytes are available then the function will wait. If parsing fails
   233+         * or EOF is signaled to the pipe, then `std::nullopt` is returned.
   234+         */
   235+        std::optional<CNetMessage> GetNetMsg() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
   

   ---

   ---

   

   Sjors commented at 1:24 pm on July 1, 2024:

   06b21ab6cb868f6b1e41e36a1f3a4d4c9c51558b: is this still useful now that you have GetBytes? It seems it’s the job for Transport to turn bytes into a CNetMessage.

   Same question for PushNetMsg below.

   See earlier discussion: #26812 (review)

6. 
   Sjors commented at 1:37 pm on July 1, 2024: member

   I’m still a bit confused on how to use this, e.g. in #30332.

   The first step is to override CreateSocket() in sv2_template_provider_tests.cpp, so that when the TemplateProvider calls it we get a mock socket.

   But how? Something like:

   0CreateSock = [](int, int, int) {
   1    return std::make_unique<DynSock>(...);
   2};
   

   The test contains a TPTester helper which has std::unique_ptr<DynSock> m_peer_socket; to represent the other side of the connection (a Sv2Transport instance that does a handshake and then sends a hardcoded protocol message SetupConnectionMsg).

   This is inialitzed with:

   0m_peer_socket = ConnectDirectly(*tp, /*manual_connection=*/true);
   

   Instead of making a real TCP/IP connection this should be making a mock connection.

7. 
   vasild commented at 1:37 pm on July 3, 2024: contributor

   @Sjors, I changed the test sv2_connman_tests/client_tests from #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.
8. 
   Sjors commented at 7:27 am on July 4, 2024: member

   @vasild thanks! At first glance these changes make sense, not sure why it’s broken. I left some comments on your commit f0dc62f8ab773ef7cbf44ba7119d08af66506428.
9. 
   Sjors commented at 11:37 am on July 4, 2024: member

   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?

10. 
    vasild commented at 12:52 pm on July 16, 2024: contributor

    @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 #30332
    • Plus a minor fixup from #30332 (review)
    • 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.
11. 
    Sjors commented at 7:22 am on July 17, 2024: member

    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).

12. vasild force-pushed on Aug 2, 2024
13. 
    vasild commented at 9:34 am on August 2, 2024: contributor

    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.

14. vasild force-pushed on Sep 3, 2024
15. 
    vasild commented at 10:36 am on September 3, 2024: contributor

    3769f89a78...e59097a0a5: rebase to pick CMake
16. in src/test/util/net.h:240 in 187ba684a9 outdated

    277+
    278     const std::string m_contents;
    279     mutable size_t m_consumed{0};
    280 };
    281 
    282-std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candidates, FastRandomContext& random_context);
    

    ---

    ---

    

    Sjors commented at 1:01 pm on September 5, 2024:

    187ba684a9d5e63d09b43511b7128cce9edbedf3 any particular reason for moving this?

    ---

    

    vasild commented at 8:25 am on September 6, 2024:

    Just stylistic - I though it would be better to have it before the classes declarations. Should I move it at the bottom, where it was?

    ---

    

    Sjors commented at 8:47 am on September 6, 2024:

    Not a big deal, but diff was already quite large.
17. in src/test/util/net.h:145 in 187ba684a9 outdated

    145-class StaticContentsSock : public Sock
    146+class ZeroSock : public Sock
    147 {
    148 public:
    149-    explicit StaticContentsSock(const std::string& contents)
    150-        : Sock{INVALID_SOCKET},
    

    ---

    ---

    

    Sjors commented at 1:13 pm on September 5, 2024:

    187ba684a9d5e63d09b43511b7128cce9edbedf3: shouldn’t Sock{INVALID_SOCKET} be preserved in the new ZeroSock constructor?

    ---

    

    vasild commented at 7:50 am on September 6, 2024:

    No, the new ZeroSock constructor’s body is now in src/test/util/net.cpp and it does

    0ZeroSock::ZeroSock() : Sock{g_mocked_sock_fd++} {}
    

    Because if all of the ZeroSock instances have m_socket == INVALID_SOCKET, then they would compare equal by EqualSharedPtrSock.

18. Sjors approved
19. 
    Sjors commented at 3:18 pm on September 5, 2024: member

    ACK e59097a0a56ecaffdb38aaa94e232f5b45881897

    You can see it in action in sv2_connman_tests in https://github.com/Sjors/bitcoin/pull/50.

    I didn’t test the CNetMessage functionality and Eof() method.

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

20. vasild force-pushed on Sep 6, 2024
21. 
    vasild commented at 8:23 am on September 6, 2024: contributor

    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:

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

22. 
    Sjors commented at 8:52 am on September 6, 2024: member

    @vasild it’s probably easier to reverse those two commits: first move the implementation out, and then split the class.
23. vasild force-pushed on Sep 6, 2024
24. 
    vasild commented at 9:24 am on September 6, 2024: contributor

    @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:

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

25. 
    Sjors commented at 1:58 pm on September 6, 2024: member

    re-ACK e7cf8e8fc658e4559fcc64279c7bd854773ad6c2

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

26. 
    jonatack commented at 1:39 pm on October 11, 2024: member

    Concept ACK
27. DrahtBot added the label CI failed on Oct 23, 2024
28. DrahtBot removed the label CI failed on Oct 25, 2024
29. test: move the implementation of StaticContentsSock to .cpp

    Move the implementation (method definitions) from `test/util/net.h` to
    `test/util/net.cpp` to make the header easier to follow.

    4b58d55878
30. test: put the generic parts from StaticContentsSock into a separate class

    This allows reusing them in other mocked implementations.

    2f6ce54212
31. test: add a mocked Sock that allows inspecting what has been Send() to it

    And also allows gradually providing the data to be returned by `Recv()`
    and sending and receiving net messages (`CNetMessage`).

    5766bbefa9
32. vasild force-pushed on Nov 18, 2024
33. 
    vasild commented at 12:31 pm on November 18, 2024: contributor

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

Contributors
vasild DrahtBot Sjors jonatack

Labels
Tests