feat(kad): allow to explicitly set Mode::{Client,Server}

[dariusc93]

Description

The current implementation in Kademlia relies on an external address to determine if it should be in client or server mode. However there are instances where a node, which may know an external address, wishes to remain in client mode and switch to server mode at a later point.

This PR introduces Kademlia::set_mode, which accepts an Option<Mode> that would allow one to set Mode::Client for client mode, Mode::Server for server mode, or None to determine if we should operate as a client or server based on our external addresses.

Resolves #4074.

Notes & open questions

This is just my thoughts on how it should be based on what was discussed in #4074. Introducing ExplicitMode would remove the possibility of complexity if we were to have Mode::Auto due to it being passed into the handler, but open to suggestions on changing this to something different instead.

1. From what it looks like, setting ExplicitMode::Server (which internally would set it to Mode::Server) still require an external address. Do we wish to keep this behaviour so the node is reachable or should we rely on another form of address (eg address(es) being listened on)?
2. When ExplicitMode::Auto is set, do we want to automatically determine the mode at that point base on the external address and notify the handler(s) or should we leave Kademlia::mode as is until NetworkBehaviour::on_swarm_event is executed to determine that for us as long as the mode is set to auto?

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

Thanks for opening this PR @dariusc93 !

I realised that the issue with Option<Mode> is that we can't tell whether we should reconfigure the mode automatically. Thus my suggestion would be:

• Delete ExplicitMode
• Introduce a new boolean field: auto_mode, defaulting to true
• Make the signature of set_explicit_mode Option<None>
• If an explicit mode is set, set auto_mode to false

Also, to be technically correct in regards to the poll contract, I think we will need a Waker that is set in poll that we can call once we change any state around the mode, be it the auto_mode boolean or the mode field itself. Otherwise we rely on another event in the network to call poll on Behaviour which might not happen immediately.

Let me know what you think!

[thomaseizinger]

1. From what it looks like, setting ExplicitMode::Server (which internally would set it to Mode::Server) still require an external address. Do we wish to keep this behaviour so the node is reachable or should we rely on another form of address (eg address(es) being listened on)?

That is just for logging though right? We don't need to pass an address to the ConnectionHandler. I think we should change the match to incorporate the above suggested auto_mode flag and log accordingly.

When ExplicitMode::Auto is set, do we want to automatically determine the mode at that point base on the external address and notify the handler(s) or should we leave Kademlia::mode as is until NetworkBehaviour::on_swarm_event is executed to determine that for us as long as the mode is set to auto?

The suggested waker should address this.

[dariusc93]

Thanks for opening this PR @dariusc93 !

I realised that the issue with Option<Mode> is that we can't tell whether we should reconfigure the mode automatically. Thus my suggestion would be:

* Delete `ExplicitMode`

* Introduce a new boolean field: `auto_mode`, defaulting to `true`

* Make the signature of `set_explicit_mode` `Option<None>`

* If an explicit mode is set, set `auto_mode` to false

Also, to be technically correct in regards to the poll contract, I think we will need a Waker that is set in poll that we can call once we change any state around the mode, be it the auto_mode boolean or the mode field itself. Otherwise we rely on another event in the network to call poll on Behaviour which might not happen immediately.

Let me know what you think!

Looks good to me. Originally I had a boolean field to determine if it should be done automatically or not but I think I also had mode be Option<Mode>, which mightve been what prompt me to introduce ExplicitMode, so I dont mind removing that and using your suggestion. I also forgot that we would need to call the waker too.

[thomaseizinger]

Great!

Just missing some tests now :)

[mxinden]

Thank you for tackling this @dariusc93.

[mxinden]

Suggested change

	pub fn set_explicit_mode(&mut self, mode: Option<Mode>) {
	/// Either set Kademlia [`Mode`] explicitly via `Some(_)` or enable automatic configuration of the Kademlia [`Mode`] based on the external addresses available via `None`.
	pub fn set_mode(&mut self, mode: Option<Mode>) {

• Nit pick, passing None enables auto-mode, thus the function name (xxx_explicit_xxx) is not accurate. Would you agree?
• Adding some documentation. Feel free to rephrase. Just a suggestion.

[thomaseizinger]

Nit pick, passing None enables auto-mode, thus the function name (xxx_explicit_xxx) is not accurate. Would you agree?

I disagree. If you call set_explicit_mode with None, you are not setting an explicit mode. Seems quite logical to me?

[thomaseizinger]

Thanks! Some final input but looks good already :)

[thomaseizinger]

Thanks! Some final nits.

[thomaseizinger]

Thank you for your patience! This turned out quite clean :)

[mxinden]

👍 thank you for the follow-ups!