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

SecretStore: PoA integration initial version #7101

Merged
merged 43 commits into from Dec 29, 2017
Merged

SecretStore: PoA integration initial version #7101

merged 43 commits into from Dec 29, 2017

Conversation

svyatonik
Copy link
Collaborator

@svyatonik svyatonik commented Nov 20, 2017

Based on https://github.com/paritytech/contracts/pull/91

What is in this PR:

  1. I needed ability to submit transactions to contract && since I already had a native_generator-generated code for this, contract, I've also added ability to encode method arguments to generated code
  2. there are now 2 possible API endpoints for KeyServer: http && service contract. Both are optional (configuration methods are different, though)
  3. the only API method available via service contract is currently 'GenerateServerKey'. It generates distributed KeyPair, which can be used to sign messages later. It is not intended for encryption (encryption-related API methods are currently questionable && not a part of this PR)
  4. service contract listener:
    4.1) listens for ServerKeyRequested event && checks if this request should be processed by this KeyServer. It is because of SS specific - there must be a 'master node' in each session (including generation session) => we can't just start session on all nodes at once. Basically, all possible H256::max_value() server_key_id values are evenly distributed among all valid KeyServers. Note: it doesn't matter who actually is a master node - it is just to beat SS protocol caveats.
    4.2) when appropriate key is requested, node starts key generation. If everything is OK, key pair is generated && all nodes know a Public of this KeyPair. Every KeyServer signs this Public with its own key && sends tx back to the contract. When enough (threshold + 1) conformations are received, ServerKeyGenerated event is fired by the contract.
    4.3) if generation fails, there's no much we can do. Possible options I've considered:
    4.3.1) admit that we can't generate the key && send fee back to the requester;
    4.3.2) admit that we can't generate the key, but do not return fee (we actually have made some actions);
    4.3.3) retry until success (optimistically)
    Of these options I've selected the last one, because there's actually no error can occur because of requester - either there are some problems with SS configuration/nodes, or we're trying to start session during maintenance interval (like when ServersSetChange session is active).
    4.4) retry occurs every N (30) blocks && only M (1) 2-nd time failures are allowed during this retry. If M failures has occured => we stop retrying until next 30 blocks are mined.
  5. if key K1 is already generated && someone asks to generate key with the same K1 id, SS should just publish existing key

What is not in this PR (but still about SS+Kovan integration):

  1. other SS APIs (need to discuss if they're suitable for usage in public blockchain at all)
  2. when validators set changes, ServersSetChange session must be automatically started (next step)
  3. integration with validators set contract - currently validators set is fixed in service_validators_fixed.sol contract. Have failed to configure PoA, based on validators set contract. This shouldn't affect Rust code, though (as I see it now)
  4. proper testing - currently having issues with UI on master => have tested it on my local 1.8 branch, then cherry-picked back to master

@svyatonik svyatonik added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Nov 20, 2017
@5chdn 5chdn mentioned this pull request Nov 20, 2017
25 tasks
@5chdn 5chdn added this to the 1.9 milestone Nov 21, 2017
@svyatonik
Copy link
Collaborator Author

Thanks for such a detailed review, @tomusdrw !!! That's actually what I'm missing when working on SecretStore!
Tried to address all the issues && responded where appropriate.

@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 20, 2017
@svyatonik
Copy link
Collaborator Author

I see some responses came, while I was adding comments - taking a time to fix these

@svyatonik svyatonik added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 20, 2017
@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Dec 20, 2017
@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 21, 2017
/// Get hash of the last block with at least n confirmations.
fn get_confirmed_block_hash(client: &Client, confirmations: u64) -> Option<H256> {
client.block_number(BlockId::Latest)
.and_then(|b| b.checked_sub(confirmations))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an edge case, but I would say that for b < confirmations we should just return 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Just curious - the only reason is to process requests from block#0 asap (instead of waiting for 3 blocks)? Or is there some other problem with previous pattern?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think that getting None from this method would indicate some errornous state (non existing historic block?) and it wouldn't be exactyl the same semantics for block 0.

self.data.contract.update();
if !self.data.contract.is_actual() {
let enacted_len = enacted.len();
if enacted_len == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enacted.is_empty()?
EDIT: just noticed you are using enacted_len later on, so I guess it's fine.

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 27, 2017
@svyatonik svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Dec 27, 2017
@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 27, 2017
@debris debris merged commit f8bd6b9 into master Dec 29, 2017
@debris debris deleted the secretstore_kovan branch December 29, 2017 09:31
@bjornwgnr bjornwgnr changed the title SecretStore: Kovan integration initial version SecretStore: PoA integration initial version Jan 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants