Validity check for signed Frontier-specific extrinsic not called in block execution

@@ -118,78 +118,26 @@ where
 		}
 	}
 
-	pub fn validate_self_contained(&self, origin: &H160) -> Option<TransactionValidity> {
+	pub fn pre_dispatch_self_contained(
+		&self,
+		origin: &H160,
+	) -> Option<Result<(), TransactionValidityError>> {
 		if let Call::transact(transaction) = self {
-			let validate = || {
-				// We must ensure a transaction can pay the cost of its data bytes.
-				// If it can't it should not be included in a block.
-				let mut gasometer = evm::gasometer::Gasometer::new(
-					transaction.gas_limit.low_u64(),
-					<T as pallet_evm::Config>::config(),
-				);
-				let transaction_cost = match transaction.action {
-					TransactionAction::Call(_) => {
-						evm::gasometer::call_transaction_cost(&transaction.input)
-					}
-					TransactionAction::Create => {
-						evm::gasometer::create_transaction_cost(&transaction.input)
-					}
-				};
-				if gasometer.record_transaction(transaction_cost).is_err() {
-					return InvalidTransaction::Custom(
-						TransactionValidationError::InvalidGasLimit as u8,
-					)
-					.into();
-				}
-
-				if let Some(chain_id) = transaction.signature.chain_id() {
-					if chain_id != T::ChainId::get() {
-						return InvalidTransaction::Custom(
-							TransactionValidationError::InvalidChainId as u8,
-						)
-						.into();
-					}
-				}
-
-				if transaction.gas_limit >= T::BlockGasLimit::get() {
-					return InvalidTransaction::Custom(
-						TransactionValidationError::InvalidGasLimit as u8,
-					)
-					.into();
-				}
-
-				let account_data = pallet_evm::Pallet::<T>::account_basic(&origin);
-
-				if transaction.nonce < account_data.nonce {
-					return InvalidTransaction::Stale.into();
-				}
-
-				let fee = transaction.gas_price.saturating_mul(transaction.gas_limit);
-				let total_payment = transaction.value.saturating_add(fee);
-				if account_data.balance < total_payment {
-					return InvalidTransaction::Payment.into();
-				}
-
-				let min_gas_price = T::FeeCalculator::min_gas_price();
-
-				if transaction.gas_price < min_gas_price {
-					return InvalidTransaction::Payment.into();
-				}
-
-				let mut builder = ValidTransactionBuilder::default()
-					.and_provides((origin, transaction.nonce))
-					.priority(transaction.gas_price.unique_saturated_into());
-
-				if transaction.nonce > account_data.nonce {
-					if let Some(prev_nonce) = transaction.nonce.checked_sub(1.into()) {
-						builder = builder.and_requires((origin, prev_nonce))
-					}
-				}
-
-				builder.build()
-			};
+			Some(Pallet::<T>::validate_transaction_in_block(
+				*origin,
+				&transaction,
+			))
+		} else {
+			None
+		}
+	}
 
-			Some(validate())
+	pub fn validate_self_contained(&self, origin: &H160) -> Option<TransactionValidity> {
+		if let Call::transact(transaction) = self {
+			Some(Pallet::<T>::validate_transaction_in_pool(
+				*origin,
+				transaction,
+			))
 		} else {
 			None
 		}
@@ -259,9 +207,12 @@ pub mod pallet {
 						"pre-block transaction signature invalid; the block cannot be built",
 					);
 
-					Self::do_transact(source, transaction).expect(
+					Self::validate_transaction_in_block(source, &transaction).expect(
 						"pre-block transaction verification failed; the block cannot be built",
 					);
+					Self::apply_validated_transaction(source, transaction).expect(
+						"pre-block transaction execution failed; the block cannot be built",
+					);
 				}
 			}
 
@@ -287,7 +238,7 @@ pub mod pallet {
 				Error::<T>::PreLogExists,
 			);
 
-			Self::do_transact(source, transaction)
+			Self::apply_validated_transaction(source, transaction)
 		}
 	}
 
@@ -418,7 +369,98 @@ impl<T: Config> Pallet<T> {
 		}
 	}
 
-	fn do_transact(source: H160, transaction: Transaction) -> DispatchResultWithPostInfo {
+	// Common controls to be performed in the same way by the pool and the
+	// State Transition Function (STF).
+	// This is the case for all controls except those concerning the nonce.
+	fn validate_transaction_common(
+		origin: H160,
+		transaction: &Transaction,
+	) -> Result<U256, TransactionValidityError> {
+		// We must ensure a transaction can pay the cost of its data bytes.
+		// If it can't it should not be included in a block.
+		let mut gasometer = evm::gasometer::Gasometer::new(
+			transaction.gas_limit.low_u64(),
+			<T as pallet_evm::Config>::config(),
+		);
+		let transaction_cost = match transaction.action {
+			TransactionAction::Call(_) => evm::gasometer::call_transaction_cost(&transaction.input),
+			TransactionAction::Create => {
+				evm::gasometer::create_transaction_cost(&transaction.input)
+			}
+		};
+		if gasometer.record_transaction(transaction_cost).is_err() {
+			return Err(InvalidTransaction::Custom(
+				TransactionValidationError::InvalidGasLimit as u8,
+			)
+			.into());
+		}
+
+		if let Some(chain_id) = transaction.signature.chain_id() {
+			if chain_id != T::ChainId::get() {
+				return Err(InvalidTransaction::Custom(
+					TransactionValidationError::InvalidChainId as u8,
+				)
+				.into());
+			}
+		}
+
+		if transaction.gas_limit >= T::BlockGasLimit::get() {
+			return Err(InvalidTransaction::Custom(
+				TransactionValidationError::InvalidGasLimit as u8,
+			)
+			.into());
+		}
+
+		let account_data = pallet_evm::Pallet::<T>::account_basic(&origin);
+
+		let fee = transaction.gas_price.saturating_mul(transaction.gas_limit);
+		let total_payment = transaction.value.saturating_add(fee);
+		if account_data.balance < total_payment {
+			return Err(InvalidTransaction::Payment.into());
+		}
+
+		let min_gas_price = T::FeeCalculator::min_gas_price();
+
+		if transaction.gas_price < min_gas_price {
+			return Err(InvalidTransaction::Payment.into());
+		}
+
+		Ok(account_data.nonce)
+	}
+
+	// Controls that must be performed by the pool.
+	// The controls common with the State Transition Function (STF) are in
+	// the function `validate_transaction_common`.
+	fn validate_transaction_in_pool(
+		origin: H160,
+		transaction: &Transaction,
+	) -> TransactionValidity {
+		let account_nonce = Self::validate_transaction_common(origin, transaction)?;
+
+		if transaction.nonce < account_nonce {
+			return Err(InvalidTransaction::Stale.into());
+		}
+
+		// The tag provides and requires must be filled correctly according to the nonce.
+		let mut builder = ValidTransactionBuilder::default()
+			.and_provides((origin, transaction.nonce))
+			.priority(transaction.gas_price.unique_saturated_into());
+
+		// In the context of the pool, a transaction with
+		// too high a nonce is still considered valid
+		if transaction.nonce > account_nonce {
+			if let Some(prev_nonce) = transaction.nonce.checked_sub(1.into()) {
+				builder = builder.and_requires((origin, prev_nonce))
+			}
+		}
+
+		builder.build()
+	}
+
+	fn apply_validated_transaction(
+		source: H160,
+		transaction: Transaction,
+	) -> DispatchResultWithPostInfo {
 		let transaction_hash =
 			H256::from_slice(Keccak256::digest(&rlp::encode(&transaction)).as_slice());
 		let transaction_index = Pending::<T>::get().len() as u32;
@@ -565,6 +607,29 @@ impl<T: Config> Pallet<T> {
 			}
 		}
 	}
+
+	/// Validate an Ethereum transaction already in block
+	///
+	/// This function must be called during the pre-dispatch phase
+	/// (just before applying the extrinsic).
+	pub fn validate_transaction_in_block(
+		origin: H160,
+		transaction: &ethereum::TransactionV0,
+	) -> Result<(), TransactionValidityError> {
+		let account_nonce = Self::validate_transaction_common(origin, transaction)?;
+
+		// In the context of the block, a transaction with a nonce that is
+		// too high should be considered invalid and make the whole block invalid.
+		if transaction.nonce > account_nonce {
+			Err(TransactionValidityError::Invalid(
+				InvalidTransaction::Future,
+			))
+		} else if transaction.nonce < account_nonce {
+			Err(TransactionValidityError::Invalid(InvalidTransaction::Stale))
+		} else {
+			Ok(())
+		}
+	}
 }
 
 #[derive(Eq, PartialEq, Clone, sp_runtime::RuntimeDebug)]

@@ -19,6 +19,7 @@
 
 use super::*;
 use crate::IntermediateStateRoot;
+use codec::{WrapperTypeDecode, WrapperTypeEncode};
 use ethereum::{TransactionAction, TransactionSignature};
 use frame_support::{parameter_types, traits::FindAuthor, ConsensusEngineId, PalletId};
 use pallet_evm::{AddressMapping, EnsureAddressTruncated, FeeCalculator};
@@ -27,11 +28,13 @@ use sha3::Digest;
 use sp_core::{H160, H256, U256};
 use sp_runtime::{
 	testing::Header,
-	traits::{BlakeTwo256, IdentityLookup},
+	traits::{BlakeTwo256, IdentityLookup, SignedExtension},
 	AccountId32,
 };
 
-type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
+pub type SignedExtra = (frame_system::CheckSpecVersion<Test>,);
+
+type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test, (), SignedExtra>;
 type Block = frame_system::mocking::MockBlock<Test>;
 
 frame_support::construct_runtime! {
@@ -166,6 +169,54 @@ impl crate::Config for Test {
 	type StateRoot = IntermediateStateRoot;
 }
 
+impl fp_self_contained::SelfContainedCall for Call {
+	type SignedInfo = H160;
+
+	fn is_self_contained(&self) -> bool {
+		match self {
+			Call::Ethereum(call) => call.is_self_contained(),
+			_ => false,
+		}
+	}
+
+	fn check_self_contained(&self) -> Option<Result<Self::SignedInfo, TransactionValidityError>> {
+		match self {
+			Call::Ethereum(call) => call.check_self_contained(),
+			_ => None,
+		}
+	}
+
+	fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option<TransactionValidity> {
+		match self {
+			Call::Ethereum(call) => call.validate_self_contained(info),
+			_ => None,
+		}
+	}
+
+	fn pre_dispatch_self_contained(
+		&self,
+		info: &Self::SignedInfo,
+	) -> Option<Result<(), TransactionValidityError>> {
+		match self {
+			Call::Ethereum(call) => call.pre_dispatch_self_contained(info),
+			_ => None,
+		}
+	}
+
+	fn apply_self_contained(
+		self,
+		info: Self::SignedInfo,
+	) -> Option<sp_runtime::DispatchResultWithInfo<sp_runtime::traits::PostDispatchInfoOf<Self>>> {
+		use sp_runtime::traits::Dispatchable as _;
+		match self {
+			call @ Call::Ethereum(crate::Call::transact(_)) => {
+				Some(call.dispatch(Origin::from(crate::RawOrigin::EthereumTransaction(info))))
+			}
+			_ => None,
+		}
+	}
+}
+
 pub struct AccountInfo {
 	pub address: H160,
 	pub account_id: AccountId32,
@@ -255,6 +306,10 @@ impl UnsignedTransaction {
 	}
 
 	pub fn sign(&self, key: &H256) -> Transaction {
+		self.sign_with_chain_id(key, ChainId::get())
+	}
+
+	pub fn sign_with_chain_id(&self, key: &H256, chain_id: u64) -> Transaction {
 		let hash = self.signing_hash();
 		let msg = libsecp256k1::Message::parse(hash.as_fixed_bytes());
 		let s = libsecp256k1::sign(
@@ -264,7 +319,7 @@ impl UnsignedTransaction {
 		let sig = s.0.serialize();
 
 		let sig = TransactionSignature::new(
-			s.1.serialize() as u64 % 2 + ChainId::get() * 2 + 35,
+			s.1.serialize() as u64 % 2 + chain_id * 2 + 35,
 			H256::from_slice(&sig[0..32]),
 			H256::from_slice(&sig[32..64]),
 		)

@@ -18,13 +18,18 @@
 //! Consensus extension module tests for BABE consensus.
 
 use crate::{
-	mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction,
-	ValidTransactionBuilder, H160, H256, U256,
+	mock::*, CallOrCreateInfo, Error, RawOrigin, Transaction, TransactionAction, H160, H256, U256,
 };
 use ethereum::TransactionSignature;
-use frame_support::{assert_err, assert_noop, assert_ok, unsigned::ValidateUnsigned};
+use frame_support::{
+	assert_err, assert_noop, assert_ok,
+	unsigned::{TransactionValidityError, ValidateUnsigned},
+};
 use rustc_hex::{FromHex, ToHex};
-use sp_runtime::transaction_validity::{InvalidTransaction, TransactionSource};
+use sp_runtime::traits::Applyable;
+use sp_runtime::transaction_validity::{
+	InvalidTransaction, TransactionSource, ValidTransactionBuilder,
+};
 use std::str::FromStr;
 
 // This ERC-20 contract mints the maximum amount of tokens to the contract creator.
@@ -91,7 +96,7 @@ fn transaction_without_enough_gas_should_not_work() {
 }
 
 #[test]
-fn transaction_with_invalid_nonce_should_not_work() {
+fn transaction_with_to_low_nonce_should_not_work() {
 	let (pairs, mut ext) = new_test_ext(1);
 	let alice = &pairs[0];
 
@@ -140,6 +145,57 @@ fn transaction_with_invalid_nonce_should_not_work() {
 	});
 }
 
+#[test]
+fn transaction_with_to_hight_nonce_should_fail_in_block() {
+	let (pairs, mut ext) = new_test_ext(1);
+	let alice = &pairs[0];
+
+	ext.execute_with(|| {
+		let mut transaction = default_erc20_creation_unsigned_transaction();
+		transaction.nonce = U256::one();
+
+		let signed = transaction.sign(&alice.private_key);
+		let call = crate::Call::<Test>::transact(signed);
+		let source = call.check_self_contained().unwrap().unwrap();
+		let extrinsic = fp_self_contained::CheckedExtrinsic::<_, _, SignedExtra, _> {
+			signed: fp_self_contained::CheckedSignature::SelfContained(source),
+			function: Call::Ethereum(call),
+		};
+		use frame_support::weights::GetDispatchInfo as _;
+		let dispatch_info = extrinsic.get_dispatch_info();
+		assert_err!(
+			extrinsic.apply::<Test>(&dispatch_info, 0),
+			TransactionValidityError::Invalid(InvalidTransaction::Future)
+		);
+	});
+}
+
+#[test]
+fn transaction_with_invalid_chain_id_should_fail_in_block() {
+	let (pairs, mut ext) = new_test_ext(1);
+	let alice = &pairs[0];
+
+	ext.execute_with(|| {
+		let transaction =
+			default_erc20_creation_unsigned_transaction().sign_with_chain_id(&alice.private_key, 1);
+
+		let call = crate::Call::<Test>::transact(transaction);
+		let source = call.check_self_contained().unwrap().unwrap();
+		let extrinsic = fp_self_contained::CheckedExtrinsic::<_, _, SignedExtra, _> {
+			signed: fp_self_contained::CheckedSignature::SelfContained(source),
+			function: Call::Ethereum(call),
+		};
+		use frame_support::weights::GetDispatchInfo as _;
+		let dispatch_info = extrinsic.get_dispatch_info();
+		assert_err!(
+			extrinsic.apply::<Test>(&dispatch_info, 0),
+			TransactionValidityError::Invalid(InvalidTransaction::Custom(
+				crate::TransactionValidationError::InvalidChainId as u8,
+			))
+		);
+	});
+}
+
 #[test]
 fn contract_constructor_should_get_executed() {
 	let (pairs, mut ext) = new_test_ext(1);

@@ -135,6 +135,12 @@ where
 				Ok(res)
 			}
 			CheckedSignature::SelfContained(signed_info) => {
+				// If pre-dispatch fail, the block must be considered invalid
+				self.function
+					.pre_dispatch_self_contained(&signed_info)
+					.ok_or(TransactionValidityError::Invalid(
+						InvalidTransaction::BadProof,
+					))??;
 				Ok(self.function.apply_self_contained(signed_info).ok_or(
 					TransactionValidityError::Invalid(InvalidTransaction::BadProof),
 				)?)

@@ -44,6 +44,20 @@ pub trait SelfContainedCall: Dispatchable {
 	/// Validate a self-contained function. Returns `None` if the
 	/// function is not a self-contained.
 	fn validate_self_contained(&self, info: &Self::SignedInfo) -> Option<TransactionValidity>;
+	/// Do any pre-flight stuff for a self-contained call.
+	///
+	/// Note this function by default delegates to `validate_self_contained`, so that
+	/// all checks performed for the transaction queue are also performed during
+	/// the dispatch phase (applying the extrinsic).
+	///
+	/// If you ever override this function, you need to make sure to always
+	/// perform the same validation as in `validate_self_contained`.
+	///
+	/// Returns `None` if the function is not a self-contained.
+	fn pre_dispatch_self_contained(
+		&self,
+		info: &Self::SignedInfo,
+	) -> Option<Result<(), TransactionValidityError>>;
 	/// Apply a self-contained function. Returns `None` if the
 	/// function is not a self-contained.
 	fn apply_self_contained(

@@ -440,6 +440,16 @@ impl fp_self_contained::SelfContainedCall for Call {
 		}
 	}
 
+	fn pre_dispatch_self_contained(
+		&self,
+		info: &Self::SignedInfo,
+	) -> Option<Result<(), TransactionValidityError>> {
+		match self {
+			Call::Ethereum(call) => call.pre_dispatch_self_contained(info),
+			_ => None,
+		}
+	}
+
 	fn apply_self_contained(
 		self,
 		info: Self::SignedInfo,