This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

SecretStore: service pack 1 #8435

Merged

svyatonik merged 11 commits into master from secretstore_service_pack1

Jun 14, 2018

Collaborator

svyatonik commented Apr 18, 2018

on top of #8357
closes #7956

This is a PR for cumulative fixes (didn't wanted to spam with many SS PRs), made during SS initial nodes deployment on Kovan. Details:

registry entry 'secretstore_acl_checker' is already occupied on Kovan => had to unify configuration options for all 3 contracts, used by SS - now their addresses can be: "none" (or option absence) means do not use contract (ACL = no permissions check, KeyServerSet = nodes list is read from configuration, Service = we do not listen to any service contracts), "registry" means read contract address from registry and address means configure for contract from this address;
fixed: key servers listens to service contract even if it is isolated from SS - now contract events are ignored in this case;
fixed: when there's not enough nodes to restore document key, request 'hangs' in service contract - now master node broadcast key version negotiation error + every other node checks if this is actual error && publishes doc key retrieval error;
fixed: if key server is the only holder of key share, key version negotiation sessions completes without returning key threshold;
fixed: servers set change session timeouts when completes instantly (only isolated nodes are removed);
fixed: share add session fails when node known as version holder doesn't report ownership (when joining node, that has lost its SS database);
fixed: node can miss KeyServerSet contract state changes sometimes;
fixed: reading pending personal doc shadow data retrieval task from service contract;
fixed: internal error during ACL check (e.g. syncing is in progress) is considered fatal;
fixed/added useful traces where required.

svyatonik added 4 commits

April 13, 2018 11:50


          SecretStore: error unify initial commit

0fcaa8d

SecretStore: pass real error in error messages

SecretStore: is_internal_error -> Error::is_non_fatal

warnings

SecretStore: ConsensusTemporaryUnreachable

fix after merge

removed comments

removed comments

SecretStore: updated HTTP error responses

SecretStore: more ConsensusTemporaryUnreachable tests

fix after rebase


          Merge branch 'master' into secretstore_single_error

6a6eab3


          SecretStore: unified SS contract config options && read

feaf0ca


          SecretStore: service pack

00ebf4c

SecretStore: service pack (continue)

svyatonik added A0-pleasereview 🤓 M4-core ⛓ labels

5chdn modified the milestones: 1.11, 1.12

svyatonik added 2 commits

May 2, 2018 09:06


          Merge branch 'master' into secretstore_service_pack1

5ac4df0


          Merge branch 'master' into secretstore_service_pack1

5ddca09

Contributor

5chdn commented May 15, 2018

Test repeatedly failed:

tests::send_message_to_handler

Collaborator

niklasad1 commented May 15, 2018

@svyatonik @5chdn I think that error is mio related and rebase/merge to master should fix it!

Contributor

5chdn commented May 15, 2018

Ah, gotcha. @niklasad1 wanna try to give this PR the first review?

niklasad1 self-assigned this


          Merge branch 'master' into secretstore_service_pack1

39e72dc

niklasad1 reviewed

View reviewed changes

parity/configuration.rs Outdated

-              		"registry" => Ok(Some(SecretStoreContractAddress::Registry)),
-              		a => Ok(Some(SecretStoreContractAddress::Address(a.parse().map_err(|e| format!("{}", e))?))),
+              fn into_secretstore_service_contract_address(s: Option<&String>) -> Result<Option<SecretStoreContractAddress>, String> {
+              	match s.map(|x| &**x) {

Collaborator

niklasad1 May 24, 2018

I think this should be simplified to s.map(String::as_str) for readability purposes!

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/admin_sessions/key_version_negotiation_session.rs Outdated

+              					Some(FailedKeyVersionContinueAction::Decrypt(Some(ref origin), ref requester)) =>
+              						self.data.lock().failed_continue_with =
+              							Some(FailedContinueAction::Decrypt(Some(origin.clone().into()), requester.clone().into())),
+              					_ => (),

Collaborator

niklasad1 May 24, 2018

use if let instead because you only care about one pattern!

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/admin_sessions/key_version_negotiation_session.rs Outdated

@@ @@ -361,8 +391,47 @@ impl<T> SessionImpl<T> where T: SessionTransport { @@
               		let confirmations = data.confirmations.as_ref().expect(reason);
               		let versions = data.versions.as_ref().expect(reason);
               		if let Some(result) = core.result_computer.compute_result(data.threshold.clone(), confirmations, versions) {
+              			// when master node processing decryption service request, it starts with a key versions negotiation session

Collaborator

niklasad1 May 24, 2018

when the master node

a key version negotiation

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/admin_sessions/key_version_negotiation_session.rs Outdated

@@ @@ -361,8 +391,47 @@ impl<T> SessionImpl<T> where T: SessionTransport { @@
               		let confirmations = data.confirmations.as_ref().expect(reason);
               		let versions = data.versions.as_ref().expect(reason);
               		if let Some(result) = core.result_computer.compute_result(data.threshold.clone(), confirmations, versions) {
+              			// when master node processing decryption service request, it starts with a key versions negotiation session
+              			// if negotiation fails, only master node knows about it

Collaborator

niklasad1 May 24, 2018

if the negotiation fails

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/admin_sessions/key_version_negotiation_session.rs Outdated

@@ @@ -361,8 +391,47 @@ impl<T> SessionImpl<T> where T: SessionTransport { @@
               		let confirmations = data.confirmations.as_ref().expect(reason);
               		let versions = data.versions.as_ref().expect(reason);
               		if let Some(result) = core.result_computer.compute_result(data.threshold.clone(), confirmations, versions) {
+              			// when master node processing decryption service request, it starts with a key versions negotiation session
+              			// if negotiation fails, only master node knows about it
+              			// => if error is fatal, only master will know about it and report to the contract && request will never be rejected

Collaborator

niklasad1 May 24, 2018

if the error is fatal, only the master ...

´report it`

the request will ...

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/admin_sessions/key_version_negotiation_session.rs

		@@ -361,8 +391,47 @@ impl<T> SessionImpl<T> where T: SessionTransport {
		let confirmations = data.confirmations.as_ref().expect(reason);

Collaborator

niklasad1 May 24, 2018

Out-of-scope for this PR, would be nice if fn try_complete would return Result instead!

svyatonik added 3 commits

May 28, 2018 11:29


          fixed grumbles

bfbd52d


          Merge branch 'master' into secretstore_service_pack1

b3e7196


          Merge branch 'master' into secretstore_service_pack1

5ca5507

Contributor

5chdn commented May 31, 2018

Needs a 2nd review :)

Contributor

5chdn commented Jun 4, 2018

Long shot, @folsen do you want to review this? 🙉

folsen approved these changes

View reviewed changes

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/admin_sessions/servers_set_change_session.rs

-              						}
+              					let key_share_owners = message.version_holders.iter().cloned().map(Into::into).collect();
+              					let new_nodes_set = data.new_nodes_set.as_ref()
+              						.expect("new_nodes_set is filled during consensus establishing; change sessions are running after this; qed");

Collaborator

niklasad1 Jun 13, 2018

Can you explain why expect() here instead of returning an error?

Collaborator Author

svyatonik Jun 13, 2018

because I'm sure that the error won't ever happen. And I'm sure because...
...the ServerSetChange session consists of two (omitting details) phases. The first one (EstablishingConsensus) is when master node prepares session structure (i.e. asking all other nodes - if they want to participate in the session, what shares do they have && what to do with these shares - either leave as is or add/remove). The second phase starts (RunningShareChangeSessions) after first phase is finished and it is the phase where shares are actually altered (new shares are added/old shares are removed). on_initialize_share_change_session is called during a second phase - it is checked at the beginning of the method. And new_nodes_set is initialized during first phase - on master node during session initialize call and on other nodes - when UnknownSessionsRequest message is received (that's the transition from phase to phase2).

niklasad1 reviewed

View reviewed changes

secret_store/src/key_server_cluster/cluster_sessions.rs

               	pub fn make_cluster_sessions() -> ClusterSessions {
               		let key_pair = Random.generate().unwrap();
               		let config = ClusterConfiguration {
               			threads: 1,
               			self_key_pair: Arc::new(PlainNodeKeyPair::new(key_pair.clone())),
               			listen_address: ("127.0.0.1".to_owned(), 100_u16),
-              			key_server_set: Arc::new(MapKeyServerSet::new(vec![(key_pair.public().clone(), format!("127.0.0.1:{}", 100).parse().unwrap())].into_iter().collect())),
+              			key_server_set: Arc::new(MapKeyServerSet::new(false, vec![(key_pair.public().clone(), format!("127.0.0.1:{}", 100).parse().unwrap())].into_iter().collect())),

Collaborator

niklasad1 Jun 13, 2018

Replace format! with "127.0.0.1:100".parse().unwrap() to get rid of a String allocation!

niklasad1 approved these changes

View reviewed changes

Collaborator

niklasad1 left a comment

Looks good but I can't say that I fully understand everything. So, would probably good with another reviewer!

5chdn added A7-looksgoodcantmerge 🙄 and removed A0-pleasereview 🤓 labels

Contributor

5chdn commented Jun 13, 2018

I think this is good to go once you resolve conflicts


          Merge branch 'master' into secretstore_service_pack1

73ea119

svyatonik added A8-looksgood 🦄 and removed A7-looksgoodcantmerge 🙄 labels

svyatonik merged commit 6f758bc into master

svyatonik deleted the secretstore_service_pack1 branch

June 14, 2018 07:01

dvdplm added a commit that referenced this pull request


          Merge branch 'master' into refactor/hashdb-generic

8b219ad

* master:
  Minor fix in chain supplier and light provider (#8906)
  Block 0 is valid in queries (#8891)
  fixed osx permissions (#8901)
  Atomic create new files with permissions to owner in ethstore (#8896)
  Add ETC Cooperative-run load balanced parity node (#8892)
  Add support for --chain tobalaba (#8870)
  fix some warns on nightly (#8889)
  Add new ovh bootnodes and fix port for foundation bootnode 3.2 (#8886)
  SecretStore: service pack 1 (#8435)

tavakyan referenced this pull request in C4Coin/c4coin-parity


          SecretStore: service pack 1 (#8435)

5443f99

* SecretStore: error unify initial commit

SecretStore: pass real error in error messages

SecretStore: is_internal_error -> Error::is_non_fatal

warnings

SecretStore: ConsensusTemporaryUnreachable

fix after merge

removed comments

removed comments

SecretStore: updated HTTP error responses

SecretStore: more ConsensusTemporaryUnreachable tests

fix after rebase

* SecretStore: unified SS contract config options && read

* SecretStore: service pack

SecretStore: service pack (continue)

* fixed grumbles

ordian added a commit to ordian/parity that referenced this pull request


          Merge branch 'master' of https://github.com/paritytech/parity into wa…

fd88e46

…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity: (29 commits)
  Block 0 is valid in queries (openethereum#8891)
  fixed osx permissions (openethereum#8901)
  Atomic create new files with permissions to owner in ethstore (openethereum#8896)
  Add ETC Cooperative-run load balanced parity node (openethereum#8892)
  Add support for --chain tobalaba (openethereum#8870)
  fix some warns on nightly (openethereum#8889)
  Add new ovh bootnodes and fix port for foundation bootnode 3.2 (openethereum#8886)
  SecretStore: service pack 1 (openethereum#8435)
  Handle removed logs in filter changes and add geth compatibility field (openethereum#8796)
  fixed ipc leak, closes openethereum#8774 (openethereum#8876)
  scripts: remove md5 checksums (openethereum#8884)
  hardware_wallet/Ledger `Sign messages` + some refactoring (openethereum#8868)
  Check whether we need resealing in miner and unwrap has_account in account_provider (openethereum#8853)
  docker: Fix alpine build (openethereum#8878)
  Remove mac os installers etc (openethereum#8875)
  README.md: update the list of dependencies (openethereum#8864)
  Fix concurrent access to signer queue (openethereum#8854)
  Tx permission contract improvement (openethereum#8400)
  Limit the number of transactions in pending set (openethereum#8777)
  Use sealing.enabled to emit eth_mining information (openethereum#8844)
  ...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A8-looksgood 🦄 M4-core ⛓