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(()) }