Skip to content

Commit

Permalink
add compatability trait to move-binary-format (#19450)
Browse files Browse the repository at this point in the history 
## Description 

Add a trait which will allow the both execution and CLI to gather the
necessary information for checking upgrade compatibility. The current
implementation supports determining layout, linking and enum variant
problems but does not hold information about each instance where it
occurs, furthermore information is lost when mapped to an enum with no
associated data. To ensure users can extract more information this new
type will accumulate the struct, enum, and function's where each error
occurs for upgrades, while preserving the previous error type for
execution.

## Test plan 

How did you test the new or updated feature?

---

## 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:
  • Loading branch information
@jordanjennings-mysten
jordanjennings-mysten authored Sep 25, 2024
1 parent b816c49 commit 34c9e3e
Show file tree
Hide file tree
Showing 3 changed files with 357 additions and 69 deletions.
123 changes: 54 additions & 69 deletions external-crates/move/crates/move-binary-format/src/compatibility.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use std::collections::BTreeSet;

use crate::{
compatibility_mode::{CompatibilityMode, ExecutionCompatibilityMode},
errors::{PartialVMError, PartialVMResult},
file_format::{AbilitySet, DatatypeTyParameter, Visibility},
file_format_common::VERSION_5,
Expand Down Expand Up @@ -92,15 +93,25 @@ impl Compatibility {

/// Check compatibility for `new_module` relative to old module `old_module`.
pub fn check(&self, old_module: &Module, new_module: &Module) -> PartialVMResult<()> {
let mut datatype_and_function_linking = true;
let mut datatype_layout = true;
let mut friend_linking = true;
let mut entry_linking = true;
let mut no_new_variants = true;
self.check_with_mode::<ExecutionCompatibilityMode>(old_module, new_module)
.map_err(|_| PartialVMError::new(StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE))
}

pub fn check_with_mode<M: CompatibilityMode>(
&self,
old_module: &Module,
new_module: &Module,
) -> Result<(), M::Error> {
let mut context = M::default();

// module's name and address are unchanged
if old_module.address != new_module.address || old_module.name != new_module.name {
datatype_and_function_linking = false;
context.module_id_mismatch(
&old_module.address,
&old_module.name,
&new_module.address,
&new_module.name,
);
}

// old module's structs are a subset of the new module's structs
Expand All @@ -109,21 +120,24 @@ impl Compatibility {
// Struct not present in new . Existing modules that depend on this struct will fail to link with the new version of the module.
// Also, struct layout cannot be guaranteed transitively, because after
// removing the struct, it could be re-added later with a different layout.
datatype_and_function_linking = false;
datatype_layout = false;
break;
context.struct_missing(name, old_struct);
continue;
};

if !datatype_abilities_compatible(
self.disallowed_new_abilities,
old_struct.abilities,
new_struct.abilities,
) || !datatype_type_parameters_compatible(
) {
context.struct_ability_mismatch(name, old_struct, new_struct);
}

if !datatype_type_parameters_compatible(
self.disallow_change_datatype_type_params,
&old_struct.type_parameters,
&new_struct.type_parameters,
) {
datatype_and_function_linking = false;
context.struct_type_param_mismatch(name, old_struct, new_struct);
}
if new_struct.fields != old_struct.fields {
// Fields changed. Code in this module will fail at runtime if it tries to
Expand All @@ -132,7 +146,8 @@ impl Compatibility {
// choose that changing the name (but not position or type) of a field is
// compatible. The VM does not care about the name of a field
// (it's purely informational), but clients presumably do.
datatype_layout = false

context.struct_field_mismatch(name, old_struct, new_struct);
}
}

Expand All @@ -141,45 +156,45 @@ impl Compatibility {
// Enum not present in new. Existing modules that depend on this enum will fail to link with the new version of the module.
// Also, enum layout cannot be guaranteed transitively, because after
// removing the enum, it could be re-added later with a different layout.
datatype_and_function_linking = false;
datatype_layout = false;
break;

context.enum_missing(name, old_enum);
continue;
};

if !datatype_abilities_compatible(
self.disallowed_new_abilities,
old_enum.abilities,
new_enum.abilities,
) || !datatype_type_parameters_compatible(
) {
context.enum_ability_mismatch(name, old_enum, new_enum);
}

if !datatype_type_parameters_compatible(
self.disallow_change_datatype_type_params,
&old_enum.type_parameters,
&new_enum.type_parameters,
) {
datatype_and_function_linking = false;
context.enum_type_param_mismatch(name, old_enum, new_enum);
}

if new_enum.variants.len() > old_enum.variants.len() {
no_new_variants = false;
}

if new_enum.variants.len() < old_enum.variants.len() {
datatype_layout = false;
context.enum_new_variant(name, old_enum, new_enum);
}

for (tag, old_variant) in old_enum.variants.iter().enumerate() {
// If the new enum has fewer variants than the old one, datatype_layout is false
// and we don't need to check the rest of the variants.
let Some(new_variant) = new_enum.variants.get(tag) else {
datatype_layout = false;
break;
context.enum_variant_missing(name, old_enum, tag);
continue;
};
if new_variant.name != old_variant.name {
// TODO: Variant renamed. This is a stricter definition than required.
// We could in principle choose that changing the name (but not position or
// type) of a variant is compatible. The VM does not care about the name of a
// variant if it's non-public (it's purely informational), but clients
// presumably would.
datatype_layout = false;
context.enum_variant_mismatch(name, old_enum, new_enum, tag);
}
if new_variant.fields != old_variant.fields {
// Fields changed. Code in this module will fail at runtime if it tries to
Expand All @@ -188,7 +203,7 @@ impl Compatibility {
// choose that changing the name (but not position or type) of a field is
// compatible. The VM does not care about the name of a field
// (it's purely informational), but clients presumably do.
datatype_layout = false
context.enum_variant_mismatch(name, old_enum, new_enum, tag);
}
}
}
Expand All @@ -210,35 +225,38 @@ impl Compatibility {
for (name, old_func) in &old_module.functions {
let Some(new_func) = new_module.functions.get(name) else {
if old_func.visibility == Visibility::Friend {
friend_linking = false;
context.function_missing_friend(name, old_func);
} else if old_func.visibility != Visibility::Private {
datatype_and_function_linking = false;
context.function_missing_public(name, old_func);
} else if old_func.is_entry && self.check_private_entry_linking {
// This must be a private entry function. So set the link breakage if we're
// checking for that.
entry_linking = false;
context.function_missing_entry(name, old_func);
}
continue;
};

// Check visibility compatibility
match (old_func.visibility, new_func.visibility) {
(Visibility::Public, Visibility::Private | Visibility::Friend) => {
datatype_and_function_linking = false
context.function_lost_public_visibility(name, old_func);
}
(Visibility::Friend, Visibility::Private) => {
context.function_lost_friend_visibility(name, old_func);
}
(Visibility::Friend, Visibility::Private) => friend_linking = false,
_ => (),
}

// Check entry compatibility
#[allow(clippy::if_same_then_else)]
if old_module.file_format_version < VERSION_5
&& new_module.file_format_version < VERSION_5
&& old_func.visibility != Visibility::Private
&& old_func.is_entry != new_func.is_entry
{
entry_linking = false
context.function_entry_compatibility(name, old_func, new_func);
} else if old_func.is_entry && !new_func.is_entry {
entry_linking = false;
context.function_entry_compatibility(name, old_func, new_func);
}

// Check signature compatibility
Expand All @@ -249,15 +267,7 @@ impl Compatibility {
&new_func.type_parameters,
)
{
match old_func.visibility {
Visibility::Friend => friend_linking = false,
Visibility::Public => datatype_and_function_linking = false,
Visibility::Private => (),
}

if old_func.is_entry {
entry_linking = false;
}
context.function_signature_mismatch(name, old_func, new_func);
}
}

Expand All @@ -269,35 +279,10 @@ impl Compatibility {
let old_friend_module_ids: BTreeSet<_> = old_module.friends.iter().cloned().collect();
let new_friend_module_ids: BTreeSet<_> = new_module.friends.iter().cloned().collect();
if !old_friend_module_ids.is_subset(&new_friend_module_ids) {
friend_linking = false;
context.friend_module_missing(old_friend_module_ids, new_friend_module_ids);
}

if self.check_datatype_and_pub_function_linking && !datatype_and_function_linking {
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
}
if self.check_datatype_layout && !datatype_layout {
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
}
if self.check_friend_linking && !friend_linking {
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
}
if self.check_private_entry_linking && !entry_linking {
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
}
if self.disallow_new_variants && !no_new_variants {
return Err(PartialVMError::new(
StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE,
));
}
Ok(())
context.finish(self)
}
}

Expand Down
Loading

0 comments on commit 34c9e3e

Please sign in to comment.