Fix last contacted (#3286)

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

@@ -133,25 +133,48 @@ 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<nano::node> (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);
-	auto channel2 = node0->network.tcp_channels.find_node_id (node1->node_id.pub);
-	ASSERT_NE (nullptr, channel2);
-	// Make sure last_contact gets updated on receiving a non-handshake message
-	auto timestamp_before_keepalive = channel2->get_last_packet_received ();
+
+	// 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);
+
+	{
+		// check that the endpoints are part of the same connection
+		std::shared_ptr<nano::socket> sock0 = channel0->socket.lock ();
+		std::shared_ptr<nano::socket> 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);
+	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);
-	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);
+	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 = channel2->get_last_packet_received ();
+	auto timestamp_after_keepalive = channel0->get_last_packet_received ();
 	ASSERT_GT (timestamp_after_keepalive, timestamp_before_keepalive);
 }
 

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

nano/node/socket.hpp

@@ -47,6 +47,7 @@ class socket : public std::enable_shared_from_this<nano::socket>
 
 	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. */