From e0ee06dcefb8fb852d338e183ba456b9ba96d524 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 05:02:38 +0800 Subject: [PATCH 01/14] Problem: uncle hash fetching is incorrect Solution: use another RPC get_uncle_by_block_number_and_index --- regtests/src/bin/main.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/regtests/src/bin/main.rs b/regtests/src/bin/main.rs index f9bdc188..3214805a 100644 --- a/regtests/src/bin/main.rs +++ b/regtests/src/bin/main.rs @@ -120,8 +120,9 @@ fn is_miner_or_uncle(address: Address, block: &RPCBlock, client: &mut GethRPCCli return true; } if block.uncles.len() > 0 { - for uncle_hash in &block.uncles { - let uncle = client.get_block_by_hash(&uncle_hash); + for i in 0..block.uncles.len() { + let uncle = client.get_uncle_by_block_number_and_index(&block.number, + &format!("0x{:x}", i)); let uncle_miner = Address::from_str(&uncle.miner).unwrap(); if uncle_miner == address { return true; From 7972f3eb9d85f0ad9e13b36fae4b022ce8399991 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 09:25:50 +0800 Subject: [PATCH 02/14] Problem: missing check return values in commit_account Solution: unwrap --- regtests/src/bin/main.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/regtests/src/bin/main.rs b/regtests/src/bin/main.rs index 3214805a..286469e3 100644 --- a/regtests/src/bin/main.rs +++ b/regtests/src/bin/main.rs @@ -82,7 +82,7 @@ fn handle_fire(client: &mut GethRPCClient, vm: &mut SeqTransactionVM, last_block address: address, balance: balance, code: code, - }); + }).unwrap(); }, Err(RequireError::AccountStorage(address, index)) => { println!("Feeding VM account storage at 0x{:x} with index 0x{:x} ...", address, index); @@ -93,7 +93,7 @@ fn handle_fire(client: &mut GethRPCClient, vm: &mut SeqTransactionVM, last_block address: address, index: index, value: value, - }); + }).unwrap(); }, Err(RequireError::AccountCode(address)) => { println!("Feeding VM account code at 0x{:x} ...", address); @@ -102,7 +102,7 @@ fn handle_fire(client: &mut GethRPCClient, vm: &mut SeqTransactionVM, last_block vm.commit_account(AccountCommitment::Code { address: address, code: code, - }); + }).unwrap(); } Err(err) => { println!("Unhandled require: {:?}", err); From 448950a4f0eb8ddbec5e93e670525fd173182e35 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 09:47:23 +0800 Subject: [PATCH 03/14] Problem: missing handling of complex account commitment fn Solution: handle situation when commit account but contains key --- sputnikvm/src/vm/commit/account.rs | 52 +++++++++++++++++++++++------- 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/sputnikvm/src/vm/commit/account.rs b/sputnikvm/src/vm/commit/account.rs index 6a7a6b9c..d43e9b37 100644 --- a/sputnikvm/src/vm/commit/account.rs +++ b/sputnikvm/src/vm/commit/account.rs @@ -159,17 +159,41 @@ impl AccountState { balance, code } => { - if self.accounts.contains_key(&address) { - return Err(CommitError::AlreadyCommitted); - } + let account = if self.accounts.contains_key(&address) { + match self.accounts.remove(&address).unwrap() { + Account::Full { .. } => return Err(CommitError::AlreadyCommitted), + Account::Create { .. } => return Err(CommitError::AlreadyCommitted), + Account::Remove(address) => Account::Remove(address), + Account::IncreaseBalance(address, topup) => { + Account::Full { + nonce, + address, + balance: balance + topup, + changing_storage: Storage::new(address, true), + code, + } + }, + Account::DecreaseBalance(address, withdraw) => { + Account::Full { + nonce, + address, + balance: balance - withdraw, + changing_storage: Storage::new(address, true), + code, + } + }, + } + } else { + Account::Full { + nonce, + address, + balance, + changing_storage: Storage::new(address, true), + code, + } + }; - self.accounts.insert(address, Account::Full { - nonce, - address, - balance, - changing_storage: Storage::new(address, true), - code, - }); + self.accounts.insert(address, account); }, AccountCommitment::Code { address, @@ -215,7 +239,13 @@ impl AccountState { ref code, .. } => return Ok(code.as_slice()), - _ => (), + &Account::Create { + ref code, + .. + } => return Ok(code.as_slice()), + &Account::Remove(_) => return Ok(&[]), + &Account::IncreaseBalance(address, _) => return Err(RequireError::Account(address)), + &Account::DecreaseBalance(address, _) => return Err(RequireError::Account(address)), } } From 62483d5747e15869fdbbdd52ef8f383bbcd94c6c Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 10:35:46 +0800 Subject: [PATCH 04/14] Problem: RPC is slow Solution: reuse connections to improve performance --- gethrpc/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gethrpc/src/lib.rs b/gethrpc/src/lib.rs index 2e0c2f3f..16e64bac 100644 --- a/gethrpc/src/lib.rs +++ b/gethrpc/src/lib.rs @@ -128,6 +128,7 @@ pub struct RPCFilter { pub struct GethRPCClient { endpoint: String, free_id: usize, + http: Client, } impl GethRPCClient { @@ -135,6 +136,7 @@ impl GethRPCClient { GethRPCClient { endpoint: endpoint.to_string(), free_id: 1, + http: Client::new(), } } @@ -151,8 +153,7 @@ impl GethRPCClient { }; self.free_id = self.free_id + 1; - let client = Client::new(); - let mut response_raw = client.post(&self.endpoint) + let mut response_raw = self.http.post(&self.endpoint) .header(ContentType::json()) .body(&serde_json::to_string(&request).unwrap()) .send().unwrap(); From 691eeff6d84ca72ffdff1df45731ff444d275cce Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 10:39:59 +0800 Subject: [PATCH 05/14] Problem: VM should eat all the gas when it fails Solution: ExitedErr(_) --- sputnikvm/src/vm/eval/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index 0dcf7a24..4f6a4a65 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -402,7 +402,7 @@ impl Machine { /// otherwise return the sum of memory gas and used gas. pub fn real_used_gas(&self) -> Gas { match self.status() { - MachineStatus::ExitedErr(MachineError::EmptyGas) => + MachineStatus::ExitedErr(_) => self.state.context.gas_limit, _ => self.state.memory_gas() + self.state.used_gas, } From cb85c91dcc7225845b0a7888c638767f1c1178d6 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 13:28:48 +0800 Subject: [PATCH 06/14] Problem: before HOMESTEAD, code deposit are forced Solution: add parameter force_code_deposit in patch --- sputnikvm/src/vm/eval/mod.rs | 6 +++++- sputnikvm/src/vm/patch.rs | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index 4f6a4a65..e5f3190e 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -174,7 +174,11 @@ impl Machine { let deposit_cost = code_deposit_gas(self.state.out.len()); if deposit_cost > self.state.available_gas() { - self.status = MachineStatus::ExitedErr(MachineError::EmptyGas); + if !self.state.patch.force_code_deposit { + self.status = MachineStatus::ExitedErr(MachineError::EmptyGas); + } else { + self.state.account_state.create(self.state.context.address, U256::zero(), &[]); + } } else { self.state.used_gas = self.state.used_gas + deposit_cost; self.state.account_state.create(self.state.context.address, U256::zero(), diff --git a/sputnikvm/src/vm/patch.rs b/sputnikvm/src/vm/patch.rs index 0ecc9bcc..affcada3 100644 --- a/sputnikvm/src/vm/patch.rs +++ b/sputnikvm/src/vm/patch.rs @@ -7,6 +7,7 @@ pub struct Patch { pub gas_call: usize, pub gas_expbyte: usize, pub gas_transaction_create: usize, + pub force_code_deposit: bool, } pub static FRONTIER_PATCH: Patch = Patch { @@ -18,6 +19,7 @@ pub static FRONTIER_PATCH: Patch = Patch { gas_call: 40, gas_expbyte: 10, gas_transaction_create: 0, + force_code_deposit: true, }; pub static HOMESTEAD_PATCH: Patch = Patch { @@ -29,6 +31,7 @@ pub static HOMESTEAD_PATCH: Patch = Patch { gas_call: 40, gas_expbyte: 10, gas_transaction_create: 32000, + force_code_deposit: false, }; pub static VMTEST_PATCH: Patch = Patch { @@ -40,6 +43,7 @@ pub static VMTEST_PATCH: Patch = Patch { gas_call: 40, gas_expbyte: 10, gas_transaction_create: 0, + force_code_deposit: true, }; pub static EIP150_PATCH: Patch = Patch { @@ -51,6 +55,7 @@ pub static EIP150_PATCH: Patch = Patch { gas_call: 700, gas_expbyte: 10, gas_transaction_create: 32000, + force_code_deposit: false, }; pub static EIP160_PATCH: Patch = Patch { @@ -62,4 +67,5 @@ pub static EIP160_PATCH: Patch = Patch { gas_call: 700, gas_expbyte: 50, gas_transaction_create: 32000, + force_code_deposit: false, }; From c25dda9dd7ffd054b42952be648289682483ea76 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 14:28:26 +0800 Subject: [PATCH 07/14] Problem: gas_limit and after_gas check potentially underflow Solution: calculate all_gas_cost first --- sputnikvm/src/vm/eval/mod.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index e5f3190e..05e6ef55 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -345,7 +345,14 @@ impl Machine { let gas_cost = gas_cost(instruction, &self.state); let gas_stipend = gas_stipend(instruction, &self.state); let gas_refund = gas_refund(instruction, &self.state); - let after_gas = self.state.context.gas_limit - memory_gas - self.state.used_gas - gas_cost + gas_stipend; + + let all_gas_cost = memory_gas + self.state.used_gas + gas_cost - gas_stipend; + if self.state.context.gas_limit < all_gas_cost { + self.status = MachineStatus::ExitedErr(MachineError::EmptyGas); + return Ok(()); + } + + let after_gas = self.state.context.gas_limit - all_gas_cost; match extra_check_opcode(instruction, &self.state, gas_stipend, after_gas) { Ok(()) => (), @@ -358,11 +365,6 @@ impl Machine { }, } - if self.state.context.gas_limit < memory_gas + self.state.used_gas + gas_cost - gas_stipend { - self.status = MachineStatus::ExitedErr(MachineError::EmptyGas); - return Ok(()); - } - let instruction = self.pc.read().unwrap(); let result = run_opcode((instruction, position), &mut self.state, gas_stipend, after_gas); From 84fe14d2c18d0078cc5e3f66d730047225a9ceeb Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 14:56:46 +0800 Subject: [PATCH 08/14] Problem: finalization should reset account state if error Solution: add fresh_account_state variable --- sputnikvm/src/vm/eval/mod.rs | 11 ++++++++++- sputnikvm/src/vm/transaction.rs | 5 ++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index 05e6ef55..4100bcaf 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -187,7 +187,16 @@ impl Machine { Ok(()) } - pub fn finalize(&mut self, real_used_gas: Gas) -> Result<(), RequireError> { + pub fn finalize(&mut self, real_used_gas: Gas, fresh_account_state: &AccountState) -> Result<(), RequireError> { + match self.status() { + MachineStatus::ExitedOk => (), + MachineStatus::ExitedErr(_) => { + // If exited with error, reset all changes. + self.state.account_state = fresh_account_state.clone(); + }, + _ => panic!(), + } + let gas_dec = real_used_gas * self.state.context.gas_price; self.state.account_state.decrease_balance(self.state.context.caller, gas_dec.into()); diff --git a/sputnikvm/src/vm/transaction.rs b/sputnikvm/src/vm/transaction.rs index 8883fbb0..59cb4974 100644 --- a/sputnikvm/src/vm/transaction.rs +++ b/sputnikvm/src/vm/transaction.rs @@ -115,6 +115,7 @@ enum TransactionVMState { intrinsic_gas: Gas, finalized: bool, code_deposit: bool, + fresh_account_state: AccountState, }, Constructing { transaction: Transaction, @@ -206,6 +207,7 @@ impl VM for TransactionVM { ref mut finalized, ref mut code_deposit, intrinsic_gas, + ref fresh_account_state, .. } => { match vm.status() { @@ -221,7 +223,7 @@ impl VM for TransactionVM { if !*finalized { let real_used_gas = vm.real_used_gas() + intrinsic_gas; - vm.machines[0].finalize(real_used_gas)?; + vm.machines[0].finalize(real_used_gas, fresh_account_state)?; *finalized = true; return Ok(()); } @@ -248,6 +250,7 @@ impl VM for TransactionVM { } self.0 = TransactionVMState::Running { + fresh_account_state: caccount_state.as_ref().unwrap().clone(), vm: ContextVM::with_states(ccontext.unwrap(), cblock.unwrap(), cpatch.unwrap(), caccount_state.unwrap(), cblockhash_state.unwrap()), intrinsic_gas: cgas.unwrap(), From 06acf319e7dfad78ce3bf04a0016e64ce0bcd03b Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 16:32:18 +0800 Subject: [PATCH 09/14] Problem: value should be transferred before EVM is run Solution: handle transfer when creating new machines --- sputnikvm/src/vm/eval/check.rs | 11 ---------- sputnikvm/src/vm/eval/mod.rs | 37 +++++++++++++++++++--------------- 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/sputnikvm/src/vm/eval/check.rs b/sputnikvm/src/vm/eval/check.rs index eedb78a4..8cf0a4c5 100644 --- a/sputnikvm/src/vm/eval/check.rs +++ b/sputnikvm/src/vm/eval/check.rs @@ -9,17 +9,6 @@ use vm::errors::{MachineError, EvalError}; use vm::eval::{State, ControlCheck}; use super::utils::{check_range, check_memory_write_range}; -const CALLSTACK_LIMIT_DEFAULT: usize = 1024; -const CALLSTACK_LIMIT_TEST: usize = 2; - -fn check_callstack_overflow(state: &State) -> Result<(), MachineError> { - if state.depth >= state.patch.callstack_limit { - return Err(MachineError::CallstackOverflow); - } else { - return Ok(()); - } -} - pub fn extra_check_opcode(instruction: Instruction, state: &State, stipend_gas: Gas, after_gas: Gas) -> Result<(), EvalError> { match instruction { Instruction::CALL => { diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index 4100bcaf..c06d9c8e 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -97,8 +97,11 @@ impl Machine { } pub fn with_states(context: Context, block: BlockHeader, patch: &'static Patch, - depth: usize, account_state: AccountState, + depth: usize, mut account_state: AccountState, blockhash_state: BlockhashState) -> Self { + account_state.decrease_balance(context.caller, context.value); + account_state.increase_balance(context.address, context.value); + Machine { pc: PC::new(context.code.as_slice()), status: MachineStatus::Running, @@ -130,6 +133,11 @@ impl Machine { /// review whether it wants to accept the result of this sub /// runtime afterwards. pub fn derive(&self, context: Context) -> Self { + let mut account_state = self.state.account_state.clone(); + + account_state.decrease_balance(context.caller, context.value); + account_state.increase_balance(context.address, context.value); + Machine { pc: PC::new(context.code.as_slice()), status: MachineStatus::Running, @@ -147,7 +155,7 @@ impl Machine { used_gas: Gas::zero(), refunded_gas: Gas::zero(), - account_state: self.state.account_state.clone(), + account_state, blockhash_state: self.state.blockhash_state.clone(), logs: self.state.logs.clone(), @@ -206,11 +214,6 @@ impl Machine { _ => panic!(), } - self.state.account_state.decrease_balance(self.state.context.caller, - self.state.context.value); - self.state.account_state.increase_balance(self.state.context.address, - self.state.context.value); - Ok(()) } @@ -248,11 +251,15 @@ impl Machine { self.state.used_gas = self.state.used_gas + sub.state.used_gas; self.state.refunded_gas = self.state.refunded_gas + sub.state.refunded_gas; if self.state.available_gas() >= code_deposit_gas(sub.state.out.len()) { - self.state.account_state.decrease_balance(self.state.context.address, - sub.state.context.value); + self.state.account_state.decrease_balance(sub.state.context.caller, + code_deposit_gas(sub.state.out.len()).into()); self.state.account_state.create(sub.state.context.address, - sub.state.context.value, + U256::zero(), sub.state.out.as_slice()); + } else { + self.state.account_state.create(sub.state.context.address, + U256::zero(), + &[]); } }, @@ -277,10 +284,6 @@ impl Machine { self.state.logs = sub.state.logs; self.state.used_gas = self.state.used_gas + sub.state.used_gas; self.state.refunded_gas = self.state.refunded_gas + sub.state.refunded_gas; - self.state.account_state.decrease_balance(self.state.context.address, - sub.state.context.value); - self.state.account_state.increase_balance(sub.state.context.address, - sub.state.context.value); copy_into_memory(&mut self.state.memory, sub.state.out.as_slice(), out_start, M256::zero(), out_len); }, @@ -326,7 +329,7 @@ impl Machine { return Ok(()); } - if self.state.depth >= 2 { + if self.state.depth >= self.state.patch.callstack_limit { self.status = MachineStatus::ExitedErr(MachineError::CallstackOverflow); return Ok(()); } @@ -419,7 +422,9 @@ impl Machine { match self.status() { MachineStatus::ExitedErr(_) => self.state.context.gas_limit, - _ => self.state.memory_gas() + self.state.used_gas, + MachineStatus::ExitedOk => + self.state.memory_gas() + self.state.used_gas - self.state.refunded_gas, + _ => Gas::zero(), } } } From f4f00ec1e2ebd8f8e90f8f984ef80ed5b67c98c0 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 20:37:53 +0800 Subject: [PATCH 10/14] Problem: account commitment management for create is incomplete Solution: handle situaions where the key is already there --- sputnikvm/src/vm/commit/account.rs | 34 +++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/sputnikvm/src/vm/commit/account.rs b/sputnikvm/src/vm/commit/account.rs index d43e9b37..34193bbf 100644 --- a/sputnikvm/src/vm/commit/account.rs +++ b/sputnikvm/src/vm/commit/account.rs @@ -327,9 +327,37 @@ impl AccountState { /// Create a new account (that should not yet have existed /// before). pub fn create(&mut self, address: Address, balance: U256, code: &[u8]) { - self.accounts.insert(address, Account::Full { - address, balance, changing_storage: Storage::new(address, false), code: code.into(), nonce: M256::zero(), - }); + let account = if self.accounts.contains_key(&address) { + match self.accounts.remove(&address).unwrap() { + Account::Full { .. } => panic!(), + Account::Create { .. } => panic!(), + Account::Remove(address) => { + Account::Create { + address, balance, code: code.into(), nonce: M256::zero(), + storage: Storage::new(address, false) + } + }, + Account::IncreaseBalance(address, topup) => { + Account::Create { + address, code: code.into(), nonce: M256::zero(), + balance: balance + topup, storage: Storage::new(address, false) + } + }, + Account::DecreaseBalance(address, withdraw) => { + Account::Create { + address, code: code.into(), nonce: M256::zero(), + balance: balance - withdraw, storage: Storage::new(address, false) + } + }, + } + } else { + Account::Create { + address, balance, code: code.into(), nonce: M256::zero(), + storage: Storage::new(address, false) + } + }; + + self.accounts.insert(address, account); } /// Increase the balance of an account. From 3447a65e4fc907a9ef94ecec5afb10fadfcea968 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Thu, 8 Jun 2017 21:06:28 +0800 Subject: [PATCH 11/14] Problem: account should be created in prior to EVM execution Solution: add create boolean to Context --- sputnikvm/src/vm/commit/account.rs | 46 ++++++++++++++++++++++++++---- sputnikvm/src/vm/eval/mod.rs | 29 +++++++++++-------- sputnikvm/src/vm/params.rs | 1 + sputnikvm/src/vm/transaction.rs | 2 ++ 4 files changed, 60 insertions(+), 18 deletions(-) diff --git a/sputnikvm/src/vm/commit/account.rs b/sputnikvm/src/vm/commit/account.rs index 34193bbf..af361e4e 100644 --- a/sputnikvm/src/vm/commit/account.rs +++ b/sputnikvm/src/vm/commit/account.rs @@ -127,6 +127,8 @@ impl AccountState { pub fn require(&self, address: Address) -> Result<(), RequireError> { match self.accounts.get(&address) { Some(&Account::Full { .. }) => return Ok(()), + Some(&Account::Create { .. }) => return Ok(()), + Some(&Account::Remove(_)) => panic!(), _ => return Err(RequireError::Account(address)), } } @@ -140,6 +142,8 @@ impl AccountState { } match self.accounts.get(&address) { Some(&Account::Full { .. }) => return Ok(()), + Some(&Account::Create { .. }) => return Ok(()), + Some(&Account::Remove(_)) => panic!(), _ => return Err(RequireError::AccountCode(address)), } } @@ -243,7 +247,7 @@ impl AccountState { ref code, .. } => return Ok(code.as_slice()), - &Account::Remove(_) => return Ok(&[]), + &Account::Remove(_) => panic!(), &Account::IncreaseBalance(address, _) => return Err(RequireError::Account(address)), &Account::DecreaseBalance(address, _) => return Err(RequireError::Account(address)), } @@ -261,6 +265,11 @@ impl AccountState { nonce, .. } => return Ok(nonce), + &Account::Create { + nonce, + .. + } => return Ok(nonce), + &Account::Remove(_) => panic!(), _ => (), } } @@ -277,6 +286,11 @@ impl AccountState { balance, .. } => return Ok(balance), + &Account::Create { + balance, + .. + } => return Ok(balance), + &Account::Remove(_) => return Ok(U256::zero()), _ => (), } } @@ -297,6 +311,7 @@ impl AccountState { ref storage, .. } => return Ok(storage), + &Account::Remove(_) => panic!(), _ => (), } } @@ -317,6 +332,7 @@ impl AccountState { ref mut storage, .. } => return Ok(storage), + &mut Account::Remove(_) => panic!(), _ => (), } } @@ -326,33 +342,33 @@ impl AccountState { /// Create a new account (that should not yet have existed /// before). - pub fn create(&mut self, address: Address, balance: U256, code: &[u8]) { + pub fn create(&mut self, address: Address, balance: U256) { let account = if self.accounts.contains_key(&address) { match self.accounts.remove(&address).unwrap() { Account::Full { .. } => panic!(), Account::Create { .. } => panic!(), Account::Remove(address) => { Account::Create { - address, balance, code: code.into(), nonce: M256::zero(), + address, balance, code: Vec::new(), nonce: M256::zero(), storage: Storage::new(address, false) } }, Account::IncreaseBalance(address, topup) => { Account::Create { - address, code: code.into(), nonce: M256::zero(), + address, code: Vec::new(), nonce: M256::zero(), balance: balance + topup, storage: Storage::new(address, false) } }, Account::DecreaseBalance(address, withdraw) => { Account::Create { - address, code: code.into(), nonce: M256::zero(), + address, code: Vec::new(), nonce: M256::zero(), balance: balance - withdraw, storage: Storage::new(address, false) } }, } } else { Account::Create { - address, balance, code: code.into(), nonce: M256::zero(), + address, balance, code: Vec::new(), nonce: M256::zero(), storage: Storage::new(address, false) } }; @@ -360,6 +376,16 @@ impl AccountState { self.accounts.insert(address, account); } + /// Deposit code in to a created account. + pub fn code_deposit(&mut self, address: Address, new_code: &[u8]) { + match self.accounts.get_mut(&address).unwrap() { + &mut Account::Create { ref mut code, .. } => { + *code = new_code.into(); + }, + _ => panic!(), + } + } + /// Increase the balance of an account. pub fn increase_balance(&mut self, address: Address, topup: U256) { if topup == U256::zero() { return; } @@ -487,6 +513,14 @@ impl AccountState { *nonce = new_nonce; Ok(()) }, + Some(&mut Account::Create { + ref mut nonce, + .. + }) => { + *nonce = new_nonce; + Ok(()) + }, + Some(&mut Account::Remove(_)) => panic!(), _ => { Err(RequireError::Account(address)) }, diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index c06d9c8e..163e8469 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -1,5 +1,5 @@ //! VM Runtime -use utils::bigint::{U256, M256}; +use utils::bigint::M256; use utils::gas::Gas; use super::commit::{AccountState, BlockhashState}; use super::errors::{RequireError, MachineError, CommitError, EvalError, PCError}; @@ -100,7 +100,11 @@ impl Machine { depth: usize, mut account_state: AccountState, blockhash_state: BlockhashState) -> Self { account_state.decrease_balance(context.caller, context.value); - account_state.increase_balance(context.address, context.value); + if context.create { + account_state.create(context.address, context.value); + } else { + account_state.increase_balance(context.address, context.value); + } Machine { pc: PC::new(context.code.as_slice()), @@ -136,7 +140,11 @@ impl Machine { let mut account_state = self.state.account_state.clone(); account_state.decrease_balance(context.caller, context.value); - account_state.increase_balance(context.address, context.value); + if context.create { + account_state.create(context.address, context.value); + } else { + account_state.increase_balance(context.address, context.value); + } Machine { pc: PC::new(context.code.as_slice()), @@ -185,12 +193,12 @@ impl Machine { if !self.state.patch.force_code_deposit { self.status = MachineStatus::ExitedErr(MachineError::EmptyGas); } else { - self.state.account_state.create(self.state.context.address, U256::zero(), &[]); + self.state.account_state.code_deposit(self.state.context.address, &[]); } } else { self.state.used_gas = self.state.used_gas + deposit_cost; - self.state.account_state.create(self.state.context.address, U256::zero(), - self.state.out.as_slice()); + self.state.account_state.code_deposit(self.state.context.address, + self.state.out.as_slice()); } Ok(()) } @@ -253,13 +261,10 @@ impl Machine { if self.state.available_gas() >= code_deposit_gas(sub.state.out.len()) { self.state.account_state.decrease_balance(sub.state.context.caller, code_deposit_gas(sub.state.out.len()).into()); - self.state.account_state.create(sub.state.context.address, - U256::zero(), - sub.state.out.as_slice()); + self.state.account_state.code_deposit(sub.state.context.address, + sub.state.out.as_slice()); } else { - self.state.account_state.create(sub.state.context.address, - U256::zero(), - &[]); + self.state.account_state.code_deposit(sub.state.context.address, &[]); } }, diff --git a/sputnikvm/src/vm/params.rs b/sputnikvm/src/vm/params.rs index 57c44379..ea040a3b 100644 --- a/sputnikvm/src/vm/params.rs +++ b/sputnikvm/src/vm/params.rs @@ -25,6 +25,7 @@ pub struct Context { pub gas_price: Gas, pub origin: Address, pub value: U256, + pub create: bool, } #[derive(Debug, Clone, PartialEq)] diff --git a/sputnikvm/src/vm/transaction.rs b/sputnikvm/src/vm/transaction.rs index 59cb4974..be6b9864 100644 --- a/sputnikvm/src/vm/transaction.rs +++ b/sputnikvm/src/vm/transaction.rs @@ -74,6 +74,7 @@ impl Transaction { gas_limit: gas_limit - upfront, code: account_state.code(address)?.into(), origin: origin.unwrap_or(caller), + create: false, }) }, Transaction::ContractCreation { @@ -96,6 +97,7 @@ impl Transaction { data: Vec::new(), code: init, origin: origin.unwrap_or(caller), + create: true, }) } } From e7f67ae49a0416e3d86372a5249dddbd03c1d2d7 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 9 Jun 2017 00:38:10 +0800 Subject: [PATCH 12/14] Problem: incorrect refund gas calculation Solution: fix refund gas cap --- sputnikvm/src/vm/commit/account.rs | 7 +++++++ sputnikvm/src/vm/eval/cost.rs | 8 ++++++-- sputnikvm/src/vm/eval/mod.rs | 12 ----------- sputnikvm/src/vm/mod.rs | 7 ------- sputnikvm/src/vm/transaction.rs | 33 +++++++++++++++++++++--------- 5 files changed, 36 insertions(+), 31 deletions(-) diff --git a/sputnikvm/src/vm/commit/account.rs b/sputnikvm/src/vm/commit/account.rs index af361e4e..f448b2f9 100644 --- a/sputnikvm/src/vm/commit/account.rs +++ b/sputnikvm/src/vm/commit/account.rs @@ -122,6 +122,13 @@ impl AccountState { self.accounts.values() } + pub fn is_removed(&self, address: Address) -> bool { + match self.accounts.get(&address) { + Some(&Account::Remove(_)) => true, + _ => false, + } + } + /// Returns Ok(()) if a full account is in this account /// state. Otherwise raise a `RequireError`. pub fn require(&self, address: Address) -> Result<(), RequireError> { diff --git a/sputnikvm/src/vm/eval/cost.rs b/sputnikvm/src/vm/eval/cost.rs index ae5c3420..4445f5bf 100644 --- a/sputnikvm/src/vm/eval/cost.rs +++ b/sputnikvm/src/vm/eval/cost.rs @@ -273,8 +273,12 @@ pub fn gas_refund(instruction: Instruction, state: &State { - // TODO: check whether I_a belongs to A_s - Gas::zero() + let address: Address = state.stack.peek(0).unwrap().into(); + if state.account_state.is_removed(address) { + Gas::zero() + } else { + Gas::from(R_SUICIDE) + } }, _ => Gas::zero() } diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index 163e8469..acaa16a7 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -420,16 +420,4 @@ impl Machine { pub fn status(&self) -> MachineStatus { self.status.clone() } - - /// If the machine is exited with EmptyGas, return gas_limit, - /// otherwise return the sum of memory gas and used gas. - pub fn real_used_gas(&self) -> Gas { - match self.status() { - MachineStatus::ExitedErr(_) => - self.state.context.gas_limit, - MachineStatus::ExitedOk => - self.state.memory_gas() + self.state.used_gas - self.state.refunded_gas, - _ => Gas::zero(), - } - } } diff --git a/sputnikvm/src/vm/mod.rs b/sputnikvm/src/vm/mod.rs index 61939788..609ed979 100644 --- a/sputnikvm/src/vm/mod.rs +++ b/sputnikvm/src/vm/mod.rs @@ -66,7 +66,6 @@ pub trait VM { fn accounts(&self) -> hash_map::Values; fn out(&self) -> &[u8]; fn available_gas(&self) -> Gas; - fn real_used_gas(&self) -> Gas; fn refunded_gas(&self) -> Gas; fn logs(&self) -> &[Log]; } @@ -199,12 +198,6 @@ impl VM for ContextVM { self.machines[0].state().available_gas() } - /// If the VM is exited with EmptyGas, return gas_limit, - /// otherwise return the sum of memory gas and used gas. - fn real_used_gas(&self) -> Gas { - self.machines[0].real_used_gas() - } - /// Returns the refunded gas of this VM. fn refunded_gas(&self) -> Gas { self.machines[0].state().refunded_gas diff --git a/sputnikvm/src/vm/transaction.rs b/sputnikvm/src/vm/transaction.rs index be6b9864..f9e887e0 100644 --- a/sputnikvm/src/vm/transaction.rs +++ b/sputnikvm/src/vm/transaction.rs @@ -1,4 +1,5 @@ use std::collections::hash_map; +use std::cmp::min; use utils::gas::Gas; use utils::address::Address; use utils::bigint::{U256, M256}; @@ -7,7 +8,7 @@ use tiny_keccak::Keccak; use super::errors::{RequireError, CommitError}; use super::{Context, ContextVM, VM, AccountState, BlockhashState, Patch, BlockHeader, Memory, - VMStatus, AccountCommitment, Log, Account}; + VMStatus, AccountCommitment, Log, Account, MachineStatus}; const G_TXDATAZERO: usize = 4; const G_TXDATANONZERO: usize = 68; @@ -164,6 +165,25 @@ impl TransactionVM { }, }) } + + pub fn real_used_gas(&self) -> Gas { + match self.0 { + TransactionVMState::Running { ref vm, intrinsic_gas, .. } => { + match vm.machines[0].status() { + MachineStatus::ExitedErr(_) => + vm.machines[0].state().context.gas_limit + intrinsic_gas, + MachineStatus::ExitedOk => { + let total_used = vm.machines[0].state().memory_gas() + vm.machines[0].state().used_gas + intrinsic_gas; + let refund_cap = total_used / Gas::from(2u64); + let refunded = min(refund_cap, vm.machines[0].state().refunded_gas); + total_used - refunded + } + _ => Gas::zero(), + } + } + TransactionVMState::Constructing { .. } => Gas::zero(), + } + } } impl VM for TransactionVM { @@ -203,12 +223,13 @@ impl VM for TransactionVM { let mut cblockhash_state: Option = None; let mut ccode_deposit: Option = None; + let real_used_gas = self.real_used_gas(); + match self.0 { TransactionVMState::Running { ref mut vm, ref mut finalized, ref mut code_deposit, - intrinsic_gas, ref fresh_account_state, .. } => { @@ -224,7 +245,6 @@ impl VM for TransactionVM { } if !*finalized { - let real_used_gas = vm.real_used_gas() + intrinsic_gas; vm.machines[0].finalize(real_used_gas, fresh_account_state)?; *finalized = true; return Ok(()); @@ -284,13 +304,6 @@ impl VM for TransactionVM { } } - fn real_used_gas(&self) -> Gas { - match self.0 { - TransactionVMState::Running { ref vm, intrinsic_gas, .. } => vm.real_used_gas() + intrinsic_gas, - TransactionVMState::Constructing { .. } => Gas::zero(), - } - } - fn refunded_gas(&self) -> Gas { match self.0 { TransactionVMState::Running { ref vm, .. } => vm.refunded_gas(), From c8cf118e5d936282d2525ce2b115193312f1ee97 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 9 Jun 2017 00:49:32 +0800 Subject: [PATCH 13/14] Problem: cargo test failed Solution: add create field for Context in jsontests --- jsontests/src/blockchain.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/jsontests/src/blockchain.rs b/jsontests/src/blockchain.rs index 5e99879a..70294bfa 100644 --- a/jsontests/src/blockchain.rs +++ b/jsontests/src/blockchain.rs @@ -267,5 +267,6 @@ pub fn create_context(v: &Value) -> Context { gas_price: gas_price, origin: origin, value: value, + create: false, } } From 9c3cf674767fa29d8d3cb90b2246b4c897b6d0fd Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Fri, 9 Jun 2017 00:56:51 +0800 Subject: [PATCH 14/14] Problem: jsontests requires None value transfer Solution: add ExecutionMode::None --- jsontests/src/blockchain.rs | 4 ++-- sputnikvm/src/vm/eval/mod.rs | 35 ++++++++++++++++++++++----------- sputnikvm/src/vm/params.rs | 7 ++++++- sputnikvm/src/vm/transaction.rs | 6 +++--- 4 files changed, 35 insertions(+), 17 deletions(-) diff --git a/jsontests/src/blockchain.rs b/jsontests/src/blockchain.rs index 70294bfa..d031987b 100644 --- a/jsontests/src/blockchain.rs +++ b/jsontests/src/blockchain.rs @@ -1,7 +1,7 @@ use sputnikvm::{Gas, M256, U256, Address, read_hex}; use sputnikvm::vm::{Machine, Log, Context, Account, Storage, AccountCommitment, - BlockHeader}; + BlockHeader, ExecutionMode}; use serde_json::Value; use std::collections::HashMap; @@ -267,6 +267,6 @@ pub fn create_context(v: &Value) -> Context { gas_price: gas_price, origin: origin, value: value, - create: false, + mode: ExecutionMode::None, } } diff --git a/sputnikvm/src/vm/eval/mod.rs b/sputnikvm/src/vm/eval/mod.rs index acaa16a7..30b35416 100644 --- a/sputnikvm/src/vm/eval/mod.rs +++ b/sputnikvm/src/vm/eval/mod.rs @@ -3,7 +3,7 @@ use utils::bigint::M256; use utils::gas::Gas; use super::commit::{AccountState, BlockhashState}; use super::errors::{RequireError, MachineError, CommitError, EvalError, PCError}; -use super::{Stack, Context, BlockHeader, Patch, PC, Memory, AccountCommitment, Log}; +use super::{Stack, Context, BlockHeader, Patch, PC, Memory, AccountCommitment, Log, ExecutionMode}; use self::check::{check_opcode, extra_check_opcode}; use self::run::run_opcode; @@ -99,11 +99,16 @@ impl Machine { pub fn with_states(context: Context, block: BlockHeader, patch: &'static Patch, depth: usize, mut account_state: AccountState, blockhash_state: BlockhashState) -> Self { - account_state.decrease_balance(context.caller, context.value); - if context.create { - account_state.create(context.address, context.value); - } else { - account_state.increase_balance(context.address, context.value); + match context.mode { + ExecutionMode::Call => { + account_state.decrease_balance(context.caller, context.value); + account_state.increase_balance(context.address, context.value); + }, + ExecutionMode::Create => { + account_state.decrease_balance(context.caller, context.value); + account_state.create(context.address, context.value); + }, + ExecutionMode::None => (), } Machine { @@ -139,11 +144,16 @@ impl Machine { pub fn derive(&self, context: Context) -> Self { let mut account_state = self.state.account_state.clone(); - account_state.decrease_balance(context.caller, context.value); - if context.create { - account_state.create(context.address, context.value); - } else { - account_state.increase_balance(context.address, context.value); + match context.mode { + ExecutionMode::Call => { + account_state.decrease_balance(context.caller, context.value); + account_state.increase_balance(context.address, context.value); + }, + ExecutionMode::Create => { + account_state.decrease_balance(context.caller, context.value); + account_state.create(context.address, context.value); + }, + ExecutionMode::None => (), } Machine { @@ -183,6 +193,7 @@ impl Machine { } pub fn code_deposit(&mut self) -> Result<(), RequireError> { + assert!(self.state.context.mode == ExecutionMode::Create); match self.status() { MachineStatus::ExitedOk | MachineStatus::ExitedErr(_) => (), _ => panic!(), @@ -204,6 +215,8 @@ impl Machine { } pub fn finalize(&mut self, real_used_gas: Gas, fresh_account_state: &AccountState) -> Result<(), RequireError> { + assert!(self.state.context.mode == ExecutionMode::Call || + self.state.context.mode == ExecutionMode::Create); match self.status() { MachineStatus::ExitedOk => (), MachineStatus::ExitedErr(_) => { diff --git a/sputnikvm/src/vm/params.rs b/sputnikvm/src/vm/params.rs index ea040a3b..03739723 100644 --- a/sputnikvm/src/vm/params.rs +++ b/sputnikvm/src/vm/params.rs @@ -14,6 +14,11 @@ pub struct BlockHeader { pub gas_limit: Gas } +#[derive(Copy, Debug, Clone, PartialEq, Eq)] +pub enum ExecutionMode { + Call, Create, None +} + #[derive(Debug, Clone)] /// A VM context. See the Yellow Paper for more information. pub struct Context { @@ -25,7 +30,7 @@ pub struct Context { pub gas_price: Gas, pub origin: Address, pub value: U256, - pub create: bool, + pub mode: ExecutionMode, } #[derive(Debug, Clone, PartialEq)] diff --git a/sputnikvm/src/vm/transaction.rs b/sputnikvm/src/vm/transaction.rs index f9e887e0..0f71c496 100644 --- a/sputnikvm/src/vm/transaction.rs +++ b/sputnikvm/src/vm/transaction.rs @@ -8,7 +8,7 @@ use tiny_keccak::Keccak; use super::errors::{RequireError, CommitError}; use super::{Context, ContextVM, VM, AccountState, BlockhashState, Patch, BlockHeader, Memory, - VMStatus, AccountCommitment, Log, Account, MachineStatus}; + VMStatus, AccountCommitment, Log, Account, MachineStatus, ExecutionMode}; const G_TXDATAZERO: usize = 4; const G_TXDATANONZERO: usize = 68; @@ -75,7 +75,7 @@ impl Transaction { gas_limit: gas_limit - upfront, code: account_state.code(address)?.into(), origin: origin.unwrap_or(caller), - create: false, + mode: ExecutionMode::Call, }) }, Transaction::ContractCreation { @@ -98,7 +98,7 @@ impl Transaction { data: Vec::new(), code: init, origin: origin.unwrap_or(caller), - create: true, + mode: ExecutionMode::Create, }) } }