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

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

Show file tree
Hide file tree
Changes from 12 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
69 changes: 64 additions & 5 deletions ethcore/evm/src/interpreter/gasometer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.reverted_storage_at(&address)?);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We only need orig inside the schedule.eip1283 branch, I'd move it there.


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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could orig be initiated in this conditional branch ?

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 need Ext handle for that, but right now we only need to pass schedule to the function, so I think it may not be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The 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))
},
Expand Down Expand Up @@ -342,6 +347,60 @@ 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.

schedule.sload_gas
} else {
if original == current {
if original.is_zero() {
schedule.sstore_set_gas
} else {
schedule.sstore_reset_gas
}
} else {
schedule.sload_gas
}
}
)
}

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 {
// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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);
				}
			}
		}
	}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

if !original.is_zero() {
if current.is_zero() {
ext.dec_sstore_refund(sstore_clears_schedule);
} else if new.is_zero() {
ext.inc_sstore_refund(sstore_clears_schedule);
}
}
if original == new {
if original.is_zero() {
let refund = U256::from(ext.schedule().sstore_set_gas - ext.schedule().sload_gas);
ext.inc_sstore_refund(refund);
} else {
let refund = U256::from(ext.schedule().sstore_reset_gas - ext.schedule().sload_gas);
ext.inc_sstore_refund(refund);
}
}
}
}
}

#[test]
fn test_mem_gas_cost() {
// given
Expand Down
10 changes: 8 additions & 2 deletions ethcore/evm/src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,9 +628,15 @@ impl<Cost: CostType> Interpreter<Cost> {
let val = self.stack.pop_back();

let current_val = U256::from(&*ext.storage_at(&address)?);
let original_val = U256::from(&*ext.reverted_storage_at(&address)?);
// Increase refund for clear
if !current_val.is_zero() && val.is_zero() {
ext.inc_sstore_clears();
if ext.schedule().eip1283 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as earlier - we only need original_val in eip1283 branch, best to avoid unnecessary ext call and keep the variable local imho (helps with readability)

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.inc_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
View file
Open in desktop
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(15_000));
This conversation was marked as resolved.
Show resolved Hide resolved
assert_store(&ext, 0, "0000000000000000000000000000000000000000000000000000000000000000"); // 5!
assert_store(&ext, 1, "0000000000000000000000000000000000000000000000000000000000000078"); // 5!
assert_eq!(gas_left, U256::from(54_117));
Expand Down
23 changes: 15 additions & 8 deletions ethcore/src/executive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ pub struct Executive<'a, B: 'a> {
schedule: &'a Schedule,
depth: usize,
static_flag: bool,
transaction_checkpoint_index: Option<usize>,
}

impl<'a, B: 'a + StateBackend> Executive<'a, B> {
Expand All @@ -184,18 +185,20 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
schedule: schedule,
depth: 0,
static_flag: false,
transaction_checkpoint_index: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a bit of comment I think. From what I understand it is to manage reentrant call by addressing the first checkpoint initial values; if it is the case renaming to first_transaction_checkpoint_index also makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This value actually just means to be transaction_checkpoint_index -- the checkpoint where if the transaction gets reverted. We set it at the first toplevel call/create executive because that checkpoint is the same as the "transaction checkpoint". I think the understanding that it's the "first checkpoint initial values" is correct, but I think it's not first_transaction_checkpoint_index.

}
}

/// Populates executive from parent properties. Increments executive depth.
pub fn from_parent(state: &'a mut State<B>, info: &'a EnvInfo, machine: &'a Machine, schedule: &'a Schedule, parent_depth: usize, static_flag: bool) -> Self {
pub fn from_parent(state: &'a mut State<B>, info: &'a EnvInfo, machine: &'a Machine, schedule: &'a Schedule, parent_depth: usize, static_flag: bool, transaction_checkpoint_index: usize) -> Self {
Executive {
state: state,
info: info,
machine: machine,
schedule: schedule,
depth: parent_depth + 1,
static_flag: static_flag,
transaction_checkpoint_index: Some(transaction_checkpoint_index),
}
}

Expand All @@ -208,9 +211,11 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
tracer: &'any mut T,
vm_tracer: &'any mut V,
static_call: bool,
current_checkpoint_index: usize,
) -> Externalities<'any, T, V, B> where T: Tracer, V: VMTracer {
let is_static = self.static_flag || static_call;
Externalities::new(self.state, self.info, self.machine, self.schedule, self.depth, origin_info, substate, output, tracer, vm_tracer, is_static)
let transaction_checkpoint_index = self.transaction_checkpoint_index.unwrap_or(current_checkpoint_index);
Externalities::new(self.state, self.info, self.machine, self.schedule, self.depth, origin_info, substate, output, tracer, vm_tracer, is_static, transaction_checkpoint_index)
}

/// This function should be used to execute transaction.
Expand Down Expand Up @@ -352,6 +357,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
params: ActionParams,
unconfirmed_substate: &mut Substate,
output_policy: OutputPolicy,
current_checkpoint_index: usize,
tracer: &mut T,
vm_tracer: &mut V
) -> vm::Result<FinalizationResult> where T: Tracer, V: VMTracer {
Expand All @@ -365,7 +371,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
let origin_info = OriginInfo::from(&params);
trace!(target: "executive", "ext.schedule.have_delegate_call: {}", self.schedule.have_delegate_call);
let mut vm = vm_factory.create(params, self.schedule, self.depth);
let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call);
let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call, current_checkpoint_index);
return vm.exec(&mut ext).finalize(ext);
}

Expand All @@ -376,7 +382,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {

scope.builder().stack_size(::std::cmp::max(self.schedule.max_depth.saturating_sub(depth_threshold) * STACK_SIZE_PER_DEPTH, local_stack_size)).spawn(move || {
let mut vm = vm_factory.create(params, self.schedule, self.depth);
let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call);
let mut ext = self.as_externalities(origin_info, unconfirmed_substate, output_policy, tracer, vm_tracer, static_call, current_checkpoint_index);
vm.exec(&mut ext).finalize(ext)
}).expect("Sub-thread creation cannot fail; the host might run out of resources; qed")
}).join()
Expand All @@ -403,7 +409,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
}

// backup used in case of running out of gas
self.state.checkpoint();
let current_checkpoint_index = self.state.checkpoint();

let schedule = self.schedule;

Expand Down Expand Up @@ -491,7 +497,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
let mut subvmtracer = vm_tracer.prepare_subtrace(params.code.as_ref().expect("scope is conditional on params.code.is_some(); qed"));

let res = {
self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return, &mut subtracer, &mut subvmtracer)
self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return, current_checkpoint_index, &mut subtracer, &mut subvmtracer)
};

vm_tracer.done_subtrace(subvmtracer);
Expand Down Expand Up @@ -560,7 +566,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
}

// backup used in case of running out of gas
self.state.checkpoint();
let current_checkpoint_index = self.state.checkpoint();

// part of substate that may be reverted
let mut unconfirmed_substate = Substate::new();
Expand Down Expand Up @@ -588,6 +594,7 @@ impl<'a, B: 'a + StateBackend> Executive<'a, B> {
params,
&mut unconfirmed_substate,
OutputPolicy::InitContract,
current_checkpoint_index,
&mut subtracer,
&mut subvmtracer
);
Expand Down Expand Up @@ -626,7 +633,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
35 changes: 23 additions & 12 deletions ethcore/src/externalities.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ pub struct Externalities<'a, T: 'a, V: 'a, B: 'a> {
tracer: &'a mut T,
vm_tracer: &'a mut V,
static_flag: bool,
transaction_checkpoint_index: usize,
}

impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B>
Expand All @@ -93,6 +94,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B>
tracer: &'a mut T,
vm_tracer: &'a mut V,
static_flag: bool,
transaction_checkpoint_index: usize,
) -> Self {
Externalities {
state: state,
Expand All @@ -106,13 +108,18 @@ impl<'a, T: 'a, V: 'a, B: 'a> Externalities<'a, T, V, B>
tracer: tracer,
vm_tracer: vm_tracer,
static_flag: static_flag,
transaction_checkpoint_index: transaction_checkpoint_index,
}
}
}

impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
where T: Tracer, V: VMTracer, B: StateBackend
{
fn reverted_storage_at(&self, key: &H256) -> vm::Result<H256> {
self.state.checkpoint_storage_at(self.transaction_checkpoint_index, &self.origin_info.address, key).map(|v| v.unwrap_or(H256::zero())).map_err(Into::into)
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I correct if I say that in the current state of the code, self.transaction_checkpoint_index is always 0 ? (just wondering if there is a way to avoid passing this index as parameter)

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 you're right! I don't think we had this assumption, but right now it's indeed the case -- checkpoints are only created via Executive::call and Executive::create, and we always made sure to remove all checkpoints before the top-level call/create is finished.

}

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 @@ -237,7 +244,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
}
}
}
let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag);
let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag, self.transaction_checkpoint_index);

// TODO: handle internal error separately
match ex.create(params, self.substate, self.tracer, self.vm_tracer) {
Expand Down Expand Up @@ -291,7 +298,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B>
params.value = ActionValue::Transfer(value);
}

let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag);
let mut ex = Executive::from_parent(self.state, self.env_info, self.machine, self.schedule, self.depth, self.static_flag, self.transaction_checkpoint_index);

match ex.call(params, self.substate, self.tracer, self.vm_tracer) {
Ok(FinalizationResult{ gas_left, return_data, apply_state: true }) => MessageCallResult::Success(gas_left, return_data),
Expand Down Expand Up @@ -390,8 +397,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 •
edited
Loading

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 {
Expand Down Expand Up @@ -473,7 +484,7 @@ mod tests {
let mut tracer = NoopTracer;
let mut vm_tracer = NoopVMTracer;

let ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);

assert_eq!(ext.env_info().number, 100);
}
Expand All @@ -485,7 +496,7 @@ mod tests {
let mut tracer = NoopTracer;
let mut vm_tracer = NoopVMTracer;

let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);

let hash = ext.blockhash(&"0000000000000000000000000000000000000000000000000000000000120000".parse::<U256>().unwrap());

Expand All @@ -509,7 +520,7 @@ mod tests {
let mut tracer = NoopTracer;
let mut vm_tracer = NoopVMTracer;

let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);

let hash = ext.blockhash(&"0000000000000000000000000000000000000000000000000000000000120000".parse::<U256>().unwrap());

Expand All @@ -524,7 +535,7 @@ mod tests {
let mut tracer = NoopTracer;
let mut vm_tracer = NoopVMTracer;

let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);

// this should panic because we have no balance on any account
ext.call(
Expand All @@ -549,7 +560,7 @@ mod tests {
let mut vm_tracer = NoopVMTracer;

{
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);
ext.log(log_topics, &log_data).unwrap();
}

Expand All @@ -566,7 +577,7 @@ mod tests {
let mut vm_tracer = NoopVMTracer;

{
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);
ext.suicide(refund_account).unwrap();
}

Expand All @@ -583,7 +594,7 @@ mod tests {
let mut vm_tracer = NoopVMTracer;

let address = {
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);
match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderAndNonce) {
ContractCreateResult::Created(address, _) => address,
_ => panic!("Test create failed; expected Created, got Failed/Reverted."),
Expand All @@ -603,7 +614,7 @@ mod tests {
let mut vm_tracer = NoopVMTracer;

let address = {
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false);
let mut ext = Externalities::new(state, &setup.env_info, &setup.machine, &setup.schedule, 0, get_test_origin(), &mut setup.sub_state, OutputPolicy::InitContract, &mut tracer, &mut vm_tracer, false, 0);

match ext.create(&U256::max_value(), &U256::zero(), &[], CreateContractAddress::FromSenderSaltAndCodeHash(H256::default())) {
ContractCreateResult::Created(address, _) => address,
Expand Down
14 changes: 11 additions & 3 deletions ethcore/src/json_tests/executive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ impl<'a, T: 'a, V: 'a, B: 'a> TestExt<'a, T, V, B>
let static_call = false;
Ok(TestExt {
nonce: state.nonce(&address)?,
ext: Externalities::new(state, info, machine, schedule, depth, origin_info, substate, output, tracer, vm_tracer, static_call),
ext: Externalities::new(state, info, machine, schedule, depth, origin_info, substate, output, tracer, vm_tracer, static_call, 0),
callcreates: vec![],
sender: address,
})
Expand All @@ -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 reverted_storage_at(&self, _key: &H256) -> vm::Result<H256> {
Ok(H256::zero())
}

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 inc_sstore_refund(&mut self, value: U256) {
self.ext.inc_sstore_refund(value)
}

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

Expand Down
Loading