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

Private contract migration and offchain state sync #10748

Merged
merged 35 commits into from
Aug 16, 2019
Merged

Conversation

grbIzl
Copy link
Collaborator

@grbIzl grbIzl commented Jun 14, 2019

  1. This PR addresses the following use cases:
    1.1) Client can setup the private contract in order to store its state locally in DB and store its hash in the contact only. This allows to use unlimited amount of data in the private contract (see TODO section for temporal limitation)
    1.2) Members, that have access to the private contract key, can be modified, new members can be added. In this case newly added members will request the state from the peers.
  1. Usage of the offchain storage is regulated via command line startup flag --private-state-offchain. By default (if flag is not used) it's set to false (to use onchain storage).

  2. Current private transactions usage scenarios were modified for the case of offchain storage

3.1) Private contract's deployment
If not all the states of private contracts required for the deployment available, deployment returns the error PrivateStateNotFound. Client has to repeat the procedure later (see TODO section for improvement of the usability)

3.2) Make changes in the private contract
Originator:
If some states are missing, originator's request saved, missing states requested
After receiving requested states, save them and fire stored originator's request

Verificator:
If during verification some states are not available, state requested and verification request saved.
After receiving actual state, save it and proceed with verification.

3.3) Read of the private state
If some state is not available, the same error is returned, client should try again later
Meanwhile state is requested, stored after retrieving

  1. The following TODOs were omitted during the development

Closes #9198, closes #9201

@grbIzl grbIzl added the M4-core ⛓ Core client code / Rust. label Jun 14, 2019
@grbIzl grbIzl added the A0-pleasereview 🤓 Pull request needs code review. label Jun 14, 2019
Copy link
Collaborator

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Never reviewed anything private-tx related before, so left few questions/comments. Will do a final review once they're answered/addressed.

ethcore/private-tx/src/lib.rs Show resolved Hide resolved
ethcore/private-tx/src/lib.rs Outdated Show resolved Hide resolved
ethcore/sync/src/chain/handler.rs Show resolved Hide resolved
ethcore/sync/src/chain/mod.rs Show resolved Hide resolved
ethcore/sync/src/chain/mod.rs Outdated Show resolved Hide resolved

/// Wrapper over storage for the private states
pub struct PrivateStateStorage {
verification_requests: RwLock<Vec<Arc<VerifiedPrivateTransaction>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you actually need so many RwLocks - all the data seems to be logically connected. Like in this code:

(1)		self.state_storage.state_sync_completed(hash);
(2)		if self.state_storage.current_sync_state() == SyncState::Idle {
			trace!(target: "privatetx", "Private state sync completed, processing pending requests");
(3)			let creation_requests = self.state_storage.drain_creation_queue();

there are 3 calls which are using 3 different RwLock. And, for example, if check (2) passes (i.e. the state is Idle) and before line (3) someone adds more states to sync, drain_*() calls could actually return these new requests, for which states are missing => tx sign/verification will fail.

So probably reconsider API to avoid such issues (and leave <= 1 internal lock)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reworked. 2 locks are left. The second one is needed for stalling requests logic implementation (in order to keep currently processed hashes in the separate structure)

@jam10o-new jam10o-new added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Jul 11, 2019
@grbIzl grbIzl added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 16, 2019
@grbIzl grbIzl added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Aug 5, 2019
ethcore/private-tx/src/lib.rs Outdated Show resolved Hide resolved
ethcore/private-tx/src/state_store.rs Show resolved Hide resolved
ethcore/service/src/service.rs Show resolved Hide resolved
ethcore/private-tx/src/lib.rs Outdated Show resolved Hide resolved
ethcore/private-tx/src/lib.rs Outdated Show resolved Hide resolved
ethcore/private-tx/Cargo.toml Outdated Show resolved Hide resolved
ethcore/private-tx/src/state_store.rs Outdated Show resolved Hide resolved
ethcore/private-tx/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

The logic looks ok to me, although I've never reviewed anything private-tx related before too.

Copy link
Collaborator

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

lgtm

let current_time = Instant::now();
syncing_hashes
.iter()
.filter(|&(_, expiration_time)| *expiration_time >= current_time)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not an issue, but probably some optimization for future PRs - there's a LinkedHashMap struct - I've used it in similar cases (for detecting stale requests) to avoid iterating the whole HashMap.

@svyatonik svyatonik added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 13, 2019
@ordian ordian added this to the 2.7 milestone Aug 13, 2019
@grbIzl grbIzl removed the A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. label Aug 16, 2019
@grbIzl grbIzl merged commit 66e4410 into master Aug 16, 2019
@grbIzl grbIzl deleted the PrivateStateRetrieval branch August 16, 2019 12:46
dvdplm added a commit that referenced this pull request Aug 20, 2019
* master:
  Fix rlp decode for inline trie nodes. (#10980)
  Private contract migration and offchain state sync (#10748)
  manual publish jobs for releases, no changes for nightlies (#10977)
dvdplm added a commit that referenced this pull request Aug 20, 2019
* master:
  Fix rlp decode for inline trie nodes. (#10980)
  Private contract migration and offchain state sync (#10748)
  manual publish jobs for releases, no changes for nightlies (#10977)
dvdplm added a commit that referenced this pull request Aug 21, 2019
* master:
  Add a 2/3 quorum option to Authority Round. (#10909)
  Fix rlp decode for inline trie nodes. (#10980)
  Private contract migration and offchain state sync (#10748)
  manual publish jobs for releases, no changes for nightlies (#10977)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. M4-core ⛓ Core client code / Rust.
Projects
None yet
4 participants