feat: all unverified inbound sessions

[acolytec3]

Features

• Adds new optional sessionConfig parameter to allow inbound sessions with invalid ENRs (i.e. where observed socket address doesn't match socket address reported in ENR - to allow nodes reporting local IP/port combinations to receive discv5 PONG messages in order to update ENRs).
  • Note: nodes with "unverified sessions" are not added to the discv5 routing table so the discv5 service will never send outbound messages to them to ensure we aren't propagating DDoS attacks
• Allows sendTalkResp to specify a target node via session key (nodeId + socketAddr) to allow responding to nodes with unverified sessions (so will not have an ENR in the routing table)
• Makes service.sendPing public so it can be triggered by portal network layer to trigger enrUpdate logic via PONG messages.

Fixes

• Fixes tests

[dapplion]

🙏

[acolytec3]

This should be ready for review.

[wemeetagain]

Does it make sense to also also allow outbound unverified sessions?
They seem less useful, but you could dial a peer via a full multiaddr, and currently, if they respond with 'unverified' ENR (wrong or no IP+port), the session is dropped.
If we allowed unverified outbound sessions, we'd be able to use them for TALKREQ/RESP at least.

[acolytec3]

Does it make sense to also also allow outbound unverified sessions? They seem less useful, but you could dial a peer via a full multiaddr, and currently, if they respond with 'unverified' ENR (wrong or no IP+port), the session is dropped. If we allowed unverified outbound sessions, we'd be able to use them for TALKREQ/RESP at least.

Potentially, though at least in a Portal Network context, we have no inbuilt way to obtain a full Multiaddr (including the public key). We do have one subprotocol that relies entirely upon TALKREQ and drops the corresponding TALKRESPs and it could be useful to allow that subprotocol to work based just upon the INodeAddress.

[acolytec3]

And now that I think about it, another way to approach this might be to have the sessionService "established" event include the observed socket address along with the ENR as well as a "verified" flag and then update the routing table to only if the verified is set. Then, Portal Network can listen for that and update our own ENR cache (since we are already also maintaining our own routing tables) with the ENRs and multiaddrs and then we can call TALKREQ/TALKRESP with ENR or multiaddr. Thoughts?

[acolytec3]

Disregard my previous feedback, I think this is now ready for review again.

[acolytec3]

Does it make sense to also also allow outbound unverified sessions? They seem less useful, but you could dial a peer via a full multiaddr, and currently, if they respond with 'unverified' ENR (wrong or no IP+port), the session is dropped. If we allowed unverified outbound sessions, we'd be able to use them for TALKREQ/RESP at least.

I went ahead and added this in as I think I now can see where it can be useful down the line. I can't think of a use case where we'll be able to make use of it within Portal Network in our current code base unless we set up an ENR/socket address cache since we wouldn't have any other way to know the correct socket address to send a TALKREQ to but as I noted above, we do have one use case where we might want to still send outbound messages to a node with an invalid ENR so this will be a useful stepping stone to that use case.

[wemeetagain]

And now that I think about it, another way to approach this might be to have the sessionService "established" event include the observed socket address along with the ENR as well as a "verified" flag and then update the routing table to only if the verified is set. Then, Portal Network can listen for that and update our own ENR cache (since we are already also maintaining our own routing tables) with the ENRs and multiaddrs and then we can call TALKREQ/TALKRESP with ENR or multiaddr. Thoughts?

I think thats a good idea. We can update the established event type to:
established: (nodeAddr: INodeAddress, enr: ENR, connectionDirection: ConnectionDirection, verified: boolean) => void;
and filter out unverified sessions as a first step in onEstablished.

This also wouldn't effect performance/behavior in the default configuration, since only verified sessions will emit established events.

[wemeetagain]

I think there should be a this.failSession(nodeAddr, RequestErrorType.InvalidRemoteENR, true) after this log and the similar log below.

[acolytec3]

Added.

[acolytec3]

I think thats a good idea. We can update the established event type to:
established: (nodeAddr: INodeAddress, enr: ENR, connectionDirection: ConnectionDirection, verified: boolean) => void;
and filter out unverified sessions as a first step in onEstablished.

I've made these updates. Can we also bubble up this established event to the discv5 service layer? I'd have to add a new event at that level. Otherwise, I can hack around it with a little ;(discv5 as any).sessionService.on("established... in our portalnetwork code if you don't want it exposed in the main API.

[acolytec3]

Looks like I have a broken test now so will look at that too.

[wemeetagain]

I think thats a good idea. We can update the established event type to:
established: (nodeAddr: INodeAddress, enr: ENR, connectionDirection: ConnectionDirection, verified: boolean) => void;
and filter out unverified sessions as a first step in onEstablished.

I've made these updates. Can we also bubble up this established event to the discv5 service layer? I'd have to add a new event at that level. Otherwise, I can hack around it with a little ;(discv5 as any).sessionService.on("established... in our portalnetwork code if you don't want it exposed in the main API.

Maybe we can just make the sessionService public. You could then subscribe to events there without having to cast to any. Make it up to consumers of the library to not meddle with the sessionService in sketchy ways.

[acolytec3]

I think thats a good idea. We can update the established event type to:
established: (nodeAddr: INodeAddress, enr: ENR, connectionDirection: ConnectionDirection, verified: boolean) => void;
and filter out unverified sessions as a first step in onEstablished.

I've made these updates. Can we also bubble up this established event to the discv5 service layer? I'd have to add a new event at that level. Otherwise, I can hack around it with a little ;(discv5 as any).sessionService.on("established... in our portalnetwork code if you don't want it exposed in the main API.

Maybe we can just make the sessionService public. You could then subscribe to events there without having to cast to any. Make it up to consumers of the library to not meddle with the sessionService in sketchy ways.

That works for me though it is another event emitter for me and any other consumer to keep track of. Doesn't seem like there are any other undue risks since even if somebody sends random messages, it seems like the worst that could happen is a session gets invalidated by the receiving node if somebody starts sending garbage data.

[wemeetagain]

Awesome! this will be a nice feature