ConnectionsInterface class that is used as an interface for interacting with the connection manager. The PeerManager is made to hold a reference to a ConnectionsInterface instead of CConnman, which makes it possible for us to mock the connection manager in the newly introduced PeerManager unit tests. Two initial unit tests are added for the version handshake and ping/pong logic.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rajarshimaitra |
| Concept ACK | jnewbery, jamesob, naumenkogs, fanquake, theStack, hernanmarino, pablomartin4btc, jonatack |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
addpermissionflags RPC and allow whitelisting outbound by brunoerg)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.
82+
83+};
84+
85+void ConnectionsInterfaceMock::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
86+{
87+ BOOST_TEST_MESSAGE(strprintf("received message %s from peer %d", msg.m_type, pnode->GetId()));
sending message %s to peer %d??
PushMessage is called to send a message to a peer. Done.
277+
278+ BOOST_TEST_MESSAGE("Reply with wrong nonce does not cancel ping");
279+ SendPong(node, *connman, *peerman, connman->m_ping_nonce + 1);
280+ AdvanceMockTime(1);
281+
282+ // Ping metrics have not been updated on the CNode object, and there is a ping outstandgin
nit:
0 // Ping metrics have not been updated on the CNode object, and there is a ping outstanding
Concept ACK.. I made a first pass and changes look good to me.. I believe its helpful to separate out the interface, and possibly add more thorough unit testing later on?
I will go through the test logic more thoroughly, for now just two observations.
97+ BOOST_TEST_MESSAGE(strprintf("received ping. Nonce:%d", m_ping_nonce));
98+}
99+
100+void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
101+{
102+ assert(node.ReceiveMsgBytes(msg_bytes, complete));
complete flag here?? Seems like we are overriding it anyway into node.ReceiveMsgBytes.. So can’t we just initiate it locally??
Could you suggest a change? I don’t understand what you mean.
complete is passed by reference and modified by ReceiveMsgBytes.
ReceiveMsgBytes by not passing complete as reference and using the bool that this function returns. Does it make sense?
I think I would retrace back my comment.. These signatures are part of the interface itself.. So there might be valid reasons for them..
Though it seems like we are ultimately dropping the bool here in test case in .. The only place its used is inside NodeReceiveMsgBytes. So at first look passing in a value that we are internally processing and then dropping in final return (in the line (void)connman.ReceiveMsgFrom(node, msg);) was confusing me.. I am probably still confused but my suggestion doesn’t make any sense as it will modify the interface..
93+ node.nProcessQueueSize += nSizeAdded;
94+ }
95+ }
96+}
97+
98+bool ConnectionsInterfaceMock::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const
93+
94+void ConnectionsInterfaceMock::OnPing(CDataStream& data)
95+{
96+ data >> m_ping_nonce;
97+ BOOST_TEST_MESSAGE(strprintf("received ping. Nonce:%d", m_ping_nonce));
98+}
OnPing() is called when sending a ping message, but the log says “received ping”. Also, the comment of m_ping_nonce is “Most recent ping nonce received”.
247+
248+ Handshake(*connman, *peerman, node, /*their_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS));
249+
250+ BOOST_TEST_MESSAGE("Advanced mocktime and check that ping is sent after connection is established");
251+ AdvanceMockTime(1);
252+ BOOST_CHECK_EQUAL(connman->m_message_types_sent[NetMsgType::PING], 1);
AdvanceMockTime(1) may be removed.
95+{
96+ data >> m_ping_nonce;
97+ BOOST_TEST_MESSAGE(strprintf("received ping. Nonce:%d", m_ping_nonce));
98+}
99+
100+void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
ConnmanTestMsg::NodeReceiveMsgBytes() + a subtle change.
253+
254+ // The ping nonce should be non-zero
255+ BOOST_CHECK(connman->m_ping_nonce != 0);
256+
257+ // No pong response has been received yet, and current ping is outstanding
258+ CheckPingTimes(node, *peerman, std::chrono::microseconds::max(), 0us, 1s);
AdvanceMockTime(1) from above belongs to just before this call. If the “1 second” is put in a variable that will make it obvious that it is the same as the 4th argument to CheckPingTimes().
130+BOOST_FIXTURE_TEST_SUITE(peerman_tests, TestingSetup)
131+
132+static void SendMessage(ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node, CSerializedNetMsg& msg)
133+{
134+ std::atomic<bool> interrupt{false};
135+ (void)connman.ReceiveMsgFrom(node, msg);
SendMessage() confusing. Naming elsewhere in the code is from our point of view.
217+ CNodeStateStats stats;
218+ peerman.GetNodeStateStats(node.GetId(), stats);
219+ BOOST_CHECK_EQUAL(stats.m_ping_wait.count(), ping_wait.count());
220+}
221+
222+static void SendPong(CNode& node, ConnectionsInterfaceMock& connman,
SendMessage(): this is simulating the reception of a pong message from the peer and the name SendPong() is confusing.
265+
266+ BOOST_TEST_MESSAGE("Reply without ping");
267+ SendPong(node, *connman, *peerman, 0);
268+
269+ // Ping metrics have not been updated on the CNode object, and there is no ping outstanding
270+ CheckPingTimes(node, *peerman, std::chrono::microseconds::max(), 0us, 0us);
ASSERT_DEBUG_LOG("Unsolicited pong without ping") here.
141+
142+using HandshakeHookFn = std::function<void(ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node)>;
143+static void Handshake(
144+ ConnectionsInterfaceMock& connman, PeerManager& peerman,
145+ CNode& node, ServiceFlags their_services,
146+ HandshakeHookFn pre_verack_hook = [](ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node) {}) noexcept
ConnmanTestMsg::Handshake(). To avoid code duplication, can it be used instead?
76+ m_message_types_sent[msg.m_type]++;
77+}
78+
79+void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
80+{
81+ assert(node.ReceiveMsgBytes(msg_bytes, complete));
According to our coding guidelines, assertions shouldn’t have side-effects.
0 bool success = node.ReceiveMsgBytes(msg_bytes, complete);
1 assert(success);
9+#include <net_processing.h>
10+#include <netmessagemaker.h>
11+#include <pubkey.h>
12+#include <script/sign.h>
13+#include <script/signingprovider.h>
14+#include <script/standard.h>
nit: Seems like a lot of these includes are unnecessary and can be removed; e.g. I don’t see any arith_uint256-, pubkey- or script-related stuff used in the unit test. With the following diff it still compiles:
0diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp
1index 8ab87be6b..9d0c02fd0 100644
2--- a/src/test/peerman_tests.cpp
3+++ b/src/test/peerman_tests.cpp
4@@ -2,16 +2,11 @@
5 // Distributed under the MIT software license, see the accompanying
6 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
7
8-#include <arith_uint256.h>
9 #include <banman.h>
10 #include <chainparams.h>
11 #include <net.h>
12 #include <net_processing.h>
13 #include <netmessagemaker.h>
14-#include <pubkey.h>
15-#include <script/sign.h>
16-#include <script/signingprovider.h>
17-#include <script/standard.h>
18 #include <serialize.h>
19 #include <span.h>
20 #include <test/util/net.h>
21@@ -22,7 +17,6 @@
22 #include <timedata.h>
23 #include <validation.h>
24
25-#include <array>
26 #include <optional>
27 #include <stdint.h>
Seems like a lot of these includes are unnecessary and can be removed… With the following diff it still compiles
A way to verify is to add the file to ci/test/06_script_b.sh and look at the lint CI output after repushing.
Reviewed up to 9efcd9668d143afa2e8213a7bacf94da7f645e4c, still have to look a bit deeper how the unit tests exactly work. To get the ball rolling, maybe it’s worth it to split up the PR into two, one for virtualising CConnman (which is I think is way easier to review and could be merged pretty fast), and one for the unit tests?
For the latter, as Marco pointed out, it seems like there is potential for deduplication with the routines in test/util/net.cpp. Left also two minor review comments below.
663@@ -663,7 +664,64 @@ class NetEventsInterface
664 ~NetEventsInterface() = default;
665 };
666
667-class CConnman
668+/** Interface class for interacting with CConnman */
669+class ConnectionsInterface
CConnman and without touching the source code at all, by capturing the sent and received messages (see net_tests/initial_advertise_from_version_message for an example).
Huh I have not seen that before, thanks for bringing it up! While we could do that, i think that virtualizing CConnman is just the right thing to do for several reasons.
net_processing.h, it (almost) only has the interface definition for PeerManager).CConnman is much easier if such an interface definition exists. That’s nice for unit tests but also nice for fuzz testing as we could have a FuzzedConnman (similar to your work on FuzzedSock).CConnman would be nice.
… i think that virtualizing
CConnmanis just the right thing to do for several reasons
Maybe put those in the OP to make the case for virtualizing stronger.
Defining an interface for it gets us a step closer to hiding more implementation details …
In general, I wonder if we are going a step too far with “hiding implementation details”. Are we aiming to remove private: sections from classes defined in header files?
PImpl and virtualization have their use cases but IMO shouldn’t be used as if they are universally good (as in “lets pimpl and virtualize everything because the code will become better just by doing that”). They could have adverse effects if abused.
- Mocking
CConnmanis much easier if such an interface definition exists …- Future tests might need more functionality besides capturing messages …
I agree.
the tests added in this PR can be done without … touching the source code at all
Yes, that works. Here it is:
0diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
1index f24509dd97..105f4e4487 100644
2--- a/src/test/net_tests.cpp
3+++ b/src/test/net_tests.cpp
4@@ -11,12 +11,14 @@
5 #include <netaddress.h>
6 #include <netbase.h>
7 #include <netmessagemaker.h>
8 #include <serialize.h>
9 #include <span.h>
10 #include <streams.h>
11+#include <test/util/logging.h>
12+#include <test/util/net.h>
13 #include <test/util/setup_common.h>
14 #include <test/util/validation.h>
15 #include <timedata.h>
16 #include <util/strencodings.h>
17 #include <util/string.h>
18 #include <util/system.h>
19@@ -902,7 +904,213 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
20 // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state
21 // in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset
22 // that state as it was before our test was run.
23 TestOnlyResetTimeData();
24 }
25
26+BOOST_AUTO_TEST_CASE(initial_messages_sent)
27+{
28+ LOCK(NetEventsInterface::g_msgproc_mutex);
29+
30+ m_node.args->ForceSetArg("-capturemessages", "1");
31+
32+ auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
33+ auto& peerman = *m_node.peerman;
34+
35+ CNode outbound_peer{
36+ /*id=*/NodeId{0},
37+ /*sock=*/nullptr,
38+ /*addrIn=*/CAddress{CService{CNetAddr{in_addr{.s_addr = htonl(0x01020304)}}, 8333}, NODE_NONE},
39+ /*nKeyedNetGroupIn=*/0,
40+ /*nLocalHostNonceIn=*/0,
41+ /*addrBindIn=*/CAddress{},
42+ /*addrNameIn=*/"",
43+ /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY,
44+ /*inbound_onion=*/false};
45+
46+ std::unordered_map<std::string, size_t> count_sent_messages;
47+
48+ const auto CaptureMessageOrig = CaptureMessage;
49+ CaptureMessage = [&count_sent_messages](const CAddress& addr,
50+ const std::string& msg_type,
51+ Span<const unsigned char> data,
52+ bool is_incoming) -> void {
53+ if (!is_incoming) {
54+ count_sent_messages[msg_type]++;
55+ }
56+ };
57+
58+ connman.Handshake(
59+ /*node=*/outbound_peer,
60+ /*successfully_connected=*/true,
61+ /*remote_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
62+ /*local_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
63+ /*version=*/PROTOCOL_VERSION,
64+ /*relay_txs=*/true);
65+
66+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::VERSION], 1);
67+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::WTXIDRELAY], 1);
68+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::SENDADDRV2], 1);
69+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::VERACK], 1);
70+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::GETADDR], 1);
71+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::SENDCMPCT], 1);
72+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::PING], 1);
73+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::GETHEADERS], 1);
74+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::FEEFILTER], 1);
75+
76+ peerman.FinalizeNode(outbound_peer);
77+
78+ CaptureMessage = CaptureMessageOrig;
79+ m_node.args->ForceSetArg("-capturemessages", "0");
80+ // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state
81+ // in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset
82+ // that state as it was before our test was run.
83+ TestOnlyResetTimeData();
84+}
85+
86+BOOST_AUTO_TEST_CASE(pingpong)
87+{
88+ // See PING_INTERVAL in net_processing.cpp
89+ static constexpr auto PING_INTERVAL{2min};
90+
91+ m_node.args->ForceSetArg("-capturemessages", "1");
92+
93+ auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
94+ auto& peerman = *m_node.peerman;
95+
96+ CNode outbound_peer{
97+ /*id=*/NodeId{0},
98+ /*sock=*/nullptr,
99+ /*addrIn=*/CAddress{CService{CNetAddr{in_addr{.s_addr = htonl(0x01020304)}}, 8333}, NODE_NONE},
100+ /*nKeyedNetGroupIn=*/0,
101+ /*nLocalHostNonceIn=*/0,
102+ /*addrBindIn=*/CAddress{},
103+ /*addrNameIn=*/"",
104+ /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY,
105+ /*inbound_onion=*/false};
106+
107+ auto CheckPingTimes = [&peerman, &outbound_peer](std::chrono::microseconds min_ping_time,
108+ std::chrono::microseconds last_ping_time,
109+ std::chrono::microseconds ping_wait) {
110+ // Check min and last ping times
111+ BOOST_CHECK_EQUAL(outbound_peer.m_min_ping_time.load().count(), min_ping_time.count());
112+ BOOST_CHECK_EQUAL(outbound_peer.m_last_ping_time.load().count(), last_ping_time.count());
113+
114+ // Check if and how long current ping has been pending
115+ CNodeStateStats stats;
116+ BOOST_REQUIRE(peerman.GetNodeStateStats(outbound_peer.GetId(), stats));
117+ BOOST_CHECK_EQUAL(stats.m_ping_wait.count(), ping_wait.count());
118+ };
119+
120+ uint64_t ping_nonce_sent;
121+
122+ const auto CaptureMessageOrig = CaptureMessage;
123+ CaptureMessage = [&ping_nonce_sent](const CAddress& addr,
124+ const std::string& msg_type,
125+ Span<const unsigned char> data,
126+ bool is_incoming) {
127+ if (!is_incoming && msg_type == NetMsgType::PING) {
128+ CDataStream stream{data, SER_NETWORK, PROTOCOL_VERSION};
129+ stream >> ping_nonce_sent;
130+ }
131+ };
132+
133+ const auto SendPing = [&peerman, &outbound_peer, &ping_nonce_sent]() EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
134+ SetMockTime(GetMockTime() + PING_INTERVAL + 1s);
135+ ping_nonce_sent = 0;
136+ peerman.SendMessages(&outbound_peer);
137+ BOOST_REQUIRE(ping_nonce_sent != 0);
138+ };
139+
140+ const auto ReceiveAndProcessMessage = [&connman, &outbound_peer](CSerializedNetMsg& msg) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
141+ connman.ReceiveMsgFrom(outbound_peer, msg);
142+ // The send buffer CConnman::nSendBufferMaxSize happens to be 0 during tests which
143+ // trips fPauseSend to be set to true which cancels processing of incoming messages.
144+ outbound_peer.fPauseSend = false;
145+ connman.ProcessMessagesOnce(outbound_peer);
146+ };
147+
148+ // Use mock time to control pings from node
149+ SetMockTime(GetTime());
150+
151+ ping_nonce_sent = 0;
152+
153+ LOCK(NetEventsInterface::g_msgproc_mutex);
154+
155+ connman.Handshake(
156+ /*node=*/outbound_peer,
157+ /*successfully_connected=*/true,
158+ /*remote_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
159+ /*local_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
160+ /*version=*/PROTOCOL_VERSION,
161+ /*relay_txs=*/true);
162+ BOOST_REQUIRE(ping_nonce_sent != 0);
163+
164+ auto time_elapsed = 1s;
165+ SetMockTime(GetMockTime() + time_elapsed);
166+ // No pong response has been received and current ping is outstanding.
167+ CheckPingTimes(std::chrono::microseconds::max(), 0us, time_elapsed);
168+
169+ CSerializedNetMsg pong_msg;
170+
171+ BOOST_TEST_MESSAGE("Receiving a PONG without nonce cancels our PING");
172+ {
173+ ASSERT_DEBUG_LOG("Short payload");
174+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG);
175+ ReceiveAndProcessMessage(pong_msg);
176+ }
177+ // Ping metrics have not been updated and there is no ping outstanding.
178+ CheckPingTimes(std::chrono::microseconds::max(), 0us, 0us);
179+
180+ BOOST_TEST_MESSAGE("Receiving an unrequested PONG is logged and ignored");
181+ {
182+ ASSERT_DEBUG_LOG("Unsolicited pong without ping");
183+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, /*nonce=*/(uint64_t)0);
184+ ReceiveAndProcessMessage(pong_msg);
185+ }
186+ // Ping metrics have not been updated and there is no ping outstanding.
187+ CheckPingTimes(std::chrono::microseconds::max(), 0us, 0us);
188+
189+ SendPing();
190+
191+ BOOST_TEST_MESSAGE("Receiving a PONG with the wrong nonce does not cancel our PING");
192+ {
193+ ASSERT_DEBUG_LOG("Nonce mismatch");
194+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, /*nonce=*/ping_nonce_sent + 1);
195+ ReceiveAndProcessMessage(pong_msg);
196+ }
197+ // Ping metrics have not been updated and there is an outstanding ping.
198+ time_elapsed = 5s;
199+ SetMockTime(GetMockTime() + time_elapsed);
200+ CheckPingTimes(std::chrono::microseconds::max(), 0us, time_elapsed);
201+
202+ BOOST_TEST_MESSAGE("Receiving a PONG with nonce=0 cancels our PING");
203+ {
204+ ASSERT_DEBUG_LOG("Nonce zero");
205+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, /*nonce=*/(uint64_t)0);
206+ ReceiveAndProcessMessage(pong_msg);
207+ }
208+ // Ping metrics have not been updated and there is no ping outstanding.
209+ CheckPingTimes(std::chrono::microseconds::max(), 0us, 0us);
210+
211+ SendPing();
212+
213+ time_elapsed = 5s;
214+ SetMockTime(GetMockTime() + time_elapsed);
215+
216+ BOOST_TEST_MESSAGE("Receiving a PONG with the correct nonce cancels our PING");
217+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, ping_nonce_sent);
218+ ReceiveAndProcessMessage(pong_msg);
219+ // Ping metrics have been updated and there is no ping outstanding.
220+ CheckPingTimes(time_elapsed, time_elapsed, 0us);
221+
222+ peerman.FinalizeNode(outbound_peer);
223+
224+ CaptureMessage = CaptureMessageOrig;
225+ m_node.args->ForceSetArg("-capturemessages", "0");
226+ // PeerManager::ProcessMessage() calls AddTimeData() which changes the internal state
227+ // in timedata.cpp and later confuses the test "timedata_tests/addtimedata". Thus reset
228+ // that state as it was before our test was run.
229+ TestOnlyResetTimeData();
230+}
231+
232 BOOST_AUTO_TEST_SUITE_END()
the tests added in this PR can be done without … touching the source code at all
Yes, that works. Here it is:
Nice!
Virtualization/dynamic dispatch has performance and overhead costs in addition to increased class/code size and complexity, and there are places in this codebase where it seems to have been avoided, and still is, for that reason. For an example, see the discussion in #25349.
If virtualization is still considered to be worth the overhead here, on its own merits above and beyond facilitating unit testing that can be done without it, it may be good to open a PR doing only that, as also suggested by someone above (and maybe consider a non-virtual public interface that calls private virtual implementation methods, i.e. the Template pattern via the Non-Virtual Interface (NVI) idiom. There is a simple example at https://github.com/jonatack/bitcoin/commit/b05383211239d564e98b19c96c80ff778e054146).
Tested ACK.
I’m not very comfortable using friend class as it breaks encapsulation, perhaps it’s something we could avoid at some point looking forward.
🐙 This pull request conflicts with the target branch and needs rebase.
90+ {
91+ LOCK(node.cs_vProcessMsg);
92+ node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg, node.vRecvMsg.begin(), it);
93+ node.nProcessQueueSize += nSizeAdded;
94+ }
95+ }
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
