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

@@ -797,7 +797,7 @@ impl miner::MinerService for Miner {
 	fn import_own_transaction<C: miner::BlockChainClient>(
 		&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]

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<C>(&self, chain: &C, transaction: PendingTransaction)
+	fn import_claimed_local_transaction<C>(&self, chain: &C, transaction: PendingTransaction, trusted: bool)
 		-> Result<(), transaction::Error>
 		where C: BlockChainClient;
 

rpc/src/v1/helpers/dispatch.rs

@@ -123,13 +123,13 @@ impl<C: miner::BlockChainClient, M: MinerService> FullDispatcher<C, M> {
 	}
 
 	/// Imports transaction to the miner's queue.
-	pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction) -> Result<H256> {
+	pub fn dispatch_transaction(client: &C, miner: &M, signed_transaction: PendingTransaction, trusted: bool) -> Result<H256> {
 		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<C: miner::BlockChainClient + BlockChainClient, M: MinerService> Dispatcher
 	}
 
 	fn dispatch_transaction(&self, signed_transaction: PendingTransaction) -> Result<H256> {
-		Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction)
+		Self::dispatch_transaction(&*self.client, &*self.miner, signed_transaction, true)
 	}
 }
 

rpc/src/v1/impls/eth.rs

@@ -825,6 +825,7 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient<
 					&*self.client,
 					&*self.miner,
 					signed_transaction.into(),
+					false
 				)
 			})
 			.map(Into::into)

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<C: Nonce + Sync>(&self, chain: &C, pending: PendingTransaction)
+	fn import_own_transaction<C: Nonce + Sync>(&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<C: Nonce + Sync>(&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<C: Nonce + Sync>(&self, chain: &C, pending: PendingTransaction, _trusted: bool)
 		-> Result<(), transaction::Error> {
 
 		// keep the pending nonces up to date