Skip to content

Commit

Permalink
[sui-execution] Verifier trait creates and accepts Meter
Browse files Browse the repository at this point in the history
Supersede `Verifier::meter_compiled_modules_with_overrides` by:

- Allowing `meter_compiled_modules` and `meter_compiled_bytes` to
  accept a custom meter.
- Extracting the generation of `VerifierConfig` out to the
  `sui-protocol-config` crate.
- Splitting up `VerifierConfig` into itself and `MeterConfig`.
- Adding a `Verifier::meter` function for creating the meter that is
  used during signing.
- Removed the meter as a field of each `Verifier`.

This means that we can now pass a different meter type when we don't
want to enforce metering, and no longer need to modify the config
in-place to achieve the same result.

To make this work, I needed to enable the use of a `&mut dyn Meter`
trait object to instantiate what was previously a `&mut impl Meter`
parameter everywhere. To do this, I created a forwarding trait
implementation of `Meter` for `&mut dyn Meter` and added an extra
relaxation of `+ ?Sized` to all the `&mut impl Meter` parameters to
allow the trait object to instantiate them.

This is in preparation for passing a custom meter into this new
parameter when instantiated by the CLI to capture all information
about scopes during metering.

Today we don't do that which means:

- We only report back information about the last module and function
  we verified.
- We can incorrectly report that everything is fine, (package would
  pass verification) even if there was some previous module or
  function that exceeded the stated limits.

Test Plan:

```
sui$ cargo build --bin sui
sui$ cargo simtest
sui$ env SUI_SKIP_SIMTESTS=1 cargo nextest run
external-crates$ ./tests.sh
deepbook$ cargo run --bin sui -p sui -- client verify-bytecode-meter
```
  • Loading branch information
amnn committed Apr 1, 2024
1 parent 448d9db commit b378ff2
Show file tree
Hide file tree
Showing 106 changed files with 583 additions and 857 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock

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

53 changes: 26 additions & 27 deletions crates/sui-framework-tests/src/metered_verifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use sui_types::{
error::{SuiError, SuiResult},
metrics::BytecodeVerifierMetrics,
};
use sui_verifier::{default_verifier_config, meter::SuiVerifierMeter};
use sui_verifier::meter::SuiVerifierMeter;

fn build(path: PathBuf) -> SuiResult<CompiledPackage> {
let mut config = sui_move_build::BuildConfig::new_for_testing();
Expand All @@ -29,18 +29,17 @@ fn test_metered_move_bytecode_verifier() {
let compiled_package = build(path).unwrap();
let compiled_modules: Vec<_> = compiled_package.get_modules().cloned().collect();

let mut metered_verifier_config = default_verifier_config(
&ProtocolConfig::get_for_max_version_UNSAFE(),
true, /* enable metering */
);
let protocol_config = ProtocolConfig::get_for_max_version_UNSAFE();
let mut verifier_config = protocol_config.verifier_config(/* for_signing */ true);
let mut meter_config = protocol_config.meter_config();
let registry = &Registry::new();
let bytecode_verifier_metrics = Arc::new(BytecodeVerifierMetrics::new(registry));
let mut meter = SuiVerifierMeter::new(&metered_verifier_config);
let mut meter = SuiVerifierMeter::new(meter_config.clone());
let timer_start = Instant::now();
// Default case should pass
let r = run_metered_move_bytecode_verifier(
&compiled_modules,
&metered_verifier_config,
&verifier_config,
&mut meter,
&bytecode_verifier_metrics,
);
Expand Down Expand Up @@ -121,16 +120,16 @@ fn test_metered_move_bytecode_verifier() {
);

// Use low limits. Should fail
metered_verifier_config.max_back_edges_per_function = Some(100);
metered_verifier_config.max_back_edges_per_module = Some(1_000);
metered_verifier_config.max_per_mod_meter_units = Some(10_000);
metered_verifier_config.max_per_fun_meter_units = Some(10_000);
verifier_config.max_back_edges_per_function = Some(100);
verifier_config.max_back_edges_per_module = Some(1_000);
meter_config.max_per_mod_meter_units = Some(10_000);
meter_config.max_per_fun_meter_units = Some(10_000);

let mut meter = SuiVerifierMeter::new(&metered_verifier_config);
let mut meter = SuiVerifierMeter::new(meter_config);
let timer_start = Instant::now();
let r = run_metered_move_bytecode_verifier(
&compiled_modules,
&metered_verifier_config,
&verifier_config,
&mut meter,
&bytecode_verifier_metrics,
);
Expand Down Expand Up @@ -221,18 +220,18 @@ fn test_metered_move_bytecode_verifier() {
let package = build(path).unwrap();
packages.push(package.get_dependency_sorted_modules(with_unpublished_deps));

let is_metered = true;
let protocol_config = ProtocolConfig::get_for_max_version_UNSAFE();
let metered_verifier_config = default_verifier_config(&protocol_config, is_metered);
let verifier_config = protocol_config.verifier_config(/* for_signing */ true);
let meter_config = protocol_config.meter_config();

// Check if the same meter is indeed used multiple invocations of the verifier
let mut meter = SuiVerifierMeter::new(&metered_verifier_config);
let mut meter = SuiVerifierMeter::new(meter_config);
for modules in &packages {
let prev_meter = meter.get_usage(Scope::Module) + meter.get_usage(Scope::Function);

run_metered_move_bytecode_verifier(
modules,
&metered_verifier_config,
&verifier_config,
&mut meter,
&bytecode_verifier_metrics,
)
Expand All @@ -248,16 +247,16 @@ fn test_metered_move_bytecode_verifier() {
fn test_meter_system_packages() {
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks));

let is_metered = true;
let metered_verifier_config =
default_verifier_config(&ProtocolConfig::get_for_max_version_UNSAFE(), is_metered);
let protocol_config = ProtocolConfig::get_for_max_version_UNSAFE();
let verifier_config = protocol_config.verifier_config(/* for_signing */ true);
let meter_config = protocol_config.meter_config();
let registry = &Registry::new();
let bytecode_verifier_metrics = Arc::new(BytecodeVerifierMetrics::new(registry));
let mut meter = SuiVerifierMeter::new(&metered_verifier_config);
let mut meter = SuiVerifierMeter::new(meter_config);
for system_package in BuiltInFramework::iter_system_packages() {
run_metered_move_bytecode_verifier(
&system_package.modules(),
&metered_verifier_config,
&verifier_config,
&mut meter,
&bytecode_verifier_metrics,
)
Expand Down Expand Up @@ -312,9 +311,9 @@ fn test_meter_system_packages() {
fn test_build_and_verify_programmability_examples() {
move_package::package_hooks::register_package_hooks(Box::new(SuiPackageHooks));

let is_metered = true;
let metered_verifier_config =
default_verifier_config(&ProtocolConfig::get_for_max_version_UNSAFE(), is_metered);
let protocol_config = ProtocolConfig::get_for_max_version_UNSAFE();
let verifier_config = protocol_config.verifier_config(/* for_signing */ true);
let meter_config = protocol_config.meter_config();
let registry = &Registry::new();
let bytecode_verifier_metrics = Arc::new(BytecodeVerifierMetrics::new(registry));
let examples =
Expand All @@ -335,10 +334,10 @@ fn test_build_and_verify_programmability_examples() {

let modules = build(path).unwrap().into_modules();

let mut meter = SuiVerifierMeter::new(&metered_verifier_config);
let mut meter = SuiVerifierMeter::new(meter_config.clone());
run_metered_move_bytecode_verifier(
&modules,
&metered_verifier_config,
&verifier_config,
&mut meter,
&bytecode_verifier_metrics,
)
Expand Down
14 changes: 5 additions & 9 deletions crates/sui-move-build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use sui_types::{
move_package::{FnInfo, FnInfoKey, FnInfoMap, MovePackage},
DEEPBOOK_ADDRESS, MOVE_STDLIB_ADDRESS, SUI_FRAMEWORK_ADDRESS, SUI_SYSTEM_ADDRESS,
};
use sui_verifier::{default_verifier_config, verifier as sui_bytecode_verifier};
use sui_verifier::verifier as sui_bytecode_verifier;

#[cfg(test)]
#[path = "unit_tests/build_tests.rs"]
Expand Down Expand Up @@ -253,20 +253,16 @@ pub fn build_from_resolution_graph(
};
let compiled_modules = package.root_modules_map();
if run_bytecode_verifier {
let verifier_config = ProtocolConfig::get_for_version(ProtocolVersion::MAX, Chain::Unknown)
.verifier_config(/* for_signing */ false);

for m in compiled_modules.iter_modules() {
move_bytecode_verifier::verify_module_unmetered(m).map_err(|err| {
SuiError::ModuleVerificationFailure {
error: err.to_string(),
}
})?;
sui_bytecode_verifier::sui_verify_module_unmetered(
m,
&fn_info,
&default_verifier_config(
&ProtocolConfig::get_for_version(ProtocolVersion::MAX, Chain::Unknown),
false,
),
)?;
sui_bytecode_verifier::sui_verify_module_unmetered(m, &fn_info, &verifier_config)?;
}
// TODO(https://github.com/MystenLabs/sui/issues/69): Run Move linker
}
Expand Down
1 change: 1 addition & 0 deletions crates/sui-protocol-config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ sui-protocol-config-macros.workspace = true
schemars.workspace = true
insta.workspace = true
clap.workspace = true
move-vm-config.workspace = true

[dev-dependencies]
insta.workspace = true
43 changes: 43 additions & 0 deletions crates/sui-protocol-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

use clap::*;
use move_vm_config::verifier::{MeterConfig, VerifierConfig};
use serde::{Deserialize, Serialize};
use serde_with::skip_serializing_none;
use std::cell::RefCell;
Expand Down Expand Up @@ -2069,6 +2070,48 @@ impl ProtocolConfig {
cfg
}

// Extract the bytecode verifier config from this protocol config. `for_signing` indicates
// whether this config is used for verification during signing or execution.
pub fn verifier_config(&self, for_signing: bool) -> VerifierConfig {
let (max_back_edges_per_function, max_back_edges_per_module) = if for_signing {
(
Some(self.max_back_edges_per_function() as usize),
Some(self.max_back_edges_per_module() as usize),
)
} else {
(None, None)
};

VerifierConfig {
max_loop_depth: Some(self.max_loop_depth() as usize),
max_generic_instantiation_length: Some(self.max_generic_instantiation_length() as usize),
max_function_parameters: Some(self.max_function_parameters() as usize),
max_basic_blocks: Some(self.max_basic_blocks() as usize),
max_value_stack_size: self.max_value_stack_size() as usize,
max_type_nodes: Some(self.max_type_nodes() as usize),
max_push_size: Some(self.max_push_size() as usize),
max_dependency_depth: Some(self.max_dependency_depth() as usize),
max_fields_in_struct: Some(self.max_fields_in_struct() as usize),
max_function_definitions: Some(self.max_function_definitions() as usize),
max_struct_definitions: Some(self.max_struct_definitions() as usize),
max_constant_vector_len: Some(self.max_move_vector_len()),
max_back_edges_per_function,
max_back_edges_per_module,
max_basic_blocks_in_script: None,
max_idenfitier_len: self.max_move_identifier_len_as_option(), // Before protocol version 9, there was no limit
allow_receiving_object_id: self.allow_receiving_object_id(),
reject_mutable_random_on_entry_functions: self
.reject_mutable_random_on_entry_functions(),
}
}

pub fn meter_config(&self) -> MeterConfig {
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),
}
}

/// Override one or more settings in the config, for testing.
/// This must be called at the beginning of the test, before get_for_(min|max)_version is
/// called, since those functions cache their return value.
Expand Down
12 changes: 7 additions & 5 deletions crates/sui-transaction-checks/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,10 +561,10 @@ mod checked {
return Ok(());
};

// We use a custom config with metering enabled
let is_metered = true;
// Use the same verifier and meter for all packages
let mut verifier = sui_execution::verifier(protocol_config, is_metered, metrics);
// Use the same verifier and meter for all packages, custom configured for signing.
let for_signing = true;
let mut verifier = sui_execution::verifier(protocol_config, for_signing, metrics);
let mut meter = verifier.meter(protocol_config.meter_config());

// Measure time for verifying all packages in the PTB
let shared_meter_verifier_timer = metrics
Expand All @@ -573,7 +573,9 @@ mod checked {

let verifier_status = pt
.non_system_packages_to_be_published()
.try_for_each(|module_bytes| verifier.meter_module_bytes(protocol_config, module_bytes))
.try_for_each(|module_bytes| {
verifier.meter_module_bytes(protocol_config, module_bytes, meter.as_mut())
})
.map_err(|e| UserInputError::PackageVerificationTimedout { err: e.to_string() });

match verifier_status {
Expand Down
2 changes: 2 additions & 0 deletions crates/sui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,12 @@ shell-words.workspace = true
tempfile.workspace = true
telemetry-subscribers.workspace = true

move-bytecode-verifier-meter.workspace = true
move-core-types.workspace = true
move-package.workspace = true
csv.workspace = true
move-vm-profiler.workspace = true
move-vm-config.workspace = true
move-command-line-common.workspace = true

[target.'cfg(not(target_env = "msvc"))'.dependencies]
Expand Down
62 changes: 38 additions & 24 deletions crates/sui/src/client_commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ use fastcrypto::{
};

use move_binary_format::CompiledModule;
use move_bytecode_verifier_meter::{bound::BoundMeter, Scope};
use move_core_types::language_storage::TypeTag;
use move_package::BuildConfig as MoveBuildConfig;
use move_vm_config::verifier::MeterConfig;
use prometheus::Registry;
use serde::Serialize;
use serde_json::{json, Value};
Expand All @@ -31,7 +33,6 @@ use sui_protocol_config::{Chain, ProtocolConfig, ProtocolVersion};
use sui_source_validation::{BytecodeSourceVerifier, SourceMode};

use shared_crypto::intent::Intent;
use sui_execution::verifier::VerifierOverrides;
use sui_json::SuiJsonValue;
use sui_json_rpc_types::{
Coin, DynamicFieldPage, SuiCoinMetadata, SuiData, SuiExecutionStatus, SuiObjectData,
Expand Down Expand Up @@ -1101,30 +1102,34 @@ impl SuiClientCommands {
}
};

let mut verifier =
sui_execution::verifier(&protocol_config, true, &bytecode_verifier_metrics);
let overrides = VerifierOverrides::new(None, None);
let for_signing = true;
let mut verifier = sui_execution::verifier(
&protocol_config,
for_signing,
&bytecode_verifier_metrics,
);

println!(
"Running bytecode verifier for {} module{}",
modules.len(),
if modules.len() != 1 { "s" } else { "" },
);

let verifier_values = verifier.meter_compiled_modules_with_overrides(
&modules,
&protocol_config,
&overrides,
)?;
// Need to pass some bounds because otherwise this meter doesn't accumulate
// anything.
let mut meter = BoundMeter::new(MeterConfig {
max_per_fun_meter_units: Some(u128::MAX),
max_per_mod_meter_units: Some(u128::MAX),
});

verifier.meter_compiled_modules(&protocol_config, &modules, &mut meter)?;

let meter_config = protocol_config.meter_config();
SuiClientCommandResult::VerifyBytecodeMeter {
max_module_ticks: verifier_values
.max_per_mod_meter_current
.unwrap_or(u128::MAX),
max_function_ticks: verifier_values
.max_per_fun_meter_current
.unwrap_or(u128::MAX),
used_function_ticks: verifier_values.fun_meter_units_result,
used_module_ticks: verifier_values.mod_meter_units_result,
max_module_ticks: meter_config.max_per_mod_meter_units,
max_function_ticks: meter_config.max_per_fun_meter_units,
used_function_ticks: meter.get_usage(Scope::Function),
used_module_ticks: meter.get_usage(Scope::Module),
}
}

Expand Down Expand Up @@ -2083,8 +2088,8 @@ impl Display for SuiClientCommandResult {
builder.set_header(vec!["", "Module", "Function"]);
builder.push_record(vec![
"Max".to_string(),
max_module_ticks.to_string(),
max_function_ticks.to_string(),
max_module_ticks.map_or_else(|| "None".to_string(), |x| x.to_string()),
max_function_ticks.map_or_else(|| "None".to_string(), |x| x.to_string()),
]);
builder.push_record(vec![
"Used".to_string(),
Expand All @@ -2093,9 +2098,18 @@ impl Display for SuiClientCommandResult {
]);
let mut table = builder.build();
table.with(TableStyle::rounded());
if (used_module_ticks > max_module_ticks)
|| (used_function_ticks > max_function_ticks)
{

let module_exceeded_ticks = matches!(
max_module_ticks,
Some(ticks) if ticks < used_module_ticks,
);

let function_exceeded_ticks = matches!(
max_function_ticks,
Some(ticks) if ticks < used_function_ticks,
);

if module_exceeded_ticks || function_exceeded_ticks {
table.with(TablePanel::header("Module will NOT pass metering check!"));
} else {
table.with(TablePanel::header("Module will pass metering check!"));
Expand Down Expand Up @@ -2377,8 +2391,8 @@ pub enum SuiClientCommandResult {
TransferSui(SuiTransactionBlockResponse),
Upgrade(SuiTransactionBlockResponse),
VerifyBytecodeMeter {
max_module_ticks: u128,
max_function_ticks: u128,
max_module_ticks: Option<u128>,
max_function_ticks: Option<u128>,
used_function_ticks: u128,
used_module_ticks: u128,
},
Expand Down
Loading

0 comments on commit b378ff2

Please sign in to comment.