transaction-pool: drop unpropagable txs if local node cant author blocks (#8048)

* transaction-pool: drop unpropagable txs if local node cant author blocks

* fix test compilation

* transaction-pool: remove unnecessary static bound on CanAuthor

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

* rpc-api: add translation for PoolError::Unactionable

* transaction-pool: add test for rejecting unactionable transactions

* basic-authorship: fix doc test

* transaction-pool: fix benchmark compilation

* transaction-pool: rename CanAuthor to IsValidator

* transaction-pool: nit in error message

Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

substrate/client/transaction-pool/graph/src/lib.rs

@@ -39,6 +39,6 @@ pub mod watcher;
 
 pub use self::base_pool::Transaction;
 pub use self::pool::{
-	Pool, Options, ChainApi, EventStream, ExtrinsicFor, ExtrinsicHash,
-	BlockHash, NumberFor, TransactionFor, ValidatedTransaction,
+	BlockHash, ChainApi, EventStream, ExtrinsicFor, ExtrinsicHash, IsValidator, NumberFor, Options,
+	Pool, TransactionFor, ValidatedTransaction,
 };

substrate/client/transaction-pool/graph/src/pool.rs

@@ -36,7 +36,7 @@ use wasm_timer::Instant;
 use futures::channel::mpsc::Receiver;
 
 use crate::validated_pool::ValidatedPool;
-pub use crate::validated_pool::ValidatedTransaction;
+pub use crate::validated_pool::{IsValidator, ValidatedTransaction};
 
 /// Modification notification event stream type;
 pub type EventStream<H> = Receiver<H>;
@@ -150,9 +150,9 @@ where
 
 impl<B: ChainApi> Pool<B> {
 	/// Create a new transaction pool.
-	pub fn new(options: Options, api: Arc<B>) -> Self {
+	pub fn new(options: Options, is_validator: IsValidator, api: Arc<B>) -> Self {
 		Pool {
-			validated_pool: Arc::new(ValidatedPool::new(options, api)),
+			validated_pool: Arc::new(ValidatedPool::new(options, is_validator, api)),
 		}
 	}
 
@@ -497,43 +497,58 @@ mod tests {
 		) -> Self::ValidationFuture {
 			let hash = self.hash_and_length(&uxt).0;
 			let block_number = self.block_id_to_number(at).unwrap().unwrap();
-			let nonce = uxt.transfer().nonce;
 
-			// This is used to control the test flow.
-			if nonce > 0 {
-				let opt = self.delay.lock().take();
-				if let Some(delay) = opt {
-					if delay.recv().is_err() {
-						println!("Error waiting for delay!");
+			let res = match uxt {
+				Extrinsic::Transfer { transfer, .. } => {
+					let nonce = transfer.nonce;
+
+					// This is used to control the test flow.
+					if nonce > 0 {
+						let opt = self.delay.lock().take();
+						if let Some(delay) = opt {
+							if delay.recv().is_err() {
+								println!("Error waiting for delay!");
+							}
+						}
 					}
-				}
-			}
 
-			if self.invalidate.lock().contains(&hash) {
-				return futures::future::ready(Ok(InvalidTransaction::Custom(0).into()));
-			}
+					if self.invalidate.lock().contains(&hash) {
+						InvalidTransaction::Custom(0).into()
+					} else if nonce < block_number {
+						InvalidTransaction::Stale.into()
+					} else {
+						let mut transaction = ValidTransaction {
+							priority: 4,
+							requires: if nonce > block_number { vec![vec![nonce as u8 - 1]] } else { vec![] },
+							provides: if nonce == INVALID_NONCE { vec![] } else { vec![vec![nonce as u8]] },
+							longevity: 3,
+							propagate: true,
+						};
 
-			futures::future::ready(if nonce < block_number {
-				Ok(InvalidTransaction::Stale.into())
-			} else {
-				let mut transaction = ValidTransaction {
-					priority: 4,
-					requires: if nonce > block_number { vec![vec![nonce as u8 - 1]] } else { vec![] },
-					provides: if nonce == INVALID_NONCE { vec![] } else { vec![vec![nonce as u8]] },
-					longevity: 3,
-					propagate: true,
-				};
+						if self.clear_requirements.lock().contains(&hash) {
+							transaction.requires.clear();
+						}
 
-				if self.clear_requirements.lock().contains(&hash) {
-					transaction.requires.clear();
-				}
+						if self.add_requirements.lock().contains(&hash) {
+							transaction.requires.push(vec![128]);
+						}
 
-				if self.add_requirements.lock().contains(&hash) {
-					transaction.requires.push(vec![128]);
-				}
+						Ok(transaction)
+					}
+				},
+				Extrinsic::IncludeData(_) => {
+					Ok(ValidTransaction {
+						priority: 9001,
+						requires: vec![],
+						provides: vec![vec![42]],
+						longevity: 9001,
+						propagate: false,
+					})
+				},
+				_ => unimplemented!(),
+			};
 
-				Ok(Ok(transaction))
-			})
+			futures::future::ready(Ok(res))
 		}
 
 		/// Returns a block number given the block id.
@@ -579,7 +594,7 @@ mod tests {
 	}
 
 	fn pool() -> Pool<TestApi> {
-		Pool::new(Default::default(), TestApi::default().into())
+		Pool::new(Default::default(), true.into(), TestApi::default().into())
 	}
 
 	#[test]
@@ -620,6 +635,26 @@ mod tests {
 		assert_matches!(res.unwrap_err(), error::Error::TemporarilyBanned);
 	}
 
+	#[test]
+	fn should_reject_unactionable_transactions() {
+		// given
+		let pool = Pool::new(
+			Default::default(),
+			// the node does not author blocks
+			false.into(),
+			TestApi::default().into(),
+		);
+
+		// after validation `IncludeData` will be set to non-propagable
+		let uxt = Extrinsic::IncludeData(vec![42]);
+
+		// when
+		let res = block_on(pool.submit_one(&BlockId::Number(0), SOURCE, uxt));
+
+		// then
+		assert_matches!(res.unwrap_err(), error::Error::Unactionable);
+	}
+
 	#[test]
 	fn should_notify_about_pool_events() {
 		let (stream, hash0, hash1) = {
@@ -722,11 +757,14 @@ mod tests {
 			count: 100,
 			total_bytes: 200,
 		};
-		let pool = Pool::new(Options {
+
+		let options = Options {
 			ready: limit.clone(),
 			future: limit.clone(),
 			..Default::default()
-		}, TestApi::default().into());
+		};
+
+		let pool = Pool::new(options, true.into(), TestApi::default().into());
 
 		let hash1 = block_on(pool.submit_one(&BlockId::Number(0), SOURCE, uxt(Transfer {
 			from: AccountId::from_h256(H256::from_low_u64_be(1)),
@@ -757,11 +795,14 @@ mod tests {
 			count: 100,
 			total_bytes: 10,
 		};
-		let pool = Pool::new(Options {
+
+		let options = Options {
 			ready: limit.clone(),
 			future: limit.clone(),
 			..Default::default()
-		}, TestApi::default().into());
+		};
+
+		let pool = Pool::new(options, true.into(), TestApi::default().into());
 
 		// when
 		block_on(pool.submit_one(&BlockId::Number(0), SOURCE, uxt(Transfer {
@@ -939,11 +980,13 @@ mod tests {
 				count: 1,
 				total_bytes: 1000,
 			};
-			let pool = Pool::new(Options {
+			let options = Options {
 				ready: limit.clone(),
 				future: limit.clone(),
 				..Default::default()
-			}, TestApi::default().into());
+			};
+
+			let pool = Pool::new(options, true.into(), TestApi::default().into());
 
 			let xt = uxt(Transfer {
 				from: AccountId::from_h256(H256::from_low_u64_be(1)),
@@ -977,7 +1020,7 @@ mod tests {
 			let (tx, rx) = std::sync::mpsc::sync_channel(1);
 			let mut api = TestApi::default();
 			api.delay = Arc::new(Mutex::new(rx.into()));
-			let pool = Arc::new(Pool::new(Default::default(), api.into()));
+			let pool = Arc::new(Pool::new(Default::default(), true.into(), api.into()));
 
 			// when
 			let xt = uxt(Transfer {

substrate/client/transaction-pool/graph/src/validated_pool.rs

@@ -90,9 +90,25 @@ pub type ValidatedTransactionFor<B> = ValidatedTransaction<
 	<B as ChainApi>::Error,
 >;
 
+/// A closure that returns true if the local node is a validator that can author blocks.
+pub struct IsValidator(Box<dyn Fn() -> bool + Send + Sync>);
+
+impl From<bool> for IsValidator {
+	fn from(is_validator: bool) -> Self {
+		IsValidator(Box::new(move || is_validator))
+	}
+}
+
+impl From<Box<dyn Fn() -> bool + Send + Sync>> for IsValidator {
+	fn from(is_validator: Box<dyn Fn() -> bool + Send + Sync>) -> Self {
+		IsValidator(is_validator)
+	}
+}
+
 /// Pool that deals with validated transactions.
 pub struct ValidatedPool<B: ChainApi> {
 	api: Arc<B>,
+	is_validator: IsValidator,
 	options: Options,
 	listener: RwLock<Listener<ExtrinsicHash<B>, B>>,
 	pool: RwLock<base::BasePool<
@@ -116,9 +132,10 @@ where
 
 impl<B: ChainApi> ValidatedPool<B> {
 	/// Create a new transaction pool.
-	pub fn new(options: Options, api: Arc<B>) -> Self {
+	pub fn new(options: Options, is_validator: IsValidator, api: Arc<B>) -> Self {
 		let base_pool = base::BasePool::new(options.reject_future_transactions);
 		ValidatedPool {
+			is_validator,
 			options,
 			listener: Default::default(),
 			api,
@@ -183,6 +200,10 @@ impl<B: ChainApi> ValidatedPool<B> {
 	fn submit_one(&self, tx: ValidatedTransactionFor<B>) -> Result<ExtrinsicHash<B>, B::Error> {
 		match tx {
 			ValidatedTransaction::Valid(tx) => {
+				if !tx.propagate && !(self.is_validator.0)() {
+					return Err(error::Error::Unactionable.into());
+				}
+
 				let imported = self.pool.write().import(tx)?;
 
 				if let base::Imported::Ready { ref hash, .. } = imported {