[loader-v2] Fixing global cache reads & read-before-write on publish (#15285) (#15298)

api/src/transactions.rs

@@ -25,8 +25,7 @@ use aptos_api_types::{
     AsConverter, EncodeSubmissionRequest, GasEstimation, GasEstimationBcs, HashValue,
     HexEncodedBytes, LedgerInfo, MoveType, PendingTransaction, SubmitTransactionRequest,
     Transaction, TransactionData, TransactionOnChainData, TransactionsBatchSingleSubmissionFailure,
-    TransactionsBatchSubmissionResult, UserTransaction, VerifyInput, VerifyInputWithRecursion,
-    MAX_RECURSIVE_TYPES_ALLOWED, U64,
+    TransactionsBatchSubmissionResult, UserTransaction, VerifyInput, VerifyInputWithRecursion, U64,
 };
 use aptos_crypto::{hash::CryptoHash, signing_message};
 use aptos_types::{
@@ -1041,10 +1040,12 @@ impl TransactionsApi {
         ledger_info: &LedgerInfo,
         data: SubmitTransactionPost,
     ) -> Result<SignedTransaction, SubmitTransactionError> {
+        pub const MAX_SIGNED_TRANSACTION_DEPTH: usize = 16;
+
         match data {
             SubmitTransactionPost::Bcs(data) => {
                 let signed_transaction: SignedTransaction =
-                    bcs::from_bytes_with_limit(&data.0, MAX_RECURSIVE_TYPES_ALLOWED as usize)
+                    bcs::from_bytes_with_limit(&data.0, MAX_SIGNED_TRANSACTION_DEPTH)
                         .context("Failed to deserialize input into SignedTransaction")
                         .map_err(|err| {
                             SubmitTransactionError::bad_request_with_code(

api/types/src/move_types.rs

@@ -521,11 +521,9 @@ pub enum MoveType {
     Unparsable(String),
 }
 
-/// Maximum number of recursive types
-/// Currently, this is allowed up to the serde limit of 16
-///
-/// TODO: Should this number be re-evaluated
-pub const MAX_RECURSIVE_TYPES_ALLOWED: u8 = 16;
+/// Maximum number of recursive types - Same as (non-public)
+/// move_core_types::safe_serialize::MAX_TYPE_TAG_NESTING
+pub const MAX_RECURSIVE_TYPES_ALLOWED: u8 = 8;
 
 impl VerifyInputWithRecursion for MoveType {
     fn verify(&self, recursion_count: u8) -> anyhow::Result<()> {

aptos-move/aptos-debugger/src/aptos_debugger.rs

@@ -11,7 +11,7 @@ use aptos_types::{
     account_address::AccountAddress,
     block_executor::{
         config::{BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig},
-        execution_state::TransactionSliceMetadata,
+        transaction_slice_metadata::TransactionSliceMetadata,
     },
     contract_event::ContractEvent,
     state_store::TStateView,

aptos-move/aptos-release-builder/data/release.yaml

@@ -1,19 +1,16 @@
 ---
-remote_endpoint: https://fullnode.mainnet.aptoslabs.com
-name: "TBD"
+remote_endpoint: ~
+name: "v1.24.0"
 proposals:
   - name: proposal_1_upgrade_framework
     metadata:
-      title: "Multi-step proposal to upgrade mainnet framework, version TBD"
-      description: "This includes changes in (TBA: URL to changes)"
+      title: "Multi-step proposal to upgrade mainnet framework, version v1.24.0"
+      description: "This includes changes in https://github.com/aptos-labs/aptos-core/releases/tag/aptos-node-v1.24.0-rc"
     execution_mode: MultiStep
     update_sequence:
       - Gas:
-          new: current
+          old: https://raw.githubusercontent.com/aptos-labs/aptos-networks/main/gas/v1.23.1-rc.json
+          new: https://raw.githubusercontent.com/aptos-labs/aptos-networks/main/gas/v1.24.0.json
       - Framework:
-          bytecode_version: 6
+          bytecode_version: 7
           git_hash: ~
-      - FeatureFlag:
-          enabled:
-            - allow_serialized_script_args
-

aptos-move/aptos-transaction-benchmarks/src/transaction_bench_state.rs

@@ -18,8 +18,8 @@ use aptos_language_e2e_tests::{
 use aptos_types::{
     block_executor::{
         config::{BlockExecutorConfig, BlockExecutorConfigFromOnchain},
-        execution_state::TransactionSliceMetadata,
         partitioner::PartitionedTransactions,
+        transaction_slice_metadata::TransactionSliceMetadata,
     },
     block_metadata::BlockMetadata,
     on_chain_config::{OnChainConfig, ValidatorSet},

aptos-move/aptos-vm-environment/Cargo.toml

@@ -31,4 +31,4 @@ once_cell = { workspace = true }
 sha3 = { workspace = true }
 
 [dev-dependencies]
-aptos-language-e2e-tests = { workspace = true }
+aptos-types = { workspace = true, features = ["testing"] }

aptos-move/aptos-vm-environment/src/environment.rs

@@ -181,12 +181,12 @@ impl Environment {
         // If no chain ID is in storage, we assume we are in a testing environment.
         let chain_id = fetch_config_and_update_hash::<ChainId>(&mut sha3_256, state_view)
             .unwrap_or_else(ChainId::test);
-        let timestamp =
+        let timestamp_micros =
             fetch_config_and_update_hash::<ConfigurationResource>(&mut sha3_256, state_view)
-                .map(|config| config.last_reconfiguration_time())
+                .map(|config| config.last_reconfiguration_time_micros())
                 .unwrap_or(0);
 
-        let mut timed_features_builder = TimedFeaturesBuilder::new(chain_id, timestamp);
+        let mut timed_features_builder = TimedFeaturesBuilder::new(chain_id, timestamp_micros);
         if let Some(profile) = get_timed_feature_override() {
             // We need to ensure the override is taken into account for the hash.
             let profile_bytes = bcs::to_bytes(&profile)
@@ -272,12 +272,12 @@ fn fetch_config_and_update_hash<T: OnChainConfig>(
 #[cfg(test)]
 pub mod test {
     use super::*;
-    use aptos_language_e2e_tests::data_store::FakeDataStore;
+    use aptos_types::state_store::MockStateView;
 
     #[test]
     fn test_new_environment() {
         // This creates an empty state.
-        let state_view = FakeDataStore::default();
+        let state_view = MockStateView::empty();
         let env = Environment::new(&state_view, false, None);
 
         // Check default values.

aptos-move/aptos-vm-types/src/resolver.rs

@@ -176,8 +176,13 @@ pub trait TModuleView {
 
 /// Allows to query state information, e.g. its usage.
 pub trait StateStorageView {
+    type Key;
+
     fn id(&self) -> StateViewId;
 
+    /// Reads the state value from the DB. Used to enforce read-before-write for module writes.
+    fn read_state_value(&self, state_key: &Self::Key) -> Result<(), StateviewError>;
+
     fn get_usage(&self) -> Result<StateStorageUsage, StateviewError>;
 }
 
@@ -203,7 +208,7 @@ pub trait TExecutorView<K, T, L, I, V>:
     + TModuleView<Key = K>
     + TAggregatorV1View<Identifier = K>
     + TDelayedFieldView<Identifier = I, ResourceKey = K, ResourceGroupTag = T>
-    + StateStorageView
+    + StateStorageView<Key = K>
 {
 }
 
@@ -212,7 +217,7 @@ impl<A, K, T, L, I, V> TExecutorView<K, T, L, I, V> for A where
         + TModuleView<Key = K>
         + TAggregatorV1View<Identifier = K>
         + TDelayedFieldView<Identifier = I, ResourceKey = K, ResourceGroupTag = T>
-        + StateStorageView
+        + StateStorageView<Key = K>
 {
 }
 
@@ -278,10 +283,17 @@ impl<S> StateStorageView for S
 where
     S: StateView,
 {
+    type Key = StateKey;
+
     fn id(&self) -> StateViewId {
         self.id()
     }
 
+    fn read_state_value(&self, state_key: &Self::Key) -> Result<(), StateviewError> {
+        self.get_state_value(state_key)?;
+        Ok(())
+    }
+
     fn get_usage(&self) -> Result<StateStorageUsage, StateviewError> {
         self.get_usage().map_err(Into::into)
     }

aptos-move/aptos-vm/src/aptos_vm.rs

@@ -49,8 +49,8 @@ use aptos_types::{
             BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig,
             BlockExecutorModuleCacheLocalConfig,
         },
-        execution_state::TransactionSliceMetadata,
         partitioner::PartitionedTransactions,
+        transaction_slice_metadata::TransactionSliceMetadata,
     },
     block_metadata::BlockMetadata,
     block_metadata_ext::{BlockMetadataExt, BlockMetadataWithRandomness},
@@ -1652,7 +1652,12 @@ impl AptosVM {
         let check_friend_linking = !self
             .features()
             .is_enabled(FeatureFlag::TREAT_FRIEND_AS_PRIVATE);
-        let compatability_checks = Compatibility::new(check_struct_layout, check_friend_linking);
+        let compatibility_checks = Compatibility::new(
+            check_struct_layout,
+            check_friend_linking,
+            self.timed_features()
+                .is_enabled(TimedFeatureFlag::EntryCompatibility),
+        );
 
         if self.features().is_loader_v2_enabled() {
             session.finish_with_module_publishing_and_initialization(
@@ -1665,7 +1670,7 @@ impl AptosVM {
                 destination,
                 bundle,
                 modules,
-                compatability_checks,
+                compatibility_checks,
             )
         } else {
             // Check what modules exist before publishing.
@@ -1689,7 +1694,7 @@ impl AptosVM {
                     bundle.into_inner(),
                     destination,
                     gas_meter,
-                    compatability_checks,
+                    compatibility_checks,
                 )
             })?;
 

aptos-move/aptos-vm/src/block_executor/mod.rs

@@ -18,7 +18,9 @@ use aptos_block_executor::{
 };
 use aptos_infallible::Mutex;
 use aptos_types::{
-    block_executor::{config::BlockExecutorConfig, execution_state::TransactionSliceMetadata},
+    block_executor::{
+        config::BlockExecutorConfig, transaction_slice_metadata::TransactionSliceMetadata,
+    },
     contract_event::ContractEvent,
     error::PanicError,
     executable::ExecutableTestType,

aptos-move/aptos-vm/src/data_cache.rs

@@ -312,10 +312,16 @@ impl<S: StateView> AsMoveResolver<S> for S {
 }
 
 impl<'e, E: ExecutorView> StateStorageView for StorageAdapter<'e, E> {
+    type Key = StateKey;
+
     fn id(&self) -> StateViewId {
         self.executor_view.id()
     }
 
+    fn read_state_value(&self, state_key: &Self::Key) -> Result<(), StateviewError> {
+        self.executor_view.read_state_value(state_key)
+    }
+
     fn get_usage(&self) -> Result<StateStorageUsage, StateviewError> {
         self.executor_view.get_usage()
     }

aptos-move/aptos-vm/src/lib.rs

@@ -128,8 +128,8 @@ pub use crate::aptos_vm::{AptosSimulationVM, AptosVM};
 use crate::sharded_block_executor::{executor_client::ExecutorClient, ShardedBlockExecutor};
 use aptos_types::{
     block_executor::{
-        config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata,
-        partitioner::PartitionedTransactions,
+        config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions,
+        transaction_slice_metadata::TransactionSliceMetadata,
     },
     state_store::StateView,
     transaction::{

aptos-move/aptos-vm/src/move_vm_ext/resolver.rs

@@ -23,7 +23,7 @@ pub trait AptosMoveResolver:
     + ModuleResolver
     + ResourceResolver
     + ResourceGroupResolver
-    + StateStorageView
+    + StateStorageView<Key = StateKey>
     + TableResolver
     + AsExecutorView
     + AsResourceGroupView

aptos-move/aptos-vm/src/move_vm_ext/session/view_with_change_set.rs

@@ -318,10 +318,16 @@ impl<'r> TModuleView for ExecutorViewWithChangeSet<'r> {
 }
 
 impl<'r> StateStorageView for ExecutorViewWithChangeSet<'r> {
+    type Key = StateKey;
+
     fn id(&self) -> StateViewId {
         self.base_executor_view.id()
     }
 
+    fn read_state_value(&self, state_key: &Self::Key) -> Result<(), StateviewError> {
+        self.base_executor_view.read_state_value(state_key)
+    }
+
     fn get_usage(&self) -> Result<StateStorageUsage, StateviewError> {
         Err(StateviewError::Other(
             "Unexpected access to get_usage()".to_string(),

aptos-move/aptos-vm/src/move_vm_ext/write_op_converter.rs

@@ -105,6 +105,22 @@ impl<'r> WriteOpConverter<'r> {
             )?;
 
             let state_key = StateKey::module_id(&module_id);
+
+            // Enforce read-before-write:
+            //   Modules can live in global cache, and so the DB may not see a module read even
+            //   when it gets republished. This violates read-before-write property. Here, we on
+            //   purpose enforce this by registering a read to the DB directly.
+            //   Note that we also do it here so that in case of storage errors, only a  single
+            //   transaction fails (e.g., if doing this read before commit in block executor we
+            //   have no way to alter the transaction outputs at that point).
+            self.remote.read_state_value(&state_key).map_err(|err| {
+                let msg = format!(
+                    "Error when enforcing read-before-write for module {}::{}: {:?}",
+                    addr, name, err
+                );
+                PartialVMError::new(StatusCode::STORAGE_ERROR).with_message(msg)
+            })?;
+
             writes.insert(state_key, ModuleWrite::new(module_id, write_op));
         }
         Ok(writes)

aptos-move/aptos-vm/src/sharded_block_executor/sharded_executor_service.rs

@@ -21,8 +21,8 @@ use aptos_logger::{info, trace};
 use aptos_types::{
     block_executor::{
         config::{BlockExecutorConfig, BlockExecutorLocalConfig},
-        execution_state::TransactionSliceMetadata,
         partitioner::{ShardId, SubBlock, SubBlocksForShard, TransactionWithDependencies},
+        transaction_slice_metadata::TransactionSliceMetadata,
     },
     state_store::StateView,
     transaction::{