Enable tracing in the execution layer #21165

awelc · 2025-02-10T21:47:49Z

Description

The main idea here is that we have a client (e.g., the upcoming replay tool) at some point calling the execute_transaction_to_effects from the Executor trait defined in sui/sui-execution/src/executor.rs to execute a PTB. We want execution of a PTB to be traced, the trace "returned" to the client which is responsible for delivering this trace to the user (e.g., by storing in to disk). In other words, we want the executor to be responsible for execution and tracing itself, and the client to be responsible for all the remaining handling of the generated trace (e.g., I/O operations). There are arguably many ways to accomplish this and this PR proposes one of them - passing optional MoveTraceBuilder as an additional argument to execute_transaction_to_effects.

Ultimately, we want execution of the entire PTB to be traced, but in this PR we only trace execution of Move calls. Tracing of the remaining PTB commands will be added at a later time via external TraceEvents, without further changing the interface being modified in this PR.

MoveTraceBuilder creates an instance of MoveTrace, which contains a list of trace events including TraceEvent::External. This type of event can represent an arbitrary value, including arguments and return values of PTB commands.

vercel · 2025-02-10T21:47:54Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 13, 2025 0:55am

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview		Feb 13, 2025 0:55am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview		Feb 13, 2025 0:55am

amnn

I think the way it's piped through the executor seems fine, but the two main themes to my questions/comments are:

Maybe it's worth plumbing it through to all the special case places we may want to support tracing + replay in now when the compiler is directing us to what needs to change, rather than later when we'll need to figure out which code path to switch up the None to a Some(_) in.
There's something funky going on, lifetime-wise, with the trace builder that's probably worth cleaning up -- it should be possible to pass it through to wherever it's needed without having to repeatedly unwrap it, pass it down and then rewrap it.

amnn · 2025-02-11T10:25:38Z

sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs

+        // This function is executed in a loop which makes passing `Option<&mut MoveTraceBuilder>`
+        // as an argument impossible. Instead, we "rebuild" a value of this type here
+        // too make Rust's borrow checker happy.
+        // TODO: Use some Rust magic to avoid this?


Couple of things I would try here:

Is there a reason to use &mut Option<&mut MoveTraceBuilder> instead of just Option<&mut MoveTraceBuilder>, if not, simplifying that may help Rust decipher the lifetimes here. Perhaps the easiest of all is to pass in a &mut Option<MoveTraceBuilder> from the very top?

Alternatively/additionally using an explicit lifetime parameter -- &'t mut MoveTraceBuilder -- may also help Rust realise that it's merged too many lifetimes together.

Is there a reason to use &mut Option<&mut MoveTraceBuilder> instead of just Option<&mut MoveTraceBuilder>

I was trying to explain it in the comment but clearly failed... This function is executed in a loop:

for (idx, command) in commands.into_iter().enumerate() { let start = Instant::now(); if let Err(err) = execute_command::<Mode>( &mut context, &mut mode_results, command, &mut trace_builder_opt, ) ...

With trace_builder_opt being Option<&mut MoveTraceBuilder>, the borrow checker complains about it, and cloning this value is also not working due to the Option holding a reference. It does seem like there should be a better way to handle this but I could not figure it out...

Perhaps the easiest of all is to pass in a &mut Option<MoveTraceBuilder> from the very top?

I did not think about this as I was fixated on Option<&mut MoveTraceBuilder> being needed down the line but this should work (and let us avoid the loop-related issue) with the as_mut conversion when needed.

amnn · 2025-02-11T10:25:50Z

sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs

@@ -951,6 +993,7 @@ mod checked {
                vec![],
                vec![],
                /* is_init */ true,
+                None, // TODO: add tracing for init functions?


Is there a reason not to do this to begin with?

I was thinking about it from the trace-debugging perspective (it was not super clear to me how/where/when to visualize these as part of a PTB execution flow) but these traces obviously do not have to be consumed by the trace debugger only, so they should indeed be generated here (even we decide for the trace debugger to ignore them initially).

amnn · 2025-02-11T10:30:24Z

sui-execution/latest/sui-adapter/src/execution_engine.rs

@@ -903,6 +911,7 @@ mod checked {
            tx_ctx,
            gas_charger,
            advance_epoch_pt,
+            None,


Would anything go wrong if we added support for tracing system transactions? In the context of Replay that should be possible, and it's something that would certainly help us debug issues during reconfiguration etc.

I did not think that replay tool will be able to handle these but I guess everything that's executed from execution_loop is fair game. Will add these!

amnn · 2025-02-11T10:31:33Z

sui-execution/latest/sui-adapter/src/programmable_transactions/context.rs

@@ -963,6 +964,7 @@ mod checked {
            function_name: &IdentStr,
            ty_args: Vec<Type>,
            args: Vec<impl Borrow<[u8]>>,
+            tracer: Option<&mut MoveTraceBuilder>,


Should the tracer be part of the ExecutionContext?

Not sure how useful it would be as I am not sure it will be used anywhere else other than runtime's execute_function_bypass_visibility function

awelc · 2025-02-11T19:39:20Z

I think the way it's piped through the executor seems fine, but the two main themes to my questions/comments are:

Maybe it's worth plumbing it through to all the special case places we may want to support tracing + replay in now when the compiler is directing us to what needs to change, rather than later when we'll need to figure out which code path to switch up the None to a Some(_) in.

There's something funky going on, lifetime-wise, with the trace builder that's probably worth cleaning up -- it should be possible to pass it through to wherever it's needed without having to repeatedly unwrap it, pass it down and then rewrap it.

I addressed both issues:

added tracing parameter to all functions accessible from execution_loop
changed tracer parameter type from Option<&mut MoveTraceBuilder> to &mut Option<MoveTraceBuilder> which fixed the argument passing problem

tzakian

LGTM although I think I'd like to cleanup the Move deps first before landing this.

tzakian · 2025-02-13T00:02:02Z

Cargo.toml

+move-trace-format = { path = "external-crates/move/crates/move-trace-format" }
+move-proc-macros = { path = "external-crates/move/crates/move-proc-macros" }
+enum-compat-util = { path = "external-crates/move/crates/enum-compat-util" }


This is a bit of a "my bad" situation here -- I don't think move-trace-format needs all these different crates (or at least doesn't use them). So I think removing them from the move-trace-format crate should allow you to remove them here and just import the move-trace-format crate.

tzakian · 2025-02-13T00:02:33Z

external-crates/move/crates/move-trace-format/src/format.rs

@@ -217,6 +217,12 @@ impl MoveTrace {
    }
 }

+impl Default for MoveTrace {


Oh clippy...

We have this clippy note disabled in external-crates fwiw. So we can remove this if we want

We have this clippy note disabled in external-crates fwiw. So we can remove this if we want

Fwiw, I got clippy complaining about it in CI...

tzakian · 2025-02-13T00:03:23Z

external-crates/move/crates/move-trace-format/src/memory_tracer.rs

@@ -138,6 +138,12 @@ impl TraceState {
    }
 }

+impl Default for TraceState {


tzakian · 2025-02-13T00:06:49Z

sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs

-        let mut result = context
-            .execute_function_bypass_visibility(
+        let mut result = if trace_builder_opt.is_some() {
+            context.execute_function_bypass_visibility(


I think since it's using the same entrypoint this if is no longer needed here

Good point! I was looking at it without actually seeing it...

amnn

LGTM as well!

awelc requested review from amnn, tnowacki, tzakian and dariorussi February 10, 2025 21:47

awelc self-assigned this Feb 10, 2025

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 21:47 — with GitHub Actions Inactive

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 21:56 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 10, 2025 22:00 View deployment

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 23:11 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 10, 2025 23:12 View deployment

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 23:27 — with GitHub Actions Inactive

awelc added 6 commits February 10, 2025 15:28

Enable tracing in the execution layer

c71c190

Added required manifest changes

c3f6693

Made clippy happy

c5d0520

More manifest changes

38b3896

Added missing arguments

fac0398

Added missing publish field

f628be8

awelc force-pushed the aw/execution-enable-tracing branch from e23c10d to f628be8 Compare February 10, 2025 23:28

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 23:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 10, 2025 23:30 View deployment

awelc added 3 commits February 10, 2025 15:42

Added missing argument

163ddeb

Added missing argument

8720ed2

Made clippy happy

cef8735

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 23:47 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 10, 2025 23:49 View deployment

Fixed manifest file

f60a4c1

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 11, 2025 00:56 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 11, 2025 00:58 View deployment

amnn reviewed Feb 11, 2025

View reviewed changes

awelc added 2 commits February 11, 2025 11:16

More elegant way of passing trace builder

6ccdaf6

Enable tracing for more transactions

d6662c0

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 11, 2025 19:37 — with GitHub Actions Inactive

awelc requested a review from amnn February 11, 2025 19:39

vercel bot deployed to Preview – sui-docs February 11, 2025 19:39 View deployment

Fixed argument type

343f263

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 11, 2025 19:42 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 11, 2025 19:43 View deployment

tzakian approved these changes Feb 13, 2025

View reviewed changes

Addressed review comments

bb76c6b

awelc temporarily deployed to sui-typescript-aws-kms-test-env February 13, 2025 00:54 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 13, 2025 00:55 View deployment

amnn approved these changes Feb 13, 2025

View reviewed changes

tzakian mentioned this pull request Feb 13, 2025

[move] Cleanup move-trace-format deps #21197

Closed

awelc merged commit fd40b0f into main Feb 13, 2025
47 checks passed

awelc deleted the aw/execution-enable-tracing branch February 13, 2025 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable tracing in the execution layer #21165

Enable tracing in the execution layer #21165

awelc commented Feb 10, 2025 •

edited

Loading

vercel bot commented Feb 10, 2025 •

edited

Loading

amnn left a comment

amnn Feb 11, 2025

awelc Feb 11, 2025

amnn Feb 11, 2025

awelc Feb 11, 2025

amnn Feb 11, 2025

awelc Feb 11, 2025

amnn Feb 11, 2025

awelc Feb 11, 2025

awelc commented Feb 11, 2025

tzakian left a comment

tzakian Feb 13, 2025

tzakian Feb 13, 2025

tnowacki Feb 14, 2025

awelc Feb 14, 2025

tzakian Feb 13, 2025

tzakian Feb 13, 2025

awelc Feb 13, 2025

amnn left a comment

Enable tracing in the execution layer #21165

Enable tracing in the execution layer #21165

Conversation

awelc commented Feb 10, 2025 • edited Loading

Description

vercel bot commented Feb 10, 2025 • edited Loading

amnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awelc commented Feb 11, 2025

tzakian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amnn left a comment

Choose a reason for hiding this comment

awelc commented Feb 10, 2025 •

edited

Loading

vercel bot commented Feb 10, 2025 •

edited

Loading