Change on-the-wire protocol names to include genesis hash & fork id

[dmitry-markin]

This PR implements #7746 for transactions, block-announces, sync, sync/warp, state, and light protocols.

polkadot companion: paritytech/polkadot#5849

[acatangiu]

Looks good to me, thanks!

Would be nice if you could add at least one of block-announces, sync, sync/warp, state, and light protocols to this PR to see how it'll be used. Although, come to think about it we already have GRANDPA and BEEFY as showcases.

[tomaka]

I don't really see the need for these standard_protocol_name and legacy_protocol_name functions. You're saving literally two lines of code.
Plus, it is in no way guaranteed that every protocol name follows the same pattern. The fact that they do should be seen as a happy coincidence.
Furthermore, moving this code to a separate function IMO decreases readability. If you want to figure out which protocol name GrandPa has, there is now even one more additional step.
I think it's a mistake to see multiple pieces of code that look the same and immediately jump to the conclusion "we should merge these pieces into one because DRY". Applying DRY has a lot of cons.

But since I'm not maintaining this code, I'm not going to be opposed to it.

[dmitry-markin]

@acatangiu Added block-announces protocol to this PR as another example.

@tomaka We will have the similar pattern of /{genesis_hash}/{fork_id}/{short_name} and /{protocol_id}/{short_name} in 8 places after I create the remaining PRs, and I thought it'd be better to standardize the protocol name in one place. Nevertheless, I can copy these lines to every protocol if you think this is better. What should I do?

[tomaka]

in 8 places after I create the remaining PRs, and I thought it'd be better to standardize the protocol name in one place.

To rephrase my point: what's the advantage of standardizing the protocol name in one place? None that I can see. We might specifically need in the future to change the pattern of one protocol name without touching the 7 others. For example, we might want to upgrade to /<genesis_hash>/transactions/2 but include /<genesis_hash>/transactions/1 in the legacy names.

What should I do?

Decision isn't mine here

[dmitry-markin]

I've updated sync, sync/warp, state, and light protocol names to match the same pattern. Also got rid of common protocol_name generation functions, as was suggested by @tomaka

@acatangiu and @tomaka , could you please re-review?

[dmitry-markin]

@tomaka Should we also update polkadot protocol names, e.g. /polkadot/validation/1, /polkadot/req_chunk/1, etc.?

[tomaka]

cc @eskimor @ordian

I believe that it would make sense to do so. This way, we automatically include in the request the chain that we are targeting.
This makes it possible, in theory, to have a single node validate on multiple relay chains at the same time.

[ordian]

cc @eskimor @ordian

I believe that it would make sense to do so. This way, we automatically include in the request the chain that we are targeting. This makes it possible, in theory, to have a single node validate on multiple relay chains at the same time.

To clarify, you're suggesting this as a follow-up PR to the companion PR to replace "/polkadot/etc" with "/{genesis_hash}/etc" and with fallback to the current names ("/polkadot/etc")? Sounds reasonable to me.

[acatangiu]

Looks good, thanks!

[dmitry-markin]

This PR has 2 approving reviews, and the companion PR is approved. Why Check reviews CI stage is failing?

[tomaka]

It is off-topic why, but my reviews don't technically count

[ordian]

An approval on behalf of @tomaka.

[dmitry-markin]

bot merge

[ordian]

Seems like this assertion is now wrong?

substrate/client/network/src/protocol.rs

Line 1580 in 6527f0c

debug_assert!(negotiated_fallback.is_none());