Fix transaction pruning in tx-pool (#6276)

The `tree_route` generated by the import notification is only from the
old best block to the new best parent. This means, it does not contain
the new best block in `enacted()`. We need to prune the transactions of
the new best block "manually" to fix this bug.

Besides that, this pr also changed the `id` parameter of the `NewBlock`
chain event to `hash`. The hash of a block is unique in contrast to the
block number. (Block id can either be number or hash)

bin/node/cli/src/service.rs

@@ -490,7 +490,7 @@ mod tests {
 					service.transaction_pool().maintain(
 						ChainEvent::NewBlock {
 							is_new_best: true,
-							id: parent_id.clone(),
+							hash: parent_header.hash(),
 							tree_route: None,
 							header: parent_header.clone(),
 						},

client/api/src/client.rs

@@ -234,7 +234,7 @@ pub struct BlockImportNotification<Block: BlockT> {
 	pub header: Block::Header,
 	/// Is this the new best block.
 	pub is_new_best: bool,
-	/// Tree route from old best to new best.
+	/// Tree route from old best to new best parent.
 	///
 	/// If `None`, there was no re-org while importing.
 	pub tree_route: Option<Arc<sp_blockchain::TreeRoute<Block>>>,
@@ -248,3 +248,22 @@ pub struct FinalityNotification<Block: BlockT> {
 	/// Imported block header.
 	pub header: Block::Header,
 }
+
+impl<B: BlockT> From<BlockImportNotification<B>> for sp_transaction_pool::ChainEvent<B> {
+	fn from(n: BlockImportNotification<B>) -> Self {
+		Self::NewBlock {
+			is_new_best: n.is_new_best,
+			hash: n.hash,
+			header: n.header,
+			tree_route: n.tree_route,
+		}
+	}
+}
+
+impl<B: BlockT> From<FinalityNotification<B>> for sp_transaction_pool::ChainEvent<B> {
+	fn from(n: FinalityNotification<B>) -> Self {
+		Self::Finalized {
+			hash: n.hash,
+		}
+	}
+}

client/basic-authorship/src/basic_authorship.rs

@@ -351,11 +351,11 @@ mod tests {
 		}.into_signed_tx()
 	}
 
-	fn chain_event<B: BlockT>(block_number: u64, header: B::Header) -> ChainEvent<B>
+	fn chain_event<B: BlockT>(header: B::Header) -> ChainEvent<B>
 		where NumberFor<B>: From<u64>
 	{
 		ChainEvent::NewBlock {
-			id: BlockId::Number(block_number.into()),
+			hash: header.hash(),
 			tree_route: None,
 			is_new_best: true,
 			header,
@@ -380,8 +380,9 @@ mod tests {
 
 		futures::executor::block_on(
 			txpool.maintain(chain_event(
-				0,
-				client.header(&BlockId::Number(0u64)).expect("header get error").expect("there should be header")
+				client.header(&BlockId::Number(0u64))
+					.expect("header get error")
+					.expect("there should be header")
 			))
 		);
 
@@ -470,7 +471,6 @@ mod tests {
 
 		futures::executor::block_on(
 			txpool.maintain(chain_event(
-				0,
 				client.header(&BlockId::Number(0u64))
 					.expect("header get error")
 					.expect("there should be header"),
@@ -574,8 +574,9 @@ mod tests {
 
 		futures::executor::block_on(
 			txpool.maintain(chain_event(
-				0,
-				client.header(&BlockId::Number(0u64)).expect("header get error").expect("there should be header")
+				client.header(&BlockId::Number(0u64))
+					.expect("header get error")
+					.expect("there should be header")
 			))
 		);
 
@@ -585,8 +586,9 @@ mod tests {
 
 		futures::executor::block_on(
 			txpool.maintain(chain_event(
-				1,
-				client.header(&BlockId::Number(1)).expect("header get error").expect("there should be header")
+				client.header(&BlockId::Number(1))
+					.expect("header get error")
+					.expect("there should be header")
 			))
 		);
 

client/consensus/manual-seal/src/lib.rs

@@ -413,9 +413,10 @@ mod tests {
 		assert!(client.header(&BlockId::Number(0)).unwrap().is_some());
 		assert!(pool.submit_one(&BlockId::Number(1), SOURCE, uxt(Alice, 1)).await.is_ok());
 
+		let header = client.header(&BlockId::Number(1)).expect("db error").expect("imported above");
 		pool.maintain(sp_transaction_pool::ChainEvent::NewBlock {
-			id: BlockId::Number(1),
-			header: client.header(&BlockId::Number(1)).expect("db error").expect("imported above"),
+			hash: header.hash(),
+			header,
 			is_new_best: true,
 			tree_route: None,
 		}).await;

client/service/src/builder.rs

@@ -55,7 +55,7 @@ use std::{
 };
 use wasm_timer::SystemTime;
 use sc_telemetry::{telemetry, SUBSTRATE_INFO};
-use sp_transaction_pool::{MaintainedTransactionPool, ChainEvent};
+use sp_transaction_pool::MaintainedTransactionPool;
 use prometheus_endpoint::Registry;
 use sc_client_db::{Backend, DatabaseSettings};
 use sp_core::traits::CodeExecutor;
@@ -1042,14 +1042,9 @@ ServiceBuilder<
 		{
 			let txpool = Arc::downgrade(&transaction_pool);
 
-			let mut import_stream = client.import_notification_stream().map(|n| ChainEvent::NewBlock {
-				id: BlockId::Hash(n.hash),
-				header: n.header,
-				tree_route: n.tree_route,
-				is_new_best: n.is_new_best,
-			}).fuse();
+			let mut import_stream = client.import_notification_stream().map(Into::into).fuse();
 			let mut finality_stream = client.finality_notification_stream()
-				.map(|n| ChainEvent::Finalized::<TBl> { hash: n.hash })
+				.map(Into::into)
 				.fuse();
 
 			let events = async move {

client/service/src/client/client.rs

@@ -786,7 +786,7 @@ impl<B, E, Block, RA> Client<B, E, Block, RA> where
 			NewBlockState::Normal
 		};
 
-		let tree_route = if is_new_best {
+		let tree_route = if is_new_best && info.best_hash != parent_hash {
 			let route_from_best = sp_blockchain::tree_route(
 				self.backend.blockchain(),
 				info.best_hash,

client/transaction-pool/Cargo.toml

@@ -36,5 +36,7 @@ wasm-timer = "0.2"
 assert_matches = "1.3.0"
 hex = "0.4"
 sp-keyring = { version = "2.0.0-rc2", path = "../../primitives/keyring" }
+sp-consensus = { version = "0.8.0-rc2", path = "../../primitives/consensus/common" }
 substrate-test-runtime-transaction-pool = { version = "2.0.0-rc2", path = "../../test-utils/runtime/transaction-pool" }
 substrate-test-runtime-client = { version = "2.0.0-rc2", path = "../../test-utils/runtime/client" }
+sc-block-builder = { version = "0.8.0-rc2", path = "../block-builder" }

client/transaction-pool/src/lib.rs

@@ -468,10 +468,11 @@ impl<PoolApi, Block> MaintainedTransactionPool for BasicPool<PoolApi, Block>
 {
 	fn maintain(&self, event: ChainEvent<Self::Block>) -> Pin<Box<dyn Future<Output=()> + Send>> {
 		match event {
-			ChainEvent::NewBlock { id, tree_route, is_new_best, .. } => {
+			ChainEvent::NewBlock { hash, tree_route, is_new_best, .. } => {
 				let pool = self.pool.clone();
 				let api = self.api.clone();
 
+				let id = BlockId::hash(hash);
 				let block_number = match api.block_id_to_number(&id) {
 					Ok(Some(number)) => number,
 					_ => {
@@ -495,21 +496,24 @@ impl<PoolApi, Block> MaintainedTransactionPool for BasicPool<PoolApi, Block>
 
 				async move {
 					// If there is a tree route, we use this to prune known tx based on the enacted
-					// blocks and otherwise we only prune known txs if the block is
-					// the new best block.
+					// blocks.
 					if let Some(ref tree_route) = tree_route {
 						future::join_all(
 							tree_route
 								.enacted()
-								.iter().map(|h|
+								.iter()
+								.map(|h|
 									prune_known_txs_for_block(
 										BlockId::Hash(h.hash.clone()),
 										&*api,
 										&*pool,
 									),
 								),
 						).await;
-					} else if is_new_best {
+					}
+
+					// If this is a new best block, we need to prune its transactions from the pool.
+					if is_new_best {
 						prune_known_txs_for_block(id.clone(), &*api, &*pool).await;
 					}