Skip to content
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

Merged
merged 5 commits into from
Jan 17, 2020

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Jan 15, 2020

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.

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 mxinden added the A0-please_review Pull request needs code review. label 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>>>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

SC: SelectChain<Block> + 'static,
NumberFor<Block>: BlockNumberOps,
NumberFor<Block>: BlockNumberOps + Unpin,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong

NeighborPacketSender<B>,
) where
B: BlockT,
pub(super) struct NeighborPacketWorker<B: BlockT> {
Copy link
Contributor

Choose a reason for hiding this comment

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

could use a doc-string

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NumberFor<B>: Unpin,

You probably have to add impl<B, N> Unpin for NetworkBridge<B, N> {} somewhere instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, thank you and thank you @tomaka ❤️


impl <B: BlockT> Stream for NeighborPacketWorker<B>
where
NumberFor<B>: Unpin,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NumberFor<B>: Unpin,

SC: SelectChain<Block> + 'static,
VR: VotingRule<Block, Client<B, E, Block, RA>> + Clone + 'static,
NumberFor<Block>: BlockNumberOps,
NumberFor<Block>: BlockNumberOps + Unpin,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
NumberFor<Block>: BlockNumberOps + Unpin,
NumberFor<Block>: BlockNumberOps,

@@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this Unpin

N: NetworkT<Block> + Sync,
NumberFor<Block>: BlockNumberOps,
N: NetworkT<Block> + Sync + Unpin,
NumberFor<Block>: BlockNumberOps + Unpin,
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, you get the idea

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.
@mxinden
Copy link
Contributor Author

mxinden commented Jan 16, 2020

@tomaka can you take another look?

Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Not 100% confident in the logic of the change because I'm not familiar with the code base, but nothing looks wrong to me.

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

lgtm

client/finality-grandpa/src/communication/mod.rs Outdated Show resolved Hide resolved
client/finality-grandpa/src/communication/mod.rs Outdated Show resolved Hide resolved
mxinden and others added 2 commits January 17, 2020 10:58
Co-Authored-By: André Silva <andre.beat@gmail.com>
Co-Authored-By: André Silva <andre.beat@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants