From ff6ef82e6afb62c4dccfd87bf537efb3d670ebd5 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Fri, 14 May 2021 10:15:37 +0100 Subject: [PATCH 1/3] network.last_contacted: rename channel2 to channel0 This is done so that node numbering numbering matches the channel numbering, which makes easy to determine which channel belongs to which node. --- nano/core_test/network.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nano/core_test/network.cpp b/nano/core_test/network.cpp index 304c727e6c..e9ddcda35f 100644 --- a/nano/core_test/network.cpp +++ b/nano/core_test/network.cpp @@ -143,15 +143,15 @@ TEST (network, last_contacted) auto channel1 = nano::establish_tcp (system, *node1, node0->network.endpoint ()); ASSERT_NE (nullptr, channel1); ASSERT_TIMELY (3s, node0->network.size () == 1); - auto channel2 = node0->network.tcp_channels.find_node_id (node1->node_id.pub); - ASSERT_NE (nullptr, channel2); + auto channel0 = node0->network.tcp_channels.find_node_id (node1->node_id.pub); + ASSERT_NE (nullptr, channel0); // Make sure last_contact gets updated on receiving a non-handshake message - auto timestamp_before_keepalive = channel2->get_last_packet_received (); + auto timestamp_before_keepalive = channel0->get_last_packet_received (); auto keepalive_count = node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in); node1->network.send_keepalive (channel1); ASSERT_TIMELY (3s, node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in) > keepalive_count); ASSERT_EQ (node0->network.size (), 1); - auto timestamp_after_keepalive = channel2->get_last_packet_received (); + auto timestamp_after_keepalive = channel0->get_last_packet_received (); ASSERT_GT (timestamp_after_keepalive, timestamp_before_keepalive); } From 3f4da62fc6b0ab2407d6bf7d8a1efd88a0c6db33 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Fri, 14 May 2021 15:11:24 +0100 Subject: [PATCH 2/3] Fix unit test network.last_contacted (#3285) The unit test network.last_contacted has the following problems: * regular keepalive messages sent automatically can interfere with the test * the test should wait for the clock to move at least one tick to eliminate the possibility of the clock not having the time to move forward * the test waits for a the keepalive counter to be incremented and then expects the last_packet_received timestamp to be updated but the timestamp is updated after the counter is incremented, which is a race condition The solution: * send a second keepalive to handle the race condition between setting the timestamp and incrementing the counter * send a third keepalive to handle the case where an automatic keepalive message is already in flight * wait for the clock to move after reading it and before continuing Assumptions: We assume that there will be no more than 1 keelalive in flight. Of course, in theory, there could be multiple keepalives queued up but for the purposes of this test, we assume max 1 keepalive in flight. It is not an unreasonable assumption in this test case. --- nano/core_test/network.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/nano/core_test/network.cpp b/nano/core_test/network.cpp index e9ddcda35f..cfea058b2c 100644 --- a/nano/core_test/network.cpp +++ b/nano/core_test/network.cpp @@ -133,23 +133,38 @@ TEST (network, send_node_id_handshake_tcp) TEST (network, last_contacted) { nano::system system (1); + auto node0 = system.nodes[0]; ASSERT_EQ (0, node0->network.size ()); + nano::node_config node1_config (nano::get_available_port (), system.logging); node1_config.tcp_incoming_connections_max = 0; // Prevent ephemeral node1->node0 channel repacement with incoming connection auto node1 (std::make_shared (system.io_ctx, nano::unique_path (), node1_config, system.work)); node1->start (); system.nodes.push_back (node1); + auto channel1 = nano::establish_tcp (system, *node1, node0->network.endpoint ()); ASSERT_NE (nullptr, channel1); ASSERT_TIMELY (3s, node0->network.size () == 1); + + // channel0 is the other side of channel1, same connection different endpoint auto channel0 = node0->network.tcp_channels.find_node_id (node1->node_id.pub); ASSERT_NE (nullptr, channel0); - // Make sure last_contact gets updated on receiving a non-handshake message + + // capture the state before and ensure the clock ticks at least once auto timestamp_before_keepalive = channel0->get_last_packet_received (); auto keepalive_count = node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in); + ASSERT_TIMELY (3s, std::chrono::steady_clock::now () > timestamp_before_keepalive); + + // send 3 keepalives + // we need an extra keepalive to handle the race condition between the timestamp set and the counter increment + // and we need one more keepalive to handle the possibility that there is a keepalive already in flight when we start the crucial part of the test + // it is possible that there could be multiple keepalives in flight but we assume here that there will be no more than one in flight for the purposes of this test + node1->network.send_keepalive (channel1); node1->network.send_keepalive (channel1); - ASSERT_TIMELY (3s, node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in) > keepalive_count); + node1->network.send_keepalive (channel1); + + ASSERT_TIMELY (3s, node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in) >= keepalive_count + 3); ASSERT_EQ (node0->network.size (), 1); auto timestamp_after_keepalive = channel0->get_last_packet_received (); ASSERT_GT (timestamp_after_keepalive, timestamp_before_keepalive); From 39bedc574c205b94db3f5acc71e1a79d52c4d402 Mon Sep 17 00:00:00 2001 From: Dimitrios Siganos Date: Fri, 14 May 2021 15:14:14 +0100 Subject: [PATCH 3/3] Check that the endpoints are part of the same connection --- nano/core_test/network.cpp | 8 ++++++++ nano/node/socket.cpp | 5 +++++ nano/node/socket.hpp | 1 + 3 files changed, 14 insertions(+) diff --git a/nano/core_test/network.cpp b/nano/core_test/network.cpp index cfea058b2c..600fdac132 100644 --- a/nano/core_test/network.cpp +++ b/nano/core_test/network.cpp @@ -151,6 +151,14 @@ TEST (network, last_contacted) auto channel0 = node0->network.tcp_channels.find_node_id (node1->node_id.pub); ASSERT_NE (nullptr, channel0); + { + // check that the endpoints are part of the same connection + std::shared_ptr sock0 = channel0->socket.lock (); + std::shared_ptr sock1 = channel1->socket.lock (); + ASSERT_TRUE (sock0->local_endpoint () == sock1->remote_endpoint ()); + ASSERT_TRUE (sock1->local_endpoint () == sock0->remote_endpoint ()); + } + // capture the state before and ensure the clock ticks at least once auto timestamp_before_keepalive = channel0->get_last_packet_received (); auto keepalive_count = node0->stats.count (nano::stat::type::message, nano::stat::detail::keepalive, nano::stat::dir::in); diff --git a/nano/node/socket.cpp b/nano/node/socket.cpp index fe2b6760ee..ec43f0003a 100644 --- a/nano/node/socket.cpp +++ b/nano/node/socket.cpp @@ -196,6 +196,11 @@ nano::tcp_endpoint nano::socket::remote_endpoint () const return remote; } +nano::tcp_endpoint nano::socket::local_endpoint () const +{ + return tcp_socket.local_endpoint (); +} + nano::server_socket::server_socket (nano::node & node_a, boost::asio::ip::tcp::endpoint local_a, size_t max_connections_a) : socket{ node_a, std::chrono::seconds::max () }, acceptor{ node_a.io_ctx }, diff --git a/nano/node/socket.hpp b/nano/node/socket.hpp index e2002c6790..70c002b179 100644 --- a/nano/node/socket.hpp +++ b/nano/node/socket.hpp @@ -47,6 +47,7 @@ class socket : public std::enable_shared_from_this void close (); boost::asio::ip::tcp::endpoint remote_endpoint () const; + boost::asio::ip::tcp::endpoint local_endpoint () const; /** Returns true if the socket has timed out */ bool has_timed_out () const; /** This can be called to change the maximum idle time, e.g. based on the type of traffic detected. */