Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gas cost for js tracer #200

Open
mattsse opened this issue Sep 17, 2024 · 6 comments · May be fixed by #220
Open

Fix gas cost for js tracer #200

mattsse opened this issue Sep 17, 2024 · 6 comments · May be fixed by #220

Comments

@mattsse
Copy link
Contributor

mattsse commented Sep 17, 2024

reported: paradigmxyz/reth#10959

we use incorrect gasCost value:

cost: interp.gas.spent(),

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:

let gas_cost = gas_remaining.saturating_sub(interp.gas().remaining());

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
Copy link
Member

Likely same for refund?

@jsvisa
Copy link
Contributor

jsvisa commented Sep 23, 2024

let me have a try

@jsvisa
Copy link
Contributor

jsvisa commented Sep 26, 2024

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

mattsse commented Sep 30, 2024

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

jsvisa commented Oct 7, 2024

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

jsvisa commented Oct 10, 2024

@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?

  • pro: the data is unbroken between step and step_end
  • con: obviously, need to extra copy at most 1024 stack and 4KB memory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants