Expose send_ping from service.rs for discv5.rs

[KolbyML]

I made a pr which adds send_ping to discv5.rs the reason for this is I am implementing the missing calls for Trin's discv5 rpc and in order to implement discv5_ping I need to expose it from this library first.

[AgeManning]

This seems reasonable to me. Although we are starting to expose some of the lower level discv5 RPC methods which wasn't the original design.

I'm not sure about the trin specs and if you need further access to lower level RPC things. If so, it could get complicated adding a lot more things into the discv5 struct.

Currently the design goes like:

discv5: High-level struct holding an API for users to perform high-level discovery actions
service: A kademelia based DHT which uses discv5 underneath to perform the standard kademlia lookups etc
handler: Handles sending/receiving high-level discv5 packets and handles all the encryption/decryption logic. This directly exposes all the discv5 RPC methods.
send/recv: Handles the encoding/decoding.

The idea was that anyone that just wanted discv5 logic, could instantiate a handler and perform all the to-spec requests and receive responses for discv5 RPC methods, without having to worry about encryption/decryption etc. They just go, send Ping, and get a response etc.

Service is the layer on top which technically isn't spec'd entirely as discv5. There's a lot of logic in there that isn't in the spec and people can change it at will to meet their needs.

I'm not sure how complicated the trin spec is, but if you need access to a lot of discv5 RPC methods/complicated logic it might be worth considering forking the service/discv5 structs and importing the Handler struct. The Handler would maintain all the discv5 spec logic and then you could customize the service and discv5 to suit trin however you see fit. I only mention this because it could get difficult in the future if Trin requires a lot more complicated logic than just the dht that is sitting on top at the moment. It might also work nicer for you if you customised the service to handle the ping response for trin, rather than passing it further up and out through discv5.

[KolbyML]

I'm not sure how complicated the trin spec is, but if you need access to a lot of discv5 RPC methods/complicated logic it might be worth considering forking the service/discv5 structs and importing the Handler struct. The Handler would maintain all the discv5 spec logic and then you could customize the service and discv5 to suit trin however you see fit. I only mention this because it could get difficult in the future if Trin requires a lot more complicated logic than just the dht that is sitting on top at the moment. It might also work nicer for you if you customised the service to handle the ping response for trin, rather than passing it further up and out through discv5.

We only need ping and a none recursive find_node for trin. You guys already expose find_node recursively through find_node and support non-recursive findnode through request_enr() (the only issue is instead of using a multiple Addr we want to use an enr, distance. Adding a non-recursive find_node is added in my other PR)

We don't want to add complicated logic we only want to expose ping (and a non-recursive findnode function with enr/distance) like you guys already did with non-recursive find_node (which was done with request_enr but with multiaddr and presets distance to 0) which is a "lower level discv5 RPC method" already exposed?

The idea was that anyone that just wanted discv5 logic, could instantiate a handler and perform all the to-spec requests and receive responses for discv5 RPC methods, without having to worry about encryption/decryption etc. They just go, send Ping, and get a response etc.

I could be confused but that is exactly what I want too!!. Currently though discv5.rs doesn't expose ping which is what this PR does the same way you guys already expose. talkreq request_enr (which is a non recursive find_node with distance 0)

Sorry if the description I wrote for my PR is confusing xD

[AgeManning]

Yep I understand. I think you guys added talkreq.

Ok cool. So if its just these two things that you need, happy to expose them. If we start needing a bunch of extra things we may need to split up some of the higher level logic to make it easier to maintain.

I'll go through and review this today :)

[KolbyML]

Thank you very much :)

[AgeManning]

Looks good. Nice

[AgeManning]

Why do we have allow(dead_code) here? Its used to construct the response right?

[KolbyML]

error: fields `enr_seq`, `ip`, and `port` are never read
   --> src\service.rs:227:5
    |
225 | pub struct Pong {
    |            ---- fields in this struct
226 |     /// The current ENR sequence number of the responder.
227 |     enr_seq: u64,
    |     ^^^^^^^
228 |     /// Our external IP address as observed by the responder.
229 |     ip: IpAddr,
    |     ^^
230 |     /// Our external UDP port as observed by the responder.
231 |     port: u16,
    |     ^^^^
    |
    = note: `Pong` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
    = note: `-D dead-code` implied by `-D warnings`

error: could not compile `discv5` due to previous error 

let response = Pong { enr_seq, ip, port };

It is used to construct the response, but when I remove that clippy complains fields `enr_seq`, `ip`, and `port` are never read

[KolbyML]

Oh I just realized I should make them public, I wonder why I didn't think of that.

[AgeManning]

Yeah if they are not public, then the data would be unusable in the app?

I assume then this is not tested in the app side where you need it?

[KolbyML]

It isn't tested yet no. When I make a pr for trin adding support for this command I will have to write a test for it there. I was planning to do that after this was merged. I still need to ask the others if we only pull crates from releases now or also commits. Cause if we only do it from releases now then I might have to wait a while.

[AgeManning]

Sure. You could just do quick tests against the branch. That would have found things like not being able to access the data you want and us having to do another PR to correct it later.

At the moment I think this is fine. I was thinking of getting your two PRs in and doing a release. I've also updated to the latest ENR.

[KolbyML]

I will test that tomorrow after my math final then and let you know :)

[AgeManning]

I have been thinking, along with your other PR. It looks like you want access to be able to do misc discv5 RPC calls.

We could make this generic. Allow the discv5 to send a generic Request which gets sent to the handler and the generic response matching the Id gets sent back (without touching the service).

This, I imagine will allow users to call arbitrary discv5 RPC. It would encapsulate the PING, FINDNODEs (your two PRs) the TALKREQ and also any future requests we implement into discv5.

It would be cleaner also as we'd just have a single function to handle all the possible requests and not individual functions for each thing we need as it comes up.

What do you think about this approach?

[KolbyML]

As long as it gets the job done I am all for it 👍. Plus if the code is simpler/more maintainable that is a win win

[KolbyML]

Also I forgot to ask do you want me to shoot my shot on what I think you mean? Or are you planning to implement that since it was your conceptual concept? I am fine with both just let me know!

[AgeManning]

Yeah, i went in to do it quickly last week, but its a bit more complicated than I expected.

The issue is that some of the RPC requests can take many responses. I'm trying to think what the best API is. Do we internally try and manage the multiple responses and return final result in a future, or do we give the user a Stream and they can decide when to drop the stream.

The TALKREQ also has Drop implemented which provides a return value to the peer.

Trying to think of the most ergonomic way to give the user control of these messages without making it too difficult for them. I'll try knock out an implementation today.

[AgeManning]

I built a version, but its not great from a user perspective with generic types. The responses then have to get matched specifically each time. Manually adding these type of things to the discv5 struct allows us to have specific return types making it much nicer.

I also remember there's really not that many low-level rpc functions. This PR with your other ones, pretty much exposes all of the used ones. So lets just do that.

[emhane]

Why did you need to send an extra ping explicitly? Where is that ENR likely to be learnt about that is passed as a param to send_ping? By other means that through discv5 peer discovery I suppose?

Otherwise also here I ask you, what is the problem with the existing ping interval that updates ENRs and simply call kbuckets and then find the latest version of the enr at trin level? This way the flow of instructions between the top level tasks service and handler that implement the core discv5 protocol isn't distorted either by the way trin uses it as Discv5, the app facing level, holds a shared reference to the kbuckets. If the problem is that the interval to update ENRs is too slow why do you not just set the ping_interval ? @KolbyML

[KolbyML]

Trin RPC Docs @emhane

I am just implementing the discv5 rpc for trin. The ping call won't be used Trin itself, but it will allow "scripts" to connect to Trin and have the possibility to "monitor the health of the node" or the network through Trin's RPC.

Where is that ENR likely to be learnt about that is passed as a param to send_ping? By other means that through discv5 peer discovery I suppose?

A potential use cases of the RPC to my knowledge would get the ENR not from Discv5 it would likely be from other sources in a potential "health test script" example I gave. The script could quickly run a test node where we already have the ENR since we made it to test against.

[emhane]

Why not just call add_enr for that use case? Or send it a talk request and wait for the talk response? Why the ping? i.e. why does trin need the pong

discv5/src/rpc.rs

Lines 95 to 103 in 715fc82

	/// A PONG response.
	Pong {
	/// The current ENR sequence number of the responder.
	enr_seq: u64,
	/// Our external IP address as observed by the responder.
	ip: IpAddr,
	/// Our external UDP port as observed by the responder.
	port: u16,
	},

@KolbyML

[KolbyML]

Why not just call add_enr for that use case? Or send it a talk request and wait for the talk response? Why the ping? i.e. why does trin need the pong

discv5/src/rpc.rs

Lines 95 to 103 in 715fc82

	/// A PONG response.
	Pong {
	/// The current ENR sequence number of the responder.
	enr_seq: u64,
	/// Our external IP address as observed by the responder.
	ip: IpAddr,
	/// Our external UDP port as observed by the responder.
	port: u16,
	},

@KolbyML

I am implementing RPC for discv5_, not history_ (I think you might be confused about that cause your response to my other PR you were referring to history) I am confused why we would send a talk_request when we aren't doing anything on a sub-protocol. There is already a discv5_addEnr RPC. The specs lay out the ping discv5_ping request responds with PONG.