Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable dedup and shuffling as defaults (for test networks) #9397

Merged
merged 6 commits into from
Aug 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions aptos-move/vm-genesis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ pub fn encode_aptos_mainnet_genesis_transaction(

// On-chain genesis process.
let consensus_config = OnChainConsensusConfig::default();
let execution_config = OnChainExecutionConfig::default();
let execution_config = OnChainExecutionConfig::default_for_genesis();
let gas_schedule = default_gas_schedule();
initialize(
&mut session,
Expand Down Expand Up @@ -794,7 +794,7 @@ pub fn generate_test_genesis(
employee_vesting_period_duration: 5 * 60, // 5 minutes
},
&OnChainConsensusConfig::default(),
&OnChainExecutionConfig::default(),
&OnChainExecutionConfig::default_for_genesis(),
&default_gas_schedule(),
);
(genesis, test_validators)
Expand All @@ -816,7 +816,7 @@ pub fn generate_mainnet_genesis(
ChainId::test(),
&mainnet_genesis_config(),
&OnChainConsensusConfig::default(),
&OnChainExecutionConfig::default(),
&OnChainExecutionConfig::default_for_genesis(),
&default_gas_schedule(),
);
(genesis, test_validators)
Expand Down
3 changes: 2 additions & 1 deletion consensus/src/epoch_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,8 @@ impl EpochManager {
match self.storage.start() {
LivenessStorageData::FullRecoveryData(initial_data) => {
let consensus_config = onchain_consensus_config.unwrap_or_default();
let execution_config = onchain_execution_config.unwrap_or_default();
let execution_config = onchain_execution_config
.unwrap_or_else(|_| OnChainExecutionConfig::default_if_missing());
self.quorum_store_enabled = self.enable_quorum_store(&consensus_config);
self.recovery_mode = false;
self.start_round_manager(
Expand Down
10 changes: 2 additions & 8 deletions crates/aptos-genesis/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ use aptos_keygen::KeyGen;
use aptos_logger::prelude::*;
use aptos_types::{
chain_id::ChainId,
on_chain_config::{
ExecutionConfigV1, GasScheduleV2, OnChainConsensusConfig, OnChainExecutionConfig,
TransactionShufflerType,
},
on_chain_config::{GasScheduleV2, OnChainConsensusConfig, OnChainExecutionConfig},
transaction::Transaction,
waypoint::Waypoint,
};
Expand Down Expand Up @@ -613,10 +610,7 @@ impl Builder {
employee_vesting_start: None,
employee_vesting_period_duration: None,
consensus_config: OnChainConsensusConfig::default(),
// Enable transaction shuffling by default in integration tests and Forge.
execution_config: OnChainExecutionConfig::V1(ExecutionConfigV1 {
transaction_shuffler_type: TransactionShufflerType::SenderAwareV2(32),
}),
execution_config: OnChainExecutionConfig::default_for_genesis(),
gas_schedule: default_gas_schedule(),
};
if let Some(init_genesis_config) = &self.init_genesis_config {
Expand Down
10 changes: 3 additions & 7 deletions crates/aptos/src/genesis/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ use aptos_genesis::{
use aptos_logger::info;
use aptos_types::{
account_address::{AccountAddress, AccountAddressWithChecks},
on_chain_config::{
ExecutionConfigV1, OnChainConsensusConfig, OnChainExecutionConfig, TransactionShufflerType,
},
on_chain_config::{OnChainConsensusConfig, OnChainExecutionConfig},
};
use aptos_vm_genesis::{default_gas_schedule, AccountBalance, EmployeePool};
use async_trait::async_trait;
Expand Down Expand Up @@ -257,7 +255,7 @@ pub fn fetch_mainnet_genesis_info(git_options: GitOptions) -> CliTypedResult<Mai
employee_vesting_start: layout.employee_vesting_start,
employee_vesting_period_duration: layout.employee_vesting_period_duration,
consensus_config: OnChainConsensusConfig::default(),
execution_config: OnChainExecutionConfig::default(),
execution_config: OnChainExecutionConfig::default_for_genesis(),
gas_schedule: default_gas_schedule(),
},
)?)
Expand Down Expand Up @@ -297,9 +295,7 @@ pub fn fetch_genesis_info(git_options: GitOptions) -> CliTypedResult<GenesisInfo
employee_vesting_start: layout.employee_vesting_start,
employee_vesting_period_duration: layout.employee_vesting_period_duration,
consensus_config: OnChainConsensusConfig::default(),
execution_config: OnChainExecutionConfig::V1(ExecutionConfigV1 {
transaction_shuffler_type: TransactionShufflerType::SenderAwareV2(32),
}),
execution_config: OnChainExecutionConfig::default_for_genesis(),
gas_schedule: default_gas_schedule(),
},
)?)
Expand Down
4 changes: 2 additions & 2 deletions testsuite/smoke-test/src/rest_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use aptos_sdk::move_types::language_storage::StructTag;
use aptos_types::{
account_address::AccountAddress,
account_config::{AccountResource, CORE_CODE_ADDRESS},
on_chain_config::{ExecutionConfigV2, OnChainExecutionConfig},
on_chain_config::{ExecutionConfigV2, OnChainExecutionConfig, TransactionShufflerType},
transaction::{authenticator::AuthenticationKey, SignedTransaction, Transaction},
};
use std::{convert::TryFrom, str::FromStr, sync::Arc, time::Duration};
Expand Down Expand Up @@ -209,8 +209,8 @@ async fn test_gas_estimation_gas_used_limit() {
let mut swarm = SwarmBuilder::new_local(1)
.with_init_genesis_config(Arc::new(|conf| {
conf.execution_config = OnChainExecutionConfig::V2(ExecutionConfigV2 {
transaction_shuffler_type: TransactionShufflerType::NoShuffling,
block_gas_limit: Some(1),
..Default::default()
});
}))
.with_init_config(Arc::new(|_, conf, _| {
Expand Down
58 changes: 24 additions & 34 deletions types/src/on_chain_config/execution_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,18 @@ pub enum OnChainExecutionConfig {
V1(ExecutionConfigV1),
V2(ExecutionConfigV2),
V3(ExecutionConfigV3),
/// To maintain backwards compatibility on replay, we must ensure that any new features resolve
/// to previous behavior (before OnChainExecutionConfig was registered) in case of Missing.
Missing,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pretty easy to make a mistake and add the next version V4 after V3 and before Missing and thus breaking backward compatibility. Can you add a comment here to add V4 after Missing ?

// Reminder: Add V4 and future versions here, after Missing (order matters for enums).
}

/// The public interface that exposes all values with safe fallback.
impl OnChainExecutionConfig {
/// The type of the transaction shuffler being used.
pub fn transaction_shuffler_type(&self) -> TransactionShufflerType {
match &self {
OnChainExecutionConfig::Missing => TransactionShufflerType::NoShuffling,
OnChainExecutionConfig::V1(config) => config.transaction_shuffler_type.clone(),
OnChainExecutionConfig::V2(config) => config.transaction_shuffler_type.clone(),
OnChainExecutionConfig::V3(config) => config.transaction_shuffler_type.clone(),
Expand All @@ -27,6 +32,7 @@ impl OnChainExecutionConfig {
/// The per-block gas limit being used.
pub fn block_gas_limit(&self) -> Option<u64> {
match &self {
OnChainExecutionConfig::Missing => None,
OnChainExecutionConfig::V1(_config) => None,
OnChainExecutionConfig::V2(config) => config.block_gas_limit,
OnChainExecutionConfig::V3(config) => config.block_gas_limit,
Expand All @@ -36,17 +42,28 @@ impl OnChainExecutionConfig {
/// The type of the transaction deduper being used.
pub fn transaction_deduper_type(&self) -> TransactionDeduperType {
match &self {
// Note, this behavior was enabled before OnChainExecutionConfig was registered.
OnChainExecutionConfig::Missing => TransactionDeduperType::TxnHashAndAuthenticatorV1,
OnChainExecutionConfig::V1(_config) => TransactionDeduperType::NoDedup,
OnChainExecutionConfig::V2(_config) => TransactionDeduperType::NoDedup,
OnChainExecutionConfig::V3(config) => config.transaction_deduper_type.clone(),
}
}
}

/// This is used when on-chain config is not initialized.
impl Default for OnChainExecutionConfig {
fn default() -> Self {
OnChainExecutionConfig::V3(ExecutionConfigV3::default())
/// The default values to use for new networks, e.g., devnet, forge.
/// Features that are ready for deployment can be enabled here.
pub fn default_for_genesis() -> Self {
OnChainExecutionConfig::V3(ExecutionConfigV3 {
transaction_shuffler_type: TransactionShufflerType::SenderAwareV2(32),
block_gas_limit: Some(35000),
transaction_deduper_type: TransactionDeduperType::TxnHashAndAuthenticatorV1,
})
}

/// The default values to use when on-chain config is not initialized.
/// This value should not be changed, for replay purposes.
pub fn default_if_missing() -> Self {
OnChainExecutionConfig::Missing
}
}

Expand All @@ -73,46 +90,19 @@ pub struct ExecutionConfigV1 {
pub transaction_shuffler_type: TransactionShufflerType,
}

impl Default for ExecutionConfigV1 {
fn default() -> Self {
Self {
transaction_shuffler_type: TransactionShufflerType::NoShuffling,
}
}
}

#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)]
pub struct ExecutionConfigV2 {
pub transaction_shuffler_type: TransactionShufflerType,
pub block_gas_limit: Option<u64>,
}

impl Default for ExecutionConfigV2 {
fn default() -> Self {
Self {
transaction_shuffler_type: TransactionShufflerType::NoShuffling,
block_gas_limit: None,
}
}
}

#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)]
pub struct ExecutionConfigV3 {
pub transaction_shuffler_type: TransactionShufflerType,
pub block_gas_limit: Option<u64>,
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::TxnHashAndAuthenticatorV1,
}
}
}

#[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 {
Expand All @@ -137,15 +127,15 @@ mod test {

#[test]
fn test_config_yaml_serialization() {
let config = OnChainExecutionConfig::default();
let config = OnChainExecutionConfig::default_for_genesis();
let s = serde_yaml::to_string(&config).unwrap();

serde_yaml::from_str::<OnChainExecutionConfig>(&s).unwrap();
}

#[test]
fn test_config_bcs_serialization() {
let config = OnChainExecutionConfig::default();
let config = OnChainExecutionConfig::default_for_genesis();
let s = bcs::to_bytes(&config).unwrap();

bcs::from_bytes::<OnChainExecutionConfig>(&s).unwrap();
Expand Down