net, test: Virtualise CConnman and add initial PeerManager unit tests #25515

dergoegge commented at 6:04 PM on June 30, 2022: member

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.

fanquake added the label P2P on Jun 30, 2022

DrahtBot commented at 7:50 PM on June 30, 2022: contributor

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.

dergoegge force-pushed on Jun 30, 2022

dergoegge force-pushed on Jul 1, 2022

jnewbery commented at 7:51 AM on July 4, 2022: contributor

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

DrahtBot added the label Needs rebase on Jul 4, 2022

dergoegge force-pushed on Jul 5, 2022

DrahtBot removed the label Needs rebase on Jul 5, 2022

jamesob commented at 4:03 PM on July 5, 2022: member

Nice, concept ACK

DrahtBot added the label Needs rebase on Jul 12, 2022

maflcko referenced this in commit 2bdce7f7ad on Jul 19, 2022

dergoegge force-pushed on Jul 19, 2022

dergoegge renamed this:
~~[draft] PeerManager unit tests~~
net, test: Virtualise CConnman and add initial PeerManager unit tests
on Jul 19, 2022

dergoegge marked this as ready for review on Jul 19, 2022

dergoegge commented at 12:26 PM on July 19, 2022: member

This PR is ready for review!

DrahtBot removed the label Needs rebase on Jul 19, 2022

naumenkogs commented at 8:10 AM on August 25, 2022: member

Concept ACK, light code review ACK 87fd3609ba87a74fa6ca74c56ffbdb39fd941ebf

dergoegge force-pushed on Sep 14, 2022

dergoegge commented at 1:25 PM on September 14, 2022: member

Rebased.

[net] Define ConnectionsInterface 4f2533c5fd

[net processing] PeerManager holds a ConnectionsInterface&, not a CConnman& 9efcd9668d

dergoegge force-pushed on Sep 27, 2022

fanquake requested review from theuni on Oct 4, 2022

fanquake commented at 12:40 PM on October 4, 2022: member

Concept ACK

theStack commented at 2:05 PM on October 14, 2022: contributor

Concept ACK

in src/test/peerman_tests.cpp:87 in f98a4e8d89 outdated

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

rajarshimaitra commented at 12:03 PM on October 17, 2022:

Maybe I am missing something, but shouldn't this be sending message %s to peer %d??

dergoegge commented at 1:18 PM on October 21, 2022:

Right, PushMessage is called to send a message to a peer. Done.

in src/test/peerman_tests.cpp:282 in f98a4e8d89 outdated

 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

rajarshimaitra commented at 12:17 PM on October 17, 2022:

nit:

    // Ping metrics have not been updated on the CNode object, and there is a ping outstanding

rajarshimaitra commented at 12:30 PM on October 17, 2022: contributor

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.

in src/test/peerman_tests.cpp:102 in f98a4e8d89 outdated

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

rajarshimaitra commented at 2:09 PM on October 19, 2022:

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

dergoegge commented at 1:16 PM on October 21, 2022:

Could you suggest a change? I don't understand what you mean.

complete is passed by reference and modified by ReceiveMsgBytes.

brunoerg commented at 2:32 PM on October 21, 2022:

I think we could simplify ReceiveMsgBytes by not passing complete as reference and using the bool that this function returns. Does it make sense?

rajarshimaitra commented at 4:06 PM on October 23, 2022:

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

hernanmarino approved

hernanmarino commented at 6:10 PM on October 19, 2022: contributor

tested ACK 👍

rajarshimaitra commented at 6:36 PM on October 19, 2022: contributor

One more question..

[test] Add unit test for version handshake bdfe134ed6

[test] Add unit test for ping/pong logic 9c04c32101

dergoegge force-pushed on Oct 21, 2022

rajarshimaitra approved

rajarshimaitra commented at 4:33 PM on October 23, 2022: contributor

tACK 9c04c32101cd458c336de1d66ad296dc7f1cf35d

in src/test/peerman_tests.cpp:98 in bdfe134ed6 outdated

  93 | +            node.nProcessQueueSize += nSizeAdded;
  94 | +        }
  95 | +    }
  96 | +}
  97 | +
  98 | +bool ConnectionsInterfaceMock::ReceiveMsgFrom(CNode& node, CSerializedNetMsg& ser_msg) const

maflcko commented at 8:17 AM on October 24, 2022:

Any reason to duplicate this from the test util? Are there plans to de-duplicate this?

in src/test/peerman_tests.cpp:98 in 9c04c32101

  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 | +}

vasild commented at 9:54 AM on November 3, 2022:

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

in src/test/peerman_tests.cpp:252 in 9c04c32101

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

vasild commented at 10:00 AM on November 3, 2022:

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.

in src/test/peerman_tests.cpp:100 in 9c04c32101

  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

vasild commented at 10:30 AM on November 3, 2022:

This is duplicated from ConnmanTestMsg::NodeReceiveMsgBytes() + a subtle change.

in src/test/peerman_tests.cpp:258 in 9c04c32101

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

vasild commented at 10:47 AM on November 3, 2022:

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

in src/test/peerman_tests.cpp:135 in 9c04c32101

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

vasild commented at 10:54 AM on November 3, 2022:

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.

in src/test/peerman_tests.cpp:222 in 9c04c32101

 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,

vasild commented at 10:56 AM on November 3, 2022:

Same as SendMessage(): this is simulating the reception of a pong message from the peer and the name SendPong() is confusing.

in src/test/peerman_tests.cpp:270 in 9c04c32101

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

vasild commented at 11:15 AM on November 3, 2022:

Maybe use ASSERT_DEBUG_LOG("Unsolicited pong without ping") here.

in src/test/peerman_tests.cpp:146 in 9c04c32101

 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

vasild commented at 12:00 PM on November 3, 2022:

This is very similar to ConnmanTestMsg::Handshake(). To avoid code duplication, can it be used instead?

in src/test/peerman_tests.cpp:81 in bdfe134ed6 outdated

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

theStack commented at 4:30 PM on November 3, 2022:

According to our coding guidelines, assertions shouldn't have side-effects.

    bool success = node.ReceiveMsgBytes(msg_bytes, complete);
    assert(success);

in src/test/peerman_tests.cpp:14 in bdfe134ed6 outdated

   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>

theStack commented at 4:35 PM on November 3, 2022:

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>

jonatack commented at 4:06 PM on April 19, 2023:

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.

theStack commented at 4:44 PM on November 3, 2022: contributor

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.

in src/net.h:668 in 9c04c32101

 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

vasild commented at 2:57 PM on November 4, 2022:

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

dergoegge commented at 3:24 PM on November 4, 2022:

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.

vasild commented at 11:57 AM on November 7, 2022:

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

vasild commented at 8:19 AM on November 15, 2022:

the tests added in this PR can be done without ... touching the source code at all

Yes, that works. Here it is:

<details> <summary>tests</summary>

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

</details>

jonatack commented at 4:10 PM on April 19, 2023:

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

hernanmarino commented at 4:40 PM on November 22, 2022: contributor

cr 9c04c32101cd458c336de1d66ad296dc7f1cf35d . 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)

pablomartin4btc commented at 5:05 PM on November 22, 2022: member

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.

dergoegge marked this as a draft on Dec 1, 2022

DrahtBot added the label Needs rebase on Feb 22, 2023

DrahtBot commented at 6:47 PM on February 22, 2023: contributor

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

in src/test/peerman_tests.cpp:95 in bdfe134ed6 outdated

  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 | +    }

dergoegge commented at 5:49 PM on March 14, 2023:

#27257 dedupes this code across the entire code base, reviewing that would help move this forward.

jonatack commented at 4:12 PM on April 19, 2023: member

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

DrahtBot commented at 2:05 AM on July 18, 2023: 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 commented at 2:23 PM on August 10, 2023: member

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 closed this on Aug 10, 2023

bitcoin locked this on Aug 9, 2024