Skip to content

Conversation

dergoegge
Copy link
Member

@dergoegge dergoegge commented Jun 30, 2022

This PR creates an abstract 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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 30, 2022

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

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26837 (I2P network optimizations by vasild)
  • #26441 (rpc, p2p: add addpermissionflags RPC and allow whitelisting outbound by brunoerg)
  • #25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
  • #25268 (refactor: Introduce EvictionManager by dergoegge)
  • #24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
  • #24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
  • #24170 (p2p, rpc: Manual block-relay-only connections with addnode by mzumsande)

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.

@jnewbery
Copy link
Contributor

jnewbery commented Jul 4, 2022

Obvious concept ACK from me. This was one of the big motivators for #19398.

@dergoegge dergoegge force-pushed the 2022-06-virt-connman branch 2 times, most recently from daac55f to e3e0b99 Compare July 5, 2022 15:40
@jamesob
Copy link
Contributor

jamesob commented Jul 5, 2022

Nice, concept ACK

maflcko pushed a commit that referenced this pull request Jul 19, 2022
…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
@dergoegge dergoegge force-pushed the 2022-06-virt-connman branch from e3e0b99 to 87fd360 Compare July 19, 2022 12:16
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a 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.

Copy link
Contributor

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK 👍

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more question..

Comment on lines +100 to +102
void ConnectionsInterfaceMock::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_bytes, bool& complete) const
{
assert(node.ReceiveMsgBytes(msg_bytes, complete));
Copy link
Contributor

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??

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a 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
Copy link
Member

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?

Copy link
Contributor

@theStack theStack left a 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));
Copy link
Contributor

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.

Suggested change
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>
Copy link
Contributor

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>

Copy link
Member

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.

Comment on lines +85 to +98
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));
}
Copy link
Contributor

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

Comment on lines +250 to +252
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);
Copy link
Contributor

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
Copy link
Contributor

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);
Copy link
Contributor

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

Comment on lines +132 to +135
static void SendMessage(ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node, CSerializedNetMsg& msg)
{
std::atomic<bool> interrupt{false};
(void)connman.ReceiveMsgFrom(node, msg);
Copy link
Contributor

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,
Copy link
Contributor

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.

Comment on lines +266 to +270
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);
Copy link
Contributor

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.

Comment on lines +143 to +146
static void Handshake(
ConnectionsInterfaceMock& connman, PeerManager& peerman,
CNode& node, ServiceFlags their_services,
HandshakeHookFn pre_verack_hook = [](ConnectionsInterfaceMock& connman, PeerManager& peerman, CNode& node) {}) noexcept
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member Author

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 for PeerManager).
  • 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 a FuzzedConnman (similar to your work on FuzzedSock).
  • Future tests might need more functionality besides capturing messages, so having the ability to mock the entire CConnman would be nice.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Member

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

Copy link
Contributor

@hernanmarino hernanmarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cr 9c04c32 . Mostly ACK in my opinion, but i agree with some of the others suggestions ( most of @vasild 's and some other's I marked with a thumbs up)

Copy link
Member

@pablomartin4btc pablomartin4btc left a 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.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Comment on lines +83 to +95
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;
}
}
Copy link
Member Author

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.

@jonatack
Copy link
Member

Concept ACK on adding testing modulo the comments above and #25515 (comment). Built and ran a green make check on each commit. Needs rebase.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@dergoegge
Copy link
Member Author

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.

@dergoegge dergoegge closed this Aug 10, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.