Add async contract invocation interface #128
Conversation
kostko
added
c:runtime/compute
kostko
force-pushed
the
kostko/feature/contract-async
branch
from
ca42766
to
f4d0242
Compare
kostko
force-pushed
the
kostko/feature/contract-async
branch
7 times, most recently
from
544e10c
to
187f4d0
Compare
kostko
force-pushed
the
kostko/feature/contract-async
branch
8 times, most recently
from
d3c9dad
to
59429a1
Compare
kostko
changed the title
kostko
force-pushed
the
kostko/feature/contract-async
branch
3 times, most recently
from
00332f6
to
d637046
Compare
compute/src/server.rs
Outdated
| } else { | ||
| break; | ||
| // No subscribers, add to missed calls. | ||
| self.missed_calls.insert(call_id, Ok(output)); |
There was a problem hiding this comment.
Can missed_calls grow arbitrarily? should we be keeping track of timing to expire these results / be able to eventually garbage collect?
There was a problem hiding this comment.
No, if you check above you will notice that the hashmap uses a LRU eviction policy with a fixed size of items (currently arbitrarily set at 2 * max_batch_size).
| } | ||
|
|
||
| /// Queue a contract call. | ||
| pub fn call<C, O>(&self, method: &str, arguments: C) -> BoxFuture<O> |
There was a problem hiding this comment.
doc comment examples of using and calling from the client would be great.
There was a problem hiding this comment.
You usually use this from a generated client interface (see macros.rs) and not via this method directly. But sure, I can add some more docs. There are examples for RPC under docs/rpc.md, but I haven't ported those over to docs/contract.md - which is a good point and I'll port those over.
There was a problem hiding this comment.
I've added documentation into the contract docs.
kostko
force-pushed
the
kostko/feature/contract-async
branch
3 times, most recently
from
d5171b2
to
a3f968d
Compare
kostko
force-pushed
the
kostko/feature/contract-async
branch
3 times, most recently
from
8bf7eb0
to
eae5fdf
Compare
kostko
force-pushed
the
kostko/feature/contract-async
branch
3 times, most recently
from
5ff6a90
to
a4cf1ae
Compare
| @@ -18,6 +18,7 @@ jobs: | |||
| --exclude ekiden-enclave-untrusted \ | |||
| --exclude ekiden-rpc-untrusted \ | |||
| --exclude ekiden-db-untrusted \ | |||
| --exclude ekiden-contract-untrusted \ | |||
There was a problem hiding this comment.
The same reason as the above untrusted crates, it doesn't compile and there is not much we can test there so I didn't bother to look into it yet.
| @@ -8,6 +8,7 @@ extern crate rand; | |||
|
|
|||
| #[macro_use] | |||
| extern crate client_utils; | |||
| extern crate ekiden_contract_client; | |||
| extern crate ekiden_core; | |||
| extern crate ekiden_rpc_client; | |||
There was a problem hiding this comment.
what remains to be done directly with the rpc client?
There was a problem hiding this comment.
As explained in the updated documentation and in #120, the RPC interface is used for interactive stateless calls to a single contract instance while the contract interface is used for stateful async calls which may be delayed longer and may be executed by multiple nodes.
And the contract client uses the RPC client underneath to initiate the contract calls.
| $contract::Client::new( | ||
| $backend, | ||
| value_t!($args, "mr-enclave", ekiden_core::enclave::quote::MrEnclave).unwrap_or_else(|e| e.exit()) |
There was a problem hiding this comment.
how did we used to get away without a , at the end here?
There was a problem hiding this comment.
The issue is that rustfmt does not seem to touch macros (only the line length rule is checked).
common/Cargo.toml
Outdated
| @@ -14,7 +14,8 @@ profiling = [] | |||
| protobuf = "1.4.3" | |||
| byteorder = "1" | |||
| fixed-hash = { version = "0.2.1", default-features = false } | |||
| etcommon-rlp = "0.2.3" | |||
| # XXX: Remove when merged upstream: https://github.com/ETCDEVTeam/etcommon-rs/pull/63 | |||
There was a problem hiding this comment.
remove completely or revert to etcommon-rlp = "0.xxx"?
common/Cargo.toml
Outdated
| @@ -14,7 +14,8 @@ profiling = [] | |||
| protobuf = "1.4.3" | |||
| byteorder = "1" | |||
| fixed-hash = { version = "0.2.1", default-features = false } | |||
| etcommon-rlp = "0.2.3" | |||
| # XXX: Remove when merged upstream: https://github.com/ETCDEVTeam/etcommon-rs/pull/63 | |||
contract/trusted/src/dispatcher.rs
Outdated
| struct ContractMethodHandlerDispatchImpl<Call, Output> { | ||
| #[allow(unused)] | ||
| descriptor: ContractMethodDescriptor, | ||
| handler: Box<ContractMethodHandler<Call, Output> + Sync + Send>, |
There was a problem hiding this comment.
if the only other field is unused, could we just use the Box directly instead?
There was a problem hiding this comment.
The reason for the descriptor is if we need to add some method attributes in the future. We will likely have those and I don't want to remove it now just to add it back later.
| /// for their invocation. | ||
| pub struct Dispatcher { | ||
| /// Registered contract methods. | ||
| methods: HashMap<String, ContractMethod>, |
There was a problem hiding this comment.
this causes the key to be duplicated in the value. do we need that? or would Box<ContractMethodHandlerDispatch> be sufficient?
There was a problem hiding this comment.
Good point, I think it can be removed.
| /// Dispatches a raw contract invocation request. | ||
| pub fn dispatch(&self, call: &Vec<u8>) -> Vec<u8> { | ||
| // Decode request method. | ||
| let method = match serde_cbor::from_slice::<SignedContractCall<Generic>>(call) { |
There was a problem hiding this comment.
does this result in parsing the arguments an additional time before the dispatch? or does serde_cbor::Value do something magical to save the extra work?
actually how robust is it to parse with SignedContractCall<Generic>? will it work if the actual arguments type has multiple fields? this has to do with how CBOR and the derived procedures deal with multi-field objects. are they stored as the fields packed next to each other with no grouping information? or is there an envelope around them?
There was a problem hiding this comment.
does this result in parsing the arguments an additional time before the dispatch?
Yes it does parse it twice.
actually how robust is it to parse with SignedContractCall? will it work if the actual arguments type has multiple fields?
Yes, it will work with arbitrary payloads. I've updated the test case (in call.rs) which previously only tested a simple argument to instead test a more complex structure.
contract/trusted/src/dispatcher.rs
Outdated
| use super::*; | ||
|
|
||
| /// Register an empty method. | ||
| fn register_empty_method() { |
There was a problem hiding this comment.
it looks like this was later changed to be a "dummy" method rather than an empty method
contract/trusted/src/dispatcher.rs
Outdated
| let result_decoded: ContractOutput<u32> = serde_cbor::from_slice(&result).unwrap(); | ||
|
|
||
| assert_eq!(result_decoded.status, Status::Success); | ||
| assert_eq!(result_decoded.data, Some(42u32)); |
|
ok
…On Sat, Apr 28, 2018, 11:00 AM Jernej Kos ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .circleci/config.yml
<#128 (comment)>:
> @@ -18,6 +18,7 @@ jobs:
--exclude ekiden-enclave-untrusted \
--exclude ekiden-rpc-untrusted \
--exclude ekiden-db-untrusted \
+ --exclude ekiden-contract-untrusted \
The same reason as the above untrusted crates, it doesn't compile and
there is not much we can test there so I didn't bother to look into it yet.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#128 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AkdULjWSkNG-3byNllhzCg94aScphC0Mks5ttK5QgaJpZM4TbVae>
.
|
kostko
force-pushed
the
kostko/feature/contract-async
branch
6 times, most recently
from
da8c2fa
to
4d7fc23
Compare
|
@pro-wh I've addressed most of your review comments, please check. |
| @@ -18,6 +18,7 @@ jobs: | |||
| --exclude ekiden-enclave-untrusted \ | |||
| --exclude ekiden-rpc-untrusted \ | |||
| --exclude ekiden-db-untrusted \ | |||
| --exclude ekiden-contract-untrusted \ | |||
| // Parse call batch. | ||
| let batch: CallBatch = read_enclave_request(call_batch_data, call_batch_length); | ||
|
|
||
| // TODO: Actually decrypt batch. |
There was a problem hiding this comment.
This seems like the core of this code. why is it a todo?
There was a problem hiding this comment.
It is not the core. The core is making contract calls async, adding encryption/decryption is easy once we decide how to do that as this part is not defined in any RFC or the master plan.
And anyway it doesn't make sense to do this until we decide how to produce a "proof of publication" that will be used to reveal the keys. I've defined the RPCs that will be used for that, but we need to define how we will be encrypting stuff before we actually go and implement something.
There was a problem hiding this comment.
Added a note about that to our overall tracking issue (#94 (comment)).
|
|
||
| pub mod batch; | ||
| pub mod dispatcher; | ||
| #[doc(hidden)] |
There was a problem hiding this comment.
why do we hide the docs here?
There was a problem hiding this comment.
Because these are internal modules not meant for public use. The only reason why they need to be public is because they export C symbols.
| )); | ||
|
|
||
| // Reveal output decryption key. | ||
| // TODO. |
There was a problem hiding this comment.
is there another method that should be here?
(OK to defer to a followup PR)
There was a problem hiding this comment.
Yes, the contract_reveal_outputs mentioned in contract.md and protocol.rs. The reason why it is not implemented is the same as I mentioned above - we first need to define what the proof of publication is.
There was a problem hiding this comment.
Added a note about that to our overall tracking issue (#94 (comment)).
| let encrypted_state_ciphertext = encrypted_state.get_ciphertext(); | ||
| if encrypted_state_ciphertext.is_empty() { | ||
| return Ok(vec![]); |
There was a problem hiding this comment.
Is this really an Ok decryption, and not an Err?
There was a problem hiding this comment.
Yes, this basically means "no state".
db/trusted/src/schema/macros.rs
Outdated
| /// [`Serializable`]: ekiden_common::serializer::Serializable | ||
| /// [`Deserializable`]: ekiden_common::serializer::Deserializable | ||
| /// [`Encodable`]: ekiden_common::rlp::Encodable | ||
| /// [`Decodable`]: ekiden_common::rlp::Decodable |
There was a problem hiding this comment.
Yup, missed that, thanks!
| + Send | ||
| + Sync | ||
| + 'static, | ||
| Rs: Send + Sync + 'static, |
There was a problem hiding this comment.
Does Rs really need to be static?
There was a problem hiding this comment.
Yes, note that this is not the lifetime of the value but the lifetime of the type. This basically means that Rs should not store any non-'static references. Also see this Rust RFC.
| @@ -57,9 +58,14 @@ impl ContractClientBackend for OcallContractClientBackend { | |||
| })) | |||
| } | |||
|
|
|||
| /// Wait for given contract call outputs to become available. | |||
| fn wait_contract_call(&self, _call_id: H256) -> ClientFuture<Vec<u8>> { | |||
| unimplemented!(); | |||
There was a problem hiding this comment.
is this a TODO? should we file a tracking issue?
There was a problem hiding this comment.
There is already an issue filed for this, I think it is called cross-contract calls and this needs a lot of design before it can be implemented (see #65).
kostko
force-pushed
the
kostko/feature/contract-async
branch
from
4d7fc23
to
58f10c8
Compare
See #120
See #121
Rendered async contract interface docs.
This PR will introduce the following BREAKING CHANGES:
ContractClient*toRpcClient*(theContractClient*prefix will now be used for the async contract interface).TODO
SerializablewithEncodable(see Consider using RLP for serialization/deserialization #20).