diff --git a/substrate/client/consensus/manual-seal/src/lib.rs b/substrate/client/consensus/manual-seal/src/lib.rs index 3bd85b9380..7262a62d12 100644 --- a/substrate/client/consensus/manual-seal/src/lib.rs +++ b/substrate/client/consensus/manual-seal/src/lib.rs @@ -231,7 +231,7 @@ mod tests { use sc_transaction_pool::{ BasicPool, txpool::Options, - testing::*, + testing::api::*, }; use sp_transaction_pool::TransactionPool; use sp_runtime::generic::BlockId; @@ -242,10 +242,7 @@ mod tests { use sc_basic_authorship::ProposerFactory; fn api() -> TestApi { - let mut api = TestApi::default(); - api.nonce_offset = 0; - - api + TestApi::empty() } #[tokio::test] @@ -422,6 +419,7 @@ mod tests { finalize: false, }).await.unwrap(); let created_block = rx.await.unwrap().unwrap(); + pool.api().increment_nonce(Alice.into()); // assert that the background task returns ok assert_eq!( @@ -451,6 +449,7 @@ mod tests { }).await.is_ok()); assert!(rx1.await.unwrap().is_ok()); assert!(backend.blockchain().header(BlockId::Number(1)).unwrap().is_some()); + pool.api().increment_nonce(Alice.into()); assert!(pool.submit_one(&BlockId::Number(2), uxt(Alice, 2)).await.is_ok()); let (tx2, rx2) = futures::channel::oneshot::channel(); diff --git a/substrate/client/transaction-pool/src/lib.rs b/substrate/client/transaction-pool/src/lib.rs index 2771471ad9..b97294abe1 100644 --- a/substrate/client/transaction-pool/src/lib.rs +++ b/substrate/client/transaction-pool/src/lib.rs @@ -22,8 +22,6 @@ mod api; pub mod error; -#[cfg(test)] -mod tests; #[cfg(any(feature = "test-helpers", test))] pub mod testing; @@ -114,7 +112,8 @@ impl BasicPool &self.pool } - #[cfg(test)] + /// Get reference to the inner chain api, for tests only. + #[cfg(any(feature = "test-helpers", test))] pub fn api(&self) -> &Arc { &self.api } diff --git a/substrate/client/transaction-pool/src/testing.rs b/substrate/client/transaction-pool/src/testing/api.rs similarity index 50% rename from substrate/client/transaction-pool/src/testing.rs rename to substrate/client/transaction-pool/src/testing/api.rs index f4fecbe1d6..c8e4b62883 100644 --- a/substrate/client/transaction-pool/src/testing.rs +++ b/substrate/client/transaction-pool/src/testing/api.rs @@ -1,4 +1,4 @@ -// Copyright 2018-2020 Parity Technologies (UK) Ltd. +// Copyright 2020 Parity Technologies (UK) Ltd. // This file is part of Substrate. // Substrate is free software: you can redistribute it and/or modify @@ -14,56 +14,74 @@ // You should have received a copy of the GNU General Public License // along with Substrate. If not, see . -//! Utilities for testing transaction pool -use super::*; +//! Test ChainApi + +use crate::*; use codec::Encode; +use parking_lot::RwLock; use sp_runtime::{ - generic::{BlockId}, - traits::{Hash as HashT, BlakeTwo256}, + generic::{self, BlockId}, + traits::{BlakeTwo256, Hash as HashT}, transaction_validity::{TransactionValidity, ValidTransaction, TransactionValidityError, InvalidTransaction}, }; use std::collections::HashSet; -use crate::BasicPool; -use parking_lot::RwLock; -use sc_transaction_graph::{self, ExHash, Pool}; use substrate_test_runtime_client::{ - runtime::{AccountId, Block, BlockNumber, Extrinsic, Hash, Header, Index, Transfer}, - AccountKeyring, + runtime::{Index, AccountId, Block, BlockNumber, Extrinsic, Hash, Header, Transfer}, + AccountKeyring::{self, *}, }; -/// `ChainApi` for test environments +#[derive(Default)] +struct ChainState { + pub block_by_number: HashMap>, + pub block_by_hash: HashMap>, + pub header_by_number: HashMap, + pub nonces: HashMap, + pub invalid_hashes: HashSet, +} + +/// Test Api for transaction pool. pub struct TestApi { - /// transaction modifier - pub modifier: RwLock>, - /// cache of block number to extrinsics - pub chain_block_by_number: RwLock>>, - /// cache of block number to header - pub chain_headers_by_number: RwLock>, - /// cache of invalid hashes - pub invalid_hashes: RwLock>>, - /// validation requests - pub validation_requests: RwLock>, - /// used for calculating nonce for extrinsics - pub nonce_offset: u64, + valid_modifier: RwLock>, + chain: RwLock, + validation_requests: RwLock>, } impl TestApi { - /// create an instance of `TestApi` - pub fn default() -> Self { - TestApi { - modifier: RwLock::new(Box::new(|_| {})), - chain_block_by_number: RwLock::new(HashMap::new()), - invalid_hashes: RwLock::new(HashSet::new()), - chain_headers_by_number: RwLock::new(HashMap::new()), + + /// Test Api with Alice nonce set initially. + pub fn with_alice_nonce(nonce: u64) -> Self { + let api = TestApi { + valid_modifier: RwLock::new(Box::new(|_| {})), + chain: Default::default(), validation_requests: RwLock::new(Default::default()), - nonce_offset: 209, - } + }; + + api.chain.write().nonces.insert(Alice.into(), nonce); + + api } - /// add a block to `TestApi` + /// Default Test Api + pub fn empty() -> Self { + let api = TestApi { + valid_modifier: RwLock::new(Box::new(|_| {})), + chain: Default::default(), + validation_requests: RwLock::new(Default::default()), + }; + + api + } + + /// Set hook on modify valid result of transaction. + pub fn set_valid_modifier(&self, modifier: Box) { + *self.valid_modifier.write() = modifier; + } + + /// Push block as a part of canonical chain under given number. pub fn push_block(&self, block_number: BlockNumber, xts: Vec) { - self.chain_block_by_number.write().insert(block_number, xts); - self.chain_headers_by_number.write().insert(block_number, Header { + let mut chain = self.chain.write(); + chain.block_by_number.insert(block_number, xts); + chain.header_by_number.insert(block_number, Header { number: block_number, digest: Default::default(), extrinsics_root: Default::default(), @@ -71,6 +89,40 @@ impl TestApi { state_root: Default::default(), }); } + + /// Push a block without a number. + /// + /// As a part of non-canonical chain. + pub fn push_fork_block(&self, block_hash: Hash, xts: Vec) { + let mut chain = self.chain.write(); + chain.block_by_hash.insert(block_hash, xts); + } + + fn hash_and_length_inner(ex: &Extrinsic) -> (Hash, usize) { + let encoded = ex.encode(); + (BlakeTwo256::hash(&encoded), encoded.len()) + } + + /// Mark some transaction is invalid. + /// + /// Next time transaction pool will try to validate this + /// extrinsic, api will return invalid result. + pub fn add_invalid(&self, xts: &Extrinsic) { + self.chain.write().invalid_hashes.insert( + Self::hash_and_length_inner(xts).0 + ); + } + + /// Query validation requests received. + pub fn validation_requests(&self) -> Vec { + self.validation_requests.read().clone() + } + + /// Increment nonce in the inner state. + pub fn increment_nonce(&self, account: AccountId) { + let mut chain = self.chain.write(); + chain.nonces.entry(account).and_modify(|n| *n += 1).or_insert(1); + } } impl sc_transaction_graph::ChainApi for TestApi { @@ -82,21 +134,20 @@ impl sc_transaction_graph::ChainApi for TestApi { fn validate_transaction( &self, - at: &BlockId, + _at: &BlockId, uxt: sc_transaction_graph::ExtrinsicFor, ) -> Self::ValidationFuture { - self.validation_requests.write().push(uxt.clone()); - let expected = index(at, self.nonce_offset); - let requires = if expected == uxt.transfer().nonce { + let chain_nonce = self.chain.read().nonces.get(&uxt.transfer().from).cloned().unwrap_or(0); + let requires = if chain_nonce == uxt.transfer().nonce { vec![] } else { - vec![vec![uxt.transfer().nonce as u8 - 1]] + vec![vec![chain_nonce as u8]] }; let provides = vec![vec![uxt.transfer().nonce as u8]]; - if self.invalid_hashes.read().contains(&self.hash_and_length(&uxt).0) { + if self.chain.read().invalid_hashes.contains(&self.hash_and_length(&uxt).0) { return futures::future::ready(Ok( Err(TransactionValidityError::Invalid(InvalidTransaction::Custom(0))) )) @@ -110,7 +161,7 @@ impl sc_transaction_graph::ChainApi for TestApi { propagate: true, }; - (self.modifier.read())(&mut validity); + (self.valid_modifier.read())(&mut validity); futures::future::ready(Ok(Ok(validity))) } @@ -127,7 +178,7 @@ impl sc_transaction_graph::ChainApi for TestApi { at: &BlockId, ) -> error::Result>> { Ok(match at { - BlockId::Hash(x) => Some(x.clone()), + generic::BlockId::Hash(x) => Some(x.clone()), _ => Some(Default::default()), }) } @@ -136,38 +187,28 @@ impl sc_transaction_graph::ChainApi for TestApi { &self, ex: &sc_transaction_graph::ExtrinsicFor, ) -> (Self::Hash, usize) { - let encoded = ex.encode(); - (BlakeTwo256::hash(&encoded), encoded.len()) + Self::hash_and_length_inner(ex) } fn block_body(&self, id: &BlockId) -> Self::BodyFuture { futures::future::ready(Ok(if let BlockId::Number(num) = id { - self.chain_block_by_number.read().get(num).cloned() + self.chain.read().block_by_number.get(num).cloned() } else { None })) } } -/// get an instance of `BasicPool` with default options and `TestApi` -/// as the `ChainApi` -pub fn maintained_pool() -> BasicPool { - BasicPool::new(Default::default(), TestApi::default()) -} - -/// generate nonce to be used with testing TestApi -pub fn index(at: &BlockId, offset: u64) -> u64 { - offset + number_of(at) -} - fn number_of(at: &BlockId) -> u64 { match at { - BlockId::Number(n) => *n as u64, + generic::BlockId::Number(n) => *n as u64, _ => 0, } } -/// generates a transfer extrinsic, given a keyring and a nonce. +/// Generate transfer extrinsic with a given nonce. +/// +/// Part of the test api. pub fn uxt(who: AccountKeyring, nonce: Index) -> Extrinsic { let transfer = Transfer { from: who.into(), @@ -179,7 +220,3 @@ pub fn uxt(who: AccountKeyring, nonce: Index) -> Extrinsic { Extrinsic::Transfer(transfer, signature.into()) } -/// creates a transaction pool with the TestApi ChainApi -pub fn pool() -> Pool { - Pool::new(Default::default(), Arc::new(TestApi::default())) -} diff --git a/substrate/client/transaction-pool/src/testing/mod.rs b/substrate/client/transaction-pool/src/testing/mod.rs new file mode 100644 index 0000000000..c216a17ee6 --- /dev/null +++ b/substrate/client/transaction-pool/src/testing/mod.rs @@ -0,0 +1,23 @@ +// Copyright 2020 Parity Technologies (UK) Ltd. +// This file is part of Substrate. + +// Substrate is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// Substrate is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with Substrate. If not, see . + +//! Tests for top-level transaction pool api + +#[cfg(any(feature = "test-helpers", test))] +pub mod api; + +#[cfg(test)] +mod pool; \ No newline at end of file diff --git a/substrate/client/transaction-pool/src/tests.rs b/substrate/client/transaction-pool/src/testing/pool.rs similarity index 68% rename from substrate/client/transaction-pool/src/tests.rs rename to substrate/client/transaction-pool/src/testing/pool.rs index b06e335faf..30a48fbec6 100644 --- a/substrate/client/transaction-pool/src/tests.rs +++ b/substrate/client/transaction-pool/src/testing/pool.rs @@ -1,34 +1,28 @@ -// Copyright 2018-2020 Parity Technologies (UK) Ltd. -// This file is part of Substrate. - -// Substrate is free software: you can redistribute it and/or modify -// it under the terms of the GNU General Public License as published by -// the Free Software Foundation, either version 3 of the License, or -// (at your option) any later version. - -// Substrate is distributed in the hope that it will be useful, -// but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -// GNU General Public License for more details. - -// You should have received a copy of the GNU General Public License -// along with Substrate. If not, see . - -use super::*; +use crate::*; +use sc_transaction_graph::{self, Pool}; use futures::executor::block_on; -use txpool::{self, Pool}; -use substrate_test_runtime_client::{runtime::Index, AccountKeyring::*}; use sp_runtime::{ generic::BlockId, transaction_validity::ValidTransaction, }; -use testing::*; +use substrate_test_runtime_client::{ + runtime::{Block, Hash, Index}, + AccountKeyring::*, +}; +use crate::testing::api::{TestApi, uxt}; + +fn pool() -> Pool { + Pool::new(Default::default(), TestApi::with_alice_nonce(209).into()) +} + +fn maintained_pool() -> BasicPool { + BasicPool::new(Default::default(), TestApi::with_alice_nonce(209)) +} #[test] fn submission_should_work() { let pool = pool(); - assert_eq!(209, index(&BlockId::number(0), 209)); block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 209))).unwrap(); let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect(); @@ -70,13 +64,19 @@ fn late_nonce_should_be_queued() { #[test] fn prune_tags_should_work() { let pool = pool(); - block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 209))).unwrap(); + let hash209 = block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 209))).unwrap(); block_on(pool.submit_one(&BlockId::number(0), uxt(Alice, 210))).unwrap(); let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect(); assert_eq!(pending, vec![209, 210]); - block_on(pool.prune_tags(&BlockId::number(1), vec![vec![209]], vec![])).unwrap(); + block_on( + pool.prune_tags( + &BlockId::number(1), + vec![vec![209]], + vec![hash209], + ) + ).expect("Prune tags"); let pending: Vec<_> = pool.ready().map(|a| a.data.transfer().nonce).collect(); assert_eq!(pending, vec![210]); @@ -100,22 +100,24 @@ fn should_ban_invalid_transactions() { #[test] fn should_correctly_prune_transactions_providing_more_than_one_tag() { - let api = TestApi::default(); - *api.modifier.write() = Box::new(|v: &mut ValidTransaction| { + let api = Arc::new(TestApi::with_alice_nonce(209)); + api.set_valid_modifier(Box::new(|v: &mut ValidTransaction| { v.provides.push(vec![155]); - }); - let pool = Pool::new(Default::default(), Arc::new(api)); + })); + let pool = Pool::new(Default::default(), api.clone()); let xt = uxt(Alice, 209); block_on(pool.submit_one(&BlockId::number(0), xt.clone())).expect("1. Imported"); assert_eq!(pool.status().ready, 1); // remove the transaction that just got imported. + api.increment_nonce(Alice.into()); block_on(pool.prune_tags(&BlockId::number(1), vec![vec![209]], vec![])).expect("1. Pruned"); assert_eq!(pool.status().ready, 0); // it's re-imported to future assert_eq!(pool.status().future, 1); // so now let's insert another transaction that also provides the 155 + api.increment_nonce(Alice.into()); let xt = uxt(Alice, 211); block_on(pool.submit_one(&BlockId::number(2), xt.clone())).expect("2. Imported"); assert_eq!(pool.status().ready, 1); @@ -124,6 +126,7 @@ fn should_correctly_prune_transactions_providing_more_than_one_tag() { assert_eq!(pending, vec![211]); // prune it and make sure the pool is empty + api.increment_nonce(Alice.into()); block_on(pool.prune_tags(&BlockId::number(3), vec![vec![155]], vec![])).expect("2. Pruned"); assert_eq!(pool.status().ready, 0); assert_eq!(pool.status().future, 2); @@ -144,7 +147,6 @@ fn should_prune_old_during_maintenance() { assert_eq!(pool.status().ready, 0); } - #[test] fn should_revalidate_during_maintenance() { let xt1 = uxt(Alice, 209); @@ -154,12 +156,47 @@ fn should_revalidate_during_maintenance() { block_on(pool.submit_one(&BlockId::number(0), xt1.clone())).expect("1. Imported"); block_on(pool.submit_one(&BlockId::number(0), xt2.clone())).expect("2. Imported"); assert_eq!(pool.status().ready, 2); - assert_eq!(pool.api.validation_requests.read().len(), 2); + assert_eq!(pool.api.validation_requests().len(), 2); pool.api.push_block(1, vec![xt1.clone()]); block_on(pool.maintain(&BlockId::number(1), &[])); assert_eq!(pool.status().ready, 1); // test that pool revalidated transaction that left ready and not included in the block - assert_eq!(pool.api.validation_requests.read().len(), 3); + assert_eq!(pool.api.validation_requests().len(), 3); +} + +#[test] +fn should_resubmit_from_retracted_during_maintaince() { + let xt = uxt(Alice, 209); + let retracted_hash = Hash::random(); + + let pool = maintained_pool(); + + block_on(pool.submit_one(&BlockId::number(0), xt.clone())).expect("1. Imported"); + assert_eq!(pool.status().ready, 1); + + pool.api.push_block(1, vec![]); + pool.api.push_fork_block(retracted_hash, vec![xt.clone()]); + + block_on(pool.maintain(&BlockId::number(1), &[retracted_hash])); + assert_eq!(pool.status().ready, 1); +} + +#[test] +fn should_not_retain_invalid_hashes_from_retracted() { + let xt = uxt(Alice, 209); + let retracted_hash = Hash::random(); + + let pool = maintained_pool(); + + block_on(pool.submit_one(&BlockId::number(0), xt.clone())).expect("1. Imported"); + assert_eq!(pool.status().ready, 1); + + pool.api.push_block(1, vec![]); + pool.api.push_fork_block(retracted_hash, vec![xt.clone()]); + pool.api.add_invalid(&xt); + + block_on(pool.maintain(&BlockId::number(1), &[retracted_hash])); + assert_eq!(pool.status().ready, 0); }