Add compare_content function to Enr to allow comparisons modulo signature

[KolbyML]

I want an easy way to check if 2 Enr's are equal to each other.

Doing enr1 == enr2 doesn't work because you can have the same Enr with different signatures because of the random nouce I believe.

1Enr { id: Some("v4"), seq: 1, NodeId: 0x7c7c8dea6ad400ec9d52fec8e896484b5cb482e224e96d26fed48dc8b4351760, signature: "59a6e974107d34db2352a6518f38923d757f8c0fa16509de928c623ce3a5f472708ada60389170c9b58412595a9014d1e58c95e7cfc7a0bd61646f7679bbf694", IpV4 UDP Socket: Some(0.0.0.0:4444), IpV6 UDP Socket: None, IpV4 TCP Socket: None, IpV6 TCP Socket: None, Other Pairs: [("c", "967420302e312e312d616c7068612e312d336134623963"), ("secp256k1", "a1035d7c15bd8f796805b2d03d7bef6686b65659e0f2d3fc8163f35faf865379bc13")] }
2Enr { id: Some("v4"), seq: 1, NodeId: 0x7c7c8dea6ad400ec9d52fec8e896484b5cb482e224e96d26fed48dc8b4351760, signature: "9c6e457408f90025d5df6dd2e0c7e3f38496d79284b0af77cc0107ae1a23a61523177fd0de927e15cfd40255b658d4b315d5df85cdc3aa61946770ef1dfda2ea", IpV4 UDP Socket: Some(0.0.0.0:4444), IpV6 UDP Socket: None, IpV4 TCP Socket: None, IpV6 TCP Socket: None, Other Pairs: [("c", "967420302e312e312d616c7068612e312d336134623963"), ("secp256k1", "a1035d7c15bd8f796805b2d03d7bef6686b65659e0f2d3fc8163f35faf865379bc13")] }
3Enr { id: Some("v4"), seq: 1, NodeId: 0x7c7c8dea6ad400ec9d52fec8e896484b5cb482e224e96d26fed48dc8b4351760, signature: "ff3e36da2e8b69780cbf22b4d9699237b2d23b3a977fab85b5bdd3dd931f40a325a606fcf50758756bd533f057e51093e8a426d426da821b7029de92acf62bf7", IpV4 UDP Socket: Some(0.0.0.0:4444), IpV6 UDP Socket: None, IpV4 TCP Socket: None, IpV6 TCP Socket: None, Other Pairs: [("c", "967420302e312e312d616c7068612e312d336134623963"), ("secp256k1", "a1035d7c15bd8f796805b2d03d7bef6686b65659e0f2d3fc8163f35faf865379bc13")] }', portalnet/src/discovery.rs:509:9

Here is an example all 3 of these Enr's are the same except for the signatures, but all of the signatures are valid you could swap them and it wouldn't make a difference.

Currently there is no way to quickly check if 2 Enr's are the same because of this, without manually checking all the content. I don't want to manually do it ideally I could just do.

if enr1.verify_by_signature(enr2.signature) && enr2.verify_by_signature(enr1.signature) {
    todo:
}

or if we fix PartialEq will work like this for the edge case posted above

if enr1 == enr2 {
    todo:
}

I think PartialEq should work like this as well as comparing the signatures isn't a valid way to tell if the Enr's are the same or not.

I want to be able to do if enr1 == enr2 and it actually be a valid if case. Without the edge case I pointed to above, because right now doing enr1 == enr2 just gives false information.

I think PartialEq should be instead like

impl<K: EnrKey> PartialEq for Enr<K> {
    fn eq(&self, other: &Self) -> bool {
        self.seq == other.seq && self.node_id == other.node_id && self.verify_by_signature(&other.signature) == other.verify_by_signature(&self.signature)
    }
}

As this would solve the edge case ^

impl<K: EnrKey> Hash for Enr<K> {
    fn hash<H: Hasher>(&self, state: &mut H) {
        self.seq.hash(state);
        self.node_id.hash(state);
        // since the struct should always have a valid signature, we can hash the signature
        // directly, rather than hashing the content.
        self.signature.hash(state);
    }
}

Here ^ we should not use the signature in the hash and instead just hash the RLP_CONTENT from that function. Since signatures are not deterministic as stated above.

[KolbyML]

@AgeManning @divagant-martian please review

[divagant-martian]

Thanks, I'll get to this monday night. Could you please add a test that creates the differing Enrs and then compares them?

[KolbyML]

Thanks, I'll get to this monday night. Could you please add a test that creates the differing Enrs and then compares them?

Sounds good I wrote the tests for both secpk256k1 && ed25519.

The edge I posted about above looks like it only exists on secpk256k1. ed25519 signatures are deterministic it seems.

[AgeManning]

Hey,
I can see the issue. I recall there was a PR to add this non-determinism into the secp256k1 keys for correct security. There was a discussion about the non-equality at the time I think.

I have a few thoughts on this.

I think its a good idea to add a function that is like compare_content(&self, other: &Enr). And this would give you the functionality you are chasing.

I'm hesitent to change the PartialEq and Hash traits however, because these ENRs are actually different. If we change the PartialEq and Hash traits, then they become indistinguishable. In hashmaps, if we have these two different ENRs they can replace each other undetected and if there's multiple versions floating in a DHT they could oscillate around in people's hashmaps as indistinguishable entities. I'm not sure its possible, but maybe there is some security risk in not checking signatures when over-writing other ENRs in our hashmaps, like maybe invalid sigs can get in there (I can't think of how atm).

I would prefer that we still maintain the ability to distinguish between these ENRs. One of the reasons is that I would consider it a small bug or issue if someone is signing the same content over and over again without increasing the sequence number. Or just signing the same content using sequence 0 if there's a chance that ENR is already out in the wild. I'm not sure about your particular use-case, but I'd think it shouldn't be common practice to sign ENR contents if they have already be signed in the past.

In most of the code i've used with ENRs we just check the seq-no and node-id then use the largest seq-no (regardless of anything else). Its up to the node doing the signing that the most up-to-date information is contained in the highest sequence number it has signed.

If we keep them indistinguishable, then the differences can be detected in the network and we can find the source of the issue and correct it. If they are identical (via the PartialEq and Hash traits) then this becomes a lot harder.

Would adding a compare_content function or equivalent resolve your issues?

[KolbyML]

Yeah that would be fine, I will change my PR to what you suggested when I get a chance.

[KolbyML]

@AgeManning ok I implemented your suggestion!

[divagant-martian]

Thanks @KolbyML. I asked @AgeManning to give this one a look because the changes while reasonable also didn't feel quite right to me. This middle ground is perfect

Only missing getting CI green and we can merge

[divagant-martian]

@KolbyML why draft? anything else you plan to add?

[KolbyML]

@divagant-martian I was just double checking something, It looks good.