Emit global uTP payload event in uTP listener when an uTP stream is closed/reset

[ogenev]

What was wrong?

UtpListener runs as a self-contained service in the background and handles all active uTP streams.
We need a way to process all closed uTP streams and pass the payload to the overlay service, so we can verify, store and propagate gossip/POKE the received content via uTP.

Subtask of #313.

How was it fixed?

1. Rename UtpSocket to UtpStream.
2. Refactor the way we are storing the received payload (DATA packets) in the uTP socket.
3. Add a new AddActiveConnection UtpListener request and move the initialization of an uTP stream inside UtpListener.
4. Add UtpStream -> UtpListener event channel and emit event inside UtpSocket when stream state changes to Closed or Reset.
5. Upon every UtpStream event, UtpListener processes this event (doing some simple active streams maintenance by removing closed or reset streams) and emit a global UtpListenerEvent
6. Remove redundant and dead code.
7. Add a test case for uTP listener events.

Manually tested OFFER/ACCEPT scenarios with Fluffy.

To-Do

• 1. Add protocol id when creating uTP socket and move uTP socket creation outside of UtpListener.
• 2. Add UtpSocket <-> UtpListener event channel and emit an event upon uTP socket closing.
• 3. Emit global UtpListener event with uTP payload, stream id, and protocol id on closed uTP socket.

The next two tasks will be addressed in the next PR (#337) as this is becoming quite large:

4. Add a uTP listener event handler to dispatch the global event to the specific overlay networks.
5. Listen and handle dispatched events in the overlay layer.

• Add entry to the release notes (may forgo for trivial changes)
• Clean up commit history

[njgheorghita]

👍

[jacobkaufmann]

I'm generally opposed to this sort of approach, where we send messages to UtpListener to "ask" for closed uTP streams. In my view, it adds both unnecessary complexity and unnecessary resource consumption.

The approach I would suggest is similar to the way Discv5 emits Discv5Event to a channel that we can stream. Upon socket close, UtpListener can emit some message to a channel where the message contains the socket info and any data that the socket received. OverlayService can listen to this channel to handle those messages.

[ogenev]

The approach I would suggest is similar to the way Discv5 emits Discv5Event to a channel that we can stream. Upon socket close, UtpListener can emit some message to a channel where the message contains the socket info and any data that the socket received. OverlayService can listen to this channel to handle those messages.

Yeah, this seems a more robust way to handle the closed streams. Here is an implementation of the above using a global overlay event: ogenev@7ba8356

However, this will require an event channel per network because if we emit a global overlay event, then only one of the overlay networks will consume it because we will run multiple overlay protocols.

Also, the connection state is closed inside UtpSocket and I think at least for now, we will still need to process all active sockets in a set time interval in UtpListener to check which one is closed and then emit the event. Another alternative is to add event channels between UtpSocket and UtpListener but this will bring more complexity.

One major issue I realized today is that now UtpListener doesn't recognize which stream for which network is related and we can't emit specific events for a specific network.

I will try to refactor a bit and first store all active uTP connections in UtpListener in a separate HashMap per network and then try to pass state and history event receivers to the OverlayService struct and emit an event when a connection is closed (after UtpListener process all active connections to check which one is closed).

As we are using the same OverlayService struct for every network, probably I should pass those event receivers as Option<UnboundedReceiver<...>>, because we may not always want to run all the networks, do you have any better idea?

Let me know what you think and if this makes sense to you!

[jacobkaufmann]

You know the UtpListener and UtpSocket far more deeply than I do, but here are some thoughts.

However, this will require an event channel per network because if we emit a global overlay event, then only one of the overlay networks will consume it because we will run multiple overlay protocols.

My initial thought was that it would work similar to PortalnetEvents where a single handler listens for closed UtpSocket messages and dispatches them to the corresponding subnetwork. This would require that the message contain some identifying information for the subnetwork, like a ProtocolId.

Also, the connection state is closed inside UtpSocket and I think at least for now, we will still need to process all active sockets in a set time interval in UtpListener to check which one is closed and then emit the event. Another alternative is to add event channels between UtpSocket and UtpListener but this will bring more complexity.

I really think doing some form of polling is the wrong direction. It seems like it should be possible for the UtpSocket to return some type to UtpListener on send/receive that indicates that it is closed. At this point, we can call some method on UtpSocket that consumes the socket and returns a different type that contains the information we need to deliver to whichever subnetwork that socket was for. UtpListener emits this information in a message, and that message gets dispatched to the proper subnetwork.

One major issue I realized today is that now UtpListener doesn't recognize which stream for which network is related and we can't emit specific events for a specific network.

If UtpListener initializes a UtpSocket from a TalkRequest, that request will contain a ProtocolId. If the UtpSocket is initialized from a UtpListenerRequest from a subnetwork, then we can add a ProtocolId to that request. Is that sufficient? I would prefer that UtpListener not have multiple data structures per subnetwork.

Hope these thoughts were helpful.

[ogenev]

Good, I updated the PR description with some TODOs and will re-open it for review when all of them are done.

[ogenev]

As this PR is becoming quite large, I'm going to wrap it and include only the first 3 TODOs here. Updated PR's description and It is open for review now.

Implementation of the overlay handler that will dispatch the UtpListener events to the overlay networks and the handling of those events in the overlay will be addressed in the next PR - #337 (4 and 5 from TODOs).

[mrferris]

👍 to the architecture decision & implementation that resulted from the above conversation.

I left some suggestions about comments and naming. Let me know if I've misunderstood anything.

[mrferris]

The name UtpStreamId is a bit confusing to me, I think because ConnId is already acting as an identifier for streams.
I wonder if UtpStreamDataType or UtpStreamOrigin would be a more descriptive name.

[ogenev]

ConnId is not really an identifier for streams in the portal network context, it is identifying all the packets that belong to the same connection. Each uTP socket has one connection ID for sending packets and a different connection ID for receiving packets.

Anyways, I'm happy to rename this if it will improve code readability, what is your opinion @jacobkaufmann?

[mrferris]

Yea sorry I wasn't making sense by conflating the terms stream and connection.

Passing something non-numeric called Id in the same calls as a numeric Id is what seemed off to me.

Though looking back, I think I'm splitting hairs. I'm good with leaving it as it is, unless anyone else likes the name UtpStreamOrigin or UtpStreamDataType a bit better too.

[mrferris]

I wonder if InitiateConnection would be a more descriptive name for this event.

The name AddActiveConnection is a little bit confusing to me because it seems like both Connect and AddActiveConnection events add an active connection to the utp_connections. In fact, Connect seems like it creates more of an "active" connection than AddActiveConnection does, because by the time a Connect event happens we necessarily have two participants in the stream, whereas an AddActiveConnection might only ever result in an inactive connection if the other peer never connects.

[ogenev]

You are right, the idea of AddActiveConnection is to create an uTP stream and start listening for incoming SYN packets.
Connect is creating an uTP stream and is sending SYN packet for a handshake with the remote peer.

[jacobkaufmann]

Looking good. I feel that there are some comments that we ought to address before merge, but most of my comments are minor or questions about things I don't quite understand. Would like to get clarity on those questions before approval.

In the future, we should minimize refactor changes like naming that add a lot of non-logical line changes to the diff. If possible, deferring those changes can make the PR less noisy.

[jacobkaufmann]

A few minor comments, but looks good overall. Giving approval on the assumption that we tested that this does not break any existing uTP functionality with other portal client implementations.

[jacobkaufmann]

Do we need this clone here?

[ogenev]

At least for now (until we update to rust 1.62), borrowing here shows a warning cannot borrow "*conn" as mutable because it is also borrowed as immutable. More information can be read here.

[jacobkaufmann]

Why are we no longer doing this in a separate task?

[ogenev]

No need for this at the moment, spawning a new task here requires additional refactoring and adds more complexity. If it causes performance issues, we will change this in the future.

[jacobkaufmann]

If this does not work, then should we make the NodeId argument an Option<NodeId> for handle_find_content? I would prefer not to have this sort of logic here if we can avoid it.