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

Handle receiving a Pong with changed NodeID #5452

Merged
merged 6 commits into from Mar 18, 2019
Merged

Handle receiving a Pong with changed NodeID #5452

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
258c891
Test for receiving a Pong with changed NodeID
gumb0 Jan 24, 2019
5373edf
m_sentPings maps pinged udp::endpoint to NodeValidation
gumb0 Mar 11, 2019
1c03c6f
When the node sends Pong with changed NodeID, drop the record with ol…
gumb0 Mar 12, 2019
85952c9
Add log message for changed NodeID
gumb0 Mar 12, 2019
68a3b59
Minor coding style issues fixed
gumb0 Mar 15, 2019
80d9071
Rename NodeValidation::pingSendTime to pingSentTime
gumb0 Mar 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 22 additions & 11 deletions libp2p/NodeTable.cpp
View file
Open in desktop
Expand Up @@ -322,7 +322,7 @@ void NodeTable::ping(Node const& _node, shared_ptr<NodeEntry> _replacementNodeEn
return;

// Don't sent Ping if one is already sent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your changes, but minor grammar issue (1st sent should be send)

if (contains(m_sentPings, _node.id))
if (m_sentPings.find(_node.endpoint) != m_sentPings.end())
{
LOG(m_logger) << "Ignoring request to ping " << _node << ", because it's already pinged";
return;
Expand All @@ -334,8 +334,9 @@ void NodeTable::ping(Node const& _node, shared_ptr<NodeEntry> _replacementNodeEn
LOG(m_logger) << p.typeName() << " to " << _node;
m_socket->send(p);

m_sentPings[_node.id] = {
_node.endpoint, chrono::steady_clock::now(), pingHash, move(_replacementNodeEntry)};
NodeValidation const validation(_node.id, _node.endpoint.tcpPort(), chrono::steady_clock::now(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use braces for initialization

pingHash, _replacementNodeEntry);
m_sentPings.insert({_node.endpoint, validation});
}

void NodeTable::schedulePing(Node const& _node)
Expand Down Expand Up @@ -470,32 +471,42 @@ void NodeTable::onPacketReceived(
{
case Pong::type:
{
auto const& pong = dynamic_cast<Pong const&>(*packet);
auto const& sourceId = pong.sourceid;

// validate pong
auto const sentPing = m_sentPings.find(sourceId);
auto const sentPing = m_sentPings.find(_from);
if (sentPing == m_sentPings.end())
{
LOG(m_logger) << "Unexpected PONG from " << _from.address().to_string() << ":"
<< _from.port();
return;
}

if (pong.echo != sentPing->second.pingHash)
auto const& pong = dynamic_cast<Pong const&>(*packet);
auto const& nodeValidation = sentPing->second;
if (pong.echo != nodeValidation.pingHash)
{
LOG(m_logger) << "Invalid PONG from " << _from.address().to_string() << ":"
<< _from.port();
return;
}

// in case the node answers with new NodeID, drop the record with the old NodeID
auto const& sourceId = pong.sourceid;
if (sourceId != nodeValidation.nodeID)
{
LOG(m_logger) << "Node " << _from << " changed public key from "
<< nodeValidation.nodeID << " to " << sourceId;
if (auto node = nodeEntry(nodeValidation.nodeID))
dropNode(move(node));
}

// create or update nodeEntry with new Pong received time
DEV_GUARDED(x_nodes)
{
auto it = m_allNodes.find(sourceId);
if (it == m_allNodes.end())
sourceNodeEntry = make_shared<NodeEntry>(m_hostNodeID, sourceId,
sentPing->second.endpoint, RLPXDatagramFace::secondsSinceEpoch(), 0);
NodeIPEndpoint{_from.address(), _from.port(), nodeValidation.tcpPort},
RLPXDatagramFace::secondsSinceEpoch(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit) can you add an inline comment stating what the purpose of the 0 literal e.g. 0 /* last pong sent time */

else
{
sourceNodeEntry = it->second;
Expand All @@ -504,7 +515,7 @@ void NodeTable::onPacketReceived(
}
}

m_sentPings.erase(sentPing);
m_sentPings.erase(_from);

// update our endpoint address and UDP port
DEV_GUARDED(x_nodes)
Expand Down Expand Up @@ -682,7 +693,7 @@ void NodeTable::doHandleTimeouts()
{
if (chrono::steady_clock::now() > it->second.pingSendTime + m_requestTimeToLive)
{
if (auto node = nodeEntry(it->first))
if (auto node = nodeEntry(it->second.nodeID))
{
dropNode(move(node));

Expand Down
16 changes: 13 additions & 3 deletions libp2p/NodeTable.h
View file
Open in desktop
Expand Up @@ -177,15 +177,25 @@ class NodeTable : UDPSocketEvents
// protected only for derived classes in tests
protected:
/**
* NodeValidation is used to record Pinged node's endpoint, the timepoint of sent PING,
* NodeValidation is used to record Pinged node's ID, the timepoint of sent PING,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the timepoint of sent PING" and "time of sending" is redundant

* time of sending and the new node ID to replace unresponsive node.
*/
struct NodeValidation
{
NodeIPEndpoint endpoint;
NodeID nodeID;
uint16_t tcpPort = 0;
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
TimePoint pingSendTime;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Nit) I think this should be called pingSentTime (since it's referring to a past event)

h256 pingHash;
std::shared_ptr<NodeEntry> replacementNodeEntry;

NodeValidation(NodeID const& _nodeID, uint16_t _tcpPort, TimePoint _pingSendTime,
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
h256 const& _pingHash, std::shared_ptr<NodeEntry> _replacementNodeEntry)
: nodeID(_nodeID),
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
tcpPort(_tcpPort),
pingSendTime(_pingSendTime),
pingHash(_pingHash),
replacementNodeEntry(std::move(_replacementNodeEntry))
{}
};

/// Constants for Kademlia, derived from address space.
Expand Down Expand Up @@ -308,7 +318,7 @@ class NodeTable : UDPSocketEvents
std::shared_ptr<NodeSocket> m_socket; ///< Shared pointer for our UDPSocket; ASIO requires shared_ptr.

// The info about PING packets we've sent to other nodes and haven't received PONG yet
std::unordered_map<NodeID, NodeValidation> m_sentPings;
std::map<bi::udp::endpoint, NodeValidation> m_sentPings;
halfalicious marked this conversation as resolved.
Show resolved Hide resolved

// Expiration time of sent discovery packets.
std::chrono::seconds m_requestTimeToLive;
Expand Down
82 changes: 65 additions & 17 deletions test/unittests/libp2p/net.cpp
View file
Open in desktop
Expand Up @@ -185,11 +185,11 @@ struct TestNodeTable: public NodeTable
return m_buckets[_bucket].nodes.back().lock();
}

boost::optional<NodeValidation> nodeValidation(NodeID const& _id)
boost::optional<NodeValidation> nodeValidation(bi::udp::endpoint const& _endpoint)
{
promise<boost::optional<NodeValidation>> promise;
m_io.post([this, &promise, _id] {
auto validation = m_sentPings.find(_id);
m_io.post([this, &promise, _endpoint] {
auto validation = m_sentPings.find(_endpoint);
if (validation != m_sentPings.end())
promise.set_value(validation->second);
else
Expand Down Expand Up @@ -544,7 +544,7 @@ BOOST_AUTO_TEST_CASE(noteActiveNodeEvictsTheNodeWhenBucketIsFull)
// least recently seen node not removed yet
BOOST_CHECK_EQUAL(nodeTable->bucketFirstNode(bucketIndex), leastRecentlySeenNode);
// but added to evictions
auto evicted = nodeTable->nodeValidation(leastRecentlySeenNode->id);
auto evicted = nodeTable->nodeValidation(leastRecentlySeenNode->endpoint);
BOOST_REQUIRE(evicted.is_initialized());
BOOST_REQUIRE(evicted->replacementNodeEntry);
BOOST_CHECK_EQUAL(evicted->replacementNodeEntry->id, newNodeId);
Expand Down Expand Up @@ -632,7 +632,7 @@ BOOST_AUTO_TEST_CASE(invalidPong)
nodeTable->packetsReceived.pop();

// pending node validation should still be not deleted
BOOST_REQUIRE(nodeTable->nodeValidation(nodePubKey));
BOOST_REQUIRE(nodeTable->nodeValidation(nodeEndpoint));
// node is not in the node table
BOOST_REQUIRE(!nodeTable->nodeExists(nodePubKey));
}
Expand Down Expand Up @@ -675,6 +675,54 @@ BOOST_AUTO_TEST_CASE(validPong)
BOOST_CHECK(addedNode->lastPongReceivedTime > 0);
}

BOOST_AUTO_TEST_CASE(pongWithChangedNodeID)
{
// NodeTable sending PING
TestNodeTableHost nodeTableHost(0);
nodeTableHost.start();
auto& nodeTable = nodeTableHost.nodeTable;
nodeTable->setRequestTimeToLive(std::chrono::seconds(1));
halfalicious marked this conversation as resolved.
Show resolved Hide resolved

// socket receiving PING
TestUDPSocketHost nodeSocketHost{randomPortNumber()};
nodeSocketHost.start();
uint16_t nodePort = nodeSocketHost.port;
halfalicious marked this conversation as resolved.
Show resolved Hide resolved

// add a node to node table, initiating PING
auto nodeEndpoint = NodeIPEndpoint{bi::address::from_string(c_localhostIp), nodePort, nodePort};
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
auto nodeKeyPair = KeyPair::create();
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
auto nodePubKey = nodeKeyPair.pub();
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
nodeTable->addNode(Node{nodePubKey, nodeEndpoint});

// handle received PING
auto pingDataReceived = nodeSocketHost.packetsReceived.pop();
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
auto pingDatagram =
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pingDataReceived));
auto ping = dynamic_cast<PingNode const&>(*pingDatagram);

// send PONG with different NodeID
Pong pong(nodeTable->m_hostNodeEndpoint);
pong.echo = ping.echo;
auto newNodeKeyPair = KeyPair::create();
auto newNodePubKey = newNodeKeyPair.pub();
pong.sign(newNodeKeyPair.secret());
nodeSocketHost.socket->send(pong);

// wait for PONG to be received and handled
nodeTable->packetsReceived.pop();
halfalicious marked this conversation as resolved.
Show resolved Hide resolved

BOOST_REQUIRE(nodeTable->nodeExists(newNodeKeyPair.pub()));
auto addedNode = nodeTable->nodeEntry(newNodePubKey);
BOOST_CHECK(addedNode->lastPongReceivedTime > 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this (and the rest of the BOOST_CHECKs in this test case) be BOOST_REQUIRE? Or is the reasoning that if the checks failed other tests would fail (in which case is there value in having the checks at all)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning is that updating the node in the node table, updating m_sentPings and removing the old node are kind of independent pieces of code, and if one fails, we can still get some useful info about whether other ones work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning is that updating the node in the node table, updating m_sentPings and removing the old node are kind of independent pieces of code, and if one fails, we can still get some useful info about whether other ones work

Yes, but I think the checks will only provide value when you do a local run (let me know if I'm wrong here)? The problem is that failed boost_checks don't trigger test failures and boost_require triggers a test failure and ceases test execution and the boost_checks are all after a boost_require, meaning that if the boost_require fails the subsequent boost_checks won't be executed, and if the boost_require passes the boost_checks might fail but won't trigger a test failure (so we won't know to look at the logs)? Also, do boost_check failures show up in CircleCI logs even if the tests themselves passed?

Can we place the boost_checks before the boost_require?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe BOOST_CHECK failures always show up in logs.


// wait for old node to be kicked out after timeout
this_thread::sleep_for(std::chrono::seconds(6));
halfalicious marked this conversation as resolved.
Show resolved Hide resolved

BOOST_CHECK(!nodeTable->nodeExists(nodePubKey));
auto sentPing = nodeTable->nodeValidation(nodeEndpoint);
BOOST_CHECK(!sentPing.is_initialized());
}

BOOST_AUTO_TEST_CASE(pingTimeout)
{
// NodeTable sending PING
Expand All @@ -698,7 +746,7 @@ BOOST_AUTO_TEST_CASE(pingTimeout)
this_thread::sleep_for(chrono::seconds(6));

BOOST_CHECK(!nodeTable->nodeExists(nodePubKey));
auto sentPing = nodeTable->nodeValidation(nodePubKey);
auto sentPing = nodeTable->nodeValidation(nodeEndpoint);
BOOST_CHECK(!sentPing.is_initialized());

// handle received PING
Expand All @@ -717,7 +765,7 @@ BOOST_AUTO_TEST_CASE(pingTimeout)
nodeTable->packetsReceived.pop();

BOOST_CHECK(!nodeTable->nodeExists(nodePubKey));
sentPing = nodeTable->nodeValidation(nodePubKey);
sentPing = nodeTable->nodeValidation(nodeEndpoint);
BOOST_CHECK(!sentPing.is_initialized());
}

Expand Down Expand Up @@ -849,7 +897,7 @@ BOOST_AUTO_TEST_CASE(evictionWithOldNodeAnswering)
// wait for eviction
evictEvents.pop(chrono::seconds(5));

auto evicted = nodeTable->nodeValidation(nodeId);
auto evicted = nodeTable->nodeValidation(nodeEndpoint);
BOOST_REQUIRE(evicted.is_initialized());

// handle received PING
Expand All @@ -871,7 +919,7 @@ BOOST_AUTO_TEST_CASE(evictionWithOldNodeAnswering)
BOOST_REQUIRE(nodeTable->nodeExists(nodeId));
auto addedNode = nodeTable->nodeEntry(nodeId);
BOOST_CHECK(addedNode->lastPongReceivedTime);
auto sentPing = nodeTable->nodeValidation(nodeId);
auto sentPing = nodeTable->nodeValidation(nodeEndpoint);
BOOST_CHECK(!sentPing.is_initialized());
// check that old node is most recently seen in the bucket
BOOST_CHECK(nodeTable->bucketLastNode(bucketIndex)->id == nodeId);
Expand All @@ -890,7 +938,7 @@ BOOST_AUTO_TEST_CASE(evictionWithOldNodeDropped)

nodeTableHost.start();

auto oldNodeId = nodeTable->bucketFirstNode(bucketIndex)->id;
auto oldNode = nodeTable->bucketFirstNode(bucketIndex);

// generate new address for the same bucket
NodeID newNodeId;
Expand All @@ -910,8 +958,8 @@ BOOST_AUTO_TEST_CASE(evictionWithOldNodeDropped)
this_thread::sleep_for(chrono::seconds(6));

// check that old node is evicted
BOOST_CHECK(!nodeTable->nodeExists(oldNodeId));
BOOST_CHECK(!nodeTable->nodeValidation(oldNodeId).is_initialized());
BOOST_CHECK(!nodeTable->nodeExists(oldNode->id));
BOOST_CHECK(!nodeTable->nodeValidation(oldNode->endpoint).is_initialized());
// check that replacement node is active
BOOST_CHECK(nodeTable->nodeExists(newNodeId));
auto newNode = nodeTable->nodeEntry(newNodeId);
Expand Down Expand Up @@ -971,12 +1019,12 @@ BOOST_AUTO_TEST_CASE(addSelf)
auto nodePubKey = KeyPair::create().pub();
Node node(nodePubKey, nodeEndpoint);
nodeTable->addNode(node);
BOOST_CHECK(nodeTable->nodeValidation(nodePubKey));
BOOST_CHECK(nodeTable->nodeValidation(nodeEndpoint));

// Create self node and verify it isn't pinged
Node self(nodeTableHost.m_alias.pub(), nodeEndpoint);
Node self(nodeTableHost.m_alias.pub(), nodeTable->m_hostNodeEndpoint);
nodeTable->addNode(self);
BOOST_CHECK(!nodeTable->nodeValidation(nodeTableHost.m_alias.pub()));
BOOST_CHECK(!nodeTable->nodeValidation(nodeTable->m_hostNodeEndpoint));
}

BOOST_AUTO_TEST_CASE(findNodeIsSentAfterPong)
Expand Down Expand Up @@ -1108,15 +1156,15 @@ BOOST_AUTO_TEST_CASE(addNodePingsNodeOnlyOnce)
auto nodePubKey = KeyPair::create().pub();
nodeTable->addNode(Node{nodePubKey, nodeEndpoint});

auto sentPing = nodeTable->nodeValidation(nodePubKey);
auto sentPing = nodeTable->nodeValidation(nodeEndpoint);
BOOST_REQUIRE(sentPing.is_initialized());

this_thread::sleep_for(chrono::milliseconds(2000));

// add it for the second time
nodeTable->addNode(Node{nodePubKey, nodeEndpoint});

auto sentPing2 = nodeTable->nodeValidation(nodePubKey);
auto sentPing2 = nodeTable->nodeValidation(nodeEndpoint);
BOOST_REQUIRE(sentPing2.is_initialized());

// check that Ping was sent only once, so Ping hash didn't change
Expand Down