diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c1e05e7e9d..d54fdcd5a52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,5 +10,6 @@ - Fixed: [#5512](https://github.com/ethereum/aleth/pull/5512) Calling `eth_call` without `from` argument. - Fixed: [#5502](https://github.com/ethereum/aleth/pull/5502) Fix Discovery terminating prematurely because of race condition. - Fixed: [#5452](https://github.com/ethereum/aleth/pull/5452) Correctly handle Discovery messages when the peer changes public key. +- Fixed: [#5519](https://github.com/ethereum/aleth/pull/5519) Correctly handle Discovery messages with known public key but unknown endpoint. [1.6.0]: https://github.com/ethereum/aleth/compare/v1.6.0-alpha.1...master \ No newline at end of file diff --git a/libp2p/NodeTable.cpp b/libp2p/NodeTable.cpp index f2c498dc8b4..0917b40460e 100644 --- a/libp2p/NodeTable.cpp +++ b/libp2p/NodeTable.cpp @@ -83,7 +83,8 @@ bool NodeTable::addNode(Node const& _node) DEV_GUARDED(x_nodes) { auto const it = m_allNodes.find(_node.id); - needToPing = (it == m_allNodes.end() || !it->second->hasValidEndpointProof()); + needToPing = (it == m_allNodes.end() || it->second->endpoint() != _node.endpoint || + !it->second->hasValidEndpointProof()); } if (needToPing) @@ -115,7 +116,7 @@ bool NodeTable::addKnownNode( if (entry->hasValidEndpointProof()) { LOG(m_logger) << "Known " << _node; - noteActiveNode(move(entry), _node.endpoint); + noteActiveNode(move(entry)); } else { @@ -355,7 +356,7 @@ void NodeTable::evict(NodeEntry const& _leastSeen, shared_ptr _replac m_nodeEventHandler->appendEvent(_leastSeen.id(), NodeEntryScheduledForEviction); } -void NodeTable::noteActiveNode(shared_ptr _nodeEntry, bi::udp::endpoint const& _endpoint) +void NodeTable::noteActiveNode(shared_ptr _nodeEntry) { assert(_nodeEntry); @@ -364,7 +365,7 @@ void NodeTable::noteActiveNode(shared_ptr _nodeEntry, bi::udp::endpoi LOG(m_logger) << "Skipping making self active."; return; } - if (!isAllowedEndpoint(NodeIPEndpoint(_endpoint.address(), _endpoint.port(), _endpoint.port()))) + if (!isAllowedEndpoint(_nodeEntry->endpoint())) { LOG(m_logger) << "Skipping making node with unallowed endpoint active. Node " << _nodeEntry->node; @@ -375,10 +376,6 @@ void NodeTable::noteActiveNode(shared_ptr _nodeEntry, bi::udp::endpoi return; LOG(m_logger) << "Active node " << _nodeEntry->node; - // TODO: don't activate in case endpoint has changed - _nodeEntry->node.endpoint.setAddress(_endpoint.address()); - _nodeEntry->node.endpoint.setUdpPort(_endpoint.port()); - shared_ptr nodeToEvict; { @@ -512,6 +509,10 @@ void NodeTable::onPacketReceived( sourceNodeEntry = it->second; sourceNodeEntry->lastPongReceivedTime = RLPXDatagramFace::secondsSinceEpoch(); + + if (sourceNodeEntry->endpoint() != _from) + sourceNodeEntry->node.endpoint = NodeIPEndpoint{ + _from.address(), _from.port(), nodeValidation.tcpPort}; } } @@ -537,6 +538,12 @@ void NodeTable::onPacketReceived( << ") not found in node table. Ignoring Neighbours packet."; return; } + if (sourceNodeEntry->endpoint() != _from) + { + LOG(m_logger) << "Neighbours packet from unexpected endpoint " << _from + << " instead of " << sourceNodeEntry->endpoint(); + return; + } auto const& in = dynamic_cast(*packet); @@ -570,6 +577,12 @@ void NodeTable::onPacketReceived( << ") not found in node table. Ignoring FindNode request."; return; } + if (sourceNodeEntry->endpoint() != _from) + { + LOG(m_logger) << "FindNode packet from unexpected endpoint " << _from + << " instead of " << sourceNodeEntry->endpoint(); + return; + } if (!sourceNodeEntry->lastPongReceivedTime) { LOG(m_logger) << "Unexpected FindNode packet! Endpoint proof hasn't been performed yet."; @@ -626,7 +639,7 @@ void NodeTable::onPacketReceived( } if (sourceNodeEntry) - noteActiveNode(move(sourceNodeEntry), _from); + noteActiveNode(move(sourceNodeEntry)); } catch (exception const& _e) { @@ -710,7 +723,7 @@ void NodeTable::doHandleTimeouts() // activate replacement nodes and put them into buckets for (auto const& n : nodesToActivate) - noteActiveNode(n, n->endpoint()); + noteActiveNode(n); doHandleTimeouts(); }); diff --git a/libp2p/NodeTable.h b/libp2p/NodeTable.h index 08e01f5d071..0725e1ea2bd 100644 --- a/libp2p/NodeTable.h +++ b/libp2p/NodeTable.h @@ -261,7 +261,7 @@ class NodeTable : UDPSocketEvents /// Called whenever activity is received from a node in order to maintain node table. Only /// called for nodes for which we've completed an endpoint proof. - void noteActiveNode(std::shared_ptr _nodeEntry, bi::udp::endpoint const& _endpoint); + void noteActiveNode(std::shared_ptr _nodeEntry); /// Used to drop node when timeout occurs or when evict() result is to keep previous node. void dropNode(std::shared_ptr _n); diff --git a/test/unittests/libp2p/net.cpp b/test/unittests/libp2p/net.cpp index 03301da9f71..b62bc591861 100644 --- a/test/unittests/libp2p/net.cpp +++ b/test/unittests/libp2p/net.cpp @@ -79,7 +79,7 @@ struct TestNodeTable: public NodeTable auto entry = make_shared(m_hostNodeID, n.first, NodeIPEndpoint(ourIp, n.second, n.second), RLPXDatagramFace::secondsSinceEpoch(), RLPXDatagramFace::secondsSinceEpoch()); - noteActiveNode(move(entry), bi::udp::endpoint(ourIp, n.second)); + noteActiveNode(move(entry)); } else break; @@ -100,7 +100,7 @@ struct TestNodeTable: public NodeTable NodeIPEndpoint(ourIp, testNode->second, testNode->second), RLPXDatagramFace::secondsSinceEpoch(), RLPXDatagramFace::secondsSinceEpoch())); auto distance = node->distance; - noteActiveNode(move(node), bi::udp::endpoint(ourIp, testNode->second)); + noteActiveNode(move(node)); { Guard stateGuard(x_state); @@ -138,7 +138,7 @@ struct TestNodeTable: public NodeTable auto entry = make_shared(m_hostNodeID, testNode->first, NodeIPEndpoint(ourIp, testNode->second, testNode->second), RLPXDatagramFace::secondsSinceEpoch(), RLPXDatagramFace::secondsSinceEpoch()); - noteActiveNode(move(entry), bi::udp::endpoint(ourIp, testNode->second)); + noteActiveNode(move(entry)); ++testNode; } @@ -500,7 +500,7 @@ BOOST_AUTO_TEST_CASE(noteActiveNodeUpdatesKnownNode) auto& nodeTable = nodeTableHost.nodeTable; auto knownNode = nodeTable->bucketFirstNode(bucketIndex); - nodeTable->noteActiveNode(knownNode, knownNode->endpoint()); + nodeTable->noteActiveNode(knownNode); // check that node was moved to the back of the bucket BOOST_CHECK_NE(nodeTable->bucketFirstNode(bucketIndex), knownNode); @@ -550,32 +550,6 @@ BOOST_AUTO_TEST_CASE(noteActiveNodeEvictsTheNodeWhenBucketIsFull) BOOST_CHECK_EQUAL(evicted->replacementNodeEntry->id(), newNodeId); } -BOOST_AUTO_TEST_CASE(noteActiveNodeReplacesNodeInFullBucketWhenEndpointChanged) -{ - TestNodeTableHost nodeTableHost(512); - int const bucketIndex = nodeTableHost.populateUntilBucketSize(16); - BOOST_REQUIRE(bucketIndex >= 0); - - auto& nodeTable = nodeTableHost.nodeTable; - auto leastRecentlySeenNode = nodeTable->bucketFirstNode(bucketIndex); - - // addNode will replace the node in the m_allNodes map, because it's the same id with enother - // endpoint - auto const port = randomPortNumber(); - NodeIPEndpoint newEndpoint{bi::address::from_string(c_localhostIp), port, port }; - nodeTable->noteActiveNode(leastRecentlySeenNode, newEndpoint); - - // the bucket is still max size - BOOST_CHECK_EQUAL(nodeTable->bucketSize(bucketIndex), 16); - // least recently seen node removed - BOOST_CHECK_NE(nodeTable->bucketFirstNode(bucketIndex)->id(), leastRecentlySeenNode->id()); - // but added as most recently seen with new endpoint - auto mostRecentNodeEntry = nodeTable->bucketLastNode(bucketIndex); - BOOST_CHECK_EQUAL(mostRecentNodeEntry->id(), leastRecentlySeenNode->id()); - BOOST_CHECK_EQUAL(mostRecentNodeEntry->endpoint().address(), newEndpoint.address()); - BOOST_CHECK_EQUAL(mostRecentNodeEntry->endpoint().udpPort(), newEndpoint.udpPort()); -} - BOOST_AUTO_TEST_CASE(unexpectedPong) { // NodeTable receiving PONG @@ -1173,6 +1147,140 @@ BOOST_AUTO_TEST_CASE(addNodePingsNodeOnlyOnce) BOOST_REQUIRE_EQUAL(sentPing->pingHash, sentPing2->pingHash); } +class PacketsWithChangedEndpointFixture : public TestOutputHelperFixture +{ +public: + PacketsWithChangedEndpointFixture() + { + nodeTableHost.start(); + nodeSocketHost1.start(); + nodePort1 = nodeSocketHost1.port; + nodeSocketHost2.start(); + nodePort2 = nodeSocketHost2.port; + + nodeEndpoint1 = + NodeIPEndpoint{bi::address::from_string(c_localhostIp), nodePort1, nodePort1}; + nodeEndpoint2 = + NodeIPEndpoint{bi::address::from_string(c_localhostIp), nodePort2, nodePort2}; + + // add a node to node table, initiating PING + nodeTable->addNode(Node{nodePubKey, nodeEndpoint1}); + + // handle received PING + auto pingDataReceived = nodeSocketHost1.packetsReceived.pop(chrono::seconds(5)); + auto pingDatagram = + DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pingDataReceived)); + BOOST_REQUIRE_EQUAL(pingDatagram->typeName(), "Ping"); + auto ping = dynamic_cast(*pingDatagram); + + // send PONG + Pong pong{nodeTable->m_hostNodeEndpoint}; + pong.echo = ping.echo; + pong.sign(nodeKeyPair.secret()); + nodeSocketHost1.socket->send(pong); + + // wait for PONG to be received and handled + nodeTable->packetsReceived.pop(chrono::seconds(5)); + + nodeEntry = nodeTable->nodeEntry(nodePubKey); + } + + TestNodeTableHost nodeTableHost{0}; + shared_ptr& nodeTable = nodeTableHost.nodeTable; + + // socket representing initial peer node + TestUDPSocketHost nodeSocketHost1; + uint16_t nodePort1 = 0; + + // socket representing peer with changed endpoint + TestUDPSocketHost nodeSocketHost2; + uint16_t nodePort2 = 0; + + NodeIPEndpoint nodeEndpoint1; + NodeIPEndpoint nodeEndpoint2; + KeyPair nodeKeyPair = KeyPair::create(); + NodeID nodePubKey = nodeKeyPair.pub(); + + shared_ptr nodeEntry; +}; + +BOOST_FIXTURE_TEST_SUITE(packetsWithChangedEndpointSuite, PacketsWithChangedEndpointFixture) + +BOOST_AUTO_TEST_CASE(addNode) +{ + // this should initiate Ping to endpoint 2 + nodeTable->addNode(Node{nodePubKey, nodeEndpoint2}); + + // handle received PING + auto pingDataReceived = nodeSocketHost2.packetsReceived.pop(); + auto pingDatagram = + DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pingDataReceived)); + BOOST_REQUIRE_EQUAL(pingDatagram->typeName(), "Ping"); + auto ping = dynamic_cast(*pingDatagram); + + // send PONG + Pong pong{nodeTable->m_hostNodeEndpoint}; + pong.echo = ping.echo; + pong.sign(nodeKeyPair.secret()); + nodeSocketHost2.socket->send(pong); + + // wait for PONG to be received and handled + auto pongDataReceived = nodeTable->packetsReceived.pop(chrono::seconds(5)); + auto pongDatagram = + DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pongDataReceived)); + BOOST_REQUIRE_EQUAL(pongDatagram->typeName(), "Pong"); + + BOOST_REQUIRE_EQUAL(nodeEntry->endpoint(), nodeEndpoint2); +} + +BOOST_AUTO_TEST_CASE(findNode) +{ + // Create and send the FindNode packet through endpoint 2 + FindNode findNode{nodeTable->m_hostNodeEndpoint, KeyPair::create().pub() /* target */}; + findNode.sign(nodeKeyPair.secret()); + nodeSocketHost2.socket->send(findNode); + + // Wait for FindNode to be received + auto findNodeDataReceived = nodeTable->packetsReceived.pop(chrono::seconds(5)); + auto findNodeDatagram = + DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(findNodeDataReceived)); + BOOST_REQUIRE_EQUAL(findNodeDatagram->typeName(), "FindNode"); + + // Verify that no neighbours response is received + BOOST_CHECK_THROW(nodeSocketHost2.packetsReceived.pop(chrono::seconds(5)), WaitTimeout); +} + +BOOST_AUTO_TEST_CASE(neighbours) +{ + // Wait for FindNode arriving to endpoint 1 + auto findNodeDataReceived = nodeSocketHost1.packetsReceived.pop(chrono::seconds(10)); + auto findNodeDatagram = + DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(findNodeDataReceived)); + BOOST_REQUIRE_EQUAL(findNodeDatagram->typeName(), "FindNode"); + auto findNode = dynamic_cast(*findNodeDatagram); + + // send Neighbours through endpoint 2 + NodeIPEndpoint neighbourEndpoint{boost::asio::ip::address::from_string("200.200.200.200"), + c_defaultListenPort, c_defaultListenPort}; + vector> nearest{ + make_shared(nodeTable->m_hostNodeID, KeyPair::create().pub(), neighbourEndpoint, + RLPXDatagramFace::secondsSinceEpoch(), 0 /* pongSentTime */)}; + Neighbours neighbours{nodeTable->m_hostNodeEndpoint, nearest}; + neighbours.sign(nodeKeyPair.secret()); + nodeSocketHost2.socket->send(neighbours); + + // Wait for Neighbours to be received + auto neighboursDataReceived = nodeTable->packetsReceived.pop(chrono::seconds(5)); + auto neighboursDatagram = + DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(neighboursDataReceived)); + BOOST_REQUIRE_EQUAL(neighboursDatagram->typeName(), "Neighbours"); + + // no Ping is sent to neighbour + auto sentPing = nodeTable->nodeValidation(neighbourEndpoint); + BOOST_REQUIRE(!sentPing.is_initialized()); +} + +BOOST_AUTO_TEST_SUITE_END() BOOST_AUTO_TEST_SUITE_END() BOOST_FIXTURE_TEST_SUITE(netTypes, TestOutputHelperFixture)