Don't remove invalid transactions when skipping. (#5121)

* Don't remove invalid transactions when skipping.

* Use a special kind of extrinsic instead of arbitrary limit.

* Fix txpool tests.

* Attempt to create more blocks.

* Bump lock

Co-authored-by: Gavin Wood <github@gavwood.com>
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>

substrate/bin/node/executor/benches/bench.rs

@@ -166,7 +166,7 @@ fn bench_execute_block(c: &mut Criterion) {
 			// Get the runtime version to initialize the runtimes cache.
 			{
 				let mut test_ext = new_test_ext(&genesis_config);
-				executor.runtime_version(&mut test_ext.ext());
+				executor.runtime_version(&mut test_ext.ext()).unwrap();
 			}
 
 			let blocks = test_blocks(&genesis_config, &executor);

substrate/client/basic-authorship/src/basic_authorship.rs

@@ -26,15 +26,15 @@ use sp_inherents::InherentData;
 use log::{error, info, debug, trace};
 use sp_core::ExecutionContext;
 use sp_runtime::{
-	traits::{Block as BlockT, Hash as HashT, Header as HeaderT, DigestFor, BlakeTwo256},
 	generic::BlockId,
+	traits::{Block as BlockT, Hash as HashT, Header as HeaderT, DigestFor, BlakeTwo256},
 };
 use sp_transaction_pool::{TransactionPool, InPoolTransaction};
 use sc_telemetry::{telemetry, CONSENSUS_INFO};
 use sc_block_builder::{BlockBuilderApi, BlockBuilderProvider};
 use sp_api::{ProvideRuntimeApi, ApiExt};
 use futures::prelude::*;
-use sp_blockchain::HeaderBackend;
+use sp_blockchain::{HeaderBackend, ApplyExtrinsicFailed};
 use std::marker::PhantomData;
 
 /// Proposer factory.
@@ -230,7 +230,7 @@ impl<A, B, Block, C> ProposerInner<B, Block, C, A>
 				Ok(()) => {
 					debug!("[{:?}] Pushed to the block.", pending_tx_hash);
 				}
-				Err(sp_blockchain::Error::ApplyExtrinsicFailed(sp_blockchain::ApplyExtrinsicFailed::Validity(e)))
+				Err(sp_blockchain::Error::ApplyExtrinsicFailed(ApplyExtrinsicFailed::Validity(e)))
 						if e.exhausted_resources() => {
 					if is_first {
 						debug!("[{:?}] Invalid transaction: FullBlock on empty block", pending_tx_hash);
@@ -246,6 +246,13 @@ impl<A, B, Block, C> ProposerInner<B, Block, C, A>
 						break;
 					}
 				}
+				Err(e) if skipped > 0 => {
+					trace!(
+						"[{:?}] Ignoring invalid transaction when skipping: {}",
+						pending_tx_hash,
+						e
+					);
+				}
 				Err(e) => {
 					debug!("[{:?}] Invalid transaction: {}", pending_tx_hash, e);
 					unqueue_invalid.push(pending_tx_hash);
@@ -292,10 +299,10 @@ mod tests {
 	use super::*;
 
 	use parking_lot::Mutex;
-	use sp_consensus::Proposer;
+	use sp_consensus::{BlockOrigin, Proposer};
 	use substrate_test_runtime_client::{
-		runtime::{Extrinsic, Transfer}, AccountKeyring, DefaultTestClientBuilderExt,
-		TestClientBuilderExt,
+		prelude::*,
+		runtime::{Extrinsic, Transfer},
 	};
 	use sc_transaction_pool::{BasicPool, FullChainApi};
 	use sp_api::Core;
@@ -395,4 +402,73 @@ mod tests {
 			storage_changes.transaction_storage_root,
 		);
 	}
+
+	#[test]
+	fn should_not_remove_invalid_transactions_when_skipping() {
+		// given
+		let mut client = Arc::new(substrate_test_runtime_client::new());
+		let txpool = Arc::new(
+			BasicPool::new(Default::default(), Arc::new(FullChainApi::new(client.clone()))).0
+		);
+
+		futures::executor::block_on(
+			txpool.submit_at(&BlockId::number(0), vec![
+				extrinsic(0),
+				extrinsic(1),
+				Transfer {
+					amount: Default::default(),
+					nonce: 2,
+					from: AccountKeyring::Alice.into(),
+					to: Default::default(),
+				}.into_resources_exhausting_tx(),
+				extrinsic(3),
+				Transfer {
+					amount: Default::default(),
+					nonce: 4,
+					from: AccountKeyring::Alice.into(),
+					to: Default::default(),
+				}.into_resources_exhausting_tx(),
+				extrinsic(5),
+				extrinsic(6),
+			])
+		).unwrap();
+
+		let mut proposer_factory = ProposerFactory::new(client.clone(), txpool.clone());
+		let mut propose_block = |
+			client: &TestClient,
+			number,
+			expected_block_extrinsics,
+			expected_pool_transactions,
+		| {
+			let mut proposer = proposer_factory.init_with_now(
+				&client.header(&BlockId::number(number)).unwrap().unwrap(),
+				Box::new(move || time::Instant::now()),
+			);
+
+			// when
+			let deadline = time::Duration::from_secs(9);
+			let block = futures::executor::block_on(
+				proposer.propose(Default::default(), Default::default(), deadline, RecordProof::No)
+			).map(|r| r.block).unwrap();
+
+			// then
+			// block should have some extrinsics although we have some more in the pool.
+			assert_eq!(block.extrinsics().len(), expected_block_extrinsics);
+			assert_eq!(txpool.ready().count(), expected_pool_transactions);
+
+			block
+		};
+
+		// let's create one block and import it
+		let block = propose_block(&client, 0, 2, 7);
+		client.import(BlockOrigin::Own, block).unwrap();
+
+		// now let's make sure that we can still make some progress
+
+		// This is most likely incorrect, and caused by #5139
+		let tx_remaining = 0;
+		let block = propose_block(&client, 1, 2, tx_remaining);
+		client.import(BlockOrigin::Own, block).unwrap();
+	}
 }
+

substrate/client/transaction-pool/graph/benches/basics.rs

@@ -113,7 +113,11 @@ impl ChainApi for TestApi {
 }
 
 fn uxt(transfer: Transfer) -> Extrinsic {
-	Extrinsic::Transfer(transfer, Default::default())
+	Extrinsic::Transfer {
+		transfer,
+		signature: Default::default(),
+		exhaust_resources_when_not_first: false,
+	}
 }
 
 fn bench_configured(pool: Pool<TestApi>, number: u64) {

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

@@ -524,7 +524,11 @@ mod tests {
 	}
 
 	fn uxt(transfer: Transfer) -> Extrinsic {
-		Extrinsic::Transfer(transfer, Default::default())
+		Extrinsic::Transfer {
+			transfer,
+			signature: Default::default(),
+			exhaust_resources_when_not_first: false,
+		}
 	}
 
 	fn pool() -> Pool<TestApi> {

substrate/test-utils/runtime/src/lib.rs

@@ -101,7 +101,25 @@ impl Transfer {
 	pub fn into_signed_tx(self) -> Extrinsic {
 		let signature = sp_keyring::AccountKeyring::from_public(&self.from)
 			.expect("Creates keyring from public key.").sign(&self.encode()).into();
-		Extrinsic::Transfer(self, signature)
+		Extrinsic::Transfer {
+			transfer: self,
+			signature,
+			exhaust_resources_when_not_first: false,
+		}
+	}
+
+	/// Convert into a signed extrinsic, which will only end up included in the block
+	/// if it's the first transaction. Otherwise it will cause `ResourceExhaustion` error
+	/// which should be considered as block being full.
+	#[cfg(feature = "std")]
+	pub fn into_resources_exhausting_tx(self) -> Extrinsic {
+		let signature = sp_keyring::AccountKeyring::from_public(&self.from)
+			.expect("Creates keyring from public key.").sign(&self.encode()).into();
+		Extrinsic::Transfer {
+			transfer: self,
+			signature,
+			exhaust_resources_when_not_first: true,
+		}
 	}
 }
 
@@ -109,7 +127,11 @@ impl Transfer {
 #[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)]
 pub enum Extrinsic {
 	AuthoritiesChange(Vec<AuthorityId>),
-	Transfer(Transfer, AccountSignature),
+	Transfer {
+		transfer: Transfer,
+		signature: AccountSignature,
+		exhaust_resources_when_not_first: bool,
+	},
 	IncludeData(Vec<u8>),
 	StorageChange(Vec<u8>, Option<Vec<u8>>),
 	ChangesTrieConfigUpdate(Option<ChangesTrieConfiguration>),
@@ -130,9 +152,9 @@ impl BlindCheckable for Extrinsic {
 	fn check(self, _signature: CheckSignature) -> Result<Self, TransactionValidityError> {
 		match self {
 			Extrinsic::AuthoritiesChange(new_auth) => Ok(Extrinsic::AuthoritiesChange(new_auth)),
-			Extrinsic::Transfer(transfer, signature) => {
+			Extrinsic::Transfer { transfer, signature, exhaust_resources_when_not_first } => {
 				if sp_runtime::verify_encoded_lazy(&signature, &transfer, &transfer.from) {
-					Ok(Extrinsic::Transfer(transfer, signature))
+					Ok(Extrinsic::Transfer { transfer, signature, exhaust_resources_when_not_first })
 				} else {
 					Err(InvalidTransaction::BadProof.into())
 				}
@@ -165,7 +187,7 @@ impl ExtrinsicT for Extrinsic {
 impl Extrinsic {
 	pub fn transfer(&self) -> &Transfer {
 		match self {
-			Extrinsic::Transfer(ref transfer, _) => transfer,
+			Extrinsic::Transfer { ref transfer, .. } => transfer,
 			_ => panic!("cannot convert to transfer ref"),
 		}
 	}

substrate/test-utils/runtime/src/system.rs

@@ -192,7 +192,7 @@ pub fn validate_transaction(utx: Extrinsic) -> TransactionValidity {
 /// This doesn't attempt to validate anything regarding the block.
 pub fn execute_transaction(utx: Extrinsic) -> ApplyExtrinsicResult {
 	let extrinsic_index: u32 = storage::unhashed::get(well_known_keys::EXTRINSIC_INDEX).unwrap();
-	let result = execute_transaction_backend(&utx);
+	let result = execute_transaction_backend(&utx, extrinsic_index);
 	ExtrinsicData::insert(extrinsic_index, utx.encode());
 	storage::unhashed::put(well_known_keys::EXTRINSIC_INDEX, &(extrinsic_index + 1));
 	result
@@ -237,7 +237,7 @@ pub fn finalize_block() -> Header {
 		extrinsics_root,
 		state_root: storage_root,
 		parent_hash,
-		digest: digest,
+		digest,
 	}
 }
 
@@ -247,13 +247,18 @@ fn check_signature(utx: &Extrinsic) -> Result<(), TransactionValidityError> {
 	utx.clone().check(CheckSignature::Yes).map_err(|_| InvalidTransaction::BadProof.into()).map(|_| ())
 }
 
-fn execute_transaction_backend(utx: &Extrinsic) -> ApplyExtrinsicResult {
+fn execute_transaction_backend(utx: &Extrinsic, extrinsic_index: u32) -> ApplyExtrinsicResult {
 	check_signature(utx)?;
 	match utx {
-		Extrinsic::Transfer(ref transfer, _) => execute_transfer_backend(transfer),
-		Extrinsic::AuthoritiesChange(ref new_auth) => execute_new_authorities_backend(new_auth),
+		Extrinsic::Transfer { exhaust_resources_when_not_first: true, .. } if extrinsic_index != 0 =>
+			Err(InvalidTransaction::ExhaustsResources.into()),
+		Extrinsic::Transfer { ref transfer, .. } =>
+			execute_transfer_backend(transfer),
+		Extrinsic::AuthoritiesChange(ref new_auth) =>
+			execute_new_authorities_backend(new_auth),
 		Extrinsic::IncludeData(_) => Ok(Ok(())),
-		Extrinsic::StorageChange(key, value) => execute_storage_change(key, value.as_ref().map(|v| &**v)),
+		Extrinsic::StorageChange(key, value) =>
+			execute_storage_change(key, value.as_ref().map(|v| &**v)),
 		Extrinsic::ChangesTrieConfigUpdate(ref new_config) =>
 			execute_changes_trie_config_update(new_config.clone()),
 	}

substrate/test-utils/runtime/transaction-pool/src/lib.rs

@@ -265,7 +265,7 @@ pub fn uxt(who: AccountKeyring, nonce: Index) -> Extrinsic {
 		nonce,
 		amount: 1,
 	};
-	let signature = transfer.using_encoded(|e| who.sign(e));
-	Extrinsic::Transfer(transfer, signature.into())
+	let signature = transfer.using_encoded(|e| who.sign(e)).into();
+	Extrinsic::Transfer { transfer, signature, exhaust_resources_when_not_first: false }
 }