Update to work with async keystore – Companion PR for #7000 #1740

rakanalh · 2020-09-21T12:32:04Z

Burnin PR: #1746

node/network/bitfield-distribution/src/lib.rs

mxinden

I don't think I am the right person to approve anything touching Polkadot as I have not been involved in this part of the code base enough to do so.

node/core/provisioner/Cargo.toml

mxinden · 2020-09-23T12:23:51Z

node/core/provisioner/src/lib.rs

 		}

 		#[test]
 		fn not_more_than_one_per_validator() {
+			let keystore_path = tempfile::tempdir().expect("Creates keystore path");
+			let keystore : CryptoStorePtr = Arc::new(LocalKeystore::open(keystore_path.path(), None)


Likely a dump question: Wouldn't a in memory keystore suffice here?

Good question. However, generating keys without seeds would skip adding those keys to additional (cache) and go straight to writing them on the filesystem. Without configuring the keystore to use the filesystem, adding any key would generate the key and then lose sight of it. This is why i opted for configuring the keystore to write on the filesystem.

Without configuring the keystore to use the filesystem, adding any key would generate the key and then lose sight of it

Could you elaborate? We are storing the pubkeys in the VALIDATORS hashmap though, why do we need a seed? Are you saying that if we replace LocalKeystore::open with LocalKeystore::in_memory the tests will fail?

If so, this warrants a comment in code.

I am not sure VALIDATORS here is needed anymore but didn't want to remove it. However, to answer your question, and according to substrate's keystore impl, generating a key without a seed would trigger this function which requires that the keystore's path is configured rather than using the in-memory storage.

I will add a comment about this.

Comment added. VALIDATORS removed.

Thanks for the clarification.
It's still can be avoided maybe by e.g. using dump seeds. But it's only used in tests anyway, so probably not worth time spent.

I am not sure VALIDATORS here is needed anymore but didn't want to remove it.

I think it's still needed (we're expecting the same key for the given ValidatorIndex), cc @coriolinus

node/network/bitfield-distribution/src/lib.rs

runtime/parachains/src/inclusion.rs

ordian

Looks reasonable.

ghost · 2020-10-08T20:57:03Z

Waiting for commit status.

node/core/backing/Cargo.toml

node/core/backing/src/lib.rs

ghost · 2020-10-08T21:05:38Z

Head SHA changed; merge aborted.

bkchr · 2020-10-08T22:01:34Z

node/network/availability-distribution/src/lib.rs

-		} else {
-			None
+	for (idx, validator) in validators.iter().enumerate() {
+		if CryptoStore::has_keys(&*keystore, &[(validator.to_raw_vec(), PARACHAIN_KEY_TYPE_ID)]).await {


This will be absolutely slow in the moment we have remote signing. Sounds like we need a function that returns all the indices of the keys that it has.

So we only make one remote call.

This needs a change in the way that has_keys works where it should return the index of the items found from the array sent as an argument instead of a bool. This could be implemented as a follow-up PR to both substrate and polkadot. What do you think?

paritytech/substrate#7285

Yeah, the idea was to fix this in a follow up pr.

bkchr · 2020-10-08T22:08:26Z

node/subsystem-util/src/lib.rs

-		.find_map(|v| keystore.key_pair::<ValidatorPair>(&v).ok())
+pub async fn signing_key(validators: &[ValidatorId], keystore: SyncCryptoStorePtr) -> Option<ValidatorId> {
+	for v in validators.iter() {
+		if CryptoStore::has_keys(&*keystore, &[(v.to_raw_vec(), ValidatorId::ID)]).await {


Same as above.

primitives/src/v0.rs

Co-authored-by: Bastian Köcher <[email protected]>

…ystore-fixes

rakanalh · 2020-10-09T10:53:57Z

bot merge

ghost · 2020-10-09T10:54:02Z

Trying merge.

tomaka · 2020-10-09T10:56:45Z

gnunicorn added the B1-releasenotes label 3 minutes ago

I don't know what is the right thing, but fwiw I put silent because the Substrate PR already has the appropriate labels.

* start working on building the real overseer Unfortunately, this fails to compile right now due to an upstream failure to compile which is probably brought on by a recent upgrade to rustc v1.47. * fill in AllSubsystems internal constructors * replace fn make_metrics with Metrics::attempt_to_register * update to account for #1740 * remove Metrics::register, rename Metrics::attempt_to_register * add 'static bounds to real_overseer type params * pass authority_discovery and network_service to real_overseer It's not straightforwardly obvious that this is the best way to handle the case when there is no authority discovery service, but it seems to be the best option available at the moment. * select a proper database configuration for the availability store db * use subdirectory for av-store database path * apply Basti's patch which avoids needing to parameterize everything on Block * simplify path extraction * get all tests to compile * Fix Prometheus double-registry error for debugging purposes, added this to node/subsystem-util/src/lib.rs:472-476: ```rust Some(registry) => Self::try_register(registry).map_err(|err| { eprintln!("PrometheusError calling {}::register: {:?}", std::any::type_name::<Self>(), err); err }), ``` That pointed out where the registration was failing, which led to this fix. The test still doesn't pass, but it now fails in a new and different way! * authorities must have authority discovery, but not necessarily overseer handlers * fix broken SpawnedSubsystem impls detailed logging determined that using the `Box::new` style of future generation, the `self.run` method was never being called, leading to dropped receivers / closed senders for those subsystems, causing the overseer to shut down immediately. This is not the final fix needed to get things working properly, but it's a good start. * use prometheus properly Prometheus lets us register simple counters, which aren't very interesting. It also allows us to register CounterVecs, which are. With a CounterVec, you can provide a set of labels, which can later be used to filter the counts. We were using them wrong, though. This pattern was repeated in a variety of places in the code: ```rust // panics with an cardinality mismatch let my_counter = register(CounterVec::new(opts, &["succeeded", "failed"])?, registry)?; my_counter.with_label_values(&["succeeded"]).inc() ``` The problem is that the labels provided in the constructor are not the set of legal values which can be annotated, but a set of individual label names which can have individual, arbitrary values. This commit fixes that. * get av-store subsystem to actually run properly and not die on first signal * typo fix: incomming -> incoming * don't disable authority discovery in test nodes * Fix rococo-v1 missing session keys * Update node/core/av-store/Cargo.toml * try dummying out av-store on non-full-nodes * overseer and subsystems are required only for full nodes * Reduce the amount of warnings on browser target * Fix two more warnings * InclusionInherent should actually have an Inherent module on rococo * Ancestry: don't return genesis' parent hash * Update Cargo.lock * fix broken test * update test script: specify chainspec as script argument * Apply suggestions from code review Co-authored-by: Bastian Köcher <[email protected]> * Update node/service/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * node/service/src/lib: Return error via ? operator * post-merge blues * add is_collator flag * prevent occasional av-store test panic * simplify fix; expand application * run authority_discovery in Role::Discover when collating * distinguish between proposer closed channel errors * add IsCollator enum, remove is_collator CLI flag * improve formatting * remove nop loop * Fix some stuff Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Fedor Sakharov <[email protected]> Co-authored-by: Robert Habermeier <[email protected]> Co-authored-by: Bastian Köcher <[email protected]> Co-authored-by: Max Inden <[email protected]>

* start working on building the real overseer Unfortunately, this fails to compile right now due to an upstream failure to compile which is probably brought on by a recent upgrade to rustc v1.47. * fill in AllSubsystems internal constructors * replace fn make_metrics with Metrics::attempt_to_register * update to account for #1740 * remove Metrics::register, rename Metrics::attempt_to_register * add 'static bounds to real_overseer type params * pass authority_discovery and network_service to real_overseer It's not straightforwardly obvious that this is the best way to handle the case when there is no authority discovery service, but it seems to be the best option available at the moment. * select a proper database configuration for the availability store db * use subdirectory for av-store database path * apply Basti's patch which avoids needing to parameterize everything on Block * simplify path extraction * get all tests to compile * Fix Prometheus double-registry error for debugging purposes, added this to node/subsystem-util/src/lib.rs:472-476: ```rust Some(registry) => Self::try_register(registry).map_err(|err| { eprintln!("PrometheusError calling {}::register: {:?}", std::any::type_name::<Self>(), err); err }), ``` That pointed out where the registration was failing, which led to this fix. The test still doesn't pass, but it now fails in a new and different way! * authorities must have authority discovery, but not necessarily overseer handlers * fix broken SpawnedSubsystem impls detailed logging determined that using the `Box::new` style of future generation, the `self.run` method was never being called, leading to dropped receivers / closed senders for those subsystems, causing the overseer to shut down immediately. This is not the final fix needed to get things working properly, but it's a good start. * use prometheus properly Prometheus lets us register simple counters, which aren't very interesting. It also allows us to register CounterVecs, which are. With a CounterVec, you can provide a set of labels, which can later be used to filter the counts. We were using them wrong, though. This pattern was repeated in a variety of places in the code: ```rust // panics with an cardinality mismatch let my_counter = register(CounterVec::new(opts, &["succeeded", "failed"])?, registry)?; my_counter.with_label_values(&["succeeded"]).inc() ``` The problem is that the labels provided in the constructor are not the set of legal values which can be annotated, but a set of individual label names which can have individual, arbitrary values. This commit fixes that. * get av-store subsystem to actually run properly and not die on first signal * typo fix: incomming -> incoming * don't disable authority discovery in test nodes * Fix rococo-v1 missing session keys * Update node/core/av-store/Cargo.toml * try dummying out av-store on non-full-nodes * overseer and subsystems are required only for full nodes * Reduce the amount of warnings on browser target * Fix two more warnings * InclusionInherent should actually have an Inherent module on rococo * Ancestry: don't return genesis' parent hash * Update Cargo.lock * fix broken test * update test script: specify chainspec as script argument * Apply suggestions from code review Co-authored-by: Bastian Köcher <[email protected]> * Update node/service/src/lib.rs Co-authored-by: Bastian Köcher <[email protected]> * node/service/src/lib: Return error via ? operator * post-merge blues * add is_collator flag * prevent occasional av-store test panic * simplify fix; expand application * run authority_discovery in Role::Discover when collating * distinguish between proposer closed channel errors * add IsCollator enum, remove is_collator CLI flag * improve formatting * remove nop loop * Fix some stuff * Adds test parachain adder collator * Add sudo to Rococo, change session length to 30 seconds and some renaming * Update to the latest changes on master * Some fixes * Fix compilation * Update parachain/test-parachains/adder/collator/src/lib.rs Co-authored-by: Sergei Shulepov <[email protected]> * Review comments * Downgrade transaction version * Fixes * MOARE * Register notification protocols * utils: remove unused error * av-store: more resilient to some errors * address review nits * address more review nits Co-authored-by: Peter Goodspeed-Niklaus <[email protected]> Co-authored-by: Andronik Ordian <[email protected]> Co-authored-by: Fedor Sakharov <[email protected]> Co-authored-by: Robert Habermeier <[email protected]> Co-authored-by: Peter Goodspeed-Niklaus <[email protected]> Co-authored-by: Max Inden <[email protected]> Co-authored-by: Sergey Shulepov <[email protected]> Co-authored-by: Sergei Shulepov <[email protected]>

Rakan Alhneiti added 3 commits September 9, 2020 17:45

Fix keystore types

605bed8

Merge remote-tracking branch 'upstream/master' into keystore-fixes

55f59c3

Use SyncCryptoStorePtr

e60ba32

rakanalh added A3-in_progress Pull request is in progress. No review needed at this stage. A4-companion A PR that needs a companion PR to merge in parallel for one of its downstream dependencies. labels Sep 21, 2020

rakanalh self-assigned this Sep 21, 2020

rakanalh mentioned this pull request Sep 21, 2020

Async keystore + Authority-Discovery async/await paritytech/substrate#7000

Merged

Rakan Alhneiti added 2 commits September 21, 2020 14:48

Borrow keystore

7a49293

Fix unused imports

1072116

rakanalh requested review from mxinden and rphmeier September 21, 2020 12:52

Rakan Alhneiti added 2 commits September 21, 2020 14:58

Merge remote-tracking branch 'upstream/master' into keystore-fixes

c86f635

Fix polkadot service

6fc0496

mxinden reviewed Sep 21, 2020

View reviewed changes

node/network/bitfield-distribution/src/lib.rs Outdated Show resolved Hide resolved

Rakan Alhneiti added 9 commits September 21, 2020 18:24

Fix bitfield-distribution tests

950860e

Merge remote-tracking branch 'upstream/master' into keystore-fixes

95b9c82

Fix indentation

5c16b3b

Fix backing tests

b6a15e5

Fix tests

8aed67d

Fix provisioner tests

2a932af

Merge remote-tracking branch 'upstream/master' into keystore-fixes

75e54e6

Merge remote-tracking branch 'upstream/master' into keystore-fixes

4812ec7

Removed SyncCryptoStorePtr

59866d2

rakanalh requested a review from mxinden September 23, 2020 02:39

Fix services

0ddfa8c

mxinden reviewed Sep 23, 2020

View reviewed changes

Address PR feedback

f3882b4

rakanalh requested review from bkchr and gnunicorn September 23, 2020 12:43

rakanalh added the A1-needsburnin label Sep 23, 2020

Fix node service - 3

e799871

ordian approved these changes Oct 7, 2020

View reviewed changes

Rakan Alhneiti and others added 3 commits October 8, 2020 19:46

Rename CryptoStorePtr to SyncCryptoStorePtr

8441c63

Merge remote-tracking branch 'upstream/master' into keystore-fixes

4423c88

"Update Substrate"

590cc51

bkchr approved these changes Oct 8, 2020

View reviewed changes

node/core/backing/Cargo.toml Outdated Show resolved Hide resolved

node/core/backing/src/lib.rs Outdated Show resolved Hide resolved

node/core/backing/src/lib.rs Outdated Show resolved Hide resolved

bkchr added 2 commits October 8, 2020 23:03

Apply suggestions from code review

e5e9174

Update node/core/backing/Cargo.toml

9ce9ecc

bkchr reviewed Oct 8, 2020

View reviewed changes

Update primitives/src/v0.rs

d2787bc

Co-authored-by: Bastian Köcher <[email protected]>

rakanalh mentioned this pull request Oct 8, 2020

keystore.has_keys behaviour should return the indicies found paritytech/substrate#7285

Open

Rakan Alhneiti added 3 commits October 9, 2020 11:23

Fix wasm build

fa43595

Merge branch 'keystore-fixes' of github.com:rakanalh/polkadot into ke…

738914e

…ystore-fixes

Update Cargo.lock

306f3cb

ordian mentioned this pull request Oct 9, 2020

start working on building the real overseer #1795

Merged

tomaka added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Oct 9, 2020

gnunicorn added B1-releasenotes and removed B0-silent Changes should not be mentioned in any release notes labels Oct 9, 2020

gnunicorn changed the title ~~Companion PR for #7000~~ Update to work with async keystore – Companion PR for #7000 Oct 9, 2020

ghost merged commit bc7d132 into paritytech:master Oct 9, 2020

rakanalh deleted the keystore-fixes branch October 9, 2020 10:56

coriolinus added a commit that referenced this pull request Oct 9, 2020

update to account for #1740

e769207

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to work with async keystore – Companion PR for #7000 #1740

Update to work with async keystore – Companion PR for #7000 #1740

rakanalh commented Sep 21, 2020 •

edited

Loading

mxinden left a comment

mxinden Sep 23, 2020

rakanalh Sep 23, 2020

ordian Sep 23, 2020

rakanalh Sep 28, 2020

rakanalh Sep 28, 2020

ordian Sep 28, 2020

ordian Sep 28, 2020

ordian left a comment

ghost commented Oct 8, 2020

ghost commented Oct 8, 2020

bkchr Oct 8, 2020

bkchr Oct 8, 2020

rakanalh Oct 8, 2020 •

edited

Loading

rakanalh Oct 8, 2020

bkchr Oct 9, 2020

bkchr Oct 8, 2020

rakanalh commented Oct 9, 2020

ghost commented Oct 9, 2020

tomaka commented Oct 9, 2020 •

edited

Loading

Update to work with async keystore – Companion PR for #7000 #1740

Update to work with async keystore – Companion PR for #7000 #1740

Conversation

rakanalh commented Sep 21, 2020 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

ghost commented Oct 8, 2020

ghost commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakanalh Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakanalh commented Oct 9, 2020

ghost commented Oct 9, 2020

tomaka commented Oct 9, 2020 • edited Loading

rakanalh commented Sep 21, 2020 •

edited

Loading

rakanalh Oct 8, 2020 •

edited

Loading

tomaka commented Oct 9, 2020 •

edited

Loading