From bfc7d1b52325efd84752418378c047f583ca53dd Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Mon, 2 Sep 2019 16:51:18 +0200 Subject: [PATCH 1/5] [trace] add trace_failed to Ext, manage stack of trace data --- ethcore/evm/src/interpreter/mod.rs | 6 +++- ethcore/machine/src/externalities.rs | 4 +++ ethcore/trace/src/executive_tracer.rs | 41 ++++++++++++--------------- ethcore/trace/src/lib.rs | 3 ++ ethcore/vm/src/ext.rs | 3 ++ 5 files changed, 33 insertions(+), 24 deletions(-) diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index 47260139521..f718c31ad4d 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -353,6 +353,7 @@ impl Interpreter { ext.trace_prepare_execute(self.reader.position - 1, opcode, requirements.gas_cost.as_u256(), Self::mem_written(instruction, &self.stack), Self::store_written(instruction, &self.stack)); } if let Err(e) = self.gasometer.as_mut().expect(GASOMETER_PROOF).verify_gas(&requirements.gas_cost) { + ext.trace_failed(); return InterpreterResult::Done(Err(e)); } self.mem.expand(requirements.memory_required_size); @@ -366,7 +367,10 @@ impl Interpreter { let result = match self.exec_instruction( current_gas, ext, instruction, requirements.provide_gas ) { - Err(x) => return InterpreterResult::Done(Err(x)), + Err(x) => { + ext.trace_failed(); + return InterpreterResult::Done(Err(x)); + }, Ok(x) => x, }; evm_debug!({ self.informant.after_instruction(instruction) }); diff --git a/ethcore/machine/src/externalities.rs b/ethcore/machine/src/externalities.rs index b951eba16fa..adab87b0013 100644 --- a/ethcore/machine/src/externalities.rs +++ b/ethcore/machine/src/externalities.rs @@ -440,6 +440,10 @@ impl<'a, T: 'a, V: 'a, B: 'a> Ext for Externalities<'a, T, V, B> self.vm_tracer.trace_prepare_execute(pc, instruction, gas_cost, mem_written, store_written) } + fn trace_failed(&mut self) { + self.vm_tracer.trace_failed(); + } + fn trace_executed(&mut self, gas_used: U256, stack_push: &[U256], mem: &[u8]) { self.vm_tracer.trace_executed(gas_used, stack_push, mem) } diff --git a/ethcore/trace/src/executive_tracer.rs b/ethcore/trace/src/executive_tracer.rs index 23739a34cc2..21da77efb00 100644 --- a/ethcore/trace/src/executive_tracer.rs +++ b/ethcore/trace/src/executive_tracer.rs @@ -18,7 +18,7 @@ use ethereum_types::{U256, Address}; use vm::{Error as VmError, ActionParams}; -use log::{debug, warn}; +use log::debug; use crate::{ Tracer, VMTracer, FlatTrace, trace::{Call, Create, Action, Res, CreateResult, CallResult, VMTrace, VMOperation, VMExecutedOperation, MemoryDiff, StorageDiff, Suicide, Reward, RewardType}, @@ -196,12 +196,16 @@ impl Tracer for ExecutiveTracer { } } +struct TraceData { + mem_written: Option<(usize, usize)>, + store_written: Option<(U256, U256)>, +} + /// Simple VM tracer. Traces all operations. pub struct ExecutiveVMTracer { data: VMTrace, depth: usize, - last_mem_written: Option<(usize, usize)>, - last_store_written: Option<(U256, U256)>, + trace_stack: Vec, } impl ExecutiveVMTracer { @@ -215,8 +219,7 @@ impl ExecutiveVMTracer { subs: vec![], }, depth: 0, - last_mem_written: None, - last_store_written: None, + trace_stack: vec![], } } @@ -243,30 +246,22 @@ impl VMTracer for ExecutiveVMTracer { executed: None, }); }); - self.last_mem_written = mem_written; - self.last_store_written = store_written; + self.trace_stack.push(TraceData { mem_written, store_written }); + } + + fn trace_failed(&mut self) { + let _ = self.trace_stack.pop().expect("pushed in trace_prepare_execute; qed"); } fn trace_executed(&mut self, gas_used: U256, stack_push: &[U256], mem: &[u8]) { - let mem_diff = self.last_mem_written.take().map(|(o, s)| { - if o + s > mem.len() { - warn!( - target: "trace", - "Last mem written is out of bounds {} (mem is {})", - o + s, - mem.len(), - ); - (o, &[][..]) - } else { - (o, &(mem[o..o+s])) - } - }); - let store_diff = self.last_store_written.take(); + let TraceData { mem_written, store_written } = self.trace_stack.pop().expect("pushed in trace_prepare_execute; qed"); + let mem_diff = mem_written.map(|(o, s)| (o, &mem[o..o+s])); + let store_diff = store_written; Self::with_trace_in_depth(&mut self.data, self.depth, move |trace| { let ex = VMExecutedOperation { gas_used: gas_used, - stack_push: stack_push.iter().cloned().collect(), - mem_diff: mem_diff.map(|(s, r)| MemoryDiff { offset: s, data: r.iter().cloned().collect() }), + stack_push: stack_push.to_vec(), + mem_diff: mem_diff.map(|(s, r)| MemoryDiff { offset: s, data: r.to_vec() }), store_diff: store_diff.map(|(l, v)| StorageDiff { location: l, value: v }), }; trace.operations.last_mut().expect("trace_executed is always called after a trace_prepare_execute; trace.operations cannot be empty; qed").executed = Some(ex); diff --git a/ethcore/trace/src/lib.rs b/ethcore/trace/src/lib.rs index 91fead6a8ca..9ff02a7a767 100644 --- a/ethcore/trace/src/lib.rs +++ b/ethcore/trace/src/lib.rs @@ -93,6 +93,9 @@ pub trait VMTracer: Send { /// Trace the preparation to execute a single valid instruction. fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256, _mem_written: Option<(usize, usize)>, _store_written: Option<(U256, U256)>) {} + /// Trace the execution failure of a single instruction. + fn trace_failed(&mut self) {} + /// Trace the finalised execution of a single valid instruction. fn trace_executed(&mut self, _gas_used: U256, _stack_push: &[U256], _mem: &[u8]) {} diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index 68e5822e451..1137f279ec9 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -166,6 +166,9 @@ pub trait Ext { /// Prepare to trace an operation. Passthrough for the VM trace. fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256, _mem_written: Option<(usize, usize)>, _store_written: Option<(U256, U256)>) {} + /// Trace the execution failure of a single instruction. + fn trace_failed(&mut self) {} + /// Trace the finalised execution of a single instruction. fn trace_executed(&mut self, _gas_used: U256, _stack_push: &[U256], _mem: &[u8]) {} From de1efb2630305ce5e3c097b9410e56d33f9be468 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Tue, 3 Sep 2019 14:26:37 +0200 Subject: [PATCH 2/5] [evm] call trace_failed only if self.do_trace --- ethcore/evm/src/interpreter/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index f718c31ad4d..5e4ad1e0e62 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -353,7 +353,9 @@ impl Interpreter { ext.trace_prepare_execute(self.reader.position - 1, opcode, requirements.gas_cost.as_u256(), Self::mem_written(instruction, &self.stack), Self::store_written(instruction, &self.stack)); } if let Err(e) = self.gasometer.as_mut().expect(GASOMETER_PROOF).verify_gas(&requirements.gas_cost) { - ext.trace_failed(); + if self.do_trace { + ext.trace_failed(); + } return InterpreterResult::Done(Err(e)); } self.mem.expand(requirements.memory_required_size); @@ -368,7 +370,9 @@ impl Interpreter { current_gas, ext, instruction, requirements.provide_gas ) { Err(x) => { - ext.trace_failed(); + if self.do_trace { + ext.trace_failed(); + } return InterpreterResult::Done(Err(x)); }, Ok(x) => x, From 020b324db03e60363078bebc9bce80297b9c2ce5 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Tue, 3 Sep 2019 15:09:46 +0200 Subject: [PATCH 3/5] [evm] add a comment about do_trace set to true --- ethcore/evm/src/interpreter/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ethcore/evm/src/interpreter/mod.rs b/ethcore/evm/src/interpreter/mod.rs index 5e4ad1e0e62..ba64dbaed4e 100644 --- a/ethcore/evm/src/interpreter/mod.rs +++ b/ethcore/evm/src/interpreter/mod.rs @@ -279,6 +279,8 @@ impl Interpreter { cache, params, reader, informant, valid_jump_destinations, gasometer, stack, done: false, + // Overridden in `step_inner` based on + // the result of `ext.trace_next_instruction`. do_trace: true, mem: Vec::new(), return_data: ReturnData::empty(), From 8196c62480ffe33a6a6a3900073b08a82f863ab4 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Wed, 4 Sep 2019 12:04:44 +0200 Subject: [PATCH 4/5] [vm] improve the doc in trace_prepare_execute --- ethcore/vm/src/ext.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ethcore/vm/src/ext.rs b/ethcore/vm/src/ext.rs index 1137f279ec9..bb153acff54 100644 --- a/ethcore/vm/src/ext.rs +++ b/ethcore/vm/src/ext.rs @@ -164,6 +164,7 @@ pub trait Ext { fn trace_next_instruction(&mut self, _pc: usize, _instruction: u8, _current_gas: U256) -> bool { false } /// Prepare to trace an operation. Passthrough for the VM trace. + /// For each call of `trace_prepare_execute` either `trace_failed` or `trace_executed` MUST be called. fn trace_prepare_execute(&mut self, _pc: usize, _instruction: u8, _gas_cost: U256, _mem_written: Option<(usize, usize)>, _store_written: Option<(U256, U256)>) {} /// Trace the execution failure of a single instruction. From f818b23a3e91546994b5985a10bfd080f15a0058 Mon Sep 17 00:00:00 2001 From: Andronik Ordian Date: Thu, 5 Sep 2019 14:24:53 +0200 Subject: [PATCH 5/5] [trace] add the bounds check back --- ethcore/trace/src/executive_tracer.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ethcore/trace/src/executive_tracer.rs b/ethcore/trace/src/executive_tracer.rs index 21da77efb00..7c6dbc66261 100644 --- a/ethcore/trace/src/executive_tracer.rs +++ b/ethcore/trace/src/executive_tracer.rs @@ -16,9 +16,10 @@ //! Simple executive tracer. +use std::cmp::min; use ethereum_types::{U256, Address}; use vm::{Error as VmError, ActionParams}; -use log::debug; +use log::{debug, warn}; use crate::{ Tracer, VMTracer, FlatTrace, trace::{Call, Create, Action, Res, CreateResult, CallResult, VMTrace, VMOperation, VMExecutedOperation, MemoryDiff, StorageDiff, Suicide, Reward, RewardType}, @@ -255,7 +256,12 @@ impl VMTracer for ExecutiveVMTracer { fn trace_executed(&mut self, gas_used: U256, stack_push: &[U256], mem: &[u8]) { let TraceData { mem_written, store_written } = self.trace_stack.pop().expect("pushed in trace_prepare_execute; qed"); - let mem_diff = mem_written.map(|(o, s)| (o, &mem[o..o+s])); + let mem_diff = mem_written.map(|(o, s)| { + if o + s > mem.len() { + warn!(target: "trace", "mem_written is out of bounds"); + } + (o, &mem[min(mem.len(), o)..min(o + s, mem.len())]) + }); let store_diff = store_written; Self::with_trace_in_depth(&mut self.data, self.depth, move |trace| { let ex = VMExecutedOperation {