sigp / enr

Ethereum Node Record
MIT License
61 stars 29 forks source link

Clarify enr content value format #71

Closed mattsse closed 6 months ago

mattsse commented 6 months ago

This states:

https://github.com/sigp/enr/blob/83cd98990e7f191b310addb202869994a8f59c96/src/builder.rs#L18-L20

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

https://github.com/sigp/enr/blob/83cd98990e7f191b310addb202869994a8f59c96/src/builder.rs#L45-L56

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

https://github.com/sigp/enr/blob/83cd98990e7f191b310addb202869994a8f59c96/src/builder.rs#L159-L162

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)) ?

https://github.com/sigp/enr/blob/83cd98990e7f191b310addb202869994a8f59c96/src/builder.rs#L45-L50

AgeManning commented 6 months ago

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?