Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Change request-response protocol names to include genesis hash & fork id #5870

Merged
merged 12 commits into from Aug 12, 2022
Merged

Change request-response protocol names to include genesis hash & fork id #5870

merged 12 commits into from Aug 12, 2022

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Aug 9, 2022 •
edited

Description

This is a follow-up PR to paritytech/substrate#11938 to rename Polkadot request-response protocols similarly to Substrate changes, originally discussed in paritytech/substrate#7746.

Open questions

This PR doesn't cover all Polkadot protocols --- changing validation & collation notification protocols requires a bit of refactoring, so I'm going to create a different PR for them. Please let me know if I missed some other protocols.

@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 9, 2022
bkchr
bkchr reviewed Aug 10, 2022
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nitpicks, but looks good in general.

node/network/bridge/src/network.rs Outdated Show resolved Hide resolved
node/network/protocol/src/request_response/mod.rs Outdated Show resolved Hide resolved
@bkchr bkchr requested review from eskimor and removed request for acatangiu August 10, 2022 13:56
eskimor
eskimor approved these changes Aug 12, 2022
View reviewed changes
Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! @slumber heads up: this will likely cause conflicts with your PR.

node/network/protocol/src/request_response/mod.rs Outdated Show resolved Hide resolved
node/service/src/lib.rs Outdated Show resolved Hide resolved
bkchr
bkchr approved these changes Aug 12, 2022
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides that last nitpicks mentioned by @eskimor it looks good!

Ty for this :)

node/service/src/lib.rs Outdated Show resolved Hide resolved
@ordian
Copy link
Member

ordian commented Aug 12, 2022 •
edited

How do we make sure not to have a conflicting protocol name with substrate? E.g. if someone later uses req_chunk in substrate for something else? Do we expect people to look at both repositories and know where to look? Would it make sense to retain the /polkadot prefix after /{genesis_hash}?

@bkchr
Copy link
Member

bkchr commented Aug 12, 2022

How do we make sure not to have a conflicting protocol name with substrate? E.g. if someone later uses req_chunk in substrate for something else? Do we expect people to look at both repositories and know where to look? Would it make sense to retain the /polkadot prefix after /{genesis_hash}?

https://github.com/paritytech/substrate/blob/a1a9b475c354d873f91135ed5fa028ac9ef6a2c4/client/network/src/request_responses.rs#L213

@dmitry-markin dmitry-markin added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Aug 12, 2022
@dmitry-markin dmitry-markin merged commit 93f45f9 into master Aug 12, 2022
@dmitry-markin dmitry-markin deleted the dm-request-response-protocol-names branch August 12, 2022 12:07
@dmitry-markin dmitry-markin mentioned this pull request Aug 13, 2022
Merged
ordian added a commit that referenced this pull request Aug 15, 2022
@ordian
Merge branch 'master' into ao-disputes-rewards
5665053 
* master:
  Transaction payment runtime apis: query call info and fee details (#5871)
  [ci] Improve cancel-pipeline job (#5874)
  Bump wasmtime from 0.38.1 to 0.38.3 (#5802)
  Incorporate changes from substrate PR #11908 (#5815)
  Add nomination pools to Polkadot runtime (#5582)
  Change request-response protocol names to include genesis hash & fork id (#5870)
  [ci] Run check-runtime only for PRs (#5858)
  Bump tokio from 1.18.2 to 1.19.2 (#5678)
  Zombienet: test disputes with malus garbage candidates (#5857)
  add unit tests to run runtime migrations (#5865)
  Trivial networking changes for Substrate PR #11940 (#5841)
  Renaming CLI prunning and keep-blocks flags (#5863)
  Update yamux to fix a potential crash (#5861)
@dmitry-markin dmitry-markin mentioned this pull request Jun 13, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eskimor eskimor eskimor approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dmitry-markin @ordian @bkchr @eskimor