Pass transaction source to validate_transaction (paritytech#5366)

* WiP * Support source in the runtime API. * Finish implementation in txpool. * Fix warning. * Fix tests. * Apply suggestions from code review Co-Authored-By: Kian Paimani <[email protected]> Co-Authored-By: Nikolay Volf <[email protected]> * Extra changes. * Fix test and benches. * fix test * Fix test & benches again. * Fix tests. * Update bumpalo * Fix doc test. * Fix doctest. * Fix doctest. Co-authored-by: Kian Paimani <[email protected]> Co-authored-by: Nikolay Volf <[email protected]>
elgammalqa · Mar 25, 2020 · 29fa243 · 29fa243
1 parent 3b7496c
commit 29fa243
Show file tree

Hide file tree

Showing 37 changed files with 414 additions and 163 deletions.
diff --git a/bin/node-template/runtime/src/lib.rs b/bin/node-template/runtime/src/lib.rs
@@ -11,8 +11,8 @@ include!(concat!(env!("OUT_DIR"), "/wasm_binary.rs"));
 use sp_std::prelude::*;
 use sp_core::OpaqueMetadata;
 use sp_runtime::{
-	ApplyExtrinsicResult, transaction_validity::TransactionValidity, generic, create_runtime_str,
-	impl_opaque_keys, MultiSignature,
+	ApplyExtrinsicResult, generic, create_runtime_str, impl_opaque_keys, MultiSignature,
+	transaction_validity::{TransactionValidity, TransactionSource},
 };
 use sp_runtime::traits::{
 	BlakeTwo256, Block as BlockT, IdentityLookup, Verify, ConvertInto, IdentifyAccount
@@ -318,8 +318,11 @@ impl_runtime_apis! {
 	}
 
 	impl sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block> for Runtime {
-		fn validate_transaction(tx: <Block as BlockT>::Extrinsic) -> TransactionValidity {
-			Executive::validate_transaction(tx)
+		fn validate_transaction(
+			source: TransactionSource,
+			tx: <Block as BlockT>::Extrinsic,
+		) -> TransactionValidity {
+			Executive::validate_transaction(source, tx)
 		}
 	}
 

diff --git a/bin/node/executor/tests/submit_transaction.rs b/bin/node/executor/tests/submit_transaction.rs
@@ -138,7 +138,7 @@ fn should_submit_signed_twice_from_the_same_account() {
 fn submitted_transaction_should_be_valid() {
 	use codec::Encode;
 	use frame_support::storage::StorageMap;
-	use sp_runtime::transaction_validity::ValidTransaction;
+	use sp_runtime::transaction_validity::{ValidTransaction, TransactionSource};
 	use sp_runtime::traits::StaticLookup;
 
 	let mut t = new_test_ext(COMPACT_CODE, false);
@@ -163,6 +163,7 @@ fn submitted_transaction_should_be_valid() {
 	let tx0 = state.read().transactions[0].clone();
 	let mut t = new_test_ext(COMPACT_CODE, false);
 	t.execute_with(|| {
+		let source = TransactionSource::External;
 		let extrinsic = UncheckedExtrinsic::decode(&mut &*tx0).unwrap();
 		// add balance to the account
 		let author = extrinsic.signature.clone().unwrap().0;
@@ -172,7 +173,7 @@ fn submitted_transaction_should_be_valid() {
 		<frame_system::Account<Runtime>>::insert(&address, account);
 
 		// check validity
-		let res = Executive::validate_transaction(extrinsic);
+		let res = Executive::validate_transaction(source, extrinsic);
 
 		assert_eq!(res.unwrap(), ValidTransaction {
 			priority: 2_411_002_000_000,

diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs
@@ -35,7 +35,7 @@ use sp_runtime::{
 	impl_opaque_keys, generic, create_runtime_str,
 };
 use sp_runtime::curve::PiecewiseLinear;
-use sp_runtime::transaction_validity::TransactionValidity;
+use sp_runtime::transaction_validity::{TransactionValidity, TransactionSource};
 use sp_runtime::traits::{
 	self, BlakeTwo256, Block as BlockT, StaticLookup, SaturatedConversion,
 	ConvertInto, OpaqueKeys,
@@ -734,8 +734,11 @@ impl_runtime_apis! {
 	}
 
 	impl sp_transaction_pool::runtime_api::TaggedTransactionQueue<Block> for Runtime {
-		fn validate_transaction(tx: <Block as BlockT>::Extrinsic) -> TransactionValidity {
-			Executive::validate_transaction(tx)
+		fn validate_transaction(
+			source: TransactionSource,
+			tx: <Block as BlockT>::Extrinsic,
+		) -> TransactionValidity {
+			Executive::validate_transaction(source, tx)
 		}
 	}
 

diff --git a/client/basic-authorship/src/basic_authorship.rs b/client/basic-authorship/src/basic_authorship.rs
@@ -315,13 +315,15 @@ mod tests {
 		prelude::*,
 		runtime::{Extrinsic, Transfer},
 	};
-	use sp_transaction_pool::{ChainEvent, MaintainedTransactionPool};
+	use sp_transaction_pool::{ChainEvent, MaintainedTransactionPool, TransactionSource};
 	use sc_transaction_pool::{BasicPool, FullChainApi};
 	use sp_api::Core;
 	use backend::Backend;
 	use sp_blockchain::HeaderBackend;
 	use sp_runtime::traits::NumberFor;
 
+	const SOURCE: TransactionSource = TransactionSource::External;
+
 	fn extrinsic(nonce: u64) -> Extrinsic {
 		Transfer {
 			amount: Default::default(),
@@ -338,7 +340,7 @@ mod tests {
 			id: BlockId::Number(block_number.into()),
 			retracted: vec![],
 			is_new_best: true,
-			header: header,
+			header,
 		}
 	}
 
@@ -351,7 +353,7 @@ mod tests {
 		);
 
 		futures::executor::block_on(
-			txpool.submit_at(&BlockId::number(0), vec![extrinsic(0), extrinsic(1)])
+			txpool.submit_at(&BlockId::number(0), SOURCE, vec![extrinsic(0), extrinsic(1)])
 		).unwrap();
 
 		futures::executor::block_on(
@@ -403,7 +405,7 @@ mod tests {
 		let block_id = BlockId::Hash(genesis_hash);
 
 		futures::executor::block_on(
-			txpool.submit_at(&BlockId::number(0), vec![extrinsic(0)]),
+			txpool.submit_at(&BlockId::number(0), SOURCE, vec![extrinsic(0)]),
 		).unwrap();
 
 		futures::executor::block_on(
@@ -454,7 +456,7 @@ mod tests {
 		);
 
 		futures::executor::block_on(
-			txpool.submit_at(&BlockId::number(0), vec![
+			txpool.submit_at(&BlockId::number(0), SOURCE, vec![
 				extrinsic(0),
 				extrinsic(1),
 				Transfer {

diff --git a/client/consensus/manual-seal/src/lib.rs b/client/consensus/manual-seal/src/lib.rs
@@ -224,7 +224,7 @@ mod tests {
 		txpool::Options,
 	};
 	use substrate_test_runtime_transaction_pool::{TestApi, uxt};
-	use sp_transaction_pool::{TransactionPool, MaintainedTransactionPool};
+	use sp_transaction_pool::{TransactionPool, MaintainedTransactionPool, TransactionSource};
 	use sp_runtime::generic::BlockId;
 	use sp_blockchain::HeaderBackend;
 	use sp_consensus::ImportedAux;
@@ -236,6 +236,8 @@ mod tests {
 		Arc::new(TestApi::empty())
 	}
 
+	const SOURCE: TransactionSource = TransactionSource::External;
+
 	#[tokio::test]
 	async fn instant_seal() {
 		let builder = TestClientBuilder::new();
@@ -278,7 +280,7 @@ mod tests {
 			rt.block_on(future);
 		});
 		// submit a transaction to pool.
-		let result = pool.submit_one(&BlockId::Number(0), uxt(Alice, 0)).await;
+		let result = pool.submit_one(&BlockId::Number(0), SOURCE, uxt(Alice, 0)).await;
 		// assert that it was successfully imported
 		assert!(result.is_ok());
 		// assert that the background task returns ok
@@ -330,7 +332,7 @@ mod tests {
 			rt.block_on(future);
 		});
 		// submit a transaction to pool.
-		let result = pool.submit_one(&BlockId::Number(0), uxt(Alice, 0)).await;
+		let result = pool.submit_one(&BlockId::Number(0), SOURCE, uxt(Alice, 0)).await;
 		// assert that it was successfully imported
 		assert!(result.is_ok());
 		let (tx, rx) = futures::channel::oneshot::channel();
@@ -399,7 +401,7 @@ mod tests {
 			rt.block_on(future);
 		});
 		// submit a transaction to pool.
-		let result = pool.submit_one(&BlockId::Number(0), uxt(Alice, 0)).await;
+		let result = pool.submit_one(&BlockId::Number(0), SOURCE, uxt(Alice, 0)).await;
 		// assert that it was successfully imported
 		assert!(result.is_ok());
 
@@ -430,7 +432,7 @@ mod tests {
 		);
 		// assert that there's a new block in the db.
 		assert!(backend.blockchain().header(BlockId::Number(0)).unwrap().is_some());
-		assert!(pool.submit_one(&BlockId::Number(1), uxt(Alice, 1)).await.is_ok());
+		assert!(pool.submit_one(&BlockId::Number(1), SOURCE, uxt(Alice, 1)).await.is_ok());
 
 		pool.maintain(sp_transaction_pool::ChainEvent::NewBlock {
 			id: BlockId::Number(1),
@@ -453,7 +455,7 @@ mod tests {
 		assert!(backend.blockchain().header(BlockId::Number(1)).unwrap().is_some());
 		pool_api.increment_nonce(Alice.into());
 
-		assert!(pool.submit_one(&BlockId::Number(2), uxt(Alice, 2)).await.is_ok());
+		assert!(pool.submit_one(&BlockId::Number(2), SOURCE, uxt(Alice, 2)).await.is_ok());
 		let (tx2, rx2) = futures::channel::oneshot::channel();
 		assert!(sink.send(EngineCommand::SealNewBlock {
 			parent_hash: Some(created_block.hash),

diff --git a/client/offchain/src/lib.rs b/client/offchain/src/lib.rs
@@ -191,7 +191,8 @@ mod tests {
 			at: &BlockId<Block>,
 			extrinsic: <Block as traits::Block>::Extrinsic,
 		) -> Result<(), ()> {
-			futures::executor::block_on(self.0.submit_one(&at, extrinsic))
+			let source = sp_transaction_pool::TransactionSource::Local;
+			futures::executor::block_on(self.0.submit_one(&at, source, extrinsic))
 				.map(|_| ())
 				.map_err(|_| ())
 		}

diff --git a/client/rpc/src/author/mod.rs b/client/rpc/src/author/mod.rs
@@ -37,7 +37,7 @@ use sp_core::{Bytes, traits::BareCryptoStorePtr};
 use sp_api::ProvideRuntimeApi;
 use sp_runtime::generic;
 use sp_transaction_pool::{
-	TransactionPool, InPoolTransaction, TransactionStatus,
+	TransactionPool, InPoolTransaction, TransactionStatus, TransactionSource,
 	BlockHash, TxHash, TransactionFor, error::IntoPoolError,
 };
 use sp_session::SessionKeys;
@@ -75,6 +75,14 @@ impl<P, Client> Author<P, Client> {
 	}
 }
 
+
+/// Currently we treat all RPC transactions as externals.
+///
+/// Possibly in the future we could allow opt-in for special treatment
+/// of such transactions, so that the block authors can inject
+/// some unique transactions via RPC and have them included in the pool.
+const TX_SOURCE: TransactionSource = TransactionSource::External;
+
 impl<P, Client> AuthorApi<TxHash<P>, BlockHash<P>> for Author<P, Client>
 	where
 		P: TransactionPool + Sync + Send + 'static,
@@ -127,7 +135,7 @@ impl<P, Client> AuthorApi<TxHash<P>, BlockHash<P>> for Author<P, Client>
 		};
 		let best_block_hash = self.client.info().best_hash;
 		Box::new(self.pool
-			.submit_one(&generic::BlockId::hash(best_block_hash), xt)
+			.submit_one(&generic::BlockId::hash(best_block_hash), TX_SOURCE, xt)
 			.compat()
 			.map_err(|e| e.into_pool_error()
 				.map(Into::into)
@@ -173,7 +181,7 @@ impl<P, Client> AuthorApi<TxHash<P>, BlockHash<P>> for Author<P, Client>
 				.map_err(error::Error::from)?;
 			Ok(
 				self.pool
-					.submit_and_watch(&generic::BlockId::hash(best_block_hash), dxt)
+					.submit_and_watch(&generic::BlockId::hash(best_block_hash), TX_SOURCE, dxt)
 					.map_err(|e| e.into_pool_error()
 						.map(error::Error::from)
 						.unwrap_or_else(|e| error::Error::Verification(Box::new(e)).into())

diff --git a/client/rpc/src/state/tests.rs b/client/rpc/src/state/tests.rs
@@ -403,7 +403,7 @@ fn should_return_runtime_version() {
 
 	let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\
 		\"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",2],\
-		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",1],[\"0x40fe3ad401f8959a\",4],\
+		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",2],[\"0x40fe3ad401f8959a\",4],\
 		[\"0xc6e9a76309f39b09\",1],[\"0xdd718d5cc53262d4\",1],[\"0xcbca25e39f142387\",1],\
 		[\"0xf78b278be53f454c\",2],[\"0xab3c0572291feb8b\",1],[\"0xbc9d89904f5b923f\",1]]}";
 

diff --git a/client/service/src/lib.rs b/client/service/src/lib.rs
@@ -621,7 +621,8 @@ where
 		match Decode::decode(&mut &encoded[..]) {
 			Ok(uxt) => {
 				let best_block_id = BlockId::hash(self.client.info().best_hash);
-				let import_future = self.pool.submit_one(&best_block_id, uxt);
+				let source = sp_transaction_pool::TransactionSource::External;
+				let import_future = self.pool.submit_one(&best_block_id, source, uxt);
 				let import_future = import_future
 					.map(move |import_result| {
 						match import_result {
@@ -674,15 +675,20 @@ mod tests {
 			Default::default(),
 			Arc::new(FullChainApi::new(client.clone())),
 		).0);
+		let source = sp_runtime::transaction_validity::TransactionSource::External;
 		let best = longest_chain.best_chain().unwrap();
 		let transaction = Transfer {
 			amount: 5,
 			nonce: 0,
 			from: AccountKeyring::Alice.into(),
 			to: Default::default(),
 		}.into_signed_tx();
-		block_on(pool.submit_one(&BlockId::hash(best.hash()), transaction.clone())).unwrap();
-		block_on(pool.submit_one(&BlockId::hash(best.hash()), Extrinsic::IncludeData(vec![1]))).unwrap();
+		block_on(pool.submit_one(
+			&BlockId::hash(best.hash()), source, transaction.clone()),
+		).unwrap();
+		block_on(pool.submit_one(
+			&BlockId::hash(best.hash()), source, Extrinsic::IncludeData(vec![1])),
+		).unwrap();
 		assert_eq!(pool.status().ready, 2);
 
 		// when

diff --git a/client/service/test/src/lib.rs b/client/service/test/src/lib.rs
@@ -482,9 +482,10 @@ pub fn sync<G, E, Fb, F, Lb, L, B, ExF, U>(
 	let first_user_data = &network.full_nodes[0].2;
 	let best_block = BlockId::number(first_service.get().client().chain_info().best_number);
 	let extrinsic = extrinsic_factory(&first_service.get(), first_user_data);
+	let source = sp_transaction_pool::TransactionSource::External;
 
 	futures::executor::block_on(
-		first_service.get().transaction_pool().submit_one(&best_block, extrinsic)
+		first_service.get().transaction_pool().submit_one(&best_block, source, extrinsic)
 	).expect("failed to submit extrinsic");
 
 	network.run_until_all_full(

diff --git a/client/transaction-pool/graph/benches/basics.rs b/client/transaction-pool/graph/benches/basics.rs
@@ -18,12 +18,14 @@ use criterion::{criterion_group, criterion_main, Criterion};
 
 use futures::{future::{ready, Ready}, executor::block_on};
 use sc_transaction_graph::*;
-use sp_runtime::transaction_validity::{ValidTransaction, InvalidTransaction};
 use codec::Encode;
 use substrate_test_runtime::{Block, Extrinsic, Transfer, H256, AccountId};
 use sp_runtime::{
 	generic::BlockId,
-	transaction_validity::{TransactionValidity, TransactionTag as Tag},
+	transaction_validity::{
+		ValidTransaction, InvalidTransaction, TransactionValidity, TransactionTag as Tag,
+		TransactionSource,
+	},
 };
 use sp_core::blake2_256;
 
@@ -55,6 +57,7 @@ impl ChainApi for TestApi {
 	fn validate_transaction(
 		&self,
 		at: &BlockId<Self::Block>,
+		_source: TransactionSource,
 		uxt: ExtrinsicFor<Self>,
 	) -> Self::ValidationFuture {
 		let nonce = uxt.transfer().nonce;
@@ -121,6 +124,7 @@ fn uxt(transfer: Transfer) -> Extrinsic {
 }
 
 fn bench_configured(pool: Pool<TestApi>, number: u64) {
+	let source = TransactionSource::External;
 	let mut futures = Vec::new();
 	let mut tags = Vec::new();
 
@@ -133,7 +137,7 @@ fn bench_configured(pool: Pool<TestApi>, number: u64) {
 		});
 
 		tags.push(to_tag(nonce, AccountId::from_h256(H256::from_low_u64_be(1))));
-		futures.push(pool.submit_one(&BlockId::Number(1), xt));
+		futures.push(pool.submit_one(&BlockId::Number(1), source, xt));
 	}
 
 	let res = block_on(futures::future::join_all(futures.into_iter()));