ENR record for host #5537

gumb0 · 2019-03-28T17:37:00Z

https://eips.ethereum.org/EIPS/eip-778

This creates ENR record for the host at program start, filling values based on --public-ip and --listen CLI parameters.

For example aleth --public-ip 13.74.189.147 --listen 30303 creates ENR like

[<signature>, 0, "id", "v4", "ip", 0xD4ABD93, "sec256k1", <compressed public key>, "tcp", 30303, "udp", 30303]

(sequence number is always 0 now, updating ENR will be added in later PRs)

At program exit ENR is serialized into network.rlp file and restored at the next program start.

cc @fjl

codecov-io · 2019-04-02T09:39:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@dbbfd5b). Click here to learn what that means.
The diff coverage is 94.41%.

@@            Coverage Diff            @@
##             master    #5537   +/-   ##
=========================================
  Coverage          ?   62.14%           
=========================================
  Files             ?      347           
  Lines             ?    28965           
  Branches          ?     3281           
=========================================
  Hits              ?    18000           
  Misses            ?     9787           
  Partials          ?     1178

gumb0 · 2019-04-04T16:53:10Z

@chfast @halfalicious I think this is ready for review.

It's based on #5542 and should be rebased when it's merged.

Also I need to add a CHANGELOG item here, but maybe we merge this after 1.6.0 release?

halfalicious · 2019-04-05T06:40:49Z

The failed tests might just be unreliable, see #5544

gumb0 · 2019-04-08T12:27:20Z

Rebased.

chfast · 2019-04-08T12:20:18Z

libp2p/ENR.cpp

+#include "ENR.h"
+#include <libdevcore/SHA3.h>
+
+static std::string const c_keyID = "id";


I don't think it is great to have global dynamically constructed objects.

https://godbolt.org/z/qZwVxl

Because of run-time initialization cost? Is it better to have dynamic allocation each time this code is run? (string is probably short-string-optimized, but vector isn't)

Yes, to avoid dynamic initialization during startup.

This can land, but I'm leaving it to consider changes.

libp2p/ENR.cpp

Co-Authored-By: gumb0 <andrei@ethereum.org>

halfalicious · 2019-04-09T03:29:10Z

libp2p/ENR.cpp

+constexpr char c_keyTCP[] = "tcp";
+constexpr char c_keyUDP[] = "udp";
+constexpr char c_IDV4[] = "v4";
+constexpr size_t c_ENRMaxSize = 300;


Would be nice to call out that this is in bytes (preferably by renaming the variable)

halfalicious · 2019-04-09T04:20:44Z

I noticed you created the tests using Gtest...is the goal to switch everything over to Gtest eventually?

halfalicious

Didn't get a chance to look at the test code yet but if they all pass then they are presumably okay.

libp2p/ENR.cpp

halfalicious · 2019-04-09T03:36:26Z

libp2p/ENR.cpp

+
+    // read key-values into vector first, to check the order
+    std::vector<std::pair<std::string const, bytes>> keyValues;
+    for (size_t i = 2; i < _rlp.itemCount(); i+= 2)


Technically the loop termination check should be i < _rlp.itemCount() - 1 (since you're increasing i by 2 on each loop iteration)

count - 1 looks dangerous for unsigned type (well in this case it shouldn't underflow though)

Also _rlp[i + 1] in the body loop should throw for out of bounds and not lead to UB.

I think _rlp[i + 1] would only throw if it happened to point to protected memory (unless you have something like full page heap enabled) and I think the chances of this are low.

I think you confuse it with arrays/vectors.

Actually I was not right - RLP::operator[] will not throw but return empty RLP for out of bounds - I forget this all the time. So in this case it will end up with key => "" if last value is absent, which is not ideal, but it's not an UB, I think I'll merge it like this for now.

I've added some comments to RLP::operator[] which is not very straight-forward a791c5b

libp2p/ENR.cpp

halfalicious · 2019-04-09T03:38:43Z

libp2p/ENR.cpp

+}
+
+ENR::ENR(uint64_t _seq, std::map<std::string, bytes> const& _keyValues, SignFunction _signFunction)
+  : m_seq(_seq), m_map(_keyValues), m_signature(_signFunction(dev::ref(content())))


{} initializers?

libp2p/Host.cpp

halfalicious · 2019-04-09T04:17:52Z

libp2p/Host.cpp

    m_lastPing(chrono::steady_clock::time_point::min()),
    m_capabilityHost(createCapabilityHost(*this))
 {
    cnetnote << "Id: " << id();
+    cnetnote << "ENR: " << m_enr;


In the interest of consistency should we call enr() here? (since we call id() rather than m_alias.pub())

libp2p/Host.cpp

gumb0 · 2019-04-09T13:35:56Z

I noticed you created the tests using Gtest...is the goal to switch everything over to Gtest eventually?

Yes, all unit tests.

halfalicious

Approved, only small issue is the loop iteration but I don't think that's a blocker

libp2p/ENR.cpp

gumb0 added the in progress label Mar 28, 2019

gumb0 force-pushed the enr branch 3 times, most recently from cbb2735 to 8740d15 Compare April 4, 2019 13:58

gumb0 removed the in progress label Apr 4, 2019

gumb0 requested review from chfast and halfalicious April 4, 2019 16:54

gumb0 added 8 commits April 8, 2019 14:11

ENR class and serializing to network.rlp

299d70a

Fix RLP serializing of ENR

46fa014

Validate ENR signature after parsing from RLP

ea648cf

Validate ENR size and "id" field

b015153

Validate order of keys in ENR

f8875f1

More unit tests for ENR creating/parsing

14a08a1

Better stream output operator for ENR

c56ac29

Add more comments

4ab6260

gumb0 force-pushed the enr branch from 4091c4e to 4ab6260 Compare April 8, 2019 12:12

chfast reviewed Apr 8, 2019

View reviewed changes

Avoid pointer arithmetic

5607e64

Co-Authored-By: gumb0 <andrei@ethereum.org>

chfast approved these changes Apr 8, 2019

View reviewed changes

Avoid global dynamically initialized object to save start-up time

16de129

gumb0 force-pushed the enr branch from 70da888 to 16de129 Compare April 8, 2019 16:52

halfalicious reviewed Apr 9, 2019

View reviewed changes

halfalicious suggested changes Apr 9, 2019

View reviewed changes

gumb0 mentioned this pull request Apr 9, 2019

Discovery ENR extension implementation plan #5551

Closed

4 tasks

Address minor review issues

6d8e150

Improve enr/createV4 test

174745c

halfalicious approved these changes Apr 10, 2019

View reviewed changes

libp2p/ENR.cpp Show resolved Hide resolved

Add changelog item

b517084

gumb0 force-pushed the enr branch from 568bb2b to b517084 Compare April 10, 2019 11:06

gumb0 merged commit 2102a41 into master Apr 10, 2019

gumb0 deleted the enr branch April 10, 2019 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENR record for host #5537

ENR record for host #5537

gumb0 commented Mar 28, 2019 •

edited

Loading

codecov-io commented Apr 2, 2019 •

edited

Loading

gumb0 commented Apr 4, 2019

halfalicious commented Apr 5, 2019

gumb0 commented Apr 8, 2019

chfast Apr 8, 2019

gumb0 Apr 8, 2019

chfast Apr 8, 2019

halfalicious Apr 9, 2019

halfalicious commented Apr 9, 2019

halfalicious left a comment

halfalicious Apr 9, 2019

gumb0 Apr 9, 2019

halfalicious Apr 10, 2019 •

edited

Loading

gumb0 Apr 10, 2019 •

edited

Loading

gumb0 Apr 10, 2019

halfalicious Apr 9, 2019

halfalicious Apr 9, 2019

gumb0 commented Apr 9, 2019

halfalicious left a comment

ENR record for host #5537

ENR record for host #5537

Conversation

gumb0 commented Mar 28, 2019 • edited Loading

codecov-io commented Apr 2, 2019 • edited Loading

Codecov Report

gumb0 commented Apr 4, 2019

halfalicious commented Apr 5, 2019

gumb0 commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfalicious commented Apr 9, 2019

halfalicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfalicious Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

gumb0 Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gumb0 commented Apr 9, 2019

halfalicious left a comment

Choose a reason for hiding this comment

gumb0 commented Mar 28, 2019 •

edited

Loading

codecov-io commented Apr 2, 2019 •

edited

Loading

halfalicious Apr 10, 2019 •

edited

Loading

gumb0 Apr 10, 2019 •

edited

Loading