Remove support for legacy ProtocolId-based protocol names
#504
Comments
|
We'd also need to modify the Polkadot specification to deprecate these. I don't know how laborious process that is going to be. Eventually we should remove them but right now they aren't doing much harm either and we probably want to keep the alias support at least on code level in case it's needed in the future. Is there a reason they should be removed in the first place? |
|
If I remember correctly, the idea was that a genesis-hash in protocol names safeguards us from connecting to the peers of a different chain at the protocol negotiation stage. This is likely not that important if nobody remembered to finish this during the last 10 months, so this is just being a little annoying to have all this redundant |
For sure. This issue is only about removing the hardcoded legacy protocol names, not the fallback protocol name mechanism itself. |
It is not important for Polkadot, but it was important for third-party chains, as they would often not set any value for As a result, if a single person of a chain A connects to a node of chain B (where A and B have the same protocol names), the peer-to-peer networks of the two chains become intertwined and everything works less well. Why nobody complained in 10 months is because builders tend to just believe that "things are complicated" and not realizing that the problems they face can be fixed. |
|
As DHT-crawling by @BulatSaif shows, we have some protocol names not respecting even Here are the protocol names for Polkadot chain: And for Kusama: As seen, protocol names like |
|
It is worth mentioning that this may be causing issues on parachains. Each Here list of the protocols reported by parachains: Click to see...Kusama statemine parachainPolkadot collective parachainRococo rockmineMoonbeam |
the-right-joyce
added
I3-annoyance
|
Bringing this up once again, we'd need to be careful when removing As was proposed by @altonen, we could drop
@altonen Could you comment if I'm confusing something? |
|
I think that approach could be less intrusive than just removing the legacy protocol in one swoop but I have no evidence to back it up. Maybe worth simulating locally how both approaches (gradual vs non-gradual phase out) work where it's easy to inspect routing tables of all nodes and see how they behave. Here you check which protocols the remote supports. If it only supports |
|
Thanks, then I confused it. I thought we somehow continue using |
|
It would still use I don't know what the Polkadot specification has to say about this (letter and spirit of the law etc.) so if you decide to go this route, you may have to specify this as part of the removal of legacy protocols. It is worth the effort, I don't know. |
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…ch#504) * Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
* Allow bridge pallet to return no finalized headers * Update Runtime APIs to optionally return best finalized header * Update relay to handle optional best finalized headers * Fix Clippy lints * Return a dummy header instead of an Option * Remove Option from runtime Apis * Remove support for handling optional finalized headers in relay
…rotocol (#3833) This PR adds to the DHT only the peers that support the genesis/fork/kad protocol. Before this PR, any peer that supported the legacy `/kad/[id]` protocol was added to the DHT. This is the first step in removing the support for the legacy kad protocols. While I have adjusted unit tests to validate the appropriate behavior, this still needs proper testing in our stack. Part of #504. cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de>
…rotocol (paritytech#3833) This PR adds to the DHT only the peers that support the genesis/fork/kad protocol. Before this PR, any peer that supported the legacy `/kad/[id]` protocol was added to the DHT. This is the first step in removing the support for the legacy kad protocols. While I have adjusted unit tests to validate the appropriate behavior, this still needs proper testing in our stack. Part of paritytech#504. cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de>
@tomaka To be honest, the warning message in smoldot made me remove our protocol id, which lead to exactly what you are describing during testing, so we added it back for now. 😉 Personally, I would prefer if it was at least optional now, so when it is not specified, the legacy protocol is just not run. The |
|
For some context here, the following PRs where merged as a first step: |
…rotocol (paritytech#3833) This PR adds to the DHT only the peers that support the genesis/fork/kad protocol. Before this PR, any peer that supported the legacy `/kad/[id]` protocol was added to the DHT. This is the first step in removing the support for the legacy kad protocols. While I have adjusted unit tests to validate the appropriate behavior, this still needs proper testing in our stack. Part of paritytech#504. cc @paritytech/networking --------- Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io> Co-authored-by: Bastian Köcher <git@kchr.de>
It's been quite a while since the genesis-hash based protocol naming convention was introduced (see issue paritytech/substrate#7746 & PRs paritytech/substrate#11938, paritytech/polkadot#5870, paritytech/polkadot#5876).
It should be safe to remove old-style legacy
ProtocolId-based protocol names like"/dot/sync/1"now.The text was updated successfully, but these errors were encountered: