From 7ae60056b21e9ca42c3b977af995f6f365a2072b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Thu, 17 Mar 2016 15:49:29 +0100 Subject: [PATCH] Common error handling --- ethcore/src/error.rs | 4 ++++ miner/src/lib.rs | 2 +- miner/src/miner.rs | 2 +- miner/src/transaction_queue.rs | 28 +++++++++++++++------------- rpc/src/v1/impls/eth.rs | 2 +- sync/src/chain.rs | 3 ++- 6 files changed, 24 insertions(+), 17 deletions(-) diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index 482a8de01..1b01ec9c9 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -65,6 +65,10 @@ pub enum ExecutionError { #[derive(Debug)] /// Errors concerning transaction processing. pub enum TransactionError { + /// Transaction is already imported to the queue + AlreadyImported, + /// Transaction is not valid anymore (state already has higher nonce) + Old, /// Transaction's gas price is below threshold. InsufficientGasPrice { /// Minimal expected gas price diff --git a/miner/src/lib.rs b/miner/src/lib.rs index c70caad66..b79a13e24 100644 --- a/miner/src/lib.rs +++ b/miner/src/lib.rs @@ -79,7 +79,7 @@ pub trait MinerService : Send + Sync { fn status(&self) -> MinerStatus; /// Imports transactions to transaction queue. - fn import_transactions(&self, transactions: Vec, fetch_account: T) -> Result<(), Error> + fn import_transactions(&self, transactions: Vec, fetch_account: T) -> Vec> where T: Fn(&Address) -> AccountDetails; /// Returns hashes of transactions currently in pending diff --git a/miner/src/miner.rs b/miner/src/miner.rs index 9d3613085..865bad10c 100644 --- a/miner/src/miner.rs +++ b/miner/src/miner.rs @@ -128,7 +128,7 @@ impl MinerService for Miner { } } - fn import_transactions(&self, transactions: Vec, fetch_account: T) -> Result<(), Error> + fn import_transactions(&self, transactions: Vec, fetch_account: T) -> Vec> where T: Fn(&Address) -> AccountDetails { let mut transaction_queue = self.transaction_queue.lock().unwrap(); transaction_queue.add_all(transactions, fetch_account) diff --git a/miner/src/transaction_queue.rs b/miner/src/transaction_queue.rs index a1fc20b0f..b34cee6f5 100644 --- a/miner/src/transaction_queue.rs +++ b/miner/src/transaction_queue.rs @@ -317,12 +317,12 @@ impl TransactionQueue { } /// Adds all signed transactions to queue to be verified and imported - pub fn add_all(&mut self, txs: Vec, fetch_account: T) -> Result<(), Error> + pub fn add_all(&mut self, txs: Vec, fetch_account: T) -> Vec> where T: Fn(&Address) -> AccountDetails { - for tx in txs.into_iter() { - try!(self.add(tx, &fetch_account)); - } - Ok(()) + + txs.into_iter() + .map(|tx| self.add(tx, &fetch_account)) + .collect() } /// Add signed transaction to queue to be verified and imported @@ -357,8 +357,7 @@ impl TransactionQueue { })); } - self.import_tx(vtx, account.nonce); - Ok(()) + self.import_tx(vtx, account.nonce).map_err(Error::Transaction) } /// Removes all transactions identified by hashes given in slice @@ -515,12 +514,14 @@ impl TransactionQueue { /// /// It ignores transactions that has already been imported (same `hash`) and replaces the transaction /// iff `(address, nonce)` is the same but `gas_price` is higher. - fn import_tx(&mut self, tx: VerifiedTransaction, state_nonce: U256) { + /// + /// Returns `true` when transaction was imported successfuly + fn import_tx(&mut self, tx: VerifiedTransaction, state_nonce: U256) -> Result<(), TransactionError> { if self.by_hash.get(&tx.hash()).is_some() { // Transaction is already imported. trace!(target: "miner", "Dropping already imported transaction: {:?}", tx.hash()); - return; + return Err(TransactionError::AlreadyImported); } @@ -537,11 +538,11 @@ impl TransactionQueue { // We have a gap - put to future Self::replace_transaction(tx, next_nonce, &mut self.future, &mut self.by_hash); self.future.enforce_limit(&mut self.by_hash); - return; + return Ok(()); } else if nonce < state_nonce { // Droping transaction trace!(target: "miner", "Dropping old transaction: {:?} (nonce: {} < {})", tx.hash(), nonce, next_nonce); - return; + return Err(TransactionError::Old); } Self::replace_transaction(tx, state_nonce, &mut self.current, &mut self.by_hash); @@ -551,6 +552,7 @@ impl TransactionQueue { self.current.enforce_limit(&mut self.by_hash); trace!(target: "miner", "status: {:?}", self.status()); + Ok(()) } /// Replaces transaction in given set (could be `future` or `current`). @@ -952,7 +954,7 @@ mod test { let fetch_last_nonce = |_a: &Address| AccountDetails{ nonce: last_nonce, balance: !U256::zero() }; // when - txq.add(tx, &fetch_last_nonce).unwrap(); + txq.add(tx, &fetch_last_nonce).unwrap_err(); // then let stats = txq.status(); @@ -972,7 +974,7 @@ mod test { assert_eq!(txq.status().pending, 0); // when - txq.add(tx2.clone(), &nonce).unwrap(); + txq.add(tx2.clone(), &nonce).unwrap_err(); // then let stats = txq.status(); diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 7ecfb86de..5cd1b2966 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -385,7 +385,7 @@ impl Eth for EthClient nonce: client.nonce(a), balance: client.balance(a), }); - match import { + match import.into_iter().collect::, _>>() { Ok(_) => to_value(&hash), Err(e) => { warn!("Error sending transaction: {:?}", e); diff --git a/sync/src/chain.rs b/sync/src/chain.rs index 8e8d4623c..044de613c 100644 --- a/sync/src/chain.rs +++ b/sync/src/chain.rs @@ -951,7 +951,8 @@ impl ChainSync { balance: chain.balance(a), }; let res = self.miner.import_transactions(transactions, fetch_account); - if res.is_ok() { + let any_transaction_imported = res.into_iter().any(|r| r.is_ok()); + if any_transaction_imported { self.miner.update_sealing(io.chain()); } Ok(())