Virtualise CConnman and add initial PeerManager unit tests (p2p, tests)

https://github.com/bitcoin/bitcoin/pull/25515

Host: larryruane  -  PR author: dergoegge

The PR branch HEAD was f98a4e8d89 at the time of this review club meeting.

Notes

  • PR 25515 is a refactoring change that allows unit-testing of the PeerManager class, by making CConnman mockable. CConnman is made mockable by defining an abstract interface class, ConnectionsInterface, that CConnman then inherits from.

  • Currently, PeerManager holds a reference to CConnman (PeerManagerImpl::m_connman) and the PR changes the type of that reference to ConnectionsInterface. Additionally, when a PeerManager instance is created, we now pass a ConnectionsInterface instead of a CConnman. With that, any unit tests can create a mocked version of the connection manager to test the internals of PeerManager.

  • To demonstrate the added benefit, the PR also adds two initial PeerManager unit tests for the version and ping/pong messages (see peerman_tests.cpp).

  • PeerManager is currently almost exclusively tested through our functional tests (See the test/functional/p2p_*.py tests). These tests spin up entire nodes and test functionality through the interfaces that nodes provide (p2p, rpc, rest, etc.).

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. Why do we need unit tests? Aren’t functional tests sufficient?

  3. Why do we need to mock CConnman to unit test PeerManager?

  4. Do mock interfaces need to implement all of the real interfaces?

  5. Should we mock other components as well? (e.g.: CTxMempool, ChainstateManager)

  6. Roughly, what is the overhead of using an interface class?

  7. How can you tell that a class is an interface class?

  8. What do the = 0 indicate at the definition of each ConnectionsInterface method? What would happen if they weren’t there?

  9. CConnMan and ConnectionsInterfaceMock both are classes derived from ConnectionsInterface. Why are their methods specified with the override keyword? (For example, PushMessage)

  10. What is the general approach of the version_handshake test? Why does it contain the series of calls to BOOST_CHECK_EQUAL()?

  11. What is the general approach of the ping test? Why does it mock the current time?

Meeting Log

  117:03 <glozow> #startmeeting
  217:03 <LarryRuane> hi everyone! yes sorry, let's get started!
  317:03 <hernanmarino> Hello
  417:03 <LarryRuane> today we're reviewing PR 25515 https://bitcoincore.reviews/25515
  517:04 <amovfx> pablomartin: yea, I just realized that after I typed it, brain is still gearing up
  617:04 <amovfx> :)
  717:04 <LarryRuane> anyone new joining us today? feel free to say hi
  817:04 <LarryRuane> don't hesitate to ask questions! Even if we've already moved on to the next topic, feel free to ask questions about something we've already covered
  917:05 <cstafford1717> Me I am new
 1017:05 <andrewtoth> hi
 1117:05 <LarryRuane> welcome @cstafford1717 !
 1217:05 <brunoerg> cstafford1717: welcome! :)
 1317:05 <Bohruz> I am new too
 1417:05 <Bohruz> My first time here
 1517:05 <LarryRuane> Let's see.. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?
 1617:06 <brunoerg> Bohruz: welcome!
 1717:06 <Bohruz> Thanks
 1817:06 <brunoerg> Concept ACK
 1917:06 <amovfx> welcome!
 2017:06 <srihari> approac ACK
 2117:06 <raj-> tested ACK..
 2217:06 <hernanmarino> Yes, tested ACK here
 2317:06 <pablomartin> tested ACK, didnt add the comments into the pr yet
 2417:06 <LarryRuane> (for anyone new, the general format is to go through the questions and discuss.. feel free to ask your own questions)
 2517:07 <LarryRuane> Good, several people have at least taken a look... Can anyone summarize the PR?
 2617:08 <raj-> Make a new interface so we can mock networking stuffs in unit tests..
 2717:09 <LarryRuane> raj-: Yes, and for anyone to answer, what is mocking in general, as a concept? Why is it useful?
 2817:09 <LarryRuane> Bohruz: welcome!
 2917:10 <amovfx> isolate and focus on code being tested and not the behavior or state of external deps.
 3017:10 <brunoerg> mock is something used to simulate the behavior of other one
 3117:10 <kouloumos> especially useful for unit tests
 3217:10 <amovfx> yea faster more reliable tests
 3317:10 <amovfx> unit tests*
 3417:11 <raj-> My understanding is its useful for unit situations where we need to mimic some behaviour but we can't directly instantiate a concrete class.. Here we can't have real CConnMan as there is no real networking in unit tests..
 3517:11 <brunoerg> raj: +1
 3617:11 <pablomartin> agree with brunoerg
 3717:11 <pablomartin> and raj yeah
 3817:12 <LarryRuane> amovfx: Yes excellent ... Here's how I think of it ... software, at least if it's well-written, is implemented in layers, and if you want to test a given layer, it will need the layers below it (call on its methods) ... but it may not be possible or practical to include all of that lower layer's functionality in the test setup
 3917:12 <LarryRuane> yes other great answers there (while I was typing all that!)
 4017:12 <LarryRuane> so what is the upper layer being test here, and what is the lower layer being mocked?
 4117:12 <LarryRuane> *tested
 4217:13 <raj-> Guess.. Tested: Peer Manager, Mocked: Connection manager..
 4317:14 <LarryRuane> raj-: yes exactly! what do these layers do, roughly?
 4417:15 <amovfx> Connection manager operates on peers?
 4517:15 <raj-> I found it hard to summarize their "management".. Wouldn't it be useful to have some docs before these classes?
 4617:15 <pablomartin> is used to manage connections and maintain statistics on each node connected
 4717:15 <pablomartin> *conman
 4817:16 <brunoerg> Peer Manager is used to manage connections to the peer and the application state of it
 4917:16 <pablomartin> peerman: manages peer state and interaction e.g. processing messages, fetching blocks & removing for misbehaviour
 5017:16 <LarryRuane> raj-: yes, good idea.. I think of it as, peer manager decides to send certain messages, and processes received ones ... connection manager is the low-level moving-bytes-over-the-wire kind of stuff
 5117:17 <LarryRuane> More basic question, Why do we need unit tests? Aren’t functional tests sufficient?
 5217:17 <LarryRuane> (that's question 2)
 5317:17 <brunoerg> basically Peer Manager depends on connection manager... we need a connection (more low level stuff) to a peer to get info from it
 5417:18 <raj-> LarryRuane : Ah that makes sense.. So the ConnMan is like per peer management, and PeerMan is like the global message handler??
 5517:18 <LarryRuane> well I think PeerManager also knows about the individual peers.. I think both layers do
 5617:19 <LarryRuane> it's just the PeerManager is high-level things (like sequencing the multiple messages in a version handshake), and connection manager is dealing with low level stuff (like the addresses of peers)
 5717:20 <brunoerg> I usually say we need unit tests to test "functions" and functional tests to test "features"
 5817:21 <brunoerg> it's not possible to use functional tests to test specific behavior of some functions/methods
 5917:21 <glozow> functional tests have a ton of overhead that makes them ill-equipped to target specific pieces of internal logic, and really really slow. we can't test eviction logic in isolation; we have to spin up a bunch of nodes, wait for evictions to be triggered, mine real blocks, to feed to the node, etc. etc.
 6017:21 <LarryRuane> Both of these layers, and many more, are exercised in the functional (python) tests, so what are the advantages of (also) unit-testing?
 6117:21 <amovfx> yea I thought functional tests would operate on multiple parts of the system, esentially simulate the program running
 6217:21 <amovfx> unit testing is faster and more specific?
 6317:22 <stratospher[m]> functional tests are more time consuming and resource intensive to set up. unit tests would be easier to debug since behaviour is compartmentalised.
 6417:22 <LarryRuane> brunoerg: yes I like that, glozow: good points, also functional tests can't test as deeply because they're limited to the RPC and P2P interfaces
 6517:22 <LarryRuane> stratospher[m]: +1
 6617:23 <brunoerg> stratospher[m]: +1
 6717:23 <LarryRuane> I think we already covered question 3: Why do we need to mock CConnman to unit test PeerManager?
 6817:23 <amovfx> there are no connections?
 6917:23 <LarryRuane> (it's because PeerManager uses a connection manager)
 7017:24 <amovfx> or there are no peers
 7117:24 <LarryRuane> amovfx: in what context? the unit tests?
 7217:24 <amovfx> yes
 7317:24 <amovfx> I read a Node can be mocked though
 7417:24 <amovfx> err CNode
 7517:25 <ws11__> Because unit tests should not use network
 7617:25 <LarryRuane> in a unit test there are no real connections, but the mock connection manager simulates one
 7717:26 <LarryRuane> ws11__: yes, there isn't a real network in the unit test framework
 7817:26 <LarryRuane> Question 4, Do mock interfaces need to implement all of the real interfaces?
 7917:27 <amovfx> I think only functions that have = 0?
 8017:27 <brunoerg> we can implement only what we are going to use in the test i guess
 8117:27 <amovfx> Its that overloading feature that demands a function be implimented
 8217:27 <pablomartin> amovfx which are virtual
 8317:27 <amovfx> yes
 8417:27 <amovfx> forgot the term because I am noob
 8517:27 <raj-> Only the ones we need to mock.. Other ones can return some default values..
 8617:27 <lightlike> only those functions that are called by the code that we want to test?
 8717:28 <raj-> But I think all needs to be override though in order to implement the interface..
 8817:28 <LarryRuane> raj-: lightlike: Exactly, that's what I was getting (question not worded well) .. All must be defined, but no, only the ones that the test causes the production code-under-test to invoke need to be (non-trivially) implemented
 8917:29 <LarryRuane> here you can see a bunch of functions that don't provide a non-trivial implementation: https://github.com/dergoegge/bitcoin/blob/f98a4e8d891dd7374ef7dc4c723797bf0705075f/src/test/peerman_tests.cpp#L50
 9017:29 <amovfx> I think I mixed up the definition of declare and define
 9117:30 <lightlike> LarryRuane: are you sure? E.g. the function AddNode() seems to be part of CConnman, but not of ConnectionsInterface
 9217:30 <LarryRuane> see how (at that link) `ForNode()` is just an empty function (`{}`) ... it must be defined but doesn't need to do anything because the test code doesn't cause the peer manager to call it!
 9317:32 <LarryRuane> lightlike: That's a good point, derived classes can provide *additional* methods beyond what the interface (base) class specifies
 9417:33 <LarryRuane> but then the other derived classes (like our mock, `ConnectionsInterfaceMock`) can't simulate it
 9517:34 <LarryRuane> But maybe `AddNode()` can be added to the interface class later, if that method needs to be simulated by a mock?
 9617:34 <kouloumos> Do we currently mock any other components? (I didn't find anything interesting with a quick search)
 9717:34 <raj-> Question: What guides the separation between interface functions and *additional* function of the derived type, while creating an interface like this?
 9817:35 <lightlike> yes, I think we'd get a compilation error if AddNode() was used in the Peerman code we are testing
 9917:37 <LarryRuane> lightlike: +1 raj-: I think what's in the interface are at least those functions that need to currently be mocked, but more can be added later (as more tests are written)
10017:38 <kouloumos> Does that mean that we could also omit (from the interface), those that are currently trivially implemented?
10117:38 <raj-> LarryRuane, yes that makes sense in my head.. So we can say that this interface is "dynamic" and can be modified later depending upon test requirements?
10217:38 <LarryRuane> kouloumos: I'm not sure.. there was a bunch of PRs listed here: https://github.com/bitcoin/bitcoin/issues/19398
10317:39 <brunoerg> Interesting
10417:39 <LarryRuane> a big motivation for which is to enable testing more using unit tests
10517:40 <LarryRuane> Hope that answers things, speak up if we missed anything ... question 5: Should we mock other components as well? (e.g.: `CTxMempool`, `ChainstateManager`)?
10617:40 <amovfx> so an overall design goal is to identify where interfaces are needed, implement them, then unit test them?
10717:41 <raj-> Sounds fun to me.. :D But probably increases code complexity with more abstractions??
10817:41 <LarryRuane> amovfx: Yes, more precisely, implement interfaces so that code that *uses those interfaces* can be unit tested
10917:41 <amovfx> I would say yes, for 1 consistency, 2 for more robust testing
11017:42 <brunoerg> raj: I think so, but it's worth
11117:43 <LarryRuane> raj-: I'd say ideally it should decrease code complexity by separating the layers more clearly, instead of everything being spaghettied (?) together
11217:43 <raj-> brunoerg, LarryRuane ya agreed on that..
11317:44 <brunoerg> It would make the code more readable and testable but maybe could increase codebase (not saying this is totally bad)
11417:44 <LarryRuane> So for question 5, I think we're on the same page that other classes should be mocked, but doing so would have to be driven by the tests that people are writing
11517:44 <LarryRuane> (in other words, exactly what the mock should do!)
11617:45 <LarryRuane> Question 6, Roughly, what is the overhead of using an interface class? (versus using the derived class directly)
11717:46 <hernanmarino> LarryRuane: +1
11817:46 <amovfx> I think it is pretty small, just adds a layer of indirection, so program size increases.
11917:47 <amovfx> Wait, is this interface only used in tests?
12017:47 <amovfx> No it isn't, it's used in the PeerManagerImpl
12117:48 <LarryRuane> amovfx: Yes, slightly, calling methods involves dereferencing a function pointer (indirect function call), which has a very low overhead
12217:49 <LarryRuane> amovfx: correct, the new interface class `ConnectionsInterface` must be used in production code (by the peer manager), but the interface class *allows* unit tests
12317:50 <lightlike> maybe the compiler can't inline functions anymore when they are virtual, which could be a slight decrease in performance?
12417:50 <raj-> Question: The `CConnMan` includes `ConnectionsInterfaceMock` as a `friend class`.. Why is that??
12517:51 <LarryRuane> lightlike: that's a good point! I agree, decision of which function to call must happen at runtime
12617:51 <raj-> Also seems to be the case for `CNode`..
12717:52 <pablomartin> raj it's to access to the private definition of the class
12817:54 <pablomartin> i'm not very comfortable to add this kind of dependency but perhaps there's no other way to do this
12917:54 <amovfx> wot
13017:54 <LarryRuane> raj-: I hadn't noticed that, good question! pablomartin: That's my understanding, it would let the friend class's methods (the mock functions in this case) to access the private members of CConnman (for example)
13117:54 <raj-> pablomartin, okay.. Ya that was clear but I was confused on why and had a hard time wrapping head around who depends on what..
13217:55 <LarryRuane> I think the idea is that test code needs to be able to dig into data structures, violate the layering so to speak, that you don't want production code to be able to do
13317:56 <kouloumos> Regarding a change like this that maybe results to the compiler not be able to inline some functions. How could someone measure the performance of such a change?
13417:57 <LarryRuane> We have a whole benchmarking framework
13517:58 <LarryRuane> maybe that would make it possible to measure? (You might need to write a specific benchmark test)
13617:58 <LarryRuane> Ok we're about out of time, let me skip ahead to question 10: What is the general approach of the version_handshake test? Why does it contain the series of calls to BOOST_CHECK_EQUAL()?
13717:58 <LarryRuane> (we sort of discussed this earlier)
13817:59 <brunoerg> To check if the messages are being sent correctly?
13917:59 <raj-> to make sure we are correctly sending all the `NetMsgType`s correctly at version handshake..
14018:00 <LarryRuane> Yes those are good answers, I was thinking it could maybe test that the p2p messages were being sent in the correct order (rather than just the number of times they were sent)
14118:00 <raj-> LarryRuane, +1.. That would be cool..
14218:01 <amovfx> makes sense, the handshake happens in a specific order correct?
14318:01 <LarryRuane> Well we're out of time, sorry we didn't get to every question, thank you all for coming! feel free to stay here and continue the discussion!
14418:01 <LarryRuane> #endmeeting