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 all 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
37 changes: 24 additions & 13 deletions libp2p/NodeTable.cpp
View file
Open in desktop
Expand Up @@ -321,8 +321,8 @@ void NodeTable::ping(Node const& _node, shared_ptr<NodeEntry> _replacementNodeEn
if (!m_socket->isOpen())
return;

// Don't sent Ping if one is already sent
if (contains(m_sentPings, _node.id))
// Don't send Ping if one is already sent
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(),
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 /* lastPongSentTime */);
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 @@ -680,9 +691,9 @@ void NodeTable::doHandleTimeouts()
vector<shared_ptr<NodeEntry>> nodesToActivate;
for (auto it = m_sentPings.begin(); it != m_sentPings.end();)
{
if (chrono::steady_clock::now() > it->second.pingSendTime + m_requestTimeToLive)
if (chrono::steady_clock::now() > it->second.pingSentTime + m_requestTimeToLive)
{
if (auto node = nodeEntry(it->first))
if (auto node = nodeEntry(it->second.nodeID))
{
dropNode(move(node));

Expand Down
26 changes: 21 additions & 5 deletions libp2p/NodeTable.h
View file
Open in desktop
Expand Up @@ -177,15 +177,31 @@ 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,
* time of sending and the new node ID to replace unresponsive node.
* NodeValidation is used to record information about the nodes that we have sent Ping to.
*/
struct NodeValidation
{
NodeIPEndpoint endpoint;
TimePoint pingSendTime;
// Public key of the target node
NodeID nodeID;
// We receive TCP port in the Ping packet, and store it here until it passes endpoint proof
// (answers with Pong), then it will be added to the bucket of the node table
uint16_t tcpPort = 0;
halfalicious marked this conversation as resolved.
Show resolved Hide resolved
// Time we sent Ping - used to handle timeouts
TimePoint pingSentTime;
// Hash of the sent Ping packet - used to validate received Pong
h256 pingHash;
// Replacement is put into the node table,
// if original pinged node doesn't answer after timeout
std::shared_ptr<NodeEntry> replacementNodeEntry;

NodeValidation(NodeID const& _nodeID, uint16_t _tcpPort, TimePoint const& _pingSentTime,
h256 const& _pingHash, std::shared_ptr<NodeEntry> _replacementNodeEntry)
: nodeID{_nodeID},
tcpPort{_tcpPort},
pingSentTime{_pingSentTime},
pingHash{_pingHash},
replacementNodeEntry{std::move(_replacementNodeEntry)}
{}
};

/// Constants for Kademlia, derived from address space.
Expand Down Expand Up @@ -308,7 +324,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
86 changes: 68 additions & 18 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,53 @@ 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(chrono::seconds(1));

// socket receiving PING
TestUDPSocketHost nodeSocketHost{randomPortNumber()};
nodeSocketHost.start();
uint16_t const nodePort = nodeSocketHost.port;

// add a node to node table, initiating PING
auto const nodeEndpoint =
NodeIPEndpoint{bi::address::from_string(c_localhostIp), nodePort, nodePort};
auto const nodeKeyPair = KeyPair::create();
auto const nodePubKey = nodeKeyPair.pub();
nodeTable->addNode(Node{nodePubKey, nodeEndpoint});

// handle received PING
auto const pingDataReceived = nodeSocketHost.packetsReceived.pop();
auto const pingDatagram =
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pingDataReceived));
BOOST_REQUIRE_EQUAL(pingDatagram->typeName(), "Ping");
auto const& ping = dynamic_cast<PingNode const&>(*pingDatagram);

// send PONG with different NodeID
Pong pong(nodeTable->m_hostNodeEndpoint);
pong.echo = ping.echo;
auto const newNodeKeyPair = KeyPair::create();
auto const 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 const 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.


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

BOOST_AUTO_TEST_CASE(pingTimeout)
{
// NodeTable sending PING
Expand All @@ -698,7 +745,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 @@ -714,10 +761,13 @@ BOOST_AUTO_TEST_CASE(pingTimeout)
nodeSocketHost.socket->send(pong);

// wait for PONG to be received and handled
nodeTable->packetsReceived.pop();
auto pongDataReceived = nodeTable->packetsReceived.pop();
auto pongDatagram =
DiscoveryDatagram::interpretUDP(bi::udp::endpoint{}, dev::ref(pongDataReceived));
BOOST_REQUIRE_EQUAL(pongDatagram->typeName(), "Pong");

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

Expand Down Expand Up @@ -849,7 +899,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 +921,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 +940,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 +960,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 +1021,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 +1158,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