Skip to content
This repository has been archived by the owner. It is now read-only.

libp2p: EIP-8 changes #46

Merged
merged 5 commits into from Feb 17, 2016
Merged

libp2p: EIP-8 changes #46

merged 5 commits into from Feb 17, 2016

Conversation

@fjl
Copy link
Contributor

@fjl fjl commented Jan 13, 2016

@fjl fjl force-pushed the fjl:p2p-eip8 branch 2 times, most recently from 10a5051 to fd0804d Jan 13, 2016
@subtly
Copy link
Member

@subtly subtly commented Jan 13, 2016

Presents!

@@ -194,6 +194,15 @@ class NodeIPEndpoint
operator bool() const { return !address.is_unspecified() && udpPort > 0 && tcpPort > 0; }

bool isAllowed() const { return NodeIPEndpoint::test_allowLocal ? !address.is_unspecified() : isPublicAddress(address); }

bool operator==(const NodeIPEndpoint rhs) const

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

NodeIPEndpoint const& _cmp. Whitespace can be removed in header (single line function).

This comment has been minimized.

@chriseth

chriseth Jan 13, 2016
Contributor

Please keep the line breaks for better readability if the line would be longer than 100 characters.

{
return address == rhs.address && udpPort == rhs.udpPort && tcpPort == rhs.tcpPort;
}
bool operator!=(const NodeIPEndpoint rhs) const

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

NodeIPEndpoint const& _cmp

found[distance(_target, p->id)].push_back(p);
else
break;
found[distance(_target, p->id)].push_back(p);

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

Presuming all tests pass, this should be:

if (!!p->endpoint && p->endpoint.isAllowed() && count++ < s_bucketSize)
    found[distance(_target, p->id)].push_back(p);

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

else
    break;
found[distance(_target, p->id)].push_back(p);
else
break;
found[distance(_target, p->id)].push_back(p);

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

Presuming all tests pass, this should be:

if (!!p->endpoint && p->endpoint.isAllowed() && count++ < s_bucketSize)
    found[distance(_target, p->id)].push_back(p);
else
    break;
if (count < s_bucketSize && tail)

if (tail > 0)

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

this should be reverted based on other changes made

// if d is 0, then we roll look forward, if last, we reverse, else, spread from d
if (head > 1 && tail != lastBin)
while (head != tail && head < s_bins && count < s_bucketSize)
while (head != tail && head < s_bins)

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

this should be reverted based on other changes made

try {
RLP rlp(rlpBytes);
switch (packetType)
auto packet = DiscoveryDatagram::interpretUDP(_from, _packet);

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

Please remove DRY from this PR and submit in a future PR, or, leave DRY in and submit functional changes in a separate PR.

switch (packetType)
auto packet = DiscoveryDatagram::interpretUDP(_from, _packet);
if (packet->isExpired()) {
BOOST_THROW_EXCEPTION(InvalidDiscoveryPacket("Timestamp is in the past"));

This comment has been minimized.

@subtly

subtly Jan 13, 2016
Member

Please remove change to add an exception from this PR and submit a future PR, or, leave this in and submit functional changes in a separate PR.

@subtly
Copy link
Member

@subtly subtly commented Jan 13, 2016

In the C++ codebase, especially in the networking code, we do not review PRs which mix semantic and functional changes into a PR as it's too difficult to effectively review changes.

Moreover, this class does not throw exceptions and logs have been removed. Please revert these as requested. Otherwise, please create github issues, assign them to me, and notify me – I will happily make the changes requested. In particular, I've really been meaning to refactor the nearestNodeEntries function (it was thrown together in a few days to make the PoC release).

@fjl fjl force-pushed the fjl:p2p-eip8 branch 8 times, most recently from 31c14a5 to 8d34e97 Jan 14, 2016
@fjl fjl force-pushed the fjl:p2p-eip8 branch from 8d34e97 to 1f96322 Jan 25, 2016
@@ -98,20 +98,25 @@ bytes ReputationManager::data(Session const& _s, std::string const& _sub) const
return bytes();
}

Host::Host(std::string const& _clientVersion, NetworkPreferences const& _n, bytesConstRef _restoreNetwork):
Host::Host(std::string const& _clientVersion, KeyPair _alias, NetworkPreferences const& _n):

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

KeyPair const&?

m_lastPing(chrono::steady_clock::time_point::min())
{
clog(NetNote) << "Id:" << id();
}

Host::Host(std::string const& _clientVersion, NetworkPreferences const& _n, bytesConstRef _restoreNetwork):

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

We do not use the std:: prefix in cpp files.

}
catch (...)
catch (std::exception const& _e)

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

This will not catch all exceptions, please either add two catch clauses or remove the additional reason.


BOOST_AUTO_TEST_CASE(test_handshake_eip8_ack2)
{
auto keyA = Secret("49a7b37aa6f6645917e7b807e9d1c00d4fa71f18343b0d4122a4d2df64dd6fee");

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

I would prefer Secret keyA("49...fee") (similar below).
auto should be avoided for safety reasons (and for reasons of usability of IDEs) unless the type is some complicated template type.

}

noteActiveNode(nodeid, _from);

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

Is it ok to move that to the top?

}
void interpretRLP(bytesConstRef _bytes)
{
RLP r(_bytes, RLP::AllowNonCanon);

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

I think we should not change the mode to RLP::AllowNonCanon.

crypto::ecdh::agree(m_host->m_alias.sec(), m_remote, sharedSecret);
m_remoteEphemeral = recover(*(Signature*)sig.data(), sharedSecret.makeInsecure() ^ m_remoteNonce);
bytesConstRef data(&m_auth);
auto sig = Signature(data.cropped(0, Signature::size));

This comment has been minimized.

@chriseth

chriseth Feb 3, 2016
Contributor

Please change to Signature sig(data.cropped(0, Signature::size)); (similar below)

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 3, 2016

As already discussed, please change the exceptions while interpreting the packet to direct logging (potentially using a helper function) plus returning an empty unique ptr.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 12, 2016

@fjl ah, ok, now I get why you want to use RLP:AllowNonCanon, will have a look.

@@ -77,6 +77,9 @@ using AddressHash = std::unordered_set<h160>;
/// A vector of secrets.
using Secrets = std::vector<Secret>;

/// Amount of bytes added when encrypting with encryptECIES.
static const unsigned c_eciesOverhead = 113;

This comment has been minimized.

@chriseth

chriseth Feb 12, 2016
Contributor

This file seems a bit too generic for this protocol detail. Can we move it somewhere else?

RLP rlp(rlpBytes);
switch (packetType)
unique_ptr<DiscoveryDatagram> packet = DiscoveryDatagram::interpretUDP(_from, _packet);
if (!packet) {

This comment has been minimized.

@chriseth

chriseth Feb 12, 2016
Contributor

No braces needed.

@fjl fjl changed the title libp2p: EIP-8 changes (WIP) libp2p: EIP-8 changes Feb 12, 2016
@fjl
Copy link
Contributor Author

@fjl fjl commented Feb 12, 2016

PTAL


// These two are set for inbound packets only.
NodeID sourceid;
h256 echo;

This comment has been minimized.

@chriseth

chriseth Feb 12, 2016
Contributor

What does echo mean?

{
if (in.version == 3)
{
compat::Pong p(in.source);

This comment has been minimized.

@chriseth

chriseth Feb 12, 2016
Contributor

Where did this logic go? Do we always send a pong now, and ignore the version?

This comment has been minimized.

@fjl

fjl Feb 16, 2016
Author Contributor

Yes. v3 compatibility is gone and we always send v4 pong back.

PingNode(bi::udp::endpoint _ep): RLPXDatagram<PingNode>(_ep), source(UnspecifiedNodeIPEndpoint), destination(UnspecifiedNodeIPEndpoint) {}
using DiscoveryDatagram::DiscoveryDatagram;
PingNode(NodeIPEndpoint _src, NodeIPEndpoint _dest): DiscoveryDatagram(_dest), source(_src), destination(_dest) {}
PingNode(bi::udp::endpoint const& _from, NodeID _fromid, h256 _echo): DiscoveryDatagram(_from, _fromid, _echo) {}

This comment has been minimized.

@chriseth

chriseth Feb 12, 2016
Contributor

const& for NodeID, h256 and NodeIPEndpoint (above)?
Also for all other packet types.

void interpretRLP(bytesConstRef _bytes)
{
RLP r(_bytes, RLP::AllowNonCanon|RLP::ThrowOnFail);
version = r[0].toInt<unsigned>(RLP::Strict);

This comment has been minimized.

@chriseth

chriseth Feb 12, 2016
Contributor

Strict should be default

{
RLP r(_bytes, RLP::AllowNonCanon|RLP::ThrowOnFail);
for (auto n: r[0])
neighbours.push_back(Neighbour(n));

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

If you like, you can also use emplace_back.

void interpretRLP(bytesConstRef _bytes)
{
RLP r(_bytes, RLP::AllowNonCanon|RLP::ThrowOnFail);
for (auto n: r[0])

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

auto const&

}
_s.appendList(2);
_s.appendList(neighbours.size());
for (auto& n: neighbours) n.streamRLP(_s);

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

const& just to be sure...

@@ -28,6 +28,8 @@ using namespace dev;
using namespace dev::p2p;
using namespace CryptoPP;

boost::random_device s_paddingEngine;

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

Hm, I think boost::random_device is not thread safe. Furthermore, I'm not sure if it is possible to e.g. deplete the entropy pool (and thus possibly degrade the security of more critical systems) by sending massive connection attempts.

m_remoteVersion = _remoteVersion;
Secret sharedSecret;
crypto::ecdh::agree(m_host->m_alias.sec(), _remotePubk, sharedSecret);
m_remoteEphemeral = recover(_sig, sharedSecret.makeInsecure() ^ _remoteNonce);

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

Perhaps being overly paranoid here: Is it possible to compute the ^ on sharedSecret's data?

This comment has been minimized.

@fjl

fjl Feb 16, 2016
Author Contributor

As discussed, it's not an issue either way.

@@ -72,9 +99,19 @@ void RLPXHandshake::writeAck()
});
}

void RLPXHandshake::setAuthValues(Signature _sig, Public _remotePubk, h256 _remoteNonce, uint64_t _remoteVersion)

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

Please pass all of them by const&

clog(NetP2PConnect) << "p2p.connect.ingress receiving " << size << "bytes EIP-8 auth from " << m_socket->remoteEndpoint();
m_authCipher.resize((size_t)size + 2);
auto rest = ba::buffer(ba::buffer(m_authCipher) + 307);
auto self(shared_from_this());

This comment has been minimized.

@chriseth

chriseth Feb 13, 2016
Contributor

Ah, good old Münchhausen!

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 13, 2016

Only minor things left apart from the randomness issue:
AFAIK, boost::random_device is not thread safe and futhermore, it might be possible to deplete the entropy pool with a lot of connection attempts which would degrade the security of other, more important modules.

@fjl
Copy link
Contributor Author

@fjl fjl commented Feb 16, 2016

For the padding we decided to go with rand(3), which is known to be unsafe for concurrent use, instead of using boost. It will be fixed later.

@chriseth
Copy link
Contributor

@chriseth chriseth commented Feb 17, 2016

Tested on linux, works fine.

@fjl fjl force-pushed the fjl:p2p-eip8 branch from 49d9fda to 4dff018 Feb 17, 2016
@fjl
Copy link
Contributor Author

@fjl fjl commented Feb 17, 2016

Squashed.

chriseth added a commit that referenced this pull request Feb 17, 2016
@chriseth chriseth merged commit d0507ce into ethereum:develop Feb 17, 2016
2 of 3 checks passed
2 of 3 checks passed
libweb3core -- ALL Build finished. No test results found.
Details
ubuntu The build has succeeded
Details
windows-slave The build has succeeded
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants