-
Notifications
You must be signed in to change notification settings - Fork 1.7k
EIP 1283: Net gas metering for SSTORE without dirty maps #9319
Changes from 36 commits
b7a9402
a082907
3ba5467
87a93c0
5f2e25c
9c56484
4861e35
11490b5
399abe8
f57853c
8909d6e
e4a6668
e2b5899
00ae258
fb99c74
3149c7f
c07d12f
971e82c
b1b7c57
b01c566
c2b9e97
7f672c3
6269c33
0e4ba15
196f831
14b89c2
f9af2c3
a718e90
f6aa738
ad8dc94
4d5555b
8b31417
72693d3
ab83e36
ef6a01c
aa6006e
aabc367
8a573a1
c31cc59
522f2e9
14283f8
ae73ed4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,7 @@ test-coverage-kcov: | |
stage: test | ||
only: | ||
- master | ||
- pr-9319 | ||
script: | ||
- scripts/gitlab/coverage.sh | ||
tags: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,13 +124,18 @@ impl<Gas: evm::CostType> Gasometer<Gas> { | |
let address = H256::from(stack.peek(0)); | ||
let newval = stack.peek(1); | ||
let val = U256::from(&*ext.storage_at(&address)?); | ||
let orig = U256::from(&*ext.initial_storage_at(&address)?); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still believe it's better to move it inside the if banch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I missed this. Moved! |
||
|
||
let gas = if val.is_zero() && !newval.is_zero() { | ||
schedule.sstore_set_gas | ||
let gas = if schedule.eip1283 { | ||
calculate_eip1283_sstore_gas(schedule, &orig, &val, &newval) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could orig be initiated in this conditional branch ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You address it in your last commit (my comment was unclear) |
||
} 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)) | ||
}, | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? 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,
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe return There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code looks is similar to your eip description (great eip by the way), but it is not that easy to ensure that all state transition are covered just by reading it. So I find it useful to write a few additional comments for checking the algorithm: 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);
if current == new {
// No refund
} else {
if original == current {
if original.is_zero() {
// No refund
} else {
if new.is_zero() {
ext.inc_sstore_refund(sstore_clears_schedule);
}
}
} else {
// put in a `dirty_refund` function??
if !original.is_zero() {
// refund case
if current.is_zero() {
// a refund happened, revert refund
ext.dec_sstore_refund(sstore_clears_schedule);
} else if new.is_zero() {
// refund
ext.inc_sstore_refund(sstore_clears_schedule);
}
}
if original == new {
// reverting to init state
if original.is_zero() {
// revert sstore full cost (minus sload amount)
let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas);
ext.inc_sstore_refund(refund);
} else {
// revert sstore change cost (revert of refund done in previous conditions)
let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas);
ext.inc_sstore_refund(refund);
}
}
}
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps pattern matching here as well? Not sure if it would be more readable though. |
||
// 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as earlier - we only need |
||
let original_val = U256::from(&*ext.initial_storage_at(&address)?); | ||
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); | ||
ext.add_sstore_refund(sstore_clears_schedule); | ||
} | ||
} | ||
ext.set_storage(address, H256::from(&val))?; | ||
}, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -574,9 +574,9 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> { | |
let prev_bal = self.state.balance(¶ms.address)?; | ||
if let ActionValue::Transfer(val) = params.value { | ||
self.state.sub_balance(¶ms.sender, &val, &mut substate.to_cleanup_mode(&schedule))?; | ||
self.state.new_contract(¶ms.address, val + prev_bal, nonce_offset); | ||
self.state.new_contract(¶ms.address, val + prev_bal, nonce_offset)?; | ||
} else { | ||
self.state.new_contract(¶ms.address, prev_bal, nonce_offset); | ||
self.state.new_contract(¶ms.address, prev_bal, nonce_offset)?; | ||
} | ||
|
||
let trace_info = tracer.prepare_trace_create(¶ms); | ||
|
@@ -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; | ||
|
@@ -1614,6 +1614,62 @@ 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 mut state = get_temp_state_with_factory(factory.clone()); | ||
state.new_contract(&x1, U256::zero(), U256::from(1)).unwrap(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this is just to test whether checkpointing is working with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry, I was confused on this and the other test: In this case, we want to create new contracts and init code for (Reading the code it seems indeed the case that calling |
||
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); | ||
|
||
// 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)); | ||
|
||
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be removed before merge