Don't remove invalid transactions when skipping.

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);
@@ -395,4 +402,47 @@ mod tests {
 			storage_changes.transaction_storage_root,
 		);
 	}
+
+	#[test]
+	fn should_not_remove_invalid_transactions_when_skipping() {
+		// given
+		let 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_exhaust_tx(),
+				extrinsic(3),
+				extrinsic(4),
+				extrinsic(5),
+			])
+		).unwrap();
+
+		let mut proposer_factory = ProposerFactory::new(client.clone(), txpool.clone());
+		let mut proposer = proposer_factory.init_with_now(
+			&client.header(&BlockId::number(0)).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(), 2);
+		assert_eq!(txpool.ready().count(), 6);
+	}
 }
+

client/network/src/protocol/sync/blocks.rs

@@ -14,7 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::mem;
 use std::cmp;
 use std::ops::Range;
 use std::collections::{HashMap, BTreeMap};

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: false,
+		}
 	}
 
 	fn pool() -> Pool<TestApi> {

test-utils/runtime/src/lib.rs

@@ -101,15 +101,36 @@ 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: false,
+		}
+	}
+
+	/// Convert into a signed extrinsic, which will not end up included
+	/// in block due to resource exhaustion.
+	#[cfg(feature = "std")]
+	pub fn into_exhaust_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: true,
+		}
 	}
 }
 
 /// Extrinsic for test-runtime.
 #[derive(Clone, PartialEq, Eq, Encode, Decode, RuntimeDebug)]
 pub enum Extrinsic {
 	AuthoritiesChange(Vec<AuthorityId>),
-	Transfer(Transfer, AccountSignature),
+	Transfer {
+		transfer: Transfer,
+		signature: AccountSignature,
+		exhaust_resources: bool,
+	},
 	IncludeData(Vec<u8>),
 	StorageChange(Vec<u8>, Option<Vec<u8>>),
 	ChangesTrieConfigUpdate(Option<ChangesTrieConfiguration>),
@@ -130,9 +151,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 } => {
 				if sp_runtime::verify_encoded_lazy(&signature, &transfer, &transfer.from) {
-					Ok(Extrinsic::Transfer(transfer, signature))
+					Ok(Extrinsic::Transfer { transfer, signature, exhaust_resources })
 				} else {
 					Err(InvalidTransaction::BadProof.into())
 				}
@@ -165,7 +186,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"),
 		}
 	}

test-utils/runtime/src/system.rs

@@ -237,7 +237,7 @@ pub fn finalize_block() -> Header {
 		extrinsics_root,
 		state_root: storage_root,
 		parent_hash,
-		digest: digest,
+		digest,
 	}
 }
 
@@ -250,10 +250,15 @@ fn check_signature(utx: &Extrinsic) -> Result<(), TransactionValidityError> {
 fn execute_transaction_backend(utx: &Extrinsic) -> 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: true, .. } =>
+			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()),
 	}

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: false }
 }