implement Ethereum Node Entry, EIP-778 #11354
Conversation
|
It looks like @NamsooCho signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
util/network-devp2p/src/enr.rs
Outdated
| seq: u64, | ||
| signature: Option<Vec<u8>>, | ||
| raw: Option<Vec<u8>>, // RLP encoded record. | ||
| pairs: HashMap<String, Value>, // Sorted list of key/value pairs. |
There was a problem hiding this comment.
Not sure what you mean by sorted list here, but HashMap is an unordered collection if you want sorted by the keys then use BTreeMap instead
util/network-devp2p/src/enr.rs
Outdated
| pub fn set(&mut self, value: &Value) { | ||
| self.invalidate(); | ||
| let val = self.pairs.entry(value.enr_key()).or_insert(value.clone()); | ||
| *val = value.clone(); |
There was a problem hiding this comment.
Seems needless to first write value if it is empty and then replace one more time,
replace line 127-128 with self.pairs.insert(value.enr_key(), value.clone()) since we just overwrite previous values if it's already in the map?
util/network-devp2p/src/enr.rs
Outdated
| } | ||
|
|
||
| /// Add or update a value for a key. | ||
| pub fn set(&mut self, value: &Value) { |
There was a problem hiding this comment.
consider taking Value by value instead because it is intended to be consumed
util/network-devp2p/src/enr.rs
Outdated
|
|
||
| /// Provide custom verify method for each scheme. | ||
| pub trait Scheme { | ||
| fn verify(&self, r: &Record, sig: &Vec<u8>) -> bool; |
There was a problem hiding this comment.
make more generic by taking slice instead of Vec
| fn verify(&self, r: &Record, sig: &Vec<u8>) -> bool; | |
| fn verify(&self, r: &Record, sig: &[u8]) -> bool; |
| /// A node record entry. | ||
| /// A new node record entry should implements this trait. | ||
| pub trait Entry { | ||
| fn enr_key(&self) -> String; |
There was a problem hiding this comment.
Can we replace this with an associated constant since these are just constants for each type that implements it?
In order to avoid allocating a string each time this is called.
There was a problem hiding this comment.
This codes use trait object for Entry type.
rust-lang/rust#26847
There was a problem hiding this comment.
So an Entry is a key-value abstraction, but do we actually use it and need it?
util/network-devp2p/src/enr.rs
Outdated
| } | ||
|
|
||
| /// Holes scheme for some type. | ||
| pub struct SchemeMap<T: Scheme>(HashMap<String, T>); |
There was a problem hiding this comment.
trait bounds on structs are considered a bad practice,
| pub struct SchemeMap<T: Scheme>(HashMap<String, T>); | |
| pub struct SchemeMap<T>(HashMap<String, T>); |
| /// Holes scheme for some type. | ||
| pub struct SchemeMap<T: Scheme>(HashMap<String, T>); | ||
|
|
||
| impl<T: Scheme> SchemeMap<T> { |
There was a problem hiding this comment.
Can you explain the relation between SchemeMap and Record?
There was a problem hiding this comment.
Current ENR scheme is only "v4".
But may be added later.
SchemeMap contains many schemes like "v4" and others.
Record represents one version of scheme.
This implementation supports many version of schemes.
There was a problem hiding this comment.
identity scheme is defined as a predefined id key in the spec
https://eips.ethereum.org/EIPS/eip-778
I'm not sure we need SchemeMap here as well
also see https://github.com/sigp/rust-libp2p/blob/discv5/misc/enr/src/lib.rs
| self.signature.clone() | ||
| } | ||
|
|
||
| pub fn scheme(&self) -> Option<String> { |
There was a problem hiding this comment.
documentation please,
I'm getting confused why this using Id("".to_owned()) only? Does scheme mean Id or what?
There was a problem hiding this comment.
Get the scheme represented by a string.
Since parameter id of get() has a Id type,
get can call enr_key().
So, we just pass it with empty string.
util/network-devp2p/src/enr.rs
Outdated
| self.raw = None; | ||
| } | ||
|
|
||
| pub fn seq(&self) -> u64 { |
There was a problem hiding this comment.
| pub fn seq(&self) -> u64 { | |
| pub fn get_seq(&self) -> u64 { |
util/network-devp2p/src/enr.rs
Outdated
|
|
||
| /// Tcp port of a node | ||
| #[derive(Clone, PartialEq, Debug)] | ||
| pub struct Tcp(u16); |
There was a problem hiding this comment.
All those probably should have pub modifiers or Deref/AsRef implementations. Otherwise the inner value will not be accessible.
|
Nice work @NamsooCho, and just want to note that we have another Rust ENR library available (from |
NamsooCho
force-pushed
the
enr
branch
2 times, most recently
from
6eab8ec to
bff887a
Compare
|
|
||
| /// Value type for KeyValue pairs in Record struct. | ||
| #[derive(Clone, PartialEq, Debug)] | ||
| pub enum Value { |
There was a problem hiding this comment.
I don't think this is correct, the Value should be a Vec<u8>, these types are just predefined keys
maybe it's better to have them as methods on Record?
| seq: u64, | ||
| signature: Option<Vec<u8>>, | ||
| raw: Option<Vec<u8>>, // RLP encoded record. | ||
| pairs: BTreeMap<String, Value>, // Sorted list of key/value pairs. |
There was a problem hiding this comment.
The keys can technically be any byte sequence
Not all bytes sequence are valid utf-8 strings, so let's create a type for keys that wraps Vec<u8>?
Also I think it should implement rlp::{Encodable, Decodable}
| pub struct Record { | ||
| seq: u64, | ||
| signature: Option<Vec<u8>>, | ||
| raw: Option<Vec<u8>>, // RLP encoded record. |
There was a problem hiding this comment.
this is not present in the spec, is that some sort of a cache of encoding? can you elaborate why we need this?
| #[derive(Default)] | ||
| pub struct Record { | ||
| seq: u64, | ||
| signature: Option<Vec<u8>>, |
| /// Holes scheme for some type. | ||
| pub struct SchemeMap<T: Scheme>(HashMap<String, T>); | ||
|
|
||
| impl<T: Scheme> SchemeMap<T> { |
There was a problem hiding this comment.
identity scheme is defined as a predefined id key in the spec
https://eips.ethereum.org/EIPS/eip-778
I'm not sure we need SchemeMap here as well
also see https://github.com/sigp/rust-libp2p/blob/discv5/misc/enr/src/lib.rs
| } | ||
|
|
||
| if s.len() > ENR_MAX_SIZE { | ||
| error!(target: "network", "Encoded bytes exceed limit(300 bytes), length is {:?}", s.len()) |
There was a problem hiding this comment.
we should have a way to return an error instead of just printing a warning
| /// A node record entry. | ||
| /// A new node record entry should implements this trait. | ||
| pub trait Entry { | ||
| fn enr_key(&self) -> String; |
There was a problem hiding this comment.
So an Entry is a key-value abstraction, but do we actually use it and need it?
|
hey @NamsooCho, thanks for the PR, may I ask you why you closed it? |
Implemets EIP-778(Ethereum Node Entry).
This is required to make EIP-2364(My current WIP PR(#11300 ).