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

Async keystore + Authority-Discovery async/await #7000

Merged
139 commits merged into from
Oct 8, 2020
Merged

Async keystore + Authority-Discovery async/await #7000

139 commits merged into from
Oct 8, 2020

Conversation

rakanalh
Copy link
Contributor

@rakanalh rakanalh commented Sep 1, 2020 •
edited
Loading

polkadot companion: paritytech/polkadot#1740

This PR introduces:

  • Currently, only a "local" keystore is supported but a remote one can be introduced.
  • All keystore methods are async
  • SyncCryptoStore wraps the asynchronous keystore to hide block_on from sync crates.
  • Authority discovery was converted to async/await with the use of the async version of the keystore.
  • All other crates are temporarily using SyncCryptoStore until converted to use async/await

Rakan Alhneiti added 30 commits August 10, 2020 11:50
Asyncify sign_with
6ca9f1b
Asyncify generate/get keys
139e31b
Complete BareCryptoStore asyncification
0a9ff21
Cleanup
2d4e2c9
Rebase
dd001bb
Add Proxy
1949f7c
Inject keystore proxy into extensions
88d48e6
Implement some methods
a6f6ffc
Await on send
d7714e9
Cleanup
73a32cf
Send result over the oneshot channel sender
b540f7a
Process one future at a time
391fd8a
Fix cargo stuff
0c352da
Asyncify sr25519_vrf_sign
0bf4779
Cherry-pick and fix changes
700dcbf
Introduce SyncCryptoStore
de8d0a9
SQUASH ME WITH THE first commit
ec5cc86
Implement into SyncCryptoStore
6fd1dfd
Implement BareCryptoStore for KeystoreProxyAdapter
8c69a47
authority-discovery
0f4a993
AURA
f199295
BABE
f1b9a50
finality-grandpa
a7dfb5b
offchain-workers
8be5932
benchmarking-cli
05734ca
sp_io
e333cc6
test-utils
c25fc7b
application-crypto
fcb9fff
Extensions and RPC
8393835
Client Service
d428bc2
@rakanalh
Copy link
Contributor Author

rakanalh commented Oct 5, 2020 •
edited
Loading

@mxinden @bkchr @gnunicorn I have implemented the suggestion above and reverted the change which imports async-std and replaces usage of futures::executor::block_on.

However, you guys didn't like introducing KeystoreContainer before, which was removed but then i had to restore that. This is because if i end up returning Arc<dyn SyncCryptoStore>, this type cannot be coerced to Arc<dyn CryptoStore>. Restoring KeystoreContainer allows me to coerce Arc<LocalKeystore> into both Arc<dyn SyncCryptoStore> and Arc<dyn CryptoStore>.

Would be nice if you can check out my latest changes so that if i get the go ahead, i'll change the polkadot companion PR.

bkchr
bkchr reviewed Oct 5, 2020
View reviewed changes
bin/node/rpc/Cargo.toml Outdated Show resolved Hide resolved
client/consensus/babe/src/tests.rs Outdated Show resolved Hide resolved
client/consensus/pow/Cargo.toml Outdated Show resolved Hide resolved
frame/example-offchain-worker/Cargo.toml Outdated Show resolved Hide resolved
primitives/consensus/common/Cargo.toml Outdated Show resolved Hide resolved
primitives/keystore/Cargo.toml Outdated Show resolved Hide resolved
primitives/keystore/src/testing.rs Outdated Show resolved Hide resolved
client/keystore/src/local.rs Show resolved Hide resolved
client/keystore/src/local.rs Outdated Show resolved Hide resolved
client/keystore/src/local.rs Outdated Show resolved Hide resolved
@rakanalh rakanalh requested a review from bkchr October 6, 2020 10:57
@rakanalh
Copy link
Contributor Author

rakanalh commented Oct 7, 2020

The PR should be good to merge if no further feedback is provided.

@rakanalh
Copy link
Contributor Author

rakanalh commented Oct 7, 2020

@bkchr i have had some review feedback from @ordian on the companion PR but i also think it's good to merge as well.

bkchr
bkchr reviewed Oct 8, 2020
View reviewed changes
primitives/keystore/src/testing.rs Outdated Show resolved Hide resolved
primitives/keystore/src/lib.rs Outdated Show resolved Hide resolved
primitives/finality-grandpa/src/lib.rs Outdated Show resolved Hide resolved
primitives/finality-grandpa/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
client/keystore/src/local.rs Outdated Show resolved Hide resolved
client/consensus/babe/rpc/src/lib.rs Outdated Show resolved Hide resolved
client/cli/src/runner.rs Outdated Show resolved Hide resolved
bin/node/cli/src/service.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Oct 8, 2020

Some last nitpicks, after that it should be good to merge.

@rakanalh rakanalh requested a review from kianenigma as a code owner October 8, 2020 17:48
@rakanalh rakanalh requested review from bkchr and removed request for andresilva and kianenigma October 8, 2020 18:10
@rakanalh
Copy link
Contributor Author

rakanalh commented Oct 8, 2020

bot merge

@ghost
Copy link

ghost commented Oct 8, 2020

Trying merge.

@ghost ghost merged commit a845ff3 into paritytech:master Oct 8, 2020
@rakanalh rakanalh deleted the async-keystore-auth-discovery branch October 8, 2020 20:57
ordian added a commit that referenced this pull request Oct 9, 2020
@ordian
Merge branch 'master' into ao-network-expose-incremental-priority-gro…
f089aaf 
…up-updates

* master:
  Async keystore + Authority-Discovery async/await (#7000)
  Fixes logging of target names with dashes (#7281)
  seal: Add automated weights for contract API calls (#7017)
  add ss58 id for nodle (#7279)
  Refactor CurrencyToVote (#6896)
  bump-allocator: document & poison (#7277)
  Reset flaming fir network (#7274)
  reschedule (#6860)
  Drop system cache for trie benchmarks (#7242)
  client: improve log formatting (#7272)
  Rework `InspectState` (#7271)
  SystemOrigin trait (#7226)
  Update ss58 registry for Dock network (#7263)
  .maintain/monitoring: Add alert when continuous task ends (#7250)
  Rename `TRANSACTION_VERSION` to `EXTRINSIC_VERSION` (#7258)
  Split block announce processing into two parts (#6958)
  Fix offchain election to respect the weight (#7215)
@rakanalh rakanalh mentioned this pull request Nov 4, 2020
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mxinden mxinden mxinden left review comments

@danforbes danforbes danforbes left review comments

@tomaka tomaka tomaka left review comments

@bkchr bkchr bkchr approved these changes

@gnunicorn gnunicorn gnunicorn approved these changes

@seunlanlege seunlanlege Awaiting requested review from seunlanlege

@expenses expenses Awaiting requested review from expenses

Assignees

@rakanalh rakanalh

Labels
A1-needs_burnin Pull request needs to be tested on a live validator node before merge. DevOps is notified via matrix C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rakanalh @tomaka @mxinden @bkchr @gnunicorn @danforbes @expenses @shawntabrizi