Skip to content
This repository was archived by the owner on Oct 28, 2021. It is now read-only.

Commit 9f2e689

Browse files
gumb0gumb0
authored
Merge pull request #5519 from ethereum/changed-endpoint
Handle receiving Discovery packets with changed endpoint
2 parents 3c6c4e5 + 28984b4 commit 9f2e689

File tree

4 files changed

+163
-41
lines changed

4 files changed

+163
-41
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,6 @@
1010
- Fixed: [#5512](https://github.com/ethereum/aleth/pull/5512) Calling `eth_call` without `from` argument.
1111
- Fixed: [#5502](https://github.com/ethereum/aleth/pull/5502) Fix Discovery terminating prematurely because of race condition.
1212
- Fixed: [#5452](https://github.com/ethereum/aleth/pull/5452) Correctly handle Discovery messages when the peer changes public key.
13+
- Fixed: [#5519](https://github.com/ethereum/aleth/pull/5519) Correctly handle Discovery messages with known public key but unknown endpoint.
1314

1415
[1.6.0]: https://github.com/ethereum/aleth/compare/v1.6.0-alpha.1...master

‎libp2p/NodeTable.cpp‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,8 @@ bool NodeTable::addNode(Node const& _node)
8383
DEV_GUARDED(x_nodes)
8484
{
8585
auto const it = m_allNodes.find(_node.id);
86-
needToPing = (it == m_allNodes.end() || !it->second->hasValidEndpointProof());
86+
needToPing = (it == m_allNodes.end() || it->second->endpoint() != _node.endpoint ||
87+
!it->second->hasValidEndpointProof());
8788
}
8889

8990
if (needToPing)
@@ -115,7 +116,7 @@ bool NodeTable::addKnownNode(
115116
if (entry->hasValidEndpointProof())
116117
{
117118
LOG(m_logger) << "Known " << _node;
118-
noteActiveNode(move(entry), _node.endpoint);
119+
noteActiveNode(move(entry));
119120
}
120121
else
121122
{
@@ -355,7 +356,7 @@ void NodeTable::evict(NodeEntry const& _leastSeen, shared_ptr<NodeEntry> _replac
355356
m_nodeEventHandler->appendEvent(_leastSeen.id(), NodeEntryScheduledForEviction);
356357
}
357358

358-
void NodeTable::noteActiveNode(shared_ptr<NodeEntry> _nodeEntry, bi::udp::endpoint const& _endpoint)
359+
void NodeTable::noteActiveNode(shared_ptr<NodeEntry> _nodeEntry)
359360
{
360361
assert(_nodeEntry);
361362

@@ -364,7 +365,7 @@ void NodeTable::noteActiveNode(shared_ptr<NodeEntry> _nodeEntry, bi::udp::endpoi
364365
LOG(m_logger) << "Skipping making self active.";
365366
return;
366367
}
367-
if (!isAllowedEndpoint(NodeIPEndpoint(_endpoint.address(), _endpoint.port(), _endpoint.port())))
368+
if (!isAllowedEndpoint(_nodeEntry->endpoint()))
368369
{
369370
LOG(m_logger) << "Skipping making node with unallowed endpoint active. Node "
370371
<< _nodeEntry->node;
@@ -375,10 +376,6 @@ void NodeTable::noteActiveNode(shared_ptr<NodeEntry> _nodeEntry, bi::udp::endpoi
375376
return;
376377

377378
LOG(m_logger) << "Active node " << _nodeEntry->node;
378-
// TODO: don't activate in case endpoint has changed
379-
_nodeEntry->node.endpoint.setAddress(_endpoint.address());
380-
_nodeEntry->node.endpoint.setUdpPort(_endpoint.port());
381-
382379

383380
shared_ptr<NodeEntry> nodeToEvict;
384381
{
@@ -512,6 +509,10 @@ void NodeTable::onPacketReceived(
512509
sourceNodeEntry = it->second;
513510
sourceNodeEntry->lastPongReceivedTime =
514511
RLPXDatagramFace::secondsSinceEpoch();
512+
513+
if (sourceNodeEntry->endpoint() != _from)
514+
sourceNodeEntry->node.endpoint = NodeIPEndpoint{
515+
_from.address(), _from.port(), nodeValidation.tcpPort};
515516
}
516517
}
517518

@@ -537,6 +538,12 @@ void NodeTable::onPacketReceived(
537538
<< ") not found in node table. Ignoring Neighbours packet.";
538539
return;
539540
}
541+
if (sourceNodeEntry->endpoint() != _from)
542+
{
543+
LOG(m_logger) << "Neighbours packet from unexpected endpoint " << _from
544+
<< " instead of " << sourceNodeEntry->endpoint();
545+
return;
546+
}
540547

541548
auto const& in = dynamic_cast<Neighbours const&>(*packet);
542549

@@ -570,6 +577,12 @@ void NodeTable::onPacketReceived(
570577
<< ") not found in node table. Ignoring FindNode request.";
571578
return;
572579
}
580+
if (sourceNodeEntry->endpoint() != _from)
581+
{
582+
LOG(m_logger) << "FindNode packet from unexpected endpoint " << _from
583+
<< " instead of " << sourceNodeEntry->endpoint();
584+
return;
585+
}
573586
if (!sourceNodeEntry->lastPongReceivedTime)
574587
{
575588
LOG(m_logger) << "Unexpected FindNode packet! Endpoint proof hasn't been performed yet.";
@@ -626,7 +639,7 @@ void NodeTable::onPacketReceived(
626639
}
627640

628641
if (sourceNodeEntry)
629-
noteActiveNode(move(sourceNodeEntry), _from);
642+
noteActiveNode(move(sourceNodeEntry));
630643
}
631644
catch (exception const& _e)
632645
{
@@ -710,7 +723,7 @@ void NodeTable::doHandleTimeouts()
710723

711724
// activate replacement nodes and put them into buckets
712725
for (auto const& n : nodesToActivate)
713-
noteActiveNode(n, n->endpoint());
726+
noteActiveNode(n);
714727

715728
doHandleTimeouts();
716729
});

‎libp2p/NodeTable.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ class NodeTable : UDPSocketEvents
261261

262262
/// Called whenever activity is received from a node in order to maintain node table. Only
263263
/// called for nodes for which we've completed an endpoint proof.
264-
void noteActiveNode(std::shared_ptr<NodeEntry> _nodeEntry, bi::udp::endpoint const& _endpoint);
264+
void noteActiveNode(std::shared_ptr<NodeEntry> _nodeEntry);
265265

266266
/// Used to drop node when timeout occurs or when evict() result is to keep previous node.
267267
void dropNode(std::shared_ptr<NodeEntry> _n);

‎test/unittests/libp2p/net.cpp‎

Lines changed: 138 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ struct TestNodeTable: public NodeTable
7979
auto entry = make_shared<NodeEntry>(m_hostNodeID, n.first,
8080
NodeIPEndpoint(ourIp, n.second, n.second),
8181
RLPXDatagramFace::secondsSinceEpoch(), RLPXDatagramFace::secondsSinceEpoch());
82-
noteActiveNode(move(entry), bi::udp::endpoint(ourIp, n.second));
82+
noteActiveNode(move(entry));
8383
}
8484
else
8585
break;
@@ -100,7 +100,7 @@ struct TestNodeTable: public NodeTable
100100
NodeIPEndpoint(ourIp, testNode->second, testNode->second),
101101
RLPXDatagramFace::secondsSinceEpoch(), RLPXDatagramFace::secondsSinceEpoch()));
102102
auto distance = node->distance;
103-
noteActiveNode(move(node), bi::udp::endpoint(ourIp, testNode->second));
103+
noteActiveNode(move(node));
104104

105105
{
106106
Guard stateGuard(x_state);
@@ -138,7 +138,7 @@ struct TestNodeTable: public NodeTable
138138
auto entry = make_shared<NodeEntry>(m_hostNodeID, testNode->first,
139139
NodeIPEndpoint(ourIp, testNode->second, testNode->second),
140140
RLPXDatagramFace::secondsSinceEpoch(), RLPXDatagramFace::secondsSinceEpoch());
141-
noteActiveNode(move(entry), bi::udp::endpoint(ourIp, testNode->second));
141+
noteActiveNode(move(entry));
142142

143143
++testNode;
144144
}
@@ -500,7 +500,7 @@ BOOST_AUTO_TEST_CASE(noteActiveNodeUpdatesKnownNode)
500500
auto& nodeTable = nodeTableHost.nodeTable;
501501
auto knownNode = nodeTable->bucketFirstNode(bucketIndex);
502502

503-
nodeTable->noteActiveNode(knownNode, knownNode->endpoint());
503+
nodeTable->noteActiveNode(knownNode);
504504

505505
// check that node was moved to the back of the bucket
506506
BOOST_CHECK_NE(nodeTable->bucketFirstNode(bucketIndex), knownNode);
@@ -550,32 +550,6 @@ BOOST_AUTO_TEST_CASE(noteActiveNodeEvictsTheNodeWhenBucketIsFull)
550550
BOOST_CHECK_EQUAL(evicted->replacementNodeEntry->id(), newNodeId);
551551
}
552552

553-
BOOST_AUTO_TEST_CASE(noteActiveNodeReplacesNodeInFullBucketWhenEndpointChanged)
554-
{
555-
TestNodeTableHost nodeTableHost(512);
556-
int const bucketIndex = nodeTableHost.populateUntilBucketSize(16);
557-
BOOST_REQUIRE(bucketIndex >= 0);
558-
559-
auto& nodeTable = nodeTableHost.nodeTable;
560-
auto leastRecentlySeenNode = nodeTable->bucketFirstNode(bucketIndex);
561-
562-
// addNode will replace the node in the m_allNodes map, because it's the same id with enother
563-
// endpoint
564-
auto const port = randomPortNumber();
565-
NodeIPEndpoint newEndpoint{bi::address::from_string(c_localhostIp), port, port };
566-
nodeTable->noteActiveNode(leastRecentlySeenNode, newEndpoint);
567-
568-
// the bucket is still max size
569-
BOOST_CHECK_EQUAL(nodeTable->bucketSize(bucketIndex), 16);
570-
// least recently seen node removed
571-
BOOST_CHECK_NE(nodeTable->bucketFirstNode(bucketIndex)->id(), leastRecentlySeenNode->id());
572-
// but added as most recently seen with new endpoint
573-
auto mostRecentNodeEntry = nodeTable->bucketLastNode(bucketIndex);
574-
BOOST_CHECK_EQUAL(mostRecentNodeEntry->id(), leastRecentlySeenNode->id());
575-
BOOST_CHECK_EQUAL(mostRecentNodeEntry->endpoint().address(), newEndpoint.address());
576-
BOOST_CHECK_EQUAL(mostRecentNodeEntry->endpoint().udpPort(), newEndpoint.udpPort());
577-
}
578-
579553
BOOST_AUTO_TEST_CASE(unexpectedPong)
580554
{
581555
// NodeTable receiving PONG
@@ -1173,6 +1147,140 @@ BOOST_AUTO_TEST_CASE(addNodePingsNodeOnlyOnce)
11731147
BOOST_REQUIRE_EQUAL(sentPing->pingHash, sentPing2->pingHash);
11741148
}
11751149

1150+
class PacketsWithChangedEndpointFixture : public TestOutputHelperFixture
1151+
{
1152+
public:
1153+
PacketsWithChangedEndpointFixture()
1154+
{
1155+
nodeTableHost.start();
1156+
nodeSocketHost1.start();
1157+
nodePort1 = nodeSocketHost1.port;
1158+
nodeSocketHost2.start();
1159+
nodePort2 = nodeSocketHost2.port;
1160+
1161+
nodeEndpoint1 =
1162+
NodeIPEndpoint{bi::address::from_string(c_localhostIp), nodePort1, nodePort1};
1163+
nodeEndpoint2 =
1164+
NodeIPEndpoint{bi::address::from_string(c_localhostIp), nodePort2, nodePort2};
1165+
1166+
// add a node to node table, initiating PING
1167+
nodeTable->addNode(Node{nodePubKey, nodeEndpoint1});
1168+
1169+
// handle received PING
1170+
auto pingDataReceived = nodeSocketHost1.packetsReceived.pop(chrono::seconds(5));
1171+
auto pingDatagram =
1172+
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pingDataReceived));
1173+
BOOST_REQUIRE_EQUAL(pingDatagram->typeName(), "Ping");
1174+
auto ping = dynamic_cast<PingNode const&>(*pingDatagram);
1175+
1176+
// send PONG
1177+
Pong pong{nodeTable->m_hostNodeEndpoint};
1178+
pong.echo = ping.echo;
1179+
pong.sign(nodeKeyPair.secret());
1180+
nodeSocketHost1.socket->send(pong);
1181+
1182+
// wait for PONG to be received and handled
1183+
nodeTable->packetsReceived.pop(chrono::seconds(5));
1184+
1185+
nodeEntry = nodeTable->nodeEntry(nodePubKey);
1186+
}
1187+
1188+
TestNodeTableHost nodeTableHost{0};
1189+
shared_ptr<TestNodeTable>& nodeTable = nodeTableHost.nodeTable;
1190+
1191+
// socket representing initial peer node
1192+
TestUDPSocketHost nodeSocketHost1;
1193+
uint16_t nodePort1 = 0;
1194+
1195+
// socket representing peer with changed endpoint
1196+
TestUDPSocketHost nodeSocketHost2;
1197+
uint16_t nodePort2 = 0;
1198+
1199+
NodeIPEndpoint nodeEndpoint1;
1200+
NodeIPEndpoint nodeEndpoint2;
1201+
KeyPair nodeKeyPair = KeyPair::create();
1202+
NodeID nodePubKey = nodeKeyPair.pub();
1203+
1204+
shared_ptr<NodeEntry> nodeEntry;
1205+
};
1206+
1207+
BOOST_FIXTURE_TEST_SUITE(packetsWithChangedEndpointSuite, PacketsWithChangedEndpointFixture)
1208+
1209+
BOOST_AUTO_TEST_CASE(addNode)
1210+
{
1211+
// this should initiate Ping to endpoint 2
1212+
nodeTable->addNode(Node{nodePubKey, nodeEndpoint2});
1213+
1214+
// handle received PING
1215+
auto pingDataReceived = nodeSocketHost2.packetsReceived.pop();
1216+
auto pingDatagram =
1217+
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pingDataReceived));
1218+
BOOST_REQUIRE_EQUAL(pingDatagram->typeName(), "Ping");
1219+
auto ping = dynamic_cast<PingNode const&>(*pingDatagram);
1220+
1221+
// send PONG
1222+
Pong pong{nodeTable->m_hostNodeEndpoint};
1223+
pong.echo = ping.echo;
1224+
pong.sign(nodeKeyPair.secret());
1225+
nodeSocketHost2.socket->send(pong);
1226+
1227+
// wait for PONG to be received and handled
1228+
auto pongDataReceived = nodeTable->packetsReceived.pop(chrono::seconds(5));
1229+
auto pongDatagram =
1230+
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pongDataReceived));
1231+
BOOST_REQUIRE_EQUAL(pongDatagram->typeName(), "Pong");
1232+
1233+
BOOST_REQUIRE_EQUAL(nodeEntry->endpoint(), nodeEndpoint2);
1234+
}
1235+
1236+
BOOST_AUTO_TEST_CASE(findNode)
1237+
{
1238+
// Create and send the FindNode packet through endpoint 2
1239+
FindNode findNode{nodeTable->m_hostNodeEndpoint, KeyPair::create().pub() /* target */};
1240+
findNode.sign(nodeKeyPair.secret());
1241+
nodeSocketHost2.socket->send(findNode);
1242+
1243+
// Wait for FindNode to be received
1244+
auto findNodeDataReceived = nodeTable->packetsReceived.pop(chrono::seconds(5));
1245+
auto findNodeDatagram =
1246+
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(findNodeDataReceived));
1247+
BOOST_REQUIRE_EQUAL(findNodeDatagram->typeName(), "FindNode");
1248+
1249+
// Verify that no neighbours response is received
1250+
BOOST_CHECK_THROW(nodeSocketHost2.packetsReceived.pop(chrono::seconds(5)), WaitTimeout);
1251+
}
1252+
1253+
BOOST_AUTO_TEST_CASE(neighbours)
1254+
{
1255+
// Wait for FindNode arriving to endpoint 1
1256+
auto findNodeDataReceived = nodeSocketHost1.packetsReceived.pop(chrono::seconds(10));
1257+
auto findNodeDatagram =
1258+
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(findNodeDataReceived));
1259+
BOOST_REQUIRE_EQUAL(findNodeDatagram->typeName(), "FindNode");
1260+
auto findNode = dynamic_cast<FindNode const&>(*findNodeDatagram);
1261+
1262+
// send Neighbours through endpoint 2
1263+
NodeIPEndpoint neighbourEndpoint{boost::asio::ip::address::from_string("200.200.200.200"),
1264+
c_defaultListenPort, c_defaultListenPort};
1265+
vector<shared_ptr<NodeEntry>> nearest{
1266+
make_shared<NodeEntry>(nodeTable->m_hostNodeID, KeyPair::create().pub(), neighbourEndpoint,
1267+
RLPXDatagramFace::secondsSinceEpoch(), 0 /* pongSentTime */)};
1268+
Neighbours neighbours{nodeTable->m_hostNodeEndpoint, nearest};
1269+
neighbours.sign(nodeKeyPair.secret());
1270+
nodeSocketHost2.socket->send(neighbours);
1271+
1272+
// Wait for Neighbours to be received
1273+
auto neighboursDataReceived = nodeTable->packetsReceived.pop(chrono::seconds(5));
1274+
auto neighboursDatagram =
1275+
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(neighboursDataReceived));
1276+
BOOST_REQUIRE_EQUAL(neighboursDatagram->typeName(), "Neighbours");
1277+
1278+
// no Ping is sent to neighbour
1279+
auto sentPing = nodeTable->nodeValidation(neighbourEndpoint);
1280+
BOOST_REQUIRE(!sentPing.is_initialized());
1281+
}
1282+
1283+
BOOST_AUTO_TEST_SUITE_END()
11761284
BOOST_AUTO_TEST_SUITE_END()
11771285

11781286
BOOST_FIXTURE_TEST_SUITE(netTypes, TestOutputHelperFixture)

0 commit comments

Comments
 (0)