Add overlay service to dispatch and handle requests #160

jacobkaufmann · 2021-10-26T23:47:44Z

Related to #135. Move message dispatch and handling into OverlayService. The Overlay communicates with the associated OverlayService via a channel.

Motivation: Autonomous routing table management requires a separate task where the task must have a mutable reference to the overlay structure. We create a new overlay structure, OverlayService, and communication between this structure and Overlay occurs over a channel, which does not require a reference.

jacobkaufmann · 2021-10-27T20:48:50Z

trin-core/src/portalnet/discovery.rs

@@ -166,7 +166,7 @@ impl Discovery {
        Ok(())
    }

-    pub async fn send_talkreq(
+    pub async fn send_talk_req(


Note that this renaming was done to be consistent with discv5.

jacobkaufmann · 2021-10-27T20:49:42Z

trin-core/src/portalnet/overlay_service.rs

+
+/// A node in the overlay network routing table.
+#[derive(Clone)]
+pub struct Node {


Not sure if it would be better for this to go in types.rs.

Personally, I'm not a big fan of using types.rs and think this makes sense to belong here.

jacobkaufmann · 2021-10-27T20:52:07Z

trin-core/src/portalnet/overlay_service.rs

+
+/// A general overlay error.
+#[derive(Error, Debug)]
+pub enum OverlayError {


It looks like there are relevant new error types defined in #163 and #164.

njgheorghita · 2021-10-29T00:09:56Z

@jacobkaufmann Do you mind rebasing this off master, and I'll dig into it tomorrow

jacobkaufmann · 2021-10-29T00:54:22Z

@njgheorghita sure, no problem. I'm not sure if you see anything that appears out of scope for this PR. I tried to keep it small, but overlay touches a lot of things.

njgheorghita · 2021-10-29T19:27:37Z

@jacobkaufmann Yeah, it's a bit large but also sometimes that's unavoidable. It also seem like it covers some of the same territory as #163, #164 - so maybe wait until those land (which should be soon) and then either rebase off them, or leave your changes here if you feel like it's a more suitable error structure (which very well might be the case).

njgheorghita

Looks great! Nice work, just one question, so it's mentioned that the overlay routing table maintenance / liveness tasks are done autonomously - which I interpret as handled within discv5 - so my understanding here is that the maintenance / liveness tasks are done over the base discv5 protocol? This seems fine for PING, but are FINDNODES lookup/bootstrapping messages part of the autonomous management at all? If so, and those are going out over the base discv5 protocol, that would be corrupt the routing table

ethportal-peertest/src/events.rs

ethportal-peertest/src/main.rs

trin-history/src/events.rs

njgheorghita · 2021-11-02T13:40:36Z

trin-core/src/portalnet/overlay_service.rs

+
+/// A node in the overlay network routing table.
+#[derive(Clone)]
+pub struct Node {


Personally, I'm not a big fan of using types.rs and think this makes sense to belong here.

jacobkaufmann · 2021-11-02T21:08:44Z

Looks great! Nice work, just one question, so it's mentioned that the overlay routing table maintenance / liveness tasks are done autonomously - which I interpret as handled within discv5 - so my understanding here is that the maintenance / liveness tasks are done over the base discv5 protocol? This seems fine for PING, but are FINDNODES lookup/bootstrapping messages part of the autonomous management at all? If so, and those are going out over the base discv5 protocol, that would be corrupt the routing table

@njgheorghita I think you are referring to the comment above OverlayService::spawn? So maybe the remark about "autonomous" processes here is premature, because those processes are not implemented in this PR. If you see #135 in the overlay/service.rs file, the main loop performs pings based on the ping queue, (#139 introduces the necessary data structure for the ping queue), and the loop also performs some maintenance tasks on the routing table for previously applied pending entries.

These processes are considered "autonomous" because we do not need to invoke a method on OverlayService for them to occur. We just have to spawn the service. Also, these processes do not occur over discv5 PING or FINDNODES messages. The ping messages that are sent are overlay PING messages, and the node updates are done via overlay FINDNODES messages that contain the latest ENR.

I hope this clarifies things. If the autonomous comment is confusing without the implementation of those processes, then I'm happy to remove it. And if anything else deserves some further elaboration, then I can add additional comments.

njgheorghita · 2021-11-03T22:30:40Z

@jacobkaufmann Ahh cool, yeah that makes perfect sense, thanks!

ogenev · 2021-11-04T14:31:54Z

ethportal-peertest/src/events.rs

@@ -49,14 +46,26 @@ impl PortalnetEvents {
            match std::str::from_utf8(request.protocol()) {
                Ok(protocol) => match protocol {


FYI this match will not work with the latest ProtocolId update in #162. I'll create an issue to fix this in a separate PR.

jacobkaufmann · 2021-11-05T16:31:20Z

I think I've addressed all the comments. Waiting on #176, which @ogenev created to address the comment above, to land before I merge to ensure there are no conflicts.

jacobkaufmann commented Oct 27, 2021

View reviewed changes

jacobkaufmann marked this pull request as ready for review October 27, 2021 20:53

jacobkaufmann requested a review from njgheorghita October 27, 2021 20:53

jacobkaufmann force-pushed the overlay-service branch 2 times, most recently from 36f62cb to e9df7e8 Compare November 2, 2021 01:54

njgheorghita approved these changes Nov 2, 2021

View reviewed changes

ogenev reviewed Nov 4, 2021

View reviewed changes

jacobkaufmann force-pushed the overlay-service branch from e9df7e8 to ef69ce4 Compare November 5, 2021 16:18

Add overlay service to dispatch and handle requests

1f3c954

jacobkaufmann force-pushed the overlay-service branch from ef69ce4 to 1f3c954 Compare November 8, 2021 16:04

jacobkaufmann merged commit 00a9479 into ethereum:master Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overlay service to dispatch and handle requests #160

Add overlay service to dispatch and handle requests #160

jacobkaufmann commented Oct 26, 2021

jacobkaufmann Oct 27, 2021

jacobkaufmann Oct 27, 2021

njgheorghita Nov 2, 2021

jacobkaufmann Oct 27, 2021

njgheorghita commented Oct 29, 2021

jacobkaufmann commented Oct 29, 2021

njgheorghita commented Oct 29, 2021

njgheorghita left a comment

njgheorghita Nov 2, 2021

jacobkaufmann commented Nov 2, 2021

njgheorghita commented Nov 3, 2021

ogenev Nov 4, 2021

jacobkaufmann commented Nov 5, 2021

		@@ -49,14 +46,26 @@ impl PortalnetEvents {
		match std::str::from_utf8(request.protocol()) {
		Ok(protocol) => match protocol {

Add overlay service to dispatch and handle requests #160

Add overlay service to dispatch and handle requests #160

Conversation

jacobkaufmann commented Oct 26, 2021

jacobkaufmann Oct 27, 2021

Choose a reason for hiding this comment

jacobkaufmann Oct 27, 2021

Choose a reason for hiding this comment

njgheorghita Nov 2, 2021

Choose a reason for hiding this comment

jacobkaufmann Oct 27, 2021

Choose a reason for hiding this comment

njgheorghita commented Oct 29, 2021

jacobkaufmann commented Oct 29, 2021

njgheorghita commented Oct 29, 2021

njgheorghita left a comment

Choose a reason for hiding this comment

njgheorghita Nov 2, 2021

Choose a reason for hiding this comment

jacobkaufmann commented Nov 2, 2021

njgheorghita commented Nov 3, 2021

ogenev Nov 4, 2021

Choose a reason for hiding this comment

jacobkaufmann commented Nov 5, 2021