From 15710ca78efda71139ce18855fe202d386551264 Mon Sep 17 00:00:00 2001 From: runtianz Date: Mon, 12 Dec 2022 19:25:56 -0800 Subject: [PATCH] [aptos-release-tooling] Add an option to compare release binary with on chain configs (#5796) * [aptos-release-tooling] Add an option to compare release binary with on chain configs * fixup! [aptos-release-tooling] Add an option to compare release binary with on chain configs --- Cargo.lock | 4 + aptos-move/aptos-release-builder/Cargo.toml | 4 + .../src/components/feature_flags.rs | 37 +++-- .../src/components/mod.rs | 154 +++++++++++++----- aptos-move/aptos-release-builder/src/main.rs | 3 +- crates/aptos-rest-client/src/lib.rs | 11 ++ 6 files changed, 161 insertions(+), 52 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3389d757f1dd5..97a789787dc8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1937,15 +1937,19 @@ dependencies = [ "anyhow", "aptos-crypto", "aptos-gas", + "aptos-rest-client", "aptos-temppath", "aptos-types", "bcs 0.1.4 (git+https://github.com/aptos-labs/bcs.git?rev=d31fab9d81748e2594be5cd5cdf845786a30562d)", "clap 3.2.17", + "futures", "move-core-types", "move-model", "serde 1.0.149", "serde_yaml 0.8.26", "tempfile", + "tokio", + "url", ] [[package]] diff --git a/aptos-move/aptos-release-builder/Cargo.toml b/aptos-move/aptos-release-builder/Cargo.toml index e745d18b726fa..237f7f05a4e25 100644 --- a/aptos-move/aptos-release-builder/Cargo.toml +++ b/aptos-move/aptos-release-builder/Cargo.toml @@ -16,15 +16,19 @@ rust-version = { workspace = true } anyhow = { workspace = true } aptos-crypto = { workspace = true } aptos-gas = { workspace = true } +aptos-rest-client = { workspace = true } aptos-temppath = { workspace = true } aptos-types = { workspace = true } bcs = { workspace = true } clap = { workspace = true } +futures = { workspace = true } move-core-types = { workspace = true } move-model = { workspace = true } serde = { workspace = true } serde_yaml = { workspace = true } tempfile = { workspace = true } +tokio = { workspace = true } +url = { workspace = true } [[bin]] name = "aptos-release-builder" diff --git a/aptos-move/aptos-release-builder/src/components/feature_flags.rs b/aptos-move/aptos-release-builder/src/components/feature_flags.rs index b5d36d65ce359..83e1df3ea6a5f 100644 --- a/aptos-move/aptos-release-builder/src/components/feature_flags.rs +++ b/aptos-move/aptos-release-builder/src/components/feature_flags.rs @@ -3,7 +3,7 @@ use crate::utils::*; use anyhow::Result; -use aptos_types::on_chain_config::FeatureFlag as AFeatureFlag; +use aptos_types::on_chain_config::{FeatureFlag as AptosFeatureFlag, Features as AptosFeatures}; use move_model::{code_writer::CodeWriter, emit, emitln, model::Loc}; use serde::{Deserialize, Serialize}; @@ -50,12 +50,12 @@ pub fn generate_feature_upgrade_proposal( let enabled = features .enabled .iter() - .map(|f| AFeatureFlag::from(f.clone()) as u64) + .map(|f| AptosFeatureFlag::from(f.clone()) as u64) .collect::>(); let disabled = features .disabled .iter() - .map(|f| AFeatureFlag::from(f.clone()) as u64) + .map(|f| AptosFeatureFlag::from(f.clone()) as u64) .collect::>(); assert!(enabled.len() < u16::MAX as usize); @@ -88,23 +88,36 @@ pub fn generate_feature_upgrade_proposal( Ok(result) } -impl From for AFeatureFlag { +impl From for AptosFeatureFlag { fn from(f: FeatureFlag) -> Self { match f { - FeatureFlag::CodeDependencyCheck => AFeatureFlag::CODE_DEPENDENCY_CHECK, - FeatureFlag::TreatFriendAsPrivate => AFeatureFlag::TREAT_FRIEND_AS_PRIVATE, - FeatureFlag::VMBinaryFormatV6 => AFeatureFlag::VM_BINARY_FORMAT_V6, + FeatureFlag::CodeDependencyCheck => AptosFeatureFlag::CODE_DEPENDENCY_CHECK, + FeatureFlag::TreatFriendAsPrivate => AptosFeatureFlag::TREAT_FRIEND_AS_PRIVATE, + FeatureFlag::VMBinaryFormatV6 => AptosFeatureFlag::VM_BINARY_FORMAT_V6, } } } // We don't need this implementation. Just to make sure we have an exhaustive 1-1 mapping between the two structs. -impl From for FeatureFlag { - fn from(f: AFeatureFlag) -> Self { +impl From for FeatureFlag { + fn from(f: AptosFeatureFlag) -> Self { match f { - AFeatureFlag::CODE_DEPENDENCY_CHECK => FeatureFlag::CodeDependencyCheck, - AFeatureFlag::TREAT_FRIEND_AS_PRIVATE => FeatureFlag::TreatFriendAsPrivate, - AFeatureFlag::VM_BINARY_FORMAT_V6 => FeatureFlag::VMBinaryFormatV6, + AptosFeatureFlag::CODE_DEPENDENCY_CHECK => FeatureFlag::CodeDependencyCheck, + AptosFeatureFlag::TREAT_FRIEND_AS_PRIVATE => FeatureFlag::TreatFriendAsPrivate, + AptosFeatureFlag::VM_BINARY_FORMAT_V6 => FeatureFlag::VMBinaryFormatV6, } } } + +impl Features { + // Compare if the current feature set is different from features that has been enabled on chain. + pub(crate) fn has_modified(&self, on_chain_features: &AptosFeatures) -> bool { + self.enabled + .iter() + .any(|f| !on_chain_features.is_enabled(AptosFeatureFlag::from(f.clone()))) + || self + .disabled + .iter() + .any(|f| on_chain_features.is_enabled(AptosFeatureFlag::from(f.clone()))) + } +} diff --git a/aptos-move/aptos-release-builder/src/components/mod.rs b/aptos-move/aptos-release-builder/src/components/mod.rs index 31a76fc72f605..3ca1f486c37fa 100644 --- a/aptos-move/aptos-release-builder/src/components/mod.rs +++ b/aptos-move/aptos-release-builder/src/components/mod.rs @@ -4,13 +4,19 @@ use crate::components::feature_flags::Features; use anyhow::{anyhow, Result}; use aptos_crypto::HashValue; -use aptos_types::on_chain_config::{GasScheduleV2, OnChainConsensusConfig, Version}; +use aptos_rest_client::Client; +use aptos_types::{ + account_config::CORE_CODE_ADDRESS, + on_chain_config::{GasScheduleV2, OnChainConfig, OnChainConsensusConfig, Version}, +}; +use futures::executor::block_on; use serde::{Deserialize, Serialize}; use std::{ fs::File, io::{Read, Write}, path::Path, }; +use url::Url; pub mod consensus_config; pub mod feature_flags; @@ -21,6 +27,7 @@ pub mod version; #[derive(Serialize, Deserialize, Clone, Eq, PartialEq)] pub struct ReleaseConfig { pub testnet: bool, + pub remote_endpoint: Option, pub framework_release: bool, #[serde(default, skip_serializing_if = "Option::is_none")] pub gas_schedule: Option, @@ -34,16 +41,53 @@ pub struct ReleaseConfig { pub is_multi_step: bool, } +// Compare the current on chain config with the value recorded on chain. Return false if there's a difference. +fn fetch_and_equals( + client: &Option, + expected: &T, +) -> Result { + match client { + Some(client) => { + let config = T::deserialize_into_config( + block_on(async { + client + .get_account_resource_bytes( + CORE_CODE_ADDRESS, + format!( + "{}::{}::{}", + T::ADDRESS, + T::MODULE_IDENTIFIER, + T::TYPE_IDENTIFIER + ) + .as_str(), + ) + .await + })? + .inner(), + )?; + + Ok(&config == expected) + } + None => Ok(false), + } +} + impl ReleaseConfig { pub fn generate_release_proposal_scripts(&self, base_path: &Path) -> Result<()> { let mut result: Vec<(String, String)> = vec![]; - let mut release_generation_functions: Vec<&dyn Fn(&Self, &mut Vec<(String, String)>)> = vec![ + let mut release_generation_functions: Vec< + &dyn Fn(&Self, &Option, &mut Vec<(String, String)>) -> Result<()>, + > = vec![ &Self::generate_framework_release, &Self::generate_gas_schedule, &Self::generate_version_file, &Self::generate_feature_flag_file, &Self::generate_consensus_file, ]; + let client = self + .remote_endpoint + .as_ref() + .map(|url| Client::new(url.clone())); // If we are generating multi-step proposal files, we generate the files in reverse order, // since we need to pass in the hash of the next file to the previous file. @@ -52,7 +96,7 @@ impl ReleaseConfig { } for f in &release_generation_functions { - (f)(self, &mut result); + (f)(self, &client, &mut result)?; } // Here we are reversing the results back, so the result would be in order. @@ -73,26 +117,32 @@ impl ReleaseConfig { Ok(()) } - fn generate_framework_release(&self, result: &mut Vec<(String, String)>) { + fn generate_framework_release( + &self, + _client: &Option, + result: &mut Vec<(String, String)>, + ) -> Result<()> { if self.framework_release { - result.append( - &mut framework::generate_upgrade_proposals( - self.testnet, - if self.is_multi_step { - Self::get_execution_hash(result) - } else { - "".to_owned() - }, - ) - .unwrap(), - ); + result.append(&mut framework::generate_upgrade_proposals( + self.testnet, + if self.is_multi_step { + Self::get_execution_hash(result) + } else { + "".to_owned() + }, + )?); } + Ok(()) } - fn generate_gas_schedule(&self, result: &mut Vec<(String, String)>) { + fn generate_gas_schedule( + &self, + client: &Option, + result: &mut Vec<(String, String)>, + ) -> Result<()> { if let Some(gas_schedule) = &self.gas_schedule { - result.append( - &mut gas::generate_gas_upgrade_proposal( + if !fetch_and_equals::(client, gas_schedule)? { + result.append(&mut gas::generate_gas_upgrade_proposal( gas_schedule, self.testnet, if self.is_multi_step { @@ -100,16 +150,20 @@ impl ReleaseConfig { } else { "".to_owned() }, - ) - .unwrap(), - ); + )?); + } } + Ok(()) } - fn generate_version_file(&self, result: &mut Vec<(String, String)>) { + fn generate_version_file( + &self, + client: &Option, + result: &mut Vec<(String, String)>, + ) -> Result<()> { if let Some(version) = &self.version { - result.append( - &mut version::generate_version_upgrade_proposal( + if !fetch_and_equals::(client, version)? { + result.append(&mut version::generate_version_upgrade_proposal( version, self.testnet, if self.is_multi_step { @@ -117,16 +171,33 @@ impl ReleaseConfig { } else { "".to_owned() }, - ) - .unwrap(), - ); + )?); + } } + Ok(()) } - fn generate_feature_flag_file(&self, result: &mut Vec<(String, String)>) { + fn generate_feature_flag_file( + &self, + client: &Option, + result: &mut Vec<(String, String)>, + ) -> Result<()> { if let Some(feature_flags) = &self.feature_flags { - result.append( - &mut feature_flags::generate_feature_upgrade_proposal( + let mut needs_update = false; + if let Some(client) = client { + let features = block_on(async { + client + .get_account_resource_bcs::( + CORE_CODE_ADDRESS, + "0x1::features::Features", + ) + .await + })?; + // Only update the feature flags section when there's a divergence between the local configs and on chain configs. + needs_update = feature_flags.has_modified(features.inner()); + } + if needs_update { + result.append(&mut feature_flags::generate_feature_upgrade_proposal( feature_flags, self.testnet, if self.is_multi_step { @@ -134,16 +205,20 @@ impl ReleaseConfig { } else { "".to_owned() }, - ) - .unwrap(), - ); + )?); + } } + Ok(()) } - fn generate_consensus_file(&self, result: &mut Vec<(String, String)>) { + fn generate_consensus_file( + &self, + client: &Option, + result: &mut Vec<(String, String)>, + ) -> Result<()> { if let Some(consensus_config) = &self.consensus_config { - result.append( - &mut consensus_config::generate_consensus_upgrade_proposal( + if fetch_and_equals(client, consensus_config)? { + result.append(&mut consensus_config::generate_consensus_upgrade_proposal( consensus_config, self.testnet, if self.is_multi_step { @@ -151,10 +226,10 @@ impl ReleaseConfig { } else { "".to_owned() }, - ) - .unwrap(), - ); + )?); + } } + Ok(()) } pub fn load_config>(path: P) -> Result { @@ -213,6 +288,7 @@ impl Default for ReleaseConfig { feature_flags: None, consensus_config: Some(OnChainConsensusConfig::default()), is_multi_step: false, + remote_endpoint: None, } } } diff --git a/aptos-move/aptos-release-builder/src/main.rs b/aptos-move/aptos-release-builder/src/main.rs index 92f1e33074e9c..cff8e96dbad0e 100644 --- a/aptos-move/aptos-release-builder/src/main.rs +++ b/aptos-move/aptos-release-builder/src/main.rs @@ -25,7 +25,8 @@ pub enum Commands { }, } -fn main() -> Result<()> { +#[tokio::main] +async fn main() -> Result<()> { let args = Argument::parse(); // TODO: Being able to parse the release config from a TOML file to generate the proposals. diff --git a/crates/aptos-rest-client/src/lib.rs b/crates/aptos-rest-client/src/lib.rs index e6a48850ffd3e..af83aabae079f 100644 --- a/crates/aptos-rest-client/src/lib.rs +++ b/crates/aptos-rest-client/src/lib.rs @@ -1026,6 +1026,17 @@ impl Client { Ok(response.map(|inner| inner.to_vec())) } + pub async fn get_account_resource_bytes( + &self, + address: AccountAddress, + resource_type: &str, + ) -> AptosResult>> { + let url = self.build_path(&format!("accounts/{}/resource/{}", address, resource_type))?; + + let response = self.get_bcs(url).await?; + Ok(response.map(|inner| inner.to_vec())) + } + pub async fn get_account_resource_at_version( &self, address: AccountAddress,