feat: implement ping extensions #1616

KolbyML · 2024-12-27T21:47:35Z

What was wrong?

implements ethereum/portal-network-specs#348

How was it fixed?

by implementing it

njgheorghita

First pass, mostly thoughts on layout / docstrings. IIUC this is a breaking change, right? If so, what's the rollout plan?

njgheorghita · 2025-01-09T15:07:54Z

ethportal-api/src/types/ping_extensions/extensions/type_0.rs

+        assert_eq!(client_info, decoded);
+    }
+
+    #[test]


Seems like you could use rstest for these fail cases, and just add inline comments to the individual cases explaining why they should fail

njgheorghita · 2025-01-09T15:09:51Z

ethportal-api/src/types/ping_extensions/extensions/type_1.rs

+    }
+}
+
+impl From<BasicRadius> for CustomPayload {


Seems like a fairly simple type, but I still feel like we should have some pass/fail test cases for type_1 & type_2 * type_65535

I added a few test cases let me know if you want any specific test I didn't think of

njgheorghita · 2025-01-09T15:13:14Z

ethportal-api/src/types/ping_extensions/mod.rs

+pub mod decode;
+pub mod extensions;
+
+use ssz::Decode;


Idk... this is a bit of a nitpick, bc I'm pretty sure we're not consistent with this throughout the codebase, but personally i'm not a fan of having logic/defining types inside the mod.rs file. Imo, it makes navigation more difficult. eg. when the logic is in a specified module, then I know exactly where to go to see the source.

Same comment applies to your file change -> overlay/service/mod.rs. I'd rather see that logic in a named module

Same comment applies to your file change -> overlay/service/mod.rs. I'd rather see that logic in a named module

I can move the impl out of mod, but I think the struct must stay in mod or the impl wouldn't be able to access the structs private with doing pub to all them

I think the struct has to go in mod yeah

So to split the impl across files, I think the struct must be in the mod, or it becomes very messy and asks me to make the struct's contents public.

If you have an idea which works where we could cleanly do this let me know

njgheorghita · 2025-01-09T15:28:12Z

ethportal-api/src/types/ping_extensions/extensions/type_0.rs

+        let parts: Vec<&str> = string.split('/').collect();
+
+        if parts.len() != 4 {
+            bail!("Invalid client info string: should have 4 /'s {}", string);


I would also include how many parts were actually found in the string (parts.len())

njgheorghita · 2025-01-09T15:30:03Z

portalnet/src/overlay/ping_extensions.rs

+use ethportal_api::types::ping_extensions::Extensions;
+
+pub trait PingExtension {
+    fn is_supported(&self, extension: Extensions) -> bool;


docstring this and & raw_extensions

njgheorghita · 2025-01-09T15:30:45Z

portalnet/src/overlay/ping_extensions.rs

+
+    /// Returns the newest extension that is supported by both clients, used for extended ping
+    /// responses.
+    fn newest_commonly_supported_base_extension(


nitpick latest_mutually_supported_base_extension

njgheorghita · 2025-01-09T15:35:26Z

portalnet/src/overlay/service/ping.rs

+    types::node::Node,
+};
+
+impl<


KolbyML · 2025-01-09T15:45:25Z

this is a breaking change, right?

Yes

If so, what's the rollout plan?

Currently our protocol doesn't have a way to deal with this, hopefully we define something in the specification soon!

So I think it is shipping breaking code, and I really want a defined way for shipping breaking change to the network 😭

KolbyML · 2025-01-10T16:59:59Z

@njgheorghita ready for another review

njgheorghita

Looks good! Mostly nits. Though, I'd say this is blocked for merging until we get at least fluffy (and probably ultralight as well, since it sounds like they're not that far away) up to spec, and then we'll coordinate the release together

njgheorghita · 2025-01-14T20:54:43Z

crates/ethportal-api/src/types/ping_extensions/custom_payload_format.rs

+
+    fn try_from(value: CustomPayload) -> Result<Self, Self::Error> {
+        CustomPayloadExtensionsFormat::from_ssz_bytes(&value.payload)
+            .map_err(|e| anyhow::anyhow!("Failed to decode CustomPayloadExtensionsFormat: {:?}", e))


nit use the var inside the string. though, this is the kind of nit I would expect clippy to pick up? seems like the rule is enabled https://github.com/ethereum/trin/blob/master/crates/ethportal-api/src/lib.rs#L5C1-L5C40 ... maybe clippy's structure isn't compatible/needs updating with the new crates/ layout?

also another nit importanyhow::anyhow at the top rather than importing it here

njgheorghita · 2025-01-14T20:56:49Z

crates/ethportal-api/src/types/ping_extensions/decode.rs

+                let capabilities =
+                    ClientInfoRadiusCapabilities::from_ssz_bytes(&ping_custom_payload.payload)
+                        .map_err(|err| {
+                            anyhow::anyhow!(


same with the anyhow import in this file

njgheorghita · 2025-01-14T21:00:19Z

crates/ethportal-api/src/types/ping_extensions/extensions/type_65535.rs

+    portal_wire::CustomPayload,
+};
+
+/// Used to response to pings which the node can't handle


nit Used to respond to ...

njgheorghita · 2025-01-14T21:01:21Z

crates/portalnet/src/overlay/ping_extensions.rs

+        extensions: &[Extensions],
+    ) -> Option<Extensions>;
+
+    /// Returns the extensions by there u16 type id that are supported by the clients subnetwork.


nit Returns the extensions by their u16...

njgheorghita · 2025-01-14T21:01:51Z

crates/portalnet/src/overlay/protocol.rs

-        )
-        .await;
+        let command_tx =
+            OverlayService::<TContentKey, TMetric, TValidator, TStore, TPingExtensions>::spawn(


Jeez, this probably needs some refactoring at some point

njgheorghita · 2025-01-14T21:04:28Z

crates/portalnet/src/overlay/service/ping.rs

+                    protocol = %self.protocol,
+                    request.source = %source,
+                    request.discv5.id = %request_id,
+                    "Received invalid Ping message, Error's should only be received from pong",


nit Errors...

njgheorghita · 2025-01-14T21:09:06Z

crates/portalnet/src/utils/db.rs

@@ -9,10 +9,10 @@ use discv5::enr::{CombinedKey, Enr, NodeId};
 use ethportal_api::{
    types::network::Network,
    utils::bytes::{hex_decode, hex_encode},
+    version::APP_NAME,


Hmmm. While you're at it... git grep -i '\"trin\"' reveals quite a few other places in the codebase where we just hardcode "trin", it could be a quick / worthwhile refactor to update those as well in a future pr, but it's nothing urgent / important.

njgheorghita · 2025-01-14T21:11:43Z

crates/portalnet/src/overlay/service/ping.rs

+    types::node::Node,
+};
+
+/// Implementation of the `OverlayService` for handing Ping and Pong Extension's.


nit handling & Extensions

njgheorghita · 2025-01-14T21:14:13Z

crates/portalnet/src/overlay/service/ping.rs

+                warn!(
+                    protocol = %self.protocol,
+                    request.dest = %node_id,
+                    "Base extension wasn't implemented: {extension:?}, sending Capabilities instead this is a bug",


... sending Capabilities instead. This is a bug!

njgheorghita · 2025-01-14T21:20:44Z

crates/portalnet/src/overlay/service/ping.rs

+        let _ = self.command_tx.send(OverlayCommand::Request(request));
+    }
+
+    fn handle_capabilities(


In general, I feel like there's some room for improvement in how this file is organized. It just seems a bit spaghetti-y to me. It's not a blocker for this pr, and tbh I don't have any good ideas off the top of my head (our overlay service is so spaghetti-y already that maybe there aren't any great options). But at the very least it seems like the last 3 methods in this file don't use the self attribute, so maybe they would be better off somewhere else? IDK, leaving it up to you to evaluate.

KolbyML changed the title ~~Implement ping extensions~~ feat: implement ping extensions Dec 27, 2024

KolbyML force-pushed the implement-ping-extensions branch 3 times, most recently from 5febe0e to efd0328 Compare December 28, 2024 09:45

KolbyML force-pushed the implement-ping-extensions branch 4 times, most recently from 6b46f62 to d416970 Compare January 8, 2025 13:21

KolbyML marked this pull request as ready for review January 8, 2025 13:44

KolbyML requested review from carver, njgheorghita, ogenev and morph-dev January 8, 2025 13:44

KolbyML self-assigned this Jan 8, 2025

njgheorghita reviewed Jan 9, 2025

View reviewed changes

KolbyML force-pushed the implement-ping-extensions branch from 596bde7 to 259dcaa Compare January 13, 2025 18:43

feat: implement ping extensions

d12c90a

KolbyML force-pushed the implement-ping-extensions branch from 259dcaa to d12c90a Compare January 13, 2025 18:53

njgheorghita approved these changes Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement ping extensions #1616

feat: implement ping extensions #1616

KolbyML commented Dec 27, 2024 •

edited

Loading

njgheorghita left a comment

njgheorghita Jan 9, 2025

njgheorghita Jan 9, 2025

KolbyML Jan 10, 2025

njgheorghita Jan 9, 2025

KolbyML Jan 10, 2025 •

edited

Loading

KolbyML Jan 10, 2025 •

edited

Loading

njgheorghita Jan 9, 2025

njgheorghita Jan 9, 2025

njgheorghita Jan 9, 2025

njgheorghita Jan 9, 2025

KolbyML commented Jan 9, 2025 •

edited

Loading

KolbyML commented Jan 10, 2025

njgheorghita left a comment

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

njgheorghita Jan 14, 2025

feat: implement ping extensions #1616

Are you sure you want to change the base?

feat: implement ping extensions #1616

Conversation

KolbyML commented Dec 27, 2024 • edited Loading

What was wrong?

How was it fixed?

njgheorghita left a 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 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

KolbyML Jan 10, 2025 • 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

KolbyML commented Jan 9, 2025 • edited Loading

KolbyML commented Jan 10, 2025

njgheorghita left a 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

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 Dec 27, 2024 •

edited

Loading

KolbyML Jan 10, 2025 •

edited

Loading

KolbyML Jan 10, 2025 •

edited

Loading

KolbyML commented Jan 9, 2025 •

edited

Loading