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

Use sc_network::NetworkStateInfo instead of implementing redundant traits #4436

Merged
gavofyork merged 13 commits into from
Dec 24, 2019
Merged

Use sc_network::NetworkStateInfo instead of implementing redundant traits #4436

gavofyork merged 13 commits into from
Dec 24, 2019

Conversation

@kigawas
Copy link
Contributor

@kigawas kigawas commented Dec 18, 2019 •
edited
Loading

ref #4439

@rphmeier rphmeier added the A0-please_review Pull request needs code review. label Dec 18, 2019
@kigawas kigawas changed the title Implement local_peer_id for gossip Use sc_network::NetworkStateInfo instead of implementing redundant traits Dec 19, 2019
@kigawas kigawas mentioned this pull request Dec 19, 2019
Closed
tomusdrw
tomusdrw approved these changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm, just the preamble is missing in newly created file.

client/authority-discovery/src/tests.rs
@@ -0,0 +1,367 @@
use std::sync::{Arc, Mutex};
Copy link
Contributor

@tomusdrw tomusdrw Dec 19, 2019

Choose a reason for hiding this comment

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

This needs preamble. Why move the tests in the first place though? It's a common convention to just have an inline module.

Copy link
Contributor Author

@kigawas kigawas Dec 20, 2019

Choose a reason for hiding this comment

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

Besides less cognitive load, it'll be easier for me to format the code with rustfmt in a separate file :)

client/authority-discovery/src/lib.rs
use libp2p::Multiaddr;
use sc_network::specialization::NetworkSpecialization;
use sc_network::{DhtEvent, ExHashT};
use sc_network::{DhtEvent, ExHashT, NetworkStateInfo};
Copy link
Contributor

@tomusdrw tomusdrw Dec 19, 2019

Choose a reason for hiding this comment

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

I feel like these things (trait + primitives) should rather be moved to primitves, but it's a separate discussion.

Copy link
Contributor Author

@kigawas kigawas Dec 20, 2019

Choose a reason for hiding this comment

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

You can check #4450

@tomusdrw tomusdrw added A5-grumble and removed A0-please_review Pull request needs code review. labels Dec 19, 2019
@kigawas
Copy link
Contributor Author

kigawas commented Dec 20, 2019 

warning: unused variable: `message`
   --> primitives/runtime/src/lib.rs:383:3
    |
375 |       /// A custom error in a module
    |  _____-
376 | |     Module {
377 | |         /// Module index, matching the metadata module index
378 | |         index: u8,
...   |
382 | |         #[codec(skip)]
383 | |         message: Option<&'static str>,
    | |         ^^^^^^-
    | |_______________|
    |                 help: try ignoring the field: `message: _`
    |
    = note: `#[warn(unused_variables)]` on by default

this made the build fail

mxinden
mxinden reviewed Dec 20, 2019
View reviewed changes
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

I have a couple of smaller comments. Thanks for the refactoring!

I would prefer the CI fix to get into master in a separate pull request given that it is not related to this change set.

client/authority-discovery/src/lib.rs Outdated
where
Block: BlockT + 'static,
Network: NetworkProvider,
Network: NetworkProvider + NetworkStateInfo,
Copy link
Contributor

@mxinden mxinden Dec 20, 2019

Choose a reason for hiding this comment

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

Instead of adding + NetworkStateInfo here and below, why not do the following?

 pub trait NetworkProvider: NetworkStateInfo {

One could read this as: The authority discovery module needs something that provides access to a network with the following functions and everything that NetworkStateInfo offers.

Copy link
Contributor Author

@kigawas kigawas Dec 20, 2019

Choose a reason for hiding this comment

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

I'll do it. But actually we already have put_value and get_value in trait Behaviour, there's still some place to improve here.

client/network/src/service.rs

impl<B: BlockT + 'static, S: NetworkSpecialization<B>, H: ExHashT> NetworkService<B, S, H> {
/// Returns the network identity of the node.
pub fn local_peer_id(&self) -> PeerId {
Copy link
Contributor

@mxinden mxinden Dec 20, 2019

Choose a reason for hiding this comment

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

Consolidating NetworkService with NetworkStateInfo sounds good to me. Great catch. Polkadot might be using NetworkService::local_peer_id, but I guess it could just use NetworkStateInfo as well.

Copy link
Contributor Author

@kigawas kigawas Dec 20, 2019

Choose a reason for hiding this comment

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

We can even do better. I already addressed this in #4450

@kigawas
Copy link
Contributor Author

kigawas commented Dec 20, 2019

About the ci fix, since it's very simple and there should exist more specific solution to it, I'd like to get it merged in this pr and try fixing it in another way later.

The problem is rather subtle and urgent - it'll block everyone until it gets fixed.

@mxinden
Copy link
Contributor

mxinden commented Dec 20, 2019

@kigawas in regards to the CI failures: #4469 has been merged within the last 3 hours with a green CI, thus I think current master should be fine now.

@kigawas kigawas requested review from mxinden and tomusdrw December 23, 2019 11:04
tomusdrw
tomusdrw approved these changes Dec 23, 2019
View reviewed changes
Copy link
Contributor

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

mxinden
mxinden approved these changes Dec 23, 2019
View reviewed changes
Copy link
Contributor

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

This looks good to me. The patch set includes a bunch of (unrelated) changes to Cargo.lock. How do we usually proceed in this regard @tomusdrw?

@tomusdrw
Copy link
Contributor

tomusdrw commented Dec 23, 2019

Good catch @mxinden, we should not bundle those changes in that PR unless they are necessary. @kigawas could you please revert Cargo.lock changes and resubmit that as a separate PR so that we can easily review them in isolation?

@kigawas
Copy link
Contributor Author

kigawas commented Dec 23, 2019

fixed

@gavofyork gavofyork merged commit 85de8d9 into paritytech:master Dec 24, 2019
@kigawas kigawas deleted the gossip-local-peer-id branch December 25, 2019 08:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@andresilva andresilva Awaiting requested review from andresilva

@Demi-Marie Demi-Marie Awaiting requested review from Demi-Marie

2 more reviewers

@tomusdrw tomusdrw tomusdrw approved these changes

@mxinden mxinden mxinden approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kigawas @mxinden @tomusdrw @gavofyork @rphmeier