-
Notifications
You must be signed in to change notification settings - Fork 37.7k
net, test: Virtualise CConnman and add initial PeerManager unit tests #25515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
41ccb44
to
cbb9a87
Compare
9d1ccaa
to
0fdf0d4
Compare
Obvious concept ACK from me. This was one of the big motivators for #19398. |
daac55f
to
e3e0b99
Compare
Nice, concept ACK |
…Services to Peer 8d8eeb4 [net processing] Remove CNode::nLocalServices (John Newbery) 5961f8e [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge) d9079fe [net processing] Remove CNode::nServices (John Newbery) 7d1c036 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery) f65e83d [net processing] Remove fClient and m_limited_node (John Newbery) fc5eb52 [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery) 1f52c47 [net processing] Add m_our_services and m_their_services to Peer (John Newbery) Pull request description: Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`. This is also a prerequisite for adding `PeerManager` unit tests (See #25515). ACKs for top commit: MarcoFalke: ACK 8d8eeb4 🔑 jnewbery: utACK 8d8eeb4 mzumsande: Code Review ACK 8d8eeb4 Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
e3e0b99
to
87fd360
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested ACK 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more question..
void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const | ||
{ | ||
assert(node.ReceiveMsgBytes(msg_bytes, complete)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to pass in the complete
flag here?? Seems like we are overriding it anyway into node.ReceiveMsgBytes
.. So can't we just initiate it locally??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you suggest a change? I don't understand what you mean.
complete
is passed by reference and modified by ReceiveMsgBytes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could simplify ReceiveMsgBytes
by not passing complete
as reference and using the bool
that this function returns. Does it make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..
f98a4e8
to
9c04c32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 9c04c32
} | ||
} | ||
|
||
bool ConnectionsInterfaceMock::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to duplicate this from the test util? Are there plans to de-duplicate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed up to 9efcd96, 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.
|
||
void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const | ||
{ | ||
assert(node.ReceiveMsgBytes(msg_bytes, complete)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to our coding guidelines, assertions shouldn't have side-effects.
assert(node.ReceiveMsgBytes(msg_bytes, complete)); | |
bool success = node.ReceiveMsgBytes(msg_bytes, complete); | |
assert(success); |
#include <pubkey.h> | ||
#include <script/sign.h> | ||
#include <script/signingprovider.h> | ||
#include <script/standard.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp
index 8ab87be6b..9d0c02fd0 100644
--- a/src/test/peerman_tests.cpp
+++ b/src/test/peerman_tests.cpp
@@ -2,16 +2,11 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-#include <arith_uint256.h>
#include <banman.h>
#include <chainparams.h>
#include <net.h>
#include <net_processing.h>
#include <netmessagemaker.h>
-#include <pubkey.h>
-#include <script/sign.h>
-#include <script/signingprovider.h>
-#include <script/standard.h>
#include <serialize.h>
#include <span.h>
#include <test/util/net.h>
@@ -22,7 +17,6 @@
#include <timedata.h>
#include <validation.h>
-#include <array>
#include <optional>
#include <stdint.h>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
void ConnectionsInterfaceMock::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) | ||
{ | ||
BOOST_TEST_MESSAGE(strprintf("sending message %s to peer %d", msg.m_type, pnode->GetId())); | ||
m_message_types_sent[msg.m_type]++; | ||
|
||
CDataStream data{msg.data, SER_NETWORK, PROTOCOL_VERSION}; | ||
if (msg.m_type == NetMsgType::PING) OnPing(data); | ||
} | ||
|
||
void ConnectionsInterfaceMock::OnPing(CDataStream& data) | ||
{ | ||
data >> m_ping_nonce; | ||
BOOST_TEST_MESSAGE(strprintf("received ping. Nonce:%d", m_ping_nonce)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
BOOST_TEST_MESSAGE("Advanced mocktime and check that ping is sent after connection is established"); | ||
AdvanceMockTime(1); | ||
BOOST_CHECK_EQUAL(connman->m_message_types_sent[NetMsgType::PING], 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sequence is misleading. The ping message is sent immediately after the handshake. We do not need to "wait" 1 second for that to happen. So AdvanceMockTime(1)
may be removed.
BOOST_TEST_MESSAGE(strprintf("received ping. Nonce:%d", m_ping_nonce)); | ||
} | ||
|
||
void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated from ConnmanTestMsg::NodeReceiveMsgBytes()
+ a subtle change.
BOOST_CHECK(connman->m_ping_nonce != 0); | ||
|
||
// No pong response has been received yet, and current ping is outstanding | ||
CheckPingTimes(node, *peerman, std::chrono::microseconds::max(), 0us, 1s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 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()
.
static void SendMessage(ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node, CSerializedNetMsg& msg) | ||
{ | ||
std::atomic<bool> interrupt{false}; | ||
(void)connman.ReceiveMsgFrom(node, msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function simulates receiving a message from the peer. I find the name SendMessage()
confusing. Naming elsewhere in the code is from our point of view.
BOOST_CHECK_EQUAL(stats.m_ping_wait.count(), ping_wait.count()); | ||
} | ||
|
||
static void SendPong(CNode& node, ConnectionsInterfaceMock& connman, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as SendMessage()
: this is simulating the reception of a pong message from the peer and the name SendPong()
is confusing.
BOOST_TEST_MESSAGE("Reply without ping"); | ||
SendPong(node, *connman, *peerman, 0); | ||
|
||
// Ping metrics have not been updated on the CNode object, and there is no ping outstanding | ||
CheckPingTimes(node, *peerman, std::chrono::microseconds::max(), 0us, 0us); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use ASSERT_DEBUG_LOG("Unsolicited pong without ping")
here.
static void Handshake( | ||
ConnectionsInterfaceMock& connman, PeerManager& peerman, | ||
CNode& node, ServiceFlags their_services, | ||
HandshakeHookFn pre_verack_hook = [](ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node) {}) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very similar to ConnmanTestMsg::Handshake()
. To avoid code duplication, can it be used instead?
@@ -663,7 +664,64 @@ class NetEventsInterface | |||
~NetEventsInterface() = default; | |||
}; | |||
|
|||
class CConnman | |||
/** Interface class for interacting with CConnman */ | |||
class ConnectionsInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the tests added in this PR can be done without virtualizing 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
- Defining an interface for it gets us a step closer to hiding more implementation details of our net module (similar to what we did with net processing. See
net_processing.h
, it (almost) only has the interface definition forPeerManager
). - Mocking
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 aFuzzedConnman
(similar to your work onFuzzedSock
). - Future tests might need more functionality besides capturing messages, so having the ability to mock the entire
CConnman
would be nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... i think that virtualizing
CConnman
is 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
CConnman
is much easier if such an interface definition exists ...- Future tests might need more functionality besides capturing messages ...
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests added in this PR can be done without ... touching the source code at all
Yes, that works. Here it is:
tests
diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index f24509dd97..105f4e4487 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -11,12 +11,14 @@
#include <netaddress.h>
#include <netbase.h>
#include <netmessagemaker.h>
#include <serialize.h>
#include <span.h>
#include <streams.h>
+#include <test/util/logging.h>
+#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <timedata.h>
#include <util/strencodings.h>
#include <util/string.h>
#include <util/system.h>
@@ -902,7 +904,213 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
// 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();
}
+BOOST_AUTO_TEST_CASE(initial_messages_sent)
+{
+ LOCK(NetEventsInterface::g_msgproc_mutex);
+
+ m_node.args->ForceSetArg("-capturemessages", "1");
+
+ auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+ auto& peerman = *m_node.peerman;
+
+ CNode outbound_peer{
+ /*id=*/NodeId{0},
+ /*sock=*/nullptr,
+ /*addrIn=*/CAddress{CService{CNetAddr{in_addr{.s_addr = htonl(0x01020304)}}, 8333}, NODE_NONE},
+ /*nKeyedNetGroupIn=*/0,
+ /*nLocalHostNonceIn=*/0,
+ /*addrBindIn=*/CAddress{},
+ /*addrNameIn=*/"",
+ /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY,
+ /*inbound_onion=*/false};
+
+ std::unordered_map<std::string, size_t> count_sent_messages;
+
+ const auto CaptureMessageOrig = CaptureMessage;
+ CaptureMessage = [&count_sent_messages](const CAddress& addr,
+ const std::string& msg_type,
+ Span<const unsigned char> data,
+ bool is_incoming) -> void {
+ if (!is_incoming) {
+ count_sent_messages[msg_type]++;
+ }
+ };
+
+ connman.Handshake(
+ /*node=*/outbound_peer,
+ /*successfully_connected=*/true,
+ /*remote_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
+ /*local_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
+ /*version=*/PROTOCOL_VERSION,
+ /*relay_txs=*/true);
+
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::VERSION], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::WTXIDRELAY], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::SENDADDRV2], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::VERACK], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::GETADDR], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::SENDCMPCT], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::PING], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::GETHEADERS], 1);
+ BOOST_CHECK_EQUAL(count_sent_messages[NetMsgType::FEEFILTER], 1);
+
+ peerman.FinalizeNode(outbound_peer);
+
+ CaptureMessage = CaptureMessageOrig;
+ m_node.args->ForceSetArg("-capturemessages", "0");
+ // 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();
+}
+
+BOOST_AUTO_TEST_CASE(pingpong)
+{
+ // See PING_INTERVAL in net_processing.cpp
+ static constexpr auto PING_INTERVAL{2min};
+
+ m_node.args->ForceSetArg("-capturemessages", "1");
+
+ auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
+ auto& peerman = *m_node.peerman;
+
+ CNode outbound_peer{
+ /*id=*/NodeId{0},
+ /*sock=*/nullptr,
+ /*addrIn=*/CAddress{CService{CNetAddr{in_addr{.s_addr = htonl(0x01020304)}}, 8333}, NODE_NONE},
+ /*nKeyedNetGroupIn=*/0,
+ /*nLocalHostNonceIn=*/0,
+ /*addrBindIn=*/CAddress{},
+ /*addrNameIn=*/"",
+ /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY,
+ /*inbound_onion=*/false};
+
+ auto CheckPingTimes = [&peerman, &outbound_peer](std::chrono::microseconds min_ping_time,
+ std::chrono::microseconds last_ping_time,
+ std::chrono::microseconds ping_wait) {
+ // Check min and last ping times
+ BOOST_CHECK_EQUAL(outbound_peer.m_min_ping_time.load().count(), min_ping_time.count());
+ BOOST_CHECK_EQUAL(outbound_peer.m_last_ping_time.load().count(), last_ping_time.count());
+
+ // Check if and how long current ping has been pending
+ CNodeStateStats stats;
+ BOOST_REQUIRE(peerman.GetNodeStateStats(outbound_peer.GetId(), stats));
+ BOOST_CHECK_EQUAL(stats.m_ping_wait.count(), ping_wait.count());
+ };
+
+ uint64_t ping_nonce_sent;
+
+ const auto CaptureMessageOrig = CaptureMessage;
+ CaptureMessage = [&ping_nonce_sent](const CAddress& addr,
+ const std::string& msg_type,
+ Span<const unsigned char> data,
+ bool is_incoming) {
+ if (!is_incoming && msg_type == NetMsgType::PING) {
+ CDataStream stream{data, SER_NETWORK, PROTOCOL_VERSION};
+ stream >> ping_nonce_sent;
+ }
+ };
+
+ const auto SendPing = [&peerman, &outbound_peer, &ping_nonce_sent]() EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
+ SetMockTime(GetMockTime() + PING_INTERVAL + 1s);
+ ping_nonce_sent = 0;
+ peerman.SendMessages(&outbound_peer);
+ BOOST_REQUIRE(ping_nonce_sent != 0);
+ };
+
+ const auto ReceiveAndProcessMessage = [&connman, &outbound_peer](CSerializedNetMsg& msg) EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) {
+ connman.ReceiveMsgFrom(outbound_peer, msg);
+ // The send buffer CConnman::nSendBufferMaxSize happens to be 0 during tests which
+ // trips fPauseSend to be set to true which cancels processing of incoming messages.
+ outbound_peer.fPauseSend = false;
+ connman.ProcessMessagesOnce(outbound_peer);
+ };
+
+ // Use mock time to control pings from node
+ SetMockTime(GetTime());
+
+ ping_nonce_sent = 0;
+
+ LOCK(NetEventsInterface::g_msgproc_mutex);
+
+ connman.Handshake(
+ /*node=*/outbound_peer,
+ /*successfully_connected=*/true,
+ /*remote_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
+ /*local_services=*/ServiceFlags{NODE_NETWORK | NODE_WITNESS},
+ /*version=*/PROTOCOL_VERSION,
+ /*relay_txs=*/true);
+ BOOST_REQUIRE(ping_nonce_sent != 0);
+
+ auto time_elapsed = 1s;
+ SetMockTime(GetMockTime() + time_elapsed);
+ // No pong response has been received and current ping is outstanding.
+ CheckPingTimes(std::chrono::microseconds::max(), 0us, time_elapsed);
+
+ CSerializedNetMsg pong_msg;
+
+ BOOST_TEST_MESSAGE("Receiving a PONG without nonce cancels our PING");
+ {
+ ASSERT_DEBUG_LOG("Short payload");
+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG);
+ ReceiveAndProcessMessage(pong_msg);
+ }
+ // Ping metrics have not been updated and there is no ping outstanding.
+ CheckPingTimes(std::chrono::microseconds::max(), 0us, 0us);
+
+ BOOST_TEST_MESSAGE("Receiving an unrequested PONG is logged and ignored");
+ {
+ ASSERT_DEBUG_LOG("Unsolicited pong without ping");
+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, /*nonce=*/(uint64_t)0);
+ ReceiveAndProcessMessage(pong_msg);
+ }
+ // Ping metrics have not been updated and there is no ping outstanding.
+ CheckPingTimes(std::chrono::microseconds::max(), 0us, 0us);
+
+ SendPing();
+
+ BOOST_TEST_MESSAGE("Receiving a PONG with the wrong nonce does not cancel our PING");
+ {
+ ASSERT_DEBUG_LOG("Nonce mismatch");
+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, /*nonce=*/ping_nonce_sent + 1);
+ ReceiveAndProcessMessage(pong_msg);
+ }
+ // Ping metrics have not been updated and there is an outstanding ping.
+ time_elapsed = 5s;
+ SetMockTime(GetMockTime() + time_elapsed);
+ CheckPingTimes(std::chrono::microseconds::max(), 0us, time_elapsed);
+
+ BOOST_TEST_MESSAGE("Receiving a PONG with nonce=0 cancels our PING");
+ {
+ ASSERT_DEBUG_LOG("Nonce zero");
+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, /*nonce=*/(uint64_t)0);
+ ReceiveAndProcessMessage(pong_msg);
+ }
+ // Ping metrics have not been updated and there is no ping outstanding.
+ CheckPingTimes(std::chrono::microseconds::max(), 0us, 0us);
+
+ SendPing();
+
+ time_elapsed = 5s;
+ SetMockTime(GetMockTime() + time_elapsed);
+
+ BOOST_TEST_MESSAGE("Receiving a PONG with the correct nonce cancels our PING");
+ pong_msg = CNetMsgMaker{PROTOCOL_VERSION}.Make(NetMsgType::PONG, ping_nonce_sent);
+ ReceiveAndProcessMessage(pong_msg);
+ // Ping metrics have been updated and there is no ping outstanding.
+ CheckPingTimes(time_elapsed, time_elapsed, 0us);
+
+ peerman.FinalizeNode(outbound_peer);
+
+ CaptureMessage = CaptureMessageOrig;
+ m_node.args->ForceSetArg("-capturemessages", "0");
+ // 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();
+}
+
BOOST_AUTO_TEST_SUITE_END()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 jonatack@b053832).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
size_t nSizeAdded = 0; | ||
auto it(node.vRecvMsg.begin()); | ||
for (; it != node.vRecvMsg.end(); ++it) { | ||
// vRecvMsg contains only completed CNetMessage | ||
// the single possible partially deserialized message are held by TransportDeserializer | ||
nSizeAdded += it->m_raw_message_size; | ||
} | ||
{ | ||
LOCK(node.cs_vProcessMsg); | ||
node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg, node.vRecvMsg.begin(), it); | ||
node.nProcessQueueSize += nSizeAdded; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#27257 dedupes this code across the entire code base, reviewing that would help move this forward.
Concept ACK on adding testing modulo the comments above and #25515 (comment). Built and ran a green make check on each commit. Needs rebase. |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
Closing for now, I want to do some interface work before adding the tests and will open a large meta PR soon that includes all the refactoring + tests at the end. |
This PR creates an abstract
ConnectionsInterface
class that is used as an interface for interacting with the connection manager. ThePeerManager
is made to hold a reference to aConnectionsInterface
instead ofCConnman
, which makes it possible for us to mock the connection manager in the newly introducedPeerManager
unit tests. Two initial unit tests are added for the version handshake and ping/pong logic.