From 34c9e3ef6fff67df7f6ec89b4ccee462fb111df2 Mon Sep 17 00:00:00 2001 From: Jort Date: Wed, 25 Sep 2024 11:16:36 -0700 Subject: [PATCH] add compatability trait to move-binary-format (#19450) ## 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: --- .../move-binary-format/src/compatibility.rs | 123 ++++--- .../src/compatibility_mode.rs | 302 ++++++++++++++++++ .../move/crates/move-binary-format/src/lib.rs | 1 + 3 files changed, 357 insertions(+), 69 deletions(-) create mode 100644 external-crates/move/crates/move-binary-format/src/compatibility_mode.rs diff --git a/external-crates/move/crates/move-binary-format/src/compatibility.rs b/external-crates/move/crates/move-binary-format/src/compatibility.rs index 4ab2aa043d0f3..aab2606ca936e 100644 --- a/external-crates/move/crates/move-binary-format/src/compatibility.rs +++ b/external-crates/move/crates/move-binary-format/src/compatibility.rs @@ -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, @@ -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::(old_module, new_module) + .map_err(|_| PartialVMError::new(StatusCode::BACKWARD_INCOMPATIBLE_MODULE_UPDATE)) + } + + pub fn check_with_mode( + &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 @@ -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 @@ -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); } } @@ -141,37 +156,37 @@ 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. @@ -179,7 +194,7 @@ impl Compatibility { // 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 @@ -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); } } } @@ -210,13 +225,13 @@ 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; }; @@ -224,21 +239,24 @@ impl Compatibility { // 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 @@ -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); } } @@ -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) } } diff --git a/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs b/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs new file mode 100644 index 0000000000000..aed0eb81546fa --- /dev/null +++ b/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs @@ -0,0 +1,302 @@ +use crate::compatibility::Compatibility; +use crate::file_format::Visibility; +use crate::normalized::{Enum, Function, Struct}; +use move_core_types::account_address::AccountAddress; +use move_core_types::identifier::{IdentStr, Identifier}; +use move_core_types::language_storage::ModuleId; +use std::collections::BTreeSet; + +/// A trait which will allow accumulating the information necessary for checking upgrade compatibility between two modules, +/// while allowing flexibility in the error type that is returned. +/// Gathers the errors and accumulates them into a single error. +/// The [`Compatibility`] struct's flags are used to determine the compatibility checks that are needed. +pub trait CompatibilityMode: Default { + /// The error type that will be returned when [`CompatibilityMode::finish`] is called, returning the accumulated result. + type Error; + + /// The module id mismatch error occurs when the module id of the old and new modules do not match. + fn module_id_mismatch( + &mut self, + old_addr: &AccountAddress, + old_name: &IdentStr, + new_addr: &AccountAddress, + new_name: &IdentStr, + ); + + /// The struct missing error occurs when a struct is present in the old module but not in the new module. + fn struct_missing(&mut self, name: &Identifier, old_struct: &Struct); + + /// The struct ability mismatch error occurs when the abilities of a struct are outside of the + /// allowed new abilities. Adding an ability is fine as long as it's not in the disallowed_new_abilities set. + fn struct_ability_mismatch( + &mut self, + name: &Identifier, + old_struct: &Struct, + new_struct: &Struct, + ); + + /// Struct type parameters mismatch error occurs when the type parameters of a struct are not the same. + fn struct_type_param_mismatch( + &mut self, + name: &Identifier, + old_struct: &Struct, + new_struct: &Struct, + ); + + /// Struct field mismatch error occurs when the fields of a struct are not the same. + fn struct_field_mismatch( + &mut self, + name: &Identifier, + old_struct: &Struct, + new_struct: &Struct, + ); + + /// Enum missing error occurs when an enum is present in the old module but not in the new module. + fn enum_missing(&mut self, name: &Identifier, old_enum: &Enum); + + /// Enum ability mismatch error occurs when the abilities of an enum are outside of the + /// allowed new abilities. Adding an ability is fine as long as it's not in the disallowed_new_abilities set. + fn enum_ability_mismatch(&mut self, name: &Identifier, old_enum: &Enum, new_enum: &Enum); + + /// Enum type parameters mismatch error occurs when the type parameters of an enum are not the same. + fn enum_type_param_mismatch(&mut self, name: &Identifier, old_enum: &Enum, new_enum: &Enum); + + /// Enum new variant error occurs when a new variant is added to an enum. + fn enum_new_variant(&mut self, name: &Identifier, old_enum: &Enum, new_enum: &Enum); + + /// Enum variant missing error occurs when a variant is present in the old enum but not in the new enum. + fn enum_variant_missing(&mut self, name: &Identifier, old_enum: &Enum, tag: usize); + + /// Enum variant mismatch error occurs when a variant is present in the old enum but not in the new enum. + fn enum_variant_mismatch( + &mut self, + name: &Identifier, + old_enum: &Enum, + new_enum: &Enum, + tag: usize, + ); + + /// Function missing public error occurs when a public function is present in the old module but not in the new module. + fn function_missing_public(&mut self, name: &Identifier, old_func: &Function); + + /// Function missing friend error occurs when a friend function is present in the old module but not in the new module. + fn function_missing_friend(&mut self, name: &Identifier, old_funcs: &Function); + + /// Function missing entry error occurs when an entry function is present in the old module but not in the new module. + fn function_missing_entry(&mut self, name: &Identifier, old_func: &Function); + + /// Function signature mismatch error occurs when the signature of a function changes. + fn function_signature_mismatch( + &mut self, + name: &Identifier, + old_func: &Function, + new_func: &Function, + ); + + /// Function lost public visibility error occurs when a function loses its public visibility. + fn function_lost_public_visibility(&mut self, name: &Identifier, old_func: &Function); + + /// Function lost friend visibility error occurs when a function loses its friend visibility. + fn function_lost_friend_visibility(&mut self, name: &Identifier, old_func: &Function); + + /// Function entry compatibility error occurs when an entry function is not compatible. + fn function_entry_compatibility( + &mut self, + name: &Identifier, + old_func: &Function, + new_func: &Function, + ); + + /// Friend module missing error occurs when a friend module is present in the old module but not in the new module. + /// This is a check that is done for `module.friends` old and new id lists. + fn friend_module_missing( + &mut self, + old_friend_module_ids: BTreeSet, + new_friend_module_ids: BTreeSet, + ); + + /// Finish the compatibility check and return the error if one has been accumulated from individual errors. + fn finish(&self, _: &Compatibility) -> Result<(), Self::Error>; +} + +/// Compatibility mode impl for execution compatibility checks. +/// These flags are set when a type safety check is violated. see [`Compatibility`] for more information. +pub struct ExecutionCompatibilityMode { + datatype_and_function_linking: bool, + datatype_layout: bool, + friend_linking: bool, + entry_linking: bool, + no_new_variants: bool, +} + +impl Default for ExecutionCompatibilityMode { + fn default() -> Self { + Self { + datatype_and_function_linking: true, + datatype_layout: true, + friend_linking: true, + entry_linking: true, + no_new_variants: true, + } + } +} + +impl CompatibilityMode for ExecutionCompatibilityMode { + /// Unit error type for execution compatibility mode. + /// We only need to know if an error has occurred. + type Error = (); + + fn module_id_mismatch( + &mut self, + _old_addr: &AccountAddress, + _old_name: &IdentStr, + _new_addr: &AccountAddress, + _new_name: &IdentStr, + ) { + self.datatype_and_function_linking = false; + } + + fn struct_missing(&mut self, _name: &Identifier, _old_struct: &Struct) { + self.datatype_and_function_linking = false; + self.datatype_layout = false; + } + + fn struct_ability_mismatch( + &mut self, + _name: &Identifier, + _old_struct: &Struct, + _new_struct: &Struct, + ) { + self.datatype_and_function_linking = false; + } + + fn struct_type_param_mismatch( + &mut self, + _name: &Identifier, + _old_struct: &Struct, + _new_struct: &Struct, + ) { + self.datatype_and_function_linking = false; + } + + fn struct_field_mismatch( + &mut self, + _name: &Identifier, + _old_struct: &Struct, + _new_struct: &Struct, + ) { + self.datatype_layout = false; + } + + fn enum_missing(&mut self, _name: &Identifier, _old_enum: &Enum) { + self.datatype_and_function_linking = false; + self.datatype_layout = false; + } + + fn enum_ability_mismatch(&mut self, _name: &Identifier, _old_enum: &Enum, _new_enum: &Enum) { + self.datatype_and_function_linking = false; + } + + fn enum_type_param_mismatch(&mut self, _name: &Identifier, _old_enum: &Enum, _new_enum: &Enum) { + self.datatype_and_function_linking = false; + } + + fn enum_new_variant(&mut self, _name: &Identifier, _old_enum: &Enum, _new_enum: &Enum) { + self.no_new_variants = false; + } + + fn enum_variant_missing(&mut self, _name: &Identifier, _old_enum: &Enum, _tag: usize) { + self.datatype_layout = false; + } + + fn enum_variant_mismatch( + &mut self, + _name: &Identifier, + _old_enum: &Enum, + _new_enum: &Enum, + _tag: usize, + ) { + self.datatype_layout = false; + } + + fn function_missing_public(&mut self, _name: &Identifier, _old_func: &Function) { + self.datatype_and_function_linking = false; + } + + fn function_missing_friend(&mut self, _name: &Identifier, _old_func: &Function) { + self.friend_linking = false; + } + + fn function_missing_entry(&mut self, _name: &Identifier, _old_func: &Function) { + self.entry_linking = false; + } + + fn function_signature_mismatch( + &mut self, + _name: &Identifier, + old_func: &Function, + _new_func: &Function, + ) { + match old_func.visibility { + Visibility::Friend => { + self.friend_linking = false; + } + Visibility::Public => { + self.datatype_and_function_linking = false; + } + Visibility::Private => (), + } + + if old_func.is_entry { + self.entry_linking = false; + } + } + + fn function_lost_public_visibility(&mut self, _name: &Identifier, _old_func: &Function) { + self.datatype_and_function_linking = false; + } + + fn function_lost_friend_visibility(&mut self, _name: &Identifier, _old_func: &Function) { + self.friend_linking = false; + } + + fn function_entry_compatibility( + &mut self, + _name: &Identifier, + _old_func: &Function, + _new_func: &Function, + ) { + self.entry_linking = false; + } + + fn friend_module_missing( + &mut self, + _old_friend_module_ids: BTreeSet, + _new_friend_module_ids: BTreeSet, + ) { + self.friend_linking = false; + } + + /// Finish by comparing against the compatibility flags. + fn finish(&self, compatability: &Compatibility) -> Result<(), ()> { + if compatability.check_datatype_and_pub_function_linking + && !self.datatype_and_function_linking + { + return Err(()); + } + if compatability.check_datatype_layout && !self.datatype_layout { + return Err(()); + } + if compatability.check_friend_linking && !self.friend_linking { + return Err(()); + } + if compatability.check_private_entry_linking && !self.entry_linking { + return Err(()); + } + if compatability.disallow_new_variants && !self.no_new_variants { + return Err(()); + } + + Ok(()) + } +} diff --git a/external-crates/move/crates/move-binary-format/src/lib.rs b/external-crates/move/crates/move-binary-format/src/lib.rs index 78e3607f7dfc8..772c396ce14c4 100644 --- a/external-crates/move/crates/move-binary-format/src/lib.rs +++ b/external-crates/move/crates/move-binary-format/src/lib.rs @@ -9,6 +9,7 @@ use std::fmt; pub mod binary_config; pub mod check_bounds; pub mod compatibility; +pub mod compatibility_mode; #[macro_use] pub mod errors; pub mod constant;