From 296bd6e2d5d163a0ff5198e2fb3e0fb6ec82c2bc Mon Sep 17 00:00:00 2001 From: Ashok Menon Date: Fri, 5 Apr 2024 00:59:26 +0100 Subject: [PATCH] [Verifier/Meter] Transfer module meter to package meter (#16945) ## 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 --- .../src/metered_verifier.rs | 42 ++--------- crates/sui-open-rpc/spec/openrpc.json | 1 + crates/sui-protocol-config/src/lib.rs | 23 +++--- ...ocol_config__test__Mainnet_version_43.snap | 1 + ...ocol_config__test__Testnet_version_43.snap | 1 + ...sui_protocol_config__test__version_43.snap | 1 + crates/sui-types/src/metrics.rs | 4 ++ crates/sui/src/client_commands.rs | 1 + .../move-bytecode-verifier-meter/src/bound.rs | 24 ++++--- .../crates/move-vm-config/src/verifier.rs | 3 + .../latest/sui-adapter/src/adapter.rs | 71 +++++++++++-------- .../latest/sui-verifier/src/meter.rs | 4 +- 12 files changed, 92 insertions(+), 84 deletions(-) diff --git a/crates/sui-framework-tests/src/metered_verifier.rs b/crates/sui-framework-tests/src/metered_verifier.rs index f313215a92160..a317bcd1ecde5 100644 --- a/crates/sui-framework-tests/src/metered_verifier.rs +++ b/crates/sui-framework-tests/src/metered_verifier.rs @@ -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(), @@ -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(), @@ -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, @@ -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); } } @@ -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(), diff --git a/crates/sui-open-rpc/spec/openrpc.json b/crates/sui-open-rpc/spec/openrpc.json index 02b3187ce7c2b..040133f378554 100644 --- a/crates/sui-open-rpc/spec/openrpc.json +++ b/crates/sui-open-rpc/spec/openrpc.json @@ -1761,6 +1761,7 @@ "max_meter_ticks_per_module": { "u64": "6000000" }, + "max_meter_ticks_per_package": null, "max_modules_in_publish": { "u32": "128" }, diff --git a/crates/sui-protocol-config/src/lib.rs b/crates/sui-protocol-config/src/lib.rs index f9efa5dab7cf5..88488705e2c16 100644 --- a/crates/sui-protocol-config/src/lib.rs +++ b/crates/sui-protocol-config/src/lib.rs @@ -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); @@ -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, - /// 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, + /// Maximum number of meter `ticks` spent verifying a Move package. Enforced by the bytecode verifier at signing. + max_meter_ticks_per_package: Option, + // === 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 @@ -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), @@ -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: // @@ -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, + ), } } diff --git a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_43.snap b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_43.snap index 8045e78d363c3..5490e5d361d30 100644 --- a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_43.snap +++ b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_43.snap @@ -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 diff --git a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_43.snap b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_43.snap index 485bf0b286ca7..daee7175154b4 100644 --- a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_43.snap +++ b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_43.snap @@ -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 diff --git a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_43.snap b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_43.snap index 7c41c69d7cbf5..c4ae3f5211694 100644 --- a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_43.snap +++ b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_43.snap @@ -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 diff --git a/crates/sui-types/src/metrics.rs b/crates/sui-types/src/metrics.rs index 22b3315bd47ee..4b1d87bff1ed2 100644 --- a/crates/sui-types/src/metrics.rs +++ b/crates/sui-types/src/metrics.rs @@ -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"; diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs index a235b4ac3b065..08fdbe5ae4d94 100644 --- a/crates/sui/src/client_commands.rs +++ b/crates/sui/src/client_commands.rs @@ -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)?; diff --git a/external-crates/move/crates/move-bytecode-verifier-meter/src/bound.rs b/external-crates/move/crates/move-bytecode-verifier-meter/src/bound.rs index 5a92c950e5a44..1f4b1d40ff936 100644 --- a/external-crates/move/crates/move-bytecode-verifier-meter/src/bound.rs +++ b/external-crates/move/crates/move-bytecode-verifier-meter/src/bound.rs @@ -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, } @@ -57,6 +58,11 @@ impl Bounds { impl BoundMeter { pub fn new(config: MeterConfig) -> Self { Self { + pkg_bounds: Bounds { + name: "".to_string(), + units: 0, + max: config.max_per_pkg_meter_units, + }, mod_bounds: Bounds { name: "".to_string(), units: 0, @@ -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."), } } diff --git a/external-crates/move/crates/move-vm-config/src/verifier.rs b/external-crates/move/crates/move-vm-config/src/verifier.rs index c5a66a9bc3343..a8e5565583110 100644 --- a/external-crates/move/crates/move-vm-config/src/verifier.rs +++ b/external-crates/move/crates/move-vm-config/src/verifier.rs @@ -30,6 +30,7 @@ pub struct VerifierConfig { pub struct MeterConfig { pub max_per_fun_meter_units: Option, pub max_per_mod_meter_units: Option, + pub max_per_pkg_meter_units: Option, } impl Default for VerifierConfig { @@ -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), } } } @@ -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, } } } diff --git a/sui-execution/latest/sui-adapter/src/adapter.rs b/sui-execution/latest/sui-adapter/src/adapter.rs index 8f154573537ca..1bc3e3bcb5ea5 100644 --- a/sui-execution/latest/sui-adapter/src/adapter.rs +++ b/sui-execution/latest/sui-adapter/src/adapter.rs @@ -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}, @@ -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 @@ -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 @@ -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(()) } } diff --git a/sui-execution/latest/sui-verifier/src/meter.rs b/sui-execution/latest/sui-verifier/src/meter.rs index 0bc3df45cad0c..e644631bd6cac 100644 --- a/sui-execution/latest/sui-verifier/src/meter.rs +++ b/sui-execution/latest/sui-verifier/src/meter.rs @@ -44,12 +44,10 @@ impl SuiVerifierMeter { ticks: 0, max_ticks: None, }, - - // Not used for now to keep backward compat package_bounds: SuiVerifierMeterBounds { name: "".to_string(), ticks: 0, - max_ticks: None, + max_ticks: config.max_per_pkg_meter_units, }, module_bounds: SuiVerifierMeterBounds { name: "".to_string(),