Handle receiving Discovery packets with changed endpoint #5519

gumb0 · 2019-03-13T15:01:20Z

codecov-io · 2019-03-14T16:09:17Z

Codecov Report

Merging #5519 into master will increase coverage by 0.08%.
The diff coverage is 95.29%.

@@            Coverage Diff             @@
##           master    #5519      +/-   ##
==========================================
+ Coverage    61.8%   61.89%   +0.08%     
==========================================
  Files         344      344              
  Lines       28694    28753      +59     
  Branches     3261     3264       +3     
==========================================
+ Hits        17735    17797      +62     
+ Misses       9797     9789       -8     
- Partials     1162     1167       +5

gumb0 · 2019-03-18T15:57:40Z

libp2p/NodeTable.cpp

@@ -375,10 +376,6 @@ void NodeTable::noteActiveNode(shared_ptr<NodeEntry> _nodeEntry, bi::udp::endpoi
        return;

    LOG(m_logger) << "Active node " << *_nodeEntry;
-    // TODO: don't activate in case endpoint has changed
-    _nodeEntry->endpoint.setAddress(_endpoint.address());


This part is basically moved to Pong handler (lines 513-516), because we should trust changed endpoint only after valid Pong

halfalicious

Some minor suggestions and a few questions

halfalicious · 2019-03-19T15:58:36Z

libp2p/NodeTable.cpp

@@ -502,6 +499,7 @@ void NodeTable::onPacketReceived(
                // create or update nodeEntry with new Pong received time
                DEV_GUARDED(x_nodes)
                {
+                    auto const& sourceId = pong.sourceid;


We already get the sourceId from the pong packet:

aleth/libp2p/NodeTable.cpp

Lines 489 to 490 in 0886a34

// in case the node answers with new NodeID, drop the record with the old NodeID

auto const& sourceId = pong.sourceid;

halfalicious · 2019-03-20T03:19:33Z

libp2p/NodeTable.cpp

@@ -512,6 +510,10 @@ void NodeTable::onPacketReceived(
                        sourceNodeEntry = it->second;
                        sourceNodeEntry->lastPongReceivedTime =
                            RLPXDatagramFace::secondsSinceEpoch();
+


I know this is good practice but do you know if we ever actually hit this case? It seems like we wouldn't given that pongs have to be received within 1 minute of the ping being sent to be considered valid?

Not sure how it is related to Pong timeout, this is the case when there is a record in the bucket of the node table (maybe added long ago), but we receive a valid Pong with the same NodeID but from another endpoint.

halfalicious · 2019-03-20T03:20:08Z

libp2p/NodeTable.cpp

@@ -537,6 +539,11 @@ void NodeTable::onPacketReceived(
                                  << ") not found in node table. Ignoring Neighbours packet.";
                    return;
                }
+                if (sourceNodeEntry->endpoint != _from)
+                {
+                    LOG(m_logger) << "Neighbours packet from unexpected endpoint.";


Should we log both endpoints here?

halfalicious · 2019-03-20T03:20:14Z

libp2p/NodeTable.cpp

@@ -570,6 +577,11 @@ void NodeTable::onPacketReceived(
                                  << ") not found in node table. Ignoring FindNode request.";
                    return;
                }
+                if (sourceNodeEntry->endpoint != _from)
+                {
+                    LOG(m_logger) << "FindNode packet from unexpected endpoint.";


Should we log both endpoints here?

test/unittests/libp2p/net.cpp

halfalicious · 2019-03-20T03:40:24Z

test/unittests/libp2p/net.cpp

+    auto findNode = dynamic_cast<FindNode const&>(*findNodeDatagram);
+
+    // send Neighbours through endpoint 2
+    // TODO fill nearest with one node


TODO? I think you already do this?

halfalicious · 2019-03-20T03:42:13Z

test/unittests/libp2p/net.cpp

+    NodeIPEndpoint neighbourEndpoint{boost::asio::ip::address::from_string("200.200.200.200"),
+        c_defaultListenPort, c_defaultListenPort};
+    vector<shared_ptr<NodeEntry>> nearest{make_shared<NodeEntry>(nodeTable->m_hostNodeID,
+        KeyPair::create().pub(), neighbourEndpoint, RLPXDatagramFace::secondsSinceEpoch(), 0)};


Would be nice to comment the 0, especially since there are a lot of ctor args

halfalicious · 2019-03-20T03:42:22Z

test/unittests/libp2p/net.cpp

+        c_defaultListenPort, c_defaultListenPort};
+    vector<shared_ptr<NodeEntry>> nearest{make_shared<NodeEntry>(nodeTable->m_hostNodeID,
+        KeyPair::create().pub(), neighbourEndpoint, RLPXDatagramFace::secondsSinceEpoch(), 0)};
+    Neighbours neighbours(nodeTable->m_hostNodeEndpoint, nearest);


(Nit) {} initializers

halfalicious · 2019-03-20T03:42:40Z

test/unittests/libp2p/net.cpp

+    nodeSocketHost2.socket->send(neighbours);
+
+    // Wait for Neighbours to be received
+    nodeTable->packetsReceived.pop(chrono::seconds(5));


Should we validate the received packet?

gumb0 · 2019-03-25T13:59:12Z

Addresses review issues, rebased and added CHANGELOG entry.

gumb0 added the in progress label Mar 13, 2019

gumb0 force-pushed the changed-endpoint branch from f246aef to e757bbb Compare March 14, 2019 16:09

gumb0 force-pushed the changed-endpoint branch 3 times, most recently from 7a9d46b to 0886a34 Compare March 18, 2019 15:47

gumb0 commented Mar 18, 2019

View reviewed changes

gumb0 requested review from halfalicious and chfast March 18, 2019 15:58

gumb0 removed the in progress label Mar 18, 2019

halfalicious suggested changes Mar 20, 2019

View reviewed changes

gumb0 force-pushed the changed-endpoint branch from 7f5c400 to 8163800 Compare March 25, 2019 13:58

gumb0 requested a review from halfalicious March 25, 2019 13:59

chfast approved these changes Mar 25, 2019

View reviewed changes

halfalicious approved these changes Mar 26, 2019

View reviewed changes

gumb0 force-pushed the changed-endpoint branch from 8163800 to 2f589f7 Compare March 26, 2019 10:19

gumb0 added 5 commits March 26, 2019 11:32

m_sentPings maps pinged udp::endpoint to NodeValidation

9343c0b

Check whether node's endpoint changed when handling discovery packets

a1d3b8d

Minor improvements

5719e7d

Add changelog message

bca8cb6

Fix build after bad rebase

28984b4

gumb0 force-pushed the changed-endpoint branch from 2f589f7 to 28984b4 Compare March 26, 2019 10:48

gumb0 merged commit 9f2e689 into master Mar 26, 2019

gumb0 deleted the changed-endpoint branch March 26, 2019 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle receiving Discovery packets with changed endpoint #5519

Handle receiving Discovery packets with changed endpoint #5519

gumb0 commented Mar 13, 2019

codecov-io commented Mar 14, 2019 •

edited

Loading

gumb0 Mar 18, 2019 •

edited

Loading

halfalicious left a comment •

edited

Loading

halfalicious Mar 19, 2019

halfalicious Mar 20, 2019

gumb0 Mar 25, 2019

halfalicious Mar 20, 2019

halfalicious Mar 20, 2019

halfalicious Mar 20, 2019

halfalicious Mar 20, 2019

halfalicious Mar 20, 2019

halfalicious Mar 20, 2019

gumb0 commented Mar 25, 2019

	// in case the node answers with new NodeID, drop the record with the old NodeID
	auto const& sourceId = pong.sourceid;

Handle receiving Discovery packets with changed endpoint #5519

Handle receiving Discovery packets with changed endpoint #5519

Conversation

gumb0 commented Mar 13, 2019

codecov-io commented Mar 14, 2019 • edited Loading

Codecov Report

gumb0 Mar 18, 2019 • edited Loading

Choose a reason for hiding this comment

halfalicious left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Mar 25, 2019

codecov-io commented Mar 14, 2019 •

edited

Loading

gumb0 Mar 18, 2019 •

edited

Loading

halfalicious left a comment •

edited

Loading