From e11353f94c98a86593c7cdd1e10fa2c9b4191f47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Fri, 13 Jan 2017 09:51:36 +0100 Subject: [PATCH] UnverifiedTransaction type (#4134) * Introducing ValidSignedTransaction * Verifiying transactions in engines * Widening use of VerifiedSignedTransaction * Renaming Transactions * Uncommenting banning queue & Fixing tests * Fixing json tests * Fixing pre-homestead test * Fixing imports * Addressing grumbles * Fixing test --- Cargo.lock | 1 - ethcore/Cargo.toml | 1 - ethcore/light/src/net/mod.rs | 6 +- ethcore/src/block.rs | 139 ++++++++------ ethcore/src/client/client.rs | 42 ++-- ethcore/src/client/test_client.rs | 6 +- ethcore/src/client/traits.rs | 2 +- ethcore/src/engines/authority_round.rs | 10 - ethcore/src/engines/basic_authority.rs | 10 - ethcore/src/engines/mod.rs | 12 +- ethcore/src/engines/tendermint/mod.rs | 10 - ethcore/src/ethereum/ethash.rs | 10 +- ethcore/src/executive.rs | 50 +---- ethcore/src/json_tests/homestead_chain.rs | 3 +- ethcore/src/json_tests/transaction.rs | 9 +- ethcore/src/lib.rs | 1 - ethcore/src/miner/banning_queue.rs | 17 +- ethcore/src/miner/miner.rs | 83 ++++---- ethcore/src/miner/mod.rs | 4 +- ethcore/src/miner/transaction_queue.rs | 147 ++++++-------- ethcore/src/snapshot/block.rs | 4 +- ethcore/src/state/mod.rs | 24 +-- ethcore/src/tests/helpers.rs | 4 +- ethcore/src/types/encoded.rs | 8 +- ethcore/src/types/transaction.rs | 223 +++++++++++++--------- ethcore/src/verification/verification.rs | 4 +- ethcore/src/views/block.rs | 10 +- ethcore/src/views/body.rs | 10 +- rpc/src/v1/helpers/dispatch.rs | 3 +- rpc/src/v1/impls/eth.rs | 12 +- rpc/src/v1/impls/parity_set.rs | 2 +- rpc/src/v1/impls/signer.rs | 5 +- rpc/src/v1/impls/traces.rs | 17 +- rpc/src/v1/tests/helpers/miner_service.rs | 14 +- rpc/src/v1/tests/mocked/eth.rs | 10 +- rpc/src/v1/tests/mocked/signing.rs | 5 +- rpc/src/v1/types/bytes.rs | 2 +- rpc/src/v1/types/transaction.rs | 14 +- sync/src/api.rs | 2 +- sync/src/chain.rs | 15 +- 40 files changed, 458 insertions(+), 493 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a015c3d7f..4f9c48dd7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -368,7 +368,6 @@ dependencies = [ "lru-cache 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "num_cpus 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)", - "rayon 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)", "rlp 0.1.0", "rust-crypto 0.2.36 (registry+https://github.com/rust-lang/crates.io-index)", "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/ethcore/Cargo.toml b/ethcore/Cargo.toml index da124073c..ed5b2a208 100644 --- a/ethcore/Cargo.toml +++ b/ethcore/Cargo.toml @@ -20,7 +20,6 @@ num_cpus = "0.2" crossbeam = "0.2.9" lazy_static = "0.2" bloomchain = "0.1" -rayon = "0.4.2" semver = "0.5" bit-set = "0.4" time = "0.1" diff --git a/ethcore/light/src/net/mod.rs b/ethcore/light/src/net/mod.rs index 384d75275..03a19e14b 100644 --- a/ethcore/light/src/net/mod.rs +++ b/ethcore/light/src/net/mod.rs @@ -19,7 +19,7 @@ //! This uses a "Provider" to answer requests. //! See https://github.com/ethcore/parity/wiki/Light-Ethereum-Subprotocol-(LES) -use ethcore::transaction::SignedTransaction; +use ethcore::transaction::UnverifiedTransaction; use ethcore::receipt::Receipt; use io::TimerToken; @@ -179,7 +179,7 @@ pub trait Handler: Send + Sync { /// Called when a peer makes an announcement. fn on_announcement(&self, _ctx: &EventContext, _announcement: &Announcement) { } /// Called when a peer requests relay of some transactions. - fn on_transactions(&self, _ctx: &EventContext, _relay: &[SignedTransaction]) { } + fn on_transactions(&self, _ctx: &EventContext, _relay: &[UnverifiedTransaction]) { } /// Called when a peer responds with block bodies. fn on_block_bodies(&self, _ctx: &EventContext, _req_id: ReqId, _bodies: &[Bytes]) { } /// Called when a peer responds with block headers. @@ -1135,7 +1135,7 @@ impl LightProtocol { let txs: Vec<_> = data.iter() .take(MAX_TRANSACTIONS) - .map(|x| x.as_val::()) + .map(|x| x.as_val::()) .collect::>()?; debug!(target: "les", "Received {} transactions to relay from peer {}", txs.len(), peer); diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index 81c32674d..4b9f1243a 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -34,7 +34,7 @@ use receipt::Receipt; use state::State; use state_db::StateDB; use trace::FlatTrace; -use transaction::SignedTransaction; +use transaction::{UnverifiedTransaction, SignedTransaction}; use verification::PreverifiedBlock; use views::BlockView; @@ -44,7 +44,7 @@ pub struct Block { /// The header of this block. pub header: Header, /// The transactions in this block. - pub transactions: Vec, + pub transactions: Vec, /// The uncles of this block. pub uncles: Vec
, } @@ -86,8 +86,9 @@ impl Decodable for Block { /// An internal type for a block's common elements. #[derive(Clone)] pub struct ExecutedBlock { - base: Block, - + header: Header, + transactions: Vec, + uncles: Vec
, receipts: Vec, transactions_set: HashSet, state: State, @@ -130,7 +131,9 @@ impl ExecutedBlock { /// Create a new block from the given `state`. fn new(state: State, tracing: bool) -> ExecutedBlock { ExecutedBlock { - base: Default::default(), + header: Default::default(), + transactions: Default::default(), + uncles: Default::default(), receipts: Default::default(), transactions_set: Default::default(), state: state, @@ -141,9 +144,9 @@ impl ExecutedBlock { /// Get a structure containing individual references to all public fields. pub fn fields_mut(&mut self) -> BlockRefMut { BlockRefMut { - header: &mut self.base.header, - transactions: &self.base.transactions, - uncles: &self.base.uncles, + header: &mut self.header, + transactions: &self.transactions, + uncles: &self.uncles, state: &mut self.state, receipts: &self.receipts, traces: &self.traces, @@ -153,9 +156,9 @@ impl ExecutedBlock { /// Get a structure containing individual references to all public fields. pub fn fields(&self) -> BlockRef { BlockRef { - header: &self.base.header, - transactions: &self.base.transactions, - uncles: &self.base.uncles, + header: &self.header, + transactions: &self.transactions, + uncles: &self.uncles, state: &self.state, receipts: &self.receipts, traces: &self.traces, @@ -169,16 +172,22 @@ pub trait IsBlock { fn block(&self) -> &ExecutedBlock; /// Get the base `Block` object associated with this. - fn base(&self) -> &Block { &self.block().base } + fn to_base(&self) -> Block { + Block { + header: self.header().clone(), + transactions: self.transactions().iter().cloned().map(Into::into).collect(), + uncles: self.uncles().to_vec(), + } + } /// Get the header associated with this object's block. - fn header(&self) -> &Header { &self.block().base.header } + fn header(&self) -> &Header { &self.block().header } /// Get the final state associated with this object's block. fn state(&self) -> &State { &self.block().state } /// Get all information on transactions in this block. - fn transactions(&self) -> &[SignedTransaction] { &self.block().base.transactions } + fn transactions(&self) -> &[SignedTransaction] { &self.block().transactions } /// Get all information on receipts in this block. fn receipts(&self) -> &[Receipt] { &self.block().receipts } @@ -187,7 +196,7 @@ pub trait IsBlock { fn traces(&self) -> &Option>> { &self.block().traces } /// Get all uncles in this block. - fn uncles(&self) -> &[Header] { &self.block().base.uncles } + fn uncles(&self) -> &[Header] { &self.block().uncles } } /// Trait for a object that has a state database. @@ -260,50 +269,50 @@ impl<'x> OpenBlock<'x> { last_hashes: last_hashes, }; - r.block.base.header.set_parent_hash(parent.hash()); - r.block.base.header.set_number(parent.number() + 1); - r.block.base.header.set_author(author); - r.block.base.header.set_timestamp_now(parent.timestamp()); - r.block.base.header.set_extra_data(extra_data); - r.block.base.header.note_dirty(); + r.block.header.set_parent_hash(parent.hash()); + r.block.header.set_number(parent.number() + 1); + r.block.header.set_author(author); + r.block.header.set_timestamp_now(parent.timestamp()); + r.block.header.set_extra_data(extra_data); + r.block.header.note_dirty(); let gas_floor_target = cmp::max(gas_range_target.0, engine.params().min_gas_limit); let gas_ceil_target = cmp::max(gas_range_target.1, gas_floor_target); - engine.populate_from_parent(&mut r.block.base.header, parent, gas_floor_target, gas_ceil_target); + engine.populate_from_parent(&mut r.block.header, parent, gas_floor_target, gas_ceil_target); engine.on_new_block(&mut r.block); Ok(r) } /// Alter the author for the block. - pub fn set_author(&mut self, author: Address) { self.block.base.header.set_author(author); } + pub fn set_author(&mut self, author: Address) { self.block.header.set_author(author); } /// Alter the timestamp of the block. - pub fn set_timestamp(&mut self, timestamp: u64) { self.block.base.header.set_timestamp(timestamp); } + pub fn set_timestamp(&mut self, timestamp: u64) { self.block.header.set_timestamp(timestamp); } /// Alter the difficulty for the block. - pub fn set_difficulty(&mut self, a: U256) { self.block.base.header.set_difficulty(a); } + pub fn set_difficulty(&mut self, a: U256) { self.block.header.set_difficulty(a); } /// Alter the gas limit for the block. - pub fn set_gas_limit(&mut self, a: U256) { self.block.base.header.set_gas_limit(a); } + pub fn set_gas_limit(&mut self, a: U256) { self.block.header.set_gas_limit(a); } /// Alter the gas limit for the block. - pub fn set_gas_used(&mut self, a: U256) { self.block.base.header.set_gas_used(a); } + pub fn set_gas_used(&mut self, a: U256) { self.block.header.set_gas_used(a); } /// Alter the uncles hash the block. - pub fn set_uncles_hash(&mut self, h: H256) { self.block.base.header.set_uncles_hash(h); } + pub fn set_uncles_hash(&mut self, h: H256) { self.block.header.set_uncles_hash(h); } /// Alter transactions root for the block. - pub fn set_transactions_root(&mut self, h: H256) { self.block.base.header.set_transactions_root(h); } + pub fn set_transactions_root(&mut self, h: H256) { self.block.header.set_transactions_root(h); } /// Alter the receipts root for the block. - pub fn set_receipts_root(&mut self, h: H256) { self.block.base.header.set_receipts_root(h); } + pub fn set_receipts_root(&mut self, h: H256) { self.block.header.set_receipts_root(h); } /// Alter the extra_data for the block. pub fn set_extra_data(&mut self, extra_data: Bytes) -> Result<(), BlockError> { if extra_data.len() > self.engine.maximum_extra_data_size() { Err(BlockError::ExtraDataOutOfBounds(OutOfBounds{min: None, max: Some(self.engine.maximum_extra_data_size()), found: extra_data.len()})) } else { - self.block.base.header.set_extra_data(extra_data); + self.block.header.set_extra_data(extra_data); Ok(()) } } @@ -313,12 +322,12 @@ impl<'x> OpenBlock<'x> { /// NOTE Will check chain constraints and the uncle number but will NOT check /// that the header itself is actually valid. pub fn push_uncle(&mut self, valid_uncle_header: Header) -> Result<(), BlockError> { - if self.block.base.uncles.len() + 1 > self.engine.maximum_uncle_count() { - return Err(BlockError::TooManyUncles(OutOfBounds{min: None, max: Some(self.engine.maximum_uncle_count()), found: self.block.base.uncles.len() + 1})); + if self.block.uncles.len() + 1 > self.engine.maximum_uncle_count() { + return Err(BlockError::TooManyUncles(OutOfBounds{min: None, max: Some(self.engine.maximum_uncle_count()), found: self.block.uncles.len() + 1})); } // TODO: check number // TODO: check not a direct ancestor (use last_hashes for that) - self.block.base.uncles.push(valid_uncle_header); + self.block.uncles.push(valid_uncle_header); Ok(()) } @@ -326,13 +335,13 @@ impl<'x> OpenBlock<'x> { pub fn env_info(&self) -> EnvInfo { // TODO: memoise. EnvInfo { - number: self.block.base.header.number(), - author: self.block.base.header.author().clone(), - timestamp: self.block.base.header.timestamp(), - difficulty: self.block.base.header.difficulty().clone(), + number: self.block.header.number(), + author: self.block.header.author().clone(), + timestamp: self.block.header.timestamp(), + difficulty: self.block.header.difficulty().clone(), last_hashes: self.last_hashes.clone(), gas_used: self.block.receipts.last().map_or(U256::zero(), |r| r.gas_used), - gas_limit: self.block.base.header.gas_limit().clone(), + gas_limit: self.block.header.gas_limit().clone(), } } @@ -349,7 +358,7 @@ impl<'x> OpenBlock<'x> { match self.block.state.apply(&env_info, self.engine, &t, self.block.traces.is_some()) { Ok(outcome) => { self.block.transactions_set.insert(h.unwrap_or_else(||t.hash())); - self.block.base.transactions.push(t); + self.block.transactions.push(t.into()); let t = outcome.trace; self.block.traces.as_mut().map(|traces| traces.push(t)); self.block.receipts.push(outcome.receipt); @@ -366,13 +375,13 @@ impl<'x> OpenBlock<'x> { let unclosed_state = s.block.state.clone(); s.engine.on_close_block(&mut s.block); - s.block.base.header.set_transactions_root(ordered_trie_root(s.block.base.transactions.iter().map(|e| e.rlp_bytes().to_vec()))); - let uncle_bytes = s.block.base.uncles.iter().fold(RlpStream::new_list(s.block.base.uncles.len()), |mut s, u| {s.append_raw(&u.rlp(Seal::With), 1); s} ).out(); - s.block.base.header.set_uncles_hash(uncle_bytes.sha3()); - s.block.base.header.set_state_root(s.block.state.root().clone()); - s.block.base.header.set_receipts_root(ordered_trie_root(s.block.receipts.iter().map(|r| r.rlp_bytes().to_vec()))); - s.block.base.header.set_log_bloom(s.block.receipts.iter().fold(LogBloom::zero(), |mut b, r| {b = &b | &r.log_bloom; b})); //TODO: use |= operator - s.block.base.header.set_gas_used(s.block.receipts.last().map_or(U256::zero(), |r| r.gas_used)); + s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes().to_vec()))); + let uncle_bytes = s.block.uncles.iter().fold(RlpStream::new_list(s.block.uncles.len()), |mut s, u| {s.append_raw(&u.rlp(Seal::With), 1); s} ).out(); + s.block.header.set_uncles_hash(uncle_bytes.sha3()); + s.block.header.set_state_root(s.block.state.root().clone()); + s.block.header.set_receipts_root(ordered_trie_root(s.block.receipts.iter().map(|r| r.rlp_bytes().to_vec()))); + s.block.header.set_log_bloom(s.block.receipts.iter().fold(LogBloom::zero(), |mut b, r| {b = &b | &r.log_bloom; b})); //TODO: use |= operator + s.block.header.set_gas_used(s.block.receipts.last().map_or(U256::zero(), |r| r.gas_used)); ClosedBlock { block: s.block, @@ -387,20 +396,20 @@ impl<'x> OpenBlock<'x> { let mut s = self; s.engine.on_close_block(&mut s.block); - if s.block.base.header.transactions_root().is_zero() || s.block.base.header.transactions_root() == &SHA3_NULL_RLP { - s.block.base.header.set_transactions_root(ordered_trie_root(s.block.base.transactions.iter().map(|e| e.rlp_bytes().to_vec()))); + if s.block.header.transactions_root().is_zero() || s.block.header.transactions_root() == &SHA3_NULL_RLP { + s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes().to_vec()))); } - let uncle_bytes = s.block.base.uncles.iter().fold(RlpStream::new_list(s.block.base.uncles.len()), |mut s, u| {s.append_raw(&u.rlp(Seal::With), 1); s} ).out(); - if s.block.base.header.uncles_hash().is_zero() { - s.block.base.header.set_uncles_hash(uncle_bytes.sha3()); + let uncle_bytes = s.block.uncles.iter().fold(RlpStream::new_list(s.block.uncles.len()), |mut s, u| {s.append_raw(&u.rlp(Seal::With), 1); s} ).out(); + if s.block.header.uncles_hash().is_zero() { + s.block.header.set_uncles_hash(uncle_bytes.sha3()); } - if s.block.base.header.receipts_root().is_zero() || s.block.base.header.receipts_root() == &SHA3_NULL_RLP { - s.block.base.header.set_receipts_root(ordered_trie_root(s.block.receipts.iter().map(|r| r.rlp_bytes().to_vec()))); + if s.block.header.receipts_root().is_zero() || s.block.header.receipts_root() == &SHA3_NULL_RLP { + s.block.header.set_receipts_root(ordered_trie_root(s.block.receipts.iter().map(|r| r.rlp_bytes().to_vec()))); } - s.block.base.header.set_state_root(s.block.state.root().clone()); - s.block.base.header.set_log_bloom(s.block.receipts.iter().fold(LogBloom::zero(), |mut b, r| {b = &b | &r.log_bloom; b})); //TODO: use |= operator - s.block.base.header.set_gas_used(s.block.receipts.last().map_or(U256::zero(), |r| r.gas_used)); + s.block.header.set_state_root(s.block.state.root().clone()); + s.block.header.set_log_bloom(s.block.receipts.iter().fold(LogBloom::zero(), |mut b, r| {b = &b | &r.log_bloom; b})); //TODO: use |= operator + s.block.header.set_gas_used(s.block.receipts.last().map_or(U256::zero(), |r| r.gas_used)); LockedBlock { block: s.block, @@ -462,7 +471,7 @@ impl LockedBlock { if seal.len() != engine.seal_fields() { return Err(BlockError::InvalidSealArity(Mismatch{expected: engine.seal_fields(), found: seal.len()})); } - s.block.base.header.set_seal(seal); + s.block.header.set_seal(seal); Ok(SealedBlock { block: s.block, uncle_bytes: s.uncle_bytes }) } @@ -471,8 +480,8 @@ impl LockedBlock { /// Returns the `ClosedBlock` back again if the seal is no good. pub fn try_seal(self, engine: &Engine, seal: Vec) -> Result { let mut s = self; - s.block.base.header.set_seal(seal); - match engine.verify_block_seal(&s.block.base.header) { + s.block.header.set_seal(seal); + match engine.verify_block_seal(&s.block.header) { Err(e) => Err((e, s)), _ => Ok(SealedBlock { block: s.block, uncle_bytes: s.uncle_bytes }), } @@ -490,8 +499,8 @@ impl SealedBlock { /// Get the RLP-encoding of the block. pub fn rlp_bytes(&self) -> Bytes { let mut block_rlp = RlpStream::new_list(3); - self.block.base.header.stream_rlp(&mut block_rlp, Seal::With); - block_rlp.append(&self.block.base.transactions); + self.block.header.stream_rlp(&mut block_rlp, Seal::With); + block_rlp.append(&self.block.transactions); block_rlp.append_raw(&self.uncle_bytes, 1); block_rlp.out() } @@ -571,6 +580,7 @@ fn push_transactions(block: &mut OpenBlock, transactions: &[SignedTransaction]) Ok(()) } +// TODO [ToDr] Pass `PreverifiedBlock` by move, this will avoid unecessary allocation /// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header #[cfg_attr(feature="dev", allow(too_many_arguments))] pub fn enact_verified( @@ -600,6 +610,7 @@ mod tests { use util::Address; use util::hash::FixedHash; use std::sync::Arc; + use transaction::SignedTransaction; /// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header #[cfg_attr(feature="dev", allow(too_many_arguments))] @@ -614,7 +625,9 @@ mod tests { ) -> Result { let block = BlockView::new(block_bytes); let header = block.header(); - enact(&header, &block.transactions(), &block.uncles(), engine, tracing, db, parent, last_hashes, factories) + let transactions: Result, Error> = block.transactions().into_iter().map(SignedTransaction::new).collect(); + let transactions = transactions?; + enact(&header, &transactions, &block.uncles(), engine, tracing, db, parent, last_hashes, factories) } /// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header. Seal the block aferwards diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index befe73ca8..20841b1fd 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -44,7 +44,7 @@ use env_info::LastHashes; use verification; use verification::{PreverifiedBlock, Verifier}; use block::*; -use transaction::{LocalizedTransaction, SignedTransaction, Transaction, PendingTransaction, Action}; +use transaction::{LocalizedTransaction, UnverifiedTransaction, SignedTransaction, Transaction, PendingTransaction, Action}; use blockchain::extras::TransactionAddress; use types::filter::Filter; use types::mode::Mode as IpcMode; @@ -596,7 +596,7 @@ impl Client { trace!(target: "external_tx", "Importing queued"); let _timer = PerfTimer::new("import_queued_transactions"); self.queue_transactions.fetch_sub(transactions.len(), AtomicOrdering::SeqCst); - let txs: Vec = transactions.iter().filter_map(|bytes| UntrustedRlp::new(bytes).as_val().ok()).collect(); + let txs: Vec = transactions.iter().filter_map(|bytes| UntrustedRlp::new(bytes).as_val().ok()).collect(); let hashes: Vec<_> = txs.iter().map(|tx| tx.hash()).collect(); self.notify(|notify| { notify.transactions_received(hashes.clone(), peer_id); @@ -854,10 +854,7 @@ impl BlockChainClient for Client { let mut state = self.state_at(block).ok_or(CallError::StatePruned)?; let original_state = if analytics.state_diffing { Some(state.clone()) } else { None }; - let sender = t.sender().map_err(|e| { - let message = format!("Transaction malformed: {:?}", e); - ExecutionError::TransactionMalformed(message) - })?; + let sender = t.sender(); let balance = state.balance(&sender); let needed_balance = t.value + t.gas * t.gas_price; if balance < needed_balance { @@ -888,16 +885,14 @@ impl BlockChainClient for Client { }; // that's just a copy of the state. let original_state = self.state_at(block).ok_or(CallError::StatePruned)?; - let sender = t.sender().map_err(|e| { - let message = format!("Transaction malformed: {:?}", e); - ExecutionError::TransactionMalformed(message) - })?; + let sender = t.sender(); let balance = original_state.balance(&sender); let options = TransactOptions { tracing: true, vm_tracing: false, check_nonce: false }; - let mut tx = t.clone(); - let mut cond = |gas| { + let cond = |gas| { + let mut tx = t.as_unsigned().clone(); tx.gas = gas; + let tx = tx.fake_sign(sender); let mut state = original_state.clone(); let needed_balance = tx.value + tx.gas * tx.gas_price; @@ -955,7 +950,7 @@ impl BlockChainClient for Client { let header = self.block_header(BlockId::Hash(address.block_hash)).ok_or(CallError::StatePruned)?; let body = self.block_body(BlockId::Hash(address.block_hash)).ok_or(CallError::StatePruned)?; let mut state = self.state_at_beginning(BlockId::Hash(address.block_hash)).ok_or(CallError::StatePruned)?; - let txs = body.transactions(); + let mut txs = body.transactions(); if address.index >= txs.len() { return Err(CallError::TransactionNotFound); @@ -972,16 +967,19 @@ impl BlockChainClient for Client { gas_used: U256::default(), gas_limit: header.gas_limit(), }; - for t in txs.iter().take(address.index) { - match Executive::new(&mut state, &env_info, &*self.engine, &self.factories.vm).transact(t, Default::default()) { + const PROOF: &'static str = "Transactions fetched from blockchain; blockchain transactions are valid; qed"; + let rest = txs.split_off(address.index); + for t in txs { + let t = SignedTransaction::new(t).expect(PROOF); + match Executive::new(&mut state, &env_info, &*self.engine, &self.factories.vm).transact(&t, Default::default()) { Ok(x) => { env_info.gas_used = env_info.gas_used + x.gas_used; } Err(ee) => { return Err(CallError::Execution(ee)) } } } - let t = &txs[address.index]; - + let first = rest.into_iter().next().expect("We split off < `address.index`; Length is checked earlier; qed"); + let t = SignedTransaction::new(first).expect(PROOF); let original_state = if analytics.state_diffing { Some(state.clone()) } else { None }; - let mut ret = Executive::new(&mut state, &env_info, &*self.engine, &self.factories.vm).transact(t, options)?; + let mut ret = Executive::new(&mut state, &env_info, &*self.engine, &self.factories.vm).transact(&t, options)?; ret.state_diff = original_state.map(|original| state.diff_from(original)); Ok(ret) @@ -1584,11 +1582,10 @@ impl Drop for Client { /// Returns `LocalizedReceipt` given `LocalizedTransaction` /// and a vector of receipts from given block up to transaction index. -fn transaction_receipt(tx: LocalizedTransaction, mut receipts: Vec) -> LocalizedReceipt { +fn transaction_receipt(mut tx: LocalizedTransaction, mut receipts: Vec) -> LocalizedReceipt { assert_eq!(receipts.len(), tx.transaction_index + 1, "All previous receipts are provided."); - let sender = tx.sender() - .expect("LocalizedTransaction is part of the blockchain; We have only valid transactions in chain; qed"); + let sender = tx.sender(); let receipt = receipts.pop().expect("Current receipt is provided; qed"); let prior_gas_used = match tx.transaction_index { 0 => 0.into(), @@ -1688,10 +1685,11 @@ mod tests { }; let tx1 = raw_tx.clone().sign(secret, None); let transaction = LocalizedTransaction { - signed: tx1.clone(), + signed: tx1.clone().into(), block_number: block_number, block_hash: block_hash, transaction_index: 1, + cached_sender: Some(tx1.sender()), }; let logs = vec![LogEntry { address: 5.into(), diff --git a/ethcore/src/client/test_client.rs b/ethcore/src/client/test_client.rs index 7568f86be..c420aa646 100644 --- a/ethcore/src/client/test_client.rs +++ b/ethcore/src/client/test_client.rs @@ -21,7 +21,7 @@ use util::*; use rlp::*; use ethkey::{Generator, Random}; use devtools::*; -use transaction::{Transaction, LocalizedTransaction, SignedTransaction, PendingTransaction, Action}; +use transaction::{Transaction, LocalizedTransaction, PendingTransaction, SignedTransaction, Action}; use blockchain::TreeRoute; use client::{ BlockChainClient, MiningBlockChainClient, EngineClient, BlockChainInfo, BlockStatus, BlockId, @@ -320,8 +320,8 @@ impl TestBlockChainClient { nonce: U256::zero() }; let signed_tx = tx.sign(keypair.secret(), None); - self.set_balance(signed_tx.sender().unwrap(), 10_000_000.into()); - let res = self.miner.import_external_transactions(self, vec![signed_tx]); + self.set_balance(signed_tx.sender(), 10_000_000.into()); + let res = self.miner.import_external_transactions(self, vec![signed_tx.into()]); let res = res.into_iter().next().unwrap().expect("Successful import"); assert_eq!(res, TransactionImportResult::Current); } diff --git a/ethcore/src/client/traits.rs b/ethcore/src/client/traits.rs index 8122aadef..0c9bd9d73 100644 --- a/ethcore/src/client/traits.rs +++ b/ethcore/src/client/traits.rs @@ -21,7 +21,7 @@ use blockchain::TreeRoute; use verification::queue::QueueInfo as BlockQueueInfo; use block::{OpenBlock, SealedBlock}; use header::{BlockNumber}; -use transaction::{LocalizedTransaction, SignedTransaction, PendingTransaction}; +use transaction::{LocalizedTransaction, PendingTransaction, SignedTransaction}; use log_entry::LocalizedLogEntry; use filter::Filter; use error::{ImportResult, CallError}; diff --git a/ethcore/src/engines/authority_round.rs b/ethcore/src/engines/authority_round.rs index adbc6ba99..1a1462507 100644 --- a/ethcore/src/engines/authority_round.rs +++ b/ethcore/src/engines/authority_round.rs @@ -33,7 +33,6 @@ use views::HeaderView; use evm::Schedule; use ethjson; use io::{IoContext, IoHandler, TimerToken, IoService}; -use transaction::SignedTransaction; use env_info::EnvInfo; use builtin::Builtin; use client::{Client, EngineClient}; @@ -312,15 +311,6 @@ impl Engine for AuthorityRound { Ok(()) } - fn verify_transaction_basic(&self, t: &SignedTransaction, _header: &Header) -> Result<(), Error> { - t.check_low_s()?; - Ok(()) - } - - fn verify_transaction(&self, t: &SignedTransaction, _header: &Header) -> Result<(), Error> { - t.sender().map(|_|()) // Perform EC recovery and cache sender - } - fn is_new_best_block(&self, _best_total_difficulty: U256, best_header: HeaderView, _parent_details: &BlockDetails, new_header: &HeaderView) -> bool { let new_number = new_header.number(); let best_number = best_header.number(); diff --git a/ethcore/src/engines/basic_authority.rs b/ethcore/src/engines/basic_authority.rs index b3ccfeb43..1d8d92557 100644 --- a/ethcore/src/engines/basic_authority.rs +++ b/ethcore/src/engines/basic_authority.rs @@ -29,7 +29,6 @@ use error::{BlockError, Error}; use evm::Schedule; use ethjson; use header::Header; -use transaction::SignedTransaction; use client::Client; use super::validator_set::{ValidatorSet, new_validator_set}; @@ -171,15 +170,6 @@ impl Engine for BasicAuthority { Ok(()) } - fn verify_transaction_basic(&self, t: &SignedTransaction, _header: &Header) -> result::Result<(), Error> { - t.check_low_s()?; - Ok(()) - } - - fn verify_transaction(&self, t: &SignedTransaction, _header: &Header) -> Result<(), Error> { - t.sender().map(|_|()) // Perform EC recovery and cache sender - } - fn register_client(&self, client: Weak) { self.validators.register_call_contract(client); } diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index b82fce1b8..85a45c3a3 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -39,7 +39,7 @@ use error::Error; use spec::CommonParams; use evm::Schedule; use header::Header; -use transaction::SignedTransaction; +use transaction::{UnverifiedTransaction, SignedTransaction}; use ethereum::ethash; use blockchain::extras::BlockDetails; use views::HeaderView; @@ -155,9 +155,15 @@ pub trait Engine : Sync + Send { /// Additional verification for transactions in blocks. // TODO: Add flags for which bits of the transaction to check. // TODO: consider including State in the params. - fn verify_transaction_basic(&self, _t: &SignedTransaction, _header: &Header) -> Result<(), Error> { Ok(()) } + fn verify_transaction_basic(&self, t: &UnverifiedTransaction, _header: &Header) -> Result<(), Error> { + t.check_low_s()?; + Ok(()) + } + /// Verify a particular transaction is valid. - fn verify_transaction(&self, _t: &SignedTransaction, _header: &Header) -> Result<(), Error> { Ok(()) } + fn verify_transaction(&self, t: UnverifiedTransaction, _header: &Header) -> Result { + SignedTransaction::new(t) + } /// The network ID that transactions should be signed with. fn signing_network_id(&self, _env_info: &EnvInfo) -> Option { None } diff --git a/ethcore/src/engines/tendermint/mod.rs b/ethcore/src/engines/tendermint/mod.rs index 9b7fa8cf6..9f44a9bcf 100644 --- a/ethcore/src/engines/tendermint/mod.rs +++ b/ethcore/src/engines/tendermint/mod.rs @@ -35,7 +35,6 @@ use error::{Error, BlockError}; use header::Header; use builtin::Builtin; use env_info::EnvInfo; -use transaction::SignedTransaction; use rlp::{UntrustedRlp, View}; use ethkey::{recover, public_to_address}; use account_provider::AccountProvider; @@ -564,15 +563,6 @@ impl Engine for Tendermint { Ok(()) } - fn verify_transaction_basic(&self, t: &SignedTransaction, _header: &Header) -> Result<(), Error> { - t.check_low_s()?; - Ok(()) - } - - fn verify_transaction(&self, t: &SignedTransaction, _header: &Header) -> Result<(), Error> { - t.sender().map(|_|()) // Perform EC recovery and cache sender - } - fn set_signer(&self, address: Address, password: String) { *self.authority.write() = address; *self.password.write() = Some(password); diff --git a/ethcore/src/ethereum/ethash.rs b/ethcore/src/ethereum/ethash.rs index 909480570..e860c06b5 100644 --- a/ethcore/src/ethereum/ethash.rs +++ b/ethcore/src/ethereum/ethash.rs @@ -24,7 +24,7 @@ use header::Header; use views::HeaderView; use state::CleanupMode; use spec::CommonParams; -use transaction::SignedTransaction; +use transaction::UnverifiedTransaction; use engines::Engine; use evm::Schedule; use ethjson; @@ -185,7 +185,7 @@ impl Engine for Ethash { } else if gas_limit > gas_ceil_target { max(gas_ceil_target, lower_limit) } else { - max(gas_floor_target, min(min(gas_ceil_target, upper_limit), + max(gas_floor_target, min(min(gas_ceil_target, upper_limit), lower_limit + (header.gas_used().clone() * 6.into() / 5.into()) / bound_divisor)) } }; @@ -310,7 +310,7 @@ impl Engine for Ethash { Ok(()) } - fn verify_transaction_basic(&self, t: &SignedTransaction, header: &Header) -> result::Result<(), Error> { + fn verify_transaction_basic(&self, t: &UnverifiedTransaction, header: &Header) -> result::Result<(), Error> { if header.number() >= self.ethash_params.homestead_transition { t.check_low_s()?; } @@ -324,10 +324,6 @@ impl Engine for Ethash { Ok(()) } - fn verify_transaction(&self, t: &SignedTransaction, _header: &Header) -> Result<(), Error> { - t.sender().map(|_|()) // Perform EC recovery and cache sender - } - fn is_new_best_block(&self, best_total_difficulty: U256, _best_header: HeaderView, parent_details: &BlockDetails, new_header: &HeaderView) -> bool { is_new_best_block(best_total_difficulty, parent_details, new_header) } diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index a8c9596ab..18ad92aef 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -122,10 +122,7 @@ impl<'a> Executive<'a> { mut tracer: T, mut vm_tracer: V ) -> Result where T: Tracer, V: VMTracer { - let sender = t.sender().map_err(|e| { - let message = format!("Transaction malformed: {:?}", e); - ExecutionError::TransactionMalformed(message) - })?; + let sender = t.sender(); let nonce = self.state.nonce(&sender); let schedule = self.engine.schedule(self.info); @@ -435,14 +432,7 @@ impl<'a> Executive<'a> { trace!("exec::finalize: t.gas={}, sstore_refunds={}, suicide_refunds={}, refunds_bound={}, gas_left_prerefund={}, refunded={}, gas_left={}, gas_used={}, refund_value={}, fees_value={}\n", t.gas, sstore_refunds, suicide_refunds, refunds_bound, gas_left_prerefund, refunded, gas_left, gas_used, refund_value, fees_value); - let sender = match t.sender() { - Ok(sender) => sender, - Err(e) => { - debug!(target: "executive", "attempted to finalize transaction without sender: {}", e); - return Err(ExecutionError::Internal); - } - }; - + let sender = t.sender(); trace!("exec::finalize: Refunding refund_value={}, sender={}\n", refund_value, sender); // Below: NoEmpty is safe since the sender must already be non-null to have sent this transaction self.state.add_balance(&sender, &refund_value, CleanupMode::NoEmpty); @@ -1057,7 +1047,7 @@ mod tests { gas_price: U256::zero(), nonce: U256::zero() }.sign(keypair.secret(), None); - let sender = t.sender().unwrap(); + let sender = t.sender(); let contract = contract_address(&sender, &U256::zero()); let mut state_result = get_temp_state(); @@ -1085,34 +1075,6 @@ mod tests { assert_eq!(state.storage_at(&contract, &H256::new()), H256::from(&U256::from(1))); } - evm_test!{test_transact_invalid_sender: test_transact_invalid_sender_jit, test_transact_invalid_sender_int} - fn test_transact_invalid_sender(factory: Factory) { - let t = Transaction { - action: Action::Create, - value: U256::from(17), - data: "3331600055".from_hex().unwrap(), - gas: U256::from(100_000), - gas_price: U256::zero(), - nonce: U256::zero() - }.invalid_sign(); - let mut state_result = get_temp_state(); - let mut state = state_result.reference_mut(); - let mut info = EnvInfo::default(); - info.gas_limit = U256::from(100_000); - let engine = TestEngine::new(0); - - let res = { - let mut ex = Executive::new(&mut state, &info, &engine, &factory); - let opts = TransactOptions { check_nonce: true, tracing: false, vm_tracing: false }; - ex.transact(&t, opts) - }; - - match res { - Err(ExecutionError::TransactionMalformed(_)) => (), - _ => assert!(false, "Expected an invalid transaction error.") - } - } - evm_test!{test_transact_invalid_nonce: test_transact_invalid_nonce_jit, test_transact_invalid_nonce_int} fn test_transact_invalid_nonce(factory: Factory) { let keypair = Random.generate().unwrap(); @@ -1124,7 +1086,7 @@ mod tests { gas_price: U256::zero(), nonce: U256::one() }.sign(keypair.secret(), None); - let sender = t.sender().unwrap(); + let sender = t.sender(); let mut state_result = get_temp_state(); let mut state = state_result.reference_mut(); @@ -1157,7 +1119,7 @@ mod tests { gas_price: U256::zero(), nonce: U256::zero() }.sign(keypair.secret(), None); - let sender = t.sender().unwrap(); + let sender = t.sender(); let mut state_result = get_temp_state(); let mut state = state_result.reference_mut(); @@ -1192,7 +1154,7 @@ mod tests { gas_price: U256::one(), nonce: U256::zero() }.sign(keypair.secret(), None); - let sender = t.sender().unwrap(); + let sender = t.sender(); let mut state_result = get_temp_state(); let mut state = state_result.reference_mut(); diff --git a/ethcore/src/json_tests/homestead_chain.rs b/ethcore/src/json_tests/homestead_chain.rs index 7a4fe2121..c1c128373 100644 --- a/ethcore/src/json_tests/homestead_chain.rs +++ b/ethcore/src/json_tests/homestead_chain.rs @@ -26,8 +26,7 @@ declare_test!{BlockchainTests_Homestead_bcBlockGasLimitTest, "BlockchainTests/Ho declare_test!{BlockchainTests_Homestead_bcForkStressTest, "BlockchainTests/Homestead/bcForkStressTest"} declare_test!{BlockchainTests_Homestead_bcGasPricerTest, "BlockchainTests/Homestead/bcGasPricerTest"} declare_test!{BlockchainTests_Homestead_bcInvalidHeaderTest, "BlockchainTests/Homestead/bcInvalidHeaderTest"} -// TODO [ToDr] Ignored because of incorrect JSON (https://github.com/ethereum/tests/pull/113) -declare_test!{ignore => BlockchainTests_Homestead_bcInvalidRLPTest, "BlockchainTests/Homestead/bcInvalidRLPTest"} +declare_test!{BlockchainTests_Homestead_bcInvalidRLPTest, "BlockchainTests/Homestead/bcInvalidRLPTest"} declare_test!{BlockchainTests_Homestead_bcMultiChainTest, "BlockchainTests/Homestead/bcMultiChainTest"} declare_test!{BlockchainTests_Homestead_bcRPC_API_Test, "BlockchainTests/Homestead/bcRPC_API_Test"} declare_test!{BlockchainTests_Homestead_bcStateTest, "BlockchainTests/Homestead/bcStateTest"} diff --git a/ethcore/src/json_tests/transaction.rs b/ethcore/src/json_tests/transaction.rs index 7f8731343..9c731133a 100644 --- a/ethcore/src/json_tests/transaction.rs +++ b/ethcore/src/json_tests/transaction.rs @@ -18,7 +18,8 @@ use super::test_common::*; use evm; use ethjson; use rlp::{UntrustedRlp, View}; -use transaction::{Action, SignedTransaction}; +use transaction::{Action, UnverifiedTransaction}; +use ethstore::ethkey::public_to_address; fn do_json_test(json_data: &[u8]) -> Vec { let tests = ethjson::transaction::Test::load(json_data).unwrap(); @@ -34,18 +35,18 @@ fn do_json_test(json_data: &[u8]) -> Vec { Some(x) if x < 1_150_000 => &old_schedule, Some(_) => &new_schedule }; - let allow_network_id_of_one = number.map_or(false, |n| n >= 2_675_000); + let allow_network_id_of_one = number.map_or(false, |n| n >= 2_675_000); let rlp: Vec = test.rlp.into(); let res = UntrustedRlp::new(&rlp) .as_val() .map_err(From::from) - .and_then(|t: SignedTransaction| t.validate(schedule, schedule.have_delegate_call, allow_network_id_of_one)); + .and_then(|t: UnverifiedTransaction| t.validate(schedule, schedule.have_delegate_call, allow_network_id_of_one)); fail_unless(test.transaction.is_none() == res.is_err(), "Validity different"); if let (Some(tx), Some(sender)) = (test.transaction, test.sender) { let t = res.unwrap(); - fail_unless(t.sender().unwrap() == sender.into(), "sender mismatch"); + fail_unless(public_to_address(&t.recover_public().unwrap()) == sender.into(), "sender mismatch"); let is_acceptable_network_id = match t.network_id() { None => true, Some(1) if allow_network_id_of_one => true, diff --git a/ethcore/src/lib.rs b/ethcore/src/lib.rs index df85ca4c7..76545ff78 100644 --- a/ethcore/src/lib.rs +++ b/ethcore/src/lib.rs @@ -89,7 +89,6 @@ extern crate num_cpus; extern crate crossbeam; extern crate ethjson; extern crate bloomchain; -extern crate rayon; extern crate hyper; extern crate ethash; extern crate ethkey; diff --git a/ethcore/src/miner/banning_queue.rs b/ethcore/src/miner/banning_queue.rs index 7d5f5c299..0d41cfa0a 100644 --- a/ethcore/src/miner/banning_queue.rs +++ b/ethcore/src/miner/banning_queue.rs @@ -90,12 +90,11 @@ impl BanningTransactionQueue { // NOTE In all checks use direct query to avoid increasing ban timeout. // Check sender - if let Ok(sender) = transaction.sender() { - let count = self.senders_bans.direct().get(&sender).map(|v| v.get()).unwrap_or(0); - if count > threshold { - debug!(target: "txqueue", "Ignoring transaction {:?} because sender is banned.", transaction.hash()); - return Err(Error::Transaction(TransactionError::SenderBanned)); - } + let sender = transaction.sender(); + let count = self.senders_bans.direct().get(&sender).map(|v| v.get()).unwrap_or(0); + if count > threshold { + debug!(target: "txqueue", "Ignoring transaction {:?} because sender is banned.", transaction.hash()); + return Err(Error::Transaction(TransactionError::SenderBanned)); } // Check recipient @@ -128,7 +127,7 @@ impl BanningTransactionQueue { let transaction = self.queue.find(hash); match transaction { Some(transaction) => { - let sender = transaction.sender().expect("Transaction is in queue, so the sender is already validated; qed"); + let sender = transaction.sender(); // Ban sender let sender_banned = self.ban_sender(sender); // Ban recipient and codehash @@ -278,14 +277,14 @@ mod tests { let tx = transaction(Action::Create); let mut txq = queue(); // Banlist once (threshold not reached) - let banlist1 = txq.ban_sender(tx.sender().unwrap()); + let banlist1 = txq.ban_sender(tx.sender()); assert!(!banlist1, "Threshold not reached yet."); // Insert once let import1 = txq.add_with_banlist(tx.clone(), 0, &default_account_details, &gas_required).unwrap(); assert_eq!(import1, TransactionImportResult::Current); // when - let banlist2 = txq.ban_sender(tx.sender().unwrap()); + let banlist2 = txq.ban_sender(tx.sender()); let import2 = txq.add_with_banlist(tx.clone(), 0, &default_account_details, &gas_required); // then diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index 59237ed6a..2235e278f 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -14,7 +14,6 @@ // You should have received a copy of the GNU General Public License // along with Parity. If not, see . -use rayon::prelude::*; use std::time::{Instant, Duration}; use util::*; @@ -26,7 +25,7 @@ use client::TransactionImportResult; use executive::contract_address; use block::{ClosedBlock, IsBlock, Block}; use error::*; -use transaction::{Action, SignedTransaction, PendingTransaction}; +use transaction::{Action, UnverifiedTransaction, PendingTransaction, SignedTransaction}; use receipt::{Receipt, RichReceipt}; use spec::Spec; use engines::{Engine, Seal}; @@ -299,9 +298,9 @@ impl Miner { self.sealing_work.lock().queue.peek_last_ref().map(|b| b.block().fields().state.clone()) } - /// Get `Some` `clone()` of the current pending block's state or `None` if we're not sealing. + /// Get `Some` `clone()` of the current pending block or `None` if we're not sealing. pub fn pending_block(&self) -> Option { - self.sealing_work.lock().queue.peek_last_ref().map(|b| b.base().clone()) + self.sealing_work.lock().queue.peek_last_ref().map(|b| b.to_base()) } #[cfg_attr(feature="dev", allow(match_same_arms))] @@ -576,11 +575,11 @@ impl Miner { fn add_transactions_to_queue( &self, chain: &MiningBlockChainClient, - transactions: Vec, + transactions: Vec, default_origin: TransactionOrigin, min_block: Option, - transaction_queue: &mut BanningTransactionQueue) - -> Vec> { + transaction_queue: &mut BanningTransactionQueue, + ) -> Vec> { let fetch_account = |a: &Address| AccountDetails { nonce: chain.latest_nonce(a), @@ -598,29 +597,32 @@ impl Miner { transactions.into_iter() .map(|tx| { - if chain.transaction_block(TransactionId::Hash(tx.hash())).is_some() { - debug!(target: "miner", "Rejected tx {:?}: already in the blockchain", tx.hash()); + let hash = tx.hash(); + if chain.transaction_block(TransactionId::Hash(hash)).is_some() { + debug!(target: "miner", "Rejected tx {:?}: already in the blockchain", hash); return Err(Error::Transaction(TransactionError::AlreadyImported)); } - match self.engine.verify_transaction_basic(&tx, &best_block_header) { + match self.engine.verify_transaction_basic(&tx, &best_block_header) + .and_then(|_| self.engine.verify_transaction(tx, &best_block_header)) + { Err(e) => { - debug!(target: "miner", "Rejected tx {:?} with invalid signature: {:?}", tx.hash(), e); + debug!(target: "miner", "Rejected tx {:?} with invalid signature: {:?}", hash, e); Err(e) }, - Ok(()) => { + Ok(transaction) => { let origin = accounts.as_ref().and_then(|accounts| { - tx.sender().ok().and_then(|sender| match accounts.contains(&sender) { + match accounts.contains(&transaction.sender()) { true => Some(TransactionOrigin::Local), false => None, - }) + } }).unwrap_or(default_origin); match origin { TransactionOrigin::Local | TransactionOrigin::RetractedBlock => { - transaction_queue.add(tx, origin, insertion_time, min_block, &fetch_account, &gas_required) + transaction_queue.add(transaction, origin, insertion_time, min_block, &fetch_account, &gas_required) }, TransactionOrigin::External => { - transaction_queue.add_with_banlist(tx, insertion_time, &fetch_account, &gas_required) + transaction_queue.add_with_banlist(transaction, insertion_time, &fetch_account, &gas_required) } } }, @@ -695,10 +697,7 @@ impl MinerService for Miner { let mut state = block.state().clone(); let original_state = if analytics.state_diffing { Some(state.clone()) } else { None }; - let sender = t.sender().map_err(|e| { - let message = format!("Transaction malformed: {:?}", e); - ExecutionError::TransactionMalformed(message) - })?; + let sender = t.sender(); let balance = state.balance(&sender); let needed_balance = t.value + t.gas * t.gas_price; if balance < needed_balance { @@ -843,7 +842,7 @@ impl MinerService for Miner { fn import_external_transactions( &self, chain: &MiningBlockChainClient, - transactions: Vec + transactions: Vec ) -> Vec> { trace!(target: "external_tx", "Importing external transactions"); let results = { @@ -876,8 +875,9 @@ impl MinerService for Miner { let imported = { // Be sure to release the lock before we call prepare_work_sealing let mut transaction_queue = self.transaction_queue.lock(); + // We need to re-validate transactions let import = self.add_transactions_to_queue( - chain, vec![pending.transaction], TransactionOrigin::Local, pending.min_block, &mut transaction_queue + chain, vec![pending.transaction.into()], TransactionOrigin::Local, pending.min_block, &mut transaction_queue ).pop().expect("one result returned per added transaction; one added => one result; qed"); match import { @@ -1013,8 +1013,7 @@ impl MinerService for Miner { contract_address: match tx.action { Action::Call(_) => None, Action::Create => { - let sender = tx.sender() - .expect("transactions in pending block have already been checked for valid sender; qed"); + let sender = tx.sender(); Some(contract_address(&sender, &tx.nonce)) } }, @@ -1126,22 +1125,16 @@ impl MinerService for Miner { // Then import all transactions... { - retracted.par_iter() - .map(|hash| { - let block = chain.block(BlockId::Hash(*hash)) - .expect("Client is sending message after commit to db and inserting to chain; the block is available; qed"); - let txs = block.transactions(); - // populate sender - for tx in &txs { - let _sender = tx.sender(); - } - txs - }).for_each(|txs| { - let mut transaction_queue = self.transaction_queue.lock(); - let _ = self.add_transactions_to_queue( - chain, txs, TransactionOrigin::RetractedBlock, None, &mut transaction_queue - ); - }); + + let mut transaction_queue = self.transaction_queue.lock(); + for hash in retracted { + let block = chain.block(BlockId::Hash(*hash)) + .expect("Client is sending message after commit to db and inserting to chain; the block is available; qed"); + let txs = block.transactions(); + let _ = self.add_transactions_to_queue( + chain, txs, TransactionOrigin::RetractedBlock, None, &mut transaction_queue + ); + } } // ...and at the end remove the old ones @@ -1177,7 +1170,7 @@ mod tests { use ethkey::{Generator, Random}; use client::{BlockChainClient, TestBlockChainClient, EachBlockWith, TransactionImportResult}; use header::BlockNumber; - use types::transaction::{Transaction, SignedTransaction, PendingTransaction, Action}; + use types::transaction::{SignedTransaction, Transaction, PendingTransaction, Action}; use spec::Spec; use tests::helpers::{generate_dummy_client}; @@ -1291,7 +1284,7 @@ mod tests { // given let client = TestBlockChainClient::default(); let miner = miner(); - let transaction = transaction(); + let transaction = transaction().into(); let best_block = 0; // when let res = miner.import_external_transactions(&client, vec![transaction]).pop().unwrap(); @@ -1313,7 +1306,7 @@ mod tests { // By default resealing is not required. assert!(!miner.requires_reseal(1u8.into())); - miner.import_external_transactions(&client, vec![transaction()]).pop().unwrap().unwrap(); + miner.import_external_transactions(&client, vec![transaction().into()]).pop().unwrap().unwrap(); assert!(miner.prepare_work_sealing(&client)); // Unless asked to prepare work. assert!(miner.requires_reseal(1u8.into())); @@ -1326,14 +1319,14 @@ mod tests { let c = generate_dummy_client(2); let client = c.reference().as_ref(); - assert_eq!(miner.import_external_transactions(client, vec![transaction()]).pop().unwrap().unwrap(), TransactionImportResult::Current); + assert_eq!(miner.import_external_transactions(client, vec![transaction().into()]).pop().unwrap().unwrap(), TransactionImportResult::Current); miner.update_sealing(client); client.flush_queue(); assert!(miner.pending_block().is_none()); assert_eq!(client.chain_info().best_block_number, 3 as BlockNumber); - assert_eq!(miner.import_own_transaction(client, PendingTransaction::new(transaction(), None)).unwrap(), TransactionImportResult::Current); + assert_eq!(miner.import_own_transaction(client, PendingTransaction::new(transaction().into(), None)).unwrap(), TransactionImportResult::Current); miner.update_sealing(client); client.flush_queue(); diff --git a/ethcore/src/miner/mod.rs b/ethcore/src/miner/mod.rs index 563e068a6..741224cc9 100644 --- a/ethcore/src/miner/mod.rs +++ b/ethcore/src/miner/mod.rs @@ -62,7 +62,7 @@ use block::ClosedBlock; use header::BlockNumber; use receipt::{RichReceipt, Receipt}; use error::{Error, CallError}; -use transaction::{SignedTransaction, PendingTransaction}; +use transaction::{UnverifiedTransaction, PendingTransaction, SignedTransaction}; /// Miner client API pub trait MinerService : Send + Sync { @@ -114,7 +114,7 @@ pub trait MinerService : Send + Sync { fn set_tx_gas_limit(&self, limit: U256); /// Imports transactions to transaction queue. - fn import_external_transactions(&self, chain: &MiningBlockChainClient, transactions: Vec) -> + fn import_external_transactions(&self, chain: &MiningBlockChainClient, transactions: Vec) -> Vec>; /// Imports own (node owner) transaction to queue. diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index fe3af79dc..be32caa99 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -251,7 +251,7 @@ impl Ord for TransactionOrder { } } -/// Verified transaction (with sender) +/// Verified transaction #[derive(Debug)] struct VerifiedTransaction { /// Transaction. @@ -265,14 +265,13 @@ struct VerifiedTransaction { } impl VerifiedTransaction { - fn new(transaction: SignedTransaction, origin: TransactionOrigin, time: QueuingInstant, min_block: Option) -> Result { - transaction.sender()?; - Ok(VerifiedTransaction { + fn new(transaction: SignedTransaction, origin: TransactionOrigin, time: QueuingInstant, min_block: Option) -> Self { + VerifiedTransaction { transaction: transaction, origin: origin, insertion_time: time, min_block: min_block, - }) + } } fn hash(&self) -> H256 { @@ -284,7 +283,7 @@ impl VerifiedTransaction { } fn sender(&self) -> Address { - self.transaction.sender().expect("Sender is verified in new; qed") + self.transaction.sender() } fn cost(&self) -> U256 { @@ -663,7 +662,7 @@ impl TransactionQueue { Err(Error::Transaction(ref err)) => { // Sometimes transactions are re-imported, so // don't overwrite transactions if they are already on the list - if !self.local_transactions.contains(&cloned_tx.hash()) { + if !self.local_transactions.contains(&hash) { self.local_transactions.mark_rejected(cloned_tx, err.clone()); } }, @@ -749,17 +748,12 @@ impl TransactionQueue { })); } - // Verify signature - tx.check_low_s()?; - - let vtx = VerifiedTransaction::new(tx, origin, time, min_block)?; - let client_account = fetch_account(&vtx.sender()); - - let cost = vtx.cost(); + let client_account = fetch_account(&tx.sender()); + let cost = tx.value + tx.gas_price * tx.gas; if client_account.balance < cost { trace!(target: "txqueue", "Dropping transaction without sufficient balance: {:?} ({} < {})", - vtx.hash(), + tx.hash(), client_account.balance, cost ); @@ -769,7 +763,9 @@ impl TransactionQueue { balance: client_account.balance })); } - + tx.check_low_s()?; + // No invalid transactions beyond this point. + let vtx = VerifiedTransaction::new(tx, origin, time, min_block); let r = self.import_tx(vtx, client_account.nonce).map_err(Error::Transaction); assert_eq!(self.future.by_priority.len() + self.current.by_priority.len(), self.by_hash.len()); r @@ -918,7 +914,7 @@ impl TransactionQueue { // Mark in locals if self.local_transactions.contains(transaction_hash) { - self.local_transactions.mark_invalid(transaction.transaction.clone()); + self.local_transactions.mark_invalid(transaction.transaction.into()); } // Remove from future @@ -1391,7 +1387,7 @@ mod test { let keypair = Random.generate().unwrap(); let secret = &keypair.secret(); - (tx1.sign(secret, None), tx2.sign(secret, None)) + (tx1.sign(secret, None).into(), tx2.sign(secret, None).into()) } /// Returns two consecutive transactions, both with increased gas price @@ -1402,7 +1398,7 @@ mod test { let keypair = Random.generate().unwrap(); let secret = &keypair.secret(); - (tx1.sign(secret, None), tx2.sign(secret, None)) + (tx1.sign(secret, None).into(), tx2.sign(secret, None).into()) } fn new_tx_pair_default(nonce_increment: U256, gas_price_increment: U256) -> (SignedTransaction, SignedTransaction) { @@ -1434,7 +1430,7 @@ mod test { // given let mut txq = TransactionQueue::with_limits(PrioritizationStrategy::GasPriceOnly, 2, !U256::zero(), !U256::zero()); let (tx1, tx2) = new_tx_pair(123.into(), 1.into(), 1.into(), 0.into()); - let sender = tx1.sender().unwrap(); + let sender = tx1.sender(); let nonce = tx1.nonce; txq.add(tx1.clone(), TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator).unwrap(); txq.add(tx2.clone(), TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator).unwrap(); @@ -1475,12 +1471,12 @@ mod test { gas_limit: !U256::zero(), }; let (tx1, tx2) = new_tx_pair_default(1.into(), 0.into()); - let tx1 = VerifiedTransaction::new(tx1, TransactionOrigin::External, 0, None).unwrap(); - let tx2 = VerifiedTransaction::new(tx2, TransactionOrigin::External, 0, None).unwrap(); + let tx1 = VerifiedTransaction::new(tx1, TransactionOrigin::External, 0, None); + let tx2 = VerifiedTransaction::new(tx2, TransactionOrigin::External, 0, None); let mut by_hash = { let mut x = HashMap::new(); - let tx1 = VerifiedTransaction::new(tx1.transaction.clone(), TransactionOrigin::External, 0, None).unwrap(); - let tx2 = VerifiedTransaction::new(tx2.transaction.clone(), TransactionOrigin::External, 0, None).unwrap(); + let tx1 = VerifiedTransaction::new(tx1.transaction.clone(), TransactionOrigin::External, 0, None); + let tx2 = VerifiedTransaction::new(tx2.transaction.clone(), TransactionOrigin::External, 0, None); x.insert(tx1.hash(), tx1); x.insert(tx2.hash(), tx2); x @@ -1518,12 +1514,12 @@ mod test { // Create two transactions with same nonce // (same hash) let (tx1, tx2) = new_tx_pair_default(0.into(), 0.into()); - let tx1 = VerifiedTransaction::new(tx1, TransactionOrigin::External, 0, None).unwrap(); - let tx2 = VerifiedTransaction::new(tx2, TransactionOrigin::External, 0, None).unwrap(); + let tx1 = VerifiedTransaction::new(tx1, TransactionOrigin::External, 0, None); + let tx2 = VerifiedTransaction::new(tx2, TransactionOrigin::External, 0, None); let by_hash = { let mut x = HashMap::new(); - let tx1 = VerifiedTransaction::new(tx1.transaction.clone(), TransactionOrigin::External, 0, None).unwrap(); - let tx2 = VerifiedTransaction::new(tx2.transaction.clone(), TransactionOrigin::External, 0, None).unwrap(); + let tx1 = VerifiedTransaction::new(tx1.transaction.clone(), TransactionOrigin::External, 0, None); + let tx2 = VerifiedTransaction::new(tx2.transaction.clone(), TransactionOrigin::External, 0, None); x.insert(tx1.hash(), tx1); x.insert(tx2.hash(), tx2); x @@ -1565,10 +1561,10 @@ mod test { gas_limit: !U256::zero(), }; let tx = new_tx_default(); - let tx1 = VerifiedTransaction::new(tx.clone(), TransactionOrigin::External, 0, None).unwrap(); + let tx1 = VerifiedTransaction::new(tx.clone(), TransactionOrigin::External, 0, None); let order1 = TransactionOrder::for_transaction(&tx1, 0.into(), 1.into(), PrioritizationStrategy::GasPriceOnly); assert!(set.insert(tx1.sender(), tx1.nonce(), order1).is_none()); - let tx2 = VerifiedTransaction::new(tx, TransactionOrigin::External, 0, None).unwrap(); + let tx2 = VerifiedTransaction::new(tx, TransactionOrigin::External, 0, None); let order2 = TransactionOrder::for_transaction(&tx2, 0.into(), 1.into(), PrioritizationStrategy::GasPriceOnly); assert!(set.insert(tx2.sender(), tx2.nonce(), order2).is_some()); } @@ -1585,7 +1581,7 @@ mod test { assert_eq!(set.gas_price_entry_limit(), 0.into()); let tx = new_tx_default(); - let tx1 = VerifiedTransaction::new(tx.clone(), TransactionOrigin::External, 0, None).unwrap(); + let tx1 = VerifiedTransaction::new(tx.clone(), TransactionOrigin::External, 0, None); let order1 = TransactionOrder::for_transaction(&tx1, 0.into(), 1.into(), PrioritizationStrategy::GasPriceOnly); assert!(set.insert(tx1.sender(), tx1.nonce(), order1.clone()).is_none()); assert_eq!(set.gas_price_entry_limit(), 2.into()); @@ -1613,7 +1609,8 @@ mod test { assert_eq!(txq.status().future, 0); assert_eq!(txq.current.by_priority.len(), 1); assert_eq!(txq.current.by_address.len(), 1); - assert_eq!(txq.top_transactions()[0], tx2); + let top = txq.top_transactions(); + assert_eq!(top[0], tx2); } #[test] @@ -1638,8 +1635,9 @@ mod test { assert_eq!(txq.status().future, 0); assert_eq!(txq.current.by_priority.len(), 2); assert_eq!(txq.current.by_address.len(), 2); - assert_eq!(txq.top_transactions()[0], tx); - assert_eq!(txq.top_transactions()[1], tx2); + let top = txq.top_transactions(); + assert_eq!(top[0], tx); + assert_eq!(top[1], tx2); } #[test] @@ -1821,33 +1819,6 @@ mod test { assert_eq!(stats.future, 0); } - #[test] - fn should_reject_incorectly_signed_transaction() { - use rlp::{self, RlpStream, Stream}; - - // given - let mut txq = TransactionQueue::default(); - let tx = new_unsigned_tx(123.into(), 100.into(), 1.into()); - let stx = { - let mut s = RlpStream::new_list(9); - s.append(&tx.nonce); - s.append(&tx.gas_price); - s.append(&tx.gas); - s.append_empty_data(); // action=create - s.append(&tx.value); - s.append(&tx.data); - s.append(&0u64); // v - s.append(&U256::zero()); // r - s.append(&U256::zero()); // s - rlp::decode(s.as_raw()) - }; - // when - let res = txq.add(stx, TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator); - - // then - assert!(res.is_err()); - } - #[test] fn should_import_txs_from_same_sender() { // given @@ -1898,8 +1869,9 @@ mod test { txq.add(tx0.clone(), TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator).unwrap(); txq.add(tx1.clone(), TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator).unwrap(); // the one with higher gas price is first - assert_eq!(txq.top_transactions()[0], tx0); - assert_eq!(txq.top_transactions()[1], tx1); + let top = txq.top_transactions(); + assert_eq!(top[0], tx0); + assert_eq!(top[1], tx1); // when // insert second as local @@ -1907,9 +1879,10 @@ mod test { // then // the order should be updated - assert_eq!(txq.top_transactions()[0], tx1); - assert_eq!(txq.top_transactions()[1], tx2); - assert_eq!(txq.top_transactions()[2], tx0); + let top = txq.top_transactions(); + assert_eq!(top[0], tx1); + assert_eq!(top[1], tx2); + assert_eq!(top[2], tx0); } #[test] @@ -1971,11 +1944,11 @@ mod test { txq.penalize(&tx1.hash()); // then - let top = txq.future_transactions(); - assert_eq!(top[0].transaction, txa); - assert_eq!(top[1].transaction, txb); - assert_eq!(top[2].transaction, tx1); - assert_eq!(top[3].transaction, tx2); + let top: Vec<_> = txq.future_transactions().into_iter().map(|tx| tx.transaction).collect(); + assert_eq!(top[0], txa); + assert_eq!(top[1], txb); + assert_eq!(top[2], tx1); + assert_eq!(top[3], tx2); assert_eq!(top.len(), 4); } @@ -2120,7 +2093,7 @@ mod test { assert_eq!(txq.status().future, 2); // when - txq.cull(tx.sender().unwrap(), next2_nonce); + txq.cull(tx.sender(), next2_nonce); // should remove both transactions since they are not valid // then @@ -2134,9 +2107,9 @@ mod test { let mut txq = TransactionQueue::default(); let kp = Random.generate().unwrap(); let secret = kp.secret(); - let tx = new_unsigned_tx(123.into(), default_gas_val(), 1.into()).sign(secret, None); - let tx1 = new_unsigned_tx(124.into(), default_gas_val(), 1.into()).sign(secret, None); - let tx2 = new_unsigned_tx(125.into(), default_gas_val(), 1.into()).sign(secret, None); + let tx = new_unsigned_tx(123.into(), default_gas_val(), 1.into()).sign(secret, None).into(); + let tx1 = new_unsigned_tx(124.into(), default_gas_val(), 1.into()).sign(secret, None).into(); + let tx2 = new_unsigned_tx(125.into(), default_gas_val(), 1.into()).sign(secret, None).into(); txq.add(tx, TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator).unwrap(); assert_eq!(txq.status().pending, 1); @@ -2166,9 +2139,8 @@ mod test { assert_eq!(txq2.status().future, 1); // when - txq2.cull(tx.sender().unwrap(), tx.nonce + U256::one()); - txq2.cull(tx2.sender().unwrap(), tx2.nonce + U256::one()); - + txq2.cull(tx.sender(), tx.nonce + U256::one()); + txq2.cull(tx2.sender(), tx2.nonce + U256::one()); // then let stats = txq2.status(); @@ -2222,7 +2194,7 @@ mod test { // given let mut txq = TransactionQueue::with_limits(PrioritizationStrategy::GasPriceOnly, 1, !U256::zero(), !U256::zero()); let (tx, tx2) = new_tx_pair_default(1.into(), 0.into()); - let sender = tx.sender().unwrap(); + let sender = tx.sender(); let nonce = tx.nonce; txq.add(tx.clone(), TransactionOrigin::External, 0, None, &default_account_details, &gas_estimator).unwrap(); assert_eq!(txq.status().pending, 1); @@ -2358,8 +2330,7 @@ mod test { assert_eq!(txq.status().pending, 3); // when - let sender = tx.sender().unwrap(); - txq.cull(sender, default_nonce() + U256::one()); + txq.cull(tx.sender(), default_nonce() + U256::one()); // then let stats = txq.status(); @@ -2375,7 +2346,7 @@ mod test { let keypair = Random.generate().unwrap(); let tx = new_unsigned_tx(123.into(), default_gas_val(), 1.into()).sign(keypair.secret(), None); let tx2 = { - let mut tx2 = (*tx).clone(); + let mut tx2 = (**tx).clone(); tx2.gas_price = U256::from(200); tx2.sign(keypair.secret(), None) }; @@ -2398,12 +2369,12 @@ mod test { let keypair = Random.generate().unwrap(); let tx0 = new_unsigned_tx(123.into(), default_gas_val(), 1.into()).sign(keypair.secret(), None); let tx1 = { - let mut tx1 = (*tx0).clone(); + let mut tx1 = (**tx0).clone(); tx1.nonce = U256::from(124); tx1.sign(keypair.secret(), None) }; let tx2 = { - let mut tx2 = (*tx1).clone(); + let mut tx2 = (**tx1).clone(); tx2.gas_price = U256::from(200); tx2.sign(keypair.secret(), None) }; @@ -2455,7 +2426,7 @@ mod test { // given let mut txq = TransactionQueue::default(); let tx = new_tx_default(); - let from = tx.sender().unwrap(); + let from = tx.sender(); let nonce = tx.nonce; let details = |_a: &Address| AccountDetails { nonce: nonce, balance: !U256::zero() }; @@ -2478,7 +2449,7 @@ mod test { txq.add(tx1, TransactionOrigin::External, 0, None, &details1, &gas_estimator).unwrap(); // when - txq.cull(tx2.sender().unwrap(), nonce2 + U256::one()); + txq.cull(tx2.sender(), nonce2 + U256::one()); // then assert!(txq.top_transactions().is_empty()); @@ -2489,7 +2460,7 @@ mod test { // given let mut txq = TransactionQueue::default(); let (tx1, tx2) = new_tx_pair_default(4.into(), 0.into()); - let sender = tx1.sender().unwrap(); + let sender = tx1.sender(); let (nonce1, nonce2) = (tx1.nonce, tx2.nonce); let details1 = |_a: &Address| AccountDetails { nonce: nonce1, balance: !U256::zero() }; @@ -2558,7 +2529,7 @@ mod test { (tx.sign(secret, None), tx2.sign(secret, None), tx2_2.sign(secret, None), tx3.sign(secret, None)) }; - let sender = tx1.sender().unwrap(); + let sender = tx1.sender(); txq.add(tx1, TransactionOrigin::Local, 0, None, &default_account_details, &gas_estimator).unwrap(); txq.add(tx2, TransactionOrigin::Local, 0, None, &default_account_details, &gas_estimator).unwrap(); txq.add(tx3, TransactionOrigin::Local, 0, None, &default_account_details, &gas_estimator).unwrap(); diff --git a/ethcore/src/snapshot/block.rs b/ethcore/src/snapshot/block.rs index 5b3846b6c..daa8c65c4 100644 --- a/ethcore/src/snapshot/block.rs +++ b/ethcore/src/snapshot/block.rs @@ -183,8 +183,8 @@ mod tests { data: "Eep!".into(), }.fake_sign(Address::from(0x55)); - b.transactions.push(t1); - b.transactions.push(t2); + b.transactions.push(t1.into()); + b.transactions.push(t2.into()); let receipts_root = b.header.receipts_root().clone(); b.header.set_transactions_root(::util::triehash::ordered_trie_root( diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index ae52ee3b1..6e40fdbb8 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -879,7 +879,7 @@ mod tests { data: FromHex::from_hex("601080600c6000396000f3006000355415600957005b60203560003555").unwrap(), }.sign(&secret(), None); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -939,7 +939,7 @@ mod tests { data: FromHex::from_hex("5b600056").unwrap(), }.sign(&secret(), None); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -977,7 +977,7 @@ mod tests { }.sign(&secret(), None); state.init_code(&0xa.into(), FromHex::from_hex("6000").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1019,7 +1019,7 @@ mod tests { data: vec![], }.sign(&secret(), None); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1268,7 +1268,7 @@ mod tests { }.sign(&secret(), None); state.init_code(&0xa.into(), FromHex::from_hex("5b600056").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1309,7 +1309,7 @@ mod tests { state.init_code(&0xa.into(), FromHex::from_hex("60006000600060006000600b602b5a03f1").unwrap()); state.init_code(&0xb.into(), FromHex::from_hex("6000").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { @@ -1368,7 +1368,7 @@ mod tests { }.sign(&secret(), None); state.init_code(&0xa.into(), FromHex::from_hex("60006000600060006045600b6000f1").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1423,7 +1423,7 @@ mod tests { }.sign(&secret(), None); state.init_code(&0xa.into(), FromHex::from_hex("600060006000600060ff600b6000f1").unwrap()); // not enough funds. - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1467,7 +1467,7 @@ mod tests { state.init_code(&0xa.into(), FromHex::from_hex("60006000600060006000600b602b5a03f1").unwrap()); state.init_code(&0xb.into(), FromHex::from_hex("5b600056").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1524,7 +1524,7 @@ mod tests { state.init_code(&0xa.into(), FromHex::from_hex("60006000600060006000600b602b5a03f1").unwrap()); state.init_code(&0xb.into(), FromHex::from_hex("60006000600060006000600c602b5a03f1").unwrap()); state.init_code(&0xc.into(), FromHex::from_hex("6000").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), @@ -1599,7 +1599,7 @@ mod tests { state.init_code(&0xa.into(), FromHex::from_hex("60006000600060006000600b602b5a03f1").unwrap()); state.init_code(&0xb.into(), FromHex::from_hex("60006000600060006000600c602b5a03f1505b601256").unwrap()); state.init_code(&0xc.into(), FromHex::from_hex("6000").unwrap()); - state.add_balance(t.sender().as_ref().unwrap(), &(100.into()), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &(100.into()), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { @@ -1671,7 +1671,7 @@ mod tests { state.init_code(&0xa.into(), FromHex::from_hex("73000000000000000000000000000000000000000bff").unwrap()); state.add_balance(&0xa.into(), &50.into(), CleanupMode::NoEmpty); - state.add_balance(t.sender().as_ref().unwrap(), &100.into(), CleanupMode::NoEmpty); + state.add_balance(&t.sender(), &100.into(), CleanupMode::NoEmpty); let result = state.apply(&info, &engine, &t, true).unwrap(); let expected_trace = vec![FlatTrace { trace_address: Default::default(), diff --git a/ethcore/src/tests/helpers.rs b/ethcore/src/tests/helpers.rs index 55306dc51..0af84a528 100644 --- a/ethcore/src/tests/helpers.rs +++ b/ethcore/src/tests/helpers.rs @@ -32,7 +32,7 @@ use ethereum::ethash::EthashParams; use devtools::*; use miner::Miner; use header::Header; -use transaction::{Action, SignedTransaction, Transaction}; +use transaction::{Action, Transaction, SignedTransaction}; use rlp::{self, RlpStream, Stream}; use views::BlockView; @@ -126,7 +126,7 @@ pub fn create_test_block_with_data(header: &Header, transactions: &[SignedTransa rlp.append(header); rlp.begin_list(transactions.len()); for t in transactions { - rlp.append_raw(&rlp::encode::(t).to_vec(), 1); + rlp.append_raw(&rlp::encode(t).to_vec(), 1); } rlp.append(&uncles); rlp.out() diff --git a/ethcore/src/types/encoded.rs b/ethcore/src/types/encoded.rs index 41db96fcf..6cb7a3cc1 100644 --- a/ethcore/src/types/encoded.rs +++ b/ethcore/src/types/encoded.rs @@ -25,7 +25,7 @@ use block::Block as FullBlock; use header::{BlockNumber, Header as FullHeader}; -use transaction::SignedTransaction; +use transaction::UnverifiedTransaction; use views; use util::{Address, Hashable, H256, H2048, U256}; @@ -126,7 +126,7 @@ impl Body { pub fn view(&self) -> views::BodyView { views::BodyView::new(&self.0) } /// Fully decode this block body. - pub fn decode(&self) -> (Vec, Vec) { + pub fn decode(&self) -> (Vec, Vec) { (self.view().transactions(), self.view().uncles()) } @@ -143,7 +143,7 @@ impl Body { // forwarders to borrowed view. impl Body { /// Get a vector of all transactions. - pub fn transactions(&self) -> Vec { self.view().transactions() } + pub fn transactions(&self) -> Vec { self.view().transactions() } /// Number of transactions in the block. pub fn transactions_count(&self) -> usize { self.view().transactions_count() } @@ -248,7 +248,7 @@ impl Block { // forwarders to body view. impl Block { /// Get a vector of all transactions. - pub fn transactions(&self) -> Vec { self.view().transactions() } + pub fn transactions(&self) -> Vec { self.view().transactions() } /// Number of transactions in the block. pub fn transactions_count(&self) -> usize { self.view().transactions_count() } diff --git a/ethcore/src/types/transaction.rs b/ethcore/src/types/transaction.rs index 1f26b156d..667e58d90 100644 --- a/ethcore/src/types/transaction.rs +++ b/ethcore/src/types/transaction.rs @@ -16,8 +16,7 @@ //! Transaction data structure. -use std::ops::{Deref, DerefMut}; -use std::cell::*; +use std::ops::Deref; use rlp::*; use util::sha3::Hashable; use util::{H256, Address, U256, Bytes, HeapSizeOf}; @@ -117,10 +116,10 @@ impl From for SignedTransaction { } } -impl From for SignedTransaction { +impl From for UnverifiedTransaction { fn from(t: ethjson::transaction::Transaction) -> Self { let to: Option = t.to.into(); - SignedTransaction { + UnverifiedTransaction { unsigned: Transaction { nonce: t.nonce.into(), gas_price: t.gas_price.into(), @@ -135,9 +134,8 @@ impl From for SignedTransaction { r: t.r.into(), s: t.s.into(), v: t.v.into(), - sender: Cell::new(None), - hash: Cell::new(None) - } + hash: 0.into(), + }.compute_hash() } } @@ -153,43 +151,45 @@ impl Transaction { pub fn sign(self, secret: &Secret, network_id: Option) -> SignedTransaction { let sig = ::ethkey::sign(secret, &self.hash(network_id)) .expect("data is valid and context has signing capabilities; qed"); - self.with_signature(sig, network_id) + SignedTransaction::new(self.with_signature(sig, network_id)) + .expect("secret is valid so it's recoverable") } /// Signs the transaction with signature. - pub fn with_signature(self, sig: Signature, network_id: Option) -> SignedTransaction { - SignedTransaction { + pub fn with_signature(self, sig: Signature, network_id: Option) -> UnverifiedTransaction { + UnverifiedTransaction { unsigned: self, r: sig.r().into(), s: sig.s().into(), v: sig.v() as u64 + if let Some(n) = network_id { 35 + n * 2 } else { 27 }, - hash: Cell::new(None), - sender: Cell::new(None), - } + hash: 0.into(), + }.compute_hash() } /// Useful for test incorrectly signed transactions. #[cfg(test)] - pub fn invalid_sign(self) -> SignedTransaction { - SignedTransaction { + pub fn invalid_sign(self) -> UnverifiedTransaction { + UnverifiedTransaction { unsigned: self, r: U256::default(), s: U256::default(), v: 0, - hash: Cell::new(None), - sender: Cell::new(None), - } + hash: 0.into(), + }.compute_hash() } /// Specify the sender; this won't survive the serialize/deserialize process, but can be cloned. pub fn fake_sign(self, from: Address) -> SignedTransaction { SignedTransaction { - unsigned: self, - r: U256::default(), - s: U256::default(), - v: 0, - hash: Cell::new(None), - sender: Cell::new(Some(from)), + transaction: UnverifiedTransaction { + unsigned: self, + r: U256::default(), + s: U256::default(), + v: 0, + hash: 0.into(), + }.compute_hash(), + sender: from, + public: Public::default(), } } @@ -208,9 +208,9 @@ impl Transaction { } /// Signed transaction information. -#[derive(Debug, Clone, Eq)] +#[derive(Debug, Clone, Eq, PartialEq)] #[cfg_attr(feature = "ipc", binary)] -pub struct SignedTransaction { +pub struct UnverifiedTransaction { /// Plain Transaction. unsigned: Transaction, /// The V field of the signature; the LS bit described which half of the curve our point falls @@ -220,19 +220,11 @@ pub struct SignedTransaction { r: U256, /// The S field of the signature; helps describe the point on the curve. s: U256, - /// Cached hash. - hash: Cell>, - /// Cached sender. - sender: Cell>, + /// Hash of the transaction + hash: H256, } -impl PartialEq for SignedTransaction { - fn eq(&self, other: &SignedTransaction) -> bool { - self.unsigned == other.unsigned && self.v == other.v && self.r == other.r && self.s == other.s - } -} - -impl Deref for SignedTransaction { +impl Deref for UnverifiedTransaction { type Target = Transaction; fn deref(&self) -> &Self::Target { @@ -240,19 +232,14 @@ impl Deref for SignedTransaction { } } -impl DerefMut for SignedTransaction { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.unsigned - } -} - -impl Decodable for SignedTransaction { +impl Decodable for UnverifiedTransaction { fn decode(decoder: &D) -> Result where D: Decoder { let d = decoder.as_rlp(); if d.item_count() != 9 { return Err(DecoderError::RlpIncorrectListLen); } - Ok(SignedTransaction { + let hash = decoder.as_raw().sha3(); + Ok(UnverifiedTransaction { unsigned: Transaction { nonce: d.val_at(0)?, gas_price: d.val_at(1)?, @@ -264,23 +251,23 @@ impl Decodable for SignedTransaction { v: d.val_at(6)?, r: d.val_at(7)?, s: d.val_at(8)?, - hash: Cell::new(None), - sender: Cell::new(None), + hash: hash, }) } } -impl Encodable for SignedTransaction { +impl Encodable for UnverifiedTransaction { fn rlp_append(&self, s: &mut RlpStream) { self.rlp_append_sealed_transaction(s) } } -impl HeapSizeOf for SignedTransaction { - fn heap_size_of_children(&self) -> usize { - self.unsigned.heap_size_of_children() +impl UnverifiedTransaction { + /// Used to compute hash of created transactions + fn compute_hash(mut self) -> UnverifiedTransaction { + let hash = (&*self.rlp_bytes()).sha3(); + self.hash = hash; + self } -} -impl SignedTransaction { /// Append object with a signature into RLP stream fn rlp_append_sealed_transaction(&self, s: &mut RlpStream) { s.begin_list(9); @@ -298,17 +285,9 @@ impl SignedTransaction { s.append(&self.s); } - /// Get the hash of this header (sha3 of the RLP). - pub fn hash(&self) -> H256 { - let hash = self.hash.get(); - match hash { - Some(h) => h, - None => { - let h = (&*self.rlp_bytes()).sha3(); - self.hash.set(Some(h)); - h - } - } + /// Reference to unsigned part of this transaction. + pub fn as_unsigned(&self) -> &Transaction { + &self.unsigned } /// 0 if `v` would have been 27 under "Electrum" notation, 1 if 28 or 4 if invalid. @@ -339,21 +318,13 @@ impl SignedTransaction { } } - /// Returns transaction sender. - pub fn sender(&self) -> Result { - let sender = self.sender.get(); - match sender { - Some(s) => Ok(s), - None => { - let s = public_to_address(&self.public_key()?); - self.sender.set(Some(s)); - Ok(s) - } - } + /// Get the hash of this header (sha3 of the RLP). + pub fn hash(&self) -> H256 { + self.hash } - /// Returns the public key of the sender. - pub fn public_key(&self) -> Result { + /// Recovers the public key of the sender. + pub fn recover_public(&self) -> Result { Ok(recover(&self.signature(), &self.unsigned.hash(self.network_id()))?) } @@ -361,7 +332,7 @@ impl SignedTransaction { // TODO: consider use in block validation. #[cfg(test)] #[cfg(feature = "json-tests")] - pub fn validate(self, schedule: &Schedule, require_low: bool, allow_network_id_of_one: bool) -> Result { + pub fn validate(self, schedule: &Schedule, require_low: bool, allow_network_id_of_one: bool) -> Result { if require_low && !self.signature().is_low_s() { return Err(EthkeyError::InvalidSignature.into()) } @@ -370,7 +341,7 @@ impl SignedTransaction { Some(1) if allow_network_id_of_one => {}, _ => return Err(TransactionError::InvalidNetworkId.into()), } - self.sender()?; + self.recover_public()?; if self.gas < U256::from(self.gas_required(&schedule)) { Err(TransactionError::InvalidGasLimit(::util::OutOfBounds{min: Some(U256::from(self.gas_required(&schedule))), max: None, found: self.gas}).into()) } else { @@ -379,22 +350,92 @@ impl SignedTransaction { } } +/// A `UnverifiedTransaction` with successfully recovered `sender`. +#[derive(Debug, Clone, Eq, PartialEq)] +pub struct SignedTransaction { + transaction: UnverifiedTransaction, + sender: Address, + public: Public, +} + +impl HeapSizeOf for SignedTransaction { + fn heap_size_of_children(&self) -> usize { + self.transaction.unsigned.heap_size_of_children() + } +} + +impl Encodable for SignedTransaction { + fn rlp_append(&self, s: &mut RlpStream) { self.transaction.rlp_append_sealed_transaction(s) } +} + +impl Deref for SignedTransaction { + type Target = UnverifiedTransaction; + fn deref(&self) -> &Self::Target { + &self.transaction + } +} + +impl From for UnverifiedTransaction { + fn from(tx: SignedTransaction) -> Self { + tx.transaction + } +} + +impl SignedTransaction { + /// Try to verify transaction and recover sender. + pub fn new(transaction: UnverifiedTransaction) -> Result { + let public = transaction.recover_public()?; + let sender = public_to_address(&public); + Ok(SignedTransaction { + transaction: transaction, + sender: sender, + public: public, + }) + } + + /// Returns transaction sender. + pub fn sender(&self) -> Address { + self.sender + } + + /// Returns a public key of the sender. + pub fn public_key(&self) -> Public { + self.public + } +} + /// Signed Transaction that is a part of canon blockchain. #[derive(Debug, Clone, PartialEq, Eq)] #[cfg_attr(feature = "ipc", binary)] pub struct LocalizedTransaction { /// Signed part. - pub signed: SignedTransaction, + pub signed: UnverifiedTransaction, /// Block number. pub block_number: BlockNumber, /// Block hash. pub block_hash: H256, /// Transaction index within block. - pub transaction_index: usize + pub transaction_index: usize, + /// Cached sender + pub cached_sender: Option
, +} + +impl LocalizedTransaction { + /// Returns transaction sender. + /// Panics if `LocalizedTransaction` is constructed using invalid `UnverifiedTransaction`. + pub fn sender(&mut self) -> Address { + if let Some(sender) = self.cached_sender { + return sender; + } + let sender = public_to_address(&self.recover_public() + .expect("LocalizedTransaction is always constructed from transaction from blockchain; Blockchain only stores verified transactions; qed")); + self.cached_sender = Some(sender); + sender + } } impl Deref for LocalizedTransaction { - type Target = SignedTransaction; + type Target = UnverifiedTransaction; fn deref(&self) -> &Self::Target { &self.signed @@ -432,7 +473,7 @@ impl From for PendingTransaction { #[test] fn sender_test() { - let t: SignedTransaction = decode(&::rustc_serialize::hex::FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap()); + let t: UnverifiedTransaction = decode(&::rustc_serialize::hex::FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap()); assert_eq!(t.data, b""); assert_eq!(t.gas, U256::from(0x5208u64)); assert_eq!(t.gas_price, U256::from(0x01u64)); @@ -441,7 +482,7 @@ fn sender_test() { assert_eq!(*to, "095e7baea6a6c7c4c2dfeb977efac326af552d87".into()); } else { panic!(); } assert_eq!(t.value, U256::from(0x0au64)); - assert_eq!(t.sender().unwrap(), "0f65fe9276bc9a24ae7083ae28e2660ef72df99e".into()); + assert_eq!(public_to_address(&t.recover_public().unwrap()), "0f65fe9276bc9a24ae7083ae28e2660ef72df99e".into()); assert_eq!(t.network_id(), None); } @@ -458,7 +499,7 @@ fn signing() { value: U256::from(1), data: b"Hello!".to_vec() }.sign(&key.secret(), None); - assert_eq!(Address::from(key.public().sha3()), t.sender().unwrap()); + assert_eq!(Address::from(key.public().sha3()), t.sender()); assert_eq!(t.network_id(), None); } @@ -472,11 +513,11 @@ fn fake_signing() { value: U256::from(1), data: b"Hello!".to_vec() }.fake_sign(Address::from(0x69)); - assert_eq!(Address::from(0x69), t.sender().unwrap()); + assert_eq!(Address::from(0x69), t.sender()); assert_eq!(t.network_id(), None); let t = t.clone(); - assert_eq!(Address::from(0x69), t.sender().unwrap()); + assert_eq!(Address::from(0x69), t.sender()); assert_eq!(t.network_id(), None); } @@ -492,7 +533,7 @@ fn should_recover_from_network_specific_signing() { value: U256::from(1), data: b"Hello!".to_vec() }.sign(&key.secret(), Some(69)); - assert_eq!(Address::from(key.public().sha3()), t.sender().unwrap()); + assert_eq!(Address::from(key.public().sha3()), t.sender()); assert_eq!(t.network_id(), Some(69)); } @@ -501,9 +542,9 @@ fn should_agree_with_vitalik() { use rustc_serialize::hex::FromHex; let test_vector = |tx_data: &str, address: &'static str| { - let signed: SignedTransaction = decode(&FromHex::from_hex(tx_data).unwrap()); - signed.check_low_s().unwrap(); - assert_eq!(signed.sender().unwrap(), address.into()); + let signed = decode(&FromHex::from_hex(tx_data).unwrap()); + let signed = SignedTransaction::new(signed).unwrap(); + assert_eq!(signed.sender(), address.into()); flushln!("networkid: {:?}", signed.network_id()); }; diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 2ff4c8963..30f8e54c5 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -83,7 +83,7 @@ pub fn verify_block_unordered(header: Header, bytes: Bytes, engine: &Engine, che { let v = BlockView::new(&bytes); for t in v.transactions() { - engine.verify_transaction(&t, &header)?; + let t = engine.verify_transaction(t, &header)?; transactions.push(t); } } @@ -460,7 +460,7 @@ mod tests { let mut uncles_rlp = RlpStream::new(); uncles_rlp.append(&good_uncles); let good_uncles_hash = uncles_rlp.as_raw().sha3(); - let good_transactions_root = ordered_trie_root(good_transactions.iter().map(|t| ::rlp::encode::(t).to_vec())); + let good_transactions_root = ordered_trie_root(good_transactions.iter().map(|t| ::rlp::encode::(t).to_vec())); let mut parent = good.clone(); parent.set_number(9); diff --git a/ethcore/src/views/block.rs b/ethcore/src/views/block.rs index 879c2b611..b09fdcd08 100644 --- a/ethcore/src/views/block.rs +++ b/ethcore/src/views/block.rs @@ -68,7 +68,7 @@ impl<'a> BlockView<'a> { } /// Return List of transactions in given block. - pub fn transactions(&self) -> Vec { + pub fn transactions(&self) -> Vec { self.rlp.val_at(1) } @@ -84,7 +84,8 @@ impl<'a> BlockView<'a> { signed: t, block_hash: block_hash.clone(), block_number: block_number, - transaction_index: i + transaction_index: i, + cached_sender: None, }).collect() } @@ -104,7 +105,7 @@ impl<'a> BlockView<'a> { } /// Returns transaction at given index without deserializing unnecessary data. - pub fn transaction_at(&self, index: usize) -> Option { + pub fn transaction_at(&self, index: usize) -> Option { self.rlp.at(1).iter().nth(index).map(|rlp| rlp.as_val()) } @@ -117,7 +118,8 @@ impl<'a> BlockView<'a> { signed: t, block_hash: block_hash, block_number: block_number, - transaction_index: index + transaction_index: index, + cached_sender: None, }) } diff --git a/ethcore/src/views/body.rs b/ethcore/src/views/body.rs index 34ab9679b..2bee157bb 100644 --- a/ethcore/src/views/body.rs +++ b/ethcore/src/views/body.rs @@ -48,7 +48,7 @@ impl<'a> BodyView<'a> { } /// Return List of transactions in given block. - pub fn transactions(&self) -> Vec { + pub fn transactions(&self) -> Vec { self.rlp.val_at(0) } @@ -61,7 +61,8 @@ impl<'a> BodyView<'a> { signed: t, block_hash: block_hash.clone(), block_number: block_number, - transaction_index: i + transaction_index: i, + cached_sender: None, }).collect() } @@ -81,7 +82,7 @@ impl<'a> BodyView<'a> { } /// Returns transaction at given index without deserializing unnecessary data. - pub fn transaction_at(&self, index: usize) -> Option { + pub fn transaction_at(&self, index: usize) -> Option { self.rlp.at(0).iter().nth(index).map(|rlp| rlp.as_val()) } @@ -91,7 +92,8 @@ impl<'a> BodyView<'a> { signed: t, block_hash: block_hash.clone(), block_number: block_number, - transaction_index: index + transaction_index: index, + cached_sender: None, }) } diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index 9fd8ab060..a340cabf8 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -179,7 +179,8 @@ pub fn sign_no_dispatch(client: &C, miner: &M, accounts: &AccountProvider, let hash = t.hash(network_id); let signature = signature(accounts, address, hash, password)?; signature.map(|sig| { - t.with_signature(sig, network_id) + SignedTransaction::new(t.with_signature(sig, network_id)) + .expect("Transaction was signed by AccountsProvider; it never produces invalid signatures; qed") }) }; Ok(signed_transaction) diff --git a/rpc/src/v1/impls/eth.rs b/rpc/src/v1/impls/eth.rs index 855b17e2e..612134c1c 100644 --- a/rpc/src/v1/impls/eth.rs +++ b/rpc/src/v1/impls/eth.rs @@ -647,11 +647,13 @@ impl Eth for EthClient where fn send_raw_transaction(&self, raw: Bytes) -> Result { self.active()?; - let raw_transaction = raw.to_vec(); - match UntrustedRlp::new(&raw_transaction).as_val() { - Ok(signed_transaction) => dispatch_transaction(&*take_weak!(self.client), &*take_weak!(self.miner), PendingTransaction::new(signed_transaction, None)).map(Into::into), - Err(e) => Err(errors::from_rlp_error(e)), - } + UntrustedRlp::new(&raw.into_vec()).as_val() + .map_err(errors::from_rlp_error) + .and_then(|tx| SignedTransaction::new(tx).map_err(errors::from_transaction_error)) + .and_then(|signed_transaction| { + dispatch_transaction(&*take_weak!(self.client), &*take_weak!(self.miner), PendingTransaction::new(signed_transaction, None)) + }) + .map(Into::into) } fn submit_transaction(&self, raw: Bytes) -> Result { diff --git a/rpc/src/v1/impls/parity_set.rs b/rpc/src/v1/impls/parity_set.rs index 06ca70f2a..dcb2cfe65 100644 --- a/rpc/src/v1/impls/parity_set.rs +++ b/rpc/src/v1/impls/parity_set.rs @@ -101,7 +101,7 @@ impl ParitySet for ParitySetClient where fn set_extra_data(&self, extra_data: Bytes) -> Result { self.active()?; - take_weak!(self.miner).set_extra_data(extra_data.to_vec()); + take_weak!(self.miner).set_extra_data(extra_data.into_vec()); Ok(true) } diff --git a/rpc/src/v1/impls/signer.rs b/rpc/src/v1/impls/signer.rs index 90b627569..f8ba6dd34 100644 --- a/rpc/src/v1/impls/signer.rs +++ b/rpc/src/v1/impls/signer.rs @@ -140,8 +140,9 @@ impl Signer for SignerClient where C: MiningBlockC signer.peek(&id).map(|confirmation| { let result = match confirmation.payload { ConfirmationPayload::SendTransaction(request) => { - let signed_transaction: SignedTransaction = UntrustedRlp::new(&bytes.0).as_val().map_err(errors::from_rlp_error)?; - let sender = signed_transaction.sender().map_err(|e| errors::invalid_params("Invalid signature.", e))?; + let signed_transaction = UntrustedRlp::new(&bytes.0).as_val().map_err(errors::from_rlp_error)?; + let signed_transaction = SignedTransaction::new(signed_transaction).map_err(|e| errors::invalid_params("Invalid signature.", e))?; + let sender = signed_transaction.sender(); // Verification let sender_matches = sender == request.from; diff --git a/rpc/src/v1/impls/traces.rs b/rpc/src/v1/impls/traces.rs index cb1fb524a..a01801594 100644 --- a/rpc/src/v1/impls/traces.rs +++ b/rpc/src/v1/impls/traces.rs @@ -128,14 +128,15 @@ impl Traces for TracesClient where C: BlockChainClient + 'static, M: self.active()?; let block = block.0; - let raw_transaction = Bytes::to_vec(raw_transaction); - match UntrustedRlp::new(&raw_transaction).as_val() { - Ok(signed) => Ok(match take_weak!(self.client).call(&signed, block.into(), to_call_analytics(flags)) { - Ok(e) => Some(TraceResults::from(e)), - _ => None, - }), - Err(e) => Err(errors::invalid_params("Transaction is not valid RLP", e)), - } + UntrustedRlp::new(&raw_transaction.into_vec()).as_val() + .map_err(|e| errors::invalid_params("Transaction is not valid RLP", e)) + .and_then(|tx| SignedTransaction::new(tx).map_err(errors::from_transaction_error)) + .and_then(|signed| { + Ok(match take_weak!(self.client).call(&signed, block.into(), to_call_analytics(flags)) { + Ok(e) => Some(TraceResults::from(e)), + _ => None, + }) + }) } fn replay_transaction(&self, transaction_hash: H256, flags: Vec) -> Result, Error> { diff --git a/rpc/src/v1/tests/helpers/miner_service.rs b/rpc/src/v1/tests/helpers/miner_service.rs index b25bcc39c..33bd63b36 100644 --- a/rpc/src/v1/tests/helpers/miner_service.rs +++ b/rpc/src/v1/tests/helpers/miner_service.rs @@ -22,7 +22,7 @@ use ethcore::error::{Error, CallError}; use ethcore::client::{MiningBlockChainClient, Executed, CallAnalytics}; use ethcore::block::{ClosedBlock, IsBlock}; use ethcore::header::BlockNumber; -use ethcore::transaction::{SignedTransaction, PendingTransaction}; +use ethcore::transaction::{UnverifiedTransaction, SignedTransaction, PendingTransaction}; use ethcore::receipt::{Receipt, RichReceipt}; use ethcore::miner::{MinerService, MinerStatus, TransactionImportResult, LocalTransactionStatus}; use ethcore::account_provider::Error as AccountError; @@ -144,12 +144,13 @@ impl MinerService for TestMinerService { } /// Imports transactions to transaction queue. - fn import_external_transactions(&self, _chain: &MiningBlockChainClient, transactions: Vec) -> + fn import_external_transactions(&self, _chain: &MiningBlockChainClient, transactions: Vec) -> Vec> { // lets assume that all txs are valid + let transactions: Vec<_> = transactions.into_iter().map(|tx| SignedTransaction::new(tx).unwrap()).collect(); self.imported_transactions.lock().extend_from_slice(&transactions); - for sender in transactions.iter().filter_map(|t| t.sender().ok()) { + for sender in transactions.iter().map(|tx| tx.sender()) { let nonce = self.last_nonce(&sender).expect("last_nonce must be populated in tests"); self.last_nonces.write().insert(sender, nonce + U256::from(1)); } @@ -164,10 +165,9 @@ impl MinerService for TestMinerService { Result { // keep the pending nonces up to date - if let Ok(ref sender) = pending.transaction.sender() { - let nonce = self.last_nonce(sender).unwrap_or(chain.latest_nonce(sender)); - self.last_nonces.write().insert(sender.clone(), nonce + U256::from(1)); - } + let sender = pending.transaction.sender(); + let nonce = self.last_nonce(&sender).unwrap_or(chain.latest_nonce(&sender)); + self.last_nonces.write().insert(sender, nonce + U256::from(1)); // lets assume that all txs are valid self.imported_transactions.lock().push(pending.transaction); diff --git a/rpc/src/v1/tests/mocked/eth.rs b/rpc/src/v1/tests/mocked/eth.rs index 32d50e90e..4706753b3 100644 --- a/rpc/src/v1/tests/mocked/eth.rs +++ b/rpc/src/v1/tests/mocked/eth.rs @@ -498,12 +498,14 @@ fn rpc_eth_transaction_count_by_number_pending() { #[test] fn rpc_eth_pending_transaction_by_hash() { - use util::*; - use ethcore::transaction::*; + use util::{H256, FromHex}; + use rlp; + use ethcore::transaction::SignedTransaction; let tester = EthTester::default(); { - let tx: SignedTransaction = ::rlp::decode(&FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap()); + let tx = rlp::decode(&FromHex::from_hex("f85f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d870a801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a0efffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804").unwrap()); + let tx = SignedTransaction::new(tx).unwrap(); tester.miner.pending_transactions.lock().insert(H256::zero(), tx); } @@ -831,7 +833,7 @@ fn rpc_eth_sign_transaction() { r#""minBlock":null,"# + &format!("\"networkId\":{},", t.network_id().map_or("null".to_owned(), |n| format!("{}", n))) + r#""nonce":"0x1","# + - &format!("\"publicKey\":\"0x{:?}\",", t.public_key().unwrap()) + + &format!("\"publicKey\":\"0x{:?}\",", t.recover_public().unwrap()) + &format!("\"r\":\"0x{}\",", U256::from(signature.r()).to_hex()) + &format!("\"raw\":\"0x{}\",", rlp.to_hex()) + &format!("\"s\":\"0x{}\",", U256::from(signature.s()).to_hex()) + diff --git a/rpc/src/v1/tests/mocked/signing.rs b/rpc/src/v1/tests/mocked/signing.rs index e6a27f832..a55b435a8 100644 --- a/rpc/src/v1/tests/mocked/signing.rs +++ b/rpc/src/v1/tests/mocked/signing.rs @@ -29,7 +29,7 @@ use v1::tests::mocked::parity; use util::{Address, FixedHash, Uint, U256, ToPretty, Hashable}; use ethcore::account_provider::AccountProvider; use ethcore::client::TestBlockChainClient; -use ethcore::transaction::{Transaction, Action}; +use ethcore::transaction::{Transaction, Action, SignedTransaction}; use ethstore::ethkey::{Generator, Random}; use futures::Future; use serde_json; @@ -269,6 +269,7 @@ fn should_add_sign_transaction_to_the_queue() { }; let signature = tester.accounts.sign(address, Some("test".into()), t.hash(None)).unwrap(); let t = t.with_signature(signature, None); + let t = SignedTransaction::new(t).unwrap(); let signature = t.signature(); let rlp = rlp::encode(&t); @@ -283,7 +284,7 @@ fn should_add_sign_transaction_to_the_queue() { r#""minBlock":null,"# + &format!("\"networkId\":{},", t.network_id().map_or("null".to_owned(), |n| format!("{}", n))) + r#""nonce":"0x1","# + - &format!("\"publicKey\":\"0x{:?}\",", t.public_key().unwrap()) + + &format!("\"publicKey\":\"0x{:?}\",", t.public_key()) + &format!("\"r\":\"0x{}\",", U256::from(signature.r()).to_hex()) + &format!("\"raw\":\"0x{}\",", rlp.to_hex()) + &format!("\"s\":\"0x{}\",", U256::from(signature.s()).to_hex()) + diff --git a/rpc/src/v1/types/bytes.rs b/rpc/src/v1/types/bytes.rs index 56d80ad1a..e7b2eb60c 100644 --- a/rpc/src/v1/types/bytes.rs +++ b/rpc/src/v1/types/bytes.rs @@ -31,7 +31,7 @@ impl Bytes { Bytes(bytes) } /// Convert back to vector - pub fn to_vec(self) -> Vec { + pub fn into_vec(self) -> Vec { self.0 } } diff --git a/rpc/src/v1/types/transaction.rs b/rpc/src/v1/types/transaction.rs index 57c29a3b4..827b4b4e4 100644 --- a/rpc/src/v1/types/transaction.rs +++ b/rpc/src/v1/types/transaction.rs @@ -162,7 +162,7 @@ impl From for RichRawTransaction { } impl From for Transaction { - fn from(t: LocalizedTransaction) -> Transaction { + fn from(mut t: LocalizedTransaction) -> Transaction { let signature = t.signature(); Transaction { hash: t.hash().into(), @@ -170,7 +170,7 @@ impl From for Transaction { block_hash: Some(t.block_hash.clone().into()), block_number: Some(t.block_number.into()), transaction_index: Some(t.transaction_index.into()), - from: t.sender().unwrap().into(), + from: t.sender().into(), to: match t.action { Action::Create => None, Action::Call(ref address) => Some(address.clone().into()) @@ -180,11 +180,11 @@ impl From for Transaction { gas: t.gas.into(), input: Bytes::new(t.data.clone()), creates: match t.action { - Action::Create => Some(contract_address(&t.sender().unwrap(), &t.nonce).into()), + Action::Create => Some(contract_address(&t.sender(), &t.nonce).into()), Action::Call(_) => None, }, raw: ::rlp::encode(&t.signed).to_vec().into(), - public_key: t.public_key().ok().map(Into::into), + public_key: t.recover_public().ok().map(Into::into), network_id: t.network_id(), standard_v: t.standard_v().into(), v: t.original_v().into(), @@ -204,7 +204,7 @@ impl From for Transaction { block_hash: None, block_number: None, transaction_index: None, - from: t.sender().unwrap().into(), + from: t.sender().into(), to: match t.action { Action::Create => None, Action::Call(ref address) => Some(address.clone().into()) @@ -214,11 +214,11 @@ impl From for Transaction { gas: t.gas.into(), input: Bytes::new(t.data.clone()), creates: match t.action { - Action::Create => Some(contract_address(&t.sender().unwrap(), &t.nonce).into()), + Action::Create => Some(contract_address(&t.sender(), &t.nonce).into()), Action::Call(_) => None, }, raw: ::rlp::encode(&t).to_vec().into(), - public_key: t.public_key().ok().map(Into::into), + public_key: Some(t.public_key().into()), network_id: t.network_id(), standard_v: t.standard_v().into(), v: t.original_v().into(), diff --git a/sync/src/api.rs b/sync/src/api.rs index 1aa8213bd..7a8a3623f 100644 --- a/sync/src/api.rs +++ b/sync/src/api.rs @@ -364,7 +364,7 @@ impl ChainNotify for EthSync { struct TxRelay(Arc); impl LightHandler for TxRelay { - fn on_transactions(&self, ctx: &EventContext, relay: &[::ethcore::transaction::SignedTransaction]) { + fn on_transactions(&self, ctx: &EventContext, relay: &[::ethcore::transaction::UnverifiedTransaction]) { trace!(target: "les", "Relaying {} transactions from peer {}", relay.len(), ctx.peer()); self.0.queue_transactions(relay.iter().map(|tx| ::rlp::encode(tx).to_vec()).collect(), ctx.peer()) } diff --git a/sync/src/chain.rs b/sync/src/chain.rs index d36cc26c9..75432c21d 100644 --- a/sync/src/chain.rs +++ b/sync/src/chain.rs @@ -2124,7 +2124,7 @@ mod tests { use std::collections::{HashSet, VecDeque}; use tests::helpers::*; use tests::snapshot::TestSnapshotService; - use util::{U256, RwLock}; + use util::{U256, Address, RwLock}; use util::sha3::Hashable; use util::hash::{H256, FixedHash}; use util::bytes::Bytes; @@ -2132,8 +2132,10 @@ mod tests { use super::*; use ::SyncConfig; use super::{PeerInfo, PeerAsking}; + use ethkey; use ethcore::header::*; use ethcore::client::*; + use ethcore::transaction::UnverifiedTransaction; use ethcore::miner::MinerService; fn get_dummy_block(order: u32, parent_hash: H256) -> Bytes { @@ -2762,6 +2764,10 @@ mod tests { #[test] fn should_add_transactions_to_queue() { + fn sender(tx: &UnverifiedTransaction) -> Address { + ethkey::public_to_address(&tx.recover_public().unwrap()) + } + // given let mut client = TestBlockChainClient::new(); client.add_blocks(98, EachBlockWith::Uncle); @@ -2775,8 +2781,9 @@ mod tests { // Add some balance to clients and reset nonces for h in &[good_blocks[0], retracted_blocks[0]] { let block = client.block(BlockId::Hash(*h)).unwrap(); - client.set_balance(block.transactions()[0].sender().unwrap(), U256::from(1_000_000_000)); - client.set_nonce(block.transactions()[0].sender().unwrap(), U256::from(0)); + let sender = sender(&block.transactions()[0]);; + client.set_balance(sender, U256::from(1_000_000_000)); + client.set_nonce(sender, U256::from(0)); } @@ -2793,7 +2800,7 @@ mod tests { // We need to update nonce status (because we say that the block has been imported) for h in &[good_blocks[0]] { let block = client.block(BlockId::Hash(*h)).unwrap(); - client.set_nonce(block.transactions()[0].sender().unwrap(), U256::from(1)); + client.set_nonce(sender(&block.transactions()[0]), U256::from(1)); } { let queue = RwLock::new(VecDeque::new());