This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
client/finality-grandpa: Reintegrate periodic neighbor packet worker #4631
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `NeighborPacketWorker` within `client/finality-grandpa` does two things: 1. It receives neighbor packets from components within `client/finality-grandpa`, sends them down to the `GossipEngine` in order for neighboring nodes to receive. 2. It periodically sends out the most recent neighbor packet to the `GossipEngine`. In order to send out packets it had a clone to a `GossipEgine` within an atomic reference counter and a mutex. The `NeighborPacketWorker` was then spawned onto its own asynchronous task. Instead of running in its own task, this patch reintegrates the `NeighborPacketWorker` into the main `client/finality-grandpa` task not requiring the `NeighborPacketWorker` to own a clone of the `GossipEngine`. The greater picture This is a tiny change within a greater refactoring. The overall goal is to **simplify** how finality-grandpa interacts with the network and to **reduce** the amount of **unbounded channels** within the logic. Why no unbounded channels: Bounding channels is needed for backpressure and proper scheduling. With unbounded channels there is no way of telling the producer side to slow down for the consumer side to catch up. Rephrased, there is no way for the scheduler to know when to favour the consumer task over the producer task on a crowded channel and the other way round for an empty channel. Reducing the amount of shared ownership simplifies the logic and enables one to use async-await syntax-suggar, given that one does not need to hold a lock across poll invocations. Using async-await enables one to use bounded channels without complex logic.
mxinden
commented
Jan 15, 2020
| // | ||
| // NetworkBridge is required to be clonable, thus one needs to be able to clone its children, | ||
| // thus one has to wrap neighor_packet_worker with an Arc Mutex. | ||
| neighbor_packet_worker: Arc<Mutex<periodic::NeighborPacketWorker<B>>>, |
There was a problem hiding this comment.
Yes, in order to reduce the amount of Mutexes, I introduce a new one here. This one is within the crate only and can be removed once NetworkBridge does not need to be clonable anymore.
| /// Sender side of the neighbor packet channel. | ||
| /// | ||
| /// Packets send into this channel are processed by the `NeighborPacketWorker` and passed on to | ||
| /// the underlying `GossipEngine`. | ||
| neighbor_sender: periodic::NeighborPacketSender<B>, |
There was a problem hiding this comment.
Suggested change
| neighbor_sender: periodic::NeighborPacketSender<B>, | |
| neighbor_packet_sender: periodic::NeighborPacketSender<B>, |
Happy to do this change, but rather as a follow up to reduce the noise within this pull request.
tomaka
reviewed
Jan 15, 2020
client/finality-grandpa/src/lib.rs
Outdated
| SC: SelectChain<Block> + 'static, | ||
| NumberFor<Block>: BlockNumberOps, | ||
| NumberFor<Block>: BlockNumberOps + Unpin, |
rphmeier
reviewed
Jan 15, 2020
| NeighborPacketSender<B>, | ||
| ) where | ||
| B: BlockT, | ||
| pub(super) struct NeighborPacketWorker<B: BlockT> { |
rphmeier
approved these changes
Jan 15, 2020
tomaka
suggested changes
Jan 16, 2020
| @@ -391,6 +411,24 @@ impl<B: BlockT, N: Network<B>> NetworkBridge<B, N> { | |||
| } | |||
| } | |||
|
|
|||
| impl<B: BlockT, N: Network<B>> Future03 for NetworkBridge<B, N> | |||
| where | |||
| NumberFor<B>: Unpin, | |||
There was a problem hiding this comment.
Suggested change
| NumberFor<B>: Unpin, |
You probably have to add impl<B, N> Unpin for NetworkBridge<B, N> {} somewhere instead.
There was a problem hiding this comment.
Thank you, thank you and thank you @tomaka ❤️
|
|
||
| impl <B: BlockT> Stream for NeighborPacketWorker<B> | ||
| where | ||
| NumberFor<B>: Unpin, |
There was a problem hiding this comment.
Suggested change
| NumberFor<B>: Unpin, |
client/finality-grandpa/src/lib.rs
Outdated
| SC: SelectChain<Block> + 'static, | ||
| VR: VotingRule<Block, Client<B, E, Block, RA>> + Clone + 'static, | ||
| NumberFor<Block>: BlockNumberOps, | ||
| NumberFor<Block>: BlockNumberOps + Unpin, |
There was a problem hiding this comment.
Suggested change
| NumberFor<Block>: BlockNumberOps + Unpin, | |
| NumberFor<Block>: BlockNumberOps, |
client/finality-grandpa/src/lib.rs
Outdated
| @@ -551,10 +551,10 @@ pub fn run_grandpa_voter<B, E, Block: BlockT, N, RA, SC, VR, X, Sp>( | |||
| Block::Hash: Ord, | |||
| B: Backend<Block> + 'static, | |||
| E: CallExecutor<Block> + Send + Sync + 'static, | |||
| N: NetworkT<Block> + Send + Sync + Clone + 'static, | |||
| N: NetworkT<Block> + Send + Sync + Clone + Unpin + 'static, | |||
client/finality-grandpa/src/lib.rs
Outdated
| N: NetworkT<Block> + Sync, | ||
| NumberFor<Block>: BlockNumberOps, | ||
| N: NetworkT<Block> + Sync + Unpin, | ||
| NumberFor<Block>: BlockNumberOps + Unpin, |
Instead of requiring `Unpin` on all generic trait bounds involved in anything ever `poll`ed, one can implement `Unpin` on the structs being `poll`ed themselves.
|
@tomaka can you take another look? |
tomaka
approved these changes
Jan 16, 2020
Co-Authored-By: André Silva <[email protected]>
Co-Authored-By: André Silva <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
NeighborPacketWorkerwithinclient/finality-grandpadoes twothings:
It receives neighbor packets from components within
client/finality-grandpa, sends them down to theGossipEngineinorder for neighboring nodes to receive.
It periodically sends out the most recent neighbor packet to the
GossipEngine.In order to send out packets it had a clone to a
GossipEginewithinan atomic reference counter and a mutex. The
NeighborPacketWorkerwasthen spawned onto its own asynchronous task.
Instead of running in its own task, this patch reintegrates the
NeighborPacketWorkerinto the mainclient/finality-grandpatask notrequiring the
NeighborPacketWorkerto own a clone of theGossipEngine.The greater picture
This is a tiny change within a greater refactoring. The overall goal is
to simplify how finality-grandpa interacts with the network and to
reduce the amount of unbounded channels within the logic.
Why no unbounded channels: Bounding channels is needed for backpressure
and proper scheduling. With unbounded channels there is no way of
telling the producer side to slow down for the consumer side to catch
up. Rephrased, there is no way for the scheduler to know when to favour
the consumer task over the producer task on a crowded channel and the
other way round for an empty channel.
Reducing the amount of shared ownership simplifies the logic and enables
one to use async-await syntax-suggar, given that one does not need to
hold a lock across poll invocations. Using async-await enables one to
use bounded channels without complex logic.