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

Enable tracing in the execution layer #21165

Merged
merged 14 commits into from
Feb 13, 2025
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ exclude = [
"external-crates/move/crates/move-stdlib-natives",
"external-crates/move/crates/move-symbol-pool",
"external-crates/move/crates/move-transactional-test-runner",
"external-crates/move/crates/move-trace-format",
"external-crates/move/crates/move-unit-test",
"external-crates/move/crates/move-vm-config",
"external-crates/move/crates/move-vm-integration-tests",
Expand Down Expand Up @@ -584,6 +585,9 @@ move-symbol-pool = { path = "external-crates/move/crates/move-symbol-pool" }
move-abstract-interpreter = { path = "external-crates/move/crates/move-abstract-interpreter" }
move-abstract-stack = { path = "external-crates/move/crates/move-abstract-stack" }
move-analyzer = { path = "external-crates/move/crates/move-analyzer" }
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" }
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


fastcrypto = { git = "https://github.com/MystenLabs/fastcrypto", rev = "69d496c71fb37e3d22fe85e5bbfd4256d61422b9" }
fastcrypto-tbls = { git = "https://github.com/MystenLabs/fastcrypto", rev = "69d496c71fb37e3d22fe85e5bbfd4256d61422b9" }
Expand Down
1 change: 1 addition & 0 deletions crates/simulacrum/src/epoch_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ impl EpochState {
kind,
signer,
tx_digest,
None,
);
Ok((inner_temp_store, gas_status, effects, result))
}
Expand Down
3 changes: 3 additions & 0 deletions crates/sui-core/src/authority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1688,6 +1688,7 @@ impl AuthorityState {
kind,
signer,
tx_digest,
None,
);

fail_point_if!("cp_execution_nondeterminism", || {
Expand Down Expand Up @@ -1880,6 +1881,7 @@ impl AuthorityState {
kind,
signer,
transaction_digest,
None,
);
let tx_digest = *effects.transaction_digest();

Expand Down Expand Up @@ -2070,6 +2072,7 @@ impl AuthorityState {
kind,
signer,
transaction.digest(),
None,
);

Ok(SimulateTransactionResult {
Expand Down
1 change: 1 addition & 0 deletions crates/sui-genesis-builder/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -940,6 +940,7 @@ fn create_genesis_transaction(
kind,
signer,
genesis_digest,
None,
);
assert!(inner_temp_store.input_objects.is_empty());
assert!(inner_temp_store.mutable_inputs.is_empty());
Expand Down
2 changes: 2 additions & 0 deletions crates/sui-replay/src/replay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,7 @@ impl LocalExec {
transaction_kind.clone(),
tx_info.sender,
*tx_digest,
None,
);

if let Err(err) = self.pretty_print_for_tracing(
Expand Down Expand Up @@ -939,6 +940,7 @@ impl LocalExec {
kind,
signer,
*executable.digest(),
None,
);

let effects =
Expand Down
1 change: 1 addition & 0 deletions crates/sui-single-node-benchmark/src/single_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ impl SingleValidator {
kind,
signer,
*executable.digest(),
None,
);
assert!(effects.status().is_ok());
store.commit_objects(inner_temp_store);
Expand Down
1 change: 1 addition & 0 deletions crates/sui-swarm-config/src/network_config_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ mod test {
kind,
signer,
genesis_digest,
None,
);

assert_eq!(&effects, genesis.effects());
Expand Down
1 change: 1 addition & 0 deletions external-crates/move/crates/move-trace-format/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ version = "0.0.1"
authors = ["Move Core Contributors"]
description = "Move Trace Format"
license = "Apache-2.0"
publish = false
edition = "2021"

[dependencies]
Expand Down
12 changes: 12 additions & 0 deletions external-crates/move/crates/move-trace-format/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,12 @@ impl MoveTrace {
}
}

impl Default for MoveTrace {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh clippy...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

fn default() -> Self {
Self::new()
}
}

impl MoveTraceBuilder {
/// Create a new `MoveTraceBuilder` with no additional tracing.
pub fn new() -> Self {
Expand Down Expand Up @@ -314,6 +320,12 @@ impl MoveTraceBuilder {
}
}

impl Default for MoveTraceBuilder {
fn default() -> Self {
Self::new()
}
}

impl Display for TraceValue {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,12 @@ impl TraceState {
}
}

impl Default for TraceState {
Copy link
Contributor

Choose a reason for hiding this comment

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

fn default() -> Self {
Self::new()
}
}

impl Tracer for TraceState {
fn notify(&mut self, event: &TraceEvent, mut write: Writer<'_>) {
self.apply_event(event);
Expand Down
1 change: 1 addition & 0 deletions sui-execution/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ sui-types.workspace = true

move-binary-format.workspace = true
move-bytecode-verifier-meter.workspace = true
move-trace-format.workspace = true
move-vm-config.workspace = true

sui-adapter-latest = { path = "latest/sui-adapter" }
Expand Down
1 change: 1 addition & 0 deletions sui-execution/latest/sui-adapter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ move-binary-format.workspace = true
move-bytecode-utils.workspace = true
move-bytecode-verifier-meter.workspace = true
move-core-types.workspace = true
move-trace-format.workspace = true
move-vm-config.workspace = true
move-vm-types.workspace = true
mysten-metrics.workspace = true
Expand Down
14 changes: 14 additions & 0 deletions sui-execution/latest/sui-adapter/src/execution_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod checked {

use crate::execution_mode::{self, ExecutionMode};
use move_binary_format::CompiledModule;
use move_trace_format::format::MoveTraceBuilder;
use move_vm_runtime::move_vm::MoveVM;
use std::{collections::HashSet, sync::Arc};
use sui_types::balance::{
Expand Down Expand Up @@ -92,6 +93,7 @@ mod checked {
metrics: Arc<LimitsMetrics>,
enable_expensive_checks: bool,
certificate_deny_set: &HashSet<TransactionDigest>,
trace_builder_opt: Option<&mut MoveTraceBuilder>,
) -> (
InnerTemporaryStore,
SuiGasStatus,
Expand Down Expand Up @@ -145,6 +147,7 @@ mod checked {
deny_cert,
contains_deleted_input,
cancelled_objects,
trace_builder_opt,
);

let status = if let Err(error) = &execution_result {
Expand Down Expand Up @@ -262,6 +265,7 @@ mod checked {
tx_context,
&mut gas_charger,
pt,
None,
)
.map_err(|(e, _)| e)?;
temporary_store.update_object_version_and_prev_tx();
Expand All @@ -281,6 +285,7 @@ mod checked {
deny_cert: bool,
contains_deleted_input: bool,
cancelled_objects: Option<(Vec<ObjectID>, SequenceNumber)>,
trace_builder_opt: Option<&mut MoveTraceBuilder>,
) -> (
GasCostSummary,
Result<Mode::ExecutionResults, ExecutionError>,
Expand Down Expand Up @@ -340,6 +345,7 @@ mod checked {
gas_charger,
protocol_config,
metrics.clone(),
trace_builder_opt,
)
};

Expand Down Expand Up @@ -566,6 +572,7 @@ mod checked {
gas_charger: &mut GasCharger,
protocol_config: &ProtocolConfig,
metrics: Arc<LimitsMetrics>,
trace_builder_opt: Option<&mut MoveTraceBuilder>,
) -> ResultWithTimings<Mode::ExecutionResults, ExecutionError> {
let result = match transaction_kind {
TransactionKind::ChangeEpoch(change_epoch) => {
Expand Down Expand Up @@ -651,6 +658,7 @@ mod checked {
tx_ctx,
gas_charger,
pt,
trace_builder_opt,
)
}
TransactionKind::EndOfEpochTransaction(txns) => {
Expand Down Expand Up @@ -903,6 +911,7 @@ mod checked {
tx_ctx,
gas_charger,
advance_epoch_pt,
None,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

);

#[cfg(msim)]
Expand Down Expand Up @@ -932,6 +941,7 @@ mod checked {
tx_ctx,
gas_charger,
advance_epoch_safe_mode_pt,
None,
)
.map_err(|(e, _)| e)
.expect("Advance epoch with safe mode must succeed");
Expand Down Expand Up @@ -1002,6 +1012,7 @@ mod checked {
tx_ctx,
gas_charger,
publish_pt,
None,
)
.map_err(|(e, _)| e)
.expect("System Package Publish must succeed");
Expand Down Expand Up @@ -1071,6 +1082,7 @@ mod checked {
tx_ctx,
gas_charger,
pt,
None,
)
.map_err(|(e, _)| e)?;
Ok(())
Expand Down Expand Up @@ -1214,6 +1226,7 @@ mod checked {
tx_ctx,
gas_charger,
pt,
None,
)
.map_err(|(e, _)| e)?;
Ok(())
Expand Down Expand Up @@ -1282,6 +1295,7 @@ mod checked {
tx_ctx,
gas_charger,
pt,
None,
)
.map_err(|(e, _)| e)?;
Ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ mod checked {
identifier::IdentStr,
language_storage::{ModuleId, StructTag, TypeTag},
};
use move_trace_format::format::MoveTraceBuilder;
use move_vm_runtime::native_extensions::NativeContextExtensions;
use move_vm_runtime::{
move_vm::MoveVM,
Expand Down Expand Up @@ -963,6 +964,7 @@ mod checked {
function_name: &IdentStr,
ty_args: Vec<Type>,
args: Vec<impl Borrow<[u8]>>,
tracer: Option<&mut MoveTraceBuilder>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the tracer be part of the ExecutionContext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

) -> VMResult<SerializedReturnValues> {
let gas_status = self.gas_charger.move_gas_status_mut();
let mut data_store = SuiDataStore::new(&self.linkage_view, &self.new_packages);
Expand All @@ -974,7 +976,7 @@ mod checked {
&mut data_store,
gas_status,
&mut self.native_extensions,
None,
tracer,
)
}

Expand Down
Loading
Loading