From 68f921b7312fc825eaa32073a961c9159ed40c28 Mon Sep 17 00:00:00 2001 From: Max Kaye Date: Sat, 16 Jun 2018 17:49:28 +1000 Subject: [PATCH] Avoid race condition from trusted sources - refactor the miner tests a bit to cut down on code reuse - add `trusted` param to dispatch_transaction and import_claimed_local_transaction --- ethcore/src/miner/miner.rs | 70 +++++++++++++---------- ethcore/src/miner/mod.rs | 2 +- rpc/src/v1/helpers/dispatch.rs | 6 +- rpc/src/v1/impls/eth.rs | 1 + rpc/src/v1/tests/helpers/miner_service.rs | 18 ++---- 5 files changed, 50 insertions(+), 47 deletions(-) diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 2b06a785b21..6563f6d3e10 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -797,7 +797,7 @@ impl miner::MinerService for Miner { fn import_own_transaction( &self, chain: &C, - pending: PendingTransaction, + pending: PendingTransaction ) -> Result<(), transaction::Error> { // note: you may want to use `import_claimed_local_transaction` instead of this one. @@ -824,10 +824,12 @@ impl miner::MinerService for Miner { &self, chain: &C, pending: PendingTransaction, + trusted: bool ) -> Result<(), transaction::Error> { // treat the tx as local if the option is enabled, or if we have the account let sender = pending.sender(); - let treat_as_local = !self.options.tx_queue_no_unfamiliar_locals + let treat_as_local = trusted + || self.options.tx_queue_no_unfamiliar_locals == false || self.accounts.as_ref().map(|accts| accts.has_account(sender)).unwrap_or(false); if treat_as_local { @@ -1218,6 +1220,34 @@ mod tests { }.sign(keypair.secret(), Some(chain_id)) } + fn post_tx_assertions( + miner: &Miner, + client: &TestBlockChainClient, + best_block: BlockNumber, + expected_pending_size: usize, // expected size for `miner.pending_transactions(best_block)` immediately after making a transaction + expect_block_to_be_sealed: bool + ) { + // By default we don't reseal on external transactions, though we do with our own transactions + assert_eq!(miner.pending_transactions(best_block).map(|txs| txs.len()).unwrap_or(0), expected_pending_size); + assert_eq!(miner.pending_receipts(best_block).map(|rs| rs.len()).unwrap_or(0), expected_pending_size); + + let client_has_best_block = client.chain_info().best_block_number >= best_block; + // If our client's best_block is >= the provided test best_block we should be able to prepare a block + if client_has_best_block { + // By default (for external transactions) we use PendingSet::AlwaysSealing, so no new transactions yet. + assert_eq!(miner.ready_transactions(client, 10, PendingOrdering::Priority).len(), expected_pending_size); + + // This method will let us know if pending block was created (before calling that method) + // This should only succeed if we expect the block has not been sealed yet + assert_eq!(miner.prepare_pending_block(client), !expect_block_to_be_sealed); + } + + if !expect_block_to_be_sealed { + // After the block is prepared one more transaction should be ready. + assert_eq!(miner.ready_transactions(client, 10, PendingOrdering::Priority).len(), expected_pending_size + 1); + } + } + #[test] fn should_make_pending_block_when_importing_own_transaction() { // given @@ -1230,11 +1260,7 @@ mod tests { // then assert_eq!(res.unwrap(), ()); - assert_eq!(miner.pending_transactions(best_block).unwrap().len(), 1); - assert_eq!(miner.pending_receipts(best_block).unwrap().len(), 1); - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); - // This method will let us know if pending block was created (before calling that method) - assert!(!miner.prepare_pending_block(&client)); + post_tx_assertions(&miner, &client, best_block, 1, true); } #[test] @@ -1249,9 +1275,7 @@ mod tests { // then assert_eq!(res.unwrap(), ()); - assert_eq!(miner.pending_transactions(best_block), None); - assert_eq!(miner.pending_receipts(best_block), None); - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); + post_tx_assertions(&miner, &client, best_block, 0, false); } #[test] @@ -1266,15 +1290,7 @@ mod tests { // then assert_eq!(res.unwrap(), ()); - // By default we don't reseal on external transactions - assert_eq!(miner.pending_transactions(best_block), None); - assert_eq!(miner.pending_receipts(best_block), None); - // By default we use PendingSet::AlwaysSealing, so no transactions yet. - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 0); - // This method will let us know if pending block was created (before calling that method) - assert!(miner.prepare_pending_block(&client)); - // After pending block is created we should see a transaction. - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); + post_tx_assertions(&miner, &client, best_block, 0, false); } #[test] @@ -1298,30 +1314,24 @@ mod tests { let best_block = 0; // when // This transaction should not be marked as local because our account_provider doesn't have the sender - let res = miner.import_claimed_local_transaction(&client, PendingTransaction::new(transaction.clone(), None)); + // and the transaction isn't trusted. + let res = miner.import_claimed_local_transaction(&client, PendingTransaction::new(transaction.clone(), None), false); // then // Check the same conditions as `should_import_external_transaction` first. Behaviour should be identical. // That is: it's treated as though we added it through `import_external_transactions` assert_eq!(res.unwrap(), ()); - assert_eq!(miner.pending_transactions(best_block), None); - assert_eq!(miner.pending_receipts(best_block), None); - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 0); - assert!(miner.prepare_pending_block(&client)); - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 1); + post_tx_assertions(&miner, &client, best_block, 0, false); // when - 2nd part: create a local transaction from account_provider. // Borrow the transaction used before & sign with our generated keypair. let local_transaction = transaction.deconstruct().0.as_unsigned().clone().sign(keypair.secret(), Some(TEST_CHAIN_ID)); - let res2 = miner.import_claimed_local_transaction(&client, PendingTransaction::new(local_transaction, None)); + let res2 = miner.import_claimed_local_transaction(&client, PendingTransaction::new(local_transaction, None), false); // then - 2nd part: we add on the results from the last pending block. // This is borrowed from `should_make_pending_block_when_importing_own_transaction` and slightly modified. assert_eq!(res2.unwrap(), ()); - assert_eq!(miner.pending_transactions(best_block).unwrap().len(), 2); - assert_eq!(miner.pending_receipts(best_block).unwrap().len(), 2); - assert_eq!(miner.ready_transactions(&client, 10, PendingOrdering::Priority).len(), 2); - assert!(!miner.prepare_pending_block(&client)); + post_tx_assertions(&miner, &client, best_block, 2, true); } #[test] diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 00d73de519f..1d7b9613bbd 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -140,7 +140,7 @@ pub trait MinerService : Send + Sync { /// Imports transactions from potentially external sources, with behaviour determined /// by the config flag `tx_queue_allow_unfamiliar_locals` - fn import_claimed_local_transaction(&self, chain: &C, transaction: PendingTransaction) + fn import_claimed_local_transaction(&self, chain: &C, transaction: PendingTransaction, trusted: bool) -> Result<(), transaction::Error> where C: BlockChainClient; diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 1ae11381733..0e6b9d443d2 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -123,13 +123,13 @@ impl FullDispatcher { } /// Imports transaction to the miner's queue. - pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction) -> Result { + pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction, trusted: bool) -> Result { let hash = signed_transaction.transaction.hash(); // use `import_claimed_local_transaction` so we can decide (based on config flags) if we want to treat // it as local or not. Nodes with public RPC interfaces will want these transactions to be treated like // external transactions. - miner.import_claimed_local_transaction(client, signed_transaction) + miner.import_claimed_local_transaction(client, signed_transaction, trusted) .map_err(errors::transaction) .map(|_| hash) } @@ -183,7 +183,7 @@ impl Dispatcher } fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result { - Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction) + Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction, true) } } diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 8de5783aa32..8423c1e2b19 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -825,6 +825,7 @@ impl Eth for EthClient< &*self.client, &*self.miner, signed_transaction.into(), + false ) }) .map(Into::into) diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index 178114727ef..63c28fb6cc4 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -155,22 +155,14 @@ impl MinerService for TestMinerService { } /// Imports transactions to transaction queue. - fn import_own_transaction(&self, chain: &C, pending: PendingTransaction) + fn import_own_transaction(&self, _chain: &C, _pending: PendingTransaction) -> Result<(), transaction::Error> { - - // keep the pending nonces up to date - let sender = pending.transaction.sender(); - let nonce = self.next_nonce(chain, &sender); - self.next_nonces.write().insert(sender, nonce); - - // lets assume that all txs are valid - self.imported_transactions.lock().push(pending.transaction); - - Ok(()) + // this function is no longer called directly from RPC + unimplemented!(); } - /// Imports transactions to queue - treats as local based on config and tx source - fn import_claimed_local_transaction(&self, chain: &C, pending: PendingTransaction) + /// Imports transactions to queue - treats as local based on trusted flag, config, and tx source + fn import_claimed_local_transaction(&self, chain: &C, pending: PendingTransaction, _trusted: bool) -> Result<(), transaction::Error> { // keep the pending nonces up to date