fix(kad): remove allow_listening handler config

[chirag-bgh]

Description

The allow_listening config option in Kademlia does nothing as it is always set to true. Thus a dummy configuration option and should be removed.

Closes #3499.

Notes

Links to any relevant issues

Open Questions

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]

CI is still failing. Did you manage to compile this locally? I typically run cargo custom-clippy for local development.

[thomaseizinger]

There are two warnings about unused imports left.

Also, this is unfortunately a breaking change because the handler module is public. I would like to change that. I am going to hold off on merging this until we decide to make the next breaking change. Then we can include this. Making a breaking change for removing an obsolete, internal config option is not worth it in my eyes.

I hope you don't mind. The change is relatively isolated so there shouldn't be many merge conflicts.

I am going to open an issue about minimizing the public API of libp2p-kad.

[thomaseizinger]

I am going to hold off on merging this until we decide to make the next breaking change.

Setting this to draft to prevent accidental merge.

[thomaseizinger]

I wrote some more about this here: #3532

[thomaseizinger]

I am going to open an issue about minimizing the public API of libp2p-kad.

Done here: #3533

[mxinden]

Also, this is unfortunately a breaking change because the handler module is public. I would like to change that. I am going to hold off on merging this until we decide to make the next breaking change. Then we can include this. Making a breaking change for removing an obsolete, internal config option is not worth it in my eyes.

I don't think we need to consider this a breaking change. I don't think anyone is using the libp2p-kad ConnectionHandler directly. @thomaseizinger do you feel strongly about being strict here?

[thomaseizinger]

Also, this is unfortunately a breaking change because the handler module is public. I would like to change that. I am going to hold off on merging this until we decide to make the next breaking change. Then we can include this. Making a breaking change for removing an obsolete, internal config option is not worth it in my eyes.

I don't think we need to consider this a breaking change. I don't think anyone is using the libp2p-kad ConnectionHandler directly. @thomaseizinger do you feel strongly about being strict here?

You'll have to override CI and merge it manually then because cargo semver-checks flags it as a breaking change. To avoid a red CI, we'd have to immediately make a patch release.

Feel free to do so if you want. Is there a pressing need though?

[mxinden]

Feel free to do so if you want. Is there a pressing need though?

Good point. In the contrived example where someone is in need for this patch, I would consider ignoring the fact that it is a breaking change. Given that no one directly depends on the change, let's treat it as a breaking change.

[thomaseizinger]

We are going to need this for client-mode right. Should we then really remove it? See #3651.

[mxinden]

For client mode it will need to be dynamic during the lifetime of a single ConnectionHandler, and not a configuration flag static during the entire lifetime of a ConnectionHandler. Does that make sense?

[thomaseizinger]

For client mode it will need to be dynamic during the lifetime of a single ConnectionHandler, and not a configuration flag static during the entire lifetime of a ConnectionHandler. Does that make sense?

That flag can also be modified by a message coming in from the behaviour. As soon as it is changed to true, the next inbound stream will also provide the kademlia protocol.

I think we will need the flag, or something equivalent. We just don't have the "change the flag" mechanism yet.

[mergify]

This pull request has merge conflicts. Could you please resolve them @chirag-bgh? 🙏

[thomaseizinger]

This is obsolete with #3877.