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

Execution trace recording #881

Closed
wants to merge 13 commits into from
Closed

Execution trace recording #881

wants to merge 13 commits into from

Conversation

Dentosal
Copy link
Member

@Dentosal Dentosal commented Dec 9, 2024

Closes #867. Core PR FuelLabs/fuel-core#2491.

Introduces a mechanism for recording an execution trace, which can be used to inspect VM states after execution without having to use a debugger. Most importantly it allows reading registers and memory at certain points of the exection, which is quite useful for tooling like indexers.

Checklist

  • Breaking changes recorded in changelog
  • New behavior is reflected in tests
  • If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
  • The specification matches the implemented behavior (no spec changes)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

@Dentosal Dentosal added enhancement New feature or request fuel-vm Related to the `fuel-vm` crate. labels Dec 9, 2024
@Dentosal Dentosal self-assigned this Dec 9, 2024
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of the code from this PR can live in the fuel-core and there we can gather tracer via the Tracer trait

@@ -80,8 +80,15 @@ where
}
}

self.instruction_inner(raw.into())
.map_err(|e| InterpreterError::from_runtime(e, raw.into()))
let result = self
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be more beneficial if, on the creation of the interpreter, we will pass some object that implements traits like Tracer or something.

trait Tracer<VM> {
    fn trace_instruction_before_execution(
        &mut self,
        instruction: &Intstuction,
        vm: &mut VM,
    );

    fn trace_instruction_after_execution(
        &mut self,
        instruction: &Intstuction,
        vm: &mut VM,
    );
}

In this case, we will have more flexibility in the implementation and can define logic to get traces on the fuel-core side. In the case of tooling, they can maybe create a local debugger. In the case of indexers, they can index something inside of these functions.

Of course, this solution means that we need to have getters for all internal fields, including MemoryInstance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nobody is actually going to use this, and it's an unnecessary complicaton. But it would be nice to be wrong about this, and it's not that much extra complication.

Fortunately we already support almost all fields this needs, as ECAL support is also done like this.

let mut changes = Vec::new();
let mut current_change: Option<MemorySliceChange> = None;

for i in 0..MEM_SIZE {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, it will be very expensive to do 10^7 iterations per each instruction=D It would be nice to have something more performant=)

If we go for now with Tracer approach, we can decide later in the fuel-core how do we want to do that in an optimal way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very aware of this. I have lots of ideas on how to optimize this, and I'm going to write some benchmarks to see what approach is the right one.

@Dentosal
Copy link
Member Author

Closing in favor of storage access recording and debugger -based approach, that doesn't require VM changes.

@Dentosal Dentosal closed this Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fuel-vm Related to the `fuel-vm` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execution traces
2 participants