Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[execution] Update to deps-only mode #20102

Merged
merged 3 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "package_upgrade_base"
version = "0.0.1"
edition = "2024.beta"

[addresses]
base_addr = "0x0"

[dependencies]
Sui = { local = "../../../../../../sui-framework/packages/sui-framework" }
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::base {

public struct A<T> {
f1: bool,
f2: T
}

public fun return_0(): u64 { abort 42 }

public fun plus_1(x: u64): u64 { x + 1 }

public(package) fun friend_fun(x: u64): u64 { x }

fun non_public_fun(y: bool): u64 { if (y) 0 else 1 }

entry fun entry_fun() { }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::friend_module {

public struct A<T> {
field1: u64,
field2: T
}

public fun friend_call(): u64 { base_addr::base::friend_fun(1) }

public fun return_0(): u64 { 0 }

public fun plus_1(x: u64): u64 { x + 1 }

fun non_public_fun(y: bool): u64 { if (y) 0 else 1 }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::new_friend_module;

public fun friend_call(): u64 { base_addr::base::friend_fun(1) }
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "package_upgrade_base"
version = "0.0.1"
edition = "2024.beta"

[addresses]
base_addr = "0x0"

[dependencies]
Sui = { local = "../../../../../../sui-framework/packages/sui-framework" }
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::base {

public struct A<T> {
f1: bool,
f2: T
}

public fun return_0(): u64 { abort 42 }

public fun plus_1(x: u64): u64 { x + 1 }

public(package) fun friend_fun(x: u64): u64 { x }

fun non_public_fun(y: bool): u64 { if (y) 0 else 1 }

entry fun entry_fun() { }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::friend_module {

public struct A<T> {
field1: u64,
field2: T
}

public fun friend_call(): u64 { base_addr::base::friend_fun(1) }

public fun return_0(): u64 { 0 }

public fun plus_1(x: u64): u64 { x + 1 }

fun non_public_fun(y: bool): u64 { if (y) 0 else 1 }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::new_module;
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "package_upgrade_base"
version = "0.0.1"
edition = "2024.beta"

[addresses]
base_addr = "0x0"

[dependencies]
Sui = { local = "../../../../../../sui-framework/packages/sui-framework" }
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

module base_addr::base {

public struct A<T> {
f1: bool,
f2: T
}

public fun return_0(): u64 { abort 42 }

public fun plus_1(x: u64): u64 { x + 1 }

public(package) fun friend_fun(x: u64): u64 { x }

fun non_public_fun(y: bool): u64 { if (y) 0 else 1 }

entry fun entry_fun() { }
}
116 changes: 116 additions & 0 deletions crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,122 @@ 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 (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_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_add_new_module_in_dep_only_mode() {
let mut runner = UpgradeStateRunner::new("move_upgrade/base").await;

let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_module");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to slice out the new module here, retry the publish, and see that it succeeds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have that exact test pretty much here: https://github.com/MystenLabs/sui/blob/main/crates/sui-core/src/unit_tests/move_package_upgrade_tests.rs#L573

This is the same package but without new_module.move.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a slightly different from what I am asking. As it stands, it is not guaranteed that the error in this test from the fact that there is a new module.
If you take the modules list and splice it out, you would guarantee that within this test.

Copy link
Contributor Author

@tzakian tzakian Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated so they all start from the valid dep_only_upgrade (which is verified in each unit test), and then files are explicitly added or removed from that.

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_add_new_friend_module_in_dep_only_mode_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 (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_friend_module");
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_add_new_friend_module_in_dep_only_mode() {
let mut runner = UpgradeStateRunner::new("move_upgrade/base").await;

let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_new_friend_module");
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_remove_module_in_dep_only_mode() {
let mut runner = UpgradeStateRunner::new("move_upgrade/base").await;

let (digest, modules) = build_upgrade_test_modules("dep_only_upgrade_remove_module");
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;
Expand Down
1 change: 1 addition & 0 deletions crates/sui-open-rpc/spec/openrpc.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 16 additions & 0 deletions crates/sui-protocol-config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))]
Expand Down Expand Up @@ -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:
//
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ mod checked {
fn check_compatibility<'a>(
context: &ExecutionContext,
existing_package: &MovePackage,
upgrading_modules: impl IntoIterator<Item = &'a CompiledModule>,
upgrading_modules: &[CompiledModule],
policy: u8,
) -> Result<(), ExecutionError> {
// Make sure this is a known upgrade policy.
Expand All @@ -660,6 +660,25 @@ mod checked {
invariant_violation!("Tried to normalize modules in existing package but failed")
};

let existing_modules_len = current_normalized.len();
let upgrading_modules_len = upgrading_modules.len();
if context
.protocol_config
.disallow_new_modules_in_deps_only_packages()
&& policy as u8 == UpgradePolicy::DEP_ONLY
&& existing_modules_len != upgrading_modules_len
tzakian marked this conversation as resolved.
Show resolved Hide resolved
{
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.into_iter());
for (name, cur_module) in current_normalized {
let Some(new_module) = new_normalized.remove(&name) else {
Expand Down
Loading