Call private contract methods from another private contract (read-only) #10086

grbIzl · 2018-12-19T13:11:28Z

This PR adds possibility to make calls of private contract methods from another private contract (with condition, that caller has access to the keys for both private contracts).
In order to achieve this, ABI of permissioning contract of Secret Store was extended with availableKeys method, that returns all keys, available for the account.
The further solution is rather straightforward: after patching the state for the current private contract, we also patch states for all private contracts, available for the node, and execute the private transaction on this state.
Some details of the solution:

Only read-only calls are supported. Client can modify the state of other private contracts during execution of the private transaction, but these changes won't be saved
All code, responsible for work with Secret Store keys, moved to the separate module

Closes #9199

sorpaas · 2019-01-03T10:53:36Z

ethcore/sync/src/tests/private.rs

@@ -24,7 +24,7 @@ use ethcore::CreateContractAddress;
 use transaction::{Transaction, Action};
 use ethcore::executive::{contract_address};
 use ethcore::test_helpers::{push_block_with_transactions};
-use ethcore_private_tx::{Provider, ProviderConfig, NoopEncryptor, Importer, SignedPrivateTransaction};
+use ethcore_private_tx::{Provider, ProviderConfig, NoopEncryptor, Importer, SignedPrivateTransaction, StoringKeyProvider	};


nit: extra tab

sorpaas · 2019-01-03T10:59:21Z

ethcore/private-tx/src/lib.rs

+		// Patch other available private contracts' states as well
+		if let Some(key_server_account) = self.keys_provider.key_server_account() {
+			if let Some(available_contracts) = self.keys_provider.available_keys(block, &key_server_account) {
+				for private_contract in available_contracts {


This may be a performance bottleneck. I don't think we need to fix it immediately, but what we can do is:

Add a hook in Externalities::call that checks the contract address.

From there, only patch an account in the state if it's actually called.

Yes, you're right! TODO added into the code and the corresponding task #10133 created

5chdn · 2019-01-28T10:07:28Z

Needs a 2nd review, could you have a look at this @dvdplm @tomusdrw ?

tomusdrw

Looks good, couple minor grumbles.

tomusdrw · 2019-01-30T12:13:21Z

ethcore/private-tx/src/key_server_keys.rs

+				contract_address);
+
+			let keys_acl_contract = self.keys_acl_contract.write();
+			keys_acl_contract.and(contract_address);


This seems like a no-op, do we actually want to update the keys_acl_contract? Can we test this function?

You're right. Sorry about that. Fixed, test added

tomusdrw · 2019-01-30T12:14:01Z

ethcore/private-tx/src/key_server_keys.rs

+impl StoringKeyProvider {
+	/// Store available keys
+	pub fn set_available_keys(&self, keys: &Vec<Address>) {
+		let mut current = self.available_keys.write();


why not just *self.available_keys.write() = Some(keys.clone())?

tomusdrw · 2019-01-30T12:14:34Z

ethcore/private-tx/src/key_server_keys.rs

+				if let Ok(value) = self.client.call_contract(block, acl_contract_address, data) {
+					self.keys_to_addresses(decoder.decode(&value).ok())
+				} else {
+					None


Should we log something here?

No, it's not an exceptional situation

tomusdrw · 2019-01-30T12:14:57Z

ethcore/private-tx/src/key_server_keys.rs

+
+	fn keys_to_addresses(&self, keys: Option<Vec<H256>>) -> Option<Vec<Address>> {
+		keys.map(|key_values| {
+			let mut addresses: Vec<Address> = Vec::new();


key_values.iter().map(key_to_address).collect()?

tomusdrw · 2019-01-30T12:17:11Z

ethcore/private-tx/src/lib.rs

@@ -511,25 +524,35 @@ impl Provider where {
 			let (new_address, _) = ethcore_contract_address(engine.create_address_scheme(env_info.number), &sender, &nonce, &transaction.data);
 			Some(new_address)
 		});
+		let contract_address = contract_address.expect("Private contract address should be non zero by this moment; qed");


should be doesn't sound like a proof, where exactly is it supposed to be initialised? Can we reach this line if private transactions are not part of the registry?

Re-phrased with more explicit proof.

tomusdrw · 2019-01-30T12:18:27Z

ethcore/private-tx/src/lib.rs

 		};
 		Ok(PrivateExecutionResult {
 			code: encrypted_code,
 			state: encrypted_storage,
-			contract_address,
+			contract_address: Some(contract_address),


Maybe it's worth to change the type if it's always Some?

This reverts commit 476c132.

tomusdrw

Looks good, minor grumble regarding redudant .expect

tomusdrw · 2019-02-04T15:43:35Z

ethcore/private-tx/src/lib.rs

@@ -512,25 +525,36 @@ impl Provider where {
 			let (new_address, _) = ethcore_contract_address(engine.create_address_scheme(env_info.number), &sender, &nonce, &transaction.data);
 			Some(new_address)
 		});
+		let contract_address = contract_address.expect("Private contract address is non zero by this point, \


Maybe change the above line to unwrap_or_else(|| ?
The proof won't be necessary and also the contract address will be evaluated lazily.

* master: fix: parity-clib/examples/cpp/CMakeLists.txt (#10313) CI optimizations (#10297) Increase number of requested block bodies in chain sync (#10247) Deprecate account management (#10213) Properly handle check_epoch_end_signal errors (#10015) fix(osx and windows builds): bump parity-daemonize (#10291) Add missing step for Using `systemd` service file (#10175) Call private contract methods from another private contract (read-only) (#10086) update ring to 0.14 (#10262) fix(secret-store): deprecation warning (#10301) Update to jsonrpc-derive 10.0.2, fixes aliases bug (#10300) Convert to jsonrpc-derive, use jsonrpc-* from crates.io (#10298)

grbIzl added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Dec 19, 2018

5chdn added this to the 2.3 milestone Jan 2, 2019

sorpaas approved these changes Jan 3, 2019

View reviewed changes

grbIzl mentioned this pull request Jan 4, 2019

[Private transactions] Optimize patching of related private states #10133

Closed

5chdn modified the milestones: 2.3, 2.4 Jan 10, 2019

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jan 28, 2019

tomusdrw reviewed Jan 30, 2019

View reviewed changes

grbIzl added 10 commits February 4, 2019 15:28

Patch available private contracts during private transaction

53b1215

Key acl ABI added

b35b433

Work with secret store keys moved to the separate struct

2f2f2b2

Private tx test refactored

e4b490a

Revert "Private tx test refactored"

4a2e07f

This reverts commit 476c132.

Test for calling private contract from another one added

7f1be68

Test fixed

64a4a62

Redundant tab removed

b9ff1e6

ACL contract processing fixed, test added

b5bf711

Merge with head

74931d9

grbIzl force-pushed the PrivateCallPrivate branch from 0d49ce6 to 74931d9 Compare February 4, 2019 15:18

tomusdrw approved these changes Feb 4, 2019

View reviewed changes

Expect replaced with closure

a3d63b5

5chdn merged commit 45d7c60 into master Feb 7, 2019

5chdn deleted the PrivateCallPrivate branch February 7, 2019 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call private contract methods from another private contract (read-only) #10086

Call private contract methods from another private contract (read-only) #10086

grbIzl commented Dec 19, 2018

sorpaas Jan 3, 2019

sorpaas Jan 3, 2019

grbIzl Jan 4, 2019

5chdn commented Jan 28, 2019

tomusdrw left a comment

tomusdrw Jan 30, 2019

grbIzl Feb 4, 2019

tomusdrw Jan 30, 2019

tomusdrw Jan 30, 2019

grbIzl Feb 4, 2019

tomusdrw Jan 30, 2019

tomusdrw Jan 30, 2019

grbIzl Feb 4, 2019

tomusdrw Jan 30, 2019

grbIzl Feb 4, 2019

tomusdrw left a comment

tomusdrw Feb 4, 2019

Call private contract methods from another private contract (read-only) #10086

Call private contract methods from another private contract (read-only) #10086

Conversation

grbIzl commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5chdn commented Jan 28, 2019

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment