Refactor global components and initialization process #677

murerfel · 2022-03-14T08:53:08Z

Further refactoring for the initialization of components in the enclave-runtime. More re-use of existing/instantiated components instead of creating new instances.
The main idea was to extract the component initialization happens explicitly, in functions that have corresponding names, not implicitly.

Explicit component initialization e-calls were added:

Sidechain component initialization

Base components are initialized with the init() function. This includes all the components necessary for running the trusted and untrusted RPC servers in order to fix #600. In the course of doing that, I encountered another issue, #684, which I also had to fix in this PR.

Linked issues: #545 #600 #684
Closes #600 #684

enclave-runtime/src/lib.rs

murerfel · 2022-03-14T09:14:28Z

enclave-runtime/src/top_pool_execution.rs

-	let authority = Ed25519Seal::unseal()?;
-	let state_key = AesSeal::unseal()?;
-
-	let state_handler = Arc::new(GlobalFileStateHandler);
-	let stf_executor = Arc::new(StfExecutor::new(Arc::new(OcallApi), state_handler.clone()));
-	let extrinsics_factory =
-		ExtrinsicsFactory::new(genesis_hash, authority.clone(), GLOBAL_NONCE_CACHE.clone());
+	let stf_executor = GLOBAL_STF_EXECUTOR_COMPONENT.get().ok_or_else(|| {
+		error!("Failed to retrieve stf executor component. Maybe it's not initialized?");
+		Error::ComponentNotInitialized
+	})?;


Get the STF executor from the global instance instead of creating another instance here.

haerdib

This is certainly an improvement. However, I'm questioning the introduction of two separate E-Calls that need to be called in a specific order. Should this really be two separate E-calls, or rather one E-call that calls internal functions in a specific order?

I'm wondering what's the end goal here:
We now have several init E-calls, ranging from plain init

worker/enclave-runtime/src/lib.rs

Line 129 in 73c8ddb

pub unsafe extern "C" fn init(

to new components inits and individual inits, such as the rpc-direct server

worker/enclave-runtime/src/lib.rs

Line 525 in 73c8ddb

pub unsafe extern "C" fn init_direct_invocation_server(

Do you have a roadmap in mind, or did you need to introduce this change to resolve something? If so, could you link this PR to a corresponding issue?

enclave-runtime/src/lib.rs

murerfel · 2022-03-14T11:12:12Z

We already have multiple init functions, and they have to be called in the correct order, otherwise stuff breaks.

We're experiencing some issues, where the order of starting up the direct invocation server and registering ourselves to the parentchain results in errors because of the initialization ordering. This PR is a step towards solving that (#600).
Of course, it would be nicer to just have one init_components function that initializes all the components. However, we have a couple of dependencies in the ordering. So we have base components that are needed for the parentchain registering to work, which is in turn needed to initialize more components. That resulted in this split of the initialization of components function. And for clarity, I think an init_direct_server should not instantiate components, that is just misleading.

To answer your question: This is just a further refactoring step, but not the end goal. We would still need to think more about our bootstrapping process and when we do what.

clangenb

Nice, looks like yet another cleanup. Only Some minor comments.

service/src/main.rs

enclave-runtime/src/lib.rs

Co-authored-by: haerdib <[email protected]>

also removed the 'init_enclave_base_components', merged it into the existing 'init' function

parentchain block is already imported and thus cannot be peeked anymore

murerfel · 2022-03-23T12:29:51Z

I now moved all the component initialization into a separate module, initialization.rs, out of the lib.rs. Also, I removed the init_enclave_base_components() function again and moved everything into the regular init() function.

In order to fix #600, I had to move the RPC server starting before the parentchain registering. Doing that revealed another issue, #684 - which I also fixed now in this PR.

haerdib

LGTM, Massive clean up, thanks! Only one minor comment

service/src/worker.rs

sidechain/consensus/aura/src/verifier.rs

(import queue is ready and filled earlier)

clangenb

It looks like a very nice clean up! I have only two comments.

clangenb · 2022-03-29T13:51:41Z

enclave-runtime/src/initialization.rs

+	let stf_executor = GLOBAL_STF_EXECUTOR_COMPONENT.get().ok_or_else(|| {
+		error!("Failed to retrieve global STF executor component (maybe it is not initialized?)");
+		Error::ComponentNotInitialized
+	})?;


This seems to be a very repeating pattern. Why not introduce: Error::ComponentNotInitialize(Component)? Then we can simply call:

GLOBAL_STF_EXECUTOR_COMPONENT.get().ok_or_else(|| Error::ComponentNotInitialized(Component)?

Then we can remove the redundant logging here, as the caller of this does print the error anyhow. (Which is not so expressive currently).

Yes, agreed, this can be simplified and the amount of logging reduced 👍 thanks for pointing that out. Now I wish I had a nameof() function in Rust too, so I wouldn't need to type all the variable names as string for the errors 🙈

I agree to that. We could introduce an enum for the components and implement toString for that, but there might be a better way.

I think refactoring the get method of ComponentContainer might be sensible, to return an error that already contains all the information. And a container can also have a name that it puts into the error itself, so the caller does not have to care about the error contents.

The component container now knows its own name, and the getter returns a proper Result with an error that includes its name. That allowed me to reduce a lot of the getter calls to just .get()?;, which is certainly a lot cleaner. Thanks for insisting 😉

Coolio! Thanks for the extra effort. I think this was worth it!

sidechain/consensus/common/src/block_import.rs

… of option the result includes the information about the component name and it not being initialized

clangenb

Looks good to me now! Thanks for the cleanup

* Add a local-setup config to the tutorial (integritee book) (integritee-network#666) * Update call status to `InSidechainBlock` only after block import (integritee-network#676) * rename on_block_created to on_block_imported * update tx status only after block import * fix clippy * fix cargo test * remove waiting time from demos * readd removed comments * udpate comments * extract call removal logic from on_block_imported * Bump substrate to commit f5f286db0da9... (integritee-network#669) * Bump substrate Bump substrate to commit 8df8d908c4d77a8dd19751784b6aca62159ddda8 Remove dependencies of scale-info 1.0.0 and parity-scale-codec 2.3.1 Upgrade substrate to commit f5f286d... substrate fix for : sp-core's full_crypto feature flag breaks no_std builds * frame-metadata from crates.io * Update CI to use updated node Co-authored-by: echevrier <[email protected]> * Dockerize (integritee-network#668) * Dockerize the binaries integritee-network#579 * Add tags to run as well integritee-network#579 * Fix running binary in docker integritee-network#579 * Add more files to docker integritee-network#579 * Add conditional runs * Add +x earlier * Rename docker-service to integritee-demo-validateer. Fixes integritee-network#579 * Lift clap to version 3.16 and move stf cli to cli crate. (integritee-network#679) * Lift clap to version 3.16 and move stf cli to cli crate. * Adapt demo script parameters * Add cli examples to README.md * update comment descriptions Co-authored-by: Gaudenz Kessler <[email protected]> * Refactor global components and initialization process (integritee-network#677) * Consistently (re-)use the component container * RPC server is initialized before registering on the parentchain * Fix issue with sidechain block import when latest parentchain block is already too new Closes integritee-network#545 integritee-network#600 integritee-network#684 integritee-network#683 * create alive service to deterine that the service is up, running and registered (integritee-network#697) * create alive service * replace alive with initialized Co-authored-by: Gaudenz Kessler <[email protected]> * Add port config for the untrusted http server for `is_initialized` (integritee-network#700) * rename wrong `signing_key` function name to `state_key` (integritee-network#704) * Move top pool and top pool author crates to core primitives (integritee-network#705) * Introduce state snapshot history (integritee-network#698) Closes integritee-network#688 * Add header to sidechain block (integritee-network#699) Closes integritee-network#686 Co-authored-by: Gaudenz Kessler <[email protected]> * introduce layer for indirection for sidechainblock (integritee-network#716) Closes integritee-network#710 Co-authored-by: Gaudenz Kessler <[email protected]> Co-authored-by: Felix Müller <[email protected]> * Persist web-socket connections (integritee-network#718) Complete overhaul of the trusted web-socket server: * using MIO to serve concurrent connections * Server keeps connections open until a client requests a close * Changed our clients to match this pattern * Upgrade to polkadot v0.9.19 (integritee-network#720) * Bump substrate to polkadot-v0.9.19 Bump RUNTIME_SPEC_VERSION to 9 Set substrate-api-client to polkadot-v0.9.19 Set integritee-node to polkadot-v0.9.19 Set integritee-node to master * Cargo update + reabse * Update github actions Co-authored-by: echevrier <[email protected]> * fix some cargo.tomls * fix cargo.lock * remove game engine * update teerex module * fix itp-registry-storage * cargo update * resinsert game engine * some further clean up * update teaclave * add ajuna commands * cargo update * carog update * update ajuna cli * fix some thins * Signed sidechain block * fix shard_id getter * fix load_initialized * fix sgx externalities import * fix compilation issues * remove patches and cargo update * update demo docu * add bin folder to docu * remove hard coded ports. Not necesasry * add ./ * update pallet verions * make tests compile again * fix import in sgx mode * merge from upstream number two.. remove itp storage verifier * cargo fmt * fix tests * update doc once again * fix script and queue game * cargo update + some code clean up Co-authored-by: Felix Müller <[email protected]> Co-authored-by: echevrier <[email protected]> Co-authored-by: echevrier <[email protected]> Co-authored-by: mosonyi <[email protected]> Co-authored-by: gaudenzkessler <[email protected]> Co-authored-by: Gaudenz Kessler <[email protected]> Co-authored-by: Felix Müller <[email protected]>

murerfel requested review from haerdib and clangenb March 14, 2022 08:56

murerfel commented Mar 14, 2022

View reviewed changes

murerfel self-assigned this Mar 14, 2022

haerdib requested changes Mar 14, 2022

View reviewed changes

enclave-runtime/src/lib.rs Outdated Show resolved Hide resolved

murerfel force-pushed the feature/fm-init-enclave-components branch from a7ebe9e to c2a8482 Compare March 17, 2022 07:33

clangenb reviewed Mar 20, 2022

View reviewed changes

service/src/main.rs Outdated Show resolved Hide resolved

enclave-runtime/src/lib.rs Outdated Show resolved Hide resolved

Felix Müller and others added 4 commits March 21, 2022 10:07

refactor global components and initialization process

8b97aea

Fix function comment

34fb925

Co-authored-by: haerdib <[email protected]>

fix cargo clippy

6b4cae4

Further refactoring, moved initialization code to separate module

c643ca5

also removed the 'init_enclave_base_components', merged it into the existing 'init' function

murerfel force-pushed the feature/fm-init-enclave-components branch from c3214ab to c643ca5 Compare March 21, 2022 10:36

Felix Müller added 5 commits March 21, 2022 11:48

Fix imports, don't use indirect crate root imports, but fully specified

50e72a4

refactor and remove the 'global worker'

35e1c91

Obsolete sidechain block is properly discarded when its corresponding

4e9218c

parentchain block is already imported and thus cannot be peeked anymore

add unit test for the verifier (#684) and make verifier non-mut

4f007ce

add comment for initialization and fix clippy warnings

716830e

murerfel requested review from haerdib and clangenb March 23, 2022 12:33

haerdib approved these changes Mar 24, 2022

View reviewed changes

service/src/worker.rs Show resolved Hide resolved

sidechain/consensus/aura/src/verifier.rs Outdated Show resolved Hide resolved

Felix Müller added 2 commits March 24, 2022 10:32

Update comment in aura verifier

83e25ef

Remove sleep from initialization sequence

6164662

(import queue is ready and filled earlier)

clangenb reviewed Mar 29, 2022

View reviewed changes

Component container knows its own name and get returns result instead…

8ba43df

… of option the result includes the information about the component name and it not being initialized

clangenb approved these changes Mar 29, 2022

View reviewed changes

murerfel merged commit 37509d3 into master Mar 30, 2022

murerfel deleted the feature/fm-init-enclave-components branch March 30, 2022 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor global components and initialization process #677

Refactor global components and initialization process #677

murerfel commented Mar 14, 2022 •

edited

Loading

murerfel Mar 14, 2022

haerdib left a comment

murerfel commented Mar 14, 2022 •

edited

Loading

clangenb left a comment

murerfel commented Mar 23, 2022

haerdib left a comment

clangenb left a comment

clangenb Mar 29, 2022

murerfel Mar 29, 2022

clangenb Mar 29, 2022

murerfel Mar 29, 2022

murerfel Mar 29, 2022

clangenb Mar 29, 2022

clangenb left a comment

Refactor global components and initialization process #677

Refactor global components and initialization process #677

Conversation

murerfel commented Mar 14, 2022 • edited Loading

murerfel Mar 14, 2022

Choose a reason for hiding this comment

haerdib left a comment

Choose a reason for hiding this comment

murerfel commented Mar 14, 2022 • edited Loading

clangenb left a comment

Choose a reason for hiding this comment

murerfel commented Mar 23, 2022

haerdib left a comment

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

clangenb Mar 29, 2022

Choose a reason for hiding this comment

murerfel Mar 29, 2022

Choose a reason for hiding this comment

clangenb Mar 29, 2022

Choose a reason for hiding this comment

murerfel Mar 29, 2022

Choose a reason for hiding this comment

murerfel Mar 29, 2022

Choose a reason for hiding this comment

clangenb Mar 29, 2022

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

murerfel commented Mar 14, 2022 •

edited

Loading

murerfel commented Mar 14, 2022 •

edited

Loading