Extract the top pool operation execution into separate modules #500

murerfel · 2021-11-10T09:53:47Z

Created new traits/components that execute the trusted operations from the top pool.
Also separated the execution and block/confirmation composition.

The is in preparation for moving these parts into the sidechain crate.

Merge Extract a new extrinsics factory and nonce cache crate #498 first, then rebase this onto master

murerfel · 2021-11-10T09:55:37Z

enclave-runtime/src/sidechain_block_composer.rs

+pub trait ComposeBlockAndConfirmation {
+	type SidechainBlockT: SignedBlockT;
+	type ParentchainBlockT: BlockT;
+
+	fn compose_block_and_confirmation(
+		&self,
+		latest_onchain_header: &<Self::ParentchainBlockT as BlockT>::Header,
+		top_call_hashes: Vec<H256>,
+		shard: ShardIdentifier,
+		state_hash_apriori: H256,
+	) -> Result<(OpaqueCall, Self::SidechainBlockT)>;
+}
+
+/// Block composer implementation for the sidechain
+pub struct BlockComposer<PB, SB, Signer, StateKey, RpcAuthor, StfExecutor> {
+	signer: Signer,
+	state_key: StateKey,
+	rpc_author: Arc<RpcAuthor>,
+	stf_executor: Arc<StfExecutor>,
+	_phantom: PhantomData<(PB, SB)>,
+}


This is the new (sidechain) block composer, extracted from the former compose_block_and_confirmation function

murerfel · 2021-11-10T09:56:22Z

enclave-runtime/src/sidechain_block_composer.rs

+		let opaque_call =
+			OpaqueCall::from_tuple(&(xt_block, shard, block_hash, state_hash_new.encode()));
+
+		self.rpc_author.on_block_created(block.signed_top_hashes(), block.hash());


The composer is now responsible for doing the callback to the rpc author

murerfel · 2021-11-10T09:57:32Z

enclave-runtime/src/lib.rs

+mod sidechain_block_composer;
 mod sidechain_impl;
 mod sync;
 pub mod tls_ra;
+pub mod top_pool_execution;
+mod top_pool_operation_executor;


Lots of code was removed from this lib.rs, refactored and moved to these new modules (temporary before we further refactor and move them into the sidechain crate)

murerfel · 2021-11-10T09:59:18Z

enclave-runtime/src/sidechain_impl.rs

-		let (calls, blocks) = execute_top_pool_trusted_calls::<PB, SB, _, _, Signer>(
-			self.author.as_ref(),
-			self.stf_executor.as_ref(),
-			self.signer.clone(),
-			&self.parentchain_header,
-			self.shard,
-			max_duration,
-		)
-		.map_err(|e| ConsensusError::Other(e.to_string().into()))?;
-
-		Ok(Proposal {
-			block: blocks.ok_or(ConsensusError::CannotPropose)?,
-			parentchain_effects: calls,
-		})
+		let latest_onchain_header = &self.parentchain_header;
+
+		let batch_execution_result = self
+			.top_pool_executor
+			.execute_trusted_calls(latest_onchain_header, self.shard, max_duration)
+			.map_err(|e| ConsensusError::Other(e.to_string().into()))?;
+
+		let mut parentchain_extrinsics = batch_execution_result.get_extrinsic_callbacks();
+
+		let executed_operation_hashes =
+			batch_execution_result.get_executed_operation_hashes().iter().copied().collect();
+
+		let (confirmation_extrinsic, sidechain_block) = self
+			.block_composer
+			.compose_block_and_confirmation(
+				latest_onchain_header,
+				executed_operation_hashes,
+				self.shard,
+				batch_execution_result.previous_state_hash,
+			)
+			.map_err(|e| ConsensusError::Other(e.to_string().into()))?;


The workflow here has now changed a bit. We have to first execute the trusted calls and then call the block composer to compose the block and corresponding confirmation extrinsic.

This is great, I had this separation of concerns also in mind. 😄

murerfel · 2021-11-10T10:01:03Z

enclave-runtime/src/sidechain_impl.rs

-pub struct BlockImporter<A, PB, SB, O, ST, StateHandler> {
+pub struct BlockImporter<A, PB, SB, O, ST, StateHandler, StateKey> {
 	state_handler: Arc<StateHandler>,
+	state_key: StateKey,
 	_phantom: PhantomData<(A, PB, SB, ST, O)>,
 }

-impl<A, PB, SB, O, ST, StateHandler> BlockImporter<A, PB, SB, O, ST, StateHandler> {
+impl<A, PB, SB, O, ST, StateHandler, StateKey>
+	BlockImporter<A, PB, SB, O, ST, StateHandler, StateKey>
+{
 	#[allow(unused)]
-	pub fn new(state_handler: Arc<StateHandler>) -> Self {
-		Self { state_handler, _phantom: Default::default() }
+	pub fn new(state_handler: Arc<StateHandler>, state_key: StateKey) -> Self {
+		Self { state_handler, state_key, _phantom: Default::default() }


This was more of a small improvement on the side: The block importer should not read the state encryption key directly from file, but rather have it passed as member when it's constructed. This makes the dependency more obvious and lets us test this importer more easily, without having to rely on a file existing on the filesystem.

murerfel · 2021-11-10T10:02:50Z

enclave-runtime/src/tests.rs

-	submit_and_execute_top(&rpc_author, &signed_getter.clone().into(), &shielding_key, shard)
-		.unwrap();
-	submit_and_execute_top(&rpc_author, &direct_top(signed_call.clone()), &shielding_key, shard)
-		.unwrap();
+	submit_operation_to_top_pool(


minor re-naming, I realized submit_and_execute_top in fact only submits to the top pool, does not execute.

Ah yes, this changed quite a while ago, thanks! 👍

murerfel · 2021-11-10T10:03:25Z

enclave-runtime/src/tests.rs

-	let stf_executor = StfExecutor::new(Arc::new(OcallApi), state_handler.clone());
+	let stf_executor = Arc::new(StfExecutor::new(Arc::new(OcallApi), state_handler.clone()));
+	let top_pool_executor = TopPoolOperationExecutor::<Block, SignedBlock, _, _>::new(
+		rpc_author.clone(),
+		stf_executor.clone(),
+	);
+	let block_composer = BlockComposer::<Block, SignedBlock, _, _, _, _>::new(
+		test_account(),
+		state_key(),
+		rpc_author.clone(),
+		stf_executor,
+	);


in some of the tests here we now need the top pool executor and the block composer explicitly

murerfel · 2021-11-10T10:04:32Z

enclave-runtime/src/tests.rs

+fn state_key() -> Aes {
+	Aes::default()
+}


Use a default AES key for state encryption instead of reading it from file (and thus requiring that file to exists when we run the tests)

murerfel · 2021-11-10T10:05:47Z

enclave-runtime/src/top_pool_execution.rs

+
+	sgx_status_t::SGX_SUCCESS
+}
+


In this file we have the top-level e-calls for executing trusted getters and trusted calls from the top pool. These will be further refactored and moved to the sidechain crate in the next PR

I know it will be further refactored - but what do you think about adding top-level-file description, i.e. documenting the thought process of about what should be placed in this file?

What I'm dreaming about:
https://github.com/paritytech/substrate/blob/master/frame/scheduler/src/lib.rs#L18-L48

What might be reality:
https://github.com/paritytech/substrate/blob/master/primitives/core/src/hash.rs#L18

But we could try?

murerfel · 2021-11-10T10:08:32Z

enclave-runtime/src/top_pool_execution.rs

+	let rpc_author = GlobalAuthorContainer.get().ok_or_else(|| {
+		error!("Failed to retrieve author mutex. It might not be initialized?");
+		Error::MutexAccess
+	})?;
+
+	let state_handler = Arc::new(GlobalFileStateHandler);
+	let stf_executor = Arc::new(StfExecutor::new(Arc::new(OcallApi), state_handler.clone()));
+
+	let shards = state_handler.list_shards()?;
+	let mut remaining_shards = shards.len() as u32;
+	let ends_at = duration_now() + MAX_TRUSTED_GETTERS_EXEC_DURATION;
+
+	let top_pool_executor = TopPoolOperationExecutor::<Block, SignedSidechainBlock, _, _>::new(
+		rpc_author,
+		stf_executor,
+	);


This is basically constructing all the necessary components to run this function. It's what a dependency injection framework would do for us. I'm thinking about having a 'sidechain container' (container being a dependency injection concept, where a container contains all registered and constructed components) that is initialized and constructed once and can be accessed at each call.

One day, I'd really love to see the architecture you have in mind.. 😵‍💫

murerfel · 2021-11-10T10:09:49Z

enclave-runtime/src/top_pool_execution.rs

+	let mut validator = LightClientSeal::<PB>::unseal()?;
+
+	let authority = Ed25519Seal::unseal()?;
+	let state_key = AesSeal::unseal()?;
+
+	let rpc_author = GlobalAuthorContainer.get().ok_or_else(|| {
+		error!("Failed to retrieve author mutex. Maybe it's not initialized?");
+		Error::MutexAccess
+	})?;
+
+	let state_handler = Arc::new(GlobalFileStateHandler);
+	let stf_executor = Arc::new(StfExecutor::new(Arc::new(OcallApi), state_handler.clone()));
+
+	let latest_onchain_header = validator.latest_finalized_header(validator.num_relays()).unwrap();
+	let genesis_hash = validator.genesis_hash(validator.num_relays())?;
+	let extrinsics_factory =
+		ExtrinsicsFactory::new(genesis_hash, authority.clone(), GLOBAL_NONCE_CACHE.clone());
+
+	let top_pool_executor =
+		Arc::new(TopPoolOperationExecutor::<PB, SignedSidechainBlock, _, _>::new(
+			rpc_author.clone(),
+			stf_executor.clone(),
+		));
+
+	let block_composer =
+		Arc::new(BlockComposer::new(authority.clone(), state_key, rpc_author, stf_executor));


All of this is also just constructing the necessary components. Will try to refactor this in the next PR (as described above)

haerdib

It looks good, but I think I need to take another look at it tomorrow.. too late now to wrap my head around everything in here.

haerdib · 2021-11-10T15:08:52Z

enclave-runtime/src/lib.rs

 	sgx_status_t::SGX_SUCCESS
 }

-#[no_mangle]


So much red. I love it !

haerdib · 2021-11-10T16:55:16Z

enclave-runtime/src/top_pool_execution.rs

+	let rpc_author = GlobalAuthorContainer.get().ok_or_else(|| {
+		error!("Failed to retrieve author mutex. It might not be initialized?");
+		Error::MutexAccess
+	})?;
+
+	let state_handler = Arc::new(GlobalFileStateHandler);
+	let stf_executor = Arc::new(StfExecutor::new(Arc::new(OcallApi), state_handler.clone()));
+
+	let shards = state_handler.list_shards()?;
+	let mut remaining_shards = shards.len() as u32;
+	let ends_at = duration_now() + MAX_TRUSTED_GETTERS_EXEC_DURATION;
+
+	let top_pool_executor = TopPoolOperationExecutor::<Block, SignedSidechainBlock, _, _>::new(
+		rpc_author,
+		stf_executor,
+	);


One day, I'd really love to see the architecture you have in mind.. 😵‍💫

haerdib · 2021-11-10T17:00:48Z

enclave-runtime/src/top_pool_execution.rs

+
+	sgx_status_t::SGX_SUCCESS
+}
+


I know it will be further refactored - but what do you think about adding top-level-file description, i.e. documenting the thought process of about what should be placed in this file?

What I'm dreaming about:
https://github.com/paritytech/substrate/blob/master/frame/scheduler/src/lib.rs#L18-L48

What might be reality:
https://github.com/paritytech/substrate/blob/master/primitives/core/src/hash.rs#L18

But we could try?

clangenb

Looks very good, nothing really to add!

clangenb · 2021-11-11T01:38:27Z

enclave-runtime/src/sidechain_impl.rs

-		let (calls, blocks) = execute_top_pool_trusted_calls::<PB, SB, _, _, Signer>(
-			self.author.as_ref(),
-			self.stf_executor.as_ref(),
-			self.signer.clone(),
-			&self.parentchain_header,
-			self.shard,
-			max_duration,
-		)
-		.map_err(|e| ConsensusError::Other(e.to_string().into()))?;
-
-		Ok(Proposal {
-			block: blocks.ok_or(ConsensusError::CannotPropose)?,
-			parentchain_effects: calls,
-		})
+		let latest_onchain_header = &self.parentchain_header;
+
+		let batch_execution_result = self
+			.top_pool_executor
+			.execute_trusted_calls(latest_onchain_header, self.shard, max_duration)
+			.map_err(|e| ConsensusError::Other(e.to_string().into()))?;
+
+		let mut parentchain_extrinsics = batch_execution_result.get_extrinsic_callbacks();
+
+		let executed_operation_hashes =
+			batch_execution_result.get_executed_operation_hashes().iter().copied().collect();
+
+		let (confirmation_extrinsic, sidechain_block) = self
+			.block_composer
+			.compose_block_and_confirmation(
+				latest_onchain_header,
+				executed_operation_hashes,
+				self.shard,
+				batch_execution_result.previous_state_hash,
+			)
+			.map_err(|e| ConsensusError::Other(e.to_string().into()))?;


This is great, I had this separation of concerns also in mind. 😄

clangenb · 2021-11-11T01:42:07Z

enclave-runtime/src/tests.rs

-	submit_and_execute_top(&rpc_author, &signed_getter.clone().into(), &shielding_key, shard)
-		.unwrap();
-	submit_and_execute_top(&rpc_author, &direct_top(signed_call.clone()), &shielding_key, shard)
-		.unwrap();
+	submit_operation_to_top_pool(


Ah yes, this changed quite a while ago, thanks! 👍

Also separated the execution and block/confirmation composition. The is in preparation for moving these parts into the sidechain crate.

murerfel commented Nov 10, 2021

View reviewed changes

murerfel self-assigned this Nov 10, 2021

murerfel requested review from clangenb and haerdib November 10, 2021 10:11

murerfel commented Nov 10, 2021

View reviewed changes

murerfel force-pushed the feature/fm-extract-top-pool-execution branch from 67622c2 to 4a267f8 Compare November 10, 2021 10:42

Base automatically changed from feature/fm-extrinsics-factory-nonce-cache to master November 10, 2021 13:49

murerfel force-pushed the feature/fm-extract-top-pool-execution branch from 4a267f8 to e50f4d0 Compare November 10, 2021 13:59

haerdib mentioned this pull request Nov 10, 2021

Separate block confirmations #501

Merged

haerdib reviewed Nov 10, 2021

View reviewed changes

clangenb approved these changes Nov 11, 2021

View reviewed changes

Extract the top pool execution logic from enclave-runtime/lib.rs

de5852b

Also separated the execution and block/confirmation composition. The is in preparation for moving these parts into the sidechain crate.

murerfel force-pushed the feature/fm-extract-top-pool-execution branch from e50f4d0 to de5852b Compare November 11, 2021 10:06

murerfel merged commit bca26ab into master Nov 11, 2021

murerfel deleted the feature/fm-extract-top-pool-execution branch November 11, 2021 11:52

Extract the top pool operation execution into separate modules #500

Extract the top pool operation execution into separate modules #500

Uh oh!

Conversation

murerfel commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haerdib left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

murerfel commented Nov 10, 2021 •

edited

Loading