feat: Allow to build an Enr with a public key and an existing signature

[leruaa]

In Reth we need to create an Enr with an existing signature (see https://github.com/paradigmxyz/reth/pull/1394).

[divagant-martian]

This change would allow creating Enrs with invalid signatures, so I don't think this is the way forward.

Regarding this comment in the linked PR

imo this is reasonable because there's already a add_public_key function https://github.com/sigp/enr/blob/master/src/builder.rs#L136

note that this function is private so this is actually not allowed.

If you can share a bit more of your use case we might be able to point to a different solution

[AgeManning]

I also had a quick look through and agree with @divagant-martian. The ENR crate is currently designed so that a user is not able to create invalid ENRs. This change would allow the creation of ENRs which would be registered as invalid by other implementations/users.

Furthermore, having this API and having a user of the ENR crate use it successfully is going to be pretty difficult. The input signature MUST match the RLP encoding of the fields. It should be noted that the ordering of the fields in ambigious. Each implementation can order the fields however they want. So if one implementation signs a bunch of fields, that signature can not necessarily be used in this ENRBuilder unless the exact ordering of the fields matches.

Therefore, the only use case of this API I can see, is for all of the following conditions to be met:

1. You have a bunch of data that looks like an ENR, but is not an ENR.
2. This data is signed, in exactly the same way and ordering as the ENRBuilder of this crate for all the fields
3. You do not have the private key relating to the signature

I would imagine this very specific use case implies you are converting something that looks like an ENR into an ENR. I think in general it would be hard to get 2 to match for all implementations outside of this crate. I would suggest a more general approach. This would be to get the data and encode it into a base64 rlp-encoded ENR. This way, you have control over the ordering of the fields and thus can make the signature work. You can then create and ENR by decoding this base64. The resulting ENR will have the field ordering matching that of the other implementation and it will include the signature (if it is valid and matches the field ordering). This way it will be impossible to create invalid ENRs and incorrect encodings or invalid signatures (wrt to the field ordering) will all be rejected.

[leruaa]

Thanks a lot for your quick and detailed answers!

I will explain more in detail what our use case is: When Reth shutdown, all known peers are persisted into a file. Previously NodeRecords were serialized and persisted, but we want to persists Enrs instead to be able to easily store more info (the announced fork id for instance). So yeah, your guess was right, we want to convert something that looks like an ENR into an ENR.

@mattsse Don't hesitate to correct me if I misunderstood something :)

[mattsse]

thank you for your detailed response. this was very helpful and I agree with that. your suggested solution to save it as base64 makes more sense.

[leruaa]

Would it be acceptable to add build_rlp_encoded_base64() to EnrBuilder? This way we won't have to duplicate all the code that populate the ENR content field, and just call something like that:

let mut builder = EnrBuilder::new("v4");

builder.ip(self.address);

if self.address.is_ipv4() {
    builder.udp4(self.udp_port);
    builder.tcp4(self.tcp_port);
} else {
    builder.udp6(self.udp_port);
    builder.tcp6(self.tcp_port);
}

let base64 = builder.build_rlp_encoded_base64(public_key, signature);

[divagant-martian]

Using the builder to do encoding is very much out of scope for this struct. This crate already provides a way to do the decoding to Enr via FromStr so you will need to implement this on your project's side. Beside, doing it in that way still has the issue that it adds functionality to create values that might be invalid since you are still giving the signature as a parameter

[leruaa]

Understood, we will impl this on our side.

[AgeManning]

@divagant-martian has pointed out to me that the spec actually specifies the field ordering. My memory tells me that this wasn't always the case. I do recall having to make the rlp_content a separate field to handle the ordering from other implementations (I guess they all agree now).

But if the specification says that the field ordering is fixed, then my points about that are invalid and all implementations should have a fairly standard signature, so you don't have to mess around with the ordering. It should be fairly straight forward to do the base-64 encoding by borrowing a some of the code in this crate. Let us know if you need any help :)

[gakonst]

Just read thru - appreciate your guys' detailed responses. We'll impl on our side.