Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

EIP 1283: Net gas metering for SSTORE without dirty maps #9319

Merged
merged 42 commits into from
Sep 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
b7a9402
Implement last_checkpoint_storage_at
sorpaas Aug 8, 2018
a082907
Add reverted_storage_at for externalities
sorpaas Aug 8, 2018
3ba5467
sstore_clears_count -> sstore_clears_refund
sorpaas Aug 8, 2018
87a93c0
Implement eip1283 for evm
sorpaas Aug 8, 2018
5f2e25c
Add eip1283Transition params
sorpaas Aug 8, 2018
9c56484
evm: fix tests
sorpaas Aug 8, 2018
4861e35
jsontests: fix test
sorpaas Aug 8, 2018
11490b5
Return checkpoint index when creating
sorpaas Aug 13, 2018
399abe8
Comply with spec Version II
sorpaas Aug 13, 2018
f57853c
Fix docs
sorpaas Aug 13, 2018
8909d6e
Merge branch 'master' of https://github.com/paritytech/parity into sp…
sorpaas Aug 14, 2018
e4a6668
Fix jsontests feature compile
sorpaas Aug 14, 2018
e2b5899
Address grumbles
sorpaas Aug 28, 2018
00ae258
Fix no-checkpoint-entry case
sorpaas Aug 28, 2018
fb99c74
Remove unnecessary expect
sorpaas Aug 28, 2018
3149c7f
Add test for State::checkpoint_storage_at
sorpaas Sep 3, 2018
c07d12f
Add executive level test for eip1283
sorpaas Sep 3, 2018
971e82c
Merge branch 'master' of https://github.com/paritytech/parity into sp…
sorpaas Sep 3, 2018
b1b7c57
Hard-code transaction_checkpoint_index to 0
sorpaas Sep 4, 2018
b01c566
Fix jsontests
sorpaas Sep 4, 2018
c2b9e97
Add tests for checkpoint discard/revert
sorpaas Sep 5, 2018
7f672c3
Require checkpoint to be empty for kill_account and commit
sorpaas Sep 5, 2018
6269c33
Get code coverage
sorpaas Sep 5, 2018
0e4ba15
Use saturating_add/saturating_sub
sorpaas Sep 5, 2018
196f831
Fix issues in insert_cache
sorpaas Sep 6, 2018
14b89c2
Clear the state again
sorpaas Sep 6, 2018
f9af2c3
Fix original_storage_at
sorpaas Sep 6, 2018
a718e90
Early return for empty RLP trie storage
sorpaas Sep 6, 2018
f6aa738
Update comments
sorpaas Sep 6, 2018
ad8dc94
Fix borrow_mut issue
sorpaas Sep 6, 2018
4d5555b
Simplify checkpoint_storage_at if branches
sorpaas Sep 6, 2018
8b31417
Better commenting for gas handling code
sorpaas Sep 6, 2018
72693d3
Address naming grumbles
sorpaas Sep 6, 2018
ab83e36
More tests
sorpaas Sep 6, 2018
ef6a01c
Fix an issue in overwrite_with
sorpaas Sep 7, 2018
aa6006e
Add another test
sorpaas Sep 7, 2018
aabc367
Fix comment
sorpaas Sep 7, 2018
8a573a1
Remove unnecessary bracket
sorpaas Sep 7, 2018
c31cc59
Move orig to inner if
sorpaas Sep 7, 2018
522f2e9
Remove test coverage for this PR
sorpaas Sep 7, 2018
14283f8
Add tests for executive original value
sorpaas Sep 7, 2018
ae73ed4
Add warn! for an unreachable cause
sorpaas Sep 7, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 93 additions & 5 deletions ethcore/evm/src/interpreter/gasometer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,17 @@ impl<Gas: evm::CostType> Gasometer<Gas> {
let newval = stack.peek(1);
let val = U256::from(&*ext.storage_at(&address)?);

let gas = if val.is_zero() && !newval.is_zero() {
schedule.sstore_set_gas
let gas = if schedule.eip1283 {
let orig = U256::from(&*ext.initial_storage_at(&address)?);
calculate_eip1283_sstore_gas(schedule, &orig, &val, &newval)
} else {
// Refund for below case is added when actually executing sstore
// !is_zero(&val) && is_zero(newval)
schedule.sstore_reset_gas
if val.is_zero() && !newval.is_zero() {
schedule.sstore_set_gas
} else {
// Refund for below case is added when actually executing sstore
// !is_zero(&val) && is_zero(newval)
schedule.sstore_reset_gas
}
};
Request::Gas(Gas::from(gas))
},
Expand Down Expand Up @@ -342,6 +347,89 @@ fn add_gas_usize<Gas: evm::CostType>(value: Gas, num: usize) -> (Gas, bool) {
value.overflow_add(Gas::from(num))
}

#[inline]
fn calculate_eip1283_sstore_gas<Gas: evm::CostType>(schedule: &Schedule, original: &U256, current: &U256, new: &U256) -> Gas {
Gas::from(
if current == new {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe early-return pattern would look a bit better here instead of this nested ifs?
Alternatively maybe try pattern matching?

match (current, new, original) {
 (current, new, _) if current == new => schedule.sload_gas,
 (current, _, original) if current != original => schedule.sload_gas,
 (_, _, original) if original.is_zero() =>  schedule.sstore_set_gas,
 _ => schedule.sstore_reset_gas,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH I think for this, using if branches may be better. In that way we can directly follow the spec of each cause. Using match is indeed shorter, but I think it may actually be more confusing.

// 1. If current value equals new value (this is a no-op), 200 gas is deducted.
schedule.sload_gas
} else {
// 2. If current value does not equal new value
if original == current {
// 2.1. If original value equals current value (this storage slot has not been changed by the current execution context)
if original.is_zero() {
// 2.1.1. If original value is 0, 20000 gas is deducted.
schedule.sstore_set_gas
} else {
// 2.1.2. Otherwise, 5000 gas is deducted.
schedule.sstore_reset_gas

// 2.1.2.1. If new value is 0, add 15000 gas to refund counter.
}
} else {
// 2.2. If original value does not equal current value (this storage slot is dirty), 200 gas is deducted. Apply both of the following clauses.
schedule.sload_gas

// 2.2.1. If original value is not 0
// 2.2.1.1. If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
// 2.2.1.2. If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.

// 2.2.2. If original value equals new value (this storage slot is reset)
// 2.2.2.1. If original value is 0, add 19800 gas to refund counter.
// 2.2.2.2. Otherwise, add 4800 gas to refund counter.
}
}
)
}

pub fn handle_eip1283_sstore_clears_refund(ext: &mut vm::Ext, original: &U256, current: &U256, new: &U256) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe return Option<U256> from here instead of calling externalities to separate reading/calculating from actually applying changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there may be issues doing this -- sometimes we need to issue two refunds at once, and the value itself can be either positive or negative. So returning a value instead of directly modifying ext would mean that we need extra codes to handle signed addition/subtraction.

let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas);

if current == new {
// 1. If current value equals new value (this is a no-op), 200 gas is deducted.
} else {
// 2. If current value does not equal new value
if original == current {
// 2.1. If original value equals current value (this storage slot has not been changed by the current execution context)
if original.is_zero() {
// 2.1.1. If original value is 0, 20000 gas is deducted.
} else {
// 2.1.2. Otherwise, 5000 gas is deducted.
if new.is_zero() {
// 2.1.2.1. If new value is 0, add 15000 gas to refund counter.
ext.add_sstore_refund(sstore_clears_schedule);
}
}
} else {
// 2.2. If original value does not equal current value (this storage slot is dirty), 200 gas is deducted. Apply both of the following clauses.

if !original.is_zero() {
// 2.2.1. If original value is not 0
if current.is_zero() {
// 2.2.1.1. If current value is 0 (also means that new value is not 0), remove 15000 gas from refund counter. We can prove that refund counter will never go below 0.
ext.sub_sstore_refund(sstore_clears_schedule);
} else if new.is_zero() {
// 2.2.1.2. If new value is 0 (also means that current value is not 0), add 15000 gas to refund counter.
ext.add_sstore_refund(sstore_clears_schedule);
}
}

if original == new {
// 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);
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);
ext.add_sstore_refund(refund);
}
}
}
}
}

#[test]
fn test_mem_gas_cost() {
// given
Expand Down
10 changes: 8 additions & 2 deletions ethcore/evm/src/interpreter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -629,8 +629,14 @@ impl<Cost: CostType> Interpreter<Cost> {

let current_val = U256::from(&*ext.storage_at(&address)?);
// Increase refund for clear
if !current_val.is_zero() && val.is_zero() {
ext.inc_sstore_clears();
if ext.schedule().eip1283 {
let original_val = U256::from(&*ext.initial_storage_at(&address)?);
gasometer::handle_eip1283_sstore_clears_refund(ext, &original_val, &current_val, &val);
} else {
if !current_val.is_zero() && val.is_zero() {
let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas);
ext.add_sstore_refund(sstore_clears_schedule);
}
}
ext.set_storage(address, H256::from(&val))?;
},
Expand Down
2 changes: 1 addition & 1 deletion ethcore/evm/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ fn test_jumps(factory: super::Factory) {
test_finalize(vm.exec(&mut ext)).unwrap()
};

assert_eq!(ext.sstore_clears, 1);
assert_eq!(ext.sstore_clears, U256::from(ext.schedule().sstore_refund_gas));
assert_store(&ext, 0, "0000000000000000000000000000000000000000000000000000000000000000"); // 5!
assert_store(&ext, 1, "0000000000000000000000000000000000000000000000000000000000000078"); // 5!
assert_eq!(gas_left, U256::from(54_117));
Expand Down
3 changes: 2 additions & 1 deletion ethcore/res/ethereum/constantinople_test.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
"eip211Transition": "0x0",
"eip214Transition": "0x0",
"eip155Transition": "0x0",
"eip658Transition": "0x0"
"eip658Transition": "0x0",
"eip1283Transition": "0x0"
},
"genesis": {
"seal": {
Expand Down
66 changes: 63 additions & 3 deletions ethcore/src/executive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -574,9 +574,9 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
let prev_bal = self.state.balance(&params.address)?;
if let ActionValue::Transfer(val) = params.value {
self.state.sub_balance(&params.sender, &val, &mut substate.to_cleanup_mode(&schedule))?;
self.state.new_contract(&params.address, val + prev_bal, nonce_offset);
self.state.new_contract(&params.address, val + prev_bal, nonce_offset)?;
} else {
self.state.new_contract(&params.address, prev_bal, nonce_offset);
self.state.new_contract(&params.address, prev_bal, nonce_offset)?;
}

let trace_info = tracer.prepare_trace_create(&params);
Expand Down Expand Up @@ -627,7 +627,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
let schedule = self.schedule;

// refunds from SSTORE nonzero -> zero
let sstore_refunds = U256::from(schedule.sstore_refund_gas) * substate.sstore_clears_count;
let sstore_refunds = substate.sstore_clears_refund;
// 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;
Expand Down Expand Up @@ -1614,6 +1614,66 @@ mod tests {
assert_eq!(state.storage_at(&contract_address, &H256::from(&U256::zero())).unwrap(), H256::from(&U256::from(0)));
}

evm_test!{test_eip1283: test_eip1283_int}
fn test_eip1283(factory: Factory) {
let x1 = Address::from(0x1000);
let x2 = Address::from(0x1001);
let y1 = Address::from(0x2001);
let y2 = Address::from(0x2002);
let operating_address = Address::from(0);
let k = H256::new();

let mut state = get_temp_state_with_factory(factory.clone());
state.new_contract(&x1, U256::zero(), U256::from(1)).unwrap();
state.init_code(&x1, "600160005560006000556001600055".from_hex().unwrap()).unwrap();
state.new_contract(&x2, U256::zero(), U256::from(1)).unwrap();
state.init_code(&x2, "600060005560016000556000600055".from_hex().unwrap()).unwrap();
state.new_contract(&y1, U256::zero(), U256::from(1)).unwrap();
state.init_code(&y1, "600060006000600061100062fffffff4".from_hex().unwrap()).unwrap();
state.new_contract(&y2, U256::zero(), U256::from(1)).unwrap();
state.init_code(&y2, "600060006000600061100162fffffff4".from_hex().unwrap()).unwrap();

let info = EnvInfo::default();
let machine = ::ethereum::new_constantinople_test_machine();
let schedule = machine.schedule(info.number);

assert_eq!(state.storage_at(&operating_address, &k).unwrap(), H256::from(U256::from(0)));
// Test a call via top-level -> y1 -> x1
let (FinalizationResult { gas_left, .. }, refund, gas) = {
let gas = U256::from(0xffffffffffu64);
let mut params = ActionParams::default();
params.code = Some(Arc::new("6001600055600060006000600061200163fffffffff4".from_hex().unwrap()));
params.gas = gas;
let mut substate = Substate::new();
let mut ex = Executive::new(&mut state, &info, &machine, &schedule);
let res = ex.call(params, &mut substate, &mut NoopTracer, &mut NoopVMTracer).unwrap();

(res, substate.sstore_clears_refund, gas)
};
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!(state.storage_at(&operating_address, &k).unwrap(), H256::from(U256::from(1)));
// Test a call via top-level -> y2 -> x2
let (FinalizationResult { gas_left, .. }, refund, gas) = {
let gas = U256::from(0xffffffffffu64);
let mut params = ActionParams::default();
params.code = Some(Arc::new("6001600055600060006000600061200263fffffffff4".from_hex().unwrap()));
params.gas = gas;
let mut substate = Substate::new();
let mut ex = Executive::new(&mut state, &info, &machine, &schedule);
let res = ex.call(params, &mut substate, &mut NoopTracer, &mut NoopVMTracer).unwrap();

(res, substate.sstore_clears_refund, gas)
};
let gas_used = gas - gas_left;
// sstore: 1 -> (1) -> () -> (0 -> 1 -> 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the first number in this comments (others are ok)? It is nice to have this test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first number is the original value. Added the tests!

assert_eq!(gas_used, U256::from(11860));
assert_eq!(refund, U256::from(19800));
}

fn wasm_sample_code() -> Arc<Vec<u8>> {
Arc::new(
"0061736d01000000010d0360027f7f0060017f0060000002270303656e7603726574000003656e760673656e646572000103656e76066d656d6f727902010110030201020404017000000501000708010463616c6c00020901000ac10101be0102057f017e4100410028020441c0006b22043602042004412c6a41106a220041003602002004412c6a41086a22014200370200200441186a41106a22024100360200200441186a41086a220342003703002004420037022c2004410036021c20044100360218200441186a1001200020022802002202360200200120032903002205370200200441106a2002360200200441086a200537030020042004290318220537022c200420053703002004411410004100200441c0006a3602040b0b0a010041040b0410c00000"
Expand Down
12 changes: 10 additions & 2 deletions ethcore/src/externalities.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B>
impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
where T: Tracer, V: VMTracer, B: StateBackend
{
fn initial_storage_at(&self, key: &H256) -> vm::Result<H256> {
self.state.checkpoint_storage_at(0, &self.origin_info.address, key).map(|v| v.unwrap_or(H256::zero())).map_err(Into::into)
}

fn storage_at(&self, key: &H256) -> vm::Result<H256> {
self.state.storage_at(&self.origin_info.address, key).map_err(Into::into)
}
Expand Down Expand Up @@ -390,8 +394,12 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
self.depth
}

fn inc_sstore_clears(&mut self) {
self.substate.sstore_clears_count = self.substate.sstore_clears_count + U256::one();
fn add_sstore_refund(&mut self, value: U256) {
self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_add(value);
}

fn sub_sstore_refund(&mut self, value: U256) {
self.substate.sstore_clears_refund = self.substate.sstore_clears_refund.saturating_sub(value);
}

fn trace_next_instruction(&mut self, pc: usize, instruction: u8, current_gas: U256) -> bool {
Expand Down
12 changes: 10 additions & 2 deletions ethcore/src/json_tests/executive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,10 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B>
self.ext.storage_at(key)
}

fn initial_storage_at(&self, key: &H256) -> vm::Result<H256> {
self.ext.initial_storage_at(key)
}

fn set_storage(&mut self, key: H256, value: H256) -> vm::Result<()> {
self.ext.set_storage(key, value)
}
Expand Down Expand Up @@ -205,8 +209,12 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for TestExt<'a, T, V, B>
false
}

fn inc_sstore_clears(&mut self) {
self.ext.inc_sstore_clears()
fn add_sstore_refund(&mut self, value: U256) {
self.ext.add_sstore_refund(value)
}

fn sub_sstore_refund(&mut self, value: U256) {
self.ext.sub_sstore_refund(value)
}
}

Expand Down
7 changes: 7 additions & 0 deletions ethcore/src/spec/spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,8 @@ pub struct CommonParams {
pub eip145_transition: BlockNumber,
/// Number of first block where EIP-1052 rules begin.
pub eip1052_transition: BlockNumber,
/// Number of first block where EIP-1283 rules begin.
pub eip1283_transition: BlockNumber,
/// Number of first block where EIP-1014 rules begin.
pub eip1014_transition: BlockNumber,
/// Number of first block where dust cleanup rules (EIP-168 and EIP169) begin.
Expand Down Expand Up @@ -183,6 +185,7 @@ impl CommonParams {
schedule.have_return_data = block_number >= self.eip211_transition;
schedule.have_bitwise_shifting = block_number >= self.eip145_transition;
schedule.have_extcodehash = block_number >= self.eip1052_transition;
schedule.eip1283 = block_number >= self.eip1283_transition;
if block_number >= self.eip210_transition {
schedule.blockhash_gas = 800;
}
Expand Down Expand Up @@ -286,6 +289,10 @@ impl From<ethjson::spec::Params> for CommonParams {
BlockNumber::max_value,
Into::into,
),
eip1283_transition: p.eip1283_transition.map_or_else(
BlockNumber::max_value,
Into::into,
),
eip1014_transition: p.eip1014_transition.map_or_else(
BlockNumber::max_value,
Into::into,
Expand Down
Loading