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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
sstore_clears_count -> sstore_clears_refund
sorpaas committed Aug 8, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
BridgeAR Ruben Bridgewater
commit 3ba54678038d81eee01e4056afc94463bc728874
3 changes: 2 additions & 1 deletion ethcore/evm/src/interpreter/mod.rs
Original file line number Diff line number Diff line change
@@ -516,7 +516,8 @@ impl<Cost: CostType> Interpreter<Cost> {
let current_val = U256::from(&*ext.storage_at(&address)?);
// Increase refund for clear
if !self.is_zero(&current_val) && self.is_zero(&val) {
ext.inc_sstore_clears();
let sstore_clears_schedule = U256::from(ext.schedule().sstore_refund_gas);
ext.inc_sstore_refund(sstore_clears_schedule);
}
ext.set_storage(address, H256::from(&val))?;
},
2 changes: 1 addition & 1 deletion ethcore/src/executive.rs
Original file line number Diff line number Diff line change
@@ -616,7 +616,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;
8 changes: 6 additions & 2 deletions ethcore/src/externalities.rs
Original file line number Diff line number Diff line change
@@ -400,8 +400,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 inc_sstore_refund(&mut self, value: U256) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it's possible to use u64 instead of U256 here? Maybe we could use u64 until it fits and fallback to U256 if it overflows? That said, I'm happy to see an improvement PR for this with some benchmarks to prove that it's actually worth maintaining the more complicated code here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That won't save memory, IMO. The issue is that we would need a enum/union for that, but it always takes more compared what U256 takes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't mean memory, but rather a performance improvement since we are not using our custom U256 arithmetic. But as mentioned, it's fine for me to see a separate PR with that if anyone is interested on working on this, it's probably not super hot code anyway, so it would be marginal.

self.substate.sstore_clears_refund = self.substate.sstore_clears_refund + value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure it won't overflow? Can we use saturating_add just to be safe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're sure it won't overflow -- Refund will never go above gas provided, and gas provided has upper limit of U256::max_value(). I think the issue here is that if it actually overflows, that indicates a consensus bug, and I think it would be better to panic instead of silently saturating if that ever happens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be a consensus bug only if it's implemented differently in other clients. Always panicking doesn't seem to me like a good idea (would actually prefer the network to split than to kill large part of it, or even the entire network if it's a private Parity-only chain).

Happy with whatever behavior that doesn't cause a panic (i.e. overflowing_add or saturating_add) - better be safe (and explicit) than sorry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomusdrw What I meant is that theoretically that cannot happen. Refund can never go over U256::max_value().

Copy link
Collaborator Author

@sorpaas sorpaas Sep 5, 2018

Choose a reason for hiding this comment

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

I realized that I was wrong here. Theoretically it's indeed possible for this value to go over U256::max_value() (but practically that should never happen). So I'm going to change this to saturating_add.

}

fn dec_sstore_refund(&mut self, value: U256) {
self.substate.sstore_clears_refund = self.substate.sstore_clears_refund - value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use saturating_sub here to make sure it doesn't panic on overflow?

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 same as above -- we're mostly sure it won't overflow, otherwise it's a consensus bug.

}

fn trace_next_instruction(&mut self, pc: usize, instruction: u8, current_gas: U256) -> bool {
12 changes: 6 additions & 6 deletions ethcore/src/state/substate.rs
Original file line number Diff line number Diff line change
@@ -34,8 +34,8 @@ pub struct Substate {
/// Any logs.
pub logs: Vec<LogEntry>,

/// Refund counter of SSTORE nonzero -> zero.
pub sstore_clears_count: U256,
/// Refund counter of SSTORE.
pub sstore_clears_refund: U256,

/// Created contracts.
pub contracts_created: Vec<Address>,
@@ -52,7 +52,7 @@ impl Substate {
self.suicides.extend(s.suicides);
self.touched.extend(s.touched);
self.logs.extend(s.logs);
self.sstore_clears_count = self.sstore_clears_count + s.sstore_clears_count;
self.sstore_clears_refund = self.sstore_clears_refund + s.sstore_clears_refund;
self.contracts_created.extend(s.contracts_created);
}

@@ -86,7 +86,7 @@ mod tests {
topics: vec![],
data: vec![]
});
sub_state.sstore_clears_count = 5.into();
sub_state.sstore_clears_refund = (15000 * 5).into();
sub_state.suicides.insert(10u64.into());

let mut sub_state_2 = Substate::new();
@@ -96,11 +96,11 @@ mod tests {
topics: vec![],
data: vec![]
});
sub_state_2.sstore_clears_count = 7.into();
sub_state_2.sstore_clears_refund = (15000 * 7).into();

sub_state.accrue(sub_state_2);
assert_eq!(sub_state.contracts_created.len(), 2);
assert_eq!(sub_state.sstore_clears_count, 12.into());
assert_eq!(sub_state.sstore_clears_refund, (15000 * 12).into());
assert_eq!(sub_state.suicides.len(), 1);
}
}
7 changes: 5 additions & 2 deletions ethcore/vm/src/ext.rs
Original file line number Diff line number Diff line change
@@ -140,8 +140,11 @@ pub trait Ext {
/// then A depth is 0, B is 1, C is 2 and so on.
fn depth(&self) -> usize;

/// Increments sstore refunds count by 1.
fn inc_sstore_clears(&mut self);
/// Increments sstore refunds counter.
fn inc_sstore_refund(&mut self, value: U256);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather call it add and sub instead of inc and dec, since it takes a parameter now. (or change the comments to say "Increase/Decrease" instead of "Increment/Decrement")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!


/// Decrements sstore refunds counter.
fn dec_sstore_refund(&mut self, value: U256);

/// 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 }
4 changes: 4 additions & 0 deletions ethcore/vm/src/schedule.rs
Original file line number Diff line number Diff line change
@@ -119,6 +119,8 @@ pub struct Schedule {
pub kill_dust: CleanDustMode,
/// Enable EIP-86 rules
pub eip86: bool,
/// Enable EIP-1283 rules
pub eip1283: bool,
/// Wasm extra schedule settings, if wasm activated
pub wasm: Option<WasmCosts>,
}
@@ -248,6 +250,7 @@ impl Schedule {
have_static_call: false,
kill_dust: CleanDustMode::Off,
eip86: false,
eip1283: false,
wasm: None,
}
}
@@ -321,6 +324,7 @@ impl Schedule {
have_static_call: false,
kill_dust: CleanDustMode::Off,
eip86: false,
eip1283: false,
wasm: None,
}
}
10 changes: 7 additions & 3 deletions ethcore/vm/src/tests.rs
Original file line number Diff line number Diff line change
@@ -56,7 +56,7 @@ pub struct FakeExt {
pub store: HashMap<H256, H256>,
pub suicides: HashSet<Address>,
pub calls: HashSet<FakeCall>,
pub sstore_clears: usize,
pub sstore_clears: U256,
pub depth: usize,
pub blockhashes: HashMap<U256, H256>,
pub codes: HashMap<Address, Arc<Bytes>>,
@@ -221,8 +221,12 @@ impl Ext for FakeExt {
self.is_static
}

fn inc_sstore_clears(&mut self) {
self.sstore_clears += 1;
fn inc_sstore_refund(&mut self, value: U256) {
self.sstore_clears = self.sstore_clears + value;
}

fn dec_sstore_refund(&mut self, value: U256) {
self.sstore_clears = self.sstore_clears - value;
}

fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _gas: U256) -> bool {
3 changes: 2 additions & 1 deletion ethcore/wasm/src/runtime.rs
Original file line number Diff line number Diff line change
@@ -284,7 +284,8 @@ impl<'a> Runtime<'a> {
self.ext.set_storage(key, val).map_err(|_| Error::StorageUpdateError)?;

if former_val != H256::zero() && val == H256::zero() {
self.ext.inc_sstore_clears();
let sstore_clears_schedule = U256::from(self.schedule().sstore_refund_gas);
self.ext.inc_sstore_refund(sstore_clears_schedule);
}

Ok(())