client/authority-discovery: Allow to be run by sentry node by mxinden · Pull Request #5568 · paritytech/substrate

mxinden · 2020-04-07T18:47:38Z

When run as a sentry node, the authority discovery module does not
publish any addresses to the dht, but still discovers validators and
sentry nodes of validators.

polkadot companion: paritytech/polkadot#984

When run as a sentry node, the authority discovery module does not publish any addresses to the dht, but still discovers validators and sentry nodes of validators.

tomaka

Looks good to me

tomaka · 2020-04-14T14:17:34Z

 use sp_core::traits::BareCryptoStorePtr;
 use sp_runtime::{traits::Block as BlockT, generic::BlockId};
 use sp_api::ProvideRuntimeApi;
+// TODO: Needed?


Suggested change

// TODO: Needed?

There is an unused import, but it's not this one.

bkchr · 2020-04-16T10:18:01Z

Does not compile.

andresilva

minor nits. overall lgtm.

andresilva · 2020-04-16T11:55:40Z

+			let sentries;
+			let authority_discovery_role;
+
+			match role {
+				sc_service::config::Role::Authority { ref sentry_nodes } => {
+					sentries = sentry_nodes.clone();
+					authority_discovery_role = sc_authority_discovery::Role::Authority (
+						service.keystore(),
+					);
+				}
+				sc_service::config::Role::Sentry {..} => {
+					sentries = vec![];
+					authority_discovery_role = sc_authority_discovery::Role::Sentry;
+				}
+				_ => unreachable!("Due to outer matches! constraint; qed.")
+			}


Suggested change

let sentries;

let authority_discovery_role;

match role {

sc_service::config::Role::Authority { ref sentry_nodes } => {

sentries = sentry_nodes.clone();

authority_discovery_role = sc_authority_discovery::Role::Authority (

service.keystore(),

);

}

sc_service::config::Role::Sentry {..} => {

sentries = vec![];

authority_discovery_role = sc_authority_discovery::Role::Sentry;

}

_ => unreachable!("Due to outer matches! constraint; qed.")

}

let (sentries, authority_discovery_role) = match role {

sc_service::config::Role::Authority { ref sentry_nodes } => (

sentry_nodes.clone(),

sc_authority_discovery::Role::Authority(service.keystore()),

),

sc_service::config::Role::Sentry { .. } => (

vec![],

sc_authority_discovery::Role::Sentry,

),

_ => unreachable!("Due to outer matches! constraint; qed."),

};

Maybe we should move this code into some method in authority discovery so that we can re-use it in polkadot service. Do you think it would make sense for the AuthorityDiscovery::new to take sc_service::config::Role? Or maybe add a new constructor that takes it.

I did have AuthorityDiscovery::new not take sc_service::config::Role for two reasons:

Make the authority discovery module independent of the underlying network.

Make it impossible at compile time to pass a key store as a sentry or to not pass a key store as a validator.

Does the above sound reasonable @andresilva ? If you think the additional code in cli/service is not worth the benefits above I can move it into the authority discovery module.

Yeah, I didn't think of the sc_service dependency and also appreciate that keystore requirement is validated statically. Let's keep it in cli/service then.
I still prefer the code as written in my suggestion (I didn't test it, just wrote it in Github), mainly because I prefer expressions over statements. 😊

Ah, sorry I that I oversaw your suggestion @andresilva. Indeed a lot better. 57d141d.

andresilva · 2020-04-16T12:01:57Z

+	fn get_own_public_keys_within_authority_set(
+		key_store: &BareCryptoStorePtr,
+		client: &Client,
+	) -> Result<HashSet<AuthorityId>> {


If we just kept this method as is and made it return an empty HashSet instead for sentries, couldn't we keep the rest of the logic above unchanged? I guess we could still keep the early exit in the method above.

I am not sure returning an empty HashSet for sentry nodes would be more expressive. This reminds me of C code returning -1 on error.

Do you think the changes above are intrusive?

Co-Authored-By: André Silva <123550+andresilva@users.noreply.github.com>

mxinden

@andresilva thanks for the review! Would you mind taking another look?

mxinden · 2020-04-16T12:54:55Z

+			let sentries;
+			let authority_discovery_role;
+
+			match role {
+				sc_service::config::Role::Authority { ref sentry_nodes } => {
+					sentries = sentry_nodes.clone();
+					authority_discovery_role = sc_authority_discovery::Role::Authority (
+						service.keystore(),
+					);
+				}
+				sc_service::config::Role::Sentry {..} => {
+					sentries = vec![];
+					authority_discovery_role = sc_authority_discovery::Role::Sentry;
+				}
+				_ => unreachable!("Due to outer matches! constraint; qed.")
+			}


I did have AuthorityDiscovery::new not take sc_service::config::Role for two reasons:

Make the authority discovery module independent of the underlying network.

Make it impossible at compile time to pass a key store as a sentry or to not pass a key store as a validator.

Does the above sound reasonable @andresilva ? If you think the additional code in cli/service is not worth the benefits above I can move it into the authority discovery module.

mxinden · 2020-04-16T12:58:34Z

+	fn get_own_public_keys_within_authority_set(
+		key_store: &BareCryptoStorePtr,
+		client: &Client,
+	) -> Result<HashSet<AuthorityId>> {


I am not sure returning an empty HashSet for sentry nodes would be more expressive. This reminds me of C code returning -1 on error.

Do you think the changes above are intrusive?

andresilva · 2020-04-16T15:17:16Z

Needs merge with master to fix conflicts.

client/authority-discovery: Allow to be run by sentry node

0452ab8

When run as a sentry node, the authority discovery module does not publish any addresses to the dht, but still discovers validators and sentry nodes of validators.

mxinden added A0-please_review Pull request needs code review. B1-clientnoteworthy labels Apr 7, 2020

client/authority-discovery/src/lib: Wrap lines at 100 characters

3f1007c

tomaka approved these changes Apr 14, 2020

View reviewed changes

mxinden added 2 commits April 16, 2020 10:49

client/authority-discovery: Remove TODO and unused import

df4113b

Merge branch 'paritytech/master' into mxinden-authority-discovery-sentry

455df73

client/authority-discovery: Pass role to new unit tests

f6f4b17

andresilva suggested changes Apr 16, 2020

View reviewed changes

client/authority-discovery: Apply suggestions

8b0d2d6

Co-Authored-By: André Silva <123550+andresilva@users.noreply.github.com>

mxinden commented Apr 16, 2020

View reviewed changes

mxinden added 2 commits April 16, 2020 17:45

bin/node/cli/src/service: Use expressions instead of statements

57d141d

Merge branch 'paritytech/master' into mxinden-authority-discovery-sentry

9915b70

andresilva approved these changes Apr 16, 2020

View reviewed changes

andresilva merged commit 75134a2 into master Apr 16, 2020

andresilva deleted the mxinden-authority-discovery-sentry branch April 16, 2020 17:11

sorpaas mentioned this pull request Apr 24, 2020

Companion PR for #5501 and #5680 paritytech/polkadot#1012

Merged

Conversation

mxinden commented Apr 7, 2020

Uh oh!

tomaka left a comment

Choose a reason for hiding this comment

Uh oh!

tomaka Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Apr 16, 2020

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tomaka Apr 14, 2020 •

edited

Loading