From f4453f77b84d9d5a19e6c33f2793e331759285bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Mon, 10 Jul 2017 13:23:40 +0200 Subject: [PATCH] Fix output of parity-evm in case of bad instruction (#5955) * Fix output of evmbin. * Use foundation chain settings by default. --- ethcore/src/evm/ext.rs | 5 ++++- ethcore/src/evm/interpreter/mod.rs | 13 +++++++++---- ethcore/src/executive.rs | 4 ++-- ethcore/src/externalities.rs | 6 +++++- ethcore/src/trace/executive_tracer.rs | 9 +++++---- ethcore/src/trace/mod.rs | 15 ++++++++++----- ethcore/src/trace/noop_tracer.rs | 11 ++++------- evmbin/src/display/json.rs | 19 ++++++++----------- evmbin/src/display/simple.rs | 2 +- evmbin/src/main.rs | 2 +- 10 files changed, 49 insertions(+), 37 deletions(-) diff --git a/ethcore/src/evm/ext.rs b/ethcore/src/evm/ext.rs index c66d1d3ff..3a8dafc37 100644 --- a/ethcore/src/evm/ext.rs +++ b/ethcore/src/evm/ext.rs @@ -129,8 +129,11 @@ pub trait Ext { /// Increments sstore refunds count by 1. fn inc_sstore_clears(&mut self); + /// Decide if any more operations should be traced. Passthrough for the VM trace. + fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8) -> bool { false } + /// Prepare to trace an operation. Passthrough for the VM trace. - fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: &U256) -> bool { false } + fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256) {} /// Trace the finalised execution of a single instruction. fn trace_executed(&mut self, _gas_used: U256, _stack_push: &[U256], _mem_diff: Option<(usize, &[u8])>, _store_diff: Option<(U256, U256)>) {} diff --git a/ethcore/src/evm/interpreter/mod.rs b/ethcore/src/evm/interpreter/mod.rs index 7b0becdb7..304d93b54 100644 --- a/ethcore/src/evm/interpreter/mod.rs +++ b/ethcore/src/evm/interpreter/mod.rs @@ -111,6 +111,7 @@ impl evm::Evm for Interpreter { self.mem.clear(); let mut informant = informant::EvmInformant::new(ext.depth()); + let mut do_trace = true; let code = ¶ms.code.as_ref().expect("exec always called with code; qed"); let mut valid_jump_destinations = None; @@ -124,13 +125,17 @@ impl evm::Evm for Interpreter { let instruction = code[reader.position]; reader.position += 1; + // TODO: make compile-time removable if too much of a performance hit. + do_trace = do_trace && ext.trace_next_instruction(reader.position - 1, instruction); + let info = &infos[instruction as usize]; self.verify_instruction(ext, instruction, info, &stack)?; // Calculate gas cost let requirements = gasometer.requirements(ext, instruction, info, &stack, self.mem.size())?; - // TODO: make compile-time removable if too much of a performance hit. - let trace_executed = ext.trace_prepare_execute(reader.position - 1, instruction, &requirements.gas_cost.as_u256()); + if do_trace { + ext.trace_prepare_execute(reader.position - 1, instruction, requirements.gas_cost.as_u256()); + } gasometer.verify_gas(&requirements.gas_cost)?; self.mem.expand(requirements.memory_required_size); @@ -139,7 +144,7 @@ impl evm::Evm for Interpreter { evm_debug!({ informant.before_instruction(reader.position, instruction, info, &gasometer.current_gas, &stack) }); - let (mem_written, store_written) = match trace_executed { + let (mem_written, store_written) = match do_trace { true => (Self::mem_written(instruction, &stack), Self::store_written(instruction, &stack)), false => (None, None), }; @@ -155,7 +160,7 @@ impl evm::Evm for Interpreter { gasometer.current_gas = gasometer.current_gas + *gas; } - if trace_executed { + if do_trace { ext.trace_executed(gasometer.current_gas.as_u256(), stack.peek_top(info.ret), mem_written.map(|(o, s)| (o, &(self.mem[o..(o + s)]))), store_written); } diff --git a/ethcore/src/executive.rs b/ethcore/src/executive.rs index 0d62831d6..07b7b67c3 100644 --- a/ethcore/src/executive.rs +++ b/ethcore/src/executive.rs @@ -376,7 +376,7 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> { self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::Return(output, trace_output.as_mut()), &mut subtracer, &mut subvmtracer) }; - vm_tracer.done_subtrace(subvmtracer, res.is_ok()); + vm_tracer.done_subtrace(subvmtracer); trace!(target: "executive", "res={:?}", res); @@ -457,7 +457,7 @@ impl<'a, B: 'a + StateBackend, E: Engine + ?Sized> Executive<'a, B, E> { self.exec_vm(params, &mut unconfirmed_substate, OutputPolicy::InitContract(trace_output.as_mut()), &mut subtracer, &mut subvmtracer) }; - vm_tracer.done_subtrace(subvmtracer, res.is_ok()); + vm_tracer.done_subtrace(subvmtracer); match res { Ok(ref res) => tracer.trace_create( diff --git a/ethcore/src/externalities.rs b/ethcore/src/externalities.rs index 3d30abf2a..0f0310649 100644 --- a/ethcore/src/externalities.rs +++ b/ethcore/src/externalities.rs @@ -379,7 +379,11 @@ impl<'a, T: 'a, V: 'a, B: 'a, E: 'a> Ext for Externalities<'a, T, V, B, E> self.substate.sstore_clears_count = self.substate.sstore_clears_count + U256::one(); } - fn trace_prepare_execute(&mut self, pc: usize, instruction: u8, gas_cost: &U256) -> bool { + fn trace_next_instruction(&mut self, pc: usize, instruction: u8) -> bool { + self.vm_tracer.trace_next_instruction(pc, instruction) + } + + fn trace_prepare_execute(&mut self, pc: usize, instruction: u8, gas_cost: U256) { self.vm_tracer.trace_prepare_execute(pc, instruction, gas_cost) } diff --git a/ethcore/src/trace/executive_tracer.rs b/ethcore/src/trace/executive_tracer.rs index fb2f91a2f..9dddd214a 100644 --- a/ethcore/src/trace/executive_tracer.rs +++ b/ethcore/src/trace/executive_tracer.rs @@ -192,14 +192,15 @@ impl ExecutiveVMTracer { } impl VMTracer for ExecutiveVMTracer { - fn trace_prepare_execute(&mut self, pc: usize, instruction: u8, gas_cost: &U256) -> bool { + fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8) -> bool { true } + + fn trace_prepare_execute(&mut self, pc: usize, instruction: u8, gas_cost: U256) { self.data.operations.push(VMOperation { pc: pc, instruction: instruction, - gas_cost: gas_cost.clone(), + gas_cost: gas_cost, executed: None, }); - true } fn trace_executed(&mut self, gas_used: U256, stack_push: &[U256], mem_diff: Option<(usize, &[u8])>, store_diff: Option<(U256, U256)>) { @@ -221,7 +222,7 @@ impl VMTracer for ExecutiveVMTracer { }} } - fn done_subtrace(&mut self, sub: Self, _is_successful: bool) { + fn done_subtrace(&mut self, sub: Self) { self.data.subs.push(sub.data); } diff --git a/ethcore/src/trace/mod.rs b/ethcore/src/trace/mod.rs index a7eea218b..c6320b34c 100644 --- a/ethcore/src/trace/mod.rs +++ b/ethcore/src/trace/mod.rs @@ -87,18 +87,23 @@ pub trait Tracer: Send { /// Used by executive to build VM traces. pub trait VMTracer: Send { - /// Trace the preparation to execute a single instruction. - /// @returns true if `trace_executed` should be called. - fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: &U256) -> bool { false } - /// Trace the finalised execution of a single instruction. + /// Trace the progression of interpreter to next instruction. + /// If tracer returns `false` it won't be called again. + /// @returns true if `trace_prepare_execute` and `trace_executed` should be called. + fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8) -> bool { false } + + /// Trace the preparation to execute a single valid instruction. + fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256) {} + + /// Trace the finalised execution of a single valid instruction. fn trace_executed(&mut self, _gas_used: U256, _stack_push: &[U256], _mem_diff: Option<(usize, &[u8])>, _store_diff: Option<(U256, U256)>) {} /// Spawn subtracer which will be used to trace deeper levels of execution. fn prepare_subtrace(&self, code: &[u8]) -> Self where Self: Sized; /// Finalize subtracer. - fn done_subtrace(&mut self, sub: Self, is_successful: bool) where Self: Sized; + fn done_subtrace(&mut self, sub: Self) where Self: Sized; /// Consumes self and returns the VM trace. fn drain(self) -> Option; diff --git a/ethcore/src/trace/noop_tracer.rs b/ethcore/src/trace/noop_tracer.rs index d8502a4a8..694d1b094 100644 --- a/ethcore/src/trace/noop_tracer.rs +++ b/ethcore/src/trace/noop_tracer.rs @@ -71,18 +71,15 @@ impl Tracer for NoopTracer { pub struct NoopVMTracer; impl VMTracer for NoopVMTracer { - /// Trace the preparation to execute a single instruction. - fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: &U256) -> bool { false } + fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8) -> bool { false } + + fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256) {} - /// Trace the finalised execution of a single instruction. fn trace_executed(&mut self, _gas_used: U256, _stack_push: &[U256], _mem_diff: Option<(usize, &[u8])>, _store_diff: Option<(U256, U256)>) {} - /// Spawn subtracer which will be used to trace deeper levels of execution. fn prepare_subtrace(&self, _code: &[u8]) -> Self { NoopVMTracer } - /// Spawn subtracer which will be used to trace deeper levels of execution. - fn done_subtrace(&mut self, _sub: Self, _is_successful: bool) {} + fn done_subtrace(&mut self, _sub: Self) {} - /// Consumes self and returns all VM traces. fn drain(self) -> Option { None } } diff --git a/evmbin/src/display/json.rs b/evmbin/src/display/json.rs index 1791640d6..13cba90bb 100644 --- a/evmbin/src/display/json.rs +++ b/evmbin/src/display/json.rs @@ -81,13 +81,18 @@ impl vm::Informant for Informant { } impl trace::VMTracer for Informant { - fn trace_prepare_execute(&mut self, pc: usize, instruction: u8, gas_cost: &U256) -> bool { + fn trace_next_instruction(&mut self, pc: usize, instruction: u8) -> bool { self.pc = pc; self.instruction = instruction; - self.gas_cost = *gas_cost; true } + fn trace_prepare_execute(&mut self, pc: usize, instruction: u8, gas_cost: U256) { + self.pc = pc; + self.instruction = instruction; + self.gas_cost = gas_cost; + } + fn trace_executed(&mut self, gas_used: U256, stack_push: &[U256], mem_diff: Option<(usize, &[u8])>, store_diff: Option<(U256, U256)>) { let info = evm::INSTRUCTIONS[self.instruction as usize]; @@ -127,17 +132,9 @@ impl trace::VMTracer for Informant { vm } - fn done_subtrace(&mut self, mut sub: Self, is_successful: bool) where Self: Sized { + fn done_subtrace(&mut self, mut sub: Self) { if sub.depth == 1 { // print last line with final state: - if is_successful { - sub.pc += 1; - sub.instruction = 0; - } else { - let push_bytes = evm::push_bytes(sub.instruction); - sub.pc += if push_bytes > 0 { push_bytes + 1 } else { 0 }; - sub.instruction = if sub.pc < sub.code.len() { sub.code[sub.pc] } else { 0 }; - } sub.gas_cost = 0.into(); let gas_used = sub.gas_used; trace::VMTracer::trace_executed(&mut sub, gas_used, &[], None, None); diff --git a/evmbin/src/display/simple.rs b/evmbin/src/display/simple.rs index 46a57bab0..9f8f7ee14 100644 --- a/evmbin/src/display/simple.rs +++ b/evmbin/src/display/simple.rs @@ -44,6 +44,6 @@ impl vm::Informant for Informant { impl trace::VMTracer for Informant { fn prepare_subtrace(&self, _code: &[u8]) -> Self where Self: Sized { Default::default() } - fn done_subtrace(&mut self, _sub: Self, _is_successful: bool) where Self: Sized {} + fn done_subtrace(&mut self, _sub: Self) {} fn drain(self) -> Option { None } } diff --git a/evmbin/src/main.rs b/evmbin/src/main.rs index 305b2cbd4..98d61b417 100644 --- a/evmbin/src/main.rs +++ b/evmbin/src/main.rs @@ -137,7 +137,7 @@ impl Args { spec::Spec::load(file)? }, None => { - spec::Spec::new_instant() + ethcore::ethereum::new_foundation() }, }) }