[move] Rewrite verifier metering (#19036)

## Description - Added ability cache for verifier ability queries - Redid metering for typing, local safety, and reference safety ## Test plan - calibrated constants --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
MystenLabs · Aug 22, 2024 · 22aa24d · 22aa24d
1 parent 2969b59
commit 22aa24d
Show file tree

Hide file tree

Showing 33 changed files with 971 additions and 574 deletions.
diff --git a/crates/sui-framework-tests/src/metered_verifier.rs b/crates/sui-framework-tests/src/metered_verifier.rs
@@ -35,7 +35,7 @@ fn test_metered_move_bytecode_verifier() {
 
     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 mut meter_config = protocol_config.meter_config_for_signing();
     let registry = &Registry::new();
     let bytecode_verifier_metrics = Arc::new(BytecodeVerifierMetrics::new(registry));
     let mut meter = SuiVerifierMeter::new(meter_config.clone());
@@ -202,7 +202,7 @@ fn test_metered_move_bytecode_verifier() {
 
     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 meter_config = protocol_config.meter_config_for_signing();
 
     // Check if the same meter is indeed used multiple invocations of the verifier
     let mut meter = SuiVerifierMeter::new(meter_config);
@@ -229,7 +229,7 @@ fn test_meter_system_packages() {
 
     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 meter_config = protocol_config.meter_config_for_signing();
     let registry = &Registry::new();
     let bytecode_verifier_metrics = Arc::new(BytecodeVerifierMetrics::new(registry));
     let mut meter = SuiVerifierMeter::new(meter_config);
@@ -283,7 +283,7 @@ fn test_build_and_verify_programmability_examples() {
 
     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 meter_config = protocol_config.meter_config_for_signing();
     let registry = &Registry::new();
     let bytecode_verifier_metrics = Arc::new(BytecodeVerifierMetrics::new(registry));
     let examples = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../examples");

diff --git a/crates/sui-protocol-config/src/lib.rs b/crates/sui-protocol-config/src/lib.rs
@@ -2736,16 +2736,13 @@ impl ProtocolConfig {
         }
     }
 
-    pub fn meter_config(&self) -> MeterConfig {
+    /// MeterConfig for metering packages during signing. It is NOT stable between binaries and
+    /// cannot used during execution.
+    pub fn meter_config_for_signing(&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),
-            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,
-            ),
+            max_per_fun_meter_units: Some(2_200_000),
+            max_per_mod_meter_units: Some(2_200_000),
+            max_per_pkg_meter_units: Some(2_200_000),
         }
     }
 

diff --git a/crates/sui-transaction-checks/src/lib.rs b/crates/sui-transaction-checks/src/lib.rs
@@ -562,7 +562,7 @@ mod checked {
         // 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());
+        let mut meter = verifier.meter(protocol_config.meter_config_for_signing());
 
         // Measure time for verifying all packages in the PTB
         let shared_meter_verifier_timer = metrics

diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs
@@ -1106,7 +1106,7 @@ impl SuiClientCommands {
                 let mut used_ticks = meter.accumulator(Scope::Package).clone();
                 used_ticks.name = pkg_name;
 
-                let meter_config = protocol_config.meter_config();
+                let meter_config = protocol_config.meter_config_for_signing();
 
                 let exceeded = matches!(
                     meter_config.max_per_pkg_meter_units,

diff --git a/external-crates/move/crates/bytecode-verifier-prop-tests/src/unit_tests/prop_tests.rs b/external-crates/move/crates/bytecode-verifier-prop-tests/src/unit_tests/prop_tests.rs
@@ -13,9 +13,11 @@ use move_binary_format::{
     proptest_types::CompiledModuleStrategyGen,
 };
 use move_bytecode_verifier::{
-    ability_field_requirements, constants, instantiation_loops::InstantiationLoopChecker,
-    DuplicationChecker, InstructionConsistency, RecursiveDataDefChecker, SignatureChecker,
+    ability_cache::AbilityCache, ability_field_requirements, constants,
+    instantiation_loops::InstantiationLoopChecker, DuplicationChecker, InstructionConsistency,
+    RecursiveDataDefChecker, SignatureChecker,
 };
+use move_bytecode_verifier_meter::dummy::DummyMeter;
 use move_core_types::{
     account_address::AccountAddress, identifier::Identifier, vm_status::StatusCode,
 };
@@ -28,7 +30,9 @@ proptest! {
 
     #[test]
     fn valid_ability_transitivity(module in CompiledModule::valid_strategy(20)) {
-        prop_assert!(ability_field_requirements::verify_module(&module).is_ok());
+        let module = &module;
+        let ability_cache = &mut AbilityCache::new(module);
+        prop_assert!(ability_field_requirements::verify_module(module, ability_cache, &mut DummyMeter).is_ok());
     }
 
     #[test]
@@ -101,18 +105,22 @@ proptest! {
 
     #[test]
     fn check_verifier_passes(module in CompiledModule::valid_strategy(20)) {
-        DuplicationChecker::verify_module(&module).expect("DuplicationChecker failure");
-        SignatureChecker::verify_module(&module).expect("SignatureChecker failure");
-        InstructionConsistency::verify_module(&module).expect("InstructionConsistency failure");
-        constants::verify_module(&module).expect("constants failure");
-        ability_field_requirements::verify_module(&module).expect("ability_field_requirements failure");
-        RecursiveDataDefChecker::verify_module(&module).expect("RecursiveDataDefChecker failure");
-        InstantiationLoopChecker::verify_module(&module).expect("InstantiationLoopChecker failure");
+        let module = &module;
+        let ability_cache = &mut AbilityCache::new(module);
+        DuplicationChecker::verify_module(module).expect("DuplicationChecker failure");
+        SignatureChecker::verify_module(module, ability_cache, &mut DummyMeter).expect("SignatureChecker failure");
+        InstructionConsistency::verify_module(module).expect("InstructionConsistency failure");
+        constants::verify_module(module).expect("constants failure");
+        ability_field_requirements::verify_module(module, ability_cache, &mut DummyMeter).expect("ability_field_requirements failure");
+        RecursiveDataDefChecker::verify_module(module).expect("RecursiveDataDefChecker failure");
+        InstantiationLoopChecker::verify_module(module).expect("InstantiationLoopChecker failure");
     }
 
     #[test]
     fn valid_signatures(module in CompiledModule::valid_strategy(20)) {
-        prop_assert!(SignatureChecker::verify_module(&module).is_ok())
+        let module = &module;
+        let ability_cache = &mut AbilityCache::new(module);
+        prop_assert!(SignatureChecker::verify_module(module, ability_cache, &mut DummyMeter).is_ok())
     }
 
     #[test]
@@ -123,7 +131,9 @@ proptest! {
         let context = SignatureRefMutation::new(&mut module, mutations);
         let expected_violations = context.apply();
 
-        let result = SignatureChecker::verify_module(&module);
+        let module = &module;
+        let ability_cache = &mut AbilityCache::new(module);
+        let result = SignatureChecker::verify_module(module, ability_cache, &mut DummyMeter);
 
         prop_assert_eq!(expected_violations, result.is_err());
     }
@@ -136,7 +146,9 @@ proptest! {
         let context = FieldRefMutation::new(&mut module, mutations);
         let expected_violations = context.apply();
 
-        let result = SignatureChecker::verify_module(&module);
+        let module = &module;
+        let ability_cache = &mut AbilityCache::new(module);
+        let result = SignatureChecker::verify_module(module, ability_cache, &mut DummyMeter);
 
         prop_assert_eq!(expected_violations, result.is_err());
     }

diff --git a/external-crates/move/crates/bytecode-verifier-tests/src/unit_tests/code_unit_tests.rs b/external-crates/move/crates/bytecode-verifier-tests/src/unit_tests/code_unit_tests.rs
@@ -4,15 +4,22 @@
 
 use crate::support::dummy_procedure_module;
 use move_binary_format::file_format::Bytecode;
-use move_bytecode_verifier::CodeUnitVerifier;
+use move_bytecode_verifier::ability_cache::AbilityCache;
+use move_bytecode_verifier::code_unit_verifier;
 use move_bytecode_verifier_meter::dummy::DummyMeter;
 use move_core_types::vm_status::StatusCode;
 use move_vm_config::verifier::VerifierConfig;
 
 #[test]
 fn invalid_fallthrough_br_true() {
-    let module = dummy_procedure_module(vec![Bytecode::LdFalse, Bytecode::BrTrue(1)]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::LdFalse, Bytecode::BrTrue(1)]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::INVALID_FALL_THROUGH
@@ -21,8 +28,14 @@ fn invalid_fallthrough_br_true() {
 
 #[test]
 fn invalid_fallthrough_br_false() {
-    let module = dummy_procedure_module(vec![Bytecode::LdTrue, Bytecode::BrFalse(1)]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::LdTrue, Bytecode::BrFalse(1)]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::INVALID_FALL_THROUGH
@@ -32,8 +45,14 @@ fn invalid_fallthrough_br_false() {
 // all non-branch instructions should trigger invalid fallthrough; just check one of them
 #[test]
 fn invalid_fallthrough_non_branch() {
-    let module = dummy_procedure_module(vec![Bytecode::LdTrue, Bytecode::Pop]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::LdTrue, Bytecode::Pop]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::INVALID_FALL_THROUGH
@@ -42,22 +61,40 @@ fn invalid_fallthrough_non_branch() {
 
 #[test]
 fn valid_fallthrough_branch() {
-    let module = dummy_procedure_module(vec![Bytecode::Branch(0)]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::Branch(0)]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert!(result.is_ok());
 }
 
 #[test]
 fn valid_fallthrough_ret() {
-    let module = dummy_procedure_module(vec![Bytecode::Ret]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::Ret]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert!(result.is_ok());
 }
 
 #[test]
 fn valid_fallthrough_abort() {
-    let module = dummy_procedure_module(vec![Bytecode::LdU64(7), Bytecode::Abort]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::LdU64(7), Bytecode::Abort]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert!(result.is_ok());
 }
 
@@ -68,10 +105,15 @@ fn test_max_number_of_bytecode() {
         nops.push(Bytecode::Nop);
     }
     nops.push(Bytecode::Ret);
-    let module = dummy_procedure_module(nops);
+    let module = &dummy_procedure_module(nops);
+    let ability_cache = &mut AbilityCache::new(module);
 
-    let result =
-        CodeUnitVerifier::verify_module(&VerifierConfig::default(), &module, &mut DummyMeter);
+    let result = code_unit_verifier::verify_module(
+        &VerifierConfig::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert!(result.is_ok());
 }
 
@@ -81,14 +123,16 @@ fn test_max_basic_blocks() {
         .map(|idx| Bytecode::Branch(idx + 1))
         .collect::<Vec<_>>();
     code.push(Bytecode::Ret);
-    let module = dummy_procedure_module(code);
+    let module = &dummy_procedure_module(code);
+    let ability_cache = &mut AbilityCache::new(module);
 
-    let result = CodeUnitVerifier::verify_module(
+    let result = code_unit_verifier::verify_module(
         &VerifierConfig {
             max_basic_blocks: Some(16),
             ..Default::default()
         },
-        &module,
+        module,
+        ability_cache,
         &mut DummyMeter,
     );
     assert_eq!(

diff --git a/external-crates/move/crates/bytecode-verifier-tests/src/unit_tests/mod.rs b/external-crates/move/crates/bytecode-verifier-tests/src/unit_tests/mod.rs
@@ -53,6 +53,6 @@ pub(crate) fn production_config() -> (VerifierConfig, MeterConfig) {
             bytecode_version: VERSION_MAX,
             max_variants_in_enum: Some(DEFAULT_MAX_VARIANTS),
         },
-        MeterConfig::default(),
+        MeterConfig::old_default(),
     )
 }
diff --git a/...al-crates/move/crates/bytecode-verifier-tests/src/unit_tests/negative_stack_size_tests.rs b/...al-crates/move/crates/bytecode-verifier-tests/src/unit_tests/negative_stack_size_tests.rs
@@ -4,14 +4,21 @@
 
 use crate::support::dummy_procedure_module;
 use move_binary_format::file_format::Bytecode;
-use move_bytecode_verifier::CodeUnitVerifier;
+use move_bytecode_verifier::ability_cache::AbilityCache;
+use move_bytecode_verifier::code_unit_verifier;
 use move_bytecode_verifier_meter::dummy::DummyMeter;
 use move_core_types::vm_status::StatusCode;
 
 #[test]
 fn one_pop_no_push() {
-    let module = dummy_procedure_module(vec![Bytecode::Pop, Bytecode::Ret]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::Pop, Bytecode::Ret]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::NEGATIVE_STACK_SIZE_WITHIN_BLOCK
@@ -21,8 +28,14 @@ fn one_pop_no_push() {
 #[test]
 fn one_pop_one_push() {
     // Height: 0 + (-1 + 1) = 0 would have passed original usage verifier
-    let module = dummy_procedure_module(vec![Bytecode::ReadRef, Bytecode::Ret]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::ReadRef, Bytecode::Ret]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::NEGATIVE_STACK_SIZE_WITHIN_BLOCK
@@ -32,8 +45,14 @@ fn one_pop_one_push() {
 #[test]
 fn two_pop_one_push() {
     // Height: 0 + 1 + (-2 + 1) = 0 would have passed original usage verifier
-    let module = dummy_procedure_module(vec![Bytecode::LdU64(0), Bytecode::Add, Bytecode::Ret]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::LdU64(0), Bytecode::Add, Bytecode::Ret]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::NEGATIVE_STACK_SIZE_WITHIN_BLOCK
@@ -42,8 +61,14 @@ fn two_pop_one_push() {
 
 #[test]
 fn two_pop_no_push() {
-    let module = dummy_procedure_module(vec![Bytecode::WriteRef, Bytecode::Ret]);
-    let result = CodeUnitVerifier::verify_module(&Default::default(), &module, &mut DummyMeter);
+    let module = &dummy_procedure_module(vec![Bytecode::WriteRef, Bytecode::Ret]);
+    let ability_cache = &mut AbilityCache::new(module);
+    let result = code_unit_verifier::verify_module(
+        &Default::default(),
+        module,
+        ability_cache,
+        &mut DummyMeter,
+    );
     assert_eq!(
         result.unwrap_err().major_status(),
         StatusCode::NEGATIVE_STACK_SIZE_WITHIN_BLOCK