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

Rework the event system of sc-network #14197

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Rework the event system of sc-network #14197

wants to merge 50 commits into from

Conversation

altonen
Copy link
Contributor

@altonen altonen commented May 23, 2023 •
edited
Loading

This PR introduces NotificationService which gives protocols direct access to the ConnectionHandler and allows them to bypass NetworkWorker when communicating with remote peers. When a protocol is initialized, a handle pair is created and the other half of the handle is given to the protocol which allows it to send and receive notifications and peer events from network. The other half is given to Notifications which it uses to communicate with the protocol directly.

PR also introduces the concept of configurable handshakes. This allows us to do two things:

  • allow protocols to start using custom handshakes
  • allow protocols to decide whether they want to accept or reject the inbound substream

Previously Peerset was entirely in control of choosing which peers to accept/reject which caused problems with /block-announces/1. The slot allocation is part of ProtocolController which may be incorporated into NotificationService in the future but whether the peer is accepted or rejected is now done by both the protocol and by Peerset.

polkadot companion: paritytech/polkadot#7582
cumulus companion: paritytech/cumulus#2983

Fixes paritytech/polkadot-sdk#515, fixes paritytech/polkadot-sdk#554, fixes paritytech/polkadot-sdk#512, fixes paritytech/polkadot-sdk#556

altonen added 17 commits May 23, 2023 10:20
@altonen
Take notification configs
cb99f5c
@altonen
Return NonDefaultSetConfig from TransactionsHandlerPrototype::new()
471616a
@altonen
Make fields of NonDefaultSetConfig private
c456edd
@altonen
Introduce NotificationService
5593a53
@altonen
Add implementation of NotificationService
22cd46c
@altonen
Initialize command streams and protocol handles in Notifications
b42b901
@altonen
Poll commands from protocols in Notifications
9685625 
Substream open/close functionality is not supported so they're left as
`todo!()s`.

Remove `SendNotification` command as sending notifications is now
supported by using the installed notification sink received in
`NotificationStreamOpened` event.
@altonen
Start using tracing_unbounded for Notifications -> protocols channel
b93cf90
@altonen
Send NotificationEvents from Notifications
8cc85c6 
The role should not be part of `NotificationStreamOpened` as
`Notifications` has no knowledge of that and the protocols that are
interested in that information should query it from `Peerset` (when the
support for that is added)
@altonen
Start using NotificationService for transactions protocol
f4f9090
@altonen
Implement clone() for NotificationService
796f826
@altonen
Fix BEEFY, NetworkGossip and GRANDPA tests
00a093a
@altonen
Start using NotificationService for SyncingEngine
a3746dc
@altonen
Validate inbound substream before emitting NotificationStreamOpened
15d093b
@altonen
Convert statement store to use NotificationService
513ca87
@altonen
Split NotificationService files logically
2e9da9c
@altonen
Add getter for peer's NotificationsSink
1fd9a08
@altonen altonen mentioned this pull request May 24, 2023
Merged
dmitry-markin
dmitry-markin reviewed May 24, 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! Have yet to go through notifications/behaviour.rs and notifications/service/tests.rs — going to finish the review tomorrow.

client/consensus/grandpa/src/communication/tests.rs Outdated Show resolved Hide resolved
client/consensus/grandpa/src/communication/tests.rs Outdated Show resolved Hide resolved
client/consensus/grandpa/src/communication/tests.rs Outdated Show resolved Hide resolved
client/consensus/grandpa/src/communication/tests.rs Outdated Show resolved Hide resolved
client/network/src/config.rs Outdated Show resolved Hide resolved
client/network/src/protocol.rs Show resolved Hide resolved
client/network/src/service/traits.rs Outdated Show resolved Hide resolved
client/network/src/protocol/notifications/service/mod.rs Show resolved Hide resolved
client/network/src/protocol/notifications/service/mod.rs Outdated Show resolved Hide resolved
client/network/src/protocol/notifications/service/mod.rs Outdated Show resolved Hide resolved
altonen added a commit to paritytech/cumulus that referenced this pull request Aug 8, 2023
@altonen
Companion for paritytech/substrate#14197
7356c00
@altonen altonen changed the title Introduce NotificationService Rework the event system of sc-network Aug 9, 2023
@altonen altonen marked this pull request as ready for review August 9, 2023 07:53
@altonen altonen 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. 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 and removed B0-silent Changes should not be mentioned in any release notes labels Aug 9, 2023
@altonen altonen requested review from kpp, vstakhov, sandreim, a team and dmitry-markin August 9, 2023 07:55
@altonen altonen added the T0-node This PR/Issue is related to the topic “node”. label Aug 9, 2023
vstakhov
vstakhov reviewed Aug 9, 2023
View reviewed changes
client/network/sync/src/engine.rs
log::debug!(target: "sync", "New best block imported {:?}/#{}", hash, number);

self.chain_sync.update_chain_info(&hash, number);
while let Poll::Pending = self
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite sure why do we need a loop here (isn't it a busy loop after all?), as there were no such a thing in the previous code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SyncingEngine is lagging behind in terms of refactoring and still heavily uses polling. There is work that tries to address this (#14675) and in time this call can be converted to a simple .await.

As far as these changes are concerned, updating the handshake is now a future because the code sends the command to Notifications directly over a bounded command channel. The channel is not busy and right now expects one message every six seconds so I don't believe this to be a grave concern. I will add a TODO to remove this ugly loop once SyncingEngine is ready.

client/network/sync/src/engine.rs Outdated Show resolved Hide resolved
client/network/sync/src/engine.rs Show resolved Hide resolved
client/network/sync/src/engine.rs Outdated Show resolved Hide resolved
dmitry-markin
dmitry-markin approved these changes Aug 10, 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! 👍

Left some comments which are mostly nitpicks, the two actually important are:

  1. We should insert a peer into PeerStore when setting the role.
  2. There is one place where report_notification_sink_replaced() should be called in Notifications.

client/network/src/protocol/notifications/service/metrics.rs Outdated Show resolved Hide resolved
client/network/src/service.rs Outdated Show resolved Hide resolved
client/network/src/service/traits.rs Outdated Show resolved Hide resolved
client/network/src/peer_store.rs Outdated Show resolved Hide resolved
client/network-gossip/src/bridge.rs Outdated Show resolved Hide resolved
client/network/sync/src/engine.rs Outdated Show resolved Hide resolved
client/network/sync/src/engine.rs Outdated Show resolved Hide resolved
client/network/sync/src/engine.rs Show resolved Hide resolved
client/network/sync/src/engine.rs Outdated Show resolved Hide resolved
client/network/test/src/service.rs Outdated Show resolved Hide resolved
altonen and others added 2 commits August 11, 2023 11:14
@altonen @dmitry-markin
Apply suggestions from code review
e5e6af0 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@altonen
Apply review comments
25c12b2
@acatangiu acatangiu mentioned this pull request Aug 29, 2023
Merged
@altonen altonen mentioned this pull request Sep 4, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

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

@vstakhov vstakhov vstakhov left review comments

@andresilva andresilva Awaiting requested review from andresilva

@kpp kpp Awaiting requested review from kpp

@sandreim sandreim Awaiting requested review from sandreim

At least 2 approving reviews are required to merge this pull request.

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
3 participants
@altonen @vstakhov @dmitry-markin