Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify enr content value format #71

Closed
mattsse opened this issue Apr 2, 2024 · 1 comment · Fixed by #72
Closed

Clarify enr content value format #71

mattsse opened this issue Apr 2, 2024 · 1 comment · Fixed by #72

Comments

@mattsse
Copy link
Contributor

mattsse commented Apr 2, 2024

This states:

enr/src/builder.rs

Lines 18 to 20 in 83cd989

/// The key-value pairs for the ENR record.
/// Values are stored as RLP encoded bytes.
content: BTreeMap<Key, Bytes>,

This will add any encodable value encoded as rlp to the map:

enr/src/builder.rs

Lines 45 to 56 in 83cd989

/// Adds an arbitrary key-value to the `ENRBuilder`.
pub fn add_value<T: Encodable>(&mut self, key: impl AsRef<[u8]>, value: &T) -> &mut Self {
let mut out = BytesMut::new();
value.encode(&mut out);
self.add_value_rlp(key, out.freeze())
}
/// Adds an arbitrary key-value where the value is raw RLP encoded bytes.
pub fn add_value_rlp(&mut self, key: impl AsRef<[u8]>, rlp: Bytes) -> &mut Self {
self.content.insert(key.as_ref().to_vec(), rlp);
self
}

on build this check ensures that all values are rlp(bytes):

enr/src/builder.rs

Lines 159 to 162 in 83cd989

// Sanitize all data, ensuring all RLP data is correctly formatted.
for value in self.content.values() {
Bytes::decode(&mut value.as_ref())?;
}

on reth, this test currently fails because the value is rlp(forkId)

https://github.com/paradigmxyz/reth/blob/bc2634981e2fdca2d968b1dc63d3ca9a716c93e4/crates/net/discv4/src/proto.rs#L660-L664

If all values should be rlp(bytes) where bytes itself can be rlp(forkid), should add_value then insert self.add_value_rlp(key, alloy_rlp::encode(&out)) ?

enr/src/builder.rs

Lines 45 to 50 in 83cd989

/// Adds an arbitrary key-value to the `ENRBuilder`.
pub fn add_value<T: Encodable>(&mut self, key: impl AsRef<[u8]>, value: &T) -> &mut Self {
let mut out = BytesMut::new();
value.encode(&mut out);
self.add_value_rlp(key, out.freeze())
}

@AgeManning
Copy link
Member

To be generic across all types we store the underlying data as bytes and convert to specific types later one.

The general spec indicates an RLP list of k,v: https://github.com/ethereum/devp2p/blob/master/enr.md#rlp-encoding

In principle, it should work for any object that is RLP encodable. In your test could you not just put the fork_id into add_value() and it will be encoded as every other type that is rlp encodeable and stored in the ENR.

Your suggestion would be to double rlp-encode objects right?
Currently we are just rlp-encoding the types and storing the bytes of the rlp-encoded object, which seems sensible to me.

Unless the .build() fails on normal types, it should be the case that all rlp-bytes can be decoded as bytes right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants