p2p/enr: initial implementation #15585

nonsense · 2017-11-30T14:58:04Z

initial implementation of ENR according to ethereum/EIPs#778

TODO:

compare RLP encoding/decoding with Python impl at https://github.com/fjl/p2p-drafts.
reject records longer than 300 bytes.
set r.raw on RLP decode, so that record is ready for RLP encoding after RLP decoding.
sort pairs on Set, use binary search on Load
ensure that setting the same key multiple times will update it instead of including it twice.
get rid of double-RLP encoding of pair.value - once in Set() and once is signAndEncode()
add documentation
use binary search on Set
add more tests to check decode/encode and sorting

nonsense · 2017-11-30T15:11:14Z

p2p/enr/enr.go

+	return nil
+}
+
+func wrapList(c []byte) []byte {


This feels like a hack. I am looking to see if there is a nicer way to wrap a list.

nonsense · 2017-11-30T16:24:29Z

p2p/enr/enr.go

+	}
+
+	digest := crypto.Keccak256Hash(content)
+	pubkey2, err := crypto.Ecrecover(digest.Bytes(), e.signature)


The signature returned from the python impl has a length of 64, and recovering of the public key fails.

fjl

I think the API for keys/values should be different because there can be arbitrary keys used by different parts of the codebase. We need a way to define new keys outside of package enr.

Another thing to keep in mind is that record validity is defined to be signature validity only. So a record containing the "ip4" key with RLP value [1, 2, 3] is still a valid (but useless) record. The valid vs. useless distinction is important.

Here's my API proposal:

package enr

// Key is implemented by known node record key types.
//
// To define a new key that is to be included in a node record,
// create a Go type that satisfies this interface. The type should
// also implement rlp.Decoder if additional checks are needed on the value.
type Key interface{
	ENRKey() string
}

// IP4 represents a 4-byte IPv4 address in a node record.
type IP4 net.IP

// ENRKey returns the node record key for an IPv4 address.
func (IP4) ENRKey() string {
	return "ip4"
}

func (v IP4) EncodeRLP(w io.Writer) error {
	ip4 := net.IP(v).To4()
	if ip4 == nil {
		return fmt.Errorf("invalid IPv4 address: %v", v)
	}
	return rlp.Encode(w, ip4)
}

func (v *IP4) DecodeRLP(s *rlp.Stream) error {
	if err := s.Decode((*net.IP)(v)); err != nil {
		return err
	}
	if len(*v) != 4 {
		return fmt.Errorf("invalid IPv4 address, want 4 bytes: %v", *v)
	}
	return nil
}

To construct a node record add Keys to the zero value, then sign it.

func settingAndEncoding() {
	// r is an empty record.
	var r enr.Record
	
	// Add basic metadata.
	r.Set(enr.IP4(net.ParseIP("127.0.0.1")))
	r.SetSeq(1)

	// At this point r is non-empty but not signed, so EncodeRLP() returns an error.
	_, err := rlp.EncodeToBytes(r)
	
	// Sign with secp256k1 key. This adds the "id" and "secp256k1" k/v pairs
	// and the signature.
	if err := r.Sign(privkey); err != nil {
		panic(err)
	}
	
	// This works now:
	_, err := rlp.EncodeToBytes(r)

	// Update metadata. This invalidates the signature.
	r.Set(enr.IP4(net.ParseIP("127.0.0.2")))
	r.SetSeq(2)

	// This will return an error again until the record is re-signed.
	_, err := rlp.EncodeToBytes(r)
}

To get a value out of a node record, pass the Key you want.
This will decode the value if present.

func decodingAndAccessing(input []byte) {
	// Decoding into enr.Record validates the signature.
	var r enr.Record
	err := rlp.DecodeBytes(input, &r)
	if err != nil {
		panic(err)
	}

	// You can get any k/v value like this:
	var ip4 enr.IP4
	ok, err := r.Load(&ip4)
	// ip4 contains the address if it was present in the record.
        // ok says whether the key was present, err says whether the
        // value is valid.
	
	// Accessing basic information:
	seq := r.Seq()
	addr := r.NodeAddr()
}

fjl · 2017-12-01T11:34:24Z

p2p/enr/enr.go

+	}
+
+	digest := crypto.Keccak256Hash(content)
+	pubkey2, err := crypto.Ecrecover(digest.Bytes(), e.signature)


You can use btcec.Signature.Verify instead: https://godoc.org/github.com/btcsuite/btcd/btcec#example-package--VerifySignature

fjl · 2017-12-01T11:34:24Z

p2p/enr/enr.go

+	return signature, raw, nil
+}
+
+func (e *ENR) SerialisedContent() ([]byte, error) {


No need to export this method.

fjl · 2017-12-01T11:34:24Z

p2p/enr/enr.go

+		}
+	}
+
+	return wrapList(buffer.Bytes()), nil


You can avoid most of the RLP hackery by using higher-level types instead. To encode a record, put elements into []interface{} then call rlp.EncodeToBytes:

list := []interface{}{nil, e.seq} // add k/v pairs for _, r := range e.records { list = append(list, k, v) } // sign the tail of the list sigcontent, err := rlp.EncodeToBytes(list[1:]) ... // add signature to front list[0] = sig record, err := rlp.EncodeToBytes(list)

fjl · 2017-12-01T12:13:19Z

Something I forgot in the review summary earlier: the terms are slightly off in your implementation.
A "record" is the whole thing. key/value "pairs" are the elements of a "record". This should be reflected in identifiers.

nonsense · 2017-12-01T17:24:55Z

@fjl thanks for the comments. The proposed API is indeed better. I think I addressed your comments, however I still haven't confirmed the RLP encoding/decoding vs the Python impl. - there might be some differences there. Will ping you when this is ready for review.

fjl

This is starting to look really nice. Thank you so much.
I have two general suggestions and a couple of nitpicks below ;)

Please move all predefined keys into one file. Their definitions aren't that large.

pairs is supposed to be sorted by key at encoding time. Maybe insert into the correct place in Set so it will be sorted at all times. If you do this you can use binary search in Load. Please also ensure that setting the same key multiple times will update it instead of including it twice.

fjl · 2017-12-01T19:18:06Z

p2p/enr/discv5.go

+	"github.com/ethereum/go-ethereum/rlp"
+)
+
+type DiscV5 uint32


UDP ports are uint16.

fjl · 2017-12-01T19:18:42Z

p2p/enr/discv5.go

+		return err
+	}
+	return nil
+}


You can remove rlp encoding methods from Discv5 because the codec 'sees through' named types and encodes them correctly.

Please rename DiscV5 -> DiscPort.

fjl · 2017-12-01T19:19:45Z

p2p/enr/enr.go

+}
+
+type pair struct {
+	k []byte


Package rlp supports strings. Please make k a string to avoid conversions.

fjl · 2017-12-01T19:20:11Z

p2p/enr/id.go

+		return err
+	}
+	return nil
+}


You can remove rlp encoding methods from ID.

fjl · 2017-12-01T19:27:31Z

p2p/enr/secp256k1.go

+		return err
+	}
+	return nil
+}


Please use

type Secp256k1 ecdsa.PublicKey

You already need to parse the public key for signature verification, might as well do it at decoding time. The additional advantage will be that users can't get invalid curve points by accident.

fjl · 2017-12-01T19:30:42Z

p2p/enr/enr.go

+
+	id := ID(ID_SECP256k1_KECCAK)
+
+	r.Set(id)


You can be less verbose and write r.Set(ID(SECP256k1_KECCAK)).

fjl · 2017-12-01T20:31:44Z

For your convenience, output of make run from p2p-drafts/testvec should be a lot more helpful now ;).

fjl · 2017-12-05T14:13:48Z

I'll re-review when the rest of the boxes are checked ;)

nonsense · 2017-12-05T14:19:49Z

@fjl great, will ping you when the TODO list is addressed.

nonsense · 2017-12-06T14:06:49Z

@fjl @zsfelfoldi I think I addressed all comments and TODOs. This should be ready for review now.

RayPylant

excellent

RayPylant

excellent

…instead of crypto

Nobody will ever check the return value of Set. Make it panic to avoid hidden bugs.

…m key/values

API changes: - Load no longer returns (bool, error), just error. Missing keys can be detected using IsNotFound. - Load errors contain the key that the caller tried to load. - NodeAddr doesn't return error anymore. I expect we'll be using this method often and it will never return an error for records decoded from RLP. Maybe it should panic if "secp256k1" doesn't exist, not sure about that yet. - The Signed method can be used to check for a signature.

zsfelfoldi

Beautiful piece of code :) Left a few comments but nothing critical, LGTM.

zsfelfoldi · 2017-12-21T12:11:49Z

p2p/enr/keys.go

+// To define a new key that is to be included in a node record,
+// create a Go type that satisfies this interface. The type should
+// also implement rlp.Decoder if additional checks are needed on the value.
+type Key interface {


This name confused me at first, I thought it is just the key from the key/value pair. Not sure what I'd like to suggest instead though, I would probably call it Entry or Item but I am not good at naming stuff :)

Entry is not a bad name for this interface. I can see how Key could confuse someone new to the package.

I just pushed a commit that renames Key to Entry where I found it to actually mean Entry, rather than key from a key/value pair. I am not 100% convinced the new version is better, probably because I am already used to the Key interface, but I don't have strong opinion on the name.

@fjl @zsfelfoldi let me know what you think, I am fine with both names to be honest.

zsfelfoldi · 2017-12-21T12:16:27Z

p2p/enr/enr.go

+// Set adds or updates the given key in the record.
+// It panics if the value can't be encoded.
+func (r *Record) Set(k Key) {
+	r.signature = nil


Shouldn't this also invalidate r.raw? (probably you are only checking r.signature for nil but maybe it would be nicer to not leave an invalid r.raw there)

I agree - we should invalidate it - someone could try to decode the record, after having set a key and changed its contents. Pushed a change.

zsfelfoldi · 2017-12-21T12:21:06Z

p2p/enr/keys.go

+// DiscPort is the "discv5" key, which holds the UDP port for discovery v5.
+type DiscPort uint16
+
+func (v DiscPort) ENRKey() string { return "discv5" }


Why not just "port" or "udp"? Discovery v5-specific port will go away soon, the UDP port used by discovery will not depend on the version of the discovery protocol so we should not name this field after that.

I honestly don't know enough about the roadmap to comment on that. Depending on how soon is soon, and when exactly ENR will be integrated with the rest of the codebase, we can decide whether to keep this DiscPort entry type, or remove it and just add Port.

I guess we can postpone this decision until Discovery v5-specific port actually goes away?

nonsense · 2017-12-21T16:33:18Z

@zsfelfoldi I think I addressed your comments. @fjl please review the Key rename to Entry and if you are not happy with it, feel free to just revert the commit. I think Entry reflects slightly better the semantics of the interface.

karalabe · 2017-12-21T17:55:31Z

Please bump the copyright headers to 2017 :)

fjl · 2017-12-21T23:57:27Z

The interface is called Key because it assigns the key to a key/value pair (that's what the method returns). I'll know if Entry is right after christmas. It doesn't really matter though.

Initial implementation of ENR according to ethereum/EIPs#778

nonsense requested review from fjl and zsfelfoldi as code owners November 30, 2017 14:58

nonsense commented Nov 30, 2017

View reviewed changes

nonsense force-pushed the enr branch from fd3e5b8 to 04d1a66 Compare November 30, 2017 16:20

nonsense commented Nov 30, 2017

View reviewed changes

fjl requested changes Dec 1, 2017

View reviewed changes

nonsense force-pushed the enr branch from 16e32cf to d982be1 Compare December 5, 2017 12:04

fjl force-pushed the enr branch from d982be1 to 55bbcd1 Compare December 5, 2017 12:48

fjl approved these changes Dec 5, 2017

View reviewed changes

nonsense force-pushed the enr branch from e7675be to f1a3118 Compare December 6, 2017 14:02

RayPylant approved these changes Dec 8, 2017

View reviewed changes

nonsense and others added 11 commits December 20, 2017 14:01

p2p/enr: initial implementation

d94202c

p2p/enr: changed api; removed wrapList hack;

992c80e

p2p/enr: using btcec library for signing and verifying of signature, …

893152e

…instead of crypto

p2p/enr: add licenses

74004d3

p2p/enr: remove redundant code and comments

0315eec

p2p/enr: update r.raw on DecodeRLP

74eda00

p2p/enr: remove redundant rlp encoding

a2572e3

p2p/enr: Secp256k1 to ecdsa.PublicKey

d960c08

p2p/enr: sort pairs prior to RLP encoding; binary search on Load

bd9696c

p2p/enr: all predefined keys in one file

e490233

p2p/enr: use compact signature encoding

12ac6cd

fjl and others added 17 commits December 20, 2017 14:01

p2p/enr: simplify Set

260989e

p2p/enr: make Set panic for encoding errors

f65565e

Nobody will ever check the return value of Set. Make it panic to avoid hidden bugs.

p2p/enr: don't drop encoding errors during signing

172ff10

p2p/enr: check for size limit in Sign; WithKey(k, v) for adding rando…

301ac8b

…m key/values

p2p/enr: test with random key/values

c219a2a

p2p/enr: improved documentation.

3b304ff

p2p/enr: use SizeLimit const instead of hardcoded 300 bytes.

86acc5d

p2p/enr: fix WithKey deserialisation; fix random tests

6eeb127

p2p/enr: size limit comment

0206b31

p2p/enr: binary search on Set

617f9e6

p2p/enr: check size limit when decoding

67301a1

p2p/enr: change record.Seq type from uint32 to uint64 as per EIP

7392363

p2p/enr: fixup tests

d97b48d

p2p/enr: fix one more IP conversion in tests

dd97fb7

p2p/enr: avoid using btcec directly

08cdd66

p2p/enr: update python test

9474f32

fjl force-pushed the enr branch from 14047d1 to 9474f32 Compare December 20, 2017 13:07

p2p/enr: add BenchmarkDecode

5c55fe7

zsfelfoldi approved these changes Dec 21, 2017

View reviewed changes

nonsense added 2 commits December 21, 2017 14:47

p2p/enr: invalidate r.raw on Set

50b3ab6

p2p/enr: rename Key to Entry

544671f

p2p/enr: bump headers to 2017

607b91f

fjl merged commit 36a1087 into ethereum:master Dec 29, 2017

karalabe added this to the 1.8.0 milestone Dec 29, 2017

5chdn mentioned this pull request Jan 30, 2018

Merge go-ethereum updates into 2.5.4 Musicoin/go-musicoin#77

Merged

mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018

p2p/enr: initial implementation (ethereum#15585)

94cc498

Initial implementation of ENR according to ethereum/EIPs#778

p2p/enr: initial implementation #15585

p2p/enr: initial implementation #15585

Conversation

nonsense commented Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

fjl commented Dec 1, 2017

nonsense commented Dec 1, 2017

fjl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl Dec 1, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

fjl Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl commented Dec 1, 2017

fjl commented Dec 5, 2017

nonsense commented Dec 5, 2017

nonsense commented Dec 6, 2017

RayPylant left a comment

Choose a reason for hiding this comment

RayPylant left a comment

Choose a reason for hiding this comment

zsfelfoldi left a comment

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

Choose a reason for hiding this comment

nonsense Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nonsense commented Dec 21, 2017

karalabe commented Dec 21, 2017

fjl commented Dec 21, 2017 • edited Loading

nonsense commented Nov 30, 2017 •

edited

Loading

fjl left a comment •

edited

Loading

fjl Dec 1, 2017 •

edited

Loading

fjl left a comment •

edited

Loading

fjl Dec 1, 2017 •

edited

Loading

fjl Dec 1, 2017 •

edited

Loading

fjl Dec 1, 2017 •

edited

Loading

nonsense Dec 21, 2017 •

edited

Loading

fjl commented Dec 21, 2017 •

edited

Loading