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

Merge Protocol and Notifications #516

Open
4 tasks
altonen opened this issue Mar 6, 2023 · 2 comments
Open
4 tasks

Merge Protocol and Notifications #516

altonen opened this issue Mar 6, 2023 · 2 comments
Assignees
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task.

Comments

@altonen
Copy link
Contributor

altonen commented Mar 6, 2023

After paritytech/substrate#12828, Protocol has no role in the networking stack anymore as the syncing protocol now lives above sc-network and the low-level API used for sync is no longer used. Notification-related events can be sent directly from Notifications.

Tasks:

  • Remove HARDCODED_PEERSET_SYNC and all functionality that references the hardcoded Peerset entry
  • Move handshake decoding to protocol handlers [1]
  • Add implementations for functions such as add_peers_to_set and disconnect_peer for Notifications
  • Remove the block_announce_config from Params in src/config.rs and treat it instead as any other Notifications protocol

[1] "By coincidence", paritytech/substrate#12441 converted the default empty handshake to send the role of the node as the default handshake. This means that each protocol now has the capability of learning the role of remote peer by decoding the handshake, meaning that this logic can be moved out of Protocol and the BlockAnnouncesHandshake decoding can also moved to SyncingEngine.

@altonen altonen added I7-refactor I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. labels Mar 6, 2023
@altonen
Copy link
Contributor Author

altonen commented Mar 6, 2023

@melekes This could be a good task to get familiar with the networking code. Some of the tasks can be done as independent PRs

@melekes melekes self-assigned this Mar 6, 2023
@altonen
Copy link
Contributor Author

altonen commented Mar 7, 2023

I just realized that we can't trust Roles to be in the handshake because it's not in the protocol specification. Until the Peerset is converted into something that can be shared between protocols easily, solutions to this problem are going to be hackish. What basically needs to happen is that BlockAnnouncesHandshake is decoded and the role is stored somewhere (for example temporarily in NetworkWorker) that can then add it to NotificationStreamOpened when it broadcasts that message to all listeners. This would be a short-term fix in order to get things moving and we would clean it up later when handshakes are properly implemented.

melekes referenced this issue in melekes/substrate Apr 13, 2023
and treat it instead as any other `Notifications` protocol

Refs #13542
@altonen altonen transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I4-refactor Code needs refactoring. D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. and removed I7-refactor labels Aug 25, 2023
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
bkchr pushed a commit that referenced this issue Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D1-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. I4-refactor Code needs refactoring. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task.
Projects
Status: Backlog 🗒
Development

No branches or pull requests

3 participants