From 2932d0f10f371c89074cc1688cc37c97de19e258 Mon Sep 17 00:00:00 2001 From: perryjrandall Date: Thu, 25 May 2023 12:53:59 -0700 Subject: [PATCH 01/16] [release] Turn of qs for release v1.5 (#8369) Test Plan: testing with ci by adding label run-e2e-tests --- .../aptos-release-builder/data/release.yaml | 26 +------------------ types/src/on_chain_config/consensus_config.rs | 2 +- 2 files changed, 2 insertions(+), 26 deletions(-) diff --git a/aptos-move/aptos-release-builder/data/release.yaml b/aptos-move/aptos-release-builder/data/release.yaml index 650000ee6c655..4b2dbd544421b 100644 --- a/aptos-move/aptos-release-builder/data/release.yaml +++ b/aptos-move/aptos-release-builder/data/release.yaml @@ -5,7 +5,7 @@ proposals: - name: upgrade_framework metadata: title: "Multi-step proposal to upgrade mainnet framework to v1.4.0" - description: "This includes changes outlined in https://github.com/aptos-labs/aptos-core/releases/aptos-node-v1.4.0. Struct constructor, generic cryptography algebra, ed25519 returns false if wrong length, quorum store, and transaction shuffling will be enabled in separate proposals." + description: "This includes changes outlined in https://github.com/aptos-labs/aptos-core/releases/aptos-node-v1.4.0. Struct constructor, generic cryptography algebra, ed25519 returns false if wrong length, and transaction shuffling will be enabled in separate proposals." execution_mode: MultiStep update_sequence: - DefaultGas @@ -23,30 +23,6 @@ proposals: enabled: - cryptography_algebra_natives - bls12381_structures - - name: enable_quorum_store - metadata: - title: "Enable Quorum Store" - description: "AIP-26: Quorum Store is a production-optimized implementation of Narwhal [1], that improves consensus throughput." - discussion_url: "https://github.com/aptos-foundation/AIPs/issues/108" - execution_mode: MultiStep - update_sequence: - - Consensus: - V2: - decoupled_execution: true - back_pressure_limit: 10 - exclude_round: 40 - proposer_election_type: - leader_reputation: - proposer_and_voter_v2: - active_weight: 1000 - inactive_weight: 10 - failed_weight: 1 - failure_threshold_percent: 10 - proposer_window_num_validators_multiplier: 10 - voter_window_num_validators_multiplier: 1 - weight_by_voting_power: true - use_history_from_previous_epoch_max_count: 5 - max_failed_authors_to_store: 10 - name: enable_charge_invariant_violation metadata: title: "Enable charge_invariant_violation" diff --git a/types/src/on_chain_config/consensus_config.rs b/types/src/on_chain_config/consensus_config.rs index 4ef79f77197b5..a94e629bf0574 100644 --- a/types/src/on_chain_config/consensus_config.rs +++ b/types/src/on_chain_config/consensus_config.rs @@ -61,7 +61,7 @@ impl OnChainConsensusConfig { /// This is used when on-chain config is not initialized. impl Default for OnChainConsensusConfig { fn default() -> Self { - OnChainConsensusConfig::V2(ConsensusConfigV1::default()) + OnChainConsensusConfig::V1(ConsensusConfigV1::default()) } } From 1512f19f5aeace804b8255137f0b20e68e1ddea1 Mon Sep 17 00:00:00 2001 From: runtianz Date: Thu, 25 May 2023 20:49:59 -0700 Subject: [PATCH 02/16] [aptos-vm] Do not log down storage error (#8383) --- aptos-move/aptos-vm/src/aptos_vm.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index c8608cf46395b..26e7ea225bb1d 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -1672,13 +1672,16 @@ impl VMAdapter for AptosVM { let (vm_status, output) = self.execute_user_transaction(resolver, txn, log_context); if let StatusType::InvariantViolation = vm_status.status_type() { - error!( - *log_context, - "[aptos_vm] Transaction breaking invariant violation. txn: {:?}, status: {:?}", - bcs::to_bytes::(&**txn), - vm_status, - ); - TRANSACTIONS_INVARIANT_VIOLATION.inc(); + // Only log down the non-storage error case as storage error can be resulted from speculative execution. + if vm_status.status_code() != StatusCode::STORAGE_ERROR { + error!( + *log_context, + "[aptos_vm] Transaction breaking invariant violation. txn: {:?}, status: {:?}", + bcs::to_bytes::(&**txn), + vm_status, + ); + TRANSACTIONS_INVARIANT_VIOLATION.inc(); + } } // Increment the counter for user transactions executed. From de3f8643d98051372a7a85130c942ee80af9a453 Mon Sep 17 00:00:00 2001 From: perryjrandall Date: Thu, 25 May 2023 21:50:39 -0700 Subject: [PATCH 03/16] [lto] Use thin LTO instead of default fat (#8384) Fat lto takes forever to compile, thin LTO acheives most of the benefit with little additional compile time Test Plan: perf build scheduled on PR via the build perf images label took 22 mins instead of 42 mins! --- Cargo.toml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b3d4f0cdceaa4..f6ebaf56a9ad5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -627,14 +627,14 @@ inherits = "release" opt-level = 3 debug = true overflow-checks = true -lto = true +lto = "thin" codegen-units = 1 [profile.cli] inherits = "release" debug = false opt-level = "z" -lto = true +lto = "thin" strip = true codegen-units = 1 From 1d5d5df1d333f10e4e658153be910e037b0a07e6 Mon Sep 17 00:00:00 2001 From: Josh Lind Date: Tue, 30 May 2023 11:55:48 -0400 Subject: [PATCH 04/16] [Aptos Node] Bump cargo version of aptos-node to 1.5 --- Cargo.lock | 2 +- aptos-node/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 59260b6b179ab..0adcbf48d8d80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2190,7 +2190,7 @@ dependencies = [ [[package]] name = "aptos-node" -version = "1.4.0" +version = "1.5.0" dependencies = [ "anyhow", "aptos-api", diff --git a/aptos-node/Cargo.toml b/aptos-node/Cargo.toml index 5602a99daf5b0..3700ba1d82de0 100644 --- a/aptos-node/Cargo.toml +++ b/aptos-node/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "aptos-node" description = "Aptos node" -version = "1.4.0" +version = "1.5.0" # Workspace inherited keys authors = { workspace = true } From 1865cc5effc8fda307dd4a182e8158e49154589e Mon Sep 17 00:00:00 2001 From: "Brian (Sunghoon) Cho" Date: Thu, 1 Jun 2023 11:24:15 +0900 Subject: [PATCH 05/16] [Execution] Add TransactionDeduper trait and implementation using (hash, signature) (#8367) (#8458) Cherry-pick #8367. Expected to be a noop until onchain config is changed. ### Description Adding TransactionDeduper trait and an implementation. The dedup is done within a block, just before transaction shuffle. It's guarded by an onchain config. The purpose is to not send duplicate transactions to execution. While execution correctness is not affected by duplicates (and other work removed false error logs that fired due to them), it's possible that duplicate transactions could hurt throughput of parallel execution. By deduping ahead of time, we don't have to worry about that. The implementation finds duplicates by matching (raw txn bcs hash, signature). Because calculating hash can be relatively expensive, it is only done when a shallow match is found of (account, seq_no), and it's done in parallel. Overhead (as seen on forge): * When there are no duplicates, the dedup is negligible -- it takes ~2ms per second. * When there are ~100 duplicates per block, the dedup takes ~10ms per second. ### Test Plan Added unit tests. --- Cargo.lock | 2 + aptos-move/aptos-vm/src/block_executor/mod.rs | 1 + consensus/Cargo.toml | 2 + consensus/src/counters.rs | 22 +- consensus/src/epoch_manager.rs | 4 + .../experimental/ordering_state_computer.rs | 2 + consensus/src/lib.rs | 2 + consensus/src/state_computer.rs | 26 +- consensus/src/state_replication.rs | 2 + .../src/test_utils/mock_state_computer.rs | 4 + consensus/src/transaction_deduper.rs | 33 ++ .../src/txn_hash_and_authenticator_deduper.rs | 362 ++++++++++++++++++ types/src/on_chain_config/execution_config.rs | 36 ++ types/src/on_chain_config/mod.rs | 3 +- types/src/transaction/mod.rs | 8 + 15 files changed, 502 insertions(+), 7 deletions(-) create mode 100644 consensus/src/transaction_deduper.rs create mode 100644 consensus/src/txn_hash_and_authenticator_deduper.rs diff --git a/Cargo.lock b/Cargo.lock index 0adcbf48d8d80..ba6a4ab66ba74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -570,6 +570,7 @@ dependencies = [ "anyhow", "aptos-bitvec", "aptos-bounded-executor", + "aptos-cached-packages", "aptos-channels", "aptos-config", "aptos-consensus-notifications", @@ -618,6 +619,7 @@ dependencies = [ "once_cell", "proptest", "rand 0.7.3", + "rayon", "serde 1.0.149", "serde_bytes", "serde_json", diff --git a/aptos-move/aptos-vm/src/block_executor/mod.rs b/aptos-move/aptos-vm/src/block_executor/mod.rs index 358b58244fbf4..da485cd6b1ff0 100644 --- a/aptos-move/aptos-vm/src/block_executor/mod.rs +++ b/aptos-move/aptos-vm/src/block_executor/mod.rs @@ -147,6 +147,7 @@ impl BlockAptosVM { // Verify the signatures of all the transactions in parallel. // This is time consuming so don't wait and do the checking // sequentially while executing the transactions. + // TODO: state sync runs this code but doesn't need to verify signatures let signature_verification_timer = BLOCK_EXECUTOR_SIGNATURE_VERIFICATION_SECONDS.start_timer(); let signature_verified_block: Vec = diff --git a/consensus/Cargo.toml b/consensus/Cargo.toml index ee58103d1dae1..6c14f6baa1e56 100644 --- a/consensus/Cargo.toml +++ b/consensus/Cargo.toml @@ -60,6 +60,7 @@ num-derive = { workspace = true } num-traits = { workspace = true } once_cell = { workspace = true } rand = { workspace = true } +rayon = { workspace = true } serde = { workspace = true } serde_bytes = { workspace = true } serde_json = { workspace = true } @@ -68,6 +69,7 @@ tokio = { workspace = true } tokio-metrics = { workspace = true } [dev-dependencies] +aptos-cached-packages = { workspace = true } aptos-config = { workspace = true, features = ["fuzzing"] } aptos-consensus-types = { workspace = true, features = ["fuzzing"] } aptos-executor-test-helpers = { workspace = true } diff --git a/consensus/src/counters.rs b/consensus/src/counters.rs index 0aa18617cf7e6..90a6cd0c687d5 100644 --- a/consensus/src/counters.rs +++ b/consensus/src/counters.rs @@ -122,12 +122,32 @@ pub static TXN_SHUFFLE_SECONDS: Lazy = Lazy::new(|| { // metric name "aptos_execution_transaction_shuffle_seconds", // metric description - "The time spent in seconds in initializing the VM in the block executor", + "The time spent in seconds in shuffle of transactions", exponential_buckets(/*start=*/ 1e-6, /*factor=*/ 2.0, /*count=*/ 30).unwrap(), ) .unwrap() }); +/// Transaction dedup call latency +pub static TXN_DEDUP_SECONDS: Lazy = Lazy::new(|| { + register_histogram!( + // metric name + "aptos_execution_transaction_dedup_seconds", + // metric description + "The time spent in seconds in dedup of transaction", + exponential_buckets(/*start=*/ 1e-6, /*factor=*/ 2.0, /*count=*/ 30).unwrap(), + ) + .unwrap() +}); + +/// Transaction dedup number of filtered +pub static TXN_DEDUP_FILTERED: Lazy = Lazy::new(|| { + register_avg_counter( + "aptos_execution_transaction_dedup_filtered", + "The number of duplicates filtered per block", + ) +}); + /// Number of rounds we were collecting votes for proposer /// (similar to PROPOSALS_COUNT, but can be larger, if we failed in creating/sending of the proposal) pub static PROPOSER_COLLECTED_ROUND_COUNT: Lazy = Lazy::new(|| { diff --git a/consensus/src/epoch_manager.rs b/consensus/src/epoch_manager.rs index c90a20aecf711..e97b413283f57 100644 --- a/consensus/src/epoch_manager.rs +++ b/consensus/src/epoch_manager.rs @@ -46,6 +46,7 @@ use crate::{ recovery_manager::RecoveryManager, round_manager::{RoundManager, UnverifiedEvent, VerifiedEvent}, state_replication::StateComputer, + transaction_deduper::create_transaction_deduper, transaction_shuffler::create_transaction_shuffler, util::time_service::TimeService, }; @@ -681,6 +682,8 @@ impl EpochManager { let transaction_shuffler = create_transaction_shuffler(onchain_execution_config.transaction_shuffler_type()); let block_gas_limit = onchain_execution_config.block_gas_limit(); + let transaction_deduper = + create_transaction_deduper(onchain_execution_config.transaction_deduper_type()); self.quorum_store_msg_tx = quorum_store_msg_tx; let payload_client = QuorumStoreClient::new( @@ -694,6 +697,7 @@ impl EpochManager { payload_manager.clone(), transaction_shuffler, block_gas_limit, + transaction_deduper, ); let state_computer = if onchain_consensus_config.decoupled_execution() { Arc::new(self.spawn_decoupled_execution( diff --git a/consensus/src/experimental/ordering_state_computer.rs b/consensus/src/experimental/ordering_state_computer.rs index 7fb5f63e4c50f..a71dbcf5917c8 100644 --- a/consensus/src/experimental/ordering_state_computer.rs +++ b/consensus/src/experimental/ordering_state_computer.rs @@ -10,6 +10,7 @@ use crate::{ }, payload_manager::PayloadManager, state_replication::{StateComputer, StateComputerCommitCallBackType}, + transaction_deduper::TransactionDeduper, transaction_shuffler::TransactionShuffler, }; use anyhow::Result; @@ -126,6 +127,7 @@ impl StateComputer for OrderingStateComputer { _payload_manager: Arc, _: Arc, _: Option, + _: Arc, ) { } diff --git a/consensus/src/lib.rs b/consensus/src/lib.rs index 455bb42c3e3fb..88e735374cb09 100644 --- a/consensus/src/lib.rs +++ b/consensus/src/lib.rs @@ -49,7 +49,9 @@ pub mod counters; pub mod network_interface; mod payload_manager; mod sender_aware_shuffler; +mod transaction_deduper; mod transaction_shuffler; +mod txn_hash_and_authenticator_deduper; use aptos_metrics_core::IntGauge; pub use consensusdb::create_checkpoint; diff --git a/consensus/src/state_computer.rs b/consensus/src/state_computer.rs index 1a8028651cf43..eab828da754d4 100644 --- a/consensus/src/state_computer.rs +++ b/consensus/src/state_computer.rs @@ -9,6 +9,7 @@ use crate::{ monitor, payload_manager::PayloadManager, state_replication::{StateComputer, StateComputerCommitCallBackType}, + transaction_deduper::TransactionDeduper, transaction_shuffler::TransactionShuffler, txn_notifier::TxnNotifier, }; @@ -57,6 +58,7 @@ pub struct ExecutionProxy { write_mutex: AsyncMutex, payload_manager: Mutex>>, transaction_shuffler: Mutex>>, + transaction_deduper: Mutex>>, } impl ExecutionProxy { @@ -90,6 +92,7 @@ impl ExecutionProxy { write_mutex: AsyncMutex::new(LogicalTime::new(0, 0)), payload_manager: Mutex::new(None), transaction_shuffler: Mutex::new(None), + transaction_deduper: Mutex::new(None), } } } @@ -117,10 +120,12 @@ impl StateComputer for ExecutionProxy { ); let payload_manager = self.payload_manager.lock().as_ref().unwrap().clone(); + let txn_deduper = self.transaction_deduper.lock().as_ref().unwrap().clone(); let txn_shuffler = self.transaction_shuffler.lock().as_ref().unwrap().clone(); let txns = payload_manager.get_transactions(block).await?; - let shuffled_txns = txn_shuffler.shuffle(txns); + let deduped_txns = txn_deduper.dedup(txns); + let shuffled_txns = txn_shuffler.shuffle(deduped_txns); let block_gas_limit = self.executor.get_block_gas_limit(); @@ -177,6 +182,7 @@ impl StateComputer for ExecutionProxy { let block_timestamp = finality_proof.commit_info().timestamp_usecs(); let payload_manager = self.payload_manager.lock().as_ref().unwrap().clone(); + let txn_deduper = self.transaction_deduper.lock().as_ref().unwrap().clone(); let txn_shuffler = self.transaction_shuffler.lock().as_ref().unwrap().clone(); let block_gas_limit = self.executor.get_block_gas_limit(); @@ -189,7 +195,8 @@ impl StateComputer for ExecutionProxy { } let signed_txns = payload_manager.get_transactions(block.block()).await?; - let shuffled_txns = txn_shuffler.shuffle(signed_txns); + let deduped_txns = txn_deduper.dedup(signed_txns); + let shuffled_txns = txn_shuffler.shuffle(deduped_txns); txns.extend(block.transactions_to_commit( &self.validators.lock(), @@ -289,6 +296,7 @@ impl StateComputer for ExecutionProxy { payload_manager: Arc, transaction_shuffler: Arc, _block_gas_limit: Option, + transaction_deduper: Arc, ) { *self.validators.lock() = epoch_state .verifier @@ -301,6 +309,7 @@ impl StateComputer for ExecutionProxy { // TODO: Temporarily disable initializing block gas limit and leave it as default None, // until there is a better way to handle the possible panic when executor is initialized. // self.executor.update_block_gas_limit(block_gas_limit); + self.transaction_deduper.lock().replace(transaction_deduper); } // Clears the epoch-specific state. Only a sync_to call is expected before calling new_epoch @@ -313,11 +322,17 @@ impl StateComputer for ExecutionProxy { #[tokio::test] async fn test_commit_sync_race() { - use crate::{error::MempoolError, transaction_shuffler::create_transaction_shuffler}; + use crate::{ + error::MempoolError, transaction_deduper::create_transaction_deduper, + transaction_shuffler::create_transaction_shuffler, + }; use aptos_consensus_notifications::Error; use aptos_types::{ - aggregate_signature::AggregateSignature, block_info::BlockInfo, ledger_info::LedgerInfo, - on_chain_config::TransactionShufflerType, transaction::SignedTransaction, + aggregate_signature::AggregateSignature, + block_info::BlockInfo, + ledger_info::LedgerInfo, + on_chain_config::{TransactionDeduperType, TransactionShufflerType}, + transaction::SignedTransaction, }; struct RecordedCommit { @@ -424,6 +439,7 @@ async fn test_commit_sync_race() { Arc::new(PayloadManager::DirectMempool), create_transaction_shuffler(TransactionShufflerType::NoShuffling), None, + create_transaction_deduper(TransactionDeduperType::NoDedup), ); executor .commit(&[], generate_li(1, 1), callback.clone()) diff --git a/consensus/src/state_replication.rs b/consensus/src/state_replication.rs index 8af7c35fed81d..55ea3400f8576 100644 --- a/consensus/src/state_replication.rs +++ b/consensus/src/state_replication.rs @@ -5,6 +5,7 @@ use crate::{ error::{QuorumStoreError, StateSyncError}, payload_manager::PayloadManager, + transaction_deduper::TransactionDeduper, transaction_shuffler::TransactionShuffler, }; use anyhow::Result; @@ -78,6 +79,7 @@ pub trait StateComputer: Send + Sync { payload_manager: Arc, transaction_shuffler: Arc, block_gas_limit: Option, + transaction_deduper: Arc, ); // Reconfigure to clear epoch state at end of epoch. diff --git a/consensus/src/test_utils/mock_state_computer.rs b/consensus/src/test_utils/mock_state_computer.rs index eaac348fed730..b03e90f60d7b7 100644 --- a/consensus/src/test_utils/mock_state_computer.rs +++ b/consensus/src/test_utils/mock_state_computer.rs @@ -8,6 +8,7 @@ use crate::{ payload_manager::PayloadManager, state_replication::{StateComputer, StateComputerCommitCallBackType}, test_utils::mock_storage::MockStorage, + transaction_deduper::TransactionDeduper, transaction_shuffler::TransactionShuffler, }; use anyhow::{format_err, Result}; @@ -139,6 +140,7 @@ impl StateComputer for MockStateComputer { _: Arc, _: Arc, _: Option, + _: Arc, ) { } @@ -176,6 +178,7 @@ impl StateComputer for EmptyStateComputer { _: Arc, _: Arc, _: Option, + _: Arc, ) { } @@ -237,6 +240,7 @@ impl StateComputer for RandomComputeResultStateComputer { _: Arc, _: Arc, _: Option, + _: Arc, ) { } diff --git a/consensus/src/transaction_deduper.rs b/consensus/src/transaction_deduper.rs new file mode 100644 index 0000000000000..24d76f0dd332f --- /dev/null +++ b/consensus/src/transaction_deduper.rs @@ -0,0 +1,33 @@ +// Copyright © Aptos Foundation +// SPDX-License-Identifier: Apache-2.0 + +use crate::txn_hash_and_authenticator_deduper::TxnHashAndAuthenticatorDeduper; +use aptos_logger::info; +use aptos_types::{on_chain_config::TransactionDeduperType, transaction::SignedTransaction}; +use std::sync::Arc; + +/// Interface to dedup transactions. The dedup filters duplicate transactions within a block. +pub trait TransactionDeduper: Send + Sync { + fn dedup(&self, txns: Vec) -> Vec; +} + +/// No Op Deduper to maintain backward compatibility +struct NoOpDeduper {} + +impl TransactionDeduper for NoOpDeduper { + fn dedup(&self, txns: Vec) -> Vec { + txns + } +} + +pub fn create_transaction_deduper( + deduper_type: TransactionDeduperType, +) -> Arc { + match deduper_type { + TransactionDeduperType::NoDedup => Arc::new(NoOpDeduper {}), + TransactionDeduperType::TxnHashAndAuthenticatorV1 => { + info!("Using simple hash set transaction deduper"); + Arc::new(TxnHashAndAuthenticatorDeduper::new()) + }, + } +} diff --git a/consensus/src/txn_hash_and_authenticator_deduper.rs b/consensus/src/txn_hash_and_authenticator_deduper.rs new file mode 100644 index 0000000000000..38ec5aeef421b --- /dev/null +++ b/consensus/src/txn_hash_and_authenticator_deduper.rs @@ -0,0 +1,362 @@ +// Copyright © Aptos Foundation +// SPDX-License-Identifier: Apache-2.0 + +use crate::{ + counters::{TXN_DEDUP_FILTERED, TXN_DEDUP_SECONDS}, + transaction_deduper::TransactionDeduper, +}; +use aptos_types::transaction::SignedTransaction; +use rayon::prelude::*; +use std::collections::{HashMap, HashSet}; + +/// An implementation of TransactionDeduper. Duplicate filtering is done using the pair +/// (raw_txn.hash(), authenticator). Both the hash and signature are required because dedup +/// happens before signatures are verified and transaction prologue is checked. (So, e.g., a bad +/// transaction could contain a txn and signature that are unrelated.) If the checks are done +/// beforehand only one of the txn hash or signature would be required. +/// +/// The implementation is written to avoid and/or parallelize the most expensive operations. Below +/// are the steps: +/// 1. Mark possible duplicates (sequential): Using a helper HashMap, mark transactions with 2+ +/// (sender, seq_no) pairs as possible duplicates. If no possible duplicates, return the original +/// transactions. +/// 2. Calculate txn hashes (parallel): For all possible duplicates, calculate the txn hash. This +/// is an expensive operation. +/// 3. Filter duplicates (sequential): Using a helper HashSet with the txn hashes calculated above +/// and signatures, filter actual duplicate transactions. +/// +/// Possible future optimizations: +/// a. Note the possible duplicates in Step 1 are independent of each other, so they could be +/// grouped independently and run in parallel in Step 3. +/// b. Txn hashes are calculated at many places within a validator. A per-txn hash cache could speed +/// up dedup or later operations. +/// c. If signature verification is moved to before dedup, then only the signature has to be matched +/// for duplicates and not the hash. +pub(crate) struct TxnHashAndAuthenticatorDeduper {} + +impl TransactionDeduper for TxnHashAndAuthenticatorDeduper { + fn dedup(&self, transactions: Vec) -> Vec { + let _timer = TXN_DEDUP_SECONDS.start_timer(); + let mut seen = HashMap::new(); + let mut is_possible_duplicate = false; + let mut possible_duplicates = vec![false; transactions.len()]; + for (i, txn) in transactions.iter().enumerate() { + match seen.get(&(txn.sender(), txn.sequence_number())) { + None => { + seen.insert((txn.sender(), txn.sequence_number()), i); + }, + Some(first_index) => { + is_possible_duplicate = true; + possible_duplicates[*first_index] = true; + possible_duplicates[i] = true; + }, + } + } + if !is_possible_duplicate { + TXN_DEDUP_FILTERED.observe(0 as f64); + return transactions; + } + + let hash_and_authenticators: Vec<_> = possible_duplicates + .into_par_iter() + .zip(&transactions) + .with_min_len(25) + .map(|(need_hash, txn)| match need_hash { + true => Some((txn.clone().committed_hash(), txn.authenticator())), + false => None, + }) + .collect(); + + // TODO: Possibly parallelize. See struct comment. + let mut seen_hashes = HashSet::new(); + let mut num_duplicates: usize = 0; + let filtered: Vec<_> = hash_and_authenticators + .into_iter() + .zip(transactions) + .filter_map(|(maybe_hash, txn)| match maybe_hash { + None => Some(txn), + Some(hash_and_authenticator) => { + if seen_hashes.insert(hash_and_authenticator) { + Some(txn) + } else { + num_duplicates += 1; + None + } + }, + }) + .collect(); + + TXN_DEDUP_FILTERED.observe(num_duplicates as f64); + filtered + } +} + +impl TxnHashAndAuthenticatorDeduper { + pub fn new() -> Self { + Self {} + } +} + +#[cfg(test)] +mod tests { + use crate::{ + transaction_deduper::TransactionDeduper, + txn_hash_and_authenticator_deduper::TxnHashAndAuthenticatorDeduper, + }; + use aptos_cached_packages::aptos_stdlib; + use aptos_crypto::ed25519::{Ed25519PrivateKey, Ed25519PublicKey}; + use aptos_keygen::KeyGen; + use aptos_types::{ + chain_id::ChainId, + transaction::{RawTransaction, Script, SignedTransaction, TransactionPayload}, + }; + use move_core_types::account_address::AccountAddress; + use std::time::Instant; + + struct Account { + addr: AccountAddress, + /// The current private key for this account. + pub privkey: Ed25519PrivateKey, + /// The current public key for this account. + pub pubkey: Ed25519PublicKey, + } + + impl Account { + pub fn new() -> Self { + let (privkey, pubkey) = KeyGen::from_os_rng().generate_ed25519_keypair(); + Self::with_keypair(privkey, pubkey) + } + + pub fn with_keypair(privkey: Ed25519PrivateKey, pubkey: Ed25519PublicKey) -> Self { + let addr = aptos_types::account_address::from_public_key(&pubkey); + Account { + addr, + privkey, + pubkey, + } + } + } + + fn raw_txn( + payload: TransactionPayload, + sender: AccountAddress, + seq_num: u64, + gas_unit_price: u64, + ) -> RawTransaction { + RawTransaction::new( + sender, + seq_num, + payload, + 500_000, + gas_unit_price, + 0, + ChainId::new(10), + ) + } + + fn empty_txn(sender: AccountAddress, seq_num: u64, gas_unit_price: u64) -> RawTransaction { + let payload = TransactionPayload::Script(Script::new(vec![], vec![], vec![])); + raw_txn(payload, sender, seq_num, gas_unit_price) + } + + fn peer_to_peer_txn( + sender: AccountAddress, + receiver: AccountAddress, + seq_num: u64, + gas_unit_price: u64, + ) -> RawTransaction { + let payload = aptos_stdlib::aptos_coin_transfer(receiver, 1); + raw_txn(payload, sender, seq_num, gas_unit_price) + } + + fn block(refs: Vec<&SignedTransaction>) -> Vec { + refs.into_iter().cloned().collect() + } + + #[test] + fn test_single_txn() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let txn = empty_txn(sender.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey) + .unwrap() + .into_inner(); + let txns = vec![txn]; + let deduped_txns = deduper.dedup(txns.clone()); + assert_eq!(txns.len(), deduped_txns.len()); + assert_eq!(txns, deduped_txns); + } + + #[test] + fn test_single_duplicate() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let txn = empty_txn(sender.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey) + .unwrap() + .into_inner(); + let txns = block(vec![&txn, &txn]); + let expected = block(vec![&txn]); + let deduped_txns = deduper.dedup(txns); + assert_eq!(expected.len(), deduped_txns.len()); + assert_eq!(expected, deduped_txns); + } + + #[test] + fn test_repeated_sequence_number() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let receiver = Account::new(); + + let txn_0a = empty_txn(sender.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey.clone()) + .unwrap() + .into_inner(); + // Different txn, same sender and sequence number. Should not be filtered. + let txn_0b = peer_to_peer_txn(sender.addr, receiver.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey) + .unwrap() + .into_inner(); + let txns = block(vec![&txn_0a, &txn_0b, &txn_0a]); + let expected = block(vec![&txn_0a, &txn_0b]); + let deduped_txns = deduper.dedup(txns); + assert_eq!(expected.len(), deduped_txns.len()); + assert_eq!(expected, deduped_txns); + } + + #[test] + fn test_bad_signer() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let bad_signer = Account::new(); + + // Txn signed by a bad signer (not the sender) + let txn_0a = empty_txn(sender.addr, 0, 100) + .sign(&bad_signer.privkey, bad_signer.pubkey.clone()) + .unwrap() + .into_inner(); + // Same txn, but signed by the correct signer (sender). Should not be filtered. + let txn_0b = empty_txn(sender.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey.clone()) + .unwrap() + .into_inner(); + let txns = block(vec![&txn_0a, &txn_0b]); + let deduped_txns = deduper.dedup(txns.clone()); + assert_eq!(txns.len(), deduped_txns.len()); + assert_eq!(txns, deduped_txns); + } + + // The perf tests are simple micro-benchmarks and just output results without checking for regressions + static PERF_TXN_PER_BLOCK: usize = 10_000; + + fn measure_dedup_time( + deduper: TxnHashAndAuthenticatorDeduper, + txns: Vec, + ) -> f64 { + let start = Instant::now(); + let mut iterations = 0; + loop { + deduper.dedup(txns.clone()); + iterations += 1; + if iterations % 100 == 0 && start.elapsed().as_millis() > 2000 { + break; + } + } + let elapsed = start.elapsed(); + println!( + "elapsed: {}, iterations: {}, time per iteration: {}", + elapsed.as_secs_f64(), + iterations, + elapsed.as_secs_f64() / iterations as f64, + ); + elapsed.as_secs_f64() / iterations as f64 + } + + #[test] + fn test_performance_unique_empty_txns() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let txns: Vec<_> = (0..PERF_TXN_PER_BLOCK) + .into_iter() + .map(|i| { + empty_txn(sender.addr, i as u64, 100) + .sign(&sender.privkey, sender.pubkey.clone()) + .unwrap() + .into_inner() + }) + .collect(); + let deduped_txns = deduper.dedup(txns.clone()); + assert_eq!(txns.len(), deduped_txns.len()); + assert_eq!(txns, deduped_txns); + + measure_dedup_time(deduper, txns); + } + + #[test] + fn test_performance_duplicate_empty_txns() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let txn = empty_txn(sender.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey) + .unwrap() + .into_inner(); + let txns: Vec<_> = std::iter::repeat(txn.clone()) + .take(PERF_TXN_PER_BLOCK) + .collect(); + let expected = block(vec![&txn]); + let deduped_txns = deduper.dedup(txns.clone()); + assert_eq!(expected.len(), deduped_txns.len()); + assert_eq!(expected, deduped_txns); + + measure_dedup_time(deduper, txns); + } + + #[test] + fn test_performance_unique_p2p_txns() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let receiver = Account::new(); + let txns: Vec<_> = (0..PERF_TXN_PER_BLOCK) + .into_iter() + .map(|i| { + peer_to_peer_txn(sender.addr, receiver.addr, i as u64, 100) + .sign(&sender.privkey, sender.pubkey.clone()) + .unwrap() + .into_inner() + }) + .collect(); + let deduped_txns = deduper.dedup(txns.clone()); + assert_eq!(txns.len(), deduped_txns.len()); + assert_eq!(txns, deduped_txns); + + measure_dedup_time(deduper, txns); + } + + #[test] + fn test_performance_duplicate_p2p_txns() { + let deduper = TxnHashAndAuthenticatorDeduper::new(); + + let sender = Account::new(); + let receiver = Account::new(); + let txn = peer_to_peer_txn(sender.addr, receiver.addr, 0, 100) + .sign(&sender.privkey, sender.pubkey) + .unwrap() + .into_inner(); + let txns: Vec<_> = std::iter::repeat(txn.clone()) + .take(PERF_TXN_PER_BLOCK) + .collect(); + let expected = block(vec![&txn]); + let deduped_txns = deduper.dedup(txns.clone()); + assert_eq!(expected.len(), deduped_txns.len()); + assert_eq!(expected, deduped_txns); + + measure_dedup_time(deduper, txns); + } +} diff --git a/types/src/on_chain_config/execution_config.rs b/types/src/on_chain_config/execution_config.rs index 3b031ee39f40d..642bf469d3502 100644 --- a/types/src/on_chain_config/execution_config.rs +++ b/types/src/on_chain_config/execution_config.rs @@ -10,6 +10,7 @@ use serde::{Deserialize, Serialize}; pub enum OnChainExecutionConfig { V1(ExecutionConfigV1), V2(ExecutionConfigV2), + V3(ExecutionConfigV3), } /// The public interface that exposes all values with safe fallback. @@ -19,6 +20,7 @@ impl OnChainExecutionConfig { match &self { OnChainExecutionConfig::V1(config) => config.transaction_shuffler_type.clone(), OnChainExecutionConfig::V2(config) => config.transaction_shuffler_type.clone(), + OnChainExecutionConfig::V3(config) => config.transaction_shuffler_type.clone(), } } @@ -27,6 +29,16 @@ impl OnChainExecutionConfig { match &self { OnChainExecutionConfig::V1(_config) => None, OnChainExecutionConfig::V2(config) => config.block_gas_limit, + OnChainExecutionConfig::V3(config) => config.block_gas_limit, + } + } + + /// The type of the transaction deduper being used. + pub fn transaction_deduper_type(&self) -> TransactionDeduperType { + match &self { + OnChainExecutionConfig::V1(_config) => TransactionDeduperType::NoDedup, + OnChainExecutionConfig::V2(_config) => TransactionDeduperType::NoDedup, + OnChainExecutionConfig::V3(config) => config.transaction_deduper_type.clone(), } } } @@ -84,6 +96,23 @@ impl Default for ExecutionConfigV2 { } } +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] +pub struct ExecutionConfigV3 { + pub transaction_shuffler_type: TransactionShufflerType, + pub block_gas_limit: Option, + pub transaction_deduper_type: TransactionDeduperType, +} + +impl Default for ExecutionConfigV3 { + fn default() -> Self { + Self { + transaction_shuffler_type: TransactionShufflerType::NoShuffling, + block_gas_limit: None, + transaction_deduper_type: TransactionDeduperType::NoDedup, + } + } +} + #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] #[serde(rename_all = "snake_case")] // cannot use tag = "type" as nested enums cannot work, and bcs doesn't support it pub enum TransactionShufflerType { @@ -91,6 +120,13 @@ pub enum TransactionShufflerType { SenderAwareV1(u32), } +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(rename_all = "snake_case")] // cannot use tag = "type" as nested enums cannot work, and bcs doesn't support it +pub enum TransactionDeduperType { + NoDedup, + TxnHashAndAuthenticatorV1, +} + #[cfg(test)] mod test { use super::*; diff --git a/types/src/on_chain_config/mod.rs b/types/src/on_chain_config/mod.rs index 10f39d1dbd5a9..e319a42ba4762 100644 --- a/types/src/on_chain_config/mod.rs +++ b/types/src/on_chain_config/mod.rs @@ -40,7 +40,8 @@ pub use self::{ ProposerElectionType, }, execution_config::{ - ExecutionConfigV1, ExecutionConfigV2, OnChainExecutionConfig, TransactionShufflerType, + ExecutionConfigV1, ExecutionConfigV2, OnChainExecutionConfig, TransactionDeduperType, + TransactionShufflerType, }, gas_schedule::{GasSchedule, GasScheduleV2, StorageGasSchedule}, timed_features::{TimedFeatureFlag, TimedFeatureOverride, TimedFeatures}, diff --git a/types/src/transaction/mod.rs b/types/src/transaction/mod.rs index 1d14060e4bbbb..67605578c1496 100644 --- a/types/src/transaction/mod.rs +++ b/types/src/transaction/mod.rs @@ -555,6 +555,10 @@ impl SignedTransaction { self.authenticator.clone() } + pub fn authenticator_ref(&self) -> &TransactionAuthenticator { + &self.authenticator + } + pub fn sender(&self) -> AccountAddress { self.raw_txn.sender } @@ -563,6 +567,10 @@ impl SignedTransaction { self.raw_txn } + pub fn raw_transaction_ref(&self) -> &RawTransaction { + &self.raw_txn + } + pub fn sequence_number(&self) -> u64 { self.raw_txn.sequence_number } From 66fe8cf56c3b1cd0f869c0160bbd42a8c93c163d Mon Sep 17 00:00:00 2001 From: "Brian (Sunghoon) Cho" Date: Thu, 1 Jun 2023 15:51:29 +0900 Subject: [PATCH 06/16] Turn on txn dedup and quorum store at genesis (#8459) This only turns on for genesis (for devnet only), without a corresponding governance proposal for now. --- types/src/on_chain_config/consensus_config.rs | 2 +- types/src/on_chain_config/execution_config.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/types/src/on_chain_config/consensus_config.rs b/types/src/on_chain_config/consensus_config.rs index a94e629bf0574..4ef79f77197b5 100644 --- a/types/src/on_chain_config/consensus_config.rs +++ b/types/src/on_chain_config/consensus_config.rs @@ -61,7 +61,7 @@ impl OnChainConsensusConfig { /// This is used when on-chain config is not initialized. impl Default for OnChainConsensusConfig { fn default() -> Self { - OnChainConsensusConfig::V1(ConsensusConfigV1::default()) + OnChainConsensusConfig::V2(ConsensusConfigV1::default()) } } diff --git a/types/src/on_chain_config/execution_config.rs b/types/src/on_chain_config/execution_config.rs index 642bf469d3502..73f74a604fc52 100644 --- a/types/src/on_chain_config/execution_config.rs +++ b/types/src/on_chain_config/execution_config.rs @@ -46,7 +46,7 @@ impl OnChainExecutionConfig { /// This is used when on-chain config is not initialized. impl Default for OnChainExecutionConfig { fn default() -> Self { - OnChainExecutionConfig::V1(ExecutionConfigV1::default()) + OnChainExecutionConfig::V3(ExecutionConfigV3::default()) } } @@ -108,7 +108,7 @@ impl Default for ExecutionConfigV3 { Self { transaction_shuffler_type: TransactionShufflerType::NoShuffling, block_gas_limit: None, - transaction_deduper_type: TransactionDeduperType::NoDedup, + transaction_deduper_type: TransactionDeduperType::TxnHashAndAuthenticatorV1, } } } From 60664ff7a9d479faee4973cf76580757bae7e0e8 Mon Sep 17 00:00:00 2001 From: Victor Gao <10379359+vgao1996@users.noreply.github.com> Date: Fri, 2 Jun 2023 10:00:26 -0700 Subject: [PATCH 07/16] cherry-pick: improved value depth checks in the VM (#8487) --- aptos-move/aptos-vm/src/move_vm_ext/vm.rs | 1 + .../move-stdlib/tests/bcs_tests.move | 2 + .../nursery/tests/event_tests.move | 2 +- .../move/move-stdlib/tests/bcs_tests.move | 2 +- .../move/move-vm/runtime/src/config.rs | 5 + .../move/move-vm/runtime/src/interpreter.rs | 90 +++++++++++ .../move/move-vm/runtime/src/loader.rs | 150 +++++++++++++++--- .../runtime_layout_deeply_nested.exp | 6 +- .../runtime_layout_deeply_nested.mvir | 3 +- .../types/src/loaded_data/runtime_types.rs | 102 +++++++++++- 10 files changed, 328 insertions(+), 35 deletions(-) diff --git a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs index 60c7fce288a62..c4e6df56eeabd 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/vm.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/vm.rs @@ -73,6 +73,7 @@ impl MoveVmExt { paranoid_type_checks: crate::AptosVM::get_paranoid_checks(), enable_invariant_violation_check_in_swap_loc, type_size_limit, + max_value_nest_depth: Some(128), }, )?, chain_id, diff --git a/aptos-move/framework/move-stdlib/tests/bcs_tests.move b/aptos-move/framework/move-stdlib/tests/bcs_tests.move index 09266136ea4a3..72437ebb00fd6 100644 --- a/aptos-move/framework/move-stdlib/tests/bcs_tests.move +++ b/aptos-move/framework/move-stdlib/tests/bcs_tests.move @@ -79,9 +79,11 @@ module std::bcs_tests { bcs::to_bytes(&box127(true)); } + /* Deactivated because we now limit the depth of values you could create inside the VM #[test] #[expected_failure(abort_code = 453, location = std::bcs)] fun encode_129() { bcs::to_bytes(&Box { x: box127(true) }); } + */ } diff --git a/third_party/move/move-stdlib/nursery/tests/event_tests.move b/third_party/move/move-stdlib/nursery/tests/event_tests.move index df5a9e3e05e00..064ebb7877eeb 100644 --- a/third_party/move/move-stdlib/nursery/tests/event_tests.move +++ b/third_party/move/move-stdlib/nursery/tests/event_tests.move @@ -73,7 +73,7 @@ module std::event_tests { } #[test(s = @0x42)] - #[expected_failure(abort_code = 0, location = std::event)] + #[expected_failure] // VM_MAX_VALUE_DEPTH_REACHED fun test_event_129(s: signer) acquires MyEvent { event_129(&s); } diff --git a/third_party/move/move-stdlib/tests/bcs_tests.move b/third_party/move/move-stdlib/tests/bcs_tests.move index 9564f5a1c530a..cf16ce111371b 100644 --- a/third_party/move/move-stdlib/tests/bcs_tests.move +++ b/third_party/move/move-stdlib/tests/bcs_tests.move @@ -96,7 +96,7 @@ module std::bcs_tests { } #[test] - #[expected_failure(abort_code = 453, location = std::bcs)] + #[expected_failure] // VM_MAX_VALUE_DEPTH_REACHED fun encode_129() { bcs::to_bytes(&Box { x: box127(true) }); } diff --git a/third_party/move/move-vm/runtime/src/config.rs b/third_party/move/move-vm/runtime/src/config.rs index faa54c80c7176..5c23b87133f07 100644 --- a/third_party/move/move-vm/runtime/src/config.rs +++ b/third_party/move/move-vm/runtime/src/config.rs @@ -4,6 +4,8 @@ use move_binary_format::file_format_common::VERSION_MAX; use move_bytecode_verifier::VerifierConfig; +pub const DEFAULT_MAX_VALUE_NEST_DEPTH: u64 = 128; + /// Dynamic config options for the Move VM. pub struct VMConfig { pub verifier: VerifierConfig, @@ -14,6 +16,8 @@ pub struct VMConfig { // When this flag is set to true, MoveVM will check invariant violation in swap_loc pub enable_invariant_violation_check_in_swap_loc: bool, pub type_size_limit: bool, + /// Maximum value nest depth for structs + pub max_value_nest_depth: Option, } impl Default for VMConfig { @@ -24,6 +28,7 @@ impl Default for VMConfig { paranoid_type_checks: false, enable_invariant_violation_check_in_swap_loc: true, type_size_limit: false, + max_value_nest_depth: Some(DEFAULT_MAX_VALUE_NEST_DEPTH), } } } diff --git a/third_party/move/move-vm/runtime/src/interpreter.rs b/third_party/move/move-vm/runtime/src/interpreter.rs index c50bcf4466953..b2c8ee03884bf 100644 --- a/third_party/move/move-vm/runtime/src/interpreter.rs +++ b/third_party/move/move-vm/runtime/src/interpreter.rs @@ -1008,6 +1008,91 @@ impl CallStack { } } +fn check_depth_of_type(resolver: &Resolver, ty: &Type) -> PartialVMResult<()> { + // Start at 1 since we always call this right before we add a new node to the value's depth. + let max_depth = match resolver.loader().vm_config().max_value_nest_depth { + Some(max_depth) => max_depth, + None => return Ok(()), + }; + check_depth_of_type_impl(resolver, ty, max_depth, 1)?; + Ok(()) +} + +fn check_depth_of_type_impl( + resolver: &Resolver, + ty: &Type, + max_depth: u64, + depth: u64, +) -> PartialVMResult { + macro_rules! check_depth { + ($additional_depth:expr) => {{ + let new_depth = depth.saturating_add($additional_depth); + if new_depth > max_depth { + return Err(PartialVMError::new(StatusCode::VM_MAX_VALUE_DEPTH_REACHED)); + } else { + new_depth + } + }}; + } + + // Calculate depth of the type itself + let ty_depth = match ty { + Type::Bool + | Type::U8 + | Type::U16 + | Type::U32 + | Type::U64 + | Type::U128 + | Type::U256 + | Type::Address + | Type::Signer => check_depth!(0), + // Even though this is recursive this is OK since the depth of this recursion is + // bounded by the depth of the type arguments, which we have already checked. + Type::Reference(ty) | Type::MutableReference(ty) | Type::Vector(ty) => { + check_depth_of_type_impl(resolver, ty, max_depth, check_depth!(1))? + }, + Type::Struct(si) => { + let struct_type = resolver.loader().get_struct_type(*si).ok_or_else(|| { + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) + .with_message("Struct Definition not resolved".to_string()) + })?; + check_depth!(struct_type + .depth + .as_ref() + .ok_or_else(|| { PartialVMError::new(StatusCode::VM_MAX_VALUE_DEPTH_REACHED) })? + .solve(&[])) + }, + // NB: substitution must be performed before calling this function + Type::StructInstantiation(si, ty_args) => { + // Calculate depth of all type arguments, and make sure they themselves are not too deep. + let ty_arg_depths = ty_args + .iter() + .map(|ty| { + // Ty args should be fully resolved and not need any type arguments + check_depth_of_type_impl(resolver, ty, max_depth, check_depth!(0)) + }) + .collect::>>()?; + let struct_type = resolver.loader().get_struct_type(*si).ok_or_else(|| { + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) + .with_message("Struct Definition not resolved".to_string()) + })?; + check_depth!(struct_type + .depth + .as_ref() + .ok_or_else(|| { PartialVMError::new(StatusCode::VM_MAX_VALUE_DEPTH_REACHED) })? + .solve(&ty_arg_depths)) + }, + Type::TyParam(_) => { + return Err( + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) + .with_message("Type parameter should be fully resolved".to_string()), + ) + }, + }; + + Ok(ty_depth) +} + /// A `Frame` is the execution context for a function. It holds the locals of the function and /// the function itself. // #[derive(Debug)] @@ -1872,6 +1957,8 @@ impl Frame { }, Bytecode::Pack(sd_idx) => { let field_count = resolver.field_count(*sd_idx); + let struct_type = resolver.get_struct_type(*sd_idx); + check_depth_of_type(resolver, &struct_type)?; gas_meter.charge_pack( false, interpreter.operand_stack.last_n(field_count as usize)?, @@ -1883,6 +1970,8 @@ impl Frame { }, Bytecode::PackGeneric(si_idx) => { let field_count = resolver.field_instantiation_count(*si_idx); + let ty = resolver.instantiate_generic_type(*si_idx, self.ty_args())?; + check_depth_of_type(resolver, &ty)?; gas_meter.charge_pack( true, interpreter.operand_stack.last_n(field_count as usize)?, @@ -2199,6 +2288,7 @@ impl Frame { }, Bytecode::VecPack(si, num) => { let ty = resolver.instantiate_single_type(*si, self.ty_args())?; + check_depth_of_type(resolver, &ty)?; gas_meter.charge_vec_pack( make_ty!(&ty), interpreter.operand_stack.last_n(*num as usize)?, diff --git a/third_party/move/move-vm/runtime/src/loader.rs b/third_party/move/move-vm/runtime/src/loader.rs index d1a6041376b4b..531c1c2339fc8 100644 --- a/third_party/move/move-vm/runtime/src/loader.rs +++ b/third_party/move/move-vm/runtime/src/loader.rs @@ -18,7 +18,7 @@ use move_binary_format::{ FieldHandleIndex, FieldInstantiationIndex, FunctionDefinition, FunctionDefinitionIndex, FunctionHandleIndex, FunctionInstantiationIndex, Signature, SignatureIndex, SignatureToken, StructDefInstantiationIndex, StructDefinition, StructDefinitionIndex, - StructFieldInformation, TableIndex, Visibility, + StructFieldInformation, TableIndex, TypeParameterIndex, Visibility, }, IndexKind, }; @@ -29,7 +29,9 @@ use move_core_types::{ value::{MoveFieldLayout, MoveStructLayout, MoveTypeLayout}, vm_status::StatusCode, }; -use move_vm_types::loaded_data::runtime_types::{CachedStructIndex, StructType, Type}; +use move_vm_types::loaded_data::runtime_types::{ + CachedStructIndex, DepthFormula, StructType, Type, +}; use parking_lot::RwLock; use sha3::{Digest, Sha3_256}; use std::{ @@ -201,6 +203,30 @@ impl ModuleCache { self.structs.truncate(starting_idx); err.finish(Location::Undefined) })?; + + let struct_defs_len = module.struct_defs.len(); + + let mut depth_cache = BTreeMap::new(); + + for cached_idx in starting_idx..(starting_idx + struct_defs_len) { + self.calculate_depth_of_struct(CachedStructIndex(cached_idx), &mut depth_cache) + .map_err(|err| err.finish(Location::Undefined))?; + } + debug_assert!(depth_cache.len() == struct_defs_len); + for (cache_idx, depth) in depth_cache { + match Arc::get_mut(self.structs.get_mut(cache_idx.0).unwrap()) { + Some(struct_type) => struct_type.depth = Some(depth), + None => { + // we have pending references to the `Arc` which is impossible, + // given the code that adds the `Arc` is above and no reference to + // it should exist. + // So in the spirit of not crashing we just leave it as None and + // log the issue. + error!("Arc cannot have any live reference while publishing"); + }, + } + } + for (idx, func) in module.function_defs().iter().enumerate() { let findex = FunctionDefinitionIndex(idx as TableIndex); let mut function = Function::new(natives, findex, func, module); @@ -256,6 +282,7 @@ impl ModuleCache { name, module, struct_def: idx, + depth: None, } } @@ -366,7 +393,7 @@ impl ModuleCache { SignatureToken::U256 => Type::U256, SignatureToken::Address => Type::Address, SignatureToken::Signer => Type::Signer, - SignatureToken::TypeParameter(idx) => Type::TyParam(*idx as usize), + SignatureToken::TypeParameter(idx) => Type::TyParam(*idx), SignatureToken::Vector(inner_tok) => { let inner_type = Self::make_type_internal(module, inner_tok, resolver)?; Type::Vector(Box::new(inner_type)) @@ -457,6 +484,81 @@ impl ModuleCache { ), } } + + fn calculate_depth_of_struct( + &self, + def_idx: CachedStructIndex, + depth_cache: &mut BTreeMap, + ) -> PartialVMResult { + let struct_type = &self.struct_at(def_idx); + + // If we've already computed this structs depth, no more work remains to be done. + if let Some(form) = &struct_type.depth { + return Ok(form.clone()); + } + if let Some(form) = depth_cache.get(&def_idx) { + return Ok(form.clone()); + } + + let formulas = struct_type + .fields + .iter() + .map(|field_type| self.calculate_depth_of_type(field_type, depth_cache)) + .collect::>>()?; + let formula = DepthFormula::normalize(formulas); + let prev = depth_cache.insert(def_idx, formula.clone()); + if prev.is_some() { + return Err( + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) + .with_message("Recursive type?".to_owned()), + ); + } + Ok(formula) + } + + fn calculate_depth_of_type( + &self, + ty: &Type, + depth_cache: &mut BTreeMap, + ) -> PartialVMResult { + Ok(match ty { + Type::Bool + | Type::U8 + | Type::U64 + | Type::U128 + | Type::Address + | Type::Signer + | Type::U16 + | Type::U32 + | Type::U256 => DepthFormula::constant(1), + Type::Vector(ty) | Type::Reference(ty) | Type::MutableReference(ty) => { + let mut inner = self.calculate_depth_of_type(ty, depth_cache)?; + inner.scale(1); + inner + }, + Type::TyParam(ty_idx) => DepthFormula::type_parameter(*ty_idx), + Type::Struct(cache_idx) => { + let mut struct_formula = self.calculate_depth_of_struct(*cache_idx, depth_cache)?; + debug_assert!(struct_formula.terms.is_empty()); + struct_formula.scale(1); + struct_formula + }, + Type::StructInstantiation(cache_idx, ty_args) => { + let ty_arg_map = ty_args + .iter() + .enumerate() + .map(|(idx, ty)| { + let var = idx as TypeParameterIndex; + Ok((var, self.calculate_depth_of_type(ty, depth_cache)?)) + }) + .collect::>>()?; + let struct_formula = self.calculate_depth_of_struct(*cache_idx, depth_cache)?; + let mut subst_struct_formula = struct_formula.subst(ty_arg_map)?; + subst_struct_formula.scale(1); + subst_struct_formula + }, + }) + } } // @@ -618,7 +720,7 @@ impl Loader { && type_arguments .iter() .map(|loaded_ty| self.count_type_nodes(loaded_ty)) - .sum::() + .sum::() > MAX_TYPE_INSTANTIATION_NODES { return Err( @@ -745,7 +847,7 @@ impl Loader { fn match_return_type<'a>( returned: &Type, expected: &'a Type, - map: &mut BTreeMap, + map: &mut BTreeMap, ) -> bool { match (returned, expected) { // The important case, deduce the type params @@ -839,7 +941,7 @@ impl Loader { let mut type_arguments = vec![]; let type_param_len = func.type_parameters().len(); for i in 0..type_param_len { - if let Option::Some(t) = map.get(&i) { + if let Option::Some(t) = map.get(&(i as u16)) { type_arguments.push((*t).clone()); } else { // Unknown type argument we are not able to infer the type arguments. @@ -1397,7 +1499,7 @@ impl Loader { } }, Type::StructInstantiation(_, struct_inst) => { - let mut sum_nodes: usize = 1; + let mut sum_nodes = 1u64; for ty in ty_args.iter().chain(struct_inst.iter()) { sum_nodes = sum_nodes.saturating_add(self.count_type_nodes(ty)); if sum_nodes > MAX_TYPE_INSTANTIATION_NODES { @@ -1591,7 +1693,7 @@ impl<'a> Resolver<'a> { } // Check if the function instantiation over all generics is larger // than MAX_TYPE_INSTANTIATION_NODES. - let mut sum_nodes: usize = 1; + let mut sum_nodes = 1u64; for ty in type_params.iter().chain(instantiation.iter()) { sum_nodes = sum_nodes.saturating_add(self.loader.count_type_nodes(ty)); if sum_nodes > MAX_TYPE_INSTANTIATION_NODES { @@ -1635,8 +1737,8 @@ impl<'a> Resolver<'a> { // Before instantiating the type, count the # of nodes of all type arguments plus // existing type instantiation. // If that number is larger than MAX_TYPE_INSTANTIATION_NODES, refuse to construct this type. - // This prevents constructing larger and lager types via struct instantiation. - let mut sum_nodes: usize = 1; + // This prevents constructing larger and larger types via struct instantiation. + let mut sum_nodes = 1u64; for ty in ty_args.iter().chain(struct_inst.instantiation.iter()) { sum_nodes = sum_nodes.saturating_add(self.loader.count_type_nodes(ty)); if sum_nodes > MAX_TYPE_INSTANTIATION_NODES { @@ -2587,8 +2689,8 @@ struct StructInfo { struct_tag: Option, struct_layout: Option, annotated_struct_layout: Option, - node_count: Option, - annotated_node_count: Option, + node_count: Option, + annotated_node_count: Option, } impl StructInfo { @@ -2616,15 +2718,15 @@ impl TypeCache { } /// Maximal depth of a value in terms of type depth. -const VALUE_DEPTH_MAX: usize = 128; +pub const VALUE_DEPTH_MAX: u64 = 128; /// Maximal nodes which are allowed when converting to layout. This includes the the types of /// fields for struct types. -const MAX_TYPE_TO_LAYOUT_NODES: usize = 256; +const MAX_TYPE_TO_LAYOUT_NODES: u64 = 256; /// Maximal nodes which are all allowed when instantiating a generic type. This does not include /// field types of structs. -const MAX_TYPE_INSTANTIATION_NODES: usize = 128; +const MAX_TYPE_INSTANTIATION_NODES: u64 = 128; impl Loader { fn struct_gidx_to_type_tag( @@ -2691,7 +2793,7 @@ impl Loader { }) } - fn count_type_nodes(&self, ty: &Type) -> usize { + fn count_type_nodes(&self, ty: &Type) -> u64 { let mut todo = vec![ty]; let mut result = 0; while let Some(ty) = todo.pop() { @@ -2716,8 +2818,8 @@ impl Loader { &self, gidx: CachedStructIndex, ty_args: &[Type], - count: &mut usize, - depth: usize, + count: &mut u64, + depth: u64, ) -> PartialVMResult { if let Some(struct_map) = self.type_cache.read().structs.get(&gidx) { if let Some(struct_info) = struct_map.get(ty_args) { @@ -2761,8 +2863,8 @@ impl Loader { fn type_to_type_layout_impl( &self, ty: &Type, - count: &mut usize, - depth: usize, + count: &mut u64, + depth: u64, ) -> PartialVMResult { if *count > MAX_TYPE_TO_LAYOUT_NODES { return Err(PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES)); @@ -2838,8 +2940,8 @@ impl Loader { &self, gidx: CachedStructIndex, ty_args: &[Type], - count: &mut usize, - depth: usize, + count: &mut u64, + depth: u64, ) -> PartialVMResult { if let Some(struct_map) = self.type_cache.read().structs.get(&gidx) { if let Some(struct_info) = struct_map.get(ty_args) { @@ -2892,8 +2994,8 @@ impl Loader { fn type_to_fully_annotated_layout_impl( &self, ty: &Type, - count: &mut usize, - depth: usize, + count: &mut u64, + depth: u64, ) -> PartialVMResult { if *count > MAX_TYPE_TO_LAYOUT_NODES { return Err(PartialVMError::new(StatusCode::TOO_MANY_TYPE_NODES)); diff --git a/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.exp b/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.exp index a7ad267396026..ff2cde123ef29 100644 --- a/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.exp +++ b/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.exp @@ -6,14 +6,14 @@ Error: Script execution failed with VMError: { sub_status: None, location: 0x42::M, indices: [], - offsets: [(FunctionDefinitionIndex(8), 3)], + offsets: [(FunctionDefinitionIndex(0), 1)], } -task 3 'run'. lines 89-97: +task 3 'run'. lines 89-96: Error: Script execution failed with VMError: { major_status: VM_MAX_VALUE_DEPTH_REACHED, sub_status: None, location: 0x42::M, indices: [], - offsets: [(FunctionDefinitionIndex(9), 4)], + offsets: [(FunctionDefinitionIndex(0), 1)], } diff --git a/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.mvir b/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.mvir index 7f2bc48e6bf95..9044139ecd6f3 100644 --- a/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.mvir +++ b/third_party/move/move-vm/transactional-tests/tests/recursion/runtime_layout_deeply_nested.mvir @@ -71,11 +71,11 @@ import 0x42.M; main(account: signer) { label b0: + // hits VM_MAX_VALUE_DEPTH_REACHED M.publish_128(&account); return; } - //# run --args @0x2 import 0x42.M; @@ -91,7 +91,6 @@ import 0x42.M; main(account: signer) { label b0: - // hits VM_MAX_VALUE_DEPTH_REACHED M.publish_257(&account); return; } diff --git a/third_party/move/move-vm/types/src/loaded_data/runtime_types.rs b/third_party/move/move-vm/types/src/loaded_data/runtime_types.rs index d8c9a99697c37..e8ed89f2e5088 100644 --- a/third_party/move/move-vm/types/src/loaded_data/runtime_types.rs +++ b/third_party/move/move-vm/types/src/loaded_data/runtime_types.rs @@ -4,15 +4,108 @@ use move_binary_format::{ errors::{PartialVMError, PartialVMResult}, - file_format::{AbilitySet, SignatureToken, StructDefinitionIndex, StructTypeParameter}, + file_format::{ + AbilitySet, SignatureToken, StructDefinitionIndex, StructTypeParameter, TypeParameterIndex, + }, }; use move_core_types::{ gas_algebra::AbstractMemorySize, identifier::Identifier, language_storage::ModuleId, vm_status::StatusCode, }; +use std::{cmp::max, collections::BTreeMap, fmt::Debug}; pub const TYPE_DEPTH_MAX: usize = 256; +#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Debug)] +/// A formula describing the value depth of a type, using (the depths of) the type parameters as inputs. +/// +/// It has the form of `max(CBase, T1 + C1, T2 + C2, ..)` where `Ti` is the depth of the ith type parameter +/// and `Ci` is just some constant. +/// +/// This form has a special property: when you compute the max of multiple formulae, you can normalize +/// them into a single formula. +pub struct DepthFormula { + pub terms: Vec<(TypeParameterIndex, u64)>, // Ti + Ci + pub constant: Option, // Cbase +} + +impl DepthFormula { + pub fn constant(constant: u64) -> Self { + Self { + terms: vec![], + constant: Some(constant), + } + } + + pub fn type_parameter(tparam: TypeParameterIndex) -> Self { + Self { + terms: vec![(tparam, 0)], + constant: None, + } + } + + pub fn normalize(formulas: Vec) -> Self { + let mut var_map = BTreeMap::new(); + let mut constant_acc = None; + for formula in formulas { + let Self { terms, constant } = formula; + for (var, cur_factor) in terms { + var_map + .entry(var) + .and_modify(|prev_factor| *prev_factor = max(cur_factor, *prev_factor)) + .or_insert(cur_factor); + } + match (constant_acc, constant) { + (_, None) => (), + (None, Some(_)) => constant_acc = constant, + (Some(c1), Some(c2)) => constant_acc = Some(max(c1, c2)), + } + } + Self { + terms: var_map.into_iter().collect(), + constant: constant_acc, + } + } + + pub fn subst( + &self, + mut map: BTreeMap, + ) -> PartialVMResult { + let Self { terms, constant } = self; + let mut formulas = vec![]; + if let Some(constant) = constant { + formulas.push(DepthFormula::constant(*constant)) + } + for (t_i, c_i) in terms { + let Some(mut u_form) = map.remove(t_i) else { + return Err(PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR).with_message(format!("{t_i:?} missing mapping"))) + }; + u_form.scale(*c_i); + formulas.push(u_form) + } + Ok(DepthFormula::normalize(formulas)) + } + + pub fn solve(&self, tparam_depths: &[u64]) -> u64 { + let Self { terms, constant } = self; + let mut depth = constant.as_ref().copied().unwrap_or(0); + for (t_i, c_i) in terms { + depth = max(depth, tparam_depths[*t_i as usize].saturating_add(*c_i)) + } + depth + } + + pub fn scale(&mut self, c: u64) { + let Self { terms, constant } = self; + for (_t_i, c_i) in terms { + *c_i = (*c_i).saturating_add(c); + } + if let Some(cbase) = constant.as_mut() { + *cbase = (*cbase).saturating_add(c); + } + } +} + #[derive(Debug, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)] pub struct StructType { pub fields: Vec, @@ -22,6 +115,7 @@ pub struct StructType { pub name: Identifier, pub module: ModuleId, pub struct_def: StructDefinitionIndex, + pub depth: Option, } impl StructType { @@ -46,7 +140,7 @@ pub enum Type { StructInstantiation(CachedStructIndex, Vec), Reference(Box), MutableReference(Box), - TyParam(usize), + TyParam(u16), U16, U32, U256, @@ -62,7 +156,7 @@ impl Type { fn apply_subst(&self, subst: F, depth: usize) -> PartialVMResult where - F: Fn(usize, usize) -> PartialVMResult + Copy, + F: Fn(u16, usize) -> PartialVMResult + Copy, { if depth > TYPE_DEPTH_MAX { return Err(PartialVMError::new(StatusCode::VM_MAX_TYPE_DEPTH_REACHED)); @@ -97,7 +191,7 @@ impl Type { pub fn subst(&self, ty_args: &[Type]) -> PartialVMResult { self.apply_subst( - |idx, depth| match ty_args.get(idx) { + |idx, depth| match ty_args.get(idx as usize) { Some(ty) => ty.clone_impl(depth), None => Err( PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) From baeb6dd5737167e6bee8832ffd951811e375a3fe Mon Sep 17 00:00:00 2001 From: geekflyer Date: Sat, 3 Jun 2023 17:55:42 -0700 Subject: [PATCH 08/16] fix trigger condition for build jobs --- .github/workflows/docker-build-test.yaml | 93 +++++++++++------------- 1 file changed, 43 insertions(+), 50 deletions(-) diff --git a/.github/workflows/docker-build-test.yaml b/.github/workflows/docker-build-test.yaml index 80f7ab8d936d9..bbcf9f388e16b 100644 --- a/.github/workflows/docker-build-test.yaml +++ b/.github/workflows/docker-build-test.yaml @@ -171,43 +171,8 @@ jobs: FEATURES: consensus-only-perf-test BUILD_ADDL_TESTING_IMAGES: true - rust-images-all: - needs: - [ - determine-docker-build-metadata, - rust-images, - rust-images-indexer, - rust-images-failpoints, - rust-images-performance, - rust-images-consensus-only-perf-test, - ] - if: always() # this ensures that the job will run even if the previous jobs were skipped - runs-on: ubuntu-latest - steps: - - name: fail if rust-images job failed - if: ${{ needs.rust-images.result == 'failure' }} - run: exit 1 - - name: fail if rust-images-indexer job failed - if: ${{ needs.rust-images-indexer.result == 'failure' }} - run: exit 1 - - name: fail if rust-images-failpoints job failed - if: ${{ needs.rust-images-failpoints.result == 'failure' }} - run: exit 1 - - name: fail if rust-images-performance job failed - if: ${{ needs.rust-images-performance.result == 'failure' }} - run: exit 1 - - name: fail if rust-images-consensus-only-perf-test job failed - if: ${{ needs.rust-images-consensus-only-perf-test.result == 'failure' }} - run: exit 1 - outputs: - rustImagesResult: ${{ needs.rust-images.result }} - rustImagesIndexerResult: ${{ needs.rust-images-indexer.result }} - rustImagesFailpointsResult: ${{ needs.rust-images-failpoints.result }} - rustImagesPerformanceResult: ${{ needs.rust-images-performance.result }} - rustImagesConsensusOnlyPerfTestResult: ${{ needs.rust-images-consensus-only-perf-test.result }} - sdk-release: - needs: [rust-images, determine-docker-build-metadata] # runs with the default release docker build variant "rust-images" + needs: [permission-check, rust-images, determine-docker-build-metadata] # runs with the default release docker build variant "rust-images" if: | (github.event_name == 'push' && github.ref_name != 'main') || github.event_name == 'workflow_dispatch' || @@ -220,7 +185,7 @@ jobs: GIT_SHA: ${{ needs.determine-docker-build-metadata.outputs.gitSha }} cli-e2e-tests: - needs: [rust-images, determine-docker-build-metadata] # runs with the default release docker build variant "rust-images" + needs: [permission-check, rust-images, determine-docker-build-metadata] # runs with the default release docker build variant "rust-images" if: | !contains(github.event.pull_request.labels.*.name, 'CICD:skip-sdk-integration-test') && ( github.event_name == 'push' || @@ -235,7 +200,7 @@ jobs: GIT_SHA: ${{ needs.determine-docker-build-metadata.outputs.gitSha }} indexer-grpc-e2e-tests: - needs: [rust-images, determine-docker-build-metadata] # runs with the default release docker build variant "rust-images" + needs: [permission-check, rust-images, determine-docker-build-metadata] # runs with the default release docker build variant "rust-images" if: | (github.event_name == 'push' && github.ref_name != 'main') || github.event_name == 'workflow_dispatch' || @@ -248,9 +213,16 @@ jobs: GIT_SHA: ${{ needs.determine-docker-build-metadata.outputs.gitSha }} forge-e2e-test: - needs: [rust-images-all, determine-docker-build-metadata] - if: | # always() ensures that the job will run even if some of the previous docker variant build jobs were skipped https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions - always() && needs.rust-images-all.result == 'success' && ( + needs: + - permission-check + - determine-docker-build-metadata + - rust-images + - rust-images-indexer + - rust-images-failpoints + - rust-images-performance + - rust-images-consensus-only-perf-test + if: | + !failure() && !cancelled() && needs.permission-check.result == 'success' && ( (github.event_name == 'push' && github.ref_name != 'main') || github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'CICD:run-e2e-tests') || @@ -272,9 +244,16 @@ jobs: # Run e2e compat test against testnet branch forge-compat-test: - needs: [rust-images-all, determine-docker-build-metadata] - if: | # always() ensures that the job will run even if some of the previous docker variant build jobs were skipped https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions - always() && needs.rust-images-all.result == 'success' && ( + needs: + - permission-check + - determine-docker-build-metadata + - rust-images + - rust-images-indexer + - rust-images-failpoints + - rust-images-performance + - rust-images-consensus-only-perf-test + if: | + !failure() && !cancelled() && needs.permission-check.result == 'success' && ( (github.event_name == 'push' && github.ref_name != 'main') || github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'CICD:run-e2e-tests') || @@ -293,9 +272,16 @@ jobs: # Run forge framework upgradability test forge-framework-upgrade-test: - needs: [rust-images-all, determine-docker-build-metadata] - if: | # always() ensures that the job will run even if some of the previous docker variant build jobs were skipped https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions - always() && needs.rust-images-all.result == 'success' && ( + needs: + - permission-check + - determine-docker-build-metadata + - rust-images + - rust-images-indexer + - rust-images-failpoints + - rust-images-performance + - rust-images-consensus-only-perf-test + if: | + !failure() && !cancelled() && needs.permission-check.result == 'success' && ( (github.event_name == 'push' && github.ref_name != 'main') || github.event_name == 'workflow_dispatch' || contains(github.event.pull_request.labels.*.name, 'CICD:run-e2e-tests') || @@ -313,9 +299,16 @@ jobs: FORGE_NAMESPACE: forge-framework-upgrade-${{ needs.determine-docker-build-metadata.outputs.targetCacheId }} forge-consensus-only-perf-test: - needs: [rust-images-all, determine-docker-build-metadata] - if: | # always() ensures that the job will run even if some of the previous docker variant build jobs were skipped https://docs.github.com/en/actions/learn-github-actions/expressions#status-check-functions - always() && needs.rust-images-all.result == 'success' && + needs: + - permission-check + - determine-docker-build-metadata + - rust-images + - rust-images-indexer + - rust-images-failpoints + - rust-images-performance + - rust-images-consensus-only-perf-test + if: | + !failure() && !cancelled() && needs.permission-check.result == 'success' && contains(github.event.pull_request.labels.*.name, 'CICD:run-consensus-only-perf-test') uses: aptos-labs/aptos-core/.github/workflows/workflow-run-forge.yaml@main secrets: inherit From 951bd31c69f9ae0e62f1ab135163b5dc89fa1884 Mon Sep 17 00:00:00 2001 From: Sherry Xiao Date: Wed, 7 Jun 2023 14:34:11 -0700 Subject: [PATCH 09/16] Update gas metering (#8526) (#8533) * Add accurate resource group gas metering * Add accurate resource group gas metering * fix * add test * Correct feature version * change trait signature * Clear any resource cache after proloque * bump gas feature * add comment * fix trigger condition for build jobs * test loadtest * add improvement * Fix respawn session whatever that maybe --------- Co-authored-by: Kevin <105028215+movekevin@users.noreply.github.com> Co-authored-by: gerben Co-authored-by: geekflyer --- aptos-move/aptos-gas/src/gas_meter.rs | 4 +- aptos-move/aptos-vm/src/aptos_vm.rs | 39 ++++++++--- aptos-move/aptos-vm/src/aptos_vm_impl.rs | 2 +- .../aptos-vm/src/block_executor/vm_wrapper.rs | 5 +- aptos-move/aptos-vm/src/data_cache.rs | 65 +++++++++++++------ .../aptos-vm/src/move_vm_ext/resolver.rs | 4 +- .../src/move_vm_ext/respawned_session.rs | 10 ++- .../aptos-vm/src/move_vm_ext/session.rs | 23 ++++--- .../async/move-async-vm/tests/testsuite.rs | 6 +- .../move/move-core/types/src/resolver.rs | 15 ++++- .../src/tests/bad_storage_tests.rs | 2 +- .../move/move-vm/runtime/src/data_cache.rs | 4 +- .../src/unit_tests/vm_arguments_tests.rs | 2 +- .../move/move-vm/test-utils/src/storage.rs | 14 ++-- .../src/sandbox/utils/on_disk_state_view.rs | 7 +- 15 files changed, 136 insertions(+), 66 deletions(-) diff --git a/aptos-move/aptos-gas/src/gas_meter.rs b/aptos-move/aptos-gas/src/gas_meter.rs index 4b249d34ab338..3851f84aef59e 100644 --- a/aptos-move/aptos-gas/src/gas_meter.rs +++ b/aptos-move/aptos-gas/src/gas_meter.rs @@ -33,6 +33,8 @@ use move_vm_types::{ use std::collections::BTreeMap; // Change log: +// - V9 +// - Accurate tracking of the cost of loading resource groups // - V8 // - Added BLS12-381 operations. // - V7 @@ -59,7 +61,7 @@ use std::collections::BTreeMap; // global operations. // - V1 // - TBA -pub const LATEST_GAS_FEATURE_VERSION: u64 = 8; +pub const LATEST_GAS_FEATURE_VERSION: u64 = 9; pub(crate) const EXECUTION_GAS_MULTIPLIER: u64 = 20; diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index 26e7ea225bb1d..30524e59c52a8 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -9,7 +9,7 @@ use crate::{ aptos_vm_impl::{get_transaction_output, AptosVMImpl, AptosVMInternals}, block_executor::BlockAptosVM, counters::*, - data_cache::{AsMoveResolver, StorageAdapter}, + data_cache::StorageAdapter, errors::expect_only_successful_execution, move_vm_ext::{MoveResolverExt, RespawnedSession, SessionExt, SessionId}, sharded_block_executor::ShardedBlockExecutor, @@ -246,6 +246,14 @@ impl AptosVM { .1 } + pub fn as_move_resolver<'a, S: StateView>(&self, state_view: &'a S) -> StorageAdapter<'a, S> { + StorageAdapter::new_with_cached_config( + state_view, + self.0.get_gas_feature_version(), + self.0.get_features(), + ) + } + fn failed_transaction_cleanup_and_keep_vm_status( &self, error_code: VMStatus, @@ -1031,6 +1039,9 @@ impl AptosVM { // have been previously cached in the prologue. // // TODO(Gas): Do this in a better way in the future, perhaps without forcing the data cache to be flushed. + // By releasing resource group cache, we start with a fresh slate for resource group + // cost accounting. + resolver.release_resource_group_cache(); session = self.0.new_session(resolver, SessionId::txn(txn), true); } @@ -1149,8 +1160,7 @@ impl AptosVM { F: FnOnce(u64, AptosGasParameters, StorageGasParameters, Gas) -> Result, { // TODO(Gas): revisit this. - let resolver = StorageAdapter::new(state_view); - let vm = AptosVM::new(&resolver); + let vm = AptosVM::new(state_view); // TODO(Gas): avoid creating txn metadata twice. let balance = TransactionMetadata::new(txn).max_gas_amount(); @@ -1161,6 +1171,11 @@ impl AptosVM { balance, )?; + let resolver = StorageAdapter::new_with_cached_config( + state_view, + vm.0.get_gas_feature_version(), + vm.0.get_features(), + ); let (status, output) = vm.execute_user_transaction_impl(&resolver, txn, log_context, &mut gas_meter); @@ -1340,7 +1355,13 @@ impl AptosVM { let vm = AptosVM::new(state_view); let simulation_vm = AptosSimulationVM(vm); let log_context = AdapterLogSchema::new(state_view.id(), 0); - simulation_vm.simulate_signed_transaction(&state_view.as_move_resolver(), txn, &log_context) + + // Try to simulate with aggregator enabled. + simulation_vm.simulate_signed_transaction( + &simulation_vm.0.as_move_resolver(state_view), + txn, + &log_context, + ) } pub fn execute_view_function( @@ -1359,8 +1380,8 @@ impl AptosVM { vm.0.get_storage_gas_parameters(&log_context)?.clone(), gas_budget, ); - let resolver = &state_view.as_move_resolver(); - let mut session = vm.new_session(resolver, SessionId::Void); + let resolver = vm.as_move_resolver(state_view); + let mut session = vm.new_session(&resolver, SessionId::Void); let func_inst = session.load_function(&module_id, &func_name, &type_args)?; let metadata = vm.0.extract_module_metadata(&module_id); @@ -1564,11 +1585,11 @@ impl VMValidator for AptosVM { }, }; - let resolver = &state_view.as_move_resolver(); - let mut session = self.new_session(resolver, SessionId::txn(&txn)); + let resolver = self.as_move_resolver(state_view); + let mut session = self.new_session(&resolver, SessionId::txn(&txn)); let validation_result = self.validate_signature_checked_transaction( &mut session, - resolver, + &resolver, &txn, true, &log_context, diff --git a/aptos-move/aptos-vm/src/aptos_vm_impl.rs b/aptos-move/aptos-vm/src/aptos_vm_impl.rs index a2b9c072b8438..a49bcfd4e7dc8 100644 --- a/aptos-move/aptos-vm/src/aptos_vm_impl.rs +++ b/aptos-move/aptos-vm/src/aptos_vm_impl.rs @@ -163,7 +163,7 @@ impl AptosVMImpl { features, }; vm.version = Version::fetch_config(&storage); - vm.transaction_validation = Self::get_transaction_validation(&StorageAdapter::new(state)); + vm.transaction_validation = Self::get_transaction_validation(&storage); vm } diff --git a/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs b/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs index 2d0aa01bcf56c..912567b908f2e 100644 --- a/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs +++ b/aptos-move/aptos-vm/src/block_executor/vm_wrapper.rs @@ -6,7 +6,6 @@ use crate::{ adapter_common::{PreprocessedTransaction, VMAdapter}, aptos_vm::AptosVM, block_executor::AptosTransactionOutput, - data_cache::{AsMoveResolver, StorageAdapter}, }; use aptos_aggregator::{delta_change_set::DeltaChangeSet, transaction::TransactionOutputExt}; use aptos_block_executor::task::{ExecutionStatus, ExecutorTask}; @@ -44,7 +43,7 @@ impl<'a, S: 'a + StateView + Sync> ExecutorTask for AptosExecutorTask<'a, S> { let _ = vm.load_module( &ModuleId::new(CORE_CODE_ADDRESS, ident_str!("account").to_owned()), - &StorageAdapter::new(argument), + &vm.as_move_resolver(argument), ); Self { @@ -67,7 +66,7 @@ impl<'a, S: 'a + StateView + Sync> ExecutorTask for AptosExecutorTask<'a, S> { match self .vm - .execute_single_transaction(txn, &view.as_move_resolver(), &log_context) + .execute_single_transaction(txn, &self.vm.as_move_resolver(view), &log_context) { Ok((vm_status, mut output_ext, sender)) => { if materialize_deltas { diff --git a/aptos-move/aptos-vm/src/data_cache.rs b/aptos-move/aptos-vm/src/data_cache.rs index b70b89300d127..9e6c0954e0bd2 100644 --- a/aptos-move/aptos-vm/src/data_cache.rs +++ b/aptos-move/aptos-vm/src/data_cache.rs @@ -21,7 +21,7 @@ use move_core_types::{ account_address::AccountAddress, language_storage::{ModuleId, StructTag}, metadata::Metadata, - resolver::{ModuleResolver, ResourceResolver}, + resolver::{resource_add_cost, ModuleResolver, ResourceResolver}, vm_status::StatusCode, }; use move_table_extension::{TableHandle, TableResolver}; @@ -42,16 +42,45 @@ pub(crate) fn get_resource_group_from_metadata( /// Adapter to convert a `StateView` into a `MoveResolverExt`. pub struct StorageAdapter<'a, S> { state_store: &'a S, + accurate_byte_count: bool, + max_binary_format_version: u32, resource_group_cache: RefCell>>>>, } impl<'a, S: StateView> StorageAdapter<'a, S> { + pub fn new_with_cached_config( + state_store: &'a S, + gas_feature_version: u64, + features: &Features, + ) -> Self { + let mut s = Self { + state_store, + accurate_byte_count: false, + max_binary_format_version: 0, + resource_group_cache: RefCell::new(BTreeMap::new()), + }; + if gas_feature_version >= 9 { + s.accurate_byte_count = true; + } + s.max_binary_format_version = get_max_binary_format_version(features, gas_feature_version); + s + } + pub fn new(state_store: &'a S) -> Self { - Self { + let mut s = Self { state_store, + accurate_byte_count: false, + max_binary_format_version: 0, resource_group_cache: RefCell::new(BTreeMap::new()), + }; + let (_, gas_feature_version) = gas_config(&s); + let features = Features::fetch_config(&s).unwrap_or_default(); + if gas_feature_version >= 9 { + s.accurate_byte_count = true; } + s.max_binary_format_version = get_max_binary_format_version(&features, gas_feature_version); + s } pub fn get(&self, access_path: AccessPath) -> PartialVMResult>> { @@ -65,7 +94,7 @@ impl<'a, S: StateView> StorageAdapter<'a, S> { address: &AccountAddress, struct_tag: &StructTag, metadata: &[Metadata], - ) -> Result>, VMError> { + ) -> Result, u64)>, VMError> { let resource_group = get_resource_group_from_metadata(struct_tag, metadata); if let Some(resource_group) = resource_group { let mut cache = self.resource_group_cache.borrow_mut(); @@ -73,10 +102,16 @@ impl<'a, S: StateView> StorageAdapter<'a, S> { if let Some(group_data) = cache.get_mut(&resource_group) { // This resource group is already cached for this address. So just return the // cached value. - return Ok(group_data.get(struct_tag).cloned()); + let buf = group_data.get(struct_tag).cloned(); + return Ok(resource_add_cost(buf, 0)); } let group_data = self.get_resource_group_data(address, &resource_group)?; if let Some(group_data) = group_data { + let len = if self.accurate_byte_count { + group_data.len() as u64 + } else { + 0 + }; let group_data: BTreeMap> = bcs::from_bytes(&group_data) .map_err(|_| { PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) @@ -84,13 +119,14 @@ impl<'a, S: StateView> StorageAdapter<'a, S> { })?; let res = group_data.get(struct_tag).cloned(); cache.insert(resource_group, group_data); - Ok(res) + Ok(resource_add_cost(res, len)) } else { cache.insert(resource_group, BTreeMap::new()); Ok(None) } } else { - self.get_standard_resource(address, struct_tag) + let buf = self.get_standard_resource(address, struct_tag)?; + Ok(resource_add_cost(buf, 0)) } } } @@ -118,13 +154,8 @@ impl<'a, S: StateView> MoveResolverExt for StorageAdapter<'a, S> { fn release_resource_group_cache( &self, - address: &AccountAddress, - resource_group: &StructTag, - ) -> Option>> { - self.resource_group_cache - .borrow_mut() - .get_mut(address)? - .remove(resource_group) + ) -> BTreeMap>>> { + self.resource_group_cache.take() } } @@ -134,7 +165,7 @@ impl<'a, S: StateView> ResourceResolver for StorageAdapter<'a, S> { address: &AccountAddress, struct_tag: &StructTag, metadata: &[Metadata], - ) -> Result>, Error> { + ) -> anyhow::Result, u64)>> { Ok(self.get_any_resource(address, struct_tag, metadata)?) } } @@ -145,13 +176,9 @@ impl<'a, S: StateView> ModuleResolver for StorageAdapter<'a, S> { Ok(Some(bytes)) => bytes, _ => return vec![], }; - let (_, gas_feature_version) = gas_config(self); - let features = Features::fetch_config(self).unwrap_or_default(); - let max_binary_format_version = - get_max_binary_format_version(&features, gas_feature_version); let module = match CompiledModule::deserialize_with_max_version( &module_bytes, - max_binary_format_version, + self.max_binary_format_version, ) { Ok(module) => module, _ => return vec![], diff --git a/aptos-move/aptos-vm/src/move_vm_ext/resolver.rs b/aptos-move/aptos-vm/src/move_vm_ext/resolver.rs index e7f0476fc5ea1..de6c3a8570122 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/resolver.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/resolver.rs @@ -29,9 +29,7 @@ pub trait MoveResolverExt: fn release_resource_group_cache( &self, - address: &AccountAddress, - resource_group: &StructTag, - ) -> Option>>; + ) -> BTreeMap>>>; // Move to API does not belong here fn is_resource_group(&self, struct_tag: &StructTag) -> bool { diff --git a/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs b/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs index b2cf0440a25b2..aa94eb4933946 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs @@ -3,7 +3,7 @@ use crate::{ aptos_vm_impl::AptosVMImpl, - data_cache::{AsMoveResolver, StorageAdapter}, + data_cache::StorageAdapter, move_vm_ext::{SessionExt, SessionId}, }; use anyhow::{bail, Result}; @@ -44,7 +44,13 @@ impl<'r, 'l> RespawnedSession<'r, 'l> { Ok(RespawnedSessionBuilder { state_view, - resolver_builder: |state_view| state_view.as_move_resolver(), + resolver_builder: |state_view| { + StorageAdapter::new_with_cached_config( + state_view, + vm.get_gas_feature_version(), + vm.get_features(), + ) + }, session_builder: |resolver| Some(vm.new_session(resolver, session_id, true)), } .build()) diff --git a/aptos-move/aptos-vm/src/move_vm_ext/session.rs b/aptos-move/aptos-vm/src/move_vm_ext/session.rs index 6a0c60af2125f..d3409ca49ab5e 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/session.rs @@ -38,6 +38,7 @@ use move_table_extension::{NativeTableContext, TableChangeSet}; use move_vm_runtime::{move_vm::MoveVM, session::Session}; use serde::{Deserialize, Serialize}; use std::{ + borrow::BorrowMut, collections::BTreeMap, ops::{Deref, DerefMut}, sync::Arc, @@ -199,19 +200,21 @@ impl<'r, 'l> SessionExt<'r, 'l> { let mut change_set_filtered = MoveChangeSet::new(); let mut resource_group_change_set = MoveChangeSet::new(); + let mut resource_group_cache = remote.release_resource_group_cache(); for (addr, account_changeset) in change_set.into_inner() { let mut resource_groups: BTreeMap = BTreeMap::new(); let mut resources_filtered = BTreeMap::new(); let (modules, resources) = account_changeset.into_inner(); for (struct_tag, blob_op) in resources { - let resource_group = runtime.with_module_metadata(&struct_tag.module_id(), |md| { - get_resource_group_from_metadata(&struct_tag, md) - }); + let resource_group_tag = runtime + .with_module_metadata(&struct_tag.module_id(), |md| { + get_resource_group_from_metadata(&struct_tag, md) + }); - if let Some(resource_group) = resource_group { + if let Some(resource_group_tag) = resource_group_tag { resource_groups - .entry(resource_group) + .entry(resource_group_tag) .or_insert_with(AccountChangeSet::new) .add_resource_op(struct_tag, blob_op) .map_err(|_| common_error())?; @@ -227,9 +230,11 @@ impl<'r, 'l> SessionExt<'r, 'l> { ) .map_err(|_| common_error())?; - for (resource_tag, resources) in resource_groups { - let mut source_data = remote - .release_resource_group_cache(&addr, &resource_tag) + for (resource_group_tag, resources) in resource_groups { + let mut source_data = resource_group_cache + .borrow_mut() + .get_mut(&addr) + .and_then(|t| t.remove(&resource_group_tag)) .unwrap_or_default(); let create = source_data.is_empty(); @@ -259,7 +264,7 @@ impl<'r, 'l> SessionExt<'r, 'l> { MoveStorageOp::Modify(bcs::to_bytes(&source_data).map_err(|_| common_error())?) }; resource_group_change_set - .add_resource_op(addr, resource_tag, op) + .add_resource_op(addr, resource_group_tag, op) .map_err(|_| common_error())?; } } diff --git a/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs b/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs index 705c4176b9f6e..46d8dde897456 100644 --- a/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs +++ b/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs @@ -23,7 +23,7 @@ use move_core_types::{ identifier::{IdentStr, Identifier}, language_storage::{ModuleId, StructTag}, metadata::Metadata, - resolver::{ModuleResolver, ResourceResolver}, + resolver::{resource_add_cost, ModuleResolver, ResourceResolver}, }; use move_prover_test_utils::{baseline_test::verify_or_update_baseline, extract_test_directives}; use move_vm_test_utils::gas_schedule::GasStatus; @@ -398,14 +398,14 @@ impl<'a> ResourceResolver for HarnessProxy<'a> { address: &AccountAddress, typ: &StructTag, _metadata: &[Metadata], - ) -> Result>, Error> { + ) -> anyhow::Result, u64)>> { let res = self .harness .resource_store .borrow() .get(&(*address, typ.clone())) .cloned(); - Ok(res) + Ok(resource_add_cost(res, 0)) } } diff --git a/third_party/move/move-core/types/src/resolver.rs b/third_party/move/move-core/types/src/resolver.rs index f61d82a23ae80..5b8e5f405be49 100644 --- a/third_party/move/move-core/types/src/resolver.rs +++ b/third_party/move/move-core/types/src/resolver.rs @@ -41,7 +41,14 @@ pub trait ResourceResolver { address: &AccountAddress, typ: &StructTag, metadata: &[Metadata], - ) -> Result>, Error>; + ) -> Result, u64)>, Error>; +} + +pub fn resource_add_cost(buf: Option>, extra: u64) -> Option<(Vec, u64)> { + buf.map(|b| { + let len = b.len() as u64 + extra; + (b, len) + }) } /// A persistent storage implementation that can resolve both resources and modules @@ -51,7 +58,9 @@ pub trait MoveResolver: ModuleResolver + ResourceResolver { address: &AccountAddress, typ: &StructTag, ) -> Result>, Error> { - self.get_resource_with_metadata(address, typ, &self.get_module_metadata(&typ.module_id())) + Ok(self + .get_resource_with_metadata(address, typ, &self.get_module_metadata(&typ.module_id()))? + .map(|(buf, _)| buf)) } } @@ -63,7 +72,7 @@ impl ResourceResolver for &T { address: &AccountAddress, tag: &StructTag, metadata: &[Metadata], - ) -> Result>, Error> { + ) -> Result, u64)>, Error> { (**self).get_resource_with_metadata(address, tag, metadata) } } diff --git a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs index bb87bcc2161a0..391fa5a027b1a 100644 --- a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs +++ b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs @@ -526,7 +526,7 @@ impl ResourceResolver for BogusStorage { _address: &AccountAddress, _tag: &StructTag, _metadata: &[Metadata], - ) -> Result>, anyhow::Error> { + ) -> anyhow::Result, u64)>> { Ok(Err( PartialVMError::new(self.bad_status_code).finish(Location::Undefined) )?) diff --git a/third_party/move/move-vm/runtime/src/data_cache.rs b/third_party/move/move-vm/runtime/src/data_cache.rs index 4df35c2b4954e..528d6d7b29ee7 100644 --- a/third_party/move/move-vm/runtime/src/data_cache.rs +++ b/third_party/move/move-vm/runtime/src/data_cache.rs @@ -193,8 +193,8 @@ impl<'r> TransactionDataCache<'r> { .remote .get_resource_with_metadata(&addr, &ty_tag, metadata) { - Ok(Some(blob)) => { - load_res = Some(Some(NumBytes::new(blob.len() as u64))); + Ok(Some((blob, bytes))) => { + load_res = Some(Some(NumBytes::new(bytes))); let val = match Value::simple_deserialize(&blob, &ty_layout) { Some(val) => val, None => { diff --git a/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs b/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs index 40e712d3a2cc8..0dea6cca8c0b1 100644 --- a/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs +++ b/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs @@ -265,7 +265,7 @@ impl ResourceResolver for RemoteStore { _address: &AccountAddress, _tag: &StructTag, _metadata: &[Metadata], - ) -> Result>, anyhow::Error> { + ) -> anyhow::Result, u64)>> { Ok(None) } } diff --git a/third_party/move/move-vm/test-utils/src/storage.rs b/third_party/move/move-vm/test-utils/src/storage.rs index 6df6246ab41e6..5f596e097ae2e 100644 --- a/third_party/move/move-vm/test-utils/src/storage.rs +++ b/third_party/move/move-vm/test-utils/src/storage.rs @@ -9,7 +9,7 @@ use move_core_types::{ identifier::Identifier, language_storage::{ModuleId, StructTag}, metadata::Metadata, - resolver::{ModuleResolver, MoveResolver, ResourceResolver}, + resolver::{resource_add_cost, ModuleResolver, MoveResolver, ResourceResolver}, }; #[cfg(feature = "table-extension")] use move_table_extension::{TableChangeSet, TableHandle, TableResolver}; @@ -44,7 +44,7 @@ impl ResourceResolver for BlankStorage { _address: &AccountAddress, _tag: &StructTag, _metadata: &[Metadata], - ) -> Result>> { + ) -> Result, u64)>> { Ok(None) } } @@ -90,10 +90,11 @@ impl<'a, 'b, S: ResourceResolver> ResourceResolver for DeltaStorage<'a, 'b, S> { address: &AccountAddress, tag: &StructTag, metadata: &[Metadata], - ) -> Result>, Error> { + ) -> Result, u64)>> { if let Some(account_storage) = self.delta.accounts().get(address) { if let Some(blob_opt) = account_storage.resources().get(tag) { - return Ok(blob_opt.clone().ok()); + let buf = blob_opt.clone().ok(); + return Ok(resource_add_cost(buf, 0)); } } @@ -303,9 +304,10 @@ impl ResourceResolver for InMemoryStorage { address: &AccountAddress, tag: &StructTag, _metadata: &[Metadata], - ) -> Result>, Error> { + ) -> Result, u64)>> { if let Some(account_storage) = self.accounts.get(address) { - return Ok(account_storage.resources.get(tag).cloned()); + let buf = account_storage.resources.get(tag).cloned(); + return Ok(resource_add_cost(buf, 0)); } Ok(None) } diff --git a/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs b/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs index fb1a85de9472d..04489f2f8254d 100644 --- a/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs +++ b/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs @@ -17,7 +17,7 @@ use move_core_types::{ language_storage::{ModuleId, StructTag, TypeTag}, metadata::Metadata, parser, - resolver::{ModuleResolver, ResourceResolver}, + resolver::{resource_add_cost, ModuleResolver, ResourceResolver}, }; use move_disassembler::disassembler::Disassembler; use move_ir_types::location::Spanned; @@ -418,8 +418,9 @@ impl ResourceResolver for OnDiskStateView { address: &AccountAddress, struct_tag: &StructTag, _metadata: &[Metadata], - ) -> Result>, anyhow::Error> { - self.get_resource_bytes(*address, struct_tag.clone()) + ) -> Result, u64)>> { + let buf = self.get_resource_bytes(*address, struct_tag.clone())?; + Ok(resource_add_cost(buf, 0)) } } From 6f4fa663559ed1498337f65cbd343d511a3950b1 Mon Sep 17 00:00:00 2001 From: Victor Gao <10379359+vgao1996@users.noreply.github.com> Date: Wed, 7 Jun 2023 16:08:41 -0700 Subject: [PATCH 10/16] cherry-pick: [gas] fix gas metering for resource groups (#8549) (#8580) --- .../aptos-gas-profiling/src/profiler.rs | 6 +++-- aptos-move/aptos-gas/src/gas_meter.rs | 10 ++++--- .../aptos-gas/src/transaction/storage.rs | 20 +++++++------- aptos-move/aptos-vm/src/data_cache.rs | 19 ++++++++------ aptos-move/framework/src/natives/object.rs | 10 +------ .../async/move-async-vm/tests/testsuite.rs | 7 ++--- .../move/move-core/types/src/resolver.rs | 17 +++++------- .../src/tests/bad_storage_tests.rs | 2 +- .../move/move-vm/runtime/src/data_cache.rs | 26 +++++++++---------- .../move/move-vm/runtime/src/interpreter.rs | 22 +++++----------- .../move-vm/runtime/src/native_functions.rs | 2 +- .../move/move-vm/runtime/src/session.rs | 2 +- .../src/unit_tests/vm_arguments_tests.rs | 4 +-- .../move-vm/test-utils/src/gas_schedule.rs | 3 ++- .../move/move-vm/test-utils/src/storage.rs | 18 +++++++------ third_party/move/move-vm/types/src/gas.rs | 8 +++--- .../src/sandbox/utils/on_disk_state_view.rs | 7 ++--- 17 files changed, 89 insertions(+), 94 deletions(-) diff --git a/aptos-move/aptos-gas-profiling/src/profiler.rs b/aptos-move/aptos-gas-profiling/src/profiler.rs index c682cd882ddb6..e96a9819d4b31 100644 --- a/aptos-move/aptos-gas-profiling/src/profiler.rs +++ b/aptos-move/aptos-gas-profiling/src/profiler.rs @@ -428,11 +428,13 @@ where &mut self, addr: AccountAddress, ty: impl TypeView, - loaded: Option<(NumBytes, impl ValueView)>, + val: Option, + bytes_loaded: NumBytes, ) -> PartialVMResult<()> { let ty_tag = ty.to_type_tag(); - let (cost, res) = self.delegate_charge(|base| base.charge_load_resource(addr, ty, loaded)); + let (cost, res) = + self.delegate_charge(|base| base.charge_load_resource(addr, ty, val, bytes_loaded)); self.active_event_stream() .push(ExecutionGasEvent::LoadResource { diff --git a/aptos-move/aptos-gas/src/gas_meter.rs b/aptos-move/aptos-gas/src/gas_meter.rs index 3851f84aef59e..610af8fae0eb6 100644 --- a/aptos-move/aptos-gas/src/gas_meter.rs +++ b/aptos-move/aptos-gas/src/gas_meter.rs @@ -491,11 +491,12 @@ impl MoveGasMeter for StandardGasMeter { &mut self, _addr: AccountAddress, _ty: impl TypeView, - loaded: Option<(NumBytes, impl ValueView)>, + val: Option, + bytes_loaded: NumBytes, ) -> PartialVMResult<()> { if self.feature_version != 0 { // TODO(Gas): Rewrite this in a better way. - if let Some((_, val)) = &loaded { + if let Some(val) = &val { self.use_heap_memory( self.gas_params .misc @@ -504,10 +505,13 @@ impl MoveGasMeter for StandardGasMeter { )?; } } + if self.feature_version <= 8 && val.is_none() && bytes_loaded != 0.into() { + return Err(PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR).with_message("in legacy versions, number of bytes loaded must be zero when the resource does not exist ".to_string())); + } let cost = self .storage_gas_params .pricing - .calculate_read_gas(loaded.map(|(num_bytes, _)| num_bytes)); + .calculate_read_gas(val.is_some(), bytes_loaded); self.charge_io(cost) } diff --git a/aptos-move/aptos-gas/src/transaction/storage.rs b/aptos-move/aptos-gas/src/transaction/storage.rs index 5f572ea1a284f..36dccd154ebf1 100644 --- a/aptos-move/aptos-gas/src/transaction/storage.rs +++ b/aptos-move/aptos-gas/src/transaction/storage.rs @@ -145,12 +145,8 @@ impl StoragePricingV2 { } } - fn calculate_read_gas(&self, loaded: Option) -> InternalGas { - self.per_item_read * (NumArgs::from(1)) - + match loaded { - Some(num_bytes) => self.per_byte_read * num_bytes, - None => 0.into(), - } + fn calculate_read_gas(&self, loaded: NumBytes) -> InternalGas { + self.per_item_read * (NumArgs::from(1)) + self.per_byte_read * loaded } fn io_gas_per_write(&self, key: &StateKey, op: &WriteOp) -> InternalGas { @@ -177,12 +173,18 @@ pub enum StoragePricing { } impl StoragePricing { - pub fn calculate_read_gas(&self, loaded: Option) -> InternalGas { + pub fn calculate_read_gas(&self, resource_exists: bool, bytes_loaded: NumBytes) -> InternalGas { use StoragePricing::*; match self { - V1(v1) => v1.calculate_read_gas(loaded), - V2(v2) => v2.calculate_read_gas(loaded), + V1(v1) => v1.calculate_read_gas( + if resource_exists { + Some(bytes_loaded) + } else { + None + }, + ), + V2(v2) => v2.calculate_read_gas(bytes_loaded), } } diff --git a/aptos-move/aptos-vm/src/data_cache.rs b/aptos-move/aptos-vm/src/data_cache.rs index 9e6c0954e0bd2..7f75f3d4d8bcb 100644 --- a/aptos-move/aptos-vm/src/data_cache.rs +++ b/aptos-move/aptos-vm/src/data_cache.rs @@ -21,7 +21,7 @@ use move_core_types::{ account_address::AccountAddress, language_storage::{ModuleId, StructTag}, metadata::Metadata, - resolver::{resource_add_cost, ModuleResolver, ResourceResolver}, + resolver::{resource_size, ModuleResolver, ResourceResolver}, vm_status::StatusCode, }; use move_table_extension::{TableHandle, TableResolver}; @@ -94,7 +94,7 @@ impl<'a, S: StateView> StorageAdapter<'a, S> { address: &AccountAddress, struct_tag: &StructTag, metadata: &[Metadata], - ) -> Result, u64)>, VMError> { + ) -> Result<(Option>, usize), VMError> { let resource_group = get_resource_group_from_metadata(struct_tag, metadata); if let Some(resource_group) = resource_group { let mut cache = self.resource_group_cache.borrow_mut(); @@ -103,12 +103,13 @@ impl<'a, S: StateView> StorageAdapter<'a, S> { // This resource group is already cached for this address. So just return the // cached value. let buf = group_data.get(struct_tag).cloned(); - return Ok(resource_add_cost(buf, 0)); + let buf_size = resource_size(&buf); + return Ok((buf, buf_size)); } let group_data = self.get_resource_group_data(address, &resource_group)?; if let Some(group_data) = group_data { let len = if self.accurate_byte_count { - group_data.len() as u64 + group_data.len() } else { 0 }; @@ -118,15 +119,17 @@ impl<'a, S: StateView> StorageAdapter<'a, S> { .finish(Location::Undefined) })?; let res = group_data.get(struct_tag).cloned(); + let res_size = resource_size(&res); cache.insert(resource_group, group_data); - Ok(resource_add_cost(res, len)) + Ok((res, res_size + len)) } else { cache.insert(resource_group, BTreeMap::new()); - Ok(None) + Ok((None, 0)) } } else { let buf = self.get_standard_resource(address, struct_tag)?; - Ok(resource_add_cost(buf, 0)) + let buf_size = resource_size(&buf); + Ok((buf, buf_size)) } } } @@ -165,7 +168,7 @@ impl<'a, S: StateView> ResourceResolver for StorageAdapter<'a, S> { address: &AccountAddress, struct_tag: &StructTag, metadata: &[Metadata], - ) -> anyhow::Result, u64)>> { + ) -> anyhow::Result<(Option>, usize)> { Ok(self.get_any_resource(address, struct_tag, metadata)?) } } diff --git a/aptos-move/framework/src/natives/object.rs b/aptos-move/framework/src/natives/object.rs index d5c4fd6b869ba..169e25ec52b2b 100644 --- a/aptos-move/framework/src/natives/object.rs +++ b/aptos-move/framework/src/natives/object.rs @@ -53,15 +53,7 @@ fn native_exists_at( })?; if let Some(num_bytes) = num_bytes { - match num_bytes { - Some(num_bytes) => { - context - .charge(gas_params.per_item_loaded + num_bytes * gas_params.per_byte_loaded)?; - }, - None => { - context.charge(gas_params.per_item_loaded)?; - }, - } + context.charge(gas_params.per_item_loaded + num_bytes * gas_params.per_byte_loaded)?; } Ok(smallvec![Value::bool(exists)]) diff --git a/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs b/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs index 46d8dde897456..28bec3e0cb2cc 100644 --- a/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs +++ b/third_party/move/extensions/async/move-async-vm/tests/testsuite.rs @@ -23,7 +23,7 @@ use move_core_types::{ identifier::{IdentStr, Identifier}, language_storage::{ModuleId, StructTag}, metadata::Metadata, - resolver::{resource_add_cost, ModuleResolver, ResourceResolver}, + resolver::{resource_size, ModuleResolver, ResourceResolver}, }; use move_prover_test_utils::{baseline_test::verify_or_update_baseline, extract_test_directives}; use move_vm_test_utils::gas_schedule::GasStatus; @@ -398,14 +398,15 @@ impl<'a> ResourceResolver for HarnessProxy<'a> { address: &AccountAddress, typ: &StructTag, _metadata: &[Metadata], - ) -> anyhow::Result, u64)>> { + ) -> anyhow::Result<(Option>, usize)> { let res = self .harness .resource_store .borrow() .get(&(*address, typ.clone())) .cloned(); - Ok(resource_add_cost(res, 0)) + let res_size = resource_size(&res); + Ok((res, res_size)) } } diff --git a/third_party/move/move-core/types/src/resolver.rs b/third_party/move/move-core/types/src/resolver.rs index 5b8e5f405be49..5a43faaced949 100644 --- a/third_party/move/move-core/types/src/resolver.rs +++ b/third_party/move/move-core/types/src/resolver.rs @@ -26,6 +26,10 @@ pub trait ModuleResolver { fn get_module(&self, id: &ModuleId) -> Result>, Error>; } +pub fn resource_size(resource: &Option>) -> usize { + resource.as_ref().map(|bytes| bytes.len()).unwrap_or(0) +} + /// A persistent storage backend that can resolve resources by address + type /// Storage backends should return /// - Ok(Some(..)) if the data exists @@ -41,14 +45,7 @@ pub trait ResourceResolver { address: &AccountAddress, typ: &StructTag, metadata: &[Metadata], - ) -> Result, u64)>, Error>; -} - -pub fn resource_add_cost(buf: Option>, extra: u64) -> Option<(Vec, u64)> { - buf.map(|b| { - let len = b.len() as u64 + extra; - (b, len) - }) + ) -> Result<(Option>, usize), Error>; } /// A persistent storage implementation that can resolve both resources and modules @@ -60,7 +57,7 @@ pub trait MoveResolver: ModuleResolver + ResourceResolver { ) -> Result>, Error> { Ok(self .get_resource_with_metadata(address, typ, &self.get_module_metadata(&typ.module_id()))? - .map(|(buf, _)| buf)) + .0) } } @@ -72,7 +69,7 @@ impl ResourceResolver for &T { address: &AccountAddress, tag: &StructTag, metadata: &[Metadata], - ) -> Result, u64)>, Error> { + ) -> Result<(Option>, usize), Error> { (**self).get_resource_with_metadata(address, tag, metadata) } } diff --git a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs index 391fa5a027b1a..9583165a268e2 100644 --- a/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs +++ b/third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs @@ -526,7 +526,7 @@ impl ResourceResolver for BogusStorage { _address: &AccountAddress, _tag: &StructTag, _metadata: &[Metadata], - ) -> anyhow::Result, u64)>> { + ) -> anyhow::Result<(Option>, usize)> { Ok(Err( PartialVMError::new(self.bad_status_code).finish(Location::Undefined) )?) diff --git a/third_party/move/move-vm/runtime/src/data_cache.rs b/third_party/move/move-vm/runtime/src/data_cache.rs index 528d6d7b29ee7..a4e826ff3e109 100644 --- a/third_party/move/move-vm/runtime/src/data_cache.rs +++ b/third_party/move/move-vm/runtime/src/data_cache.rs @@ -157,7 +157,7 @@ impl<'r> TransactionDataCache<'r> { map.get_mut(k).unwrap() } - // Retrieve data from the local cache or loads it from the remote cache into the local cache. + // Retrieves data from the local cache or loads it from the remote cache into the local cache. // All operations on the global data are based on this API and they all load the data // into the cache. pub(crate) fn load_resource( @@ -165,7 +165,7 @@ impl<'r> TransactionDataCache<'r> { loader: &Loader, addr: AccountAddress, ty: &Type, - ) -> PartialVMResult<(&mut GlobalValue, Option>)> { + ) -> PartialVMResult<(&mut GlobalValue, Option)> { let account_cache = Self::get_mut_or_insert_with(&mut self.account_map, &addr, || { (addr, AccountDataCache::new()) }); @@ -189,12 +189,17 @@ impl<'r> TransactionDataCache<'r> { None => &[], }; - let gv = match self + let (data, bytes_loaded) = self .remote .get_resource_with_metadata(&addr, &ty_tag, metadata) - { - Ok(Some((blob, bytes))) => { - load_res = Some(Some(NumBytes::new(bytes))); + .map_err(|err| { + let msg = format!("Unexpected storage error: {:?}", err); + PartialVMError::new(StatusCode::STORAGE_ERROR).with_message(msg) + })?; + load_res = Some(NumBytes::new(bytes_loaded as u64)); + + let gv = match data { + Some(blob) => { let val = match Value::simple_deserialize(&blob, &ty_layout) { Some(val) => val, None => { @@ -209,14 +214,7 @@ impl<'r> TransactionDataCache<'r> { GlobalValue::cached(val)? }, - Ok(None) => { - load_res = Some(None); - GlobalValue::none() - }, - Err(err) => { - let msg = format!("Unexpected storage error: {:?}", err); - return Err(PartialVMError::new(StatusCode::STORAGE_ERROR).with_message(msg)); - }, + None => GlobalValue::none(), }; account_cache.data_map.insert(ty.clone(), (ty_layout, gv)); diff --git a/third_party/move/move-vm/runtime/src/interpreter.rs b/third_party/move/move-vm/runtime/src/interpreter.rs index b2c8ee03884bf..37352abf4ef01 100644 --- a/third_party/move/move-vm/runtime/src/interpreter.rs +++ b/third_party/move/move-vm/runtime/src/interpreter.rs @@ -544,21 +544,13 @@ impl Interpreter { ) -> PartialVMResult<&'c mut GlobalValue> { match data_store.load_resource(loader, addr, ty) { Ok((gv, load_res)) => { - if let Some(loaded) = load_res { - let opt = match loaded { - Some(num_bytes) => { - let view = gv.view().ok_or_else(|| { - PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) - .with_message( - "Failed to create view for global value".to_owned(), - ) - })?; - - Some((num_bytes, view)) - }, - None => None, - }; - gas_meter.charge_load_resource(addr, TypeWithLoader { ty, loader }, opt)?; + if let Some(bytes_loaded) = load_res { + gas_meter.charge_load_resource( + addr, + TypeWithLoader { ty, loader }, + gv.view(), + bytes_loaded, + )?; } Ok(gv) }, diff --git a/third_party/move/move-vm/runtime/src/native_functions.rs b/third_party/move/move-vm/runtime/src/native_functions.rs index 3446e6e4ffd5e..d93a593231690 100644 --- a/third_party/move/move-vm/runtime/src/native_functions.rs +++ b/third_party/move/move-vm/runtime/src/native_functions.rs @@ -128,7 +128,7 @@ impl<'a, 'b, 'c> NativeContext<'a, 'b, 'c> { &mut self, address: AccountAddress, type_: &Type, - ) -> VMResult<(bool, Option>)> { + ) -> VMResult<(bool, Option)> { let (value, num_bytes) = self .data_store .load_resource(self.resolver.loader(), address, type_) diff --git a/third_party/move/move-vm/runtime/src/session.rs b/third_party/move/move-vm/runtime/src/session.rs index 1f35f27817cb8..ad698c656ae96 100644 --- a/third_party/move/move-vm/runtime/src/session.rs +++ b/third_party/move/move-vm/runtime/src/session.rs @@ -283,7 +283,7 @@ impl<'r, 'l> Session<'r, 'l> { &mut self, addr: AccountAddress, ty: &Type, - ) -> PartialVMResult<(&mut GlobalValue, Option>)> { + ) -> PartialVMResult<(&mut GlobalValue, Option)> { self.data_cache .load_resource(self.move_vm.runtime.loader(), addr, ty) } diff --git a/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs b/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs index 0dea6cca8c0b1..b7d2c457fe536 100644 --- a/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs +++ b/third_party/move/move-vm/runtime/src/unit_tests/vm_arguments_tests.rs @@ -265,8 +265,8 @@ impl ResourceResolver for RemoteStore { _address: &AccountAddress, _tag: &StructTag, _metadata: &[Metadata], - ) -> anyhow::Result, u64)>> { - Ok(None) + ) -> anyhow::Result<(Option>, usize)> { + Ok((None, 0)) } } diff --git a/third_party/move/move-vm/test-utils/src/gas_schedule.rs b/third_party/move/move-vm/test-utils/src/gas_schedule.rs index 7394a2c306b7a..3a0d918ca18d2 100644 --- a/third_party/move/move-vm/test-utils/src/gas_schedule.rs +++ b/third_party/move/move-vm/test-utils/src/gas_schedule.rs @@ -355,7 +355,8 @@ impl<'b> GasMeter for GasStatus<'b> { &mut self, _addr: AccountAddress, _ty: impl TypeView, - _loaded: Option<(NumBytes, impl ValueView)>, + _val: Option, + _bytes_loaded: NumBytes, ) -> PartialVMResult<()> { Ok(()) } diff --git a/third_party/move/move-vm/test-utils/src/storage.rs b/third_party/move/move-vm/test-utils/src/storage.rs index 5f596e097ae2e..47b3cd80c61ef 100644 --- a/third_party/move/move-vm/test-utils/src/storage.rs +++ b/third_party/move/move-vm/test-utils/src/storage.rs @@ -9,7 +9,7 @@ use move_core_types::{ identifier::Identifier, language_storage::{ModuleId, StructTag}, metadata::Metadata, - resolver::{resource_add_cost, ModuleResolver, MoveResolver, ResourceResolver}, + resolver::{resource_size, ModuleResolver, MoveResolver, ResourceResolver}, }; #[cfg(feature = "table-extension")] use move_table_extension::{TableChangeSet, TableHandle, TableResolver}; @@ -44,8 +44,8 @@ impl ResourceResolver for BlankStorage { _address: &AccountAddress, _tag: &StructTag, _metadata: &[Metadata], - ) -> Result, u64)>> { - Ok(None) + ) -> Result<(Option>, usize)> { + Ok((None, 0)) } } @@ -90,11 +90,12 @@ impl<'a, 'b, S: ResourceResolver> ResourceResolver for DeltaStorage<'a, 'b, S> { address: &AccountAddress, tag: &StructTag, metadata: &[Metadata], - ) -> Result, u64)>> { + ) -> Result<(Option>, usize)> { if let Some(account_storage) = self.delta.accounts().get(address) { if let Some(blob_opt) = account_storage.resources().get(tag) { let buf = blob_opt.clone().ok(); - return Ok(resource_add_cost(buf, 0)); + let buf_size = resource_size(&buf); + return Ok((buf, buf_size)); } } @@ -304,12 +305,13 @@ impl ResourceResolver for InMemoryStorage { address: &AccountAddress, tag: &StructTag, _metadata: &[Metadata], - ) -> Result, u64)>> { + ) -> Result<(Option>, usize)> { if let Some(account_storage) = self.accounts.get(address) { let buf = account_storage.resources.get(tag).cloned(); - return Ok(resource_add_cost(buf, 0)); + let buf_size = resource_size(&buf); + return Ok((buf, buf_size)); } - Ok(None) + Ok((None, 0)) } } diff --git a/third_party/move/move-vm/types/src/gas.rs b/third_party/move/move-vm/types/src/gas.rs index 9afb608d2020d..5cd0c01d776aa 100644 --- a/third_party/move/move-vm/types/src/gas.rs +++ b/third_party/move/move-vm/types/src/gas.rs @@ -264,8 +264,6 @@ pub trait GasMeter { /// Charges for loading a resource from storage. This is only called when the resource is not /// cached. - /// - `Some(n)` means `n` bytes are loaded. - /// - `None` means a load operation is performed but the resource does not exist. /// /// WARNING: This can be dangerous if you execute multiple user transactions in the same /// session -- identical transactions can have different gas costs. Use at your own risk. @@ -273,7 +271,8 @@ pub trait GasMeter { &mut self, addr: AccountAddress, ty: impl TypeView, - loaded: Option<(NumBytes, impl ValueView)>, + val: Option, + bytes_loaded: NumBytes, ) -> PartialVMResult<()>; /// Charge for executing a native function. @@ -501,7 +500,8 @@ impl GasMeter for UnmeteredGasMeter { &mut self, _addr: AccountAddress, _ty: impl TypeView, - _loaded: Option<(NumBytes, impl ValueView)>, + _val: Option, + _bytes_loaded: NumBytes, ) -> PartialVMResult<()> { Ok(()) } diff --git a/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs b/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs index 04489f2f8254d..7cd608489522a 100644 --- a/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs +++ b/third_party/move/tools/move-cli/src/sandbox/utils/on_disk_state_view.rs @@ -17,7 +17,7 @@ use move_core_types::{ language_storage::{ModuleId, StructTag, TypeTag}, metadata::Metadata, parser, - resolver::{resource_add_cost, ModuleResolver, ResourceResolver}, + resolver::{resource_size, ModuleResolver, ResourceResolver}, }; use move_disassembler::disassembler::Disassembler; use move_ir_types::location::Spanned; @@ -418,9 +418,10 @@ impl ResourceResolver for OnDiskStateView { address: &AccountAddress, struct_tag: &StructTag, _metadata: &[Metadata], - ) -> Result, u64)>> { + ) -> Result<(Option>, usize)> { let buf = self.get_resource_bytes(*address, struct_tag.clone())?; - Ok(resource_add_cost(buf, 0)) + let buf_size = resource_size(&buf); + Ok((buf, buf_size)) } } From b03517fe92a5695d774e12d864d94b40f8194c89 Mon Sep 17 00:00:00 2001 From: Sherry Xiao Date: Thu, 8 Jun 2023 08:08:31 -0700 Subject: [PATCH 11/16] Integrate fixes into mainnet (#8568) (#8582) Co-authored-by: gerben-stavenga <54682677+gerben-stavenga@users.noreply.github.com> --- .../verifier/transaction_arg_validation.rs | 217 ++++++++++++------ .../aptos-vm/src/verifier/view_function.rs | 50 +--- .../pack/sources/args_test.move | 19 ++ .../src/tests/constructor_args.rs | 22 ++ 4 files changed, 197 insertions(+), 111 deletions(-) diff --git a/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs b/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs index 3e69987cd8ac9..ec00229ebe57d 100644 --- a/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs +++ b/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs @@ -7,7 +7,11 @@ //! for strings whether they consist of correct characters. use crate::{move_vm_ext::SessionExt, VMStatus}; -use move_binary_format::{errors::VMError, file_format_common::read_uleb128_as_u64}; +use move_binary_format::{ + errors::{Location, PartialVMError}, + file_format::FunctionDefinitionIndex, + file_format_common::read_uleb128_as_u64, +}; use move_core_types::{ account_address::AccountAddress, ident_str, @@ -123,10 +127,9 @@ pub(crate) fn validate_combine_signer_and_txn_args( } let allowed_structs = get_allowed_structs(are_struct_constructors_enabled); - // validate all non_signer params - let mut needs_construction = vec![]; - for (idx, ty) in func.parameters[signer_param_cnt..].iter().enumerate() { - let (valid, construction) = is_valid_txn_arg( + // Need to keep this here to ensure we return the historic correct error code for replay + for ty in func.parameters[signer_param_cnt..].iter() { + let valid = is_valid_txn_arg( session, &ty.subst(&func.type_arguments).unwrap(), allowed_structs, @@ -137,9 +140,6 @@ pub(crate) fn validate_combine_signer_and_txn_args( None, )); } - if construction { - needs_construction.push(idx + signer_param_cnt); - } } if (signer_param_cnt + args.len()) != func.parameters.len() { @@ -148,10 +148,20 @@ pub(crate) fn validate_combine_signer_and_txn_args( None, )); } + + let args = construct_args( + session, + &func.parameters[signer_param_cnt..], + args, + &func.type_arguments, + allowed_structs, + false, + )?; + // if function doesn't require signer, we reuse txn args // if the function require signer, we check senders number same as signers // and then combine senders with txn args. - let mut combined_args = if signer_param_cnt == 0 { + let combined_args = if signer_param_cnt == 0 { args } else { // the number of txn senders should be the same number of signers @@ -167,15 +177,6 @@ pub(crate) fn validate_combine_signer_and_txn_args( .chain(args) .collect() }; - if !needs_construction.is_empty() { - construct_args( - session, - &needs_construction, - &mut combined_args, - func, - allowed_structs, - )?; - } Ok(combined_args) } @@ -184,21 +185,21 @@ pub(crate) fn is_valid_txn_arg( session: &SessionExt, typ: &Type, allowed_structs: &ConstructorMap, -) -> (bool, bool) { +) -> bool { use move_vm_types::loaded_data::runtime_types::Type::*; match typ { - Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address => (true, false), + Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address => true, Vector(inner) => is_valid_txn_arg(session, inner, allowed_structs), Struct(idx) | StructInstantiation(idx, _) => { if let Some(st) = session.get_struct_type(*idx) { let full_name = format!("{}::{}", st.module.short_str_lossless(), st.name); - (allowed_structs.contains_key(&full_name), true) + allowed_structs.contains_key(&full_name) } else { - (false, false) + false } }, - Signer | Reference(_) | MutableReference(_) | TyParam(_) => (false, false), + Signer | Reference(_) | MutableReference(_) | TyParam(_) => false, } } @@ -207,41 +208,81 @@ pub(crate) fn is_valid_txn_arg( // TODO: This needs a more solid story and a tighter integration with the VM. pub(crate) fn construct_args( session: &mut SessionExt, - idxs: &[usize], - args: &mut [Vec], - func: &LoadedFunctionInstantiation, + types: &[Type], + args: Vec>, + ty_args: &[Type], allowed_structs: &ConstructorMap, -) -> Result<(), VMStatus> { + is_view: bool, +) -> Result>, VMStatus> { // Perhaps in a future we should do proper gas metering here let mut gas_meter = UnmeteredGasMeter; - for (idx, ty) in func.parameters.iter().enumerate() { - if !idxs.contains(&idx) { - continue; - } - let arg = &mut args[idx]; - let mut cursor = Cursor::new(&arg[..]); - let mut new_arg = vec![]; - recursively_construct_arg( + let mut res_args = vec![]; + if types.len() != args.len() { + return Err(invalid_signature()); + } + for (ty, arg) in types.iter().zip(args.into_iter()) { + let arg = construct_arg( session, - &ty.subst(&func.type_arguments).unwrap(), + &ty.subst(ty_args).unwrap(), allowed_structs, - &mut cursor, + arg, &mut gas_meter, - &mut new_arg, + is_view, )?; - // Check cursor has parsed everything - // Unfortunately, is_empty is only enabled in nightly, so we check this way. - if cursor.position() != arg.len() as u64 { - return Err(VMStatus::Error( - StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT, - Some(String::from( - "The serialized arguments to constructor contained extra data", - )), - )); - } - *arg = new_arg; + res_args.push(arg); + } + Ok(res_args) +} + +fn invalid_signature() -> VMStatus { + VMStatus::Error(StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE, None) +} + +fn construct_arg( + session: &mut SessionExt, + ty: &Type, + allowed_structs: &ConstructorMap, + arg: Vec, + gas_meter: &mut impl GasMeter, + is_view: bool, +) -> Result, VMStatus> { + use move_vm_types::loaded_data::runtime_types::Type::*; + match ty { + Bool | U8 | U16 | U32 | U64 | U128 | U256 | Address => Ok(arg), + Vector(_) | Struct(_) | StructInstantiation(_, _) => { + let mut cursor = Cursor::new(&arg[..]); + let mut new_arg = vec![]; + let mut max_invocations = 10; // Read from config in the future + recursively_construct_arg( + session, + ty, + allowed_structs, + &mut cursor, + gas_meter, + &mut max_invocations, + &mut new_arg, + )?; + // Check cursor has parsed everything + // Unfortunately, is_empty is only enabled in nightly, so we check this way. + if cursor.position() != arg.len() as u64 { + return Err(VMStatus::Error( + StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT, + Some(String::from( + "The serialized arguments to constructor contained extra data", + )), + )); + } + Ok(new_arg) + }, + Signer => { + if is_view { + Ok(arg) + } else { + Err(invalid_signature()) + } + }, + Reference(_) | MutableReference(_) | TyParam(_) => Err(invalid_signature()), } - Ok(()) } // A Cursor is used to recursively walk the serialized arg manually and correctly. In effect we @@ -253,6 +294,7 @@ pub(crate) fn recursively_construct_arg( allowed_structs: &ConstructorMap, cursor: &mut Cursor<&[u8]>, gas_meter: &mut impl GasMeter, + max_invocations: &mut u64, arg: &mut Vec, ) -> Result<(), VMStatus> { use move_vm_types::loaded_data::runtime_types::Type::*; @@ -263,7 +305,15 @@ pub(crate) fn recursively_construct_arg( let mut len = get_len(cursor)?; serialize_uleb128(len, arg); while len > 0 { - recursively_construct_arg(session, inner, allowed_structs, cursor, gas_meter, arg)?; + recursively_construct_arg( + session, + inner, + allowed_structs, + cursor, + gas_meter, + max_invocations, + arg, + )?; len -= 1; } }, @@ -272,11 +322,11 @@ pub(crate) fn recursively_construct_arg( // performed in `is_valid_txn_arg` let st = session .get_struct_type(*idx) - .expect("unreachable, type must exist"); + .ok_or_else(invalid_signature)?; let full_name = format!("{}::{}", st.module.short_str_lossless(), st.name); let constructor = allowed_structs .get(&full_name) - .expect("unreachable: struct must be allowed"); + .ok_or_else(invalid_signature)?; // By appending the BCS to the output parameter we construct the correct BCS format // of the argument. arg.append(&mut validate_and_construct( @@ -286,6 +336,7 @@ pub(crate) fn recursively_construct_arg( allowed_structs, cursor, gas_meter, + max_invocations, )?); }, Bool | U8 => read_n_bytes(1, cursor, arg)?, @@ -294,11 +345,8 @@ pub(crate) fn recursively_construct_arg( U64 => read_n_bytes(8, cursor, arg)?, U128 => read_n_bytes(16, cursor, arg)?, U256 | Address => read_n_bytes(32, cursor, arg)?, - Signer | Reference(_) | MutableReference(_) | TyParam(_) => { - unreachable!("We already checked for this in is-valid-txn-arg") - }, + Signer | Reference(_) | MutableReference(_) | TyParam(_) => return Err(invalid_signature()), }; - Ok(()) } @@ -313,7 +361,45 @@ fn validate_and_construct( allowed_structs: &ConstructorMap, cursor: &mut Cursor<&[u8]>, gas_meter: &mut impl GasMeter, + max_invocations: &mut u64, ) -> Result, VMStatus> { + if *max_invocations == 0 { + return Err(VMStatus::Error( + StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT, + None, + )); + } + // HACK mitigation of performance attack + // To maintain compatibility with vector or so on, we need to allow unlimited strings. + // So we do not count the string constructor against the max_invocations, instead we + // shortcut the string case to avoid the performance attack. + if constructor.func_name.as_str() == "utf8" { + let constructor_error = || { + // A slight hack, to prevent additional piping of the feature flag through all + // function calls. We know the feature is active when more structs then just strings are + // allowed. + let are_struct_constructors_enabled = allowed_structs.len() > 1; + if are_struct_constructors_enabled { + PartialVMError::new(StatusCode::ABORTED) + .with_sub_status(1) + .at_code_offset(FunctionDefinitionIndex::new(0), 0) + .finish(Location::Module(constructor.module_id.clone())) + .into_vm_status() + } else { + VMStatus::Error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT, None) + } + }; + // short cut for the utf8 constructor, which is a special case + let len = get_len(cursor)?; + let mut arg = vec![]; + read_n_bytes(len, cursor, &mut arg)?; + std::str::from_utf8(&arg).map_err(|_| constructor_error())?; + return bcs::to_bytes(&arg) + .map_err(|_| VMStatus::Error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT, None)); + } else { + *max_invocations -= 1; + } + let (function, instantiation) = session.load_function_with_type_arg_inference( &constructor.module_id, constructor.func_name, @@ -328,24 +414,13 @@ fn validate_and_construct( allowed_structs, cursor, gas_meter, + max_invocations, &mut arg, )?; args.push(arg); } - let constructor_error = |e: VMError| { - // A slight hack, to prevent additional piping of the feature flag through all - // function calls. We know the feature is active when more structs then just strings are - // allowed. - let are_struct_constructors_enabled = allowed_structs.len() > 1; - if are_struct_constructors_enabled { - e.into_vm_status() - } else { - VMStatus::Error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT, None) - } - }; - let serialized_result = session - .execute_instantiated_function(function, instantiation, args, gas_meter) - .map_err(constructor_error)?; + let serialized_result = + session.execute_instantiated_function(function, instantiation, args, gas_meter)?; let mut ret_vals = serialized_result.return_values; // We know ret_vals.len() == 1 let deserialize_error = VMStatus::Error( diff --git a/aptos-move/aptos-vm/src/verifier/view_function.rs b/aptos-move/aptos-vm/src/verifier/view_function.rs index 1870fddcf9ddd..00cc38e0efd3c 100644 --- a/aptos-move/aptos-vm/src/verifier/view_function.rs +++ b/aptos-move/aptos-vm/src/verifier/view_function.rs @@ -9,7 +9,6 @@ use aptos_framework::RuntimeModuleMetadataV1; use move_binary_format::errors::{PartialVMError, PartialVMResult}; use move_core_types::{identifier::IdentStr, vm_status::StatusCode}; use move_vm_runtime::session::LoadedFunctionInstantiation; -use move_vm_types::loaded_data::runtime_types::Type; /// Based on the function attributes in the module metadata, determine whether a /// function is a view function. @@ -31,7 +30,7 @@ pub fn determine_is_view( /// function, and validates the arguments. pub(crate) fn validate_view_function( session: &mut SessionExt, - mut args: Vec>, + args: Vec>, fun_name: &IdentStr, fun_inst: &LoadedFunctionInstantiation, module_metadata: Option<&RuntimeModuleMetadataV1>, @@ -55,43 +54,14 @@ pub(crate) fn validate_view_function( } let allowed_structs = get_allowed_structs(struct_constructors_feature); - // Validate arguments. We allow all what transaction allows, in addition, signers can - // be passed. Some arguments (e.g. utf8 strings) need validation which happens here. - let mut needs_construction = vec![]; - for (idx, ty) in fun_inst.parameters.iter().enumerate() { - match ty { - Type::Signer => continue, - Type::Reference(inner_type) if matches!(&**inner_type, Type::Signer) => continue, - _ => { - let (valid, construction) = - transaction_arg_validation::is_valid_txn_arg(session, ty, allowed_structs); - if !valid { - return Err( - PartialVMError::new(StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE) - .with_message("invalid view function argument".to_string()), - ); - } - if construction { - needs_construction.push(idx); - } - }, - } - } - if !needs_construction.is_empty() - && transaction_arg_validation::construct_args( - session, - &needs_construction, - &mut args, - fun_inst, - allowed_structs, - ) - .is_err() - { - return Err( - PartialVMError::new(StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE) - .with_message("invalid view function argument: failed validation".to_string()), - ); - } - + let args = transaction_arg_validation::construct_args( + session, + &fun_inst.parameters, + args, + &fun_inst.type_arguments, + allowed_structs, + true, + ) + .map_err(|e| PartialVMError::new(e.status_code()))?; Ok(args) } diff --git a/aptos-move/e2e-move-tests/src/tests/constructor_args.data/pack/sources/args_test.move b/aptos-move/e2e-move-tests/src/tests/constructor_args.data/pack/sources/args_test.move index a5fbaed167e68..0dd613099c3c9 100644 --- a/aptos-move/e2e-move-tests/src/tests/constructor_args.data/pack/sources/args_test.move +++ b/aptos-move/e2e-move-tests/src/tests/constructor_args.data/pack/sources/args_test.move @@ -90,6 +90,25 @@ module 0xCAFE::test { }; } + // Valuable data that should not be able to be fabricated by a malicious tx + struct MyPrecious { + value: u64, + } + + public entry fun ensure_no_fabrication(my_precious: Option) { + if (std::option::is_none(&my_precious)) { + std::option::destroy_none(my_precious) + } else { + let MyPrecious { value : _ } = std::option::destroy_some(my_precious); + } + } + + public entry fun ensure_vector_vector_u8(o: Object, _: vector>) acquires ModuleData { + let addr = aptos_std::object::object_address(&o); + // guaranteed to exist + borrow_global_mut(addr).state = std::string::utf8(b"vector>"); + } + fun convert(x: u128): String { let s = std::vector::empty(); let ascii0 = 48; diff --git a/aptos-move/e2e-move-tests/src/tests/constructor_args.rs b/aptos-move/e2e-move-tests/src/tests/constructor_args.rs index 89e5275f063d7..f4fa91804d09d 100644 --- a/aptos-move/e2e-move-tests/src/tests/constructor_args.rs +++ b/aptos-move/e2e-move-tests/src/tests/constructor_args.rs @@ -156,6 +156,14 @@ fn constructor_args_good() { ], "pff vectors of optionals", ), + ( + "0xcafe::test::ensure_vector_vector_u8", + vec![ + bcs::to_bytes(&OBJECT_ADDRESS).unwrap(), // Object + bcs::to_bytes(&vec![vec![1u8], vec![2u8]]).unwrap(), // vector> + ], + "vector>", + ), ]; let mut h = MoveHarness::new_with_features(vec![FeatureFlag::STRUCT_CONSTRUCTORS], vec![]); @@ -228,6 +236,20 @@ fn constructor_args_bad() { ) }), ), + ( + "0xcafe::test::ensure_no_fabrication", + vec![ + bcs::to_bytes(&vec![1u64]).unwrap(), // Option + ], + Box::new(|e| { + matches!( + e, + TransactionStatus::Keep(ExecutionStatus::MiscellaneousError(Some( + StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE + ))) + ) + }), + ), ]; fail(tests); From dbaef4054bdd04e8a4a9d6519a97dedee34b194d Mon Sep 17 00:00:00 2001 From: Kevin <105028215+movekevin@users.noreply.github.com> Date: Wed, 14 Jun 2023 08:09:12 -0500 Subject: [PATCH 12/16] Fix the order of signer and non-signer tx arg validation to maintain backward compatibility (#8649) (#8659) --- .../verifier/transaction_arg_validation.rs | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs b/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs index ec00229ebe57d..3807eceb29e4d 100644 --- a/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs +++ b/aptos-move/aptos-vm/src/verifier/transaction_arg_validation.rs @@ -149,6 +149,20 @@ pub(crate) fn validate_combine_signer_and_txn_args( )); } + // If the invoked function expects one or more signers, we need to check that the number of + // signers actually passed is matching first to maintain backward compatibility before + // moving on to the validation of non-signer args. + // the number of txn senders should be the same number of signers + if signer_param_cnt > 0 && senders.len() != signer_param_cnt { + return Err(VMStatus::Error( + StatusCode::NUMBER_OF_SIGNER_ARGUMENTS_MISMATCH, + None, + )); + } + + // This also validates that the args are valid. If they are structs, they have to be allowed + // and must be constructed successfully. If construction fails, this would fail with a + // FAILED_TO_DESERIALIZE_ARGUMENT error. let args = construct_args( session, &func.parameters[signer_param_cnt..], @@ -158,19 +172,10 @@ pub(crate) fn validate_combine_signer_and_txn_args( false, )?; - // if function doesn't require signer, we reuse txn args - // if the function require signer, we check senders number same as signers - // and then combine senders with txn args. + // Combine signer and non-signer arguments. let combined_args = if signer_param_cnt == 0 { args } else { - // the number of txn senders should be the same number of signers - if senders.len() != signer_param_cnt { - return Err(VMStatus::Error( - StatusCode::NUMBER_OF_SIGNER_ARGUMENTS_MISMATCH, - None, - )); - } senders .into_iter() .map(|s| MoveValue::Signer(s).simple_serialize().unwrap()) From 3cf34648e9460f0e50e1fa66027d91450c0f3e97 Mon Sep 17 00:00:00 2001 From: Guoteng Rao <3603304+grao1991@users.noreply.github.com> Date: Wed, 14 Jun 2023 15:58:48 -0700 Subject: [PATCH 13/16] [CP][Fix] Return the correct version for state snapshot when the root is not committed. (#8672) --- storage/aptosdb/src/state_merkle_db.rs | 15 ++++-- .../src/state_store/state_store_test.rs | 50 +++++++++++++++++-- 2 files changed, 59 insertions(+), 6 deletions(-) diff --git a/storage/aptosdb/src/state_merkle_db.rs b/storage/aptosdb/src/state_merkle_db.rs index 57ddd55afd293..2a81d6080dd81 100644 --- a/storage/aptosdb/src/state_merkle_db.rs +++ b/storage/aptosdb/src/state_merkle_db.rs @@ -257,9 +257,18 @@ impl StateMerkleDb { .rev_iter::(Default::default())?; iter.seek_for_prev(&NodeKey::new_empty_path(max_possible_version))?; if let Some((key, _node)) = iter.next().transpose()? { - // TODO: If we break up a single update batch to multiple commits, we would need to - // deal with a partial version, which hasn't got the root committed. - return Ok(Some(key.version())); + let version = key.version(); + if self + .metadata_db() + .get::(&NodeKey::new_empty_path(version))? + .is_some() + { + return Ok(Some(version)); + } + // Since we split state merkle commit into multiple batches, it's possible that + // the root is not committed yet. In this case we need to look at the previous + // root. + return self.get_state_snapshot_version_before(version); } } // No version before genesis. diff --git a/storage/aptosdb/src/state_store/state_store_test.rs b/storage/aptosdb/src/state_store/state_store_test.rs index af57d09fec2f3..630564dcd0d61 100644 --- a/storage/aptosdb/src/state_store/state_store_test.rs +++ b/storage/aptosdb/src/state_store/state_store_test.rs @@ -4,18 +4,23 @@ use super::*; use crate::{ + jellyfish_merkle_node::JellyfishMerkleNodeSchema, new_sharded_kv_schema_batch, state_restore::StateSnapshotRestore, test_helper::{arb_state_kv_sets, update_store}, AptosDB, }; -use aptos_jellyfish_merkle::TreeReader; +use aptos_jellyfish_merkle::{ + node_type::{Node, NodeKey}, + TreeReader, +}; use aptos_storage_interface::{ jmt_update_refs, jmt_updates, DbReader, DbWriter, StateSnapshotReceiver, }; use aptos_temppath::TempPath; use aptos_types::{ - access_path::AccessPath, account_address::AccountAddress, state_store::state_key::StateKeyTag, + access_path::AccessPath, account_address::AccountAddress, nibble::nibble_path::NibblePath, + state_store::state_key::StateKeyTag, }; use proptest::{collection::hash_map, prelude::*}; use std::collections::HashMap; @@ -287,7 +292,46 @@ pub fn test_get_state_snapshot_before() { assert_eq!(store.get_state_snapshot_before(3).unwrap(), Some((2, hash))); assert_eq!(store.get_state_snapshot_before(2).unwrap(), Some((0, hash))); assert_eq!(store.get_state_snapshot_before(1).unwrap(), Some((0, hash))); - assert_eq!(store.get_state_snapshot_before(0).unwrap(), None,); + assert_eq!(store.get_state_snapshot_before(0).unwrap(), None); + + // Only consider a version as available when the root node is there. + // Here we are adding another non-root node, and removing the root node, to verify if there is + // a node at version X but the root node at version X doesn't exist, we shouldn't return + // version X. + let batch = SchemaBatch::new(); + batch + .put::( + &NodeKey::new(2, NibblePath::new_odd(vec![0])), + &Node::Null, + ) + .unwrap(); + db.state_merkle_db + .metadata_db() + .write_schemas(batch) + .unwrap(); + + assert_eq!( + db.state_merkle_db + .get_state_snapshot_version_before(4) + .unwrap(), + Some(2) + ); + + let batch = SchemaBatch::new(); + batch + .delete::(&NodeKey::new_empty_path(2)) + .unwrap(); + db.state_merkle_db + .metadata_db() + .write_schemas(batch) + .unwrap(); + + assert_eq!( + db.state_merkle_db + .get_state_snapshot_version_before(4) + .unwrap(), + Some(0) + ); } proptest! { From 4fbd3d72130c361f54ded13c734400e5ed170bc1 Mon Sep 17 00:00:00 2001 From: Sherry Xiao Date: Wed, 14 Jun 2023 19:34:40 -0700 Subject: [PATCH 14/16] [aptos-vm] Skip converting storage error (#8675) Co-authored-by: Runtian Zhou --- aptos-move/aptos-vm/src/errors.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/aptos-move/aptos-vm/src/errors.rs b/aptos-move/aptos-vm/src/errors.rs index 49dc6796a2329..919748dc28a14 100644 --- a/aptos-move/aptos-vm/src/errors.rs +++ b/aptos-move/aptos-vm/src/errors.rs @@ -132,6 +132,8 @@ pub fn convert_prologue_error( }; VMStatus::Error(new_major_status, None) }, + // Storage error can be a result of speculation failure so throw the error back for caller to handle. + e @ VMStatus::Error(StatusCode::STORAGE_ERROR, _) => e, status @ VMStatus::ExecutionFailure { .. } | status @ VMStatus::Error(..) => { speculative_error!( log_context, @@ -176,7 +178,8 @@ pub fn convert_epilogue_error( VMStatus::Error(StatusCode::UNEXPECTED_ERROR_FROM_KNOWN_MOVE_FUNCTION, None) }, }, - + // Storage error can be a result of speculation failure so throw the error back for caller to handle. + e @ VMStatus::Error(StatusCode::STORAGE_ERROR, _) => e, status => { speculative_error!( log_context, @@ -198,7 +201,8 @@ pub fn expect_only_successful_execution( let status = error.into_vm_status(); Err(match status { VMStatus::Executed => VMStatus::Executed, - + // Storage error can be a result of speculation failure so throw the error back for caller to handle. + e @ VMStatus::Error(StatusCode::STORAGE_ERROR, _) => e, status => { // Only trigger a warning here as some errors could be a result of the speculative parallel execution. // We will report the errors after we obtained the final transaction output in update_counters_for_processed_chunk From d19e3d91639ba0251606b30a3b2e1164a671eca4 Mon Sep 17 00:00:00 2001 From: perryjrandall Date: Fri, 16 Jun 2023 10:12:05 -0700 Subject: [PATCH 15/16] [release] Fix framework release description to v1.5.0 (#8704) * [release] Fix framework release description to v1.5.0 Test plan: is valid yaml :P * update with steps and right metadata --------- Co-authored-by: Frances Liu --- .../aptos-release-builder/data/release.yaml | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/aptos-move/aptos-release-builder/data/release.yaml b/aptos-move/aptos-release-builder/data/release.yaml index 4b2dbd544421b..31c2acf7a47aa 100644 --- a/aptos-move/aptos-release-builder/data/release.yaml +++ b/aptos-move/aptos-release-builder/data/release.yaml @@ -2,20 +2,23 @@ remote_endpoint: ~ name: "v1.5" proposals: - - name: upgrade_framework + - name: step_1_upgrade_framework metadata: - title: "Multi-step proposal to upgrade mainnet framework to v1.4.0" - description: "This includes changes outlined in https://github.com/aptos-labs/aptos-core/releases/aptos-node-v1.4.0. Struct constructor, generic cryptography algebra, ed25519 returns false if wrong length, and transaction shuffling will be enabled in separate proposals." + title: "Multi-step proposal to upgrade mainnet framework to v1.5.0" + description: "This includes changes outlined in https://github.com/aptos-labs/aptos-core/releases/aptos-node-v1.5.0. bls12381, crytography algebra natives, and charging for invariant violations will be enabled in separate proposals." + source_code_url: "" + discussion_url: "https://github.com/aptos-labs/aptos-core/issues/8574" execution_mode: MultiStep update_sequence: - DefaultGas - Framework: bytecode_version: 6 git_hash: ~ - - name: enable_bls12381 + - name: step_2_enable_bls12381 metadata: title: "Enable bls12381" description: "AIP-20: Support of generic cryptography algebra operations in Aptos standard library." + source_code_url: "" discussion_url: "https://github.com/aptos-foundation/AIPs/issues/94" execution_mode: MultiStep update_sequence: @@ -23,12 +26,12 @@ proposals: enabled: - cryptography_algebra_natives - bls12381_structures - - name: enable_charge_invariant_violation + - name: step_3_enable_charge_invariant_violation metadata: title: "Enable charge_invariant_violation" - description: "AIP-20: Support of generic cryptography algebra operations in Aptos standard library." - source_code_url: "https://github.com/aptos-labs/aptos-core/pull/8213" - discussion_url: "https://github.com/aptos-foundation/AIPs/blob/main/aips/aip-35.md" + description: "AIP-35: Charge transactions that triggered invariant violation error instead of discarding them." + source_code_url: "" + discussion_url: "https://github.com/aptos-foundation/AIPs/issues/144" execution_mode: MultiStep update_sequence: - FeatureFlag: From d013f7ccfa76d98e163f16e36b453f60185efcb2 Mon Sep 17 00:00:00 2001 From: Perry Randall Date: Fri, 16 Jun 2023 12:07:37 -0700 Subject: [PATCH 16/16] [release] Add source code url Silly, add source code url TBD, i suspect this not being set is failing release job Test Plan: yq is valid yaml --- aptos-move/aptos-release-builder/data/release.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/aptos-move/aptos-release-builder/data/release.yaml b/aptos-move/aptos-release-builder/data/release.yaml index 31c2acf7a47aa..869f0f26f8f01 100644 --- a/aptos-move/aptos-release-builder/data/release.yaml +++ b/aptos-move/aptos-release-builder/data/release.yaml @@ -6,7 +6,6 @@ proposals: metadata: title: "Multi-step proposal to upgrade mainnet framework to v1.5.0" description: "This includes changes outlined in https://github.com/aptos-labs/aptos-core/releases/aptos-node-v1.5.0. bls12381, crytography algebra natives, and charging for invariant violations will be enabled in separate proposals." - source_code_url: "" discussion_url: "https://github.com/aptos-labs/aptos-core/issues/8574" execution_mode: MultiStep update_sequence: @@ -18,7 +17,6 @@ proposals: metadata: title: "Enable bls12381" description: "AIP-20: Support of generic cryptography algebra operations in Aptos standard library." - source_code_url: "" discussion_url: "https://github.com/aptos-foundation/AIPs/issues/94" execution_mode: MultiStep update_sequence: @@ -30,7 +28,6 @@ proposals: metadata: title: "Enable charge_invariant_violation" description: "AIP-35: Charge transactions that triggered invariant violation error instead of discarding them." - source_code_url: "" discussion_url: "https://github.com/aptos-foundation/AIPs/issues/144" execution_mode: MultiStep update_sequence: