Check extrinsic status in indirect_calls_executor (#1193)

* try to resolve #1092

* move `ParentchainUncheckedExtrinsicWithStatus` to itp_types

* revert update

* revert update

* update

* done

* fix unit tests

* add unit tests

* minor update

* fmt

* fix unit test

* add script pre-commit.sh

* update log level

* minor update

* update comment

---------

Co-authored-by: ericzhang <jingleizhang@users.noreply.github.com>
Co-authored-by: Kai <7630809+Kailai-Wang@users.noreply.github.com>
Co-authored-by: BillyWooo <thedreamofbilly@gmail.com>

Author: 4 people

scripts/pre-commit.sh

@@ -0,0 +1,30 @@
+#!/usr/bin/env bash
+# It is recommended to execute the script before commit
+# which will help us to reduce test/fmt/clippy failures in CI
+
+start=$(date +%s)
+
+make fmt
+taplo fmt
+
+root_dir=$(git rev-parse --show-toplevel)
+CARGO_TARGET_DIR=${root_dir}/target
+
+cd "${root_dir}/tee-worker" || exit
+taplo fmt
+CARGO_TARGET_DIR=${CARGO_TARGET_DIR} cargo clippy --release -- -D warnings || exit
+
+cd "${root_dir}/tee-worker/enclave-runtime" || exit
+taplo fmt
+CARGO_TARGET_DIR=${CARGO_TARGET_DIR} cargo clippy --release -- -D warnings || exit
+
+cd "${root_dir}/tee-worker" || exit
+#RUST_LOG=info CARGO_TARGET_DIR=/root/work/tmp SKIP_WASM_BUILD=1 cargo test --release -- --show-output
+RUST_LOG=info CARGO_TARGET_DIR=${CARGO_TARGET_DIR} SKIP_WASM_BUILD=1 cargo test --release -- --show-output
+SGX_MODE=SW SKIP_WASM_BUILD=1 make
+
+cd "${root_dir}/tee-worker/bin" || exit
+./integritee-service test --all
+
+end=$(date +%s)
+echo "Elapsed Time: $((end-start)) seconds"

tee-worker/Cargo.lock

@@ -20,7 +20,7 @@ use itp_types::{Header, SignedBlock};
 use sp_core::{storage::StorageKey, Pair, H256};
 use sp_finality_grandpa::{AuthorityList, VersionedAuthorityList, GRANDPA_AUTHORITIES_KEY};
 use sp_runtime::MultiSignature;
-use substrate_api_client::{Api, ExtrinsicParams, RpcClient};
+use substrate_api_client::{Api, Events, ExtrinsicParams, RpcClient};
 
 pub type StorageProof = Vec<Vec<u8>>;
 
@@ -37,6 +37,7 @@ pub trait ChainApi {
 	fn is_grandpa_available(&self) -> ApiResult<bool>;
 	fn grandpa_authorities(&self, hash: Option<H256>) -> ApiResult<AuthorityList>;
 	fn grandpa_authorities_proof(&self, hash: Option<H256>) -> ApiResult<StorageProof>;
+	fn events(&self, hash: Option<H256>) -> ApiResult<Events>;
 }
 
 impl<P: Pair, Client: RpcClient, Params: ExtrinsicParams> ChainApi for Api<P, Client, Params>
@@ -99,4 +100,10 @@ where
 			.map(|read_proof| read_proof.proof.into_iter().map(|bytes| bytes.0).collect())
 			.unwrap_or_default())
 	}
+
+	fn events(&self, at_block: Option<H256>) -> ApiResult<Events> {
+		let storagekey = self.metadata.storage_value_key("System", "Events")?;
+		let events_bytes = self.get_opaque_storage_by_key_hash(storagekey, at_block)?;
+		Ok(Events::new(self.metadata.clone(), at_block.unwrap(), events_bytes.unwrap()))
+	}
 }

tee-worker/core-primitives/node-api/api-client-extensions/src/chain.rs

@@ -17,6 +17,9 @@ serde_json = { version = "1.0", default-features = false, features = ["alloc"] }
 # local dependencies
 itp-sgx-runtime-primitives = { path = "../../core-primitives/sgx-runtime-primitives", default-features = false }
 
+# scs
+substrate-api-client = { default-features = false, git = "https://github.com/scs/substrate-api-client.git", branch = "polkadot-v0.9.36" }
+
 # substrate-deps
 frame-system = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.36" }
 pallet-balances = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.36" }
@@ -27,6 +30,12 @@ sp-std = { default-features = false, git = "https://github.com/paritytech/substr
 # litentry
 litentry-primitives = { path = "../../litentry/primitives", default-features = false }
 
+[dev-dependencies]
+sp-core = { git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.36" }
+sp-runtime = { git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.36" }
+substrate-api-client = { git = "https://github.com/scs/substrate-api-client.git", branch = "polkadot-v0.9.36" }
+itp-api-client-types = { path = "../../core-primitives/node-api/api-client-types" }
+
 [features]
 default = ["std"]
 std = [
@@ -36,6 +45,7 @@ std = [
     "serde_json/std",
     "primitive-types/std",
     "itp-sgx-runtime-primitives/std",
+    "substrate-api-client/std",
     # substrate
     "frame-system/std",
     "pallet-balances/std",

tee-worker/core-primitives/types/Cargo.toml

@@ -0,0 +1,126 @@
+// Copyright 2020-2023 Litentry Technologies GmbH.
+// This file is part of Litentry.
+//
+// Litentry is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Litentry is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Litentry.  If not, see <https://www.gnu.org/licenses/>.
+
+use codec::{Decode, Encode, Error, Input};
+use sp_runtime::OpaqueExtrinsic;
+use sp_std::vec::Vec;
+use substrate_api_client::{PlainTip, SubstrateDefaultSignedExtra, UncheckedExtrinsicV4};
+
+/// Same function as in primitives::generic. Needed to be copied as it is private there.
+fn encode_with_vec_prefix<T: Encode, F: Fn(&mut Vec<u8>)>(encoder: F) -> Vec<u8> {
+	let size = sp_std::mem::size_of::<T>();
+	let reserve = match size {
+		0..=0b0011_1111 => 1,
+		0b0100_0000..=0b0011_1111_1111_1111 => 2,
+		_ => 4,
+	};
+	let mut v = Vec::with_capacity(reserve + size);
+	v.resize(reserve, 0);
+	encoder(&mut v);
+
+	// need to prefix with the total length to ensure it's binary compatible with
+	// Vec<u8>.
+	let mut length: Vec<()> = Vec::new();
+	length.resize(v.len() - reserve, ());
+	length.using_encoded(|s| {
+		v.splice(0..reserve, s.iter().cloned());
+	});
+
+	v
+}
+
+#[derive(PartialEq, Eq, Clone, Debug)]
+pub struct OpaqueExtrinsicWithStatus {
+	pub xt: OpaqueExtrinsic,
+	pub status: bool,
+}
+
+impl Encode for OpaqueExtrinsicWithStatus {
+	fn encode(&self) -> Vec<u8> {
+		encode_with_vec_prefix::<Self, _>(|v| {
+			self.xt.encode_to(v);
+			self.status.encode_to(v);
+		})
+	}
+}
+
+#[derive(Clone, Eq, PartialEq, Debug)]
+pub struct ParentchainUncheckedExtrinsicWithStatus<Call> {
+	pub xt: UncheckedExtrinsicV4<Call, SubstrateDefaultSignedExtra<PlainTip>>,
+	pub status: bool,
+}
+
+impl<Call> Decode for ParentchainUncheckedExtrinsicWithStatus<Call>
+where
+	UncheckedExtrinsicV4<Call, SubstrateDefaultSignedExtra<PlainTip>>: Decode,
+{
+	fn decode<I: Input>(input: &mut I) -> Result<Self, Error> {
+		// This is a little more complicated than usual since the binary format must be compatible
+		// with substrate's generic `Vec<u8>` type. Basically this just means accepting that there
+		// will be a prefix of vector length (we don't need
+		// to use this).
+		let _length_do_not_remove_me_see_above: Vec<()> = Decode::decode(input)?;
+
+		Ok(ParentchainUncheckedExtrinsicWithStatus::<Call> {
+			xt: Decode::decode(input)?,
+			status: Decode::decode(input)?,
+		})
+	}
+}
+
+pub fn fill_opaque_extrinsic_with_status(
+	opaque_extrinsic: OpaqueExtrinsic,
+	status: bool,
+) -> Result<OpaqueExtrinsic, codec::Error> {
+	let opaque_extrinsic_with_status = OpaqueExtrinsicWithStatus { xt: opaque_extrinsic, status };
+	OpaqueExtrinsic::from_bytes(opaque_extrinsic_with_status.encode().as_slice())
+}
+
+// #[cfg(test)]
+// mod tests {
+// 	use crate::extrinsics::OpaqueExtrinsicWithStatus;
+// 	use itp_api_client_types::ParentchainUncheckedExtrinsicWithStatus;
+// 	use sp_core::{hexdisplay::HexDisplay, Pair, H256 as Hash};
+// 	use sp_runtime::{generic::Era, testing::sr25519, MultiSignature, OpaqueExtrinsic};
+// 	use substrate_api_client::{
+// 		BaseExtrinsicParams, PlainTip, PlainTipExtrinsicParams, PlainTipExtrinsicParamsBuilder,
+// 		SubstrateDefaultSignedExtra, UncheckedExtrinsicV4,
+// 	};
+//
+// 	#[test]
+// 	fn encode_decode_works() {
+// 		let msg = &b"test-message"[..];
+// 		let (pair, _) = sr25519::Pair::generate();
+// 		let signature = pair.sign(msg);
+// 		let multi_sig = MultiSignature::from(signature);
+// 		let account: AccountId = pair.public().into();
+// 		let tx_params =
+// 			PlainTipExtrinsicParamsBuilder::new().era(Era::mortal(8, 0), Hash::from([0u8; 32]));
+//
+// 		let default_extra = BaseExtrinsicParams::new(0, 0, 2, Hash::from([0u8; 32]), tx_params);
+// 		let xt = UncheckedExtrinsicV4::new_signed(
+// 			vec![1, 1, 1],
+// 			account.into(),
+// 			multi_sig,
+// 			default_extra.signed_extra(),
+// 		);
+// 		let unchecked_with_status =
+// 			ParentchainUncheckedExtrinsicWithStatus { xt: xt.clone(), status: true };
+// 		let op_xt = OpaqueExtrinsic::from_bytes(xt.encode().as_slice()).unwrap();
+// 		let op_xt_with_status = OpaqueExtrinsicWithStatus { raw: op_xt, status: true };
+// 		assert_eq!(with_status, Decode::decode(&mut op_xt_with_status.encode().as_slice()).unwrap())
+// 	}
+// }

tee-worker/core-primitives/types/src/extrinsics.rs

@@ -24,6 +24,7 @@ use codec::{Decode, Encode};
 use sgx_tstd as std;
 use sp_std::vec::Vec;
 
+pub mod extrinsics;
 pub mod storage;
 
 /// Substrate runtimes provide no string type. Hence, for arbitrary data of varying length the

tee-worker/core-primitives/types/src/lib.rs

@@ -26,7 +26,7 @@ use itp_node_api::{
 use itp_sgx_crypto::{key_repository::AccessKey, ShieldingCryptoDecrypt, ShieldingCryptoEncrypt};
 use itp_stf_executor::traits::StfEnclaveSigning;
 use itp_top_pool_author::traits::AuthorApi;
-use itp_types::H256;
+use itp_types::{extrinsics::ParentchainUncheckedExtrinsicWithStatus, H256};
 
 pub mod call_worker;
 pub mod litentry;
@@ -67,8 +67,8 @@ pub(crate) trait Executor<
 	fn decode(
 		&self,
 		input: &mut &[u8],
-	) -> Result<ParentchainUncheckedExtrinsic<Self::Call>, CodecError> {
-		ParentchainUncheckedExtrinsic::<Self::Call>::decode(input)
+	) -> Result<ParentchainUncheckedExtrinsicWithStatus<Self::Call>, CodecError> {
+		ParentchainUncheckedExtrinsicWithStatus::<Self::Call>::decode(input)
 	}
 
 	/// extrinisc in this function should execute successfully on parentchain
@@ -126,10 +126,18 @@ where
 		>,
 		input: &mut &[u8],
 	) -> Result<ExecutionStatus<H256>, Error> {
-		if let Ok(xt) = self.decode(input) {
+		if let Ok(ParentchainUncheckedExtrinsicWithStatus { xt, status }) = self.decode(input) {
 			if self.is_target_call(&xt.function, context.node_meta_data_provider.as_ref()) {
-				self.execute(context, xt.clone())
-					.map(|_| ExecutionStatus::Success(hash_of(&xt)))
+				if status {
+					self.execute(context, xt.clone())
+						.map(|_| ExecutionStatus::Success(hash_of(&xt)))
+				} else {
+					log::warn!(
+						"extrinsic(call index: {:?}) fail to execute on parentchain.",
+						self.call_index(&xt.function)
+					);
+					Ok(ExecutionStatus::Skip)
+				}
 			} else {
 				Ok(ExecutionStatus::NextExecutor)
 			}

tee-worker/core/parentchain/indirect-calls-executor/src/executor/mod.rs

@@ -71,6 +71,7 @@ use std::{sync::Arc, vec, vec::Vec};
 pub enum ExecutionStatus<R> {
 	Success(R),
 	NextExecutor,
+	Skip,
 }
 
 /// Trait to execute the indirect calls found in the extrinsics of a block.
@@ -233,6 +234,7 @@ impl<ShieldingKeyRepository, StfEnclaveSigner, TopPoolAuthor, NodeMetadataProvid
 						break
 					},
 					Ok(ExecutionStatus::NextExecutor) => continue,
+					Ok(ExecutionStatus::Skip) => break,
 					Err(e) => {
 						log::warn!("fail to execute indirect_call. due to {:?} ", e);
 						match e {
@@ -296,7 +298,10 @@ mod test {
 	use itp_stf_primitives::types::AccountId;
 	use itp_test::mock::shielding_crypto_mock::ShieldingCryptoMock;
 	use itp_top_pool_author::mocks::AuthorApiMock;
-	use itp_types::{Block, CallWorkerFn, Request, ShardIdentifier, ShieldFundsFn};
+	use itp_types::{
+		extrinsics::fill_opaque_extrinsic_with_status, Block, CallWorkerFn, Request,
+		ShardIdentifier, ShieldFundsFn,
+	};
 	use sp_core::{ed25519, Pair};
 	use sp_runtime::{MultiSignature, OpaqueExtrinsic};
 	use std::assert_matches::assert_matches;
@@ -328,7 +333,9 @@ mod test {
 				.unwrap();
 
 		let parentchain_block = ParentchainBlockBuilder::default()
-			.with_extrinsics(vec![opaque_extrinsic])
+			.with_extrinsics(vec![
+				fill_opaque_extrinsic_with_status(opaque_extrinsic, true).unwrap()
+			])
 			.build();
 
 		indirect_calls_executor
@@ -338,6 +345,30 @@ mod test {
 		assert_eq!(1, top_pool_author.pending_tops(shard_id()).unwrap().len());
 	}
 
+	#[test]
+	fn failed_indirect_call_is_skipped() {
+		let _ = env_logger::builder().is_test(true).try_init();
+
+		let (indirect_calls_executor, top_pool_author, _) =
+			test_fixtures([0u8; 32], NodeMetadataMock::new());
+
+		let opaque_extrinsic =
+			OpaqueExtrinsic::from_bytes(call_worker_unchecked_extrinsic().encode().as_slice())
+				.unwrap();
+
+		let parentchain_block = ParentchainBlockBuilder::default()
+			.with_extrinsics(vec![
+				fill_opaque_extrinsic_with_status(opaque_extrinsic, false).unwrap()
+			])
+			.build();
+
+		indirect_calls_executor
+			.execute_indirect_calls_in_extrinsics(&parentchain_block)
+			.unwrap();
+
+		assert_eq!(0, top_pool_author.pending_tops(shard_id()).unwrap().len());
+	}
+
 	#[test]
 	fn shielding_call_can_be_added_to_pool_successfully() {
 		let _ = env_logger::builder().is_test(true).try_init();
@@ -353,7 +384,9 @@ mod test {
 		.unwrap();
 
 		let parentchain_block = ParentchainBlockBuilder::default()
-			.with_extrinsics(vec![opaque_extrinsic])
+			.with_extrinsics(vec![
+				fill_opaque_extrinsic_with_status(opaque_extrinsic, true).unwrap()
+			])
 			.build();
 
 		indirect_calls_executor