From 57626b60e7e2d501f76a598b7d51022ea7dff00a Mon Sep 17 00:00:00 2001 From: Arkadiy Paronyan Date: Wed, 28 Jun 2017 09:10:57 +0200 Subject: [PATCH] EIP-168, 169: Dust protection (#4757) * Dust protection * Track touched accounts in the substate * Minor alterations --- ethcore/src/block.rs | 7 +- ethcore/src/client/client.rs | 6 +- ethcore/src/client/evm_test_client.rs | 2 +- ethcore/src/engines/mod.rs | 10 +- ethcore/src/error.rs | 3 + ethcore/src/ethereum/mod.rs | 3 +- ethcore/src/evm/mod.rs | 2 +- ethcore/src/evm/schedule.rs | 22 ++- ethcore/src/executive.rs | 21 +-- ethcore/src/externalities.rs | 4 +- ethcore/src/miner/miner.rs | 5 +- ethcore/src/miner/transaction_queue.rs | 34 ++++- ethcore/src/snapshot/consensus/authority.rs | 4 +- .../src/snapshot/tests/proof_of_authority.rs | 2 +- ethcore/src/spec/spec.rs | 13 +- ethcore/src/state/account.rs | 5 + ethcore/src/state/mod.rs | 132 +++++++++++++----- ethcore/src/state/substate.rs | 16 +-- ethcore/src/types/executed.rs | 3 + ethcore/src/verification/verification.rs | 44 ++++++ json/src/spec/params.rs | 8 ++ parity/dapps.rs | 2 +- parity/light_helpers/queue_cull.rs | 2 +- parity/run.rs | 3 +- rpc/src/v1/helpers/dispatch.rs | 2 +- 25 files changed, 270 insertions(+), 85 deletions(-) diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index 60c7531e4..12b7430b8 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -261,7 +261,8 @@ impl<'x> OpenBlock<'x> { gas_range_target: (U256, U256), extra_data: Bytes, ) -> Result { - let state = State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce(), factories)?; + let number = parent.number() + 1; + let state = State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce(number), factories)?; let mut r = OpenBlock { block: ExecutedBlock::new(state, tracing), engine: engine, @@ -269,7 +270,7 @@ impl<'x> OpenBlock<'x> { }; r.block.header.set_parent_hash(parent.hash()); - r.block.header.set_number(parent.number() + 1); + r.block.header.set_number(number); r.block.header.set_author(author); r.block.header.set_timestamp_now(parent.timestamp()); r.block.header.set_extra_data(extra_data); @@ -556,7 +557,7 @@ pub fn enact( ) -> Result { { if ::log::max_log_level() >= ::log::LogLevel::Trace { - let s = State::from_existing(db.boxed_clone(), parent.state_root().clone(), engine.account_start_nonce(), factories.clone())?; + let s = State::from_existing(db.boxed_clone(), parent.state_root().clone(), engine.account_start_nonce(parent.number() + 1), factories.clone())?; trace!(target: "enact", "num={}, root={}, author={}, author_balance={}\n", header.number(), s.root(), header.author(), s.balance(&header.author())?); } diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index a42b5723e..8b71ec0de 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -810,7 +810,7 @@ impl Client { } let root = header.state_root(); - State::from_existing(db, root, self.engine.account_start_nonce(), self.factories.clone()).ok() + State::from_existing(db, root, self.engine.account_start_nonce(block_number), self.factories.clone()).ok() }) } @@ -835,7 +835,7 @@ impl Client { State::from_existing( self.state_db.lock().boxed_clone_canon(&header.hash()), header.state_root(), - self.engine.account_start_nonce(), + self.engine.account_start_nonce(header.number()), self.factories.clone()) .expect("State root of best block header always valid.") } @@ -987,7 +987,7 @@ impl Client { fn contract_call_tx(&self, block_id: BlockId, address: Address, data: Bytes) -> SignedTransaction { let from = Address::default(); Transaction { - nonce: self.nonce(&from, block_id).unwrap_or_else(|| self.engine.account_start_nonce()), + nonce: self.nonce(&from, block_id).unwrap_or_else(|| self.engine.account_start_nonce(0)), action: Action::Call(address), gas: U256::from(50_000_000), gas_price: U256::default(), diff --git a/ethcore/src/client/evm_test_client.rs b/ethcore/src/client/evm_test_client.rs index 945de14d7..300e2fc92 100644 --- a/ethcore/src/client/evm_test_client.rs +++ b/ethcore/src/client/evm_test_client.rs @@ -89,7 +89,7 @@ impl EvmTestClient { -> Result<(U256, Vec), EvmTestError> { let genesis = self.spec.genesis_header(); - let mut state = state::State::from_existing(self.state_db.boxed_clone(), *genesis.state_root(), self.spec.engine.account_start_nonce(), self.factories.clone()) + let mut state = state::State::from_existing(self.state_db.boxed_clone(), *genesis.state_root(), self.spec.engine.account_start_nonce(0), self.factories.clone()) .map_err(EvmTestError::Trie)?; let info = client::EnvInfo { number: genesis.number(), diff --git a/ethcore/src/engines/mod.rs b/ethcore/src/engines/mod.rs index 849e08cf2..922eb11be 100644 --- a/ethcore/src/engines/mod.rs +++ b/ethcore/src/engines/mod.rs @@ -161,8 +161,14 @@ pub trait Engine : Sync + Send { fn maximum_uncle_count(&self) -> usize { 2 } /// The number of generations back that uncles can be. fn maximum_uncle_age(&self) -> usize { 6 } - /// The nonce with which accounts begin. - fn account_start_nonce(&self) -> U256 { self.params().account_start_nonce } + /// The nonce with which accounts begin at given block. + fn account_start_nonce(&self, block: u64) -> U256 { + if block >= self.params().dust_protection_transition { + U256::from(self.params().nonce_cap_increment) * U256::from(block) + } else { + self.params().account_start_nonce + } + } /// Block transformation functions, before the transactions. fn on_new_block(&self, block: &mut ExecutedBlock, last_hashes: Arc) -> Result<(), Error> { diff --git a/ethcore/src/error.rs b/ethcore/src/error.rs index dd9b7464c..e3a32db01 100644 --- a/ethcore/src/error.rs +++ b/ethcore/src/error.rs @@ -165,6 +165,8 @@ pub enum BlockError { InvalidNumber(Mismatch), /// Block number isn't sensible. RidiculousNumber(OutOfBounds), + /// Too many transactions from a particular address. + TooManyTransactions(Address), /// Parent given is unknown. UnknownParent(H256), /// Uncle parent given is unknown. @@ -205,6 +207,7 @@ impl fmt::Display for BlockError { UnknownParent(ref hash) => format!("Unknown parent: {}", hash), UnknownUncleParent(ref hash) => format!("Unknown uncle parent: {}", hash), UnknownEpochTransition(ref num) => format!("Unknown transition to epoch number: {}", num), + TooManyTransactions(ref address) => format!("Too many transactions from: {}", address), }; f.write_fmt(format_args!("Block error ({})", msg)) diff --git a/ethcore/src/ethereum/mod.rs b/ethcore/src/ethereum/mod.rs index 475496c2b..f94c39b33 100644 --- a/ethcore/src/ethereum/mod.rs +++ b/ethcore/src/ethereum/mod.rs @@ -98,8 +98,7 @@ mod tests { let engine = &spec.engine; let genesis_header = spec.genesis_header(); let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap(); - - let s = State::from_existing(db, genesis_header.state_root().clone(), engine.account_start_nonce(), Default::default()).unwrap(); + let s = State::from_existing(db, genesis_header.state_root().clone(), engine.account_start_nonce(0), Default::default()).unwrap(); assert_eq!(s.balance(&"0000000000000000000000000000000000000001".into()).unwrap(), 1u64.into()); assert_eq!(s.balance(&"0000000000000000000000000000000000000002".into()).unwrap(), 1u64.into()); assert_eq!(s.balance(&"0000000000000000000000000000000000000003".into()).unwrap(), 1u64.into()); diff --git a/ethcore/src/evm/mod.rs b/ethcore/src/evm/mod.rs index 91b44a543..a48004290 100644 --- a/ethcore/src/evm/mod.rs +++ b/ethcore/src/evm/mod.rs @@ -38,5 +38,5 @@ pub use self::ext::{Ext, ContractCreateResult, MessageCallResult, CreateContract pub use self::instructions::{InstructionInfo, INSTRUCTIONS, push_bytes}; pub use self::vmtype::VMType; pub use self::factory::Factory; -pub use self::schedule::Schedule; +pub use self::schedule::{Schedule, CleanDustMode}; pub use types::executed::CallType; diff --git a/ethcore/src/evm/schedule.rs b/ethcore/src/evm/schedule.rs index de4406767..8971aa16c 100644 --- a/ethcore/src/evm/schedule.rs +++ b/ethcore/src/evm/schedule.rs @@ -108,6 +108,19 @@ pub struct Schedule { pub blockhash_gas: usize, /// Static Call opcode enabled. pub have_static_call: bool, + /// Kill basic accounts below this balance if touched. + pub kill_dust: CleanDustMode, +} + +/// Dust accounts cleanup mode. +#[derive(PartialEq, Eq)] +pub enum CleanDustMode { + /// Dust cleanup is disabled. + Off, + /// Basic dust accounts will be removed. + BasicOnly, + /// Basic and contract dust accounts will be removed. + WithCodeAndStorage, } impl Schedule { @@ -168,15 +181,16 @@ impl Schedule { kill_empty: kill_empty, blockhash_gas: 20, have_static_call: false, + kill_dust: CleanDustMode::Off, } } - /// Schedule for the Metropolis era from common spec params. + /// Schedule for the post-EIP-150-era of the Ethereum main net. pub fn from_params(block_number: u64, params: &CommonParams) -> Schedule { let mut schedule = Schedule::new_post_eip150(usize::max_value(), true, true, true); schedule.apply_params(block_number, params); schedule - } + } /// Apply common spec config parameters to the schedule. pub fn apply_params(&mut self, block_number: u64, params: &CommonParams) { @@ -186,6 +200,9 @@ impl Schedule { if block_number >= params.eip210_transition { self.blockhash_gas = 350; } + if block_number >= params.dust_protection_transition { + self.kill_dust = if params.remove_dust_contracts { CleanDustMode::WithCodeAndStorage } else { CleanDustMode::BasicOnly }; + } } /// Schedule for the Metropolis of the Ethereum main net. @@ -244,6 +261,7 @@ impl Schedule { kill_empty: false, blockhash_gas: 20, have_static_call: false, + kill_dust: CleanDustMode::Off, } } } diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 7e582ccae..b486aa8d4 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -22,7 +22,7 @@ use engines::Engine; use types::executed::CallType; use env_info::EnvInfo; use error::ExecutionError; -use evm::{self, Ext, Finalize, CreateContractAddress, FinalizationResult, ReturnData}; +use evm::{self, Ext, Finalize, CreateContractAddress, FinalizationResult, ReturnData, CleanDustMode}; use externalities::*; use trace::{FlatTrace, Tracer, NoopTracer, ExecutiveTracer, VMTrace, VMTracer, ExecutiveVMTracer, NoopVMTracer}; use transaction::{Action, SignedTransaction}; @@ -164,6 +164,10 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> { return Err(From::from(ExecutionError::NotEnoughBaseGas { required: base_gas_required, got: t.gas })); } + if !t.is_unsigned() && check_nonce && schedule.kill_dust != CleanDustMode::Off && !self.state.exists(&sender)? { + return Err(From::from(ExecutionError::SenderMustExist)); + } + let init_gas = t.gas - base_gas_required; // validate transaction nonce @@ -191,13 +195,13 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> { return Err(From::from(ExecutionError::NotEnoughCash { required: total_cost, got: balance512 })); } + let mut substate = Substate::new(); + // NOTE: there can be no invalid transactions from this point. if !t.is_unsigned() { self.state.inc_nonce(&sender)?; } - self.state.sub_balance(&sender, &U256::from(gas_cost))?; - - let mut substate = Substate::new(); + self.state.sub_balance(&sender, &U256::from(gas_cost), &mut substate.to_cleanup_mode(&schedule))?; let (result, output) = match t.action { Action::Create => { @@ -434,7 +438,7 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> { let nonce_offset = if schedule.no_empty {1} else {0}.into(); let prev_bal = self.state.balance(¶ms.address)?; if let ActionValue::Transfer(val) = params.value { - self.state.sub_balance(¶ms.sender, &val)?; + self.state.sub_balance(¶ms.sender, &val, &mut substate.to_cleanup_mode(&schedule))?; self.state.new_contract(¶ms.address, val + prev_bal, nonce_offset); } else { self.state.new_contract(¶ms.address, prev_bal, nonce_offset); @@ -512,11 +516,8 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> { } // perform garbage-collection - for address in &substate.garbage { - if self.state.exists(address)? && !self.state.exists_and_not_null(address)? { - self.state.kill_account(address); - } - } + let min_balance = if schedule.kill_dust != CleanDustMode::Off { Some(U256::from(schedule.tx_gas) * t.gas_price) } else { None }; + self.state.kill_garbage(&substate.touched, schedule.kill_empty, &min_balance, schedule.kill_dust == CleanDustMode::WithCodeAndStorage)?; match result { Err(evm::Error::Internal(msg)) => Err(ExecutionError::Internal(msg)), diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 72c13f062..6abb6e3fa 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -17,7 +17,7 @@ //! Transaction Execution environment. use util::*; use action_params::{ActionParams, ActionValue}; -use state::{Backend as StateBackend, State, Substate}; +use state::{Backend as StateBackend, State, Substate, CleanupMode}; use engines::Engine; use env_info::EnvInfo; use executive::*; @@ -347,7 +347,7 @@ impl<'a, T: 'a, V: 'a, B: 'a, E: 'a> Ext for Externalities<'a, T, V, B, E> let balance = self.balance(&address)?; if &address == refund_address { // TODO [todr] To be consistent with CPP client we set balance to 0 in that case. - self.state.sub_balance(&address, &balance)?; + self.state.sub_balance(&address, &balance, &mut CleanupMode::NoEmpty)?; } else { trace!(target: "ext", "Suiciding {} -> {} (xfer: {})", address, refund_address, balance); self.state.transfer_balance( diff --git a/ethcore/src/miner/miner.rs b/ethcore/src/miner/miner.rs index e5edc6d05..c772490ba 100644 --- a/ethcore/src/miner/miner.rs +++ b/ethcore/src/miner/miner.rs @@ -328,7 +328,10 @@ impl Miner { let _timer = PerfTimer::new("prepare_block"); let chain_info = chain.chain_info(); let (transactions, mut open_block, original_work_hash) = { - let transactions = {self.transaction_queue.read().top_transactions_at(chain_info.best_block_number, chain_info.best_block_timestamp)}; + let nonce_cap = if chain_info.best_block_number + 1 >= self.engine.params().dust_protection_transition { + Some((self.engine.params().nonce_cap_increment * (chain_info.best_block_number + 1)).into()) + } else { None }; + let transactions = {self.transaction_queue.read().top_transactions_at(chain_info.best_block_number, chain_info.best_block_timestamp, nonce_cap)}; let mut sealing_work = self.sealing_work.lock(); let last_work_hash = sealing_work.queue.peek_last_ref().map(|pb| pb.block().fields().header.hash()); let best_hash = chain_info.best_block_hash; diff --git a/ethcore/src/miner/transaction_queue.rs b/ethcore/src/miner/transaction_queue.rs index 03a2d37ad..59ab3eacf 100644 --- a/ethcore/src/miner/transaction_queue.rs +++ b/ethcore/src/miner/transaction_queue.rs @@ -1084,11 +1084,11 @@ impl TransactionQueue { /// Returns top transactions from the queue ordered by priority. pub fn top_transactions(&self) -> Vec { - self.top_transactions_at(BlockNumber::max_value(), u64::max_value()) + self.top_transactions_at(BlockNumber::max_value(), u64::max_value(), None) } - fn filter_pending_transaction(&self, best_block: BlockNumber, best_timestamp: u64, mut f: F) + fn filter_pending_transaction(&self, best_block: BlockNumber, best_timestamp: u64, nonce_cap: Option, mut f: F) where F: FnMut(&VerifiedTransaction) { let mut delayed = HashSet::new(); @@ -1098,6 +1098,11 @@ impl TransactionQueue { if delayed.contains(&sender) { continue; } + if let Some(max_nonce) = nonce_cap { + if tx.nonce() >= max_nonce { + continue; + } + } let delay = match tx.condition { Some(Condition::Number(n)) => n > best_block, Some(Condition::Timestamp(t)) => t > best_timestamp, @@ -1112,16 +1117,16 @@ impl TransactionQueue { } /// Returns top transactions from the queue ordered by priority. - pub fn top_transactions_at(&self, best_block: BlockNumber, best_timestamp: u64) -> Vec { + pub fn top_transactions_at(&self, best_block: BlockNumber, best_timestamp: u64, nonce_cap: Option) -> Vec { let mut r = Vec::new(); - self.filter_pending_transaction(best_block, best_timestamp, |tx| r.push(tx.transaction.clone())); + self.filter_pending_transaction(best_block, best_timestamp, nonce_cap, |tx| r.push(tx.transaction.clone())); r } /// Return all ready transactions. pub fn pending_transactions(&self, best_block: BlockNumber, best_timestamp: u64) -> Vec { let mut r = Vec::new(); - self.filter_pending_transaction(best_block, best_timestamp, |tx| r.push(PendingTransaction::new(tx.transaction.clone(), tx.condition.clone()))); + self.filter_pending_transaction(best_block, best_timestamp, None, |tx| r.push(PendingTransaction::new(tx.transaction.clone(), tx.condition.clone()))); r } @@ -2205,9 +2210,9 @@ pub mod test { // then assert_eq!(res1, TransactionImportResult::Current); assert_eq!(res2, TransactionImportResult::Current); - let top = txq.top_transactions_at(0, 0); + let top = txq.top_transactions_at(0, 0, None); assert_eq!(top.len(), 0); - let top = txq.top_transactions_at(1, 0); + let top = txq.top_transactions_at(1, 0, None); assert_eq!(top.len(), 2); } @@ -2809,4 +2814,19 @@ pub mod test { // then assert_eq!(txq.top_transactions().len(), 1); } + + #[test] + fn should_not_return_transactions_over_nonce_cap() { + // given + let keypair = Random.generate().unwrap(); + let mut txq = TransactionQueue::default(); + // when + for nonce in 123..130 { + let tx = new_unsigned_tx(nonce.into(), default_gas_val(), default_gas_price()).sign(keypair.secret(), None); + txq.add(tx, TransactionOrigin::External, 0, None, &default_tx_provider()).unwrap(); + } + + // then + assert_eq!(txq.top_transactions_at(BlockNumber::max_value(), u64::max_value(), Some(127.into())).len(), 4); + } } diff --git a/ethcore/src/snapshot/consensus/authority.rs b/ethcore/src/snapshot/consensus/authority.rs index 0d7595c76..9cbffb62b 100644 --- a/ethcore/src/snapshot/consensus/authority.rs +++ b/ethcore/src/snapshot/consensus/authority.rs @@ -211,7 +211,7 @@ fn make_tx_and_env( use transaction::{Action, Transaction}; let transaction = Transaction { - nonce: engine.account_start_nonce(), + nonce: engine.account_start_nonce(header.number()), action: Action::Call(addr), gas: 50_000_000.into(), gas_price: 0.into(), @@ -469,7 +469,7 @@ impl Rebuilder for ChunkRebuilder { let mut state = State::from_existing( db.boxed_clone(), self.manifest.state_root.clone(), - engine.account_start_nonce(), + engine.account_start_nonce(target_header.number()), factories, ).map_err(|e| format!("State root mismatch: {}", e))?; diff --git a/ethcore/src/snapshot/tests/proof_of_authority.rs b/ethcore/src/snapshot/tests/proof_of_authority.rs index 5958a5f64..beec143cb 100644 --- a/ethcore/src/snapshot/tests/proof_of_authority.rs +++ b/ethcore/src/snapshot/tests/proof_of_authority.rs @@ -124,7 +124,7 @@ fn make_chain(accounts: Arc, blocks_beyond: usize, transitions: }; // execution callback for native contract: push transaction to be sealed. - let nonce = RefCell::new(client.engine().account_start_nonce()); + let nonce = RefCell::new(client.engine().account_start_nonce(0)); let exec = |addr, data| { let mut nonce = nonce.borrow_mut(); let transaction = Transaction { diff --git a/ethcore/src/spec/spec.rs b/ethcore/src/spec/spec.rs index d1a7ce484..1d0d2bc6f 100644 --- a/ethcore/src/spec/spec.rs +++ b/ethcore/src/spec/spec.rs @@ -76,6 +76,12 @@ pub struct CommonParams { pub eip211_transition: BlockNumber, /// Number of first block where EIP-214 rules begin. pub eip214_transition: BlockNumber, + /// Number of first block where dust cleanup rules (EIP-168 and EIP169) begin. + pub dust_protection_transition: BlockNumber, + /// Nonce cap increase per block. Nonce cap is only checked if dust protection is enabled. + pub nonce_cap_increment : u64, + /// Enable dust cleanup for contracts. + pub remove_dust_contracts : bool, } impl From for CommonParams { @@ -100,6 +106,9 @@ impl From for CommonParams { eip210_contract_gas: p.eip210_contract_gas.map_or(1000000.into(), Into::into), eip211_transition: p.eip211_transition.map_or(BlockNumber::max_value(), Into::into), eip214_transition: p.eip214_transition.map_or(BlockNumber::max_value(), Into::into), + dust_protection_transition: p.dust_protection_transition.map_or(BlockNumber::max_value(), Into::into), + nonce_cap_increment: p.nonce_cap_increment.map_or(64, Into::into), + remove_dust_contracts: p.remove_dust_contracts.unwrap_or(false), } } } @@ -224,7 +233,7 @@ impl Spec { ); } - let start_nonce = self.engine.account_start_nonce(); + let start_nonce = self.engine.account_start_nonce(0); let (root, db) = { let mut state = State::from_existing( @@ -460,7 +469,7 @@ mod tests { ::ethcore_logger::init_log(); let spec = Spec::new_test_constructor(); let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap(); - let state = State::from_existing(db.boxed_clone(), spec.state_root(), spec.engine.account_start_nonce(), Default::default()).unwrap(); + let state = State::from_existing(db.boxed_clone(), spec.state_root(), spec.engine.account_start_nonce(0), Default::default()).unwrap(); let expected = H256::from_str("0000000000000000000000000000000000000000000000000000000000000001").unwrap(); assert_eq!(state.storage_at(&Address::from_str("0000000000000000000000000000000000000005").unwrap(), &H256::zero()).unwrap(), expected); } diff --git a/ethcore/src/state/account.rs b/ethcore/src/state/account.rs index d8aad430a..205ba6af4 100644 --- a/ethcore/src/state/account.rs +++ b/ethcore/src/state/account.rs @@ -315,6 +315,11 @@ impl Account { self.code_hash == SHA3_EMPTY } + /// Check if account is basic (Has no code). + pub fn is_basic(&self) -> bool { + self.code_hash == SHA3_EMPTY + } + /// Return the storage root associated with this account or None if it has been altered via the overlay. pub fn storage_root(&self) -> Option<&H256> { if self.storage_is_clean() {Some(&self.storage_root)} else {None} } diff --git a/ethcore/src/state/mod.rs b/ethcore/src/state/mod.rs index 42289c3f8..501601fde 100644 --- a/ethcore/src/state/mod.rs +++ b/ethcore/src/state/mod.rs @@ -96,7 +96,11 @@ enum AccountState { /// Account entry can contain existing (`Some`) or non-existing /// account (`None`) struct AccountEntry { + /// Account entry. `None` if account known to be non-existant. account: Option, + /// Unmodified account balance. + old_balance: Option, + /// Entry state. state: AccountState, } @@ -107,6 +111,10 @@ impl AccountEntry { self.state == AccountState::Dirty } + fn exists_and_is_null(&self) -> bool { + self.account.as_ref().map_or(false, |a| a.is_null()) + } + /// Clone dirty data into new `AccountEntry`. This includes /// basic account data and modified storage keys. /// Returns None if clean. @@ -121,6 +129,7 @@ impl AccountEntry { /// basic account data and modified storage keys. fn clone_dirty(&self) -> AccountEntry { AccountEntry { + old_balance: self.old_balance, account: self.account.as_ref().map(Account::clone_dirty), state: self.state, } @@ -129,6 +138,7 @@ impl AccountEntry { // Create a new account entry and mark it as dirty. fn new_dirty(account: Option) -> AccountEntry { AccountEntry { + old_balance: account.as_ref().map(|a| a.balance().clone()), account: account, state: AccountState::Dirty, } @@ -137,6 +147,7 @@ impl AccountEntry { // Create a new account entry and mark it as clean. fn new_clean(account: Option) -> AccountEntry { AccountEntry { + old_balance: account.as_ref().map(|a| a.balance().clone()), account: account, state: AccountState::CleanFresh, } @@ -145,6 +156,7 @@ impl AccountEntry { // Create a new account entry and mark it as clean and cached. fn new_clean_cached(account: Option) -> AccountEntry { AccountEntry { + old_balance: account.as_ref().map(|a| a.balance().clone()), account: account, state: AccountState::CleanCached, } @@ -181,7 +193,7 @@ pub fn check_proof( let res = State::from_existing( backend, root, - engine.account_start_nonce(), + engine.account_start_nonce(env_info.number), factories ); @@ -266,8 +278,8 @@ pub enum CleanupMode<'a> { ForceCreate, /// Don't delete null accounts upon touching, but also don't create them. NoEmpty, - /// Add encountered null accounts to the provided kill-set, to be deleted later. - KillEmpty(&'a mut HashSet
), + /// Mark all touched accounts. + TrackTouched(&'a mut HashSet
), } const SEC_TRIE_DB_UNWRAP_STR: &'static str = "A state can only be created with valid root. Creating a SecTrieDB with a valid root will not fail. \ @@ -549,31 +561,30 @@ impl State { let is_value_transfer = !incr.is_zero(); if is_value_transfer || (cleanup_mode == CleanupMode::ForceCreate && !self.exists(a)?) { self.require(a, false)?.add_balance(incr); - } else { - match cleanup_mode { - CleanupMode::KillEmpty(set) => if !is_value_transfer && self.exists(a)? && !self.exists_and_not_null(a)? { - set.insert(a.clone()); - }, - _ => {} + } else if let CleanupMode::TrackTouched(set) = cleanup_mode { + if self.exists(a)? { + set.insert(*a); + self.touch(a)?; } } - Ok(()) } /// Subtract `decr` from the balance of account `a`. - pub fn sub_balance(&mut self, a: &Address, decr: &U256) -> trie::Result<()> { + pub fn sub_balance(&mut self, a: &Address, decr: &U256, cleanup_mode: &mut CleanupMode) -> trie::Result<()> { trace!(target: "state", "sub_balance({}, {}): {}", a, decr, self.balance(a)?); if !decr.is_zero() || !self.exists(a)? { self.require(a, false)?.sub_balance(decr); } - + if let CleanupMode::TrackTouched(ref mut set) = *cleanup_mode { + set.insert(*a); + } Ok(()) } /// Subtracts `by` from the balance of `from` and adds it to that of `to`. - pub fn transfer_balance(&mut self, from: &Address, to: &Address, by: &U256, cleanup_mode: CleanupMode) -> trie::Result<()> { - self.sub_balance(from, by)?; + pub fn transfer_balance(&mut self, from: &Address, to: &Address, by: &U256, mut cleanup_mode: CleanupMode) -> trie::Result<()> { + self.sub_balance(from, by, &mut cleanup_mode)?; self.add_balance(to, by, cleanup_mode)?; Ok(()) } @@ -640,33 +651,33 @@ impl State { } } - /// Commit accounts to SecTrieDBMut. This is similar to cpp-ethereum's dev::eth::commit. - /// `accounts` is mutable because we may need to commit the code or storage and record that. + fn touch(&mut self, a: &Address) -> trie::Result<()> { + self.require(a, false)?; + Ok(()) + } + + /// Commits our cached account changes into the trie. #[cfg_attr(feature="dev", allow(match_ref_pats))] #[cfg_attr(feature="dev", allow(needless_borrow))] - fn commit_into( - factories: &Factories, - db: &mut B, - root: &mut H256, - accounts: &mut HashMap - ) -> Result<(), Error> { + pub fn commit(&mut self) -> Result<(), Error> { // first, commit the sub trees. + let mut accounts = self.cache.borrow_mut(); for (address, ref mut a) in accounts.iter_mut().filter(|&(_, ref a)| a.is_dirty()) { if let Some(ref mut account) = a.account { let addr_hash = account.address_hash(address); { - let mut account_db = factories.accountdb.create(db.as_hashdb_mut(), addr_hash); - account.commit_storage(&factories.trie, account_db.as_hashdb_mut())?; + let mut account_db = self.factories.accountdb.create(self.db.as_hashdb_mut(), addr_hash); + account.commit_storage(&self.factories.trie, account_db.as_hashdb_mut())?; account.commit_code(account_db.as_hashdb_mut()); } if !account.is_empty() { - db.note_non_null_account(address); + self.db.note_non_null_account(address); } } } { - let mut trie = factories.trie.from_existing(db.as_hashdb_mut(), root)?; + let mut trie = self.factories.trie.from_existing(self.db.as_hashdb_mut(), &mut self.root)?; for (address, ref mut a) in accounts.iter_mut().filter(|&(_, ref a)| a.is_dirty()) { a.state = AccountState::Committed; match a.account { @@ -676,7 +687,7 @@ impl State { None => { trie.remove(address)?; }, - } + }; } } @@ -692,17 +703,30 @@ impl State { } } - /// Commits our cached account changes into the trie. - pub fn commit(&mut self) -> Result<(), Error> { - assert!(self.checkpoints.borrow().is_empty()); - Self::commit_into(&self.factories, &mut self.db, &mut self.root, &mut *self.cache.borrow_mut()) - } - /// Clear state cache pub fn clear(&mut self) { self.cache.borrow_mut().clear(); } + /// Remove any touched empty or dust accounts. + pub fn kill_garbage(&mut self, touched: &HashSet
, remove_empty_touched: bool, min_balance: &Option, kill_contracts: bool) -> trie::Result<()> { + let to_kill: HashSet<_> = { + self.cache.borrow().iter().filter_map(|(address, ref entry)| + if touched.contains(address) && // Check all touched accounts + ((remove_empty_touched && entry.exists_and_is_null()) // Remove all empty touched accounts. + || min_balance.map_or(false, |ref balance| entry.account.as_ref().map_or(false, |account| + (account.is_basic() || kill_contracts) // Remove all basic and optionally contract accounts where balance has been decreased. + && account.balance() < balance && entry.old_balance.as_ref().map_or(false, |b| account.balance() < b)))) { + + Some(address.clone()) + } else { None }).collect() + }; + for address in to_kill { + self.kill_account(&address); + } + Ok(()) + } + #[cfg(test)] #[cfg(feature = "json-tests")] /// Populate the state from `accounts`. @@ -1926,7 +1950,7 @@ mod tests { assert_eq!(state.balance(&a).unwrap(), U256::from(69u64)); state.commit().unwrap(); assert_eq!(state.balance(&a).unwrap(), U256::from(69u64)); - state.sub_balance(&a, &U256::from(42u64)).unwrap(); + state.sub_balance(&a, &U256::from(42u64), &mut CleanupMode::NoEmpty).unwrap(); assert_eq!(state.balance(&a).unwrap(), U256::from(27u64)); state.commit().unwrap(); assert_eq!(state.balance(&a).unwrap(), U256::from(27u64)); @@ -2026,4 +2050,44 @@ mod tests { new_state.diff_from(state).unwrap(); } + #[test] + fn should_kill_garbage() { + let a = 10.into(); + let b = 20.into(); + let c = 30.into(); + let d = 40.into(); + let e = 50.into(); + let x = 0.into(); + let db = get_temp_state_db(); + let (root, db) = { + let mut state = State::new(db, U256::from(0), Default::default()); + state.add_balance(&a, &U256::default(), CleanupMode::ForceCreate).unwrap(); // create an empty account + state.add_balance(&b, &100.into(), CleanupMode::ForceCreate).unwrap(); // create a dust account + state.add_balance(&c, &101.into(), CleanupMode::ForceCreate).unwrap(); // create a normal account + state.add_balance(&d, &99.into(), CleanupMode::ForceCreate).unwrap(); // create another dust account + state.new_contract(&e, 100.into(), 1.into()); // create a contract account + state.init_code(&e, vec![0x00]).unwrap(); + state.commit().unwrap(); + state.drop() + }; + + let mut state = State::from_existing(db, root, U256::from(0u8), Default::default()).unwrap(); + let mut touched = HashSet::new(); + state.add_balance(&a, &U256::default(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account + state.transfer_balance(&b, &x, &1.into(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account decreasing its balance + state.transfer_balance(&c, &x, &1.into(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account decreasing its balance + state.transfer_balance(&e, &x, &1.into(), CleanupMode::TrackTouched(&mut touched)).unwrap(); // touch an account decreasing its balance + state.kill_garbage(&touched, true, &None, false).unwrap(); + assert!(!state.exists(&a).unwrap()); + assert!(state.exists(&b).unwrap()); + state.kill_garbage(&touched, true, &Some(100.into()), false).unwrap(); + assert!(!state.exists(&b).unwrap()); + assert!(state.exists(&c).unwrap()); + assert!(state.exists(&d).unwrap()); + assert!(state.exists(&e).unwrap()); + state.kill_garbage(&touched, true, &Some(100.into()), true).unwrap(); + assert!(state.exists(&c).unwrap()); + assert!(state.exists(&d).unwrap()); + assert!(!state.exists(&e).unwrap()); + } } diff --git a/ethcore/src/state/substate.rs b/ethcore/src/state/substate.rs index 61824c898..76f6eaed7 100644 --- a/ethcore/src/state/substate.rs +++ b/ethcore/src/state/substate.rs @@ -18,7 +18,7 @@ use std::collections::HashSet; use util::{Address, U256}; use log_entry::LogEntry; -use evm::Schedule; +use evm::{Schedule, CleanDustMode}; use super::CleanupMode; /// State changes which should be applied in finalize, @@ -28,8 +28,8 @@ pub struct Substate { /// Any accounts that have suicided. pub suicides: HashSet
, - /// Any accounts that are tagged for garbage collection. - pub garbage: HashSet
, + /// Any accounts that are touched. + pub touched: HashSet
, /// Any logs. pub logs: Vec, @@ -50,7 +50,7 @@ impl Substate { /// Merge secondary substate `s` into self, accruing each element correspondingly. pub fn accrue(&mut self, s: Substate) { self.suicides.extend(s.suicides.into_iter()); - self.garbage.extend(s.garbage.into_iter()); + self.touched.extend(s.touched.into_iter()); self.logs.extend(s.logs.into_iter()); self.sstore_clears_count = self.sstore_clears_count + s.sstore_clears_count; self.contracts_created.extend(s.contracts_created.into_iter()); @@ -59,10 +59,10 @@ impl Substate { /// Get the cleanup mode object from this. #[cfg_attr(feature="dev", allow(wrong_self_convention))] pub fn to_cleanup_mode(&mut self, schedule: &Schedule) -> CleanupMode { - match (schedule.no_empty, schedule.kill_empty) { - (false, _) => CleanupMode::ForceCreate, - (true, false) => CleanupMode::NoEmpty, - (true, true) => CleanupMode::KillEmpty(&mut self.garbage), + match (schedule.kill_dust != CleanDustMode::Off, schedule.no_empty, schedule.kill_empty) { + (false, false, _) => CleanupMode::ForceCreate, + (false, true, false) => CleanupMode::NoEmpty, + (false, true, true) | (true, _, _,) => CleanupMode::TrackTouched(&mut self.touched), } } } diff --git a/ethcore/src/types/executed.rs b/ethcore/src/types/executed.rs index 571d8af97..37c8748aa 100644 --- a/ethcore/src/types/executed.rs +++ b/ethcore/src/types/executed.rs @@ -151,6 +151,8 @@ pub enum ExecutionError { }, /// When execution tries to modify the state in static context MutableCallInStaticContext, + /// Returned when transacting from a non-existing account with dust protection enabled. + SenderMustExist, /// Returned when internal evm error occurs. Internal(String), /// Returned when generic transaction occurs @@ -179,6 +181,7 @@ impl fmt::Display for ExecutionError { format!("Cost of transaction exceeds sender balance. {} is required \ but the sender only has {}", required, got), MutableCallInStaticContext => "Mutable Call in static context".to_owned(), + SenderMustExist => "Transacting from an empty account".to_owned(), Internal(ref msg) => msg.clone(), TransactionMalformed(ref err) => format!("Malformed transaction: {}", err), }; diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 096b348c3..0df4b86b6 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -80,10 +80,18 @@ pub fn verify_block_unordered(header: Header, bytes: Bytes, engine: &Engine, che } // Verify transactions. let mut transactions = Vec::new(); + let nonce_cap = if header.number() >= engine.params().dust_protection_transition { + Some((engine.params().nonce_cap_increment * header.number()).into()) + } else { None }; { let v = BlockView::new(&bytes); for t in v.transactions() { let t = engine.verify_transaction(t, &header)?; + if let Some(max_nonce) = nonce_cap { + if t.nonce >= max_nonce { + return Err(BlockError::TooManyTransactions(t.sender()).into()); + } + } transactions.push(t); } } @@ -391,6 +399,12 @@ mod tests { verify_block_family(&header, bytes, engine, bc) } + fn unordered_test(bytes: &[u8], engine: &Engine) -> Result<(), Error> { + let header = BlockView::new(bytes).header(); + verify_block_unordered(header, bytes.to_vec(), engine, false)?; + Ok(()) + } + #[test] #[cfg_attr(feature="dev", allow(similar_names))] fn test_verify_block() { @@ -556,4 +570,34 @@ mod tests { // TODO: some additional uncle checks } + + #[test] + fn dust_protection() { + use ethkey::{Generator, Random}; + use types::transaction::{Transaction, Action}; + use engines::NullEngine; + + let mut params = CommonParams::default(); + params.dust_protection_transition = 0; + params.nonce_cap_increment = 2; + + let mut header = Header::default(); + header.set_number(1); + + let keypair = Random.generate().unwrap(); + let bad_transactions: Vec<_> = (0..3).map(|i| Transaction { + action: Action::Create, + value: U256::zero(), + data: Vec::new(), + gas: 0.into(), + gas_price: U256::zero(), + nonce: i.into(), + }.sign(keypair.secret(), None)).collect(); + + let good_transactions = [bad_transactions[0].clone(), bad_transactions[1].clone()]; + + let engine = NullEngine::new(params, BTreeMap::new()); + check_fail(unordered_test(&create_test_block_with_data(&header, &bad_transactions, &[]), &engine), TooManyTransactions(keypair.address())); + unordered_test(&create_test_block_with_data(&header, &good_transactions, &[]), &engine).unwrap(); + } } diff --git a/json/src/spec/params.rs b/json/src/spec/params.rs index b17067426..8103515cb 100644 --- a/json/src/spec/params.rs +++ b/json/src/spec/params.rs @@ -81,6 +81,14 @@ pub struct Params { /// See `CommonParams` docs. #[serde(rename="eip214Transition")] pub eip214_transition: Option, + /// See `CommonParams` docs. + #[serde(rename="dustProtectionTransition")] + pub dust_protection_transition: Option, + /// See `CommonParams` docs. + #[serde(rename="nonceCapIncrement")] + pub nonce_cap_increment: Option, + /// See `CommonParams` docs. + pub remove_dust_contracts : Option, } #[cfg(test)] diff --git a/parity/dapps.rs b/parity/dapps.rs index b19eaebb6..7e1cf82c1 100644 --- a/parity/dapps.rs +++ b/parity/dapps.rs @@ -107,7 +107,7 @@ impl ContractClient for LightRegistrar { self.on_demand .request(ctx, on_demand::request::TransactionProof { tx: Transaction { - nonce: self.client.engine().account_start_nonce(), + nonce: self.client.engine().account_start_nonce(header.number()), action: Action::Call(address), gas: 50_000_000.into(), gas_price: 0.into(), diff --git a/parity/light_helpers/queue_cull.rs b/parity/light_helpers/queue_cull.rs index 090521ba5..982e69e48 100644 --- a/parity/light_helpers/queue_cull.rs +++ b/parity/light_helpers/queue_cull.rs @@ -67,7 +67,7 @@ impl IoHandler for QueueCull { let (sync, on_demand, txq) = (self.sync.clone(), self.on_demand.clone(), self.txq.clone()); let best_header = self.client.best_block_header(); - let start_nonce = self.client.engine().account_start_nonce(); + let start_nonce = self.client.engine().account_start_nonce(best_header.number()); info!(target: "cull", "Attempting to cull queued transactions from {} senders.", senders.len()); self.remote.spawn_with_timeout(move || { diff --git a/parity/run.rs b/parity/run.rs index bcb18aeeb..35515d54f 100644 --- a/parity/run.rs +++ b/parity/run.rs @@ -227,7 +227,8 @@ fn execute_light(cmd: RunCmd, can_restart: bool, logger: Arc) -> } // start on_demand service. - let on_demand = Arc::new(::light::on_demand::OnDemand::new(cache.clone())); + let account_start_nonce = service.client().engine().account_start_nonce(0); + let on_demand = Arc::new(::light::on_demand::OnDemand::new(cache.clone(), account_start_nonce)); // set network path. net_conf.net_config_path = Some(db_dirs.network_path().to_string_lossy().into_owned()); diff --git a/rpc/src/v1/helpers/dispatch.rs b/rpc/src/v1/helpers/dispatch.rs index be46a4763..ce978422d 100644 --- a/rpc/src/v1/helpers/dispatch.rs +++ b/rpc/src/v1/helpers/dispatch.rs @@ -284,7 +284,7 @@ impl LightDispatcher { } let best_header = self.client.best_block_header(); - let account_start_nonce = self.client.engine().account_start_nonce(); + let account_start_nonce = self.client.engine().account_start_nonce(best_header.number()); let nonce_future = self.sync.with_context(|ctx| self.on_demand.request(ctx, request::Account { header: best_header.into(), address: addr,