Skip to content

[RPC] VM error visibility indexing #26595

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.

Sign up for GitHub

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

Open
jordanjennings-mysten wants to merge 15 commits into
base: main
Choose a base branch
from
Open

[RPC] VM error visibility indexing #26595

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
ce7ef8a
[jsonrpc] vm error visibility indexing
jordanjennings-mysten May 5, 2026
c32b90f
fix
jordanjennings-mysten May 18, 2026
c55ac5f
replay fix
jordanjennings-mysten May 18, 2026
ab0c0e3
test validator doesnt store, no rows created on empty metadata
jordanjennings-mysten May 18, 2026
075964c
Track execution error metadata sidecar
jordanjennings-mysten May 19, 2026
c296ef6
Down-convert replay execution status errors
jordanjennings-mysten May 19, 2026
c165a1c
nit
jordanjennings-mysten May 19, 2026
06842bb
do not leak ExecutionErrorContext, proto from sui-apis, flexible table
jordanjennings-mysten May 27, 2026
05c92a5
move ExecutionErrorMetadata in rpc proto conversions
jordanjennings-mysten May 28, 2026
56d1e3d
revert to execution error source()
jordanjennings-mysten May 28, 2026
4306021
document bytes in authority store, remove less useful test
jordanjennings-mysten May 28, 2026
15ef267
move ExecutionErrorMetadata in rpc proto to the bottom of error section
jordanjennings-mysten May 28, 2026
0ed0ba6
drop context from older execution versions
jordanjennings-mysten May 28, 2026
ddcdc1c
move context to sui-execution
jordanjennings-mysten May 28, 2026
d8d0590
dont touch cargo.lock for now
jordanjennings-mysten May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 82 additions & 36 deletions crates/sui-core/src/authority.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ use sui_types::effects::{
InputConsensusObject, SignedTransactionEffects, TransactionEffects, TransactionEffectsAPI,
TransactionEvents, VerifiedSignedTransactionEffects,
};
use sui_types::error::{ExecutionError, SuiErrorKind, UserInputError};
use sui_types::error::{ExecutionError, ExecutionErrorContext, SuiErrorKind, UserInputError};
use sui_types::event::EventID;
use sui_types::executable_transaction::VerifiedExecutableTransaction;
use sui_types::execution_status::ExecutionErrorKind;
Expand Down Expand Up @@ -1461,7 +1461,7 @@ impl AuthorityState {
certificate: &VerifiedExecutableTransaction,
mut execution_env: ExecutionEnv,
epoch_store: &Arc<AuthorityPerEpochStore>,
) -> ExecutionOutput<(TransactionEffects, Option<ExecutionError>)> {
) -> ExecutionOutput<(TransactionEffects, Option<ExecutionErrorContext>)> {
let _scope = monitored_scope("Execution::try_execute_immediately");
let _metrics_guard = self.metrics.internal_execution_latency.start_timer();

Expand Down Expand Up @@ -1640,7 +1640,10 @@ impl AuthorityState {
&self,
certificate: &VerifiedCertificate,
execution_env: ExecutionEnv,
) -> (VerifiedSignedTransactionEffects, Option<ExecutionError>) {
) -> (
VerifiedSignedTransactionEffects,
Option<ExecutionErrorContext>,
) {
let epoch_store = self.epoch_store_for_testing();
let (effects, execution_error_opt) = self
.try_execute_immediately(
Expand All @@ -1658,7 +1661,10 @@ impl AuthorityState {
&self,
executable: &VerifiedExecutableTransaction,
execution_env: ExecutionEnv,
) -> (VerifiedSignedTransactionEffects, Option<ExecutionError>) {
) -> (
VerifiedSignedTransactionEffects,
Option<ExecutionErrorContext>,
) {
let epoch_store = self.epoch_store_for_testing();
let (effects, execution_error_opt) = self
.try_execute_immediately(executable, execution_env, &epoch_store)
Expand Down Expand Up @@ -1734,7 +1740,7 @@ impl AuthorityState {
) -> ExecutionOutput<(
TransactionOutputs,
Vec<ExecutionTiming>,
Option<ExecutionError>,
Option<ExecutionErrorContext>,
)> {
let _scope = monitored_scope("Execution::process_certificate");
let tx_digest = *certificate.digest();
Expand Down Expand Up @@ -1905,40 +1911,69 @@ impl AuthorityState {
rewritten_inputs: Option<Vec<bool>>,
signer: SuiAddress,
tx_digest: TransactionDigest,
is_fullnode: bool,
) -> (
InnerTemporaryStore,
SuiGasStatus,
TransactionEffects,
Vec<ExecutionTiming>,
Result<(), ExecutionError>,
Result<(), ExecutionErrorContext>,
) {
let (inner_temp_store, gas_status, effects, timings, execution_error) = executor
// TODO only run this function on FullNodes, use `execute_transaction_to_effects` on validators.
Comment thread
jordanjennings-mysten marked this conversation as resolved.
.execute_transaction_to_effects_and_execution_error(
store,
protocol_config,
self.metrics.execution_metrics.clone(),
enable_expensive_checks,
execution_params,
epoch_id,
epoch_timestamp_ms,
input_objects,
gas_data,
if is_fullnode {
Comment thread
jordanjennings-mysten marked this conversation as resolved.
let (inner_temp_store, gas_status, effects, timings, execution_error) = executor
.execute_transaction_to_effects_and_execution_error(
store,
protocol_config,
self.metrics.execution_metrics.clone(),
enable_expensive_checks,
execution_params,
epoch_id,
epoch_timestamp_ms,
input_objects,
gas_data,
gas_status,
kind,
rewritten_inputs,
signer,
tx_digest,
&mut None,
);

(
inner_temp_store,
gas_status,
kind,
rewritten_inputs,
signer,
tx_digest,
&mut None,
);
effects,
timings,
execution_error,
)
} else {
let (inner_temp_store, gas_status, effects, timings, execution_failure) = executor
.execute_transaction_to_effects(
store,
protocol_config,
self.metrics.execution_metrics.clone(),
enable_expensive_checks,
execution_params,
epoch_id,
epoch_timestamp_ms,
input_objects,
gas_data,
gas_status,
kind,
rewritten_inputs,
signer,
tx_digest,
&mut None,
);

(
inner_temp_store,
gas_status,
effects,
timings,
execution_error,
)
(
inner_temp_store,
gas_status,
effects,
timings,
execution_failure.map_err(ExecutionErrorContext::from),

@amnn amnn May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be worthwhile to split the extra metadata into its own distinct type? I.e. have Authority::execute_transaction_to_effects return ExecutionError and then a separate, optional ExecutionErrorContext or ExecutionMetadata (whatever you want to call the extra information only)?

The benefit (for me) of using separate types, is that I would be able to clearly see which part of the richer data is dropped in the validator path, because it would be statically None here.

This also fits the onward data flow better because the execution error would need to be indexed with the rest of the base transaction effect data, while the metadata goes into its own table.

@bmwill bmwill May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would also recommend we try to align on the shape of the type using a protobuf message and storing the protobuf message itself in the DB so that we can iterate on the shape if need be.

@amnn amnn May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bmwill, is it possible to do that today with TideHunter/TypedStore? I was personally less worried about the evolvability because the metadata structure is already quite generic, but agree that if it's possible, then using a more generic protobuf value would be nicer for future evolvability.

@jordanjennings-mysten jordanjennings-mysten May 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

put prost and serialize and it seemed to work out, I expect we will add some protos somewhere at some point but for now just used the prost derive.

)
}
}

/// execute_certificate validates the transaction input, and executes the certificate,
Expand All @@ -1961,7 +1996,7 @@ impl AuthorityState {
) -> ExecutionOutput<(
TransactionOutputs,
Vec<ExecutionTiming>,
Option<ExecutionError>,
Option<ExecutionErrorContext>,
)> {
let _scope = monitored_scope("Execution::prepare_certificate");
let _metrics_guard = self.metrics.prepare_certificate_latency.start_timer();
Expand Down Expand Up @@ -2025,7 +2060,7 @@ impl AuthorityState {
let tracking_store = TrackingBackingStore::new(self.get_backing_store().as_ref());

#[allow(unused_mut)]
let (inner_temp_store, _, mut effects, timings, execution_error_opt) = self
let (inner_temp_store, _, mut effects, timings, execution_error_result) = self
Comment thread
jordanjennings-mysten marked this conversation as resolved.
Outdated
.execute_transaction_to_effects(
&**epoch_store.executor(),
&tracking_store,
Expand All @@ -2048,8 +2083,11 @@ impl AuthorityState {
rewritten_inputs,
signer,
tx_digest,
self.is_fullnode(epoch_store),
);

let execution_error_opt = execution_error_result.err();

let object_funds_checker = self.object_funds_checker.load();
if let Some(object_funds_checker) = object_funds_checker.as_ref()
&& !object_funds_checker.should_commit_object_funds_withdraws(
Expand Down Expand Up @@ -2159,6 +2197,10 @@ impl AuthorityState {
&tracking_store.into_read_objects(),
);

let execution_error_metadata = execution_error_opt

@stefan-mysten stefan-mysten May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the possible size of this error metadata? If I recall correctly, this will be added onto fullnodes, I assume in a table somewhere. If it's very large, can it cause any disruption/issues during insertion/retrieval?

@jordanjennings-mysten jordanjennings-mysten May 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

my napkin math says about 1 gb of storage per week with 300 bytes per error, 2% error rate on ~20 million txs a day, depending on when the fullnode prunes it could be a bit smaller, maybe more in the 0.33gb range. based on a 5 hour sample window a few weeks back.

@stefan-mysten stefan-mysten May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for sharing more concrete numbers. I was wondering if there's any limits in the DB of how much data can be inserted in a field in a column?

@jordanjennings-mysten jordanjennings-mysten May 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think they are quite large but you would hit performance problems at some point since its rocks db.

.as_ref()
.and_then(ExecutionErrorContext::metadata_with_source);

// index certificate
let _ = self
.post_process_one_tx(certificate, &effects, &inner_temp_store, epoch_store)
Expand All @@ -2174,6 +2216,7 @@ impl AuthorityState {
effects,
inner_temp_store,
unchanged_loaded_runtime_objects,
execution_error_metadata,
);

let elapsed = prepare_certificate_start_time.elapsed().as_micros() as f64;
Expand All @@ -2187,7 +2230,7 @@ impl AuthorityState {
);
}

ExecutionOutput::Success((transaction_outputs, timings, execution_error_opt.err()))
ExecutionOutput::Success((transaction_outputs, timings, execution_error_opt))
}

pub fn prepare_certificate_for_benchmark(
Expand All @@ -2209,7 +2252,10 @@ impl AuthorityState {
epoch_store,
)
.unwrap();
Ok((transaction_outputs, execution_error_opt))
Ok((
transaction_outputs,
execution_error_opt.map(ExecutionError::from),
))
}

#[instrument(skip_all)]
Expand Down Expand Up @@ -2339,7 +2385,7 @@ impl AuthorityState {
let execution_error_source = execution_result
.as_ref()
.err()
.and_then(|e| e.source().as_ref().map(|e| e.to_string()));
.and_then(|e| std::error::Error::source(e).map(|e| e.to_string()));
Comment thread
jordanjennings-mysten marked this conversation as resolved.
Outdated

let response = DryRunTransactionBlockResponse {
suggested_gas_price,
Expand Down
19 changes: 18 additions & 1 deletion crates/sui-core/src/authority/authority_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use move_core_types::resolver::{ModuleResolver, SerializedPackage};
use serde::{Deserialize, Serialize};
use sui_config::node::AuthorityStorePruningConfig;
use sui_macros::fail_point_arg;
use sui_types::error::{SuiErrorKind, UserInputError};
use sui_types::error::{ExecutionErrorMetadata, SuiErrorKind, UserInputError};
use sui_types::execution::TypeLayoutStore;
use sui_types::global_state_hash::GlobalStateHash;
use sui_types::message_envelope::Message;
Expand Down Expand Up @@ -344,6 +344,13 @@ impl AuthorityStore {
.get(digest)
}

pub fn get_execution_error_metadata(
Comment thread
jordanjennings-mysten marked this conversation as resolved.
&self,
digest: &TransactionDigest,
) -> Result<Option<ExecutionErrorMetadata>, TypedStoreError> {
self.perpetual_tables.execution_error_metadata.get(digest)
}

pub fn multi_get_effects<'a>(
&self,
effects_digests: impl Iterator<Item = &'a TransactionEffectsDigest>,
Expand Down Expand Up @@ -791,6 +798,7 @@ impl AuthorityStore {
written,
events,
unchanged_loaded_runtime_objects,
execution_error_metadata,
locks_to_delete,
new_locks_to_init,
..
Expand Down Expand Up @@ -863,6 +871,15 @@ impl AuthorityStore {
)?;
}

if let Some(metadata) = execution_error_metadata
&& !metadata.is_empty()
{
write_batch.insert_batch(
&self.perpetual_tables.execution_error_metadata,
[(transaction_digest, metadata)],
)?;
}

self.initialize_live_object_markers_impl(write_batch, new_locks_to_init, false)?;

// Note: deletes locks for received objects as well (but not for objects that were in
Expand Down
2 changes: 2 additions & 0 deletions crates/sui-core/src/authority/authority_store_pruner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ impl AuthorityStorePruner {
&perpetual_db.unchanged_loaded_runtime_objects,
transactions.iter(),
)?;
perpetual_batch
.delete_batch(&perpetual_db.execution_error_metadata, transactions.iter())?;
perpetual_batch.delete_batch(&perpetual_db.effects, effect_digests)?;

let mut checkpoints_batch = checkpoint_db.tables.certified_checkpoints.batch();
Expand Down
14 changes: 14 additions & 0 deletions crates/sui-core/src/authority/authority_store_tables.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use std::path::Path;
use std::sync::atomic::AtomicU64;
use sui_types::base_types::SequenceNumber;
use sui_types::effects::{TransactionEffects, TransactionEvents};
use sui_types::error::ExecutionErrorMetadata;
use sui_types::global_state_hash::GlobalStateHash;
use sui_types::storage::MarkerValue;
use typed_store::metrics::SamplingInterval;
Expand Down Expand Up @@ -100,6 +101,9 @@ pub struct AuthorityPerpetualTables {
// Loaded (and unchanged) runtime object references.
pub(crate) unchanged_loaded_runtime_objects: DBMap<TransactionDigest, Vec<ObjectKey>>,

// Local execution error metadata, keyed by the digest of the transaction that produced it.
Comment thread
jordanjennings-mysten marked this conversation as resolved.
pub(crate) execution_error_metadata: DBMap<TransactionDigest, ExecutionErrorMetadata>,

/// DEPRECATED in favor of the table of the same name in authority_per_epoch_store.
/// Please do not add new accessors/callsites.
/// When transaction is executed via checkpoint executor, we store association here
Expand Down Expand Up @@ -331,6 +335,16 @@ impl AuthorityPerpetualTables {
digest_prefix.clone(),
),
),
(
"execution_error_metadata".to_string(),
ThConfig::new_with_rm_prefix(
32,
mutexes,
uniform_key,
KeySpaceConfig::default().with_relocation_filter(|_, _| Decision::Remove),
digest_prefix.clone(),
),
),
(
"executed_transactions_to_checkpoint".to_string(),
ThConfig::new_with_rm_prefix(
Expand Down
8 changes: 6 additions & 2 deletions crates/sui-core/src/authority/authority_test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ pub async fn execute_from_consensus(
.try_execute_executable_for_test(&executable, env)
.await;
let effects = result.inner().data().clone();
(effects, execution_error_opt)
(effects, execution_error_opt.map(ExecutionError::from))
}

/// This is the primary test helper for executing transactions end-to-end.
Expand Down Expand Up @@ -221,7 +221,11 @@ pub async fn submit_and_execute_with_error(
execution_error_opt = fullnode_execution_error_opt;
}

Ok((executable, result.into_inner(), execution_error_opt))
Ok((
executable,
result.into_inner(),
execution_error_opt.map(ExecutionError::from),
))
}

/// Enqueues multiple transactions for execution after they've been through consensus.
Expand Down
7 changes: 7 additions & 0 deletions crates/sui-core/src/checkpoints/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4190,6 +4190,13 @@ mod tests {
unimplemented!()
}

fn get_execution_error_metadata(
&self,
_digest: &TransactionDigest,
) -> Option<sui_types::error::ExecutionErrorMetadata> {
unimplemented!()
}

fn transaction_executed_in_last_epoch(&self, _: &TransactionDigest, _: EpochId) -> bool {
unimplemented!()
}
Expand Down
7 changes: 6 additions & 1 deletion crates/sui-core/src/execution_cache.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use sui_protocol_config::ProtocolVersion;
use sui_types::base_types::{FullObjectID, VerifiedExecutionData};
use sui_types::digests::{TransactionDigest, TransactionEffectsDigest};
use sui_types::effects::{TransactionEffects, TransactionEvents};
use sui_types::error::{SuiError, SuiErrorKind, SuiResult, UserInputError};
use sui_types::error::{ExecutionErrorMetadata, SuiError, SuiErrorKind, SuiResult, UserInputError};
use sui_types::executable_transaction::VerifiedExecutableTransaction;
use sui_types::messages_checkpoint::CheckpointSequenceNumber;
use sui_types::object::Object;
Expand Down Expand Up @@ -534,6 +534,11 @@ pub trait TransactionCacheRead: Send + Sync {
digest: &TransactionDigest,
) -> Option<Vec<ObjectKey>>;

fn get_execution_error_metadata(
Comment thread
jordanjennings-mysten marked this conversation as resolved.
&self,
digest: &TransactionDigest,
) -> Option<ExecutionErrorMetadata>;

fn take_accumulator_events(&self, digest: &TransactionDigest) -> Option<Vec<AccumulatorEvent>>;

fn notify_read_executed_effects_digests<'a>(
Expand Down
Loading
Loading