paradigmxyz / revm-inspectors

EVM Execution Hooks.
Apache License 2.0
186 stars 73 forks source link

Fix gas cost for js tracer #200

Open mattsse opened 2 months ago

mattsse commented 2 months ago

reported: https://github.com/paradigmxyz/reth/issues/10959

we use incorrect gasCost value:

https://github.com/paradigmxyz/revm-inspectors/blob/0648673aab70f83722d5ad2832d023b87c0dcbbe/src/tracing/js/mod.rs#L401-L401

this should be gas cost of the opcode:

https://github.com/ethereum/go-ethereum/blob/0dd173a727dd2d2409b8e401b22e85d20c25b71f/core/vm/interpreter.go#L237-L237

this becomes a bit tricky for dynamic gas, because we don't know how much gas the opcode will cost if we call the tracer in the step fn, only if we track and call it on step_end.

see also:

https://github.com/paradigmxyz/revm-inspectors/blob/0648673aab70f83722d5ad2832d023b87c0dcbbe/src/opcode.rs#L78-L78

so we'd need to move this call to step_end, but this messes with memory because the tracer should be called with mem before opcode I believe...

I don't think there's an easier way to get this than doing

step: gas_remaining step_end: gas_remaining - spent @rakita

DaniPopes commented 2 months ago

Likely same for refund?

jsvisa commented 2 months ago

let me have a try

jsvisa commented 2 months ago

As the gas cost is tracked along with the executing of each op code, Seems couldn't find a proper way to set the cost to the right value before invoking try_step().

How about we calculate the cost like go-ethereum's implemation, something like below:

fn step(&mut self, interp: &mut Interpreter, context: &mut EvmContext<DB>) {
    if self.step_fn.is_none() {
        return;
    }

    let (db, _db_guard) = EvmDbRef::new(&context.journaled_state.state, &context.db);

    let (stack, _stack_guard) = StackRef::new(&interp.stack);
    let (memory, _memory_guard) = MemoryRef::new(&interp.shared_memory);

    // Calculate the cost of the current operation
    let cost = self.calculate_gas_cost(interp, context);

    let step = StepLog {
        stack,
        op: interp.current_opcode().into(),
        memory,
        pc: interp.program_counter() as u64,
        gas_remaining: interp.gas.remaining(),
        cost,
        depth: context.journaled_state.depth(),
        refund: interp.gas.refunded() as u64,
        error: None,
        contract: self.active_call().contract.clone(),
    };

    if self.try_step(step, db).is_err() {
        interp.instruction_result = InstructionResult::Revert;
    }
}

fn calculate_gas_cost(&self, interp: &Interpreter, context: &EvmContext<DB>) -> u64 {
    match interp.current_opcode() {
        Opcode::ADD => 3,
        Opcode::MUL => 5,
        Opcode::SUB => 3,
        // ... other opcodes ...
        Opcode::SSTORE => {
            // SSTORE has a dynamic gas cost calculation
        },
        // ... more opcodes ...
    }
}

But this will require too much re-implemation, and only for the cost/refund field seems not a good deal.

mattsse commented 1 month ago

yeah... the problem is also that we don't get accurate gas costs for dynamic opcodes until after, so we def would need to call the js function on step_end I think, we could likely partially fill the step object on start step and then invoke the js call on step_end

jsvisa commented 1 month ago

yeah... the problem is also that we don't get accurate gas costs for dynamic opcodes until after, so we def would need to call the js function on step_end I think, we could likely partially fill the step object on start step and then invoke the js call on step_end

Sorry for the late reply, I get your point, I'll have another try.

jsvisa commented 1 month ago

@mattsse sorry, I couldn't find out a better solution of the memory/stack messed up, how about we copy the stack/memory in step instead of reference them, so we can use them at the step_end?