Updating host ENR by gumb0 · Pull Request #5593 · ethereum/aleth

gumb0 · 2019-05-07T14:51:27Z

Implements part 3 of #5551

New host ENR is generated:

when network config given to Host contains new public address / listen port
when EndpointTracker in NodeTable detects new public UDP endpoint

fjl · 2019-05-09T09:57:02Z

libp2p/ENR.cpp

+
+    auto const keyValuePairs = createKeyValuePairs(_secret, _ip, _tcpPort, _udpPort);

+    return ENR{0 /* sequence number */, keyValuePairs, signFunction};


Would be better to start the sequence number at 1.

codecov-io · 2019-05-09T11:11:41Z

Codecov Report

Merging #5593 into master will increase coverage by 0.08%.
The diff coverage is 87.66%.

@@            Coverage Diff             @@
##           master    #5593      +/-   ##
==========================================
+ Coverage   62.33%   62.42%   +0.08%     
==========================================
  Files         350      350              
  Lines       29428    29531     +103     
  Branches     3322     3327       +5     
==========================================
+ Hits        18344    18434      +90     
- Misses       9886     9893       +7     
- Partials     1198     1204       +6

gumb0 · 2019-05-09T16:55:32Z

Currently ENR is updated too often - every time we receive Pong with different destination port (this is how we always updated our knowledge of UDP external port).

I'll try to investigate geth code for a better approach, like updating it only when certain number of packets with specific port is received, it seems it shouldn't be too difficult to implement something like this.

gumb0 · 2019-05-15T10:23:53Z

I plan to rebase this after #5602 is merged

halfalicious · 2019-05-16T03:09:06Z

Currently ENR is updated too often - every time we receive Pong with different destination port (this is how we always updated our knowledge of UDP external port).

I'll try to investigate geth code for a better approach, like updating it only when certain number of packets with specific port is received, it seems it shouldn't be too difficult to implement something like this.

For context what is the downside of updating a node’s ENR this frequently?

gumb0 · 2019-05-16T13:37:55Z

@halfalicious Well discv5 nodes should keep ENRs of their peers up-to-date, so changing it all the time would make it necessary to re-request it from us all the time.

(I suspect putting different endpoint values into our Pings all the time might negatively affect simple v4 discovery, too, somehow)

Also I think signing ENR is pretty costly operation (now it happens sometimes several times a second)

geth implementation ignores ENRs with seq=0

Also make ENR output in log more readable.

also move ip, tcp and udp reading to ENR class, as they're not specific to the v4 Identity Scheme. Rename ENR::m_map to m_keyValuePairs.

gumb0 · 2019-05-22T15:35:25Z

libp2p/Host.cpp

        m_listenPort = m_netConfig.listenPort;

-    determinePublic();
+    m_tcpPublic = determinePublic();


I made determinePublic return tcp::endpoint as result, instead of assigning it inside, to make more explicit that it's the only result of this function.

gumb0 · 2019-05-23T15:24:44Z

I tried it with geth, and it look to be working, geth doesn't complain about our ENR.

The logs look like this:

DEBUG 05-23 17:21:54 p2p  discov Ping from ##6f611751…@127.0.0.1:30303
DEBUG 05-23 17:21:54 p2p  discov Adding node ##6f611751…@127.0.0.1:30303
DEBUG 05-23 17:21:54 p2p  discov Pong to ##6f611751…@127.0.0.1:30303
DEBUG 05-23 17:21:54 p2p  discov Active node ##6f611751…@127.0.0.1:30303
DEBUG 05-23 17:21:54 p2p  discov ENRRequest from ##6f611751…@127.0.0.1:30303
DEBUG 05-23 17:21:54 p2p  discov ENRResponse to ##6f611751…@127.0.0.1:30303
DEBUG 05-23 17:21:54 p2p  discov Active node ##6f611751…@127.0.0.1:30303

TRACE[05-23|17:21:53.797] New dial task                            task="discovery lookup"
TRACE[05-23|17:21:54.570] >> PING/v4                               id=651d77d331e7d70c addr=127.0.0.1:30305     err=nil
TRACE[05-23|17:21:54.572] << PONG/v4                               id=651d77d331e7d70c addr=127.0.0.1:30305     err=nil
TRACE[05-23|17:21:54.573] >> ENRREQUEST/v4                         id=651d77d331e7d70c addr=127.0.0.1:30305     err=nil
TRACE[05-23|17:21:54.575] << ENRRESPONSE/v4                        id=651d77d331e7d70c addr=127.0.0.1:30305     err=nil
DEBUG[05-23|17:21:54.576] Revalidated node                         b=13 id=651d77d331e7d70c checks=1

chfast · 2019-05-28T11:11:54Z

libp2p/ENR.cpp

+std::array<byte, N> bytesToAddress(bytesConstRef _bytes)
+{
+    std::array<byte, N> address;
+    std::copy(_bytes.begin(), _bytes.begin() + N, address.begin());


Use std::copy_n()

chfast · 2019-05-28T11:14:12Z

libp2p/ENR.cpp

-        // The resulting 64-byte signature is encoded as the concatenation of the r and s signature values.
-        return bytes(&s[0], &s[64]);
-    };
+    return ENR(m_seq + 1, _keyValuePairs, _signFunction);


Suggested change

return ENR(m_seq + 1, _keyValuePairs, _signFunction);

return ENR{m_seq + 1, _keyValuePairs, _signFunction};

or

Suggested change

return ENR(m_seq + 1, _keyValuePairs, _signFunction);

return {m_seq + 1, _keyValuePairs, _signFunction};

chfast · 2019-05-28T11:18:58Z

libp2p/ENR.cpp

    {
-        _out << keyValue.first << "=";
-        _out << toHexPrefixed(RLP{keyValue.second}.toBytes()) << " ";
+        _out << "key=" << IdentitySchemeV4::publicKey(_enr).abridged() << " ip=" << _enr.ip()


This can partly output something.

Also fix operator<< to handle incorrect tcp and udp.

gumb0 added the in progress label May 7, 2019

fjl reviewed May 9, 2019

View reviewed changes

gumb0 force-pushed the enr-update branch 2 times, most recently from 8771c9a to b49f1fc Compare May 9, 2019 10:23

gumb0 force-pushed the enr-update branch from c2d3b7f to 7dc369f Compare May 15, 2019 10:23

gumb0 force-pushed the enr-update branch from 7dc369f to b5b6e69 Compare May 20, 2019 13:18

gumb0 added 9 commits May 22, 2019 12:34

Add support for updating ENR

48e893e

Create initial ENR with sequence number 1.

c6f072a

geth implementation ignores ENRs with seq=0

Function to get predefined ENR fields as a typed struct.

1ce397a

Also make ENR output in log more readable.

Update ENR at program start in case new public address is found

8d98cd4

Update host ENR when receiving Pong with new endpoint in destination

6f9ebb2

Split IdentitySchemeV4::info into separate functions for each field;

94d05fd

also move ip, tcp and udp reading to ENR class, as they're not specific to the v4 Identity Scheme. Rename ENR::m_map to m_keyValuePairs.

Tests for updating ENR

b7049d9

Test for updating host ENR from config value in Host

835c140

Add changelog item

f9ccb51

gumb0 force-pushed the enr-update branch from a18a383 to f9ccb51 Compare May 22, 2019 13:39

Add test for ENR updated in NodeTable

242fd6c

gumb0 commented May 22, 2019

View reviewed changes

Triple slashes in comments in ENR.h

d424f0e

gumb0 removed the in progress label May 22, 2019

gumb0 requested review from chfast and halfalicious May 22, 2019 16:45

chfast reviewed May 28, 2019

View reviewed changes

Address review issues

c40e6dd

Don't allow invalid data in tcp and udp fields of ENR.

4242f59

Also fix operator<< to handle incorrect tcp and udp.

chfast approved these changes May 28, 2019

View reviewed changes

gumb0 merged commit 312fff8 into master May 28, 2019

gumb0 deleted the enr-update branch May 28, 2019 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating host ENR#5593

Updating host ENR#5593
gumb0 merged 13 commits intomasterfrom
enr-update

gumb0 commented May 7, 2019 •

edited

Loading

Uh oh!

fjl May 9, 2019

Uh oh!

codecov-io commented May 9, 2019 •

edited

Loading

Uh oh!

gumb0 commented May 9, 2019

Uh oh!

gumb0 commented May 15, 2019

Uh oh!

halfalicious commented May 16, 2019

Uh oh!

gumb0 commented May 16, 2019

Uh oh!

gumb0 May 22, 2019 •

edited

Loading

Uh oh!

gumb0 commented May 23, 2019

Uh oh!

chfast May 28, 2019

Uh oh!

chfast May 28, 2019

Uh oh!

chfast May 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants


		auto const keyValuePairs = createKeyValuePairs(_secret, _ip, _tcpPort, _udpPort);

		return ENR{0 /* sequence number */, keyValuePairs, signFunction};

	return ENR(m_seq + 1, _keyValuePairs, _signFunction);
	return ENR{m_seq + 1, _keyValuePairs, _signFunction};

	return ENR(m_seq + 1, _keyValuePairs, _signFunction);
	return {m_seq + 1, _keyValuePairs, _signFunction};

Conversation

gumb0 commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjl May 9, 2019

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gumb0 commented May 9, 2019

Uh oh!

gumb0 commented May 15, 2019

Uh oh!

halfalicious commented May 16, 2019

Uh oh!

gumb0 commented May 16, 2019

Uh oh!

gumb0 May 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumb0 commented May 23, 2019

Uh oh!

chfast May 28, 2019

Choose a reason for hiding this comment

Uh oh!

chfast May 28, 2019

Choose a reason for hiding this comment

Uh oh!

chfast May 28, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gumb0 commented May 7, 2019 •

edited

Loading

codecov-io commented May 9, 2019 •

edited

Loading

gumb0 May 22, 2019 •

edited

Loading