Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the possibility to negotiate fallback Kademlia protocol names #2837

Closed
dmitry-markin opened this issue Aug 22, 2022 · 4 comments · Fixed by #2846
Closed

Add the possibility to negotiate fallback Kademlia protocol names #2837

dmitry-markin opened this issue Aug 22, 2022 · 4 comments · Fixed by #2846

Comments

@dmitry-markin
Copy link
Contributor

Description

Add the possibility to negotiate multiple Kademlia protocol names, like it's done, for example for request-response protocol.

Motivation

This would allow to change the protocol name in a backward-compatible manner. We've updated the protocol names in Polkadot/Substrate and the Kademlia protocol is the only one remaining due to this limit of one on-the-wire protocol name. Original discussion is in paritytech/substrate#7746 (comment).

Current Implementation

Currently only one protocol name is returned in UpgardeInfo for KademliaProtocolConfig:

impl UpgradeInfo for KademliaProtocolConfig {
type Info = Cow<'static, [u8]>;
type InfoIter = iter::Once<Self::Info>;
fn protocol_info(&self) -> Self::InfoIter {
iter::once(self.protocol_name.clone())
}
}

Proposed Solution

We can add a field fallback_names to KademliaProtocolConfig and a setter set_fallback_names(). This way the change will be backward-compatible in terms of the API.

Open Questions

Generally, it might worth reporting the negotiated protocol name for clients to be able to change the behavior accordingly, but this will require to change KadStreamSink and will be an API breaking change. I don't know whether it makes sense doing this, probably it doesn't. CC @tomaka

Are you planning to do it yourself in a pull request?

Yes.

@mxinden
Copy link
Member

mxinden commented Aug 23, 2022

This makes sense to me.

We can add a field fallback_names to KademliaProtocolConfig and a setter set_fallback_names(). This way the change will be backward-compatible in terms of the API.

I don't mind the breaking change on the API. I am fine with KademliaConfig::set_protocol_nameS to take more than one protocol.


One thing to keep in mind:

Given node A, B and C, where A speaks only the old protocol, B speaks both and C speaks only the new protocol.

  1. Node B connects to node A and thus adds node A to its routing table.
  2. Node C is looking for a value close to node A. Node C is only aware of node B. Node C connects to node B.
  3. Node B returns the addresses of node A to node C.
  4. Node C tries to connect to node A but fails as it does not support the old protocol.

I don't think this is a blocker. Just something to keep in mind when it comes to your role out strategy.

@thomaseizinger
Copy link
Contributor

Back when this was implemented, was the alternative considered of just adding libp2p_kad::Behaviour twice into the composing Behaviour? Once you have two instances, you can join different DHTs, one with the legacy protocol and one with the new one during the rollout and later just remove the old one.

The API of this module is about to change and I am wondering if we need to keep the functionality of specifying multiple names: #5006

@dmitry-markin
Copy link
Contributor Author

Back when this was implemented, was the alternative considered of just adding libp2p_kad::Behaviour twice into the composing Behaviour? Once you have two instances, you can join different DHTs, one with the legacy protocol and one with the new one during the rollout and later just remove the old one.

The API of this module is about to change and I am wondering if we need to keep the functionality of specifying multiple names: #5006

No, I was not aware of such option. It would be easier to keep multiple protocol names for now to not incur the refactoring of the client code. CC @altonen

@thomaseizinger
Copy link
Contributor

Back when this was implemented, was the alternative considered of just adding libp2p_kad::Behaviour twice into the composing Behaviour? Once you have two instances, you can join different DHTs, one with the legacy protocol and one with the new one during the rollout and later just remove the old one.
The API of this module is about to change and I am wondering if we need to keep the functionality of specifying multiple names: #5006

No, I was not aware of such option. It would be easier to keep multiple protocol names for now to not incur the refactoring of the client code. CC @altonen

Yes, for now the API is staying. But with #5006, we get the opportunity to introduce a new one and I think we should be fine to just handle a single protocol there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants