From 5319d33bc6b22ac3ca06e561912c6cc4ca472e26 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 15 Oct 2018 17:09:55 +0800 Subject: [PATCH] Use signed 256-bit integer for sstore gas refund substate (#9746) * Add signed refund * Use signed 256-bit integer for sstore gas refund substate * Fix tests * Remove signed mod and use i128 directly * Fix evm test case casting * Fix jsontests ext signature --- ethcore/evm/src/interpreter/gasometer.rs | 6 +++--- ethcore/evm/src/interpreter/mod.rs | 2 +- ethcore/evm/src/tests.rs | 2 +- ethcore/src/executive.rs | 7 ++++--- ethcore/src/externalities.rs | 8 ++++---- ethcore/src/json_tests/executive.rs | 4 ++-- ethcore/src/state/substate.rs | 6 +++--- ethcore/vm/src/ext.rs | 4 ++-- ethcore/vm/src/tests.rs | 10 +++++----- ethcore/wasm/src/runtime.rs | 2 +- 10 files changed, 26 insertions(+), 25 deletions(-) diff --git a/ethcore/evm/src/interpreter/gasometer.rs b/ethcore/evm/src/interpreter/gasometer.rs index 524791fe9..60ea30da0 100644 --- a/ethcore/evm/src/interpreter/gasometer.rs +++ b/ethcore/evm/src/interpreter/gasometer.rs @@ -403,7 +403,7 @@ fn calculate_eip1283_sstore_gas(schedule: &Schedule, origina } pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, current: &U256, new: &U256) { - let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); + let sstore_clears_schedule = ext.schedule().sstore_refund_gas; if current == new { // 1. If current value equals new value (this is a no-op), 200 gas is deducted. @@ -438,11 +438,11 @@ pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, c // 2.2.2. If original value equals new value (this storage slot is reset) if original.is_zero() { // 2.2.2.1. If original value is 0, add 19800 gas to refund counter. - let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas); + let refund = ext.schedule().sstore_set_gas - ext.schedule().sload_gas; ext.add_sstore_refund(refund); } else { // 2.2.2.2. Otherwise, add 4800 gas to refund counter. - let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas); + let refund = ext.schedule().sstore_reset_gas - ext.schedule().sload_gas; ext.add_sstore_refund(refund); } } diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index d16615320..87f6aac81 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -720,7 +720,7 @@ impl Interpreter { gasometer::handle_eip1283_sstore_clears_refund(ext, &original_val, ¤t_val, &val); } else { if !current_val.is_zero() && val.is_zero() { - let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas); + let sstore_clears_schedule = ext.schedule().sstore_refund_gas; ext.add_sstore_refund(sstore_clears_schedule); } } diff --git a/ethcore/evm/src/tests.rs b/ethcore/evm/src/tests.rs index 49599f91d..af16ab1e5 100644 --- a/ethcore/evm/src/tests.rs +++ b/ethcore/evm/src/tests.rs @@ -716,7 +716,7 @@ fn test_jumps(factory: super::Factory) { test_finalize(vm.exec(&mut ext).ok().unwrap()).unwrap() }; - assert_eq!(ext.sstore_clears, U256::from(ext.schedule().sstore_refund_gas)); + assert_eq!(ext.sstore_clears, ext.schedule().sstore_refund_gas as i128); assert_store(&ext, 0, "0000000000000000000000000000000000000000000000000000000000000000"); // 5! assert_store(&ext, 1, "0000000000000000000000000000000000000000000000000000000000000078"); // 5! assert_eq!(gas_left, U256::from(54_117)); diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index cc9ac9a0b..9ad15c1aa 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -1091,7 +1091,8 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { let schedule = self.schedule; // refunds from SSTORE nonzero -> zero - let sstore_refunds = substate.sstore_clears_refund; + assert!(substate.sstore_clears_refund >= 0, "On transaction level, sstore clears refund cannot go below zero."); + let sstore_refunds = U256::from(substate.sstore_clears_refund as u64); // refunds from contract suicides let suicide_refunds = U256::from(schedule.suicide_refund_gas) * U256::from(substate.suicides.len()); let refunds_bound = sstore_refunds + suicide_refunds; @@ -2095,7 +2096,7 @@ mod tests { let gas_used = gas - gas_left; // sstore: 0 -> (1) -> () -> (1 -> 0 -> 1) assert_eq!(gas_used, U256::from(41860)); - assert_eq!(refund, U256::from(19800)); + assert_eq!(refund, 19800); assert_eq!(state.storage_at(&operating_address, &k).unwrap(), H256::from(U256::from(1))); // Test a call via top-level -> y2 -> x2 @@ -2113,7 +2114,7 @@ mod tests { let gas_used = gas - gas_left; // sstore: 1 -> (1) -> () -> (0 -> 1 -> 0) assert_eq!(gas_used, U256::from(11860)); - assert_eq!(refund, U256::from(19800)); + assert_eq!(refund, 19800); } fn wasm_sample_code() -> Arc> { diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index df8a9c75a..778ef5cc7 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -394,12 +394,12 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> self.depth } - fn add_sstore_refund(&mut self, value: U256) { - self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_add(value); + fn add_sstore_refund(&mut self, value: usize) { + self.substate.sstore_clears_refund += value as i128; } - fn sub_sstore_refund(&mut self, value: U256) { - self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_sub(value); + fn sub_sstore_refund(&mut self, value: usize) { + self.substate.sstore_clears_refund -= value as i128; } fn trace_next_instruction(&mut self, pc: usize, instruction: u8, current_gas: U256) -> bool { diff --git a/ethcore/src/json_tests/executive.rs b/ethcore/src/json_tests/executive.rs index 0f8903aed..1999ee4d6 100644 --- a/ethcore/src/json_tests/executive.rs +++ b/ethcore/src/json_tests/executive.rs @@ -218,11 +218,11 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B> false } - fn add_sstore_refund(&mut self, value: U256) { + fn add_sstore_refund(&mut self, value: usize) { self.ext.add_sstore_refund(value) } - fn sub_sstore_refund(&mut self, value: U256) { + fn sub_sstore_refund(&mut self, value: usize) { self.ext.sub_sstore_refund(value) } } diff --git a/ethcore/src/state/substate.rs b/ethcore/src/state/substate.rs index 054b8b384..5565c8f91 100644 --- a/ethcore/src/state/substate.rs +++ b/ethcore/src/state/substate.rs @@ -16,7 +16,7 @@ //! Execution environment substate. use std::collections::HashSet; -use ethereum_types::{U256, Address}; +use ethereum_types::Address; use log_entry::LogEntry; use evm::{Schedule, CleanDustMode}; use super::CleanupMode; @@ -35,7 +35,7 @@ pub struct Substate { pub logs: Vec, /// Refund counter of SSTORE. - pub sstore_clears_refund: U256, + pub sstore_clears_refund: i128, /// Created contracts. pub contracts_created: Vec
, @@ -52,7 +52,7 @@ impl Substate { self.suicides.extend(s.suicides); self.touched.extend(s.touched); self.logs.extend(s.logs); - self.sstore_clears_refund = self.sstore_clears_refund + s.sstore_clears_refund; + self.sstore_clears_refund += s.sstore_clears_refund; self.contracts_created.extend(s.contracts_created); } diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index aa45066b8..a437da53e 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -151,10 +151,10 @@ pub trait Ext { fn depth(&self) -> usize; /// Increments sstore refunds counter. - fn add_sstore_refund(&mut self, value: U256); + fn add_sstore_refund(&mut self, value: usize); /// Decrements sstore refunds counter. - fn sub_sstore_refund(&mut self, value: U256); + fn sub_sstore_refund(&mut self, value: usize); /// Decide if any more operations should be traced. Passthrough for the VM trace. fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _current_gas: U256) -> bool { false } diff --git a/ethcore/vm/src/tests.rs b/ethcore/vm/src/tests.rs index 687fef999..00c986af1 100644 --- a/ethcore/vm/src/tests.rs +++ b/ethcore/vm/src/tests.rs @@ -57,7 +57,7 @@ pub struct FakeExt { pub store: HashMap, pub suicides: HashSet
, pub calls: HashSet, - pub sstore_clears: U256, + pub sstore_clears: i128, pub depth: usize, pub blockhashes: HashMap, pub codes: HashMap>, @@ -231,12 +231,12 @@ impl Ext for FakeExt { self.is_static } - fn add_sstore_refund(&mut self, value: U256) { - self.sstore_clears = self.sstore_clears + value; + fn add_sstore_refund(&mut self, value: usize) { + self.sstore_clears += value as i128; } - fn sub_sstore_refund(&mut self, value: U256) { - self.sstore_clears = self.sstore_clears - value; + fn sub_sstore_refund(&mut self, value: usize) { + self.sstore_clears -= value as i128; } fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _gas: U256) -> bool { diff --git a/ethcore/wasm/src/runtime.rs b/ethcore/wasm/src/runtime.rs index a48ac9709..b6de43e9f 100644 --- a/ethcore/wasm/src/runtime.rs +++ b/ethcore/wasm/src/runtime.rs @@ -285,7 +285,7 @@ impl<'a> Runtime<'a> { self.ext.set_storage(key, val).map_err(|_| Error::StorageUpdateError)?; if former_val != H256::zero() && val == H256::zero() { - let sstore_clears_schedule = U256::from(self.schedule().sstore_refund_gas); + let sstore_clears_schedule = self.schedule().sstore_refund_gas; self.ext.add_sstore_refund(sstore_clears_schedule); }