From 6b8ad6c7341dd53635e4773f0f85bc0b6197b0d9 Mon Sep 17 00:00:00 2001 From: Igor Date: Tue, 15 Oct 2024 11:32:25 -0700 Subject: [PATCH] unit test and mark dirty --- .../src/components/feature_flags.rs | 3 + .../framework/move-stdlib/src/natives/mem.rs | 17 +++ .../move-vm/types/src/values/value_tests.rs | 100 ++++++++++++++++++ .../move-vm/types/src/values/values_impl.rs | 3 + types/src/on_chain_config/aptos_features.rs | 7 ++ 5 files changed, 130 insertions(+) diff --git a/aptos-move/aptos-release-builder/src/components/feature_flags.rs b/aptos-move/aptos-release-builder/src/components/feature_flags.rs index 130ec720f1660..71db5b69c5f69 100644 --- a/aptos-move/aptos-release-builder/src/components/feature_flags.rs +++ b/aptos-move/aptos-release-builder/src/components/feature_flags.rs @@ -131,6 +131,7 @@ pub enum FeatureFlag { FederatedKeyless, TransactionSimulationEnhancement, CollectionOwner, + NativeMemoryOperations, } fn generate_features_blob(writer: &CodeWriter, data: &[u64]) { @@ -347,6 +348,7 @@ impl From for AptosFeatureFlag { AptosFeatureFlag::TRANSACTION_SIMULATION_ENHANCEMENT }, FeatureFlag::CollectionOwner => AptosFeatureFlag::COLLECTION_OWNER, + FeatureFlag::NativeMemoryOperations => AptosFeatureFlag::NATIVE_MEMORY_OPERATIONS, } } } @@ -490,6 +492,7 @@ impl From for FeatureFlag { FeatureFlag::TransactionSimulationEnhancement }, AptosFeatureFlag::COLLECTION_OWNER => FeatureFlag::CollectionOwner, + AptosFeatureFlag::NATIVE_MEMORY_OPERATIONS => FeatureFlag::NativeMemoryOperations, } } } diff --git a/aptos-move/framework/move-stdlib/src/natives/mem.rs b/aptos-move/framework/move-stdlib/src/natives/mem.rs index 67233cc9c4595..f997fec95749c 100644 --- a/aptos-move/framework/move-stdlib/src/natives/mem.rs +++ b/aptos-move/framework/move-stdlib/src/natives/mem.rs @@ -22,6 +22,16 @@ use move_vm_types::{ use smallvec::{smallvec, SmallVec}; use std::collections::VecDeque; +/// The feature is not enabled. +/// (0xD is unavailable) +pub const EFEATURE_NOT_ENABLED: u64 = 0x0D_0001; + +pub fn get_feature_not_available_error() -> SafeNativeError { + SafeNativeError::Abort { + abort_code: EFEATURE_NOT_ENABLED, + } +} + /*************************************************************************************************** * native fun native_swap * @@ -33,6 +43,13 @@ fn native_swap( _ty_args: Vec, mut args: VecDeque, ) -> SafeNativeResult> { + if !context + .get_feature_flags() + .is_native_memory_operations_enabled() + { + return Err(get_feature_not_available_error()); + } + debug_assert!(args.len() == 2); if args.len() != 2 { diff --git a/third_party/move/move-vm/types/src/values/value_tests.rs b/third_party/move/move-vm/types/src/values/value_tests.rs index 4e939be5c7a23..14de4ab3b5123 100644 --- a/third_party/move/move-vm/types/src/values/value_tests.rs +++ b/third_party/move/move-vm/types/src/values/value_tests.rs @@ -3,6 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::{loaded_data::runtime_types::TypeBuilder, values::*, views::*}; +use claims::{assert_err, assert_ok}; use move_binary_format::errors::*; use move_core_types::{account_address::AccountAddress, u256::U256}; @@ -229,6 +230,105 @@ fn test_vm_value_vector_u64_casting() { ); } +#[test] +fn test_mem_swap() -> PartialVMResult<()> { + let mut locals = Locals::new(20); + // IndexedRef(Locals) + locals.store_loc(0, Value::u64(0), false)?; + locals.store_loc(1, Value::u64(1), false)?; + locals.store_loc(2, Value::address(AccountAddress::ZERO), false)?; + locals.store_loc(3, Value::address(AccountAddress::ONE), false)?; + + // ContainerRef + + // - Specialized + locals.store_loc(4, Value::vector_u64(vec![1, 2]), false)?; + locals.store_loc(5, Value::vector_u64(vec![3, 4, 5]), false)?; + locals.store_loc(6, Value::vector_address(vec![AccountAddress::ZERO]), false)?; + locals.store_loc(7, Value::vector_address(vec![AccountAddress::ONE]), false)?; + + // - Generic + // -- Container of container + locals.store_loc(8, Value::struct_(Struct::pack(vec![Value::u16(4)])), false)?; + locals.store_loc(9, Value::struct_(Struct::pack(vec![Value::u16(5)])), false)?; + locals.store_loc(10, Value::signer(AccountAddress::ZERO), false)?; + locals.store_loc(11, Value::signer(AccountAddress::ONE), false)?; + + // -- Container of vector + locals.store_loc( + 12, + Value::vector_for_testing_only(vec![Value::u64(1u64), Value::u64(2u64)]), + false, + )?; + locals.store_loc( + 13, + Value::vector_for_testing_only(vec![Value::u64(3u64), Value::u64(4u64)]), + false, + )?; + locals.store_loc( + 14, + Value::vector_for_testing_only(vec![Value::signer(AccountAddress::ZERO)]), + false, + )?; + locals.store_loc( + 15, + Value::vector_for_testing_only(vec![Value::signer(AccountAddress::ONE)]), + false, + )?; + + let mut locals2 = Locals::new(2); + locals2.store_loc(0, Value::u64(0), false)?; + + let get_local = + |ls: &Locals, idx: usize| ls.borrow_loc(idx).unwrap().value_as::().unwrap(); + + for i in (0..16).step_by(2) { + assert_ok!(get_local(&locals, i).swap_values(get_local(&locals, i + 1))); + } + + assert_ok!(get_local(&locals, 0).swap_values(get_local(&locals2, 0))); + + for i in (0..16).step_by(2) { + for j in ((i + 2)..16).step_by(2) { + let result = get_local(&locals, i).swap_values(get_local(&locals, j)); + + // These would all fail in `call_native` typing checks. + // But here some do pass: + if j < 4 // locals are not checked between each other + || (8 <= i && j < 12) // ContainerRef of containers is not checked between each other + || (12 <= i && j < 16) + // ContainerRef of vector is not checked between each other + // || i >= 8 // containers are also interchangeable + { + assert_ok!(result, "{} and {}", i, j); + } else { + assert_err!(result, "{} and {}", i, j); + } + } + } + + // assert_err!(get_local(&locals, 0).swap_values(get_local(&locals, 4))); + // assert_err!(get_local(&locals, 0).swap_values(get_local(&locals, 6))); + // assert_err!(get_local(&locals, 0).swap_values(get_local(&locals, 8))); + // assert_err!(get_local(&locals, 0).swap_values(get_local(&locals, 10))); + // assert_err!(get_local(&locals, 0).swap_values(get_local(&locals, 12))); + // assert_err!(get_local(&locals, 2).swap_values(get_local(&locals, 4))); + // assert_err!(get_local(&locals, 2).swap_values(get_local(&locals, 4))); + // assert_err!(get_local(&locals, 2).swap_values(get_local(&locals, 4))); + // assert_err!(get_local(&locals, 2).swap_values(get_local(&locals, 4))); + + // assert_ok!(get_local(&locals, 0).swap_values(get_local(&locals, 1))); + // assert_ok!(get_local(&locals, 2).swap_values(get_local(&locals, 3))); + // assert_ok!(get_local(&locals, 4).swap_values(get_local(&locals, 5))); + // assert_ok!(get_local(&locals, 6).swap_values(get_local(&locals, 7))); + // assert_ok!(get_local(&locals, 8).swap_values(get_local(&locals, 9))); + // assert_ok!(get_local(&locals, 10).swap_values(get_local(&locals, 11))); + // assert_ok!(get_local(&locals, 12).swap_values(get_local(&locals, 13))); + // assert_ok!(get_local(&locals, 14).swap_values(get_local(&locals, 15))); + + Ok(()) +} + #[cfg(test)] mod native_values { use super::*; diff --git a/third_party/move/move-vm/types/src/values/values_impl.rs b/third_party/move/move-vm/types/src/values/values_impl.rs index 9c35f8040d43e..c1e93129e4ce5 100644 --- a/third_party/move/move-vm/types/src/values/values_impl.rs +++ b/third_party/move/move-vm/types/src/values/values_impl.rs @@ -1099,6 +1099,9 @@ impl IndexedRef { }, } + self.container_ref.mark_dirty(); + other.container_ref.mark_dirty(); + Ok(()) } } diff --git a/types/src/on_chain_config/aptos_features.rs b/types/src/on_chain_config/aptos_features.rs index 7c5f24edd4c36..416b9136fabfb 100644 --- a/types/src/on_chain_config/aptos_features.rs +++ b/types/src/on_chain_config/aptos_features.rs @@ -95,6 +95,8 @@ pub enum FeatureFlag { FEDERATED_KEYLESS = 77, TRANSACTION_SIMULATION_ENHANCEMENT = 78, COLLECTION_OWNER = 79, + /// covers mem::swap and vector::move_range + NATIVE_MEMORY_OPERATIONS = 80, } impl FeatureFlag { @@ -172,6 +174,7 @@ impl FeatureFlag { FeatureFlag::ENABLE_RESOURCE_ACCESS_CONTROL, FeatureFlag::REJECT_UNSTABLE_BYTECODE_FOR_SCRIPT, FeatureFlag::TRANSACTION_SIMULATION_ENHANCEMENT, + FeatureFlag::NATIVE_MEMORY_OPERATIONS, ] } } @@ -317,6 +320,10 @@ impl Features { self.is_enabled(FeatureFlag::TRANSACTION_SIMULATION_ENHANCEMENT) } + pub fn is_native_memory_operations_enabled(&self) -> bool { + self.is_enabled(FeatureFlag::NATIVE_MEMORY_OPERATIONS) + } + pub fn get_max_identifier_size(&self) -> u64 { if self.is_enabled(FeatureFlag::LIMIT_MAX_IDENTIFIER_LENGTH) { IDENTIFIER_SIZE_MAX