This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
SecretStore: use in-memory transport in cluster tests #9850
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
svyatonik
added
A0-pleasereview 🤓
niklasad1
reviewed
Nov 28, 2018
secret_store/src/key_server.rs
Outdated
| let config = NetClusterConfiguration { | ||
| threads: config.threads, | ||
| let threads = config.threads; | ||
| let cconfig = NetClusterConfiguration { |
There was a problem hiding this comment.
consider renaming this to cluster_config
niklasad1
reviewed
Nov 28, 2018
secret_store/src/key_server.rs
Outdated
| @@ -460,7 +463,7 @@ pub mod tests { | |||
| #[test] | |||
| fn decryption_session_is_delegated_when_node_does_not_have_key_share() { | |||
| //::logger::init_log(); | |||
There was a problem hiding this comment.
Not related to this PR, however this logger/comment should be enabled or removed IMHO
niklasad1
reviewed
Nov 28, 2018
| } | ||
|
|
||
| struct MessageLoop { | ||
| pub struct MessageLoop<S> { | ||
| pub ml: MessagesLoop, |
There was a problem hiding this comment.
hrm, am I reading this correctly: is every MessageLoop instance containing another MessageLoop?
niklasad1
reviewed
Nov 28, 2018
| ml: &MessagesLoop, | ||
| idx: usize | ||
| ) -> SessionImpl { | ||
| meta.self_node_id = ml.node_key_pair(idx).public().clone(); |
There was a problem hiding this comment.
clone on Copy type
Suggested change
| meta.self_node_id = ml.node_key_pair(idx).public().clone(); | |
| meta.self_node_id = *ml.node_key_pair(idx).public(); |
niklasad1
reviewed
Nov 28, 2018
| // all active nodes set | ||
| let mut all_nodes_set: BTreeSet<_> = gml.nodes.keys() | ||
| let mut all_nodes_set: BTreeSet<_> = ml.nodes().iter() |
There was a problem hiding this comment.
Suggested change
| let mut all_nodes_set: BTreeSet<_> = ml.nodes().iter() | |
| let mut all_nodes_set: BTreeSet<_> = ml.nodes().into_iter() | |
| .filter(|n| !isolated_nodes_ids.contains(n)) | |
| .collect(); | |
niklasad1
reviewed
Nov 28, 2018
| .filter(|n| !isolated_nodes_ids.contains(n)) | ||
| .cloned() | ||
| .collect(); | ||
| // new nodes set includes all old nodes, except nodes being removed + all nodes being added | ||
| let new_nodes_set: BTreeSet<NodeId> = all_nodes_set.iter().cloned() | ||
| .chain(new_nodes_ids.iter().cloned()) | ||
| .chain(add.iter().map(|kp| kp.public().clone())) |
There was a problem hiding this comment.
Suggested change
| .chain(add.iter().map(|kp| kp.public().clone())) | |
| .chain(add.iter().map(|kp| *kp.public())) |
niklasad1
reviewed
Nov 28, 2018
| .filter(|n| !removed_nodes_ids.contains(n)) | ||
| .collect(); | ||
| all_nodes_set.extend(new_nodes_ids.iter().cloned()); | ||
| let mut old_set_to_sign = all_nodes_set.clone(); | ||
| all_nodes_set.extend(add.iter().map(|kp| kp.public().clone())); |
There was a problem hiding this comment.
Suggested change
| all_nodes_set.extend(add.iter().map(|kp| kp.public().clone())); | |
| all_nodes_set.extend(add.iter().map(|kp| *kp.public())); |
niklasad1
reviewed
Nov 28, 2018
| let mut old_set_to_sign = all_nodes_set.clone(); | ||
| all_nodes_set.extend(add.iter().map(|kp| kp.public().clone())); | ||
| if C::SIGN_NEW_NODES { | ||
| old_set_to_sign.extend(add.iter().map(|kp| kp.public().clone())); |
There was a problem hiding this comment.
Suggested change
| old_set_to_sign.extend(add.iter().map(|kp| kp.public().clone())); | |
| old_set_to_sign.extend(add.iter().map(|kp| *kp.public())); |
niklasad1
reviewed
Nov 28, 2018
| for isolated_node_id in &isolated_nodes_ids { | ||
| all_nodes_set.remove(isolated_node_id); | ||
| } | ||
|
|
||
| let meta = ShareChangeSessionMeta { | ||
| self_node_id: master_node_id.clone(), | ||
| master_node_id: master_node_id.clone(), | ||
| self_node_id: master.clone(), |
There was a problem hiding this comment.
NodeID is Copy
Suggested change
| self_node_id: master.clone(), | |
| self_node_id: master, |
niklasad1
reviewed
Nov 28, 2018
|
|
||
| // prepare set of nodes | ||
| let sessions: BTreeMap<_, _> = (0..ml.nodes().len()) | ||
| .map(|idx| (ml.node(idx), C::create(meta.clone(), admin_public.clone(), all_nodes_set.clone(), &ml, idx))) |
There was a problem hiding this comment.
Suggested change
| .map(|idx| (ml.node(idx), C::create(meta.clone(), admin_public.clone(), all_nodes_set.clone(), &ml, idx))) | |
| .map(|idx| (ml.node(idx), C::create(meta.clone(), admin_public, all_nodes_set.clone(), &ml, idx))) |
niklasad1
reviewed
Nov 28, 2018
| idx: usize | ||
| ) -> SessionImpl<IsolatedSessionTransport> { | ||
| let key_storage = ml.key_storage(idx).clone(); | ||
| let key_version = key_storage.get(&meta.id).unwrap().map(|ks| ks.last_version().unwrap().hash.clone()); |
niklasad1
reviewed
Nov 28, 2018
| let key_storage = ml.key_storage(idx).clone(); | ||
| let key_version = key_storage.get(&meta.id).unwrap().map(|ks| ks.last_version().unwrap().hash.clone()); | ||
|
|
||
| meta.self_node_id = ml.node_key_pair(idx).public().clone(); |
There was a problem hiding this comment.
Suggested change
| meta.self_node_id = ml.node_key_pair(idx).public().clone(); | |
| meta.self_node_id = *ml.node_key_pair(idx).public(); |
niklasad1
reviewed
Nov 28, 2018
| SessionImpl::new(SessionParams { | ||
| meta: meta.clone(), | ||
| transport: IsolatedSessionTransport::new(meta.id, key_version, 1, ml.cluster(idx).view().unwrap()), | ||
| key_storage: key_storage, |
There was a problem hiding this comment.
Suggested change
| key_storage: key_storage, | |
| key_storage, |
niklasad1
reviewed
Nov 28, 2018
|
|
||
| // try to add 1 node using this node as a master node | ||
| let add = vec![Random.generate().unwrap()]; | ||
| let master = add[0].public().clone(); |
There was a problem hiding this comment.
Suggested change
| let master = add[0].public().clone(); | |
| let master = *add[0].public(); |
niklasad1
reviewed
Nov 28, 2018
| let mut oldest_nodes_set = gml.0.nodes(); | ||
| oldest_nodes_set.remove(&node_to_isolate); | ||
| let add = (0..add).map(|_| Random.generate().unwrap()).collect::<Vec<_>>(); | ||
| let newest_nodes_set = add.iter().map(|kp| kp.public().clone()).collect::<Vec<_>>(); |
There was a problem hiding this comment.
Suggested change
| let newest_nodes_set = add.iter().map(|kp| kp.public().clone()).collect::<Vec<_>>(); | |
| let newest_nodes_set = add.iter().map(|kp| *kp.public()).collect::<Vec<_>>(); |
niklasad1
reviewed
Nov 28, 2018
| debug_assert!(::std::iter::once(&isolated_node_id).all(|n| vec![version_a.clone(), version_c.clone()] == | ||
| ml.nodes[n].key_storage.get(&Default::default()).unwrap().unwrap().versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); | ||
| ml.ml.key_storage_of(n).get(&Default::default()).unwrap().unwrap() | ||
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); |
There was a problem hiding this comment.
Suggested change
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); | |
| .versions.iter().map(|v| v.hash).collect::<Vec<_>>())); |
niklasad1
reviewed
Nov 28, 2018
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); | ||
| debug_assert!(::std::iter::once(&node_to_isolate).all(|n| vec![version_a.clone(), version_c.clone()] == | ||
| ml.ml.key_storage_of(n).get(&Default::default()).unwrap().unwrap() | ||
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); |
There was a problem hiding this comment.
Suggested change
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); | |
| .versions.iter().map(|v| v.hash).collect::<Vec<_>>())); |
niklasad1
reviewed
Nov 28, 2018
| debug_assert!(newest_nodes_set.iter().all(|n| vec![version_b.clone(), version_c.clone()] == | ||
| ml.nodes[n].key_storage.get(&Default::default()).unwrap().unwrap().versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); | ||
| ml.ml.key_storage_of(n).get(&Default::default()).unwrap().unwrap() | ||
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); |
There was a problem hiding this comment.
Suggested change
| .versions.iter().map(|v| v.hash.clone()).collect::<Vec<_>>())); | |
| .versions.iter().map(|v| v.hash).collect::<Vec<_>>())); |
niklasad1
reviewed
Nov 28, 2018
| pub fn second_slave(&self) -> &SessionImpl { | ||
| &self.nodes.values().nth(2).unwrap().session | ||
| pub fn take_message_confirm_initialization(&self) -> (NodeId, NodeId, ConfirmInitialization) { | ||
| match self.0.take_message() { |
There was a problem hiding this comment.
Optional, but I prefer if/else instead of pattern match with only two variants
niklasad1
reviewed
Nov 28, 2018
| assert_eq!(l.process_message(message.clone()).unwrap_err(), Error::InvalidStateForRequest); | ||
| let ml = MessageLoop::new(2).init(0).unwrap(); | ||
| let (from, to, msg) = ml.0.take_message().unwrap(); | ||
| ml.0.process_message(from.clone(), to.clone(), msg.clone()); |
There was a problem hiding this comment.
Suggested change
| ml.0.process_message(from.clone(), to.clone(), msg.clone()); | |
| ml.0.process_message(from, to, msg.clone()); |
niklasad1
reviewed
Nov 28, 2018
| let signature = ethkey::sign(requester.secret(), &SessionId::default()).unwrap(); | ||
| self.0.cluster(0).client() | ||
| .new_ecdsa_signing_session(Default::default(), signature.into(), key_version, message_hash.clone()) | ||
| .map(|_| (self, requester.public().clone(), message_hash)) |
There was a problem hiding this comment.
Suggested change
| .map(|_| (self, requester.public().clone(), message_hash)) | |
| .map(|_| (self, *requester.public(), message_hash)) |
niklasad1
reviewed
Nov 28, 2018
| let requester = Random.generate().unwrap(); | ||
| let signature = ethkey::sign(requester.secret(), &SessionId::default()).unwrap(); | ||
| self.0.cluster(0).client() | ||
| .new_ecdsa_signing_session(Default::default(), signature.into(), key_version, message_hash.clone()) |
There was a problem hiding this comment.
Suggested change
| .new_ecdsa_signing_session(Default::default(), signature.into(), key_version, message_hash.clone()) | |
| .new_ecdsa_signing_session(Default::default(), signature.into(), key_version, message_hash) |
niklasad1
reviewed
Nov 28, 2018
| Default::default(), | ||
| signature.into(), | ||
| key_version, | ||
| message_hash.clone()).map(|_| (self, requester.public().clone(), message_hash)) |
There was a problem hiding this comment.
Suggested change
| message_hash.clone()).map(|_| (self, requester.public().clone(), message_hash)) | |
| message_hash).map(|_| (self, *requester.public(), message_hash)) |
niklasad1
reviewed
Nov 28, 2018
| assert_eq!(sl.master().initialize(sl.version.clone(), 777.into()), Ok(())); | ||
| assert_eq!(sl.master().initialize(sl.version.clone(), 777.into()), Err(Error::InvalidStateForRequest)); | ||
| let (ml, _, _) = MessageLoop::new(1, 0).unwrap().init().unwrap(); | ||
| assert_eq!(ml.session_at(0).initialize(ml.key_version(), 777.into()), |
There was a problem hiding this comment.
nit: this fits on a single line (< 120 chars)
niklasad1
reviewed
Nov 29, 2018
| match message { | ||
| Message::Generation(message) => self | ||
| .process_message(&self.sessions.generation_sessions, connection, Message::Generation(message)) | ||
| .map(|_| ()).unwrap_or_default(), |
niklasad1
reviewed
Nov 29, 2018
| use key_server_cluster::cluster_sessions::AdminSession; | ||
| use key_server_cluster::cluster_connections::{Connection}; | ||
| use key_server_cluster::cluster_connections_net::{NetConnectionsContainer}; | ||
| use types::{Error, NodeId}; | ||
| use {NodeKeyPair}; |
There was a problem hiding this comment.
unnecessary blocks for single import
Suggested change
| use {NodeKeyPair}; | |
| use NodeKeyPair; |
niklasad1
reviewed
Nov 29, 2018
| @@ -95,6 +97,11 @@ pub struct TriggerConnections { | |||
| } | |||
|
|
|||
| impl SimpleConnectionTrigger { | |||
| /// Create new simple from cluster configuration. | |||
| pub fn with_config(config: &ClusterConfiguration) -> Self { | |||
| Self::new(config.key_server_set.clone(), config.self_key_pair.clone(), config.admin_public.clone()) | |||
There was a problem hiding this comment.
Suggested change
| Self::new(config.key_server_set.clone(), config.self_key_pair.clone(), config.admin_public.clone()) | |
| Self::new(config.key_server_set.clone(), config.self_key_pair.clone(), config.admin_public) |
niklasad1
reviewed
Nov 29, 2018
| use key_server_cluster::{MapKeyServerSet, PlainNodeKeyPair, KeyServerSetSnapshot, KeyServerSetMigration}; | ||
| use key_server_cluster::cluster_connections_net::{NetConnectionsContainer}; |
There was a problem hiding this comment.
unnecessary blocks for single import
niklasad1
reviewed
Nov 29, 2018
| @@ -21,7 +21,8 @@ use ethereum_types::H256; | |||
| use ethkey::Public; | |||
| use parking_lot::Mutex; | |||
| use key_server_cluster::{KeyServerSet, KeyServerSetSnapshot, KeyServerSetMigration, is_migration_required}; | |||
| use key_server_cluster::cluster::{ClusterClient, ClusterConnectionsData}; | |||
| use key_server_cluster::cluster::{ClusterConfiguration, ServersSetChangeParams}; | |||
| use key_server_cluster::cluster_connections_net::{NetConnectionsContainer}; | |||
There was a problem hiding this comment.
unnecessary blocks for single import
niklasad1
reviewed
Nov 29, 2018
| match signatures { | ||
| Ok((old_set_signature, new_set_signature)) => Some(ServersSetChangeParams { | ||
| session_id: None, | ||
| migration_id: Some(migration.id.clone()), |
There was a problem hiding this comment.
Suggested change
| migration_id: Some(migration.id.clone()), | |
| migration_id: Some(migration.id), |
niklasad1
approved these changes
Nov 29, 2018
niklasad1
added
A6-mustntgrumble 💦
svyatonik
added
A0-pleasereview 🤓
niklasad1
added
A8-looksgood 🦄
|
👍 needs a 2nd review |
|
could you rebase this @svyatonik? who wants to do a 2nd review @grbIzl @seunlanlege? |
|
Needs a 2nd review 🙄 😆 |
ordian
added a commit
that referenced
this pull request
Apr 5, 2019
* master: Add message to IO errors (#10324) chore(bump parity-daemonize): require rust >= 1.31 (#10359) SecretStore: use in-memory transport in cluster tests (#9850) Add fields to `memzero`'s Cargo.toml (#10362) snap: release untagged versions from branches to the candidate snap channel (#10357) fix(compilation warns): `no-default-features` (#10346)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #9635 (at least if my assumption that it has been caused by timeout is true)
This PR doesn't introduces any new functionality, though it has some (non-negligible) non-test code modifications. These are mostly about splitting huge
cluster.rsinto several files:cluster_connections.rs- contains trait for connections used by cluster + test implementation of these traits;cluster_connections_net.rs- network implementation of connection traits. This isn't a new code - it is moved from thecluster.rs;cluster_message_processor.rs- entity that connects cluster connections with cluster sessions.Test-related notes:
cluster.rsare now using in-memory connections implementation => no timeouts;key_server.rstests. I thought about switching to in-memory connections there too, but decided to leave as-is at least for now (until writing proper tests for net-connections). So treat these like integration tests for now. Also - iirc this timeout has never failed - 1s is enough for establishing network connections;cluster.rsand various*_session.rsare about switching to the newMessagesLoopfromcluster.rs(this removes a lot of duplicate && boilerplate code). It isn't finished yet (there are more*_session.rs), but enough for first PR.