Use new consensus backend in compute node #172

kostko · 2018-04-30T11:07:02Z

willscott · 2018-05-01T22:42:07Z

compute/src/main.rs

-            None
+    // Setup consensus backend.
+    // TODO: Change dummy backend to get computation group from another backend.
+    let consensus_backend = Arc::new(ekiden_consensus_dummy::DummyConsensusBackend::new(vec![


Shouldn't this be keyed off of the argument above?

What do you mean by this? Also, you seem to have commented on something that is out of date?

willscott · 2018-05-02T17:42:16Z

compute/src/consensus.rs

+    block: Block,
+}
+
+struct ConsensusFrontendInner {


define this next to consensusfrontend below, without the configuration struct in-between them?

Also: this seems is defining the consensus 'client library'. why is it in compute, and not consensus?

Also: this seems is defining the consensus 'client library'. why is it in compute, and not consensus?

This understanding is not correct. The consensus/* defines the client interface (e.g., the interface that clients use to talk to the backend) and implementations of these clients (e.g. dummy backend). So this frontend is just using the client interface for a compute node specific purpose. It makes absolutely no sense to put that into consensus?

👍 I think the core of this and a couple other one i flagged below is that the terms 'frontend' and 'backend' are context specific. I think this makes sense to me now.

willscott · 2018-05-02T17:58:03Z

compute/src/consensus.rs

+    /// Start consensus frontend.
+    ///
+    /// Returns list of futures that should be spawned in the event loop.
+    pub fn start(&self) -> Vec<BoxFuture<()>> {


Why is this returning a vector of 3 futures? Can you join them into a single future?

Would it be cleaner to have start take a reactor / task pool in which to insert its work?

Why is this returning a vector of 3 futures? Can you join them into a single future?

You can join them, but then they will be scheduled in the same task which would be wrong as it may cause deadlocks.

Would it be cleaner to have start take a reactor / task pool in which to insert its work?

I was thinking about this, but the problem is that the reactor is a generic term and there is no trait that would be implemented for all reactors (e.g., the gRPC reactor that we are using). We could create our own trait and implement it for all these reactors.

Actually with futures 0.2, handling of execution has been much improved. But futures 0.2 is considered unstable and will be replaced by 0.3, so we should revisit this when 0.3 is released and other libraries start supporting it. I'll open an issue and inbetween I'll change it so that an executor is passed as an argument.

willscott · 2018-05-02T17:58:25Z

compute/src/consensus.rs

+                self.inner
+                    .backend
+                    .get_events()
+                    .for_each(move |event| -> BoxFuture<()> {


This method is long enough that it would be worth refactoring the three handlers defined in it into named methods.

Good point, I'll refactor it. Of course they will be functions without the self argument not instance methods as those are currently unusable in futures.

willscott · 2018-05-02T18:04:45Z

compute/src/consensus.rs

+
+struct ConsensusFrontendInner {
+    /// Consensus backend.
+    backend: Arc<ConsensusBackend + Send + Sync>,


this should be named something with consensus if it stays within compute/src/, since otherwise in the methods below i was double checking myself that it was referring to consensus versus compute.

So this is in ConsensusFrontend and it referes to the consensus backend. Is there some other backend that this could refer to? So self.backend should be read as ConsensusFrontend -> backend, e.g. the backend part that he frontend is talking to. To me it sounds strange if there was a consensus_backend in consensus frontend as in this case the consensus part is duplicated.

On other thought, consensus_backend may really be more clear in futures where there's no self, so if you still think this is confusing, I'll change it.

willscott · 2018-05-02T18:27:36Z

compute/src/main.rs

@@ -87,6 +107,12 @@ fn main() {
                .takes_value(true)
                .default_value("9003"),
        )
+        .arg(
+            Arg::with_name("consensus-backend")


Yes, currently not, I'll remove it as we will probably want some way to specify backend-specific configuration when we support multiple backends.

willscott · 2018-05-02T18:35:17Z

compute/src/main.rs

-        })
-    } else {
-        eprintln!("WARNING: IAS is not configured, validation will always return an error.");
+    // Setup key pair.


Wouldn't it be better to have a config file for the binary that hold the key state and also the other identity information / parameters used to resume the same service?

It feels like we'll want the config soon enough, and then we'll have to deal with the interaction of two state files.

What state do you mean, the local contract identity? That is transient and may not even be persisted, especially if we will be supporting compute nodes that run arbitrary contracts.

But I agree regarding the config file and this is something that is out of scope for this PR.

Issue for config file is in #37.

(meant the key_pair, along with configured backend and whatever else we consider as state). Sounds good.

Yes, the first part of the refactor changes arguments into a giant "compute node configuration" structure. The next step would probably be to make it serializable / deserializable to TOML or something.

willscott · 2018-05-02T18:41:43Z

compute/src/worker.rs

+        let (contract, identity_proof) =
+            Self::create_contract(&config.contract_filename, ias, config.saved_identity_path);
+
+        // Construct consensus client.


The worker is given a consensus frontend. why does it also talk to the consensus directly? can we route communication through the frontend to make the communication pattern a bit more constrained?

No, this is the old "consensus" which is actually storage. It will be removed as soon as storage lands (I am currently blocked on the storage interface which is why I did consensus first, otherwise storage was going to be first).

willscott · 2018-05-02T18:43:50Z

compute/src/worker.rs

+            None
+        };
+
+        #[cfg(not(feature = "no_diffs"))]


This is doing something weird. needs a comment.

This is old code, just copied from the previous worker. Will be removed with new storage.

willscott · 2018-05-02T19:25:44Z

compute/src/worker.rs

+    /// Create new contract worker.
+    pub fn new(
+        config: WorkerConfiguration,
+        grpc_environment: Arc<grpcio::Environment>,


This is an awkward interface: we should give it an instantiated service at the same level as consensus_frontend below, rather than having the worker build its own grpc to consensus. Won't there be multiple nodes in the consensus committee eventually that the service should be able to fail over to?

This is old code and will be removed when we have storage, see my comments above.

kostko · 2018-05-03T11:16:32Z

@willscott I've changed how the consensus frontend and worker interact, making the consensus frontend responsible for batching which makes much more sense. The interaction is now as in the following figure:

I also added TODOs around things that should be removed once we switch to use the new storage backend.

kostko force-pushed the kostko/feature/compute-consensus branch 5 times, most recently from 0f558ee to 15fe047 Compare May 1, 2018 14:39

kostko mentioned this pull request May 1, 2018

Allow sharing of contract client between threads #177

Merged

kostko force-pushed the kostko/feature/compute-consensus branch 2 times, most recently from 6bf7b3a to e271416 Compare May 1, 2018 17:23

kostko added the c:runtime/compute Category: runtime compute worker label May 2, 2018

kostko force-pushed the kostko/feature/compute-consensus branch from e271416 to e17159a Compare May 2, 2018 07:42

kostko mentioned this pull request May 2, 2018

Use proper logging #40

Closed

kostko requested a review from willscott May 2, 2018 07:57

kostko changed the title ~~WIP: Use new consensus backend in compute node~~ Use new consensus backend in compute node May 2, 2018

kostko requested a review from pro-wh May 2, 2018 07:57

kostko force-pushed the kostko/feature/compute-consensus branch from e17159a to 3a5be89 Compare May 2, 2018 07:58

This was referenced May 2, 2018

Tracking issue for state, consensus and storage interface #94

Closed

Replace old consensus-based storage with new storage API #178

Closed

kostko force-pushed the kostko/feature/compute-consensus branch from 3a5be89 to 9640c09 Compare May 2, 2018 08:10

kostko mentioned this pull request May 2, 2018

Compute (Commit / Reveal) to Consensus layer #163

Closed

willscott reviewed May 2, 2018

View reviewed changes

kostko force-pushed the kostko/feature/compute-consensus branch from 9640c09 to 9a637f0 Compare May 3, 2018 11:13

Refactor compute node

e1f8118

kostko force-pushed the kostko/feature/compute-consensus branch from 9a637f0 to d90f085 Compare May 3, 2018 11:17

Add compute node consensus frontend

1319c30

kostko force-pushed the kostko/feature/compute-consensus branch from d90f085 to 1319c30 Compare May 3, 2018 16:47

peterjgilbert mentioned this pull request May 3, 2018

CI: Build in a shared target dir oasislabs/oasis-ethwasi-runtime#3

Merged

3 tasks

willscott approved these changes May 4, 2018

View reviewed changes

kostko merged commit 0e02e72 into master May 4, 2018

kostko deleted the kostko/feature/compute-consensus branch May 4, 2018 08:23

kostko mentioned this pull request May 4, 2018

Contract Registry - trait and centralized implementation #182

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new consensus backend in compute node #172

Use new consensus backend in compute node #172

kostko commented Apr 30, 2018 •

edited

Loading

willscott May 1, 2018

kostko May 2, 2018

willscott May 2, 2018

kostko May 2, 2018 •

edited

Loading

willscott May 2, 2018

willscott May 2, 2018

kostko May 2, 2018

kostko May 3, 2018

kostko May 3, 2018

willscott May 2, 2018

kostko May 2, 2018

kostko May 3, 2018

willscott May 2, 2018

kostko May 2, 2018

kostko May 2, 2018 •

edited

Loading

willscott May 2, 2018

kostko May 2, 2018 •

edited

Loading

willscott May 2, 2018

kostko May 2, 2018

kostko May 2, 2018

willscott May 2, 2018

kostko May 2, 2018 •

edited

Loading

willscott May 2, 2018

kostko May 2, 2018

willscott May 2, 2018

kostko May 2, 2018

willscott May 2, 2018

kostko May 2, 2018

kostko commented May 3, 2018 •

edited

Loading

Use new consensus backend in compute node #172

Use new consensus backend in compute node #172

Conversation

kostko commented Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostko May 2, 2018 • edited Loading

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

kostko May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostko May 2, 2018 • edited Loading

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

kostko May 2, 2018 • edited Loading

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

kostko commented May 3, 2018 • edited Loading

kostko commented Apr 30, 2018 •

edited

Loading

kostko May 2, 2018 •

edited

Loading

kostko May 2, 2018 •

edited

Loading

kostko May 2, 2018 •

edited

Loading

kostko May 2, 2018 •

edited

Loading

kostko commented May 3, 2018 •

edited

Loading