fix: enr signature changes on restart even if enr content is the same #952

KolbyML · 2023-10-03T20:14:29Z

What was wrong?

I made a PR sigp/enr#53 and Age Manning gave me the insight that the signature should always be the same in the client if the content is the same.

We had an issue before my increase seq pr and after that even though our enr could be the same, the signature would change which isn't good.

How was it fixed?

It was fixed by restructuring a bit of my code. I also cleaned it up a bit for how to compare them from suggestions from Age and Nick.

I also resolved a param name nitpik nick had in my last PR.

divagant-martian · 2023-10-03T22:21:42Z

portalnet/src/discovery.rs

@@ -425,7 +430,7 @@ impl AsyncUdpSocket<UtpEnr> for Discv5UdpSocket {

 // todo: remove this once sigp/enr implements this for enr's
 // we need this because signatures can be different for the same data but still valid
-fn verify_by_signature(enr: &Enr, signature: &[u8]) -> bool {
+fn get_enr_rlp_content(enr: &Enr) -> BytesMut {


@KolbyML I don't think there is any harm in exposing rlp_content from Enr

When we get the update Enr version we can just use compare_content. If we want to expose rlp_content() that is fine too. It just looked like rlp_content() was intentionally privated so that is why I tried to get a compare/verify_by_signature* function added. Cause I didn't want to manually calculate rlp_content in multiple codebases.

Anyways up to you. I didn't know making the rlp_content() function public was an option tbh.

I do think compare_content is cleaner though
enr1.compare_content(&enr2)
vs
enr1.rlp_content() == enr2.rlp_content()

Neither is a problem. Nothing wrong with exposing it, this does not change the enr and it's not private info in any way

njgheorghita · 2023-10-04T19:49:18Z

portalnet/src/types/messages.rs

@@ -182,7 +182,7 @@ impl Default for PortalnetConfig {
            data_radius: Distance::MAX,
            internal_ip: false,
            no_stun: false,
-            enr_file_location: None,
+            trin_data_dir: PathBuf::default(),


Well, sorry to be a stickler, but this still feels off. Essentially we're still doing the same thing I was previously against, except this time with PathBuf::default() instead of an Option

Ultimately, it feels like PortalnetConfig is not an appropriate place for this argument, since there is no reasonable default value.

I think we just remove this from PortalnetConfig and pass node_data_dir into Discovery::new() directly. Thoughts?

njgheorghita · 2023-10-04T19:53:09Z

portalnet/src/discovery.rs

+        // This should only never run in test cases.
+        if portal_config.trin_data_dir != PathBuf::default() {
+            // Check if we have an old version of our Enr and if we do, increase our sequence number
+            let trin_enr_file_location = portal_config.trin_data_dir.join(ENR_FILE_NAME);


I would change to trin_enr_path - basically, _path suffix to these types of variable names describing a file path is what we typically do in this codebase, and I think we should continue with that convention to maintain consistency

trin/portal-bridge/src/cli.rs

Line 142 in 040ad41

const EXECUTABLE_PATH: &str = "path/to/executable";

trin/portal-bridge/src/bridge.rs

Line 50 in 040ad41

pub epoch_acc_path: PathBuf,

trin/rpc/src/lib.rs

Line 74 in 040ad41

.web3_ipc_path

njgheorghita · 2023-10-04T19:54:07Z

portalnet/src/discovery.rs

-        // Check if we have an old version of our Enr and if we do, increase our sequence number
-        if let Some(enr_file_location) = portal_config.enr_file_location {
-            let trin_enr_file_location = enr_file_location.join(ENR_FILE_NAME);
+        // This should only never run in test cases.


// This should never run in test cases. - much less confusing in my opinion. But also the fact that this check needs to happen feels off. If we go with my suggestion about node_data_dir then we can just get rid of this check completely

The issue I have with this is some Discovery tests don't use a path, but I cause I can just generate a tmp path for those

KolbyML · 2023-10-04T20:40:43Z

@njgheorghita I believe I resolved your concerns let me know what you think

njgheorghita

Nice! Yeah, it's not ideal to create all those temp dirs in the tests but in the long run I think we're left off with better / easier to read production code this way

njgheorghita · 2023-10-04T21:26:02Z

portalnet/src/discovery.rs

+        if trin_enr_path.is_file() {
+            let data = fs::read_to_string(trin_enr_path.clone())
+                .expect("Unable to read Trin Enr from file");
+            let old_enr = Enr::from_str(&data).expect("Expected read trin.enr to be valid");


nit "Expected to read valid Trin Enr from file"

fix: resolve changing signatures on trin restart

8d2b766

KolbyML requested a review from njgheorghita October 3, 2023 20:14

KolbyML changed the title ~~fix: resolve changing signatures on trin restart~~ fix: resolve changing signatures on trin enr after restart if content didn't change Oct 3, 2023

KolbyML self-assigned this Oct 3, 2023

KolbyML changed the title ~~fix: resolve changing signatures on trin enr after restart if content didn't change~~ fix: changing signatures on trin enr after restart if content didn't change Oct 3, 2023

KolbyML changed the title ~~fix: changing signatures on trin enr after restart if content didn't change~~ fix: enr signature changes on restart even if enr content is the same Oct 3, 2023

Fix test discovery config config edge case

ffa6079

KolbyML marked this pull request as ready for review October 3, 2023 20:32

divagant-martian reviewed Oct 3, 2023

View reviewed changes

Fix test to properly test edge case

1546379

njgheorghita reviewed Oct 4, 2023

View reviewed changes

Resolve pr comments

7cbbfc3

njgheorghita approved these changes Oct 4, 2023

View reviewed changes

fixup

c7f03ac

KolbyML merged commit 80cf8ca into ethereum:master Oct 4, 2023

pipermerriam mentioned this pull request Oct 10, 2023

Add Kolby Moroz Liebl protocolguild/documentation#109

Merged

KolbyML deleted the improve-increase-enr-seq branch January 22, 2025 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: enr signature changes on restart even if enr content is the same #952

fix: enr signature changes on restart even if enr content is the same #952

KolbyML commented Oct 3, 2023

divagant-martian Oct 3, 2023

KolbyML Oct 3, 2023 •

edited

Loading

KolbyML Oct 3, 2023

divagant-martian Oct 3, 2023

njgheorghita Oct 4, 2023

njgheorghita Oct 4, 2023

njgheorghita Oct 4, 2023

KolbyML Oct 4, 2023

KolbyML commented Oct 4, 2023

njgheorghita left a comment

njgheorghita Oct 4, 2023

fix: enr signature changes on restart even if enr content is the same #952

fix: enr signature changes on restart even if enr content is the same #952

Conversation

KolbyML commented Oct 3, 2023

What was wrong?

How was it fixed?

Choose a reason for hiding this comment

KolbyML Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML commented Oct 4, 2023

njgheorghita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KolbyML Oct 3, 2023 •

edited

Loading