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

client/network-gossip: Move sink IO outside of state_machine #5669

Merged
merged 5 commits into from
Apr 20, 2020

Conversation

mxinden
Copy link
Contributor

@mxinden mxinden commented Apr 16, 2020

ConsensusGossip is supposed to be a deterministic state machine.
GossipEngine wrapping ConsensusGossip should handle IO operations.

This commit moves the message_sink IO operations to GossipEngine.
More specifically on incoming messages a GossipEngine calls
ConsensusGossip::on_incoming to validate and register the messages.
ConsensusGossip returns the valid messages which are then forwarded by
GossipEngine to the upper layer via the message_sinks.

mxinden added 2 commits April 16, 2020 15:02
`ConsensusGossip` is supposed to be a deterministic state machine.
`GossipEngine` wrapping `ConsensusGossip` should handle IO operations.

This commit moves the `message_sink` IO operations to `GossipEngine`.
More specifically on incoming messages a `GossipEngine` calls
`ConsensusGossip::on_incoming` to validate and register the messages.
`ConsensusGossip` returns the valid messages which are then forwarded by
`GossipEngine` to the upper layer via the `message_sinks`.
@mxinden mxinden added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes labels Apr 16, 2020
@mxinden mxinden requested a review from tomaka April 16, 2020 13:08
@@ -544,22 +526,6 @@ mod tests {
assert_eq!(consensus.messages.len(), 2);
}

#[test]
fn can_keep_multiple_subscribers_per_topic() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been moved to bridge.rs and renamed to keeps_multiple_subscribers_per_topic_updated_with_both_old_and_new_messages.

"Pushing consensus message to sinks for {}.", topic,
);
entry.get_mut().retain(move |sink| {
if let Err(e) = sink.unbounded_send(notification.clone()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this pull request sending on the event channel is happening in an asynchronous context. Thus we could replace the unbounded channel with a bounded channel in a follow up pull request.

@mxinden
Copy link
Contributor Author

mxinden commented Apr 17, 2020

Thanks for the review @tomaka and thanks for the help @gnunicorn!

I am guessing that the polkadot-companion failure is not related.

 error[E0560]: struct `sp_version::RuntimeVersion` has no field named `transaction_version`
3249   --> /builds/parity/substrate/polkadot/runtime/polkadot/src/lib.rs:92:2
3250    |
3251 92 |     transaction_version: 1,
3252    |     ^^^^^^^^^^^^^^^^^^^ `sp_version::RuntimeVersion` does not have this field
3253    |
3254    = note: available fields are: `spec_name`, `impl_name`, `authoring_version`, `spec_version`, `impl_version`, `apis`

@gnunicorn
Copy link
Contributor

@mxinden still tests failing...

);

for (topic, notification) in to_forward.into_iter() {
Copy link
Contributor

Choose a reason for hiding this comment

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

For the follow-up (I won't introduce another CI cycle for this issue), the into_iter() call is redundant.

Copy link
Contributor Author

@mxinden mxinden Apr 21, 2020

Choose a reason for hiding this comment

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

👍 Fixed in f277e6c.

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. B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants