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

Modular block request handler #14014

Closed
wants to merge 14 commits into from
Closed

Modular block request handler #14014

wants to merge 14 commits into from

Conversation

rahulksnv
Copy link

@rahulksnv rahulksnv commented Apr 25, 2023 •
edited

If a substrate user wants to implement a custom block relay protocol (like Compact Blocks, Graphene, etc), it is not currently possible. This is because the block request handler is tightly coupled with the rest of the stack. This change makes the block request handler modular.

Main changes:

  1. ChainSync currently calls into the block request handler directly. Instead, move the block request handler behind a trait. This allows new protocols to be plugged into ChainSync.
  2. BuildNetworkParams is changed so that custom relay protocol implementations can be (optionally) passed in during network creation time. If custom protocol is not specified, it defaults to the existing block handler
  3. BlockServer and BlockDownloader traits are introduced for the protocol implementation. The existing block handler has been changed to implement these traits
  4. Other changes:
    • make TxHash serializable. This is needed for exchanging the serialized hash in the relay protocol messages
    • clean up types no longer used(OpaqueBlockRequest, OpaqueBlockResponse)

polkadot companion: paritytech/polkadot#7134
cumulus companion: paritytech/cumulus#2489

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Apr 25, 2023

User @rahulksnv, please sign the CLA here.

@bkchr bkchr requested a review from a team April 25, 2023 20:49
@rahulksnv rahulksnv mentioned this pull request Apr 25, 2023
Closed
dmitry-markin
dmitry-markin approved these changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@dmitry-markin dmitry-markin 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, just one minor comment. Thanks!

client/network/sync/src/block_relay_protocol.rs Outdated Show resolved Hide resolved
client/network/sync/src/lib.rs Show resolved Hide resolved
altonen
altonen reviewed Apr 26, 2023
View reviewed changes
client/network/sync/src/block_relay_protocol.rs Show resolved Hide resolved
client/network/sync/src/block_relay_protocol.rs Outdated Show resolved Hide resolved
client/network/sync/src/block_relay_protocol.rs Outdated Show resolved Hide resolved
client/network/sync/src/block_request_handler.rs Show resolved Hide resolved
client/network/sync/src/lib.rs Outdated Show resolved Hide resolved
@rahulksnv
Copy link
Author

Please point me how to fix the failing cumulus test (could not find anything in the contributing doc about making a dependent cumulus PR)

@altonen
Copy link
Contributor

altonen commented Apr 27, 2023

Please point me how to fix the failing cumulus test (could not find anything in the contributing doc about making a dependent cumulus PR)

You need to make a cumulus companion, much the same way as you made the polkadot companion

@rahulksnv rahulksnv mentioned this pull request Apr 27, 2023
Closed
rahulksnv added a commit to subspace/substrate that referenced this pull request Apr 28, 2023
@rahulksnv
Patch upstream PR for modular block handler
a02c7d6 
Upstream PR: paritytech#14014

This reverts commit c9276cb.
@rahulksnv rahulksnv mentioned this pull request Apr 28, 2023
Merged
@rahulksnv rahulksnv requested a review from altonen May 1, 2023 20:22
@altonen
Copy link
Contributor

altonen commented May 9, 2023

You need to merge master to polkadot/cumulus companions

@rahulksnv
Copy link
Author

You need to merge master to polkadot/cumulus companions

Done, thanks

melekes
melekes approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@rahulksnv
Copy link
Author

Hi @altonen @melekes @dmitry-markin Can you please let me know the next steps here, in terms of getting this merged? Sorry I am not clear about the process, as I am submitting an upstream patch for the first time. Thanks!

@altonen altonen added A0-please_review Pull request needs code review. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T0-node This PR/Issue is related to the topic “node”. C1-low PR touches the given topic and has a low impact on builders. labels May 11, 2023
@dmitry-markin
Copy link
Contributor

Hi @altonen @melekes @dmitry-markin Can you please let me know the next steps here, in terms of getting this merged? Sorry I am not clear about the process, as I am submitting an upstream patch for the first time. Thanks!

Hi @rahulksnv, please merge master into you PR to resolve the merge conflicts. Than I would wait for @altonen approval, and we can merge the PR.

@rahulksnv
Copy link
Author

Hi @altonen @melekes @dmitry-markin Can you please let me know the next steps here, in terms of getting this merged? Sorry I am not clear about the process, as I am submitting an upstream patch for the first time. Thanks!

Hi @rahulksnv, please merge master into you PR to resolve the merge conflicts. Than I would wait for @altonen approval, and we can merge the PR.

@dmitry-markin merged with latest master, thanks

@dmitry-markin
Copy link
Contributor

@dmitry-markin merged with latest master, thanks

Could you do the same with companion PRs for tests to pass?

@rahulksnv
Copy link
Author

rahulksnv commented May 22, 2023 •
edited

@dmitry-markin merged with latest master, thanks

Could you do the same with companion PRs for tests to pass?

Re-synced all three repos to the corresponding master.

@stale
Copy link

stale bot commented Jun 21, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added A3-stale and removed A3-stale labels Jun 21, 2023
@rahulksnv
Copy link
Author

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

Thanks for the reminder, was on a break, will resync with master and try to wrap this up

@stale
Copy link

stale bot commented Jul 21, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 21, 2023
@stale stale bot removed the A3-stale label Jul 28, 2023
@rahulksnv rahulksnv mentioned this pull request Sep 12, 2023
Merged
@rahulksnv rahulksnv closed this by deleting the head repository Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@melekes melekes melekes approved these changes

@dmitry-markin dmitry-markin dmitry-markin approved these changes

@altonen altonen altonen approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rahulksnv @altonen @dmitry-markin @melekes