Merge pull request #1870 from subtly/discovery

Guard m_node.endpoint access and updates.

libp2p/Common.h

@@ -172,12 +172,13 @@ struct NodeIPEndpoint
 	/// Setting true causes isAllowed to return true for all addresses. (Used by test fixtures)
 	static bool test_allowLocal;
 
+	NodeIPEndpoint() = default;
 	NodeIPEndpoint(bi::address _addr, uint16_t _udp, uint16_t _tcp): address(_addr), udpPort(_udp), tcpPort(_tcp) {}
 	NodeIPEndpoint(RLP const& _r) { interpretRLP(_r); }
 
-	bi::address address;
-	uint16_t udpPort;
-	uint16_t tcpPort;
+	bi::address address = bi::address();
+	uint16_t udpPort = 0;
+	uint16_t tcpPort = 0;
 	
 	operator bi::udp::endpoint() const { return std::move(bi::udp::endpoint(address, udpPort)); }
 	operator bi::tcp::endpoint() const { return std::move(bi::tcp::endpoint(address, tcpPort)); }

libp2p/NodeTable.cpp

@@ -38,7 +38,7 @@ const char* NodeTableAllDetail::name() { return "=P="; }
 const char* NodeTableEgress::name() { return ">>P"; }
 const char* NodeTableIngress::name() { return "<<P"; }
 
-NodeEntry::NodeEntry(Node _src, Public _pubk, NodeIPEndpoint _gw): Node(_pubk, _gw), distance(NodeTable::distance(_src.id,_pubk)) {}
+NodeEntry::NodeEntry(NodeId const& _src, Public const& _pubk, NodeIPEndpoint const& _gw): Node(_pubk, _gw), distance(NodeTable::distance(_src, _pubk)) {}
 
 NodeTable::NodeTable(ba::io_service& _io, KeyPair const& _alias, NodeIPEndpoint const& _endpoint):
 	m_node(Node(_alias.pub(), _endpoint)),
@@ -79,7 +79,7 @@ shared_ptr<NodeEntry> NodeTable::addNode(Node const& _node, NodeRelation _relati
 {
 	if (_relation == Known)
 	{
-		shared_ptr<NodeEntry> ret(new NodeEntry(m_node, _node.id, _node.endpoint));
+		shared_ptr<NodeEntry> ret(new NodeEntry(m_node.id, _node.id, _node.endpoint));
 		ret->pending = false;
 		DEV_GUARDED(x_nodes)
 			m_nodes[_node.id] = ret;
@@ -93,11 +93,10 @@ shared_ptr<NodeEntry> NodeTable::addNode(Node const& _node, NodeRelation _relati
 	// ping address to recover nodeid if nodeid is empty
 	if (!_node.id)
 	{
-		clog(NodeTableConnect) << "Sending public key discovery Ping to" << (bi::udp::endpoint)_node.endpoint << "(Advertising:" << (bi::udp::endpoint)m_node.endpoint << ")";
-		{
-			Guard l(x_pubkDiscoverPings);
+		DEV_GUARDED(x_nodes)
+			clog(NodeTableConnect) << "Sending public key discovery Ping to" << (bi::udp::endpoint)_node.endpoint << "(Advertising:" << (bi::udp::endpoint)m_node.endpoint << ")";
+		DEV_GUARDED(x_pubkDiscoverPings);
 			m_pubkDiscoverPings[_node.endpoint.address] = std::chrono::steady_clock::now();
-		}
 		ping(_node.endpoint);
 		return move(shared_ptr<NodeEntry>());
 	}
@@ -106,7 +105,7 @@ shared_ptr<NodeEntry> NodeTable::addNode(Node const& _node, NodeRelation _relati
 		if (m_nodes.count(_node.id))
 			return m_nodes[_node.id];
 	
-	shared_ptr<NodeEntry> ret(new NodeEntry(m_node, _node.id, _node.endpoint));
+	shared_ptr<NodeEntry> ret(new NodeEntry(m_node.id, _node.id, _node.endpoint));
 	DEV_GUARDED(x_nodes)
 		m_nodes[_node.id] = ret;
 	clog(NodeTableConnect) << "addNode pending for" << _node.endpoint;
@@ -288,7 +287,10 @@ vector<shared_ptr<NodeEntry>> NodeTable::nearestNodeEntries(NodeId _target)
 
 void NodeTable::ping(NodeIPEndpoint _to) const
 {
-	PingNode p(m_node.endpoint, _to);
+	NodeIPEndpoint src;
+	DEV_GUARDED(x_nodes)
+		src = m_node.endpoint;
+	PingNode p(src, _to);
 	p.sign(m_secret);
 	m_socketPointer->send(p);
 }
@@ -466,9 +468,12 @@ void NodeTable::onReceived(UDPSocketFace*, bi::udp::endpoint const& _from, bytes
 				}
 				
 				// update our endpoint address and UDP port
-				if ((!m_node.endpoint || !m_node.endpoint.isAllowed()) && isPublicAddress(in.destination.address))
-					m_node.endpoint.address = in.destination.address;
-				m_node.endpoint.udpPort = in.destination.udpPort;
+				DEV_GUARDED(x_nodes)
+				{
+					if ((!m_node.endpoint || !m_node.endpoint.isAllowed()) && isPublicAddress(in.destination.address))
+						m_node.endpoint.address = in.destination.address;
+					m_node.endpoint.udpPort = in.destination.udpPort;
+				}
 				
 				clog(NodeTableConnect) << "PONG from " << nodeid << _from;
 				break;

libp2p/NodeTable.h

@@ -40,7 +40,7 @@ namespace p2p
  */
 struct NodeEntry: public Node
 {
-	NodeEntry(Node _src, Public _pubk, NodeIPEndpoint _gw);
+	NodeEntry(NodeId const& _src, Public const& _pubk, NodeIPEndpoint const& _gw);
 	unsigned const distance;	///< Node's distance (xor of _src as integer).
 	bool pending = true;		///< Node will be ignored until Pong is received
 };
@@ -203,7 +203,7 @@ private:
 	void ping(NodeEntry* _n) const;
 
 	/// Returns center node entry which describes this node and used with dist() to calculate xor metric for node table nodes.
-	NodeEntry center() const { return NodeEntry(m_node, m_node.publicKey(), m_node.endpoint); }
+	NodeEntry center() const { return NodeEntry(m_node.id, m_node.publicKey(), m_node.endpoint); }
 
 	/// Used by asynchronous operations to return NodeEntry which is active and managed by node table.
 	std::shared_ptr<NodeEntry> nodeEntry(NodeId _id);
@@ -247,7 +247,7 @@ private:
 
 	std::unique_ptr<NodeTableEventHandler> m_nodeEventHandler;		///< Event handler for node events.
 
-	Node m_node;													///< This node.
+	Node m_node;													///< This node. LOCK x_state if endpoint access or mutation is required. Do not modify id.
 	Secret m_secret;												///< This nodes secret key.
 
 	mutable Mutex x_nodes;											///< LOCK x_state first if both locks are required. Mutable for thread-safe copy in nodes() const.

test/libp2p/net.cpp

@@ -99,7 +99,7 @@ struct TestNodeTable: public NodeTable
 				// manually add node for test
 				{
 					Guard ln(x_nodes);
-					shared_ptr<NodeEntry> node(new NodeEntry(m_node, n.first.pub(), NodeIPEndpoint(ourIp, n.second, n.second)));
+					shared_ptr<NodeEntry> node(new NodeEntry(m_node.id, n.first.pub(), NodeIPEndpoint(ourIp, n.second, n.second)));
 					node->pending = false;
 					m_nodes[node->id] = node;
 				}