From f2670718f694edd59e8a062bc932d4efd96ba765 Mon Sep 17 00:00:00 2001 From: Tim Zakian <2895723+tzakian@users.noreply.github.com> Date: Wed, 30 Oct 2024 16:49:01 -0700 Subject: [PATCH] [execution] Update to deps-only mode (#20102) ## Description Disallow new modules from being added to packages in deps-only mode. ## Test plan Added new tests to check for both previous and new behavior. --- ## 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. - [X] Protocol: Updated restrictions on deps-only packages to not allow adding new modules to the package. - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API: --- .../unit_tests/move_package_upgrade_tests.rs | 199 ++++++++++++++++-- crates/sui-open-rpc/spec/openrpc.json | 1 + crates/sui-protocol-config/src/lib.rs | 16 ++ ...ocol_config__test__Mainnet_version_68.snap | 1 + ...ocol_config__test__Testnet_version_68.snap | 1 + ...sui_protocol_config__test__version_68.snap | 1 + .../programmable_transactions/execution.rs | 28 ++- 7 files changed, 229 insertions(+), 18 deletions(-) diff --git a/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs b/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs index eed9dcf2af4be..4ca5c82492a5f 100644 --- a/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs +++ b/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs @@ -15,7 +15,12 @@ use sui_types::{ MOVE_STDLIB_PACKAGE_ID, SUI_FRAMEWORK_PACKAGE_ID, }; -use std::{collections::BTreeSet, path::PathBuf, str::FromStr, sync::Arc}; +use std::{ + collections::BTreeSet, + path::{Path, PathBuf}, + str::FromStr, + sync::Arc, +}; use sui_types::effects::{TransactionEffects, TransactionEffectsAPI}; use sui_types::error::{SuiError, UserInputError}; use sui_types::execution_config_utils::to_binary_config; @@ -48,11 +53,62 @@ macro_rules! move_call { } } +enum FileOverlay<'a> { + Remove(&'a str), + Add { + file_name: &'a str, + contents: &'a str, + }, +} + +fn build_upgrade_test_modules_with_overlay( + base_pkg: &str, + overlay: FileOverlay<'_>, +) -> (Vec, Vec>) { + // Root temp dirs under `move_upgrade` directory so that dependency paths remain correct. + let mut tmp_dir_root_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + tmp_dir_root_path.extend(["src", "unit_tests", "data", "move_upgrade"]); + + let tmp_dir = tempfile::TempDir::new_in(tmp_dir_root_path).unwrap(); + let tmp_dir_path = tmp_dir.path(); + + let mut copy_options = fs_extra::dir::CopyOptions::new(); + copy_options.copy_inside = true; + copy_options.content_only = true; + let source_dir = pkg_path_of(base_pkg); + fs_extra::dir::copy(source_dir, tmp_dir_path, ©_options).unwrap(); + + match overlay { + FileOverlay::Remove(file_name) => { + let file_path = tmp_dir_path.join(format!("sources/{}", file_name)); + std::fs::remove_file(file_path).unwrap(); + } + FileOverlay::Add { + file_name, + contents, + } => { + let new_file_path = tmp_dir_path.join(format!("sources/{}", file_name)); + std::fs::write(new_file_path, contents).unwrap(); + } + } + + build_pkg_at_path(tmp_dir_path) +} + fn build_upgrade_test_modules(test_dir: &str) -> (Vec, Vec>) { + let path = pkg_path_of(test_dir); + build_pkg_at_path(&path) +} + +fn pkg_path_of(pkg_name: &str) -> PathBuf { let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); - path.extend(["src", "unit_tests", "data", "move_upgrade", test_dir]); + path.extend(["src", "unit_tests", "data", "move_upgrade", pkg_name]); + path +} + +fn build_pkg_at_path(path: &Path) -> (Vec, Vec>) { let with_unpublished_deps = false; - let package = BuildConfig::new_for_testing().build(&path).unwrap(); + let package = BuildConfig::new_for_testing().build(path).unwrap(); ( package.get_package_digest(with_unpublished_deps).to_vec(), package.get_package_bytes(with_unpublished_deps), @@ -457,6 +513,116 @@ async fn test_upgrade_package_compatible_in_dep_only_mode() { ); } +#[tokio::test] +async fn test_upgrade_package_add_new_module_in_dep_only_mode_pre_v68() { + // Allow new modules in deps-only mode for this test. + let _guard = ProtocolConfig::apply_overrides_for_testing(|_, mut config| { + config.set_disallow_new_modules_in_deps_only_packages_for_testing(false); + config + }); + + let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let (digest, modules) = build_upgrade_test_modules_with_overlay( + base_pkg, + FileOverlay::Add { + file_name: "new_module.move", + contents: "module base_addr::new_module;", + }, + ); + let effects = runner + .upgrade( + UpgradePolicy::DEP_ONLY, + digest, + modules, + vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], + ) + .await; + + assert!(effects.status().is_ok(), "{:#?}", effects.status()); +} + +#[tokio::test] +async fn test_upgrade_package_invalid_dep_only_upgrade_pre_v68() { + let _guard = ProtocolConfig::apply_overrides_for_testing(|_, mut config| { + config.set_disallow_new_modules_in_deps_only_packages_for_testing(false); + config + }); + + let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let overlays = [ + FileOverlay::Add { + file_name: "new_friend_module.move", + contents: r#" +module base_addr::new_friend_module; +public fun friend_call(): u64 { base_addr::base::friend_fun(1) } + "#, + }, + FileOverlay::Remove("friend_module.move"), + ]; + for overlay in overlays { + let (digest, modules) = build_upgrade_test_modules_with_overlay(base_pkg, overlay); + let effects = runner + .upgrade( + UpgradePolicy::DEP_ONLY, + digest, + modules, + vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], + ) + .await; + + assert_eq!( + effects.into_status().unwrap_err().0, + ExecutionFailureStatus::PackageUpgradeError { + upgrade_error: PackageUpgradeError::IncompatibleUpgrade + }, + ); + } +} + +#[tokio::test] +async fn test_invalid_dep_only_upgrades() { + let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; + let base_pkg = "dep_only_upgrade"; + assert_valid_dep_only_upgrade(&mut runner, base_pkg).await; + let overlays = [ + FileOverlay::Add { + file_name: "new_module.move", + contents: "module base_addr::new_module;", + }, + FileOverlay::Add { + file_name: "new_friend_module.move", + contents: r#" +module base_addr::new_friend_module; +public fun friend_call(): u64 { base_addr::base::friend_fun(1) } + "#, + }, + FileOverlay::Remove("friend_module.move"), + ]; + + for overlay in overlays { + let (digest, modules) = build_upgrade_test_modules_with_overlay(base_pkg, overlay); + let effects = runner + .upgrade( + UpgradePolicy::DEP_ONLY, + digest, + modules, + vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], + ) + .await; + + assert_eq!( + effects.into_status().unwrap_err().0, + ExecutionFailureStatus::PackageUpgradeError { + upgrade_error: PackageUpgradeError::IncompatibleUpgrade + }, + ); + } +} + #[tokio::test] async fn test_upgrade_package_compatible_in_additive_mode() { let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; @@ -572,18 +738,7 @@ async fn test_upgrade_package_additive_dep_only_mode() { #[tokio::test] async fn test_upgrade_package_dep_only_mode() { let mut runner = UpgradeStateRunner::new("move_upgrade/base").await; - - let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade"); - let effects = runner - .upgrade( - UpgradePolicy::DEP_ONLY, - digest, - modules, - vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], - ) - .await; - - assert!(effects.status().is_ok(), "{:#?}", effects.status()); + assert_valid_dep_only_upgrade(&mut runner, "dep_only_upgrade").await; } #[tokio::test] @@ -1432,3 +1587,17 @@ async fn test_upgrade_more_than_max_packages_error() { } ); } + +async fn assert_valid_dep_only_upgrade(runner: &mut UpgradeStateRunner, package_name: &str) { + let (digest, modules) = build_upgrade_test_modules(package_name); + let effects = runner + .upgrade( + UpgradePolicy::DEP_ONLY, + digest, + modules, + vec![SUI_FRAMEWORK_PACKAGE_ID, MOVE_STDLIB_PACKAGE_ID], + ) + .await; + + assert!(effects.status().is_ok(), "{:#?}", effects.status()); +} diff --git a/crates/sui-open-rpc/spec/openrpc.json b/crates/sui-open-rpc/spec/openrpc.json index d504bad18c95c..de8467490bdab 100644 --- a/crates/sui-open-rpc/spec/openrpc.json +++ b/crates/sui-open-rpc/spec/openrpc.json @@ -1310,6 +1310,7 @@ "disable_invariant_violation_check_in_swap_loc": false, "disallow_adding_abilities_on_upgrade": false, "disallow_change_struct_type_params_on_upgrade": false, + "disallow_new_modules_in_deps_only_packages": false, "enable_coin_deny_list": false, "enable_coin_deny_list_v2": false, "enable_effects_v2": false, diff --git a/crates/sui-protocol-config/src/lib.rs b/crates/sui-protocol-config/src/lib.rs index 08c5991f28371..caccb8d70d662 100644 --- a/crates/sui-protocol-config/src/lib.rs +++ b/crates/sui-protocol-config/src/lib.rs @@ -194,6 +194,7 @@ const MAX_PROTOCOL_VERSION: u64 = 68; // Update to Move stdlib. // Enable gas based congestion control with overage. // Further reduce minimum number of random beacon shares. +// Disallow adding new modules in `deps-only` packages. #[derive(Copy, Clone, Debug, Hash, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] pub struct ProtocolVersion(u64); @@ -560,6 +561,9 @@ struct FeatureFlags { // Enable uncompressed group elements in BLS123-81 G1 #[serde(skip_serializing_if = "is_false")] uncompressed_g1_group_elements: bool, + + #[serde(skip_serializing_if = "is_false")] + disallow_new_modules_in_deps_only_packages: bool, } fn is_false(b: &bool) -> bool { @@ -1659,6 +1663,11 @@ impl ProtocolConfig { pub fn uncompressed_g1_group_elements(&self) -> bool { self.feature_flags.uncompressed_g1_group_elements } + + pub fn disallow_new_modules_in_deps_only_packages(&self) -> bool { + self.feature_flags + .disallow_new_modules_in_deps_only_packages + } } #[cfg(not(msim))] @@ -2920,6 +2929,8 @@ impl ProtocolConfig { // Further reduce minimum number of random beacon shares. cfg.random_beacon_reduction_lower_bound = Some(500); + + cfg.feature_flags.disallow_new_modules_in_deps_only_packages = true; } // Use this template when making changes: // @@ -3086,6 +3097,11 @@ impl ProtocolConfig { pub fn set_gc_depth_for_testing(&mut self, val: u32) { self.consensus_gc_depth = Some(val); } + + pub fn set_disallow_new_modules_in_deps_only_packages_for_testing(&mut self, val: bool) { + self.feature_flags + .disallow_new_modules_in_deps_only_packages = val; + } } type OverrideFn = dyn Fn(ProtocolVersion, ProtocolConfig) -> ProtocolConfig + Send; diff --git a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_68.snap b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_68.snap index e05651d46bef8..492aebef1a352 100644 --- a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_68.snap +++ b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Mainnet_version_68.snap @@ -64,6 +64,7 @@ feature_flags: consensus_round_prober: true validate_identifier_inputs: true relocate_event_module: true + disallow_new_modules_in_deps_only_packages: true max_tx_size_bytes: 131072 max_input_objects: 2048 max_size_written_objects: 5000000 diff --git a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_68.snap b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_68.snap index e05651d46bef8..492aebef1a352 100644 --- a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_68.snap +++ b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__Testnet_version_68.snap @@ -64,6 +64,7 @@ feature_flags: consensus_round_prober: true validate_identifier_inputs: true relocate_event_module: true + disallow_new_modules_in_deps_only_packages: true max_tx_size_bytes: 131072 max_input_objects: 2048 max_size_written_objects: 5000000 diff --git a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_68.snap b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_68.snap index 613b09985531d..68e55955524b4 100644 --- a/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_68.snap +++ b/crates/sui-protocol-config/src/snapshots/sui_protocol_config__test__version_68.snap @@ -71,6 +71,7 @@ feature_flags: mysticeti_fastpath: true relocate_event_module: true uncompressed_g1_group_elements: true + disallow_new_modules_in_deps_only_packages: true max_tx_size_bytes: 131072 max_input_objects: 2048 max_size_written_objects: 5000000 diff --git a/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs b/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs index 12f144fbb04cf..2b72db7222021 100644 --- a/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs +++ b/sui-execution/latest/sui-adapter/src/programmable_transactions/execution.rs @@ -640,10 +640,10 @@ mod checked { )]) } - fn check_compatibility<'a>( + fn check_compatibility( context: &ExecutionContext, existing_package: &MovePackage, - upgrading_modules: impl IntoIterator, + upgrading_modules: &[CompiledModule], policy: u8, ) -> Result<(), ExecutionError> { // Make sure this is a known upgrade policy. @@ -660,7 +660,26 @@ mod checked { invariant_violation!("Tried to normalize modules in existing package but failed") }; - let mut new_normalized = normalize_deserialized_modules(upgrading_modules.into_iter()); + let existing_modules_len = current_normalized.len(); + let upgrading_modules_len = upgrading_modules.len(); + let disallow_new_modules = context + .protocol_config + .disallow_new_modules_in_deps_only_packages() + && policy as u8 == UpgradePolicy::DEP_ONLY; + + if disallow_new_modules && existing_modules_len != upgrading_modules_len { + return Err(ExecutionError::new_with_source( + ExecutionErrorKind::PackageUpgradeError { + upgrade_error: PackageUpgradeError::IncompatibleUpgrade, + }, + format!( + "Existing package has {existing_modules_len} modules, but new package has \ + {upgrading_modules_len}. Adding or removing a module to a deps only package is not allowed." + ), + )); + } + + let mut new_normalized = normalize_deserialized_modules(upgrading_modules.iter()); for (name, cur_module) in current_normalized { let Some(new_module) = new_normalized.remove(&name) else { return Err(ExecutionError::new_with_source( @@ -674,6 +693,9 @@ mod checked { check_module_compatibility(&policy, &cur_module, &new_module)?; } + // If we disallow new modules double check that there are no modules left in `new_normalized`. + debug_assert!(!disallow_new_modules || new_normalized.is_empty()); + Ok(()) }