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

client/finality-grandpa: Reintegrate gossip validator report stream #4661

Merged
merged 2 commits into from
Jan 17, 2020

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Jan 17, 2020

The finality-grandpa GossipValidator is called by the GossipEngine
in a synchronous fashion on each gossip message. Its main task is to
decide whether to gossip the given message on, or whether to drop it.

In addition it also updates the reputation of a node's peers based on
the incoming gossip messages. To do so it needs to be able to report the
reputation change which it does through an unbounded channel (in order
to stay synchronous).

Previously the receiving side of this channel would be handled by a new
task, polling the channel and forwarding the changes to a clone of the
GossipEngine that it would own.

Instead the receiver of the above mentioned channel is now being polled
by the NetworkBridge within its Future::poll implementation.
Reputation changes are reported through the already existing
GossipEngine instance within NetworkBridge.

For details on the overall goal, see d4fbb89.

The `finality-grandpa` `GossipValidator` is called by the `GossipEngine`
in a synchronous fashion on each gossip message. Its main task is to
decide whether to gossip the given message on, or whether to drop it.

In addition it also updates the reputation of a node's peers based on
the incoming gossip messages. To do so it needs to be able to report the
reputation change which it does through an unbounded channel (in order
to stay synchronous).

Previously the receiving side of this channel would be handled by a new
task, polling the channel and forwarding the changes to a clone of the
`GossipEngine` that it would own.

Instead the receiver of the above mentioned channel is now being polled
by the `NetworkBridge` within its `Future::poll` implementation.
Reputation changes are reported through the already existing
`GossipEngine` instance within `NetworkBridge`.

For details on the overall goal, see d4fbb89.
@mxinden mxinden added the A0-please_review Pull request needs code review. label Jan 17, 2020
// thus one has to wrap gossip_validator_report_stream with an `Arc` `Mutex`. Given that it is
// just an `UnboundedReceiver`, one could also switch to a multi-producer-*multi*-consumer
// channel implementation.
gossip_validator_report_stream: Arc<Mutex<mpsc03::UnboundedReceiver<PeerReport>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, what does the plan to phase out the unbounded channel look like after this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
pub(super) struct PeerReport {
pub who: PeerId,
pub cost_benefit: ReputationChange,
Copy link
Contributor

Choose a reason for hiding this comment

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

docs on this struct & members?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #4684

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

just the tiny docs nit, which you can ignore if you want

@rphmeier rphmeier added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jan 17, 2020
@gavofyork gavofyork merged commit f0c1852 into paritytech:master Jan 17, 2020
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.

was a bit late for this but lgtm!

@mxinden
Copy link
Contributor Author

mxinden commented Jan 20, 2020

out of curiosity, what does the plan to phase out the unbounded channel look like after this?

@rphmeier my plan roughly looks like this:

  1. I have a pull request for removing the unbounded within round_communication given that the function returns a Stream and a Sink, which have asynchronous interfaces.

  2. With client/finality-grandpa: Reintegrate periodic neighbor packet worker #4631 and this pull request and a couple of other changes (e.g. integrating offband worker tasks) GossipEngine would not need to be clonable anymore.

  3. Once that happened I can remove the struct { inner: Arc<Mutex<>> } construction on `GossipEngine.

  4. With exclusive ownership I can use async within messages_for, being able to remove its unbounded channel.

  5. ....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants