lambdaclass / era_vm

EraVM implementation
MIT License
20 stars 3 forks source link

Rethink how to move pc #141

Closed MarcosNicolau closed 3 months ago

MarcosNicolau commented 3 months ago

Description

After every instruction loop, we increment the pc with the opcode_pc_set function. On far_calls, near_calls, jump, ret(Panic) and ret(Revert) opcodes the pc should not get incremented. We are working this around by decreasing the pc by one, except for the far_call, where we are actually contemplating that behavior:

fn opcode_pc_set(opcode: &Opcode, current_pc: u64) -> u64 {
    match opcode.variant {
        Variant::FarCall(_) => 0,
        _ => current_pc + 1,
    }
}

Also, whenever there is an inexplicit_panic, we can't use the opcode_pc_set function, since a far_call can actually fail and then pc would not be correctly set as the pc(since we are decrementing it to account for the later increment, see here). In such cases we are always incrementing the pc:

match inexplicit_panic(&mut vm, storage) {
    Ok(false) => {
        vm.current_frame_mut()?.pc += 1;
        continue;
    }
    _ => return Ok((ExecutionOutput::Panic, vm)),
}

This makes the management of the pc to be spread across the whole codebase, adding unrelated logic inside other functions were we need to account for the pc number.

Possible solution

My proposed solution is to extend the matching in the op_code_pc_set so that we centralized the pc management and we don't have them all around the codebase. For example:

// Set the next PC according to the next opcode
fn opcode_pc_set(opcode: &Opcode, current_pc: u64) -> u64 {
    match opcode.variant {
        Variant::FarCall(_) => 0,
        Variant::NearCall(_) => 0,
        Variant::Ret(RetOpcode::Panic) => 0,
        Variant::Ret(RetOpcode::Revert) => 0,
        Variant::Jump(_) => 0,
        _ =>  1,
    }
}