From f8801ec74a0e8e512f53663056bdf7785a55766d Mon Sep 17 00:00:00 2001 From: Victor Gao Date: Wed, 7 Dec 2022 00:23:06 +0000 Subject: [PATCH 1/3] [gas] remove test_only --- aptos-move/aptos-gas/src/move_stdlib.rs | 13 +-- aptos-move/aptos-gas/src/natives.rs | 103 +++--------------------- 2 files changed, 19 insertions(+), 97 deletions(-) diff --git a/aptos-move/aptos-gas/src/move_stdlib.rs b/aptos-move/aptos-gas/src/move_stdlib.rs index d8716ce4da46f..66e1878838941 100644 --- a/aptos-move/aptos-gas/src/move_stdlib.rs +++ b/aptos-move/aptos-gas/src/move_stdlib.rs @@ -4,6 +4,12 @@ use crate::gas_meter::EXECUTION_GAS_MULTIPLIER as MUL; use move_stdlib::natives::GasParameters; +#[cfg(all(test, not(feature = "testing")))] +const UNIT_TEST_ENTRIES: usize = 0; + +#[cfg(all(test, feature = "testing"))] +const UNIT_TEST_ENTRIES: usize = 2; + crate::natives::define_gas_parameters_for_natives!(GasParameters, "move_stdlib", [ [.bcs.to_bytes.per_byte_serialized, "bcs.to_bytes.per_byte_serialized", 10 * MUL], [.bcs.to_bytes.failure, "bcs.to_bytes.failure", 1000 * MUL], @@ -25,9 +31,4 @@ crate::natives::define_gas_parameters_for_natives!(GasParameters, "move_stdlib", [.string.index_of.base, "string.index_of.base", 400 * MUL], [.string.index_of.per_byte_pattern, "string.index_of.per_byte_pattern", 20 * MUL], [.string.index_of.per_byte_searched, "string.index_of.per_byte_searched", 10 * MUL], - - // TODO(Gas): these should only be enabled when feature "testing" is present - // TODO(Gas): rename these in the move repo - [test_only .unit_test.create_signers_for_testing.base_cost, "unit_test.create_signers_for_testing.base", 1], - [test_only .unit_test.create_signers_for_testing.unit_cost, "unit_test.create_signers_for_testing.unit", 1] -], allow_unmapped = 1 /* bcs */ + 2 /* hash */ + 8 /* vector */ + 2 /* type_name */); +], allow_unmapped = 1 /* bcs */ + 2 /* hash */ + 8 /* vector */ + 2 /* type_name */ + UNIT_TEST_ENTRIES); diff --git a/aptos-move/aptos-gas/src/natives.rs b/aptos-move/aptos-gas/src/natives.rs index 476c8a82e26e4..7ff0104e77837 100644 --- a/aptos-move/aptos-gas/src/natives.rs +++ b/aptos-move/aptos-gas/src/natives.rs @@ -15,90 +15,19 @@ macro_rules! expand_get_impl_for_native_gas_params { }; } -macro_rules! expand_get_for_native_gas_params { - (test_only $(.$field: ident)+, $(optional $($dummy: ident)?)? $key: literal, $initial_val: expr, $param_ty: ty, $package_name: literal, $params: ident, $gas_schedule: ident) => { - // TODO(Gas): this is a hack to work-around issue - // https://github.com/rust-lang/rust/issues/15701 - { - #[cfg(feature = "testing")] - fn assign(params: &mut $param_ty, gas_schedule: &std::collections::BTreeMap) -> Option<()> { - $crate::natives::expand_get_impl_for_native_gas_params!(params $(.$field)+, gas_schedule, $package_name, $(optional $($dummy)?)? $key); - Some(()) - } - - #[cfg(not(feature = "testing"))] - fn assign(_params: &mut $param_ty, _gas_schedule: &std::collections::BTreeMap) -> Option<()> { - Some(()) - } - - assign(&mut $params, &$gas_schedule)?; - } - }; - ($(.$field: ident)+, $(optional $($dummy: ident)?)? $key: literal, $initial_val: expr, $param_ty: ty, $package_name: literal, $params: ident, $gas_schedule: ident) => { - $crate::natives::expand_get_impl_for_native_gas_params!($params $(.$field)+, $gas_schedule, $package_name, $(optional $($dummy)?)? $key); - } -} - -macro_rules! expand_set_for_native_gas_params { - (test_only $(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr, $param_ty: ty, $package_name: literal, $params: ident) => { - { - #[cfg(feature = "testing")] - fn assign(params: &mut $param_ty) { - params $(.$field)+ = $initial_val.into(); - } - - #[cfg(not(feature = "testing"))] - fn assign(_params: &mut $param_ty) { - } - - assign(&mut $params); - } - }; - ($(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr, $param_ty: ty, $package_name: literal, $params: ident) => { - $params $(.$field)+ = $initial_val.into() - }; -} - -macro_rules! expand_kv_for_native_gas_params { - (test_only $(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr, $self: ident) => { - #[cfg(feature = "testing")] - ($key, u64::from($self $(.$field)+)) - }; - ($(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr, $self: ident) => { - ($key, u64::from($self $(.$field)+)) - } -} - -#[cfg(test)] -macro_rules! extract_key_for_native_gas_params { - (test_only $(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr) => { - #[cfg(feature = "testing")] - $key - }; - ($(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr) => { - $key - }; -} - -#[cfg(test)] -macro_rules! extract_path_for_native_gas_params { - (test_only $(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr) => { - #[cfg(feature = "testing")] - stringify!($($field).*) - }; - ($(.$field: ident)+, $(optional)? $key: literal, $initial_val: expr) => { - stringify!($($field).*) - }; -} - macro_rules! define_gas_parameters_for_natives { - ($param_ty: ty, $package_name: literal, [$([$($t: tt)*]),* $(,)?] $(, allow_unmapped = $allow_unmapped: expr)?) => { + ( + $param_ty: ty, + $package_name: literal, + [$([$(.$field: ident)+, $(optional $($dummy: ident)?)? $key: literal, $initial_val: expr]),+ $(,)?] + $(, allow_unmapped = $allow_unmapped: expr)? + ) => { impl crate::gas_meter::FromOnChainGasSchedule for $param_ty { fn from_on_chain_gas_schedule(gas_schedule: &std::collections::BTreeMap) -> Option { let mut params = <$param_ty>::zeros(); $( - crate::natives::expand_get_for_native_gas_params!($($t)*, $param_ty, $package_name, params, gas_schedule); + $crate::natives::expand_get_impl_for_native_gas_params!(params $(.$field)+, gas_schedule, $package_name, $(optional $($dummy)?)? $key); )* Some(params) @@ -107,7 +36,7 @@ macro_rules! define_gas_parameters_for_natives { impl crate::gas_meter::ToOnChainGasSchedule for $param_ty { fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)> { - [$(crate::natives::expand_kv_for_native_gas_params!($($t)*, self)),*] + [$(($key, u64::from(self $(.$field)+))),*] .into_iter().map(|(key, val)| (format!("{}.{}", $package_name, key), val)).collect() } } @@ -117,7 +46,7 @@ macro_rules! define_gas_parameters_for_natives { let mut params = <$param_ty>::zeros(); $( - crate::natives::expand_set_for_native_gas_params!($($t)*, $param_ty, $package_name, params); + params $(.$field)+ = $initial_val.into(); )* params @@ -128,7 +57,7 @@ macro_rules! define_gas_parameters_for_natives { fn keys_should_be_unique() { let mut map = std::collections::BTreeMap::<&str, ()>::new(); - for key in [$(crate::natives::extract_key_for_native_gas_params!($($t)*)),*] { + for key in [$($key),*] { if map.insert(key.clone(), ()).is_some() { panic!("duplicated key {}", key); } @@ -139,7 +68,7 @@ macro_rules! define_gas_parameters_for_natives { fn paths_must_be_unique() { let mut map = std::collections::BTreeMap::<&str, ()>::new(); - for path in [$(crate::natives::extract_path_for_native_gas_params!($($t)*)),*] { + for path in [$(stringify!($($field).*)),*] { if map.insert(path.clone(), ()).is_some() { panic!("duplicated path {}", path); } @@ -149,7 +78,7 @@ macro_rules! define_gas_parameters_for_natives { #[test] fn all_parameters_mapped() { let total = format!("{:?}", &<$param_ty>::zeros()).matches(": 0").count(); - let mapped = [$(crate::natives::extract_key_for_native_gas_params!($($t)*)),*].len() $(+ $allow_unmapped)?; + let mapped = [$($key),*].len() $(+ $allow_unmapped)?; if mapped != total { panic!("only {} out of the {} entries are mapped", mapped, total) } @@ -158,15 +87,7 @@ macro_rules! define_gas_parameters_for_natives { } pub(crate) use define_gas_parameters_for_natives; -pub(crate) use expand_get_for_native_gas_params; pub(crate) use expand_get_impl_for_native_gas_params; -pub(crate) use expand_kv_for_native_gas_params; -pub(crate) use expand_set_for_native_gas_params; - -#[cfg(test)] -pub(crate) use extract_key_for_native_gas_params; -#[cfg(test)] -pub(crate) use extract_path_for_native_gas_params; #[cfg(test)] mod tests { From fe436d6dbd5eca73651d94a8758bcf83a6d78e16 Mon Sep 17 00:00:00 2001 From: Victor Gao Date: Tue, 6 Dec 2022 23:23:11 +0000 Subject: [PATCH 2/3] [gas] versioned gas schedule -- params --- aptos-move/aptos-gas/src/gas_meter.rs | 70 +++++++++++----- aptos-move/aptos-gas/src/gen.rs | 2 +- aptos-move/aptos-gas/src/instr.rs | 4 +- aptos-move/aptos-gas/src/misc.rs | 14 +++- aptos-move/aptos-gas/src/natives.rs | 12 ++- aptos-move/aptos-gas/src/params.rs | 88 +++++++++------------ aptos-move/aptos-gas/src/transaction/mod.rs | 12 +-- 7 files changed, 116 insertions(+), 86 deletions(-) diff --git a/aptos-move/aptos-gas/src/gas_meter.rs b/aptos-move/aptos-gas/src/gas_meter.rs index 51407273f3cd1..ba31a33b81688 100644 --- a/aptos-move/aptos-gas/src/gas_meter.rs +++ b/aptos-move/aptos-gas/src/gas_meter.rs @@ -55,7 +55,10 @@ pub trait FromOnChainGasSchedule: Sized { /// Constructs a value of this type from a map representation of the on-chain gas schedule. /// `None` should be returned when the gas schedule is missing some required entries. /// Unused entries should be safely ignored. - fn from_on_chain_gas_schedule(gas_schedule: &BTreeMap) -> Option; + fn from_on_chain_gas_schedule( + gas_schedule: &BTreeMap, + feature_version: u64, + ) -> Option; } /// A trait for converting to a list of entries of the on-chain gas schedule. @@ -63,7 +66,7 @@ pub trait ToOnChainGasSchedule { /// Converts `self` into a list of entries of the on-chain gas schedule. /// Each entry is a key-value pair where the key is a string representing the name of the /// parameter, where the value is the gas parameter itself. - fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)>; + fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)>; } /// A trait for defining an initial value to be used in the genesis. @@ -81,20 +84,35 @@ pub struct NativeGasParameters { } impl FromOnChainGasSchedule for NativeGasParameters { - fn from_on_chain_gas_schedule(gas_schedule: &BTreeMap) -> Option { + fn from_on_chain_gas_schedule( + gas_schedule: &BTreeMap, + feature_version: u64, + ) -> Option { Some(Self { - move_stdlib: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, - aptos_framework: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, - table: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, + move_stdlib: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, + aptos_framework: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, + table: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, }) } } impl ToOnChainGasSchedule for NativeGasParameters { - fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)> { - let mut entries = self.move_stdlib.to_on_chain_gas_schedule(); - entries.extend(self.aptos_framework.to_on_chain_gas_schedule()); - entries.extend(self.table.to_on_chain_gas_schedule()); + fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)> { + let mut entries = self.move_stdlib.to_on_chain_gas_schedule(feature_version); + entries.extend( + self.aptos_framework + .to_on_chain_gas_schedule(feature_version), + ); + entries.extend(self.table.to_on_chain_gas_schedule(feature_version)); entries } } @@ -130,22 +148,34 @@ pub struct AptosGasParameters { } impl FromOnChainGasSchedule for AptosGasParameters { - fn from_on_chain_gas_schedule(gas_schedule: &BTreeMap) -> Option { + fn from_on_chain_gas_schedule( + gas_schedule: &BTreeMap, + feature_version: u64, + ) -> Option { Some(Self { - misc: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, - instr: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, - txn: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, - natives: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, + misc: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, + instr: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, + txn: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule, feature_version)?, + natives: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, }) } } impl ToOnChainGasSchedule for AptosGasParameters { - fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)> { - let mut entries = self.instr.to_on_chain_gas_schedule(); - entries.extend(self.txn.to_on_chain_gas_schedule()); - entries.extend(self.natives.to_on_chain_gas_schedule()); - entries.extend(self.misc.to_on_chain_gas_schedule()); + fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)> { + let mut entries = self.instr.to_on_chain_gas_schedule(feature_version); + entries.extend(self.txn.to_on_chain_gas_schedule(feature_version)); + entries.extend(self.natives.to_on_chain_gas_schedule(feature_version)); + entries.extend(self.misc.to_on_chain_gas_schedule(feature_version)); entries } } diff --git a/aptos-move/aptos-gas/src/gen.rs b/aptos-move/aptos-gas/src/gen.rs index 44764bb46a7b9..08ad17cfc683e 100644 --- a/aptos-move/aptos-gas/src/gen.rs +++ b/aptos-move/aptos-gas/src/gen.rs @@ -15,7 +15,7 @@ use std::path::{Path, PathBuf}; pub fn current_gas_schedule() -> GasScheduleV2 { GasScheduleV2 { feature_version: LATEST_GAS_FEATURE_VERSION, - entries: AptosGasParameters::initial().to_on_chain_gas_schedule(), + entries: AptosGasParameters::initial().to_on_chain_gas_schedule(LATEST_GAS_FEATURE_VERSION), } } diff --git a/aptos-move/aptos-gas/src/instr.rs b/aptos-move/aptos-gas/src/instr.rs index 278a585529342..5aa1cbd267f91 100644 --- a/aptos-move/aptos-gas/src/instr.rs +++ b/aptos-move/aptos-gas/src/instr.rs @@ -69,7 +69,7 @@ crate::params::define_gas_parameters!( // call [call_base: InternalGas, "call.base", 1000 * MUL], [call_per_arg: InternalGasPerArg, "call.per_arg", 100 * MUL], - [call_per_local: InternalGasPerArg, optional "call.per_local", 100 * MUL], + [call_per_local: InternalGasPerArg, { 1.. => "call.per_local" }, 100 * MUL], [call_generic_base: InternalGas, "call_generic.base", 1000 * MUL], [ call_generic_per_ty_arg: InternalGasPerArg, @@ -81,7 +81,7 @@ crate::params::define_gas_parameters!( "call_generic.per_arg", 100 * MUL ], - [call_generic_per_local: InternalGasPerArg, optional "call_generic.per_local", 100 * MUL], + [call_generic_per_local: InternalGasPerArg, { 1.. => "call_generic.per_local" }, 100 * MUL], // struct [pack_base: InternalGas, "pack.base", 220 * MUL], [pack_per_field: InternalGasPerArg, "pack.per_field", 40 * MUL], diff --git a/aptos-move/aptos-gas/src/misc.rs b/aptos-move/aptos-gas/src/misc.rs index d48f2b0aa178b..12eab8a4e9679 100644 --- a/aptos-move/aptos-gas/src/misc.rs +++ b/aptos-move/aptos-gas/src/misc.rs @@ -579,16 +579,22 @@ pub struct MiscGasParameters { } impl FromOnChainGasSchedule for MiscGasParameters { - fn from_on_chain_gas_schedule(gas_schedule: &BTreeMap) -> Option { + fn from_on_chain_gas_schedule( + gas_schedule: &BTreeMap, + feature_version: u64, + ) -> Option { Some(Self { - abs_val: FromOnChainGasSchedule::from_on_chain_gas_schedule(gas_schedule)?, + abs_val: FromOnChainGasSchedule::from_on_chain_gas_schedule( + gas_schedule, + feature_version, + )?, }) } } impl ToOnChainGasSchedule for MiscGasParameters { - fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)> { - self.abs_val.to_on_chain_gas_schedule() + fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)> { + self.abs_val.to_on_chain_gas_schedule(feature_version) } } diff --git a/aptos-move/aptos-gas/src/natives.rs b/aptos-move/aptos-gas/src/natives.rs index 7ff0104e77837..c105f3a7b5834 100644 --- a/aptos-move/aptos-gas/src/natives.rs +++ b/aptos-move/aptos-gas/src/natives.rs @@ -23,7 +23,7 @@ macro_rules! define_gas_parameters_for_natives { $(, allow_unmapped = $allow_unmapped: expr)? ) => { impl crate::gas_meter::FromOnChainGasSchedule for $param_ty { - fn from_on_chain_gas_schedule(gas_schedule: &std::collections::BTreeMap) -> Option { + fn from_on_chain_gas_schedule(gas_schedule: &std::collections::BTreeMap, feature_version: u64) -> Option { let mut params = <$param_ty>::zeros(); $( @@ -35,7 +35,7 @@ macro_rules! define_gas_parameters_for_natives { } impl crate::gas_meter::ToOnChainGasSchedule for $param_ty { - fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)> { + fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)> { [$(($key, u64::from(self $(.$field)+))),*] .into_iter().map(|(key, val)| (format!("{}.{}", $package_name, key), val)).collect() } @@ -92,7 +92,7 @@ pub(crate) use expand_get_impl_for_native_gas_params; #[cfg(test)] mod tests { use super::*; - use crate::gas_meter::FromOnChainGasSchedule; + use crate::gas_meter::{FromOnChainGasSchedule, LATEST_GAS_FEATURE_VERSION}; use move_core_types::gas_algebra::InternalGas; #[derive(Debug, Clone)] @@ -121,12 +121,16 @@ mod tests { assert!(matches!( GasParameters::from_on_chain_gas_schedule( &[("test.foo".to_string(), 0)].into_iter().collect(), + LATEST_GAS_FEATURE_VERSION ), Some(_) )); assert!(matches!( - GasParameters::from_on_chain_gas_schedule(&[].into_iter().collect()), + GasParameters::from_on_chain_gas_schedule( + &[].into_iter().collect(), + LATEST_GAS_FEATURE_VERSION + ), None )); } diff --git a/aptos-move/aptos-gas/src/params.rs b/aptos-move/aptos-gas/src/params.rs index 0a18acede3a27..144f9e4f60cbc 100644 --- a/aptos-move/aptos-gas/src/params.rs +++ b/aptos-move/aptos-gas/src/params.rs @@ -1,15 +1,17 @@ // Copyright (c) Aptos // SPDX-License-Identifier: Apache-2.0 -macro_rules! expand_get_for_gas_parameters { - ($params: ident . $name: ident, $map: ident, $prefix: literal, optional $key: literal) => { - if let Some(val) = $map.get(&format!("{}.{}", $prefix, $key)) { - $params.$name = (*val).into(); - } - }; - ($params: ident . $name: ident, $map: ident, $prefix: literal, $key: literal) => { - $params.$name = $map.get(&format!("{}.{}", $prefix, $key)).cloned()?.into(); +macro_rules! define_gas_parameters_extract_key_at_version { + ($key: literal, $cur_ver: expr) => { + Some($key) }; + + ({ $($ver: pat => $key: literal),+ }, $cur_ver: expr) => { + match $cur_ver { + $($ver => Some($key)),+, + _ => None, + } + } } macro_rules! define_gas_parameters { @@ -17,7 +19,7 @@ macro_rules! define_gas_parameters { $params_name: ident, $prefix: literal, [$( - [$name: ident: $ty: ty, $(optional $($dummy: ident)?)? $key: literal $(,)?, $initial: expr $(,)?] + [$name: ident: $ty: ty, $key_bindings: tt, $initial: expr $(,)?] ),* $(,)?] ) => { #[derive(Debug, Clone)] @@ -26,11 +28,14 @@ macro_rules! define_gas_parameters { } impl $crate::gas_meter::FromOnChainGasSchedule for $params_name { - fn from_on_chain_gas_schedule(gas_schedule: &std::collections::BTreeMap) -> Option { + #[allow(unused_variables)] + fn from_on_chain_gas_schedule(gas_schedule: &std::collections::BTreeMap, feature_version: u64) -> Option { let mut params = $params_name::zeros(); $( - $crate::params::expand_get_for_gas_parameters!(params . $name, gas_schedule, $prefix, $(optional $($dummy)?)? $key); + if let Some(key) = $crate::params::define_gas_parameters_extract_key_at_version!($key_bindings, feature_version) { + params.$name = gas_schedule.get(&format!("{}.{}", $prefix, key)).cloned()?.into(); + } )* Some(params) @@ -38,8 +43,17 @@ macro_rules! define_gas_parameters { } impl $crate::gas_meter::ToOnChainGasSchedule for $params_name { - fn to_on_chain_gas_schedule(&self) -> Vec<(String, u64)> { - vec![$((format!("{}.{}", $prefix, $key), self.$name.into())),*] + #[allow(unused_variables)] + fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)> { + let mut output = vec![]; + + $( + if let Some(key) = $crate::params::define_gas_parameters_extract_key_at_version!($key_bindings, feature_version) { + output.push((format!("{}.{}", $prefix, key), self.$name.into())) + } + )* + + output } } @@ -59,47 +73,23 @@ macro_rules! define_gas_parameters { } } + #[test] - fn keys_should_be_unique() { - let mut map = std::collections::BTreeMap::<&str, ()>::new(); + fn keys_should_be_unique_for_all_versions() { + for ver in 0..=$crate::gas_meter::LATEST_GAS_FEATURE_VERSION { + let mut map = std::collections::BTreeMap::<&str, ()>::new(); - for key in [$($key),*] { - assert!(map.insert(key, ()).is_none()); + $( + if let Some(key) = $crate::params::define_gas_parameters_extract_key_at_version!($key_bindings, ver) { + if map.insert(key, ()).is_some() { + panic!("duplicated key {} at version {}", key, ver); + } + } + )* } } }; } pub(crate) use define_gas_parameters; -pub(crate) use expand_get_for_gas_parameters; - -#[cfg(test)] -mod tests { - use crate::gas_meter::FromOnChainGasSchedule; - - use super::*; - use move_core_types::gas_algebra::InternalGas; - - define_gas_parameters!( - GasParameters, - "test", - [[foo: InternalGas, "foo", 0], [bar: InternalGas, optional "bar", 0]] - ); - - #[test] - fn optional_should_be_honored() { - assert!( - matches!( - GasParameters::from_on_chain_gas_schedule( - &[("test.foo".to_string(), 0)].into_iter().collect(), - ), - Some(_) - ) - ); - - assert!(matches!( - GasParameters::from_on_chain_gas_schedule(&[].into_iter().collect()), - None - )); - } -} +pub(crate) use define_gas_parameters_extract_key_at_version; diff --git a/aptos-move/aptos-gas/src/transaction/mod.rs b/aptos-move/aptos-gas/src/transaction/mod.rs index a6e13ad4e9214..ed84816b19b16 100644 --- a/aptos-move/aptos-gas/src/transaction/mod.rs +++ b/aptos-move/aptos-gas/src/transaction/mod.rs @@ -99,30 +99,30 @@ crate::params::define_gas_parameters!( "write_data.per_byte_in_val", 10_000 ], - [memory_quota: AbstractValueSize, optional "memory_quota", 10_000_000], + [memory_quota: AbstractValueSize, { 1.. => "memory_quota" }, 10_000_000], [ free_write_bytes_quota: NumBytes, - optional "free_write_bytes_quota", + { 5.. => "free_write_bytes_quota" }, 1024, // 1KB free per state write ], [ max_bytes_per_write_op: NumBytes, - optional "max_bytes_per_write_op", + { 5.. => "max_bytes_per_write_op" }, 1 << 20, // a single state item is 1MB max ], [ max_bytes_all_write_ops_per_transaction: NumBytes, - optional "max_bytes_all_write_ops_per_transaction", + { 5.. => "max_bytes_all_write_ops_per_transaction" }, 10 << 20, // all write ops from a single transaction are 10MB max ], [ max_bytes_per_event: NumBytes, - optional "max_bytes_per_event", + { 5.. => "max_bytes_per_event" }, 1 << 20, // a single event is 1MB max ], [ max_bytes_all_events_per_transaction: NumBytes, - optional "max_bytes_all_events_per_transaction", + { 5.. => "max_bytes_all_events_per_transaction"}, 10 << 20, // all events from a single transaction are 10MB max ], ] From 33c71d8d1aff23137735e39ff8d2fa802bb4422f Mon Sep 17 00:00:00 2001 From: Victor Gao Date: Wed, 7 Dec 2022 01:30:08 +0000 Subject: [PATCH 3/3] [gas] versioned gas schedule -- natives --- api/src/context.rs | 5 +- aptos-move/aptos-gas/src/aptos_framework.rs | 18 ++-- aptos-move/aptos-gas/src/instr.rs | 12 +-- aptos-move/aptos-gas/src/misc.rs | 12 +-- aptos-move/aptos-gas/src/natives.rs | 106 +++++++------------- aptos-move/aptos-vm/src/aptos_vm_impl.rs | 4 +- aptos-move/e2e-move-tests/src/harness.rs | 3 +- aptos-move/vm-genesis/src/lib.rs | 2 +- testsuite/smoke-test/src/rest_api.rs | 8 +- testsuite/smoke-test/src/rosetta.rs | 8 +- testsuite/smoke-test/src/upgrade.rs | 2 +- 11 files changed, 81 insertions(+), 99 deletions(-) diff --git a/api/src/context.rs b/api/src/context.rs index c1af4fcb04f2a..cbaf7e3a9258d 100644 --- a/api/src/context.rs +++ b/api/src/context.rs @@ -911,14 +911,15 @@ impl Context { let gas_schedule_params = match GasScheduleV2::fetch_config(&storage_adapter).and_then(|gas_schedule| { + let feature_version = gas_schedule.feature_version; let gas_schedule = gas_schedule.to_btree_map(); - AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule) + AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, feature_version) }) { Some(gas_schedule) => Ok(gas_schedule), None => GasSchedule::fetch_config(&storage_adapter) .and_then(|gas_schedule| { let gas_schedule = gas_schedule.to_btree_map(); - AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule) + AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule, 0) }) .ok_or_else(|| { E::internal_with_code( diff --git a/aptos-move/aptos-gas/src/aptos_framework.rs b/aptos-move/aptos-gas/src/aptos_framework.rs index b30727c511085..2d948a4a0a076 100644 --- a/aptos-move/aptos-gas/src/aptos_framework.rs +++ b/aptos-move/aptos-gas/src/aptos_framework.rs @@ -72,8 +72,8 @@ crate::natives::define_gas_parameters_for_natives!(GasParameters, "aptos_framewo [.hash.sip_hash.base, "hash.sip_hash.base", 1000 * MUL], [.hash.sip_hash.per_byte, "hash.sip_hash.per_byte", 20 * MUL], - [.hash.keccak256.base, optional "hash.keccak256.base", 4000 * MUL], - [.hash.keccak256.per_byte, optional "hash.keccak256.per_byte", 45 * MUL], + [.hash.keccak256.base, { 1.. => "hash.keccak256.base" }, 4000 * MUL], + [.hash.keccak256.per_byte, { 1.. => "hash.keccak256.per_byte" }, 45 * MUL], [.type_info.type_of.base, "type_info.type_of.base", 300 * MUL], // TODO(Gas): the on-chain name is wrong... @@ -81,17 +81,17 @@ crate::natives::define_gas_parameters_for_natives!(GasParameters, "aptos_framewo [.type_info.type_name.base, "type_info.type_name.base", 300 * MUL], // TODO(Gas): the on-chain name is wrong... [.type_info.type_name.per_byte_in_str, "type_info.type_name.per_abstract_memory_unit", 5 * MUL], - [.type_info.chain_id.base, optional "type_info.chain_id.base", 150 * MUL], + [.type_info.chain_id.base, { 4.. => "type_info.chain_id.base" }, 150 * MUL], // Reusing SHA2-512's cost from Ristretto - [.hash.sha2_512.base, optional "hash.sha2_512.base", 3_240], - [.hash.sha2_512.per_byte, optional "hash.sha2_512.per_byte", 60], + [.hash.sha2_512.base, { 4.. => "hash.sha2_512.base" }, 3_240], + [.hash.sha2_512.per_byte, { 4.. => "hash.sha2_512.per_byte" }, 60], // Back-of-the-envelop approximation from SHA3-256's (4000 base, 45 per-byte) costs - [.hash.sha3_512.base, optional "hash.sha3_512.base", 4_500], - [.hash.sha3_512.per_byte, optional "hash.sha3_512.per_byte", 50], + [.hash.sha3_512.base, { 4.. => "hash.sha3_512.base" }, 4_500], + [.hash.sha3_512.per_byte, { 4.. => "hash.sha3_512.per_byte" }, 50], // Using SHA2-256's cost - [.hash.ripemd160.base, optional "hash.ripemd160.base", 3000], - [.hash.ripemd160.per_byte, optional "hash.ripemd160.per_byte", 50], + [.hash.ripemd160.base, { 4.. => "hash.ripemd160.base" }, 3000], + [.hash.ripemd160.per_byte, { 4.. => "hash.ripemd160.per_byte" }, 50], [.util.from_bytes.base, "util.from_bytes.base", 300 * MUL], [.util.from_bytes.per_byte, "util.from_bytes.per_byte", 5 * MUL], diff --git a/aptos-move/aptos-gas/src/instr.rs b/aptos-move/aptos-gas/src/instr.rs index 5aa1cbd267f91..10800efd83b46 100644 --- a/aptos-move/aptos-gas/src/instr.rs +++ b/aptos-move/aptos-gas/src/instr.rs @@ -29,11 +29,11 @@ crate::params::define_gas_parameters!( // stack [pop: InternalGas, "pop", 40 * MUL], [ld_u8: InternalGas, "ld_u8", 60 * MUL], - [ld_u16: InternalGas, optional "ld_u16", 60 * MUL], - [ld_u32: InternalGas, optional "ld_u32", 60 * MUL], + [ld_u16: InternalGas, { 5.. => "ld_u16" }, 60 * MUL], + [ld_u32: InternalGas, { 5.. => "ld_u32" }, 60 * MUL], [ld_u64: InternalGas, "ld_u64", 60 * MUL], [ld_u128: InternalGas, "ld_u128", 80 * MUL], - [ld_u256: InternalGas, optional "ld_u256", 80 * MUL], + [ld_u256: InternalGas, { 5.. => "ld_u256" }, 80 * MUL], [ld_true: InternalGas, "ld_true", 60 * MUL], [ld_false: InternalGas, "ld_false", 60 * MUL], [ld_const_base: InternalGas, "ld_const.base", 650 * MUL], @@ -110,11 +110,11 @@ crate::params::define_gas_parameters!( [freeze_ref: InternalGas, "freeze_ref", 10 * MUL], // casting [cast_u8: InternalGas, "cast_u8", 120 * MUL], - [cast_u16: InternalGas, optional "cast_u16", 120 * MUL], - [cast_u32: InternalGas, optional "cast_u32", 120 * MUL], + [cast_u16: InternalGas, { 5.. => "cast_u16" }, 120 * MUL], + [cast_u32: InternalGas, { 5.. => "cast_u32" }, 120 * MUL], [cast_u64: InternalGas, "cast_u64", 120 * MUL], [cast_u128: InternalGas, "cast_u128", 120 * MUL], - [cast_u256: InternalGas, optional "cast_u256", 120 * MUL], + [cast_u256: InternalGas, { 5.. => "cast_u256" }, 120 * MUL], // arithmetic [add: InternalGas, "add", 160 * MUL], [sub: InternalGas, "sub", 160 * MUL], diff --git a/aptos-move/aptos-gas/src/misc.rs b/aptos-move/aptos-gas/src/misc.rs index 12eab8a4e9679..e725f0b5a6f38 100644 --- a/aptos-move/aptos-gas/src/misc.rs +++ b/aptos-move/aptos-gas/src/misc.rs @@ -17,26 +17,26 @@ crate::params::define_gas_parameters!( [ // abstract value size [u8: AbstractValueSize, "u8", 40], - [u16: AbstractValueSize, optional "u16", 40], - [u32: AbstractValueSize, optional "u32", 40], + [u16: AbstractValueSize, { 5.. => "u16" }, 40], + [u32: AbstractValueSize, { 5.. => "u32" }, 40], [u64: AbstractValueSize, "u64", 40], [u128: AbstractValueSize, "u128", 40], - [u256: AbstractValueSize, optional "u256", 40], + [u256: AbstractValueSize, { 5.. => "u256" }, 40], [bool: AbstractValueSize, "bool", 40], [address: AbstractValueSize, "address", 40], [struct_: AbstractValueSize, "struct", 40], [vector: AbstractValueSize, "vector", 40], [reference: AbstractValueSize, "reference", 40], [per_u8_packed: AbstractValueSizePerArg, "per_u8_packed", 1], - [per_u16_packed: AbstractValueSizePerArg, optional "per_u16_packed", 2], - [per_u32_packed: AbstractValueSizePerArg, optional "per_u32_packed", 4], + [per_u16_packed: AbstractValueSizePerArg, { 5.. => "per_u16_packed" }, 2], + [per_u32_packed: AbstractValueSizePerArg, { 5.. => "per_u32_packed" }, 4], [per_u64_packed: AbstractValueSizePerArg, "per_u64_packed", 8], [ per_u128_packed: AbstractValueSizePerArg, "per_u128_packed", 16 ], - [per_u256_packed: AbstractValueSizePerArg, optional "per_u256_packed", 32], + [per_u256_packed: AbstractValueSizePerArg, { 5.. => "per_u256_packed" }, 32], [ per_bool_packed: AbstractValueSizePerArg, "per_bool_packed", diff --git a/aptos-move/aptos-gas/src/natives.rs b/aptos-move/aptos-gas/src/natives.rs index c105f3a7b5834..74c2f730a52f5 100644 --- a/aptos-move/aptos-gas/src/natives.rs +++ b/aptos-move/aptos-gas/src/natives.rs @@ -4,30 +4,35 @@ //! This module defines some macros to help implement the mapping between the on-chain gas schedule //! and its rust representation. -macro_rules! expand_get_impl_for_native_gas_params { - ($params: ident $(.$field: ident)+, $map: ident, $prefix: literal, optional $key: literal) => { - if let Some(val) = $map.get(&format!("{}.{}", $prefix, $key)) { - $params $(.$field)+ = (*val).into(); - } - }; - ($params: ident $(.$field: ident)+, $map: ident, $prefix: literal, $key: literal) => { - $params $(.$field)+ = $map.get(&format!("{}.{}", $prefix, $key)).cloned()?.into(); +macro_rules! define_gas_parameters_for_natives_extract_key_at_version { + ($key: literal, $cur_ver: expr) => { + Some($key) }; + + ({ $($ver: pat => $key: literal),+ }, $cur_ver: expr) => { + match $cur_ver { + $($ver => Some($key)),+, + _ => None, + } + } } macro_rules! define_gas_parameters_for_natives { ( $param_ty: ty, $package_name: literal, - [$([$(.$field: ident)+, $(optional $($dummy: ident)?)? $key: literal, $initial_val: expr]),+ $(,)?] + [$([$(.$field: ident)+, $key_bindings: tt, $initial_val: expr]),+ $(,)?] $(, allow_unmapped = $allow_unmapped: expr)? ) => { impl crate::gas_meter::FromOnChainGasSchedule for $param_ty { + #[allow(unused_variables)] fn from_on_chain_gas_schedule(gas_schedule: &std::collections::BTreeMap, feature_version: u64) -> Option { let mut params = <$param_ty>::zeros(); $( - $crate::natives::expand_get_impl_for_native_gas_params!(params $(.$field)+, gas_schedule, $package_name, $(optional $($dummy)?)? $key); + if let Some(key) = $crate::natives::define_gas_parameters_for_natives_extract_key_at_version!($key_bindings, feature_version) { + params $(.$field)+ = gas_schedule.get(&format!("{}.{}", $package_name, key)).cloned()?.into(); + } )* Some(params) @@ -35,9 +40,17 @@ macro_rules! define_gas_parameters_for_natives { } impl crate::gas_meter::ToOnChainGasSchedule for $param_ty { + #[allow(unused_variables)] fn to_on_chain_gas_schedule(&self, feature_version: u64) -> Vec<(String, u64)> { - [$(($key, u64::from(self $(.$field)+))),*] - .into_iter().map(|(key, val)| (format!("{}.{}", $package_name, key), val)).collect() + let mut output = vec![]; + + $( + if let Some(key) = $crate::natives::define_gas_parameters_for_natives_extract_key_at_version!($key_bindings, feature_version) { + output.push((format!("{}.{}", $package_name, key), u64::from(self $(.$field)+))); + } + )* + + output } } @@ -54,16 +67,22 @@ macro_rules! define_gas_parameters_for_natives { } #[test] - fn keys_should_be_unique() { - let mut map = std::collections::BTreeMap::<&str, ()>::new(); + fn keys_should_be_unique_for_all_versions() { + for ver in 0..=$crate::gas_meter::LATEST_GAS_FEATURE_VERSION { + let mut map = std::collections::BTreeMap::<&str, ()>::new(); - for key in [$($key),*] { - if map.insert(key.clone(), ()).is_some() { - panic!("duplicated key {}", key); - } + $( + if let Some(key) = $crate::natives::define_gas_parameters_for_natives_extract_key_at_version!($key_bindings, ver) { + if map.insert(key, ()).is_some() { + panic!("duplicated key {} at version {}", key, ver); + } + } + )* } } + + #[test] fn paths_must_be_unique() { let mut map = std::collections::BTreeMap::<&str, ()>::new(); @@ -78,7 +97,7 @@ macro_rules! define_gas_parameters_for_natives { #[test] fn all_parameters_mapped() { let total = format!("{:?}", &<$param_ty>::zeros()).matches(": 0").count(); - let mapped = [$($key),*].len() $(+ $allow_unmapped)?; + let mapped = [$(stringify!($($field).*)),*].len() $(+ $allow_unmapped)?; if mapped != total { panic!("only {} out of the {} entries are mapped", mapped, total) } @@ -87,51 +106,4 @@ macro_rules! define_gas_parameters_for_natives { } pub(crate) use define_gas_parameters_for_natives; -pub(crate) use expand_get_impl_for_native_gas_params; - -#[cfg(test)] -mod tests { - use super::*; - use crate::gas_meter::{FromOnChainGasSchedule, LATEST_GAS_FEATURE_VERSION}; - use move_core_types::gas_algebra::InternalGas; - - #[derive(Debug, Clone)] - struct GasParameters { - pub foo: InternalGas, - pub bar: InternalGas, - } - - impl GasParameters { - pub fn zeros() -> Self { - Self { - foo: 0.into(), - bar: 0.into(), - } - } - } - - define_gas_parameters_for_natives!( - GasParameters, - "test", - [[.foo, "foo", 0], [.bar, optional "bar", 0]] - ); - - #[test] - fn optional_should_be_honored() { - assert!(matches!( - GasParameters::from_on_chain_gas_schedule( - &[("test.foo".to_string(), 0)].into_iter().collect(), - LATEST_GAS_FEATURE_VERSION - ), - Some(_) - )); - - assert!(matches!( - GasParameters::from_on_chain_gas_schedule( - &[].into_iter().collect(), - LATEST_GAS_FEATURE_VERSION - ), - None - )); - } -} +pub(crate) use define_gas_parameters_for_natives_extract_key_at_version; diff --git a/aptos-move/aptos-vm/src/aptos_vm_impl.rs b/aptos-move/aptos-vm/src/aptos_vm_impl.rs index 1b74199b20fa6..ae8362663772b 100644 --- a/aptos-move/aptos-vm/src/aptos_vm_impl.rs +++ b/aptos-move/aptos-vm/src/aptos_vm_impl.rs @@ -68,14 +68,14 @@ impl AptosVMImpl { let feature_version = gas_schedule.feature_version; let map = gas_schedule.to_btree_map(); ( - AptosGasParameters::from_on_chain_gas_schedule(&map), + AptosGasParameters::from_on_chain_gas_schedule(&map, feature_version), feature_version, ) } None => match GasSchedule::fetch_config(&storage) { Some(gas_schedule) => { let map = gas_schedule.to_btree_map(); - (AptosGasParameters::from_on_chain_gas_schedule(&map), 0) + (AptosGasParameters::from_on_chain_gas_schedule(&map, 0), 0) } None => (None, 0), }, diff --git a/aptos-move/e2e-move-tests/src/harness.rs b/aptos-move/e2e-move-tests/src/harness.rs index ffeb66490fcea..e0389aacc42cf 100644 --- a/aptos-move/e2e-move-tests/src/harness.rs +++ b/aptos-move/e2e-move-tests/src/harness.rs @@ -417,7 +417,8 @@ impl MoveHarness { // TODO: The AptosGasParameters::zeros() schedule doesn't do what we want, so // explicitly manipulating gas entries. Wasn't obvious from the gas code how to // do this differently then below, so perhaps improve this... - let entries = AptosGasParameters::initial().to_on_chain_gas_schedule(); + let entries = AptosGasParameters::initial() + .to_on_chain_gas_schedule(aptos_gas::LATEST_GAS_FEATURE_VERSION); let entries = entries .into_iter() .map(|(name, val)| { diff --git a/aptos-move/vm-genesis/src/lib.rs b/aptos-move/vm-genesis/src/lib.rs index 96d8db29178ab..80a5b9bdb5652 100644 --- a/aptos-move/vm-genesis/src/lib.rs +++ b/aptos-move/vm-genesis/src/lib.rs @@ -81,7 +81,7 @@ pub static GENESIS_KEYPAIR: Lazy<(Ed25519PrivateKey, Ed25519PublicKey)> = Lazy:: pub fn default_gas_schedule() -> GasScheduleV2 { GasScheduleV2 { feature_version: aptos_gas::LATEST_GAS_FEATURE_VERSION, - entries: AptosGasParameters::initial().to_on_chain_gas_schedule(), + entries: AptosGasParameters::initial().to_on_chain_gas_schedule(LATEST_GAS_FEATURE_VERSION), } } diff --git a/testsuite/smoke-test/src/rest_api.rs b/testsuite/smoke-test/src/rest_api.rs index 3a4ca4c402993..67704f90ef49e 100644 --- a/testsuite/smoke-test/src/rest_api.rs +++ b/testsuite/smoke-test/src/rest_api.rs @@ -78,8 +78,12 @@ async fn test_gas_estimation() { .await .unwrap() .into_inner(); - let gas_params = - AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule.to_btree_map()).unwrap(); + let feaure_version = gas_schedule.feature_version; + let gas_params = AptosGasParameters::from_on_chain_gas_schedule( + &gas_schedule.to_btree_map(), + feaure_version, + ) + .unwrap(); // No transactions should always return 1 as the estimated gas assert_eq!( diff --git a/testsuite/smoke-test/src/rosetta.rs b/testsuite/smoke-test/src/rosetta.rs index 03f73eef15da9..2ca80b46add9e 100644 --- a/testsuite/smoke-test/src/rosetta.rs +++ b/testsuite/smoke-test/src/rosetta.rs @@ -704,8 +704,12 @@ async fn test_block() { .await .unwrap() .into_inner(); - let gas_params = - AptosGasParameters::from_on_chain_gas_schedule(&gas_schedule.to_btree_map()).unwrap(); + let feaure_version = gas_schedule.feature_version; + let gas_params = AptosGasParameters::from_on_chain_gas_schedule( + &gas_schedule.to_btree_map(), + feaure_version, + ) + .unwrap(); let min_gas_price = u64::from(gas_params.txn.min_price_per_gas_unit); let private_key_0 = cli.private_key(0); diff --git a/testsuite/smoke-test/src/upgrade.rs b/testsuite/smoke-test/src/upgrade.rs index 487776f4cefb7..52e4dd1325d81 100644 --- a/testsuite/smoke-test/src/upgrade.rs +++ b/testsuite/smoke-test/src/upgrade.rs @@ -44,7 +44,7 @@ async fn test_upgrade_flow() { let gas_schedule = aptos_types::on_chain_config::GasScheduleV2 { feature_version: aptos_gas::LATEST_GAS_FEATURE_VERSION, - entries: gas_parameters.to_on_chain_gas_schedule(), + entries: gas_parameters.to_on_chain_gas_schedule(aptos_gas::LATEST_GAS_FEATURE_VERSION), }; let (_, update_gas_script) = generate_gas_upgrade_proposal(&gas_schedule, true, "".to_owned())