Skip to content

Commit

Permalink
[Verifier/Meter] Transfer module meter to package meter (#16945)
Browse files Browse the repository at this point in the history
## Description

The motivation for this change is to properly track the modules being
visited in the `Meter`, so that in our command line tool, we can provide
an output that correctly tracks how many meter ticks were used to verify
each function and module.

Prior to this change, the metered verifier pretended like there was only
"one module" that all the functions being verified were in. This change
introduces the level of packages that ticks from modules are transferred
to.

We also add an explicit protocol config parameter to set a max tick
limit per package (whose value is the same as the function and module
level limits currently). This was not strictly necessary but was done to
avoid confusion (caused by treating the module level limit as the
package level limit).

This should be a behaviour preserving change, but in some cases we may
do more work verifying transactions because of how the ticks for each
module are now gathered in isolation before being transferred to the
package scope. In any case, a behaviour change would be okay, because
metering (and related errors) is only used during signing, and not
execution.

## Test Plan

```
sui$ cargo simtest
deepbook$ cargo run --bin sui -p sui -- client verify-bytecode-meter
```

## Stack

- #16903 
- #16941
  • Loading branch information
amnn authored Apr 4, 2024
1 parent f8809c0 commit 296bd6e
Show file tree
Hide file tree
Showing 12 changed files with 92 additions and 84 deletions.
42 changes: 5 additions & 37 deletions crates/sui-framework-tests/src/metered_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,7 @@ fn test_metered_move_bytecode_verifier() {
bytecode_verifier_metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::MOVE_VERIFIER_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.get(),
);

assert_eq!(
0,
bytecode_verifier_metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::SUI_VERIFIER_TAG,
BytecodeVerifierMetrics::OVERALL_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.get(),
Expand Down Expand Up @@ -177,18 +166,7 @@ fn test_metered_move_bytecode_verifier() {
bytecode_verifier_metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::MOVE_VERIFIER_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.get(),
);
// Sui verifier did not fail
assert_eq!(
0,
bytecode_verifier_metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::SUI_VERIFIER_TAG,
BytecodeVerifierMetrics::OVERALL_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.get(),
Expand Down Expand Up @@ -227,7 +205,7 @@ fn test_metered_move_bytecode_verifier() {
// Check if the same meter is indeed used multiple invocations of the verifier
let mut meter = SuiVerifierMeter::new(meter_config);
for modules in &packages {
let prev_meter = meter.get_usage(Scope::Module) + meter.get_usage(Scope::Function);
let prev_meter = meter.get_usage(Scope::Package);

run_metered_move_bytecode_verifier(
modules,
Expand All @@ -237,7 +215,7 @@ fn test_metered_move_bytecode_verifier() {
)
.expect("Verification should not timeout");

let curr_meter = meter.get_usage(Scope::Module) + meter.get_usage(Scope::Function);
let curr_meter = meter.get_usage(Scope::Package);
assert!(curr_meter > prev_meter);
}
}
Expand Down Expand Up @@ -275,17 +253,7 @@ fn test_meter_system_packages() {
bytecode_verifier_metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::MOVE_VERIFIER_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.get(),
);
assert_eq!(
0,
bytecode_verifier_metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::SUI_VERIFIER_TAG,
BytecodeVerifierMetrics::OVERALL_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.get(),
Expand Down
1 change: 1 addition & 0 deletions crates/sui-open-rpc/spec/openrpc.json
Original file line number Diff line number Diff line change
Expand Up @@ -1761,6 +1761,7 @@
"max_meter_ticks_per_module": {
"u64": "6000000"
},
"max_meter_ticks_per_package": null,
"max_modules_in_publish": {
"u32": "128"
},
Expand Down
23 changes: 15 additions & 8 deletions crates/sui-protocol-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ const MAX_PROTOCOL_VERSION: u64 = 43;
// Version 41: Enable group operations native functions in testnet and mainnet (without msm).
// Version 42: Migrate sui framework and related code to Move 2024
// Version 43: Introduce the upper bound delta config for a zklogin signature's max epoch.
// Introduce an explicit parameter for the tick limit per package (previously this was
// represented by the parameter for the tick limit per module).

#[derive(Copy, Clone, Debug, Hash, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
pub struct ProtocolVersion(u64);

Expand Down Expand Up @@ -645,9 +648,12 @@ pub struct ProtocolConfig {
/// Maximum number of meter `ticks` spent verifying a Move function. Enforced by the bytecode verifier at signing.
max_verifier_meter_ticks_per_function: Option<u64>,

/// Maximum number of meter `ticks` spent verifying a Move function. Enforced by the bytecode verifier at signing.
/// Maximum number of meter `ticks` spent verifying a Move module. Enforced by the bytecode verifier at signing.
max_meter_ticks_per_module: Option<u64>,

/// Maximum number of meter `ticks` spent verifying a Move package. Enforced by the bytecode verifier at signing.
max_meter_ticks_per_package: Option<u64>,

// === Object runtime internal operation limits ====
// These affect dynamic fields
/// Maximum number of cached objects in the object runtime ObjectStore. Enforced by object runtime during execution
Expand Down Expand Up @@ -1398,17 +1404,11 @@ impl ProtocolConfig {
max_event_emit_size: Some(250 * 1024),
max_move_vector_len: Some(256 * 1024),

// TODO: Is this too low/high?
max_back_edges_per_function: Some(10_000),

// TODO: Is this too low/high?
max_back_edges_per_module: Some(10_000),

// TODO: Is this too low/high?
max_verifier_meter_ticks_per_function: Some(6_000_000),

// TODO: Is this too low/high?
max_meter_ticks_per_module: Some(6_000_000),
max_meter_ticks_per_package: None,

object_runtime_max_num_cached_objects: Some(1000),
object_runtime_max_num_cached_objects_system_tx: Some(1000 * 16),
Expand Down Expand Up @@ -2064,6 +2064,7 @@ impl ProtocolConfig {
42 => {}
43 => {
cfg.feature_flags.zklogin_max_epoch_upper_bound_delta = Some(30);
cfg.max_meter_ticks_per_package = Some(16_000_000);
}
// Use this template when making changes:
//
Expand Down Expand Up @@ -2120,6 +2121,12 @@ impl ProtocolConfig {
MeterConfig {
max_per_fun_meter_units: Some(self.max_verifier_meter_ticks_per_function() as u128),
max_per_mod_meter_units: Some(self.max_meter_ticks_per_module() as u128),
max_per_pkg_meter_units: Some(
// Until the per-package limit was introduced, the per-module limit played double
// duty.
self.max_meter_ticks_per_package_as_option()
.unwrap_or_else(|| self.max_meter_ticks_per_module()) as u128,
),
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ max_back_edges_per_function: 10000
max_back_edges_per_module: 10000
max_verifier_meter_ticks_per_function: 16000000
max_meter_ticks_per_module: 16000000
max_meter_ticks_per_package: 16000000
object_runtime_max_num_cached_objects: 1000
object_runtime_max_num_cached_objects_system_tx: 16000
object_runtime_max_num_store_entries: 1000
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ max_back_edges_per_function: 10000
max_back_edges_per_module: 10000
max_verifier_meter_ticks_per_function: 16000000
max_meter_ticks_per_module: 16000000
max_meter_ticks_per_package: 16000000
object_runtime_max_num_cached_objects: 1000
object_runtime_max_num_cached_objects_system_tx: 16000
object_runtime_max_num_store_entries: 1000
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ max_back_edges_per_function: 10000
max_back_edges_per_module: 10000
max_verifier_meter_ticks_per_function: 16000000
max_meter_ticks_per_module: 16000000
max_meter_ticks_per_package: 16000000
object_runtime_max_num_cached_objects: 1000
object_runtime_max_num_cached_objects_system_tx: 16000
object_runtime_max_num_store_entries: 1000
Expand Down
4 changes: 4 additions & 0 deletions crates/sui-types/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,12 @@ pub struct BytecodeVerifierMetrics {
}

impl BytecodeVerifierMetrics {
/// DEPRECATED in latest metered verifier, which only report overall success or timeout.
pub const MOVE_VERIFIER_TAG: &'static str = "move_verifier";

/// DEPRECATED in latest metered verifier, which only report overall success or timeout.
pub const SUI_VERIFIER_TAG: &'static str = "sui_verifier";

pub const OVERALL_TAG: &'static str = "overall";
pub const SUCCESS_TAG: &'static str = "success";
pub const TIMEOUT_TAG: &'static str = "failed";
Expand Down
1 change: 1 addition & 0 deletions crates/sui/src/client_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,7 @@ impl SuiClientCommands {
let mut meter = BoundMeter::new(MeterConfig {
max_per_fun_meter_units: Some(u128::MAX),
max_per_mod_meter_units: Some(u128::MAX),
max_per_pkg_meter_units: Some(u128::MAX),
});

verifier.meter_compiled_modules(&protocol_config, &modules, &mut meter)?;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use move_vm_config::verifier::MeterConfig;

/// Module and function level metering.
pub struct BoundMeter {
pkg_bounds: Bounds,
mod_bounds: Bounds,
fun_bounds: Bounds,
}
Expand Down Expand Up @@ -57,6 +58,11 @@ impl Bounds {
impl BoundMeter {
pub fn new(config: MeterConfig) -> Self {
Self {
pkg_bounds: Bounds {
name: "<unknown>".to_string(),
units: 0,
max: config.max_per_pkg_meter_units,
},
mod_bounds: Bounds {
name: "<unknown>".to_string(),
units: 0,
Expand All @@ -71,18 +77,20 @@ impl BoundMeter {
}

fn get_bounds_mut(&mut self, scope: Scope) -> &mut Bounds {
if scope == Scope::Module {
&mut self.mod_bounds
} else {
&mut self.fun_bounds
match scope {
Scope::Package => &mut self.pkg_bounds,
Scope::Module => &mut self.mod_bounds,
Scope::Function => &mut self.fun_bounds,
Scope::Transaction => panic!("transaction scope unsupported."),
}
}

fn get_bounds(&self, scope: Scope) -> &Bounds {
if scope == Scope::Module {
&self.mod_bounds
} else {
&self.fun_bounds
match scope {
Scope::Package => &self.pkg_bounds,
Scope::Module => &self.mod_bounds,
Scope::Function => &self.fun_bounds,
Scope::Transaction => panic!("transaction scope unsupported."),
}
}

Expand Down
3 changes: 3 additions & 0 deletions external-crates/move/crates/move-vm-config/src/verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub struct VerifierConfig {
pub struct MeterConfig {
pub max_per_fun_meter_units: Option<u128>,
pub max_per_mod_meter_units: Option<u128>,
pub max_per_pkg_meter_units: Option<u128>,
}

impl Default for VerifierConfig {
Expand Down Expand Up @@ -74,6 +75,7 @@ impl Default for MeterConfig {
Self {
max_per_fun_meter_units: Some(1000 * 8000),
max_per_mod_meter_units: Some(1000 * 8000),
max_per_pkg_meter_units: Some(1000 * 8000),
}
}
}
Expand All @@ -83,6 +85,7 @@ impl MeterConfig {
Self {
max_per_fun_meter_units: None,
max_per_mod_meter_units: None,
max_per_pkg_meter_units: None,
}
}
}
71 changes: 43 additions & 28 deletions sui-execution/latest/sui-adapter/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ mod checked {
use anyhow::Result;
use move_binary_format::{access::ModuleAccess, file_format::CompiledModule};
use move_bytecode_verifier::verify_module_with_config_metered;
use move_bytecode_verifier_meter::Meter;
use move_bytecode_verifier_meter::{Meter, Scope};
use move_core_types::account_address::AccountAddress;
use move_vm_config::{
runtime::{VMConfig, VMRuntimeLimitsConfig},
Expand Down Expand Up @@ -156,30 +156,7 @@ mod checked {
.verifier_runtime_per_module_success_latency
.start_timer();

if let Err(e) = verify_module_with_config_metered(verifier_config, module, meter) {
// Check that the status indicates metering timeout
if check_for_verifier_timeout(&e.major_status()) {
// Discard success timer, but record timeout/failure timer
metrics
.verifier_runtime_per_module_timeout_latency
.observe(per_module_meter_verifier_timer.stop_and_discard());
metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::MOVE_VERIFIER_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.inc();
return Err(SuiError::ModuleVerificationFailure {
error: format!("Verification timedout: {}", e),
});
};
} else if let Err(err) = sui_verify_module_metered_check_timeout_only(
module,
&BTreeMap::new(),
meter,
verifier_config,
) {
if let Err(e) = verify_module_timeout_only(module, verifier_config, meter) {
// We only checked that the failure was due to timeout
// Discard success timer, but record timeout/failure timer
metrics
Expand All @@ -188,12 +165,14 @@ mod checked {
metrics
.verifier_timeout_metrics
.with_label_values(&[
BytecodeVerifierMetrics::SUI_VERIFIER_TAG,
BytecodeVerifierMetrics::OVERALL_TAG,
BytecodeVerifierMetrics::TIMEOUT_TAG,
])
.inc();
return Err(err.into());
}

return Err(e);
};

// Save the success timer
per_module_meter_verifier_timer.stop_and_record();
metrics
Expand All @@ -204,6 +183,42 @@ mod checked {
])
.inc();
}

Ok(())
}

/// Run both the Move verifier and the Sui verifier, checking just for timeouts. Returns Ok(())
/// if the verifier completes within the module meter limit and the ticks are successfully
/// transfered to the package limit (regardless of whether verification succeeds or not).
fn verify_module_timeout_only(
module: &CompiledModule,
verifier_config: &VerifierConfig,
meter: &mut (impl Meter + ?Sized),
) -> Result<(), SuiError> {
meter.enter_scope(module.self_id().name().as_str(), Scope::Module);

if let Err(e) = verify_module_with_config_metered(verifier_config, module, meter) {
// Check that the status indicates metering timeout.
if check_for_verifier_timeout(&e.major_status()) {
return Err(SuiError::ModuleVerificationFailure {
error: format!("Verification timed out: {}", e),
});
}
} else if let Err(err) = sui_verify_module_metered_check_timeout_only(
module,
&BTreeMap::new(),
meter,
verifier_config,
) {
return Err(err.into());
}

if meter.transfer(Scope::Module, Scope::Package, 1.0).is_err() {
return Err(SuiError::ModuleVerificationFailure {
error: "Verification timed out".to_string(),
});
}

Ok(())
}
}
4 changes: 1 addition & 3 deletions sui-execution/latest/sui-verifier/src/meter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,10 @@ impl SuiVerifierMeter {
ticks: 0,
max_ticks: None,
},

// Not used for now to keep backward compat
package_bounds: SuiVerifierMeterBounds {
name: "<unknown>".to_string(),
ticks: 0,
max_ticks: None,
max_ticks: config.max_per_pkg_meter_units,
},
module_bounds: SuiVerifierMeterBounds {
name: "<unknown>".to_string(),
Expand Down

0 comments on commit 296bd6e

Please sign in to comment.