Separate send xts and send blocks #487

murerfel · 2021-11-03T07:45:38Z

This is another refactoring step towards #301.

The goal is to have the functionality that was originally in the prepare_and_send_xts_and_block function in a separate crate, so that the content of sidechain_impl.rs can be moved from the enclave_runtime crate, to the sidechain crate, where it belongs.

As I studied the code, I realized that prepare_and_send_xts_and_block does 2 things that are not necessarily related:

Crate and send extrinsics confirmations to the parentchain
Send, i.e. distribute/gossip sidechain blocks

So I separated those 2 functionalities, which resulted in a lot of changes to the O-call API (and all its layers). In addition, I did some refactoring regarding side-effects of those functions. The nonce is now not a &mut in/out parameter, but just an in-parameter and the mutated nonce value is return in the result. The second side-effect was the signer key pair, that we retrieved in a static method. I now pass this key down as a parameter (which is abstracted by a type parameter).

Merge Extract the STF execution logic to a separate crate 'stf-executor' #485 first, then rebase this onto master

core-primitives/ocall-api/src/lib.rs

murerfel · 2021-11-03T07:59:39Z

enclave-runtime/src/lib.rs

-fn create_extrinsics<PB>(
+fn create_extrinsics<PB, Signer>(
 	genesis_hash: HashFor<PB>,
+	signer: Signer,
 	calls: Vec<OpaqueCall>,
-	nonce: &mut u32,
-) -> Result<Vec<OpaqueExtrinsic>>
+	mut nonce: u32,
+) -> Result<(Vec<OpaqueExtrinsic>, u32)>


Take the signer as parameter (rather than having the Ed25519Seal::unseal()?; side-effect in the function) and the nonce is now an input parameter and returned in the result.

enclave-runtime/src/lib.rs

murerfel · 2021-11-03T08:12:39Z

enclave-runtime/src/sidechain_impl.rs

 			.unzip();

-	prepare_and_send_xts_and_block::<_, SB, _, _>(
+	ocall_api.send_sidechain_blocks(blocks)?;


Sending the sidechain blocks is now done by just calling the corresponding method on ocall_api

murerfel · 2021-11-03T08:14:54Z

service/src/ocall_bridge/worker_on_chain_ocall.rs

+		if !confirmation_calls.is_empty() {
+			debug!("Enclave wants to send {} extrinsics", confirmation_calls.len());
+			for call in confirmation_calls.into_iter() {
+				api.send_extrinsic(hex_encode(call.encode()), XtStatus::Ready).unwrap();


I just saw this in the review here: We have some unwraps still in this part of the code. We should probably fix this at some point?

Yes, totally agreed! Does anything speak against fixing it in this PR? 😇

Time is the reason 😝 For now we have to live with it I think, otherwise I will never be able to get through this refactoring in this sprint 😄 We have many places where we still use unwrap() instead of proper error handling. I think we should fix all of those in one go.

haerdib

Looks good all in all I think. I left some comments ;)

core-primitives/ocall-api/src/lib.rs

enclave-runtime/src/lib.rs

haerdib · 2021-11-08T15:21:07Z

service/src/ocall_bridge/bridge_api.rs

-		confirmations: Vec<u8>,
-		signed_blocks: Vec<u8>,
-	) -> OCallBridgeResult<()>;
+	fn send_sidechain_blocks(&self, signed_blocks: Vec<u8>) -> OCallBridgeResult<()>;


Renaming suggestion (@clangenb may correct me here): propose_sidechain_blocks

Yes, I agree, and I think we should actually move the function from another place. The current location was confusing for me. This function gossips the sidechain blocks to peer-validateers now, it has nothing to do with WorkerOnchainBridge.

However, I see now that cleanly separating the sidechain stuff means that we kind of need to replicate some structure from the core-primitives domain into the sidechain domain. This might also be an overkill, so we might just make an extra trait here called SidechainBridge or so?

Finally, @haerdib I think you need to become consistent with your sidechain-block naming. In the teerex pallet, you introduced the calls like propose_sidechainblock. ;)

I will do the re-naming and create a new bridge, called SideChainBridge, to where I'll move that function.

Oh, you're right @clangenb . Sorry for that - sidechainblocks it is ;)

TBH I don't really like propose_sidechainblocks, it's just not how you would write it in English. Sidechain can be a composed noun, but sidechainblock is more like you would form the word in German than in English (where I'd prefer sidechain block)

I can agree with both, as long as we stay consistent, I guess.

Well, I guess you could argue that we are in an emerging field, and we henceforth coin the term 'Sidechainblock`, I don't have a strong preference here, but except for the pallet, we have been going with 'sidechain block' until now.

Lets go with _block then. I'll adapt the pallet.

For sidechain and parentchain I can agree to have it as a compound noun. But I believe it's very rare in English to have a compound noun made of three words. Otherwise, using your argument, we could also have: parentchainblockconfirmationheader - which is what you would have in German, but definitely not in English

haerdib · 2021-11-08T15:22:45Z

service/src/ocall_bridge/bridge_api.rs

-	) -> OCallBridgeResult<()>;
+	fn send_sidechain_blocks(&self, signed_blocks: Vec<u8>) -> OCallBridgeResult<()>;
+
+	fn send_confirmations(&self, confirmations: Vec<u8>) -> OCallBridgeResult<()>;


Renaming suggestion (@clangenb may correct me here): send_confirmations_to_layer_one or something like that..?

We decided not to use layer_one anymore but rather relative naming as there might be multiple hierarchies, so I'd suggest:

send_to_parentchain(&self, extrinsics_vec_encoded)

The current naming is actually wrong. The 'confirmations' may also contain callbacks from the trustedcall's execution.

👍 I will re-name the function

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

service/src/ocall_bridge/worker_on_chain_ocall.rs

+		if !confirmation_calls.is_empty() {
+			debug!("Enclave wants to send {} extrinsics", confirmation_calls.len());
+			for call in confirmation_calls.into_iter() {
+				api.send_extrinsic(hex_encode(call.encode()), XtStatus::Ready).unwrap();


Yes, totally agreed! Does anything speak against fixing it in this PR? 😇

clangenb

Minor comments but looks nice; divide and conquer!

core-primitives/ocall-api/src/lib.rs

clangenb · 2021-11-09T03:32:10Z

enclave-runtime/src/lib.rs

-		.send_block_and_confirmation::<SB>(extrinsics, blocks)
-		.map_err(|e| Error::Other(format!("Failed to send block and confirmation: {}", e).into()))
+	ocall_api.send_confirmations(extrinsics).map_err(|e| {
+		Error::Other(format!("Failed to send block and confirmation: {}", e).into())


Now, it only sends confirmations, no? :)

enclave-runtime/src/lib.rs

clangenb · 2021-11-09T03:42:25Z

enclave-runtime/src/sidechain_impl.rs

-	Authority: Pair<Public = sp_core::ed25519::Public>,
-	Authority::Public: Encode,
 	PB: Block<Hash = H256>,
-	SB: SignedBlock<Public = Authority::Public, Signature = MultiSignature> + 'static,
+	SB: SignedBlock<Public = sp_core::ed25519::Public, Signature = MultiSignature> + 'static,
 	SB::Block: SidechainBlockT<ShardIdentifier = H256, Public = Authority::Public>,
+	Authority: Pair<Public = SB::Public>,
+	Authority::Public: Encode,
+	SB::Signature: From<Authority::Signature>,


Why did you invert the relation here? Now we also we have a definition cycle:

Authority: Pair<Public = SB::Public> SB::Signature: From<Authority::Signature>,

Anyhow, I think the block should not have the MultiSignature in the future but just Authority::Signature, but I could not change that easily when introducing aura ...

To be consistent, everywhere else we have the authority being defined by the sidechain block public type. Only here it was different, so I changed it.

You are probably right regarding the definition cycle. So I might have to invert it again, but then do it in all the places where we have these constraint definitions.

Ok, I see. Good catch in that case! 😄

I think you can leave it until we introduce the change that makes the following possible:

SB: SignedBlock<Public = Authority::Public, Signature = Authority::Signature + 'static,

Well now that you mentioned the cyclic definition path, it's bugging me as well. I'll try to change it (and hope I don't shoot myself in the foot with those trait constraints)

clangenb · 2021-11-09T04:06:47Z

service/src/ocall_bridge/bridge_api.rs

-		confirmations: Vec<u8>,
-		signed_blocks: Vec<u8>,
-	) -> OCallBridgeResult<()>;
+	fn send_sidechain_blocks(&self, signed_blocks: Vec<u8>) -> OCallBridgeResult<()>;


Yes, I agree, and I think we should actually move the function from another place. The current location was confusing for me. This function gossips the sidechain blocks to peer-validateers now, it has nothing to do with WorkerOnchainBridge.

However, I see now that cleanly separating the sidechain stuff means that we kind of need to replicate some structure from the core-primitives domain into the sidechain domain. This might also be an overkill, so we might just make an extra trait here called SidechainBridge or so?

Finally, @haerdib I think you need to become consistent with your sidechain-block naming. In the teerex pallet, you introduced the calls like propose_sidechainblock. ;)

clangenb · 2021-11-09T04:13:24Z

service/src/ocall_bridge/bridge_api.rs

-	) -> OCallBridgeResult<()>;
+	fn send_sidechain_blocks(&self, signed_blocks: Vec<u8>) -> OCallBridgeResult<()>;
+
+	fn send_confirmations(&self, confirmations: Vec<u8>) -> OCallBridgeResult<()>;


We decided not to use layer_one anymore but rather relative naming as there might be multiple hierarchies, so I'd suggest:

send_to_parentchain(&self, extrinsics_vec_encoded)

The current naming is actually wrong. The 'confirmations' may also contain callbacks from the trustedcall's execution.

…ions This specifically affects the o-call, which is also split now.

extract a separate sidechain bridge from the onchain bridge

murerfel · 2021-11-09T08:01:16Z

core-primitives/ocall-api/src/lib.rs

+pub trait EnclaveSideChainOCallApi: Clone + Debug + Send + Sync {
+	fn propose_sidechain_blocks<SB: Encode>(&self, signed_blocks: Vec<SB>) -> SgxResult<()>;
+}


Extraced the propose_sidechain_blocks to a separate trait and ocall bridge everywhere

clangenb

Only wondering about case conventions, otherwise it looks very good!

clangenb · 2021-11-09T08:47:32Z

core-primitives/ocall-api/src/lib.rs

 	) -> SgxResult<Vec<WorkerResponse<V>>>;
 }

+pub trait EnclaveSideChainOCallApi: Clone + Debug + Send + Sync {


For consistency, the trait should contain Sidechain not SideChain (capital C).

clangenb · 2021-11-09T08:48:43Z

enclave-runtime/src/ocall/sidechain_ocall.rs

+use itp_ocall_api::{alloc::prelude::v1::Vec, EnclaveSideChainOCallApi};
+use sgx_types::{sgx_status_t, SgxResult};
+
+impl EnclaveSideChainOCallApi for OcallApi {


Again the capital 'C'

clangenb · 2021-11-09T08:49:01Z

enclave-runtime/src/ocall/sidechain_ocall.rs

@@ -0,0 +1,43 @@
+/*


Woah, even a separate file. 😄

clangenb · 2021-11-09T08:50:06Z

service/src/ocall_bridge/bridge_api.rs

+	#[error("Propose sidechain block failed: {0}")]
+	ProposeSideChainBlock(String),
+	#[error("Sending extrinsics to parentchain failed: {0}")]
+	SendExtrinsicsToParentChain(String),


Also the capital 'C'. :)

clangenb · 2021-11-09T08:52:15Z

service/src/ocall_bridge/sidechain_ocall.rs

+use log::*;
+use std::sync::Arc;
+
+pub struct SideChainOCall<S, D> {


Now I wonder. Did you make it on purpose with the capital 'C's in the CamelCase names? I'd prefer it to be consistent with the snake_case versoin.

murerfel · 2021-11-09T09:00:31Z

hmm either I've gone completely crazy, or GitHub does not let me answer to comments anymore, I just have the 'Resolve Conversation' option 😮
Regarding the naming of SideChain @clangenb : you are absolutely right, it should be Sidechain to be consistent with the snake case sidechain. My bad 😄

clangenb

Wow, I am surprised that we'd have so many SideChains left. Thanks for fixing.

haerdib · 2021-11-09T09:33:08Z

service/src/ocall_bridge/bridge_api.rs

 			OCallBridgeError::GetIasSocket(_) => sgx_status_t::SGX_ERROR_UNEXPECTED,
-			OCallBridgeError::SendBlockAndConfirmation(_) => sgx_status_t::SGX_ERROR_UNEXPECTED,
+			OCallBridgeError::ProposeSidechainBlock(_) => sgx_status_t::SGX_ERROR_UNEXPECTED,
+			OCallBridgeError::SendExtrinsicsToParentChain(_) => sgx_status_t::SGX_ERROR_UNEXPECTED,


Heh, I also found one :-)

Ah parentchain - yes I've only checked for sidechain 😛 another search and replace commit then

haerdib

Only minor comments. Hence, I'm approving.

haerdib · 2021-11-09T09:34:21Z

service/src/ocall_bridge/ffi/propose_sidechain_blocks.rs

+	match sidechain_api.propose_sidechain_blocks(signed_blocks_vec) {
+		Ok(_) => sgx_status_t::SGX_SUCCESS,
+		Err(e) => {
+			error!("send sidechain blocks failed: {:?}", e);


Suggested change

error!("send sidechain blocks failed: {:?}", e);

error!("Proposing sidechain blocks failed: {:?}", e);

haerdib · 2021-11-09T09:36:56Z

service/src/ocall_bridge/ffi/send_to_parentchain.rs


-fn send_block_and_confirmation(
+fn send_to_parentchain(
 	confirmations: *const u8,


since these have been renamed in woker_on_chain_ocall.rs, should we also rename them to extrinsics_encoded here? Or it that taking it too far?

yes, you're right - I missed that one when re-naming

murerfel · 2021-11-09T08:04:56Z

enclave-runtime/src/lib.rs

-	SB: SignedBlockT<Public = sp_core::ed25519::Public, Signature = MultiSignature>,
+	SB: SignedBlockT<Public = Signer::Public, Signature = MultiSignature>,
 	SB::Block: SidechainBlockT<ShardIdentifier = H256, Public = sp_core::ed25519::Public>,
+	SB::Signature: From<Signer::Signature>,
 	RpcAuthor:
 		AuthorApi<H256, PB::Hash> + SendState<Hash = PB::Hash> + OnBlockCreated<Hash = PB::Hash>,
 	StateHandler: QueryShardState,
 	StfExecutor: StfExecuteTimedCallsBatch<Externalities = SgxExternalities>
 		+ StfExecuteGenericUpdate<Externalities = SgxExternalities>,
+	Signer: Pair<Public = sp_core::ed25519::Public>,
+	Signer::Public: Encode,


Changed the trait bounds definition now, such that the SignedBlock::Public type is defined by the Signer::Public type

Also re-name the 'confirmations' to 'extrinsics'

murerfel commented Nov 3, 2021

View reviewed changes

core-primitives/ocall-api/src/lib.rs Outdated Show resolved Hide resolved

murerfel commented Nov 3, 2021

View reviewed changes

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

murerfel commented Nov 3, 2021

View reviewed changes

murerfel self-assigned this Nov 3, 2021

murerfel force-pushed the feature/fm-trusted-call-handler branch from 174fc87 to 12743a4 Compare November 3, 2021 15:33

murerfel force-pushed the feature/fm-separate-send-xts-send-blocks branch 2 times, most recently from a0edcd9 to 7468e12 Compare November 4, 2021 09:24

Base automatically changed from feature/fm-trusted-call-handler to master November 4, 2021 10:30

murerfel force-pushed the feature/fm-separate-send-xts-send-blocks branch from 7468e12 to 0b6cbd9 Compare November 4, 2021 15:09

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

murerfel marked this pull request as ready for review November 8, 2021 11:41

murerfel force-pushed the feature/fm-separate-send-xts-send-blocks branch from 0b6cbd9 to 99980a2 Compare November 8, 2021 12:31

haerdib reviewed Nov 8, 2021

View reviewed changes

clangenb reviewed Nov 9, 2021

View reviewed changes

haerdib mentioned this pull request Nov 9, 2021

rename to chainblock to chain_block integritee-network/pallets#14

Merged

Felix Müller added 3 commits November 9, 2021 08:59

Separate the 'prepare_and_send_xts_and_block' into two separate funct…

446972d

…ions This specifically affects the o-call, which is also split now.

re-name ocalls for parentchain extrinsics and sidechain blocks

2566e61

extract a separate sidechain bridge from the onchain bridge

invert the trait bound type definitions with signer and sidechain block

98451ba

murerfel force-pushed the feature/fm-separate-send-xts-send-blocks branch from 472a0e2 to 98451ba Compare November 9, 2021 08:00

murerfel commented Nov 9, 2021

View reviewed changes

clangenb reviewed Nov 9, 2021

View reviewed changes

use 'Sidechain' and 'sidechain' consistently, as a compound word

234bfd6

clangenb approved these changes Nov 9, 2021

View reviewed changes

murerfel mentioned this pull request Nov 9, 2021

Extract a new extrinsics factory and nonce cache crate #498

Merged

1 task

haerdib reviewed Nov 9, 2021

View reviewed changes

haerdib approved these changes Nov 9, 2021

View reviewed changes

murerfel commented Nov 9, 2021

View reviewed changes

Use 'Parentchain' instead of 'ParentChain'

a4d5614

Also re-name the 'confirmations' to 'extrinsics'

murerfel merged commit e58c934 into master Nov 9, 2021

murerfel deleted the feature/fm-separate-send-xts-send-blocks branch November 9, 2021 11:07

	error!("send sidechain blocks failed: {:?}", e);
	error!("Proposing sidechain blocks failed: {:?}", e);

Separate send xts and send blocks #487

Separate send xts and send blocks #487

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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

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!

clangenb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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

murerfel commented Nov 3, 2021 •

edited

Loading

murerfel Nov 3, 2021 •

edited

Loading

murerfel Nov 9, 2021 •

edited

Loading

murerfel commented Nov 9, 2021 •

edited

Loading