Discovery protocol updates #1706

subtly · 2015-04-21T22:35:53Z

storing/sharing of udp+tcp ports and add endpoint data to ping/pong packets (1557,1558)
bump network version
RLP encapsulation of NodeIPEndpoint
update save/restore nodes
remove redundant addNode
add additional port to discovery endpoint encapsulation #1557, update ping and pong packets #1558

Conflicts: libp2p/NodeTable.cpp

chriseth · 2015-04-24T14:42:39Z

libp2p/NodeTable.h

-	unsigned ts;
+	NodeIPEndpoint source;
+	NodeIPEndpoint destination;
+	uint32_t ts;


Not always initialized.

chriseth · 2015-04-24T14:45:24Z

Are there tests for ipv6?

subtly · 2015-04-24T16:57:47Z

@chriseth simple IPv6 goes in after this is working, where simple means that only IPv4 or IPv6 is supposed (not both at once). It's supported by encapsulation but because the listener doesn't support IPv6 such nodes are simply not added to the table. IPv6 was removed previously because some changes are needed:

additional address verification and NAT is different
most IPv6 systems can connect to IPv4 systems, but reverse isn't true
must consider considering IPv6 as another network (node table, networkid)

… initial values for packet timestamps.

fjl · 2015-04-27T00:44:49Z

This is not interoperable with go-ethereum (this branch) because IP addresses are encoded as RLP lists.
I think lists are not a good choice for IP addresses. But we need to decide.

I0427 02:33:57.213023   87752 udp.go:404] Bad packet from 127.0.0.1:30303: rlp: expected input string or byte for net.IP, decoding into (discover.ping).From.IP

The ping packets sent by Go look like this:

dc 04  cb 845b40e7ba    827660 827660  c9 847f000001 82765f 80   84553d833f
[  v   [  91.64.231.186 30304  30304 ] [  127.0.0.1  30303  0  ] expiration ]

The ones sent by C++ look like this:

de 04  cb c480808080    82765f 82765f  cb c47f808001    827660 827660  84553e0dc8 
[  v   [  [ 0 0 0 0 ]   30303  30303 ] [  [ 127 0 0 1 ] 30304  30304 ] expiration ]

fjl · 2015-04-27T10:31:02Z

libp2p/Common.h

@@ -177,6 +185,9 @@ struct NodeIPEndpoint
 	operator bool() const { return !address.is_unspecified() && udpPort > 0 && tcpPort > 0; }

 	bool isAllowed() const { return NodeIPEndpoint::test_allowLocal ? !address.is_unspecified() : isPublicAddress(address); }
+
+	void streamRLP(RLPStream& _s, RLPAppend _inline = StreamList) const { if (_inline == StreamList) _s.appendList(3); if (address.is_v4()) _s << address.to_v4().to_bytes(); else if (address.is_v6()) _s << address.to_v6().to_bytes(); else _s << ""; _s << udpPort << tcpPort; }
+	void interpretRLP(RLP const& _r) { if (_r[0].size() == 0) address = bi::address(); else if (_r[0].size() == 4) address = bi::address_v4(_r[0].toArray<byte, 4>()); else address = bi::address_v6(_r[0].toArray<byte, 16>()); udpPort = _r[1].toInt<uint16_t>(); tcpPort = _r[2].toInt<uint16_t>(); }


Stylistic issues aside (296 characters in a line, including a three-way braceless conditional, oh my), there seems to be some confusion about the type of input here.

size() only works for RLP strings and returns 0 otherwise, yet toArray() is used, which only works for lists. If IP addresses are supposed to be represented as lists maybe this should switch on itemCount() instead.

@fjl Byte string makes sense. Is that supported by Go?

gavofyork · 2015-04-29T14:33:10Z

libp2p/NodeTable.cpp

 				ret.push_back(n);
 	return move(ret);
 }

-void NodeTable::ping(bi::udp::endpoint _to) const
+void NodeTable::ping(NodeIPEndpoint _to) const


gavofyork · 2015-04-29T14:34:42Z

code looks reasonable - as long as this is 100% compatible with Go, it looksgood.

fjl · 2015-04-30T06:18:19Z

The RLP encoding of pong is missing the destination endpoint:

@@ -330,8 +330,8 @@ struct Pong: RLPXDatagram<Pong>
    h256 echo;              ///< MCD of PingNode
    uint32_t ts = 0;
-   void streamRLP(RLPStream& _s) const { _s.appendList(2); _s << echo << ts; }
-   void interpretRLP(bytesConstRef _bytes) { RLP r(_bytes); echo = (h256)r[0]; ts = r[1].toInt<uint32_t>(); }
+   void streamRLP(RLPStream& _s) const { _s.appendList(3); destination.streamRLP(_s); _s << echo << ts; }
+   void interpretRLP(bytesConstRef _bytes) { RLP r(_bytes); destination.interpretRLP(r[0]); echo = (h256)r[1]; ts = r[2].toInt<uint32_t>(); }
 };

 /**

fjl · 2015-04-30T06:26:28Z

With the pong encoding fixed, we are interoperable once again.

fjl · 2015-04-30T06:44:53Z

Is the sha3 distance going to be part of this PR?

subtly · 2015-04-30T11:16:39Z

@fjl Nice catch. How'd you create a comment like that?

fjl · 2015-04-30T11:18:23Z

I pasted the diff and put diff ... around it.

fjl · 2015-04-30T13:29:19Z

I tested again, the ping/pong seems to work (geth is accepting a pong sent by cpp). It looks like
eth doesn't accept the pongs from geth, though, because it doesn't reply to findnode.

eth crashed after ~15seconds. The nodes exchanged blocks before the crash. Trace here: https://gist.github.com/fjl/f08e36c5d4026f37085c

…s called

fjl · 2015-05-05T07:45:12Z

I have tested this again. The crash is no longer happening and cpp responds to findnode if it has nodes in the table.

Conflicts: libp2p/Host.cpp libp2p/NodeTable.cpp libp2p/NodeTable.h

…ferencing shared pointer from weak ptr without verifying weak ptr).

Discovery protocol updates

subtly added 5 commits April 21, 2015 05:30

compiles (v4 endpoint changes)

e9dfa86

update tests (v4 endpoint)

0850c7a

update net tests (v4 endpoint)

c70f21a

update peer host test. support unspecified ip.

d76dfef

more test updates (v4 endpoints. ethereum#1557, ethereum#1558)

c71d431

subtly added the network label Apr 21, 2015

subtly mentioned this pull request Apr 21, 2015

ping/pong dance for unknown/invalid endpoints #1707

Open

subtly added 10 commits April 23, 2015 03:00

Merge branch 'develop' into discovery

862452a

handle new endpoint properties for ping

50f5c96

Merge branch 'develop' into discovery

eb2ca75

Merge branch 'develop' into discovery

b3ba7d3

Minor semantic updates. Support unspecified address for NodeIPEndpoint.

b3f4e77

check node discovery timestamps.

e606ef9

simple endpoint update

9c541f9

Merge branch 'develop' into discovery

6dd5b57

Conflicts: libp2p/NodeTable.cpp

cleanup

12fbcc8

cleanup

eb946bb

subtly added the pleasereview label Apr 24, 2015

chriseth reviewed Apr 24, 2015
View reviewed changes

libp2p/NodeTable.h

unsigned ts;

NodeIPEndpoint source;

NodeIPEndpoint destination;

uint32_t ts;

Copy link

Contributor

chriseth Apr 24, 2015

Choose a reason for hiding this comment

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

Not always initialized.

chriseth added gotissues and removed pleasereview labels Apr 24, 2015

fix for insensitive windows compiler which reserves 'inline'. default…

00cbab8

… initial values for packet timestamps.

fjl reviewed Apr 27, 2015
View reviewed changes

subtly added pleasereview and removed gotissues labels Apr 27, 2015

subtly added 2 commits April 28, 2015 14:18

Windows keyword error for _inline.

30acd13

Merge branch 'develop' into discovery

41ded89

subtly added pleasereview and removed gotissues labels Apr 28, 2015

Merge branch 'develop' into discovery

fdefc0e

gavofyork reviewed Apr 29, 2015
View reviewed changes

update pong encoding

c84d5f3

subtly added 4 commits April 30, 2015 16:16

add missing const

f176a1c

Merge branch 'develop' into discovery

c5e6c7d

revert assumption that node exists in nodetable when noteactivenode i…

3ebaffa

…s called

Merge branch 'develop' into discovery

e541c23

subtly added 5 commits May 6, 2015 22:42

Merge branch 'develop' into discovery

c27ff03

Merge branch 'develop' into discovery

9046e4f

Conflicts: libp2p/Host.cpp libp2p/NodeTable.cpp libp2p/NodeTable.h

Missed in merge.

d46bd2b

Merge stash into discovery.

03db500

fix restore network to support both old and new network file format

4ac289b

subtly removed the pleasereview label May 6, 2015

subtly added 2 commits May 7, 2015 00:51

Merge branch 'develop' into discovery

5f1b711

Fix network-snapshot method when there's a lot of node activity (dere…

d651501

…ferencing shared pointer from weak ptr without verifying weak ptr).

gavofyork pushed a commit that referenced this pull request May 7, 2015

Merge pull request #1706 from subtly/discovery

c85b3f6

Discovery protocol updates

gavofyork merged commit c85b3f6 into ethereum:develop May 7, 2015

subtly deleted the discovery branch May 7, 2015 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discovery protocol updates #1706

Discovery protocol updates #1706

subtly commented Apr 21, 2015

chriseth Apr 24, 2015

chriseth commented Apr 24, 2015

subtly commented Apr 24, 2015

fjl commented Apr 27, 2015

fjl Apr 27, 2015

subtly Apr 27, 2015

gavofyork Apr 29, 2015

gavofyork commented Apr 29, 2015

fjl commented Apr 30, 2015

fjl commented Apr 30, 2015

fjl commented Apr 30, 2015

subtly commented Apr 30, 2015

fjl commented Apr 30, 2015

fjl commented Apr 30, 2015

fjl commented May 5, 2015

Discovery protocol updates #1706

Discovery protocol updates #1706

Conversation

subtly commented Apr 21, 2015

chriseth Apr 24, 2015

Choose a reason for hiding this comment

chriseth commented Apr 24, 2015

subtly commented Apr 24, 2015

fjl commented Apr 27, 2015

fjl Apr 27, 2015

Choose a reason for hiding this comment

subtly Apr 27, 2015

Choose a reason for hiding this comment

gavofyork Apr 29, 2015

Choose a reason for hiding this comment

gavofyork commented Apr 29, 2015

fjl commented Apr 30, 2015

fjl commented Apr 30, 2015

fjl commented Apr 30, 2015

subtly commented Apr 30, 2015

fjl commented Apr 30, 2015

fjl commented Apr 30, 2015

fjl commented May 5, 2015