diff --git a/bin/node/executor/benches/bench.rs b/bin/node/executor/benches/bench.rs index 3209bb8729ab3..ef0cdf445a561 100644 --- a/bin/node/executor/benches/bench.rs +++ b/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); diff --git a/client/basic-authorship/src/basic_authorship.rs b/client/basic-authorship/src/basic_authorship.rs index 231d0255919b4..41216c2b54421 100644 --- a/client/basic-authorship/src/basic_authorship.rs +++ b/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 ProposerInner 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 ProposerInner 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(); + } } + diff --git a/client/transaction-pool/graph/benches/basics.rs b/client/transaction-pool/graph/benches/basics.rs index 54bbe930b393b..6f5d39f09f5c5 100644 --- a/client/transaction-pool/graph/benches/basics.rs +++ b/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, number: u64) { diff --git a/client/transaction-pool/graph/src/pool.rs b/client/transaction-pool/graph/src/pool.rs index 12601bb8d21be..f0bf17dcb8dd2 100644 --- a/client/transaction-pool/graph/src/pool.rs +++ b/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 { diff --git a/test-utils/runtime/src/lib.rs b/test-utils/runtime/src/lib.rs index 497de36c8b7ab..5ddeff390107a 100644 --- a/test-utils/runtime/src/lib.rs +++ b/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), - Transfer(Transfer, AccountSignature), + Transfer { + transfer: Transfer, + signature: AccountSignature, + exhaust_resources_when_not_first: bool, + }, IncludeData(Vec), StorageChange(Vec, Option>), ChangesTrieConfigUpdate(Option), @@ -130,9 +152,9 @@ impl BlindCheckable for Extrinsic { fn check(self, _signature: CheckSignature) -> Result { 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"), } } diff --git a/test-utils/runtime/src/system.rs b/test-utils/runtime/src/system.rs index 9eb501c4c89e2..4ce774598f3b0 100644 --- a/test-utils/runtime/src/system.rs +++ b/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()), } diff --git a/test-utils/runtime/transaction-pool/src/lib.rs b/test-utils/runtime/transaction-pool/src/lib.rs index aedc7dc4c3757..8cd4e58954b73 100644 --- a/test-utils/runtime/transaction-pool/src/lib.rs +++ b/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 } }