Extract consensus_gossip.rs and put it in its own crate #4284
Conversation
| /// handle to a gossip service or similar. | ||
| /// | ||
| /// Intended to be a lightweight handle such as an `Arc`. | ||
| pub trait Network<Block: BlockT>: Clone + Send + 'static { |
There was a problem hiding this comment.
I wouldn't mind something like this going into consensus-common either - where we can focus on the properties a network needs to provide to a consensus engine.
| while let Some(Ok(event)) = stream.next().await { | ||
| match event { | ||
| Event::NotifOpened { remote, engine_id: msg_engine_id } => { | ||
| if msg_engine_id != engine_id { |
There was a problem hiding this comment.
i don't get why it's necessary to echo the engine ID in the notifications. shouldn't it be implicit from context?
There was a problem hiding this comment.
We get events from all the registered protocols, not just ours. The event stream is global to the network. It is not filtered.
There was a problem hiding this comment.
oh, that's kind of strange. why do we get events from all subprotocols?
There was a problem hiding this comment.
I don't think there's any real drawback in doing so, except that we duplicate all messages once event stream (which is why the message itself is a Bytes, which is kind of an Arc<Vec<u8>>, rather than just a Vec<u8>).
I did that in order to not inflict ourself any artificial constraint later, for example if you want the same code to register two different protocols, or if you want to know whether the node you're talking to also has a substream to a different specific protocol open.
And it's also easier to implement this way.
|
Let us know once this is ready for another review @tomaka. |
|
This is ready. All tests are passing and documentation has been written. The corresponding Polkadot PR still needs to be written, though. |
|
Small update: Polkadot PR almost ready, I just have to fix a tests module. |
|
|
||
| let validator = Arc::new(validator); | ||
| service.register_validator(validator.clone()); | ||
| let gossip_engine = GossipEngine::new(gossip_engine, executor, GRANDPA_ENGINE_ID, validator.clone()); |
There was a problem hiding this comment.
My previous comment might not have been very clear. I meant to rename the left side of the equation. Now one creates a GossipEngine with a gossip_engine as an argument. The latter should be a network or a service, no?
|
Polkadot PR now ready: paritytech/polkadot#680 |
4 participants
Extracted from #4125
This extracts
consensus_gossip.rsfrom the networking crate and puts it in its own crate.In details:
consensus_gossip.rsfrom the network, as well as the gossiping-related methods:with_gossipandgossip_consensus_message.register_notifications_protocol. Callevent_streamin order to obtain a stream of the events that happen on the network. Callwrite_notificationsin order to send a message to a peer we're connected to.NetworkServiceno longer returns events and is now aFutureagain. Everything is done throughevent_stream.finality_grandpa::Networkhas changed to be lower-level. It's GrandPa that sets up the gossiping system.executorparameter toGrandpaParamsand torun_grandpa_observer, which must implement thefutures::task::Spawntrait and that is used to spawn GrandPa's background tasks. This trait has been implemented onservice::SpawnTaskHandler, so that it's the service that spawns GrandPa's tasks (which is also something we've been willing to do for a while).Note: the Polkadot PR is not ready. I'll work on it while this gets reviewed.