From dd9a02feb9c7c8e78ed459fc4aefebef5b3e22be Mon Sep 17 00:00:00 2001 From: David Edey Date: Mon, 23 Sep 2024 11:55:03 +0100 Subject: [PATCH] tweak: SystemVersion is generic --- .../tests/kernel/kernel_open_substate.rs | 3 +- radix-engine-tests/tests/kernel/panics.rs | 2 +- radix-engine-tests/tests/vm/native_vm.rs | 6 +- radix-engine/src/system/module.rs | 19 +- radix-engine/src/system/system.rs | 9 +- radix-engine/src/system/system_callback.rs | 309 +++++++++++------- .../system_modules/execution_trace/module.rs | 8 +- .../src/transaction/transaction_executor.rs | 12 +- scrypto-test/src/environment/builder.rs | 3 +- scrypto-test/src/environment/types.rs | 2 +- .../src/ledger_simulator/ledger_simulator.rs | 6 +- 11 files changed, 231 insertions(+), 148 deletions(-) diff --git a/radix-engine-tests/tests/kernel/kernel_open_substate.rs b/radix-engine-tests/tests/kernel/kernel_open_substate.rs index d2b6b7d83e9..a89aac30bbf 100644 --- a/radix-engine-tests/tests/kernel/kernel_open_substate.rs +++ b/radix-engine-tests/tests/kernel/kernel_open_substate.rs @@ -22,8 +22,7 @@ pub fn test_open_substate_of_invisible_package_address() { .commit_each_protocol_update(&mut database); // Create kernel - let mut system = System::new( - SystemVersion::latest(), + let mut system = LatestSystem::new( Vm { scrypto_vm: &scrypto_vm, native_vm, diff --git a/radix-engine-tests/tests/kernel/panics.rs b/radix-engine-tests/tests/kernel/panics.rs index ae04403ae48..c1746b33efd 100644 --- a/radix-engine-tests/tests/kernel/panics.rs +++ b/radix-engine-tests/tests/kernel/panics.rs @@ -16,7 +16,7 @@ use scrypto_test::prelude::*; #[test] fn panics_at_the_system_layer_or_below_can_be_caught() { // Arrange - let mut kernel = MockKernel(PhantomData::>>); + let mut kernel = MockKernel(PhantomData::>>); let mut system_service = SystemService::new(&mut kernel); // Act diff --git a/radix-engine-tests/tests/vm/native_vm.rs b/radix-engine-tests/tests/vm/native_vm.rs index aa283350ba3..891f499c18b 100644 --- a/radix-engine-tests/tests/vm/native_vm.rs +++ b/radix-engine-tests/tests/vm/native_vm.rs @@ -65,8 +65,7 @@ fn panics_can_be_caught_in_the_native_vm_and_converted_into_results() { let native_vm = NativeVm::new_with_extension(Extension); let intent_hash = Hash([0; 32]); - let mut system = System::new( - SystemVersion::latest(), + let mut system = LatestSystem::new( Vm { scrypto_vm: &scrypto_vm, native_vm, @@ -133,8 +132,7 @@ fn any_panics_can_be_caught_in_the_native_vm_and_converted_into_results() { let native_vm = NativeVm::new_with_extension(NonStringPanicExtension); let intent_hash = Hash([0; 32]); - let mut system = System::new( - SystemVersion::latest(), + let mut system = LatestSystem::new( Vm { scrypto_vm: &scrypto_vm, native_vm, diff --git a/radix-engine/src/system/module.rs b/radix-engine/src/system/module.rs index d6854ea0ce5..4fcb48b53f3 100644 --- a/radix-engine/src/system/module.rs +++ b/radix-engine/src/system/module.rs @@ -36,19 +36,27 @@ impl<'a, K: KernelInternalApi + ?Sized> SystemModuleApiImpl<'a, K> { } pub trait SystemModuleApi { + type SystemVersionLogic: SystemVersionLogic; type SystemCallback: SystemCallbackObject; - fn system(&mut self) -> &mut System; + fn system(&mut self) -> &mut System; - fn system_state(&mut self) -> SystemState<'_, System>; + fn system_state( + &mut self, + ) -> SystemState<'_, System>; /// Gets the number of call frames that are currently in the call frame stack fn current_stack_depth(&self) -> usize; } -impl<'a, V: SystemCallbackObject, K: KernelInternalApi> + ?Sized> SystemModuleApi - for SystemModuleApiImpl<'a, K> +impl< + 'a, + L: SystemVersionLogic, + V: SystemCallbackObject, + K: KernelInternalApi> + ?Sized, + > SystemModuleApi for SystemModuleApiImpl<'a, K> { + type SystemVersionLogic = L; type SystemCallback = V; fn system(&mut self) -> &mut K::System { @@ -76,8 +84,9 @@ pub trait SystemModuleApiFor: SystemModuleAp impl< 'a, + L: SystemVersionLogic, V: SystemCallbackObject, - K: KernelInternalApi> + ?Sized, + K: KernelInternalApi> + ?Sized, M: ResolvableSystemModule + ?Sized, > SystemModuleApiFor for SystemModuleApiImpl<'a, K> { diff --git a/radix-engine/src/system/system.rs b/radix-engine/src/system/system.rs index 3e8f415fcce..90578fb8863 100644 --- a/radix-engine/src/system/system.rs +++ b/radix-engine/src/system/system.rs @@ -108,7 +108,7 @@ impl<'a, Y: SystemBasedKernelApi + ?Sized> SystemService<'a, Y> { self.api } - pub fn system(&mut self) -> &mut System { + pub fn system(&mut self) -> &mut System { self.api.kernel_get_system() } @@ -2168,12 +2168,7 @@ impl<'a, Y: SystemBasedKernelApi> SystemCostingApi for SystemServi &mut self, costing_entry: ClientCostingEntry, ) -> Result<(), RuntimeError> { - let system_logic = self - .api - .kernel_get_system_state() - .system - .versioned_system_logic; - if !system_logic.should_consume_cost_units(self.api) { + if !Y::SystemVersionLogic::should_consume_cost_units(self.api) { return Ok(()); } diff --git a/radix-engine/src/system/system_callback.rs b/radix-engine/src/system/system_callback.rs index 3db678a02a8..98dd52f7c0e 100644 --- a/radix-engine/src/system/system_callback.rs +++ b/radix-engine/src/system/system_callback.rs @@ -159,89 +159,114 @@ impl SystemVersion { pub const fn latest() -> Self { Self::V2 } +} + +pub trait SystemVersionLogic: Sized + 'static { + fn execute_transaction( + api: &mut Y, + executable: ExecutableTransaction, + global_address_reservations: Vec, + ) -> Result, RuntimeError>; + + fn should_consume_cost_units(api: &mut Y) -> bool; + + fn create_auth_module( + executable: &ExecutableTransaction, + ) -> Result; +} + +#[derive(Default)] +pub struct SystemVersionLogicV1; + +impl SystemVersionLogic for SystemVersionLogicV1 { + fn execute_transaction( + api: &mut Y, + executable: ExecutableTransaction, + global_address_reservations: Vec, + ) -> Result, RuntimeError> { + let mut system_service = SystemService::new(api); + let intent = executable + .intents() + .get(0) + .expect("This should have been checked in init"); + let rtn = system_service.call_function( + TRANSACTION_PROCESSOR_PACKAGE, + TRANSACTION_PROCESSOR_BLUEPRINT, + TRANSACTION_PROCESSOR_RUN_IDENT, + scrypto_encode(&TransactionProcessorRunInputEfficientEncodable { + manifest_encoded_instructions: intent.encoded_instructions.clone(), + global_address_reservations, + references: Rc::new(intent.references.clone()), + blobs: intent.blobs.clone(), + }) + .unwrap(), + )?; + let output: Vec = scrypto_decode(&rtn).unwrap(); + Ok(output) + } + + fn should_consume_cost_units(api: &mut Y) -> bool { + // Skip client-side costing requested by TransactionProcessor + if api.kernel_get_current_depth() == 1 { + return false; + } + + true + } fn create_auth_module( - &self, executable: &ExecutableTransaction, ) -> Result { - let auth_module = match self { - SystemVersion::V1 => { - // This isn't exactly a necessary check as node logic should protect against this - // but keep it here for sanity - let intent = if executable.intents().len() != 1 { - return Err(RejectionReason::TransactionNotYetSupported); - } else { - executable.intents().get(0).unwrap() - }; - AuthModule::new_with_transaction_processor_auth_zone(intent.auth_zone_init.clone()) - } - SystemVersion::V2 => AuthModule::new(), + // This isn't exactly a necessary check as node logic should protect against this + // but keep it here for sanity + let intent = if executable.intents().len() != 1 { + return Err(RejectionReason::TransactionNotYetSupported); + } else { + executable.intents().get(0).unwrap() }; + let auth_module = + AuthModule::new_with_transaction_processor_auth_zone(intent.auth_zone_init.clone()); + Ok(auth_module) } +} +#[derive(Default)] +pub struct SystemVersionLogicV2; +pub type LatestSystemVersion = SystemVersionLogicV2; + +impl SystemVersionLogic for SystemVersionLogicV2 { fn execute_transaction( - &self, api: &mut Y, executable: ExecutableTransaction, global_address_reservations: Vec, ) -> Result, RuntimeError> { - let output = match self { - SystemVersion::V1 => { - let mut system_service = SystemService::new(api); - let intent = executable - .intents() - .get(0) - .expect("This should have been checked in init"); - let rtn = system_service.call_function( - TRANSACTION_PROCESSOR_PACKAGE, - TRANSACTION_PROCESSOR_BLUEPRINT, - TRANSACTION_PROCESSOR_RUN_IDENT, - scrypto_encode(&TransactionProcessorRunInputEfficientEncodable { - manifest_encoded_instructions: intent.encoded_instructions.clone(), - global_address_reservations, - references: Rc::new(intent.references.clone()), - blobs: intent.blobs.clone(), - }) - .unwrap(), - )?; - let output: Vec = scrypto_decode(&rtn).unwrap(); - output - } - SystemVersion::V2 => { - let mut txn_threads = - MultiThreadedTxnProcessor::init(executable, global_address_reservations, api)?; - txn_threads.execute(api)?; - let output = txn_threads - .threads - .get_mut(0) - .unwrap() - .0 - .outputs - .drain(..) - .collect(); - txn_threads.cleanup(api)?; - output - } - }; + let mut txn_threads = + MultiThreadedTxnProcessor::init(executable, global_address_reservations, api)?; + txn_threads.execute(api)?; + let output = txn_threads + .threads + .get_mut(0) + .unwrap() + .0 + .outputs + .drain(..) + .collect(); + txn_threads.cleanup(api)?; Ok(output) } - pub fn should_consume_cost_units(&self, api: &mut Y) -> bool { - match self { - SystemVersion::V1 => { - // Skip client-side costing requested by TransactionProcessor - if api.kernel_get_current_depth() == 1 { - return false; - } - } - SystemVersion::V2 => {} - } - + fn should_consume_cost_units(_api: &mut Y) -> bool { true } + + fn create_auth_module( + _executable: &ExecutableTransaction, + ) -> Result { + Ok(AuthModule::new()) + } } #[derive(Clone)] @@ -293,7 +318,10 @@ impl SystemLockData { } /// Effectively a trait alias for `KernelApi>` -pub trait SystemBasedKernelApi: KernelApi> { +pub trait SystemBasedKernelApi: + KernelApi> +{ + type SystemVersionLogic: SystemVersionLogic; type SystemCallback: SystemCallbackObject; fn system_service(&mut self) -> SystemService<'_, Self> { @@ -301,14 +329,21 @@ pub trait SystemBasedKernelApi: KernelApi>> SystemBasedKernelApi for K { +impl< + L: SystemVersionLogic, + V: SystemCallbackObject, + K: KernelApi>, + > SystemBasedKernelApi for K +{ + type SystemVersionLogic = L; type SystemCallback = V; } /// Effectively a trait alias for `KernelInternalApi>` pub trait SystemBasedKernelInternalApi: - KernelInternalApi> + KernelInternalApi> { + type SystemVersionLogic: SystemVersionLogic; type SystemCallback: SystemCallbackObject; fn system_module_api(&mut self) -> SystemModuleApiImpl { @@ -316,9 +351,13 @@ pub trait SystemBasedKernelInternalApi: } } -impl>> SystemBasedKernelInternalApi - for K +impl< + L: SystemVersionLogic, + V: SystemCallbackObject, + K: KernelInternalApi>, + > SystemBasedKernelInternalApi for K { + type SystemVersionLogic = L; type SystemCallback = V; } @@ -334,24 +373,71 @@ impl>> SystemIni substate_db: &impl SubstateDatabase, execution_config: ExecutionConfig, callback_init: I, - ) -> Self { + ) -> (SystemVersion, Self) { let system_boot = SystemBoot::load(substate_db, &execution_config); - let self_init = SystemSelfInit::new( - execution_config, - system_boot.system_version(), - system_boot.into_parameters(), - ); - Self { + let version = system_boot.system_version(); + let self_init = + SystemSelfInit::new(execution_config.clone(), system_boot.into_parameters()); + let init = Self { self_init, callback_init, + }; + (version, init) + } + + pub fn load_checked_latest( + substate_db: &impl SubstateDatabase, + execution_config: ExecutionConfig, + callback_init: I, + ) -> VersionedSystemInit { + let (version, init) = Self::load(substate_db, execution_config, callback_init); + init.latest(version) + } + + pub fn latest( + self, + loaded_version: SystemVersion, + ) -> VersionedSystemInit { + self.with_checked_version(loaded_version, SystemVersion::latest()) + } + + pub fn v1(self, loaded_version: SystemVersion) -> VersionedSystemInit { + self.with_checked_version(loaded_version, SystemVersion::V1) + } + + pub fn v2(self, loaded_version: SystemVersion) -> VersionedSystemInit { + self.with_checked_version(loaded_version, SystemVersion::V2) + } + + fn with_checked_version( + self, + loaded_version: SystemVersion, + expected_version: SystemVersion, + ) -> VersionedSystemInit { + if expected_version != loaded_version { + panic!("{expected_version:?} was requested, but it didn't match the loaded system version {loaded_version:?}"); + }; + VersionedSystemInit { + self_init: self.self_init, + callback_init: self.callback_init, + version: PhantomData, } } } -impl>> InitializationParameters - for SystemInit +pub struct VersionedSystemInit< + L: SystemVersionLogic, + I: InitializationParameters>, +> { + pub self_init: SystemSelfInit, + pub callback_init: I, + version: PhantomData, +} + +impl>> + InitializationParameters for VersionedSystemInit { - type For = System; + type For = System; } pub struct SystemSelfInit { @@ -363,43 +449,41 @@ pub struct SystemSelfInit { // Configuration pub system_parameters: SystemParameters, - pub system_logic_version: SystemVersion, pub system_overrides: Option, } impl SystemSelfInit { - pub fn new( - execution_config: ExecutionConfig, - system_logic_version: SystemVersion, - system_parameters: SystemParameters, - ) -> Self { + pub fn new(execution_config: ExecutionConfig, system_parameters: SystemParameters) -> Self { Self { enable_kernel_trace: execution_config.enable_kernel_trace, enable_cost_breakdown: execution_config.enable_cost_breakdown, enable_debug_information: execution_config.enable_debug_information, execution_trace: execution_config.execution_trace, system_overrides: execution_config.system_overrides, - system_logic_version, system_parameters, } } } -pub struct System { - pub versioned_system_logic: SystemVersion, +pub type SystemV1 = System; +pub type SystemV2 = System; +pub type LatestSystem = System; + +pub struct System { pub callback: V, pub blueprint_cache: NonIterMap>, pub schema_cache: NonIterMap>, pub auth_cache: NonIterMap, pub modules: SystemModuleMixer, pub finalization: SystemFinalization, + version: PhantomData, } pub trait HasModules { fn modules_mut(&mut self) -> &mut SystemModuleMixer; } -impl HasModules for System { +impl HasModules for System { #[inline] fn modules_mut(&mut self) -> &mut SystemModuleMixer { &mut self.modules @@ -418,13 +502,8 @@ impl SystemFinalization { } } -impl System { - pub fn new( - versioned_system_logic: SystemVersion, - callback: V, - modules: SystemModuleMixer, - finalization: SystemFinalization, - ) -> Self { +impl System { + pub fn new(callback: V, modules: SystemModuleMixer, finalization: SystemFinalization) -> Self { Self { callback, blueprint_cache: NonIterMap::new(), @@ -432,7 +511,7 @@ impl System { schema_cache: NonIterMap::new(), modules, finalization, - versioned_system_logic, + version: PhantomData, } } @@ -484,7 +563,7 @@ impl System { } } -impl System { +impl System { #[cfg(not(feature = "alloc"))] fn print_executable(executable: &ExecutableTransaction) { println!("{:-^120}", "Executable"); @@ -1389,7 +1468,6 @@ impl System { init_input: SystemSelfInit, ) -> Result { let mut system_parameters = init_input.system_parameters; - let system_logic_version = init_input.system_logic_version; let mut enabled_modules = { let mut enabled_modules = EnabledModules::AUTH | EnabledModules::TRANSACTION_RUNTIME; @@ -1458,17 +1536,14 @@ impl System { on_apply_cost: Default::default(), }; - let auth_module = system_logic_version - .create_auth_module(&executable) - .map_err(|reason| { - let print_execution_summary = - enabled_modules.contains(EnabledModules::KERNEL_TRACE); - Self::create_non_commit_receipt( - TransactionResult::Reject(RejectResult { reason }), - print_execution_summary, - costing_module.clone(), - ) - })?; + let auth_module = L::create_auth_module(&executable).map_err(|reason| { + let print_execution_summary = enabled_modules.contains(EnabledModules::KERNEL_TRACE); + Self::create_non_commit_receipt( + TransactionResult::Reject(RejectResult { reason }), + print_execution_summary, + costing_module.clone(), + ) + })?; let module_mixer = SystemModuleMixer::new( enabled_modules, @@ -1487,8 +1562,8 @@ impl System { } } -impl KernelTransactionExecutor for System { - type Init = SystemInit; +impl KernelTransactionExecutor for System { + type Init = VersionedSystemInit; type Executable = ExecutableTransaction; type ExecutionOutput = Vec; type Receipt = TransactionReceipt; @@ -1504,7 +1579,6 @@ impl KernelTransactionExecutor for System { Self::print_executable(executable); } - let logic_version = init_input.self_init.system_logic_version; let mut modules = Self::resolve_modules(executable, init_input.self_init)?; // NOTE: Have to use match pattern rather than map_err to appease the borrow checker @@ -1584,7 +1658,6 @@ impl KernelTransactionExecutor for System { }; let system = System::new( - logic_version, callback, modules, SystemFinalization { @@ -1617,13 +1690,7 @@ impl KernelTransactionExecutor for System { global_address_reservations.push(global_address_reservation); } - let system_logic_version = system_service.system().versioned_system_logic; - - let output = system_logic_version.execute_transaction( - api, - executable, - global_address_reservations, - )?; + let output = L::execute_transaction(api, executable, global_address_reservations)?; Ok(output) } @@ -1713,7 +1780,7 @@ impl KernelTransactionExecutor for System { } } -impl KernelCallbackObject for System { +impl KernelCallbackObject for System { type LockData = SystemLockData; type CallFrameData = Actor; diff --git a/radix-engine/src/system/system_modules/execution_trace/module.rs b/radix-engine/src/system/system_modules/execution_trace/module.rs index c17dfdc9990..31b8eaccb79 100644 --- a/radix-engine/src/system/system_modules/execution_trace/module.rs +++ b/radix-engine/src/system/system_modules/execution_trace/module.rs @@ -103,8 +103,12 @@ trait SystemModuleApiResourceSnapshotExtension { fn read_proof_uncosted(&self, proof_id: &NodeId) -> Option; } -impl<'a, V: SystemCallbackObject, K: KernelInternalApi>> - SystemModuleApiResourceSnapshotExtension for SystemModuleApiImpl<'a, K> +impl< + 'a, + L: SystemVersionLogic, + V: SystemCallbackObject, + K: KernelInternalApi>, + > SystemModuleApiResourceSnapshotExtension for SystemModuleApiImpl<'a, K> { fn read_bucket_uncosted(&self, bucket_id: &NodeId) -> Option { let (is_fungible_bucket, resource_address) = if let Some(substate) = diff --git a/radix-engine/src/transaction/transaction_executor.rs b/radix-engine/src/transaction/transaction_executor.rs index 96366926f25..0e70d28f2a1 100644 --- a/radix-engine/src/transaction/transaction_executor.rs +++ b/radix-engine/src/transaction/transaction_executor.rs @@ -265,8 +265,16 @@ pub fn execute_transaction<'v, V: VmInitialize>( executable: ExecutableTransaction, ) -> TransactionReceipt { let vm_init = VmInit::load(substate_db, vm_modules); - let system_init = SystemInit::load(substate_db, execution_config.clone(), vm_init); - KernelInit::load(substate_db, system_init).execute(executable) + let (system_version, system_init) = + SystemInit::load(substate_db, execution_config.clone(), vm_init); + match system_version { + SystemVersion::V1 => { + KernelInit::load(substate_db, system_init.v1(system_version)).execute(executable) + } + SystemVersion::V2 => { + KernelInit::load(substate_db, system_init.v2(system_version)).execute(executable) + } + } } pub fn execute_and_commit_transaction<'s, V: VmInitialize>( diff --git a/scrypto-test/src/environment/builder.rs b/scrypto-test/src/environment/builder.rs index 1ecfc0b8573..cb3ccb90598 100644 --- a/scrypto-test/src/environment/builder.rs +++ b/scrypto-test/src/environment/builder.rs @@ -236,8 +236,7 @@ where on_apply_cost: Default::default(), }; - System::new( - SystemVersion::latest(), + LatestSystem::new( Vm { scrypto_vm, native_vm: native_vm.clone(), diff --git a/scrypto-test/src/environment/types.rs b/scrypto-test/src/environment/types.rs index bfb526a0bfe..6a2d49de804 100644 --- a/scrypto-test/src/environment/types.rs +++ b/scrypto-test/src/environment/types.rs @@ -5,6 +5,6 @@ use crate::prelude::*; pub type TestVm<'g> = Vm<'g, DefaultWasmEngine, NoExtension>; pub type TestTrack<'g, D> = Track<'g, D>; -pub type TestSystemConfig<'g> = System>; +pub type TestSystemConfig<'g> = LatestSystem>; pub type TestKernel<'g, D> = Kernel<'g, TestSystemConfig<'g>, TestTrack<'g, D>>; pub type TestSystemService<'g, D> = SystemService<'g, TestKernel<'g, D>>; diff --git a/scrypto-test/src/ledger_simulator/ledger_simulator.rs b/scrypto-test/src/ledger_simulator/ledger_simulator.rs index 37f294cb78f..6dd6b8f64cf 100644 --- a/scrypto-test/src/ledger_simulator/ledger_simulator.rs +++ b/scrypto-test/src/ledger_simulator/ledger_simulator.rs @@ -1208,7 +1208,11 @@ impl LedgerSimulator { let vm_init = VmInit::load(&self.database, &self.vm_modules); let system_init = InjectCostingErrorInit { - system_input: SystemInit::load(&self.database, execution_config, vm_init), + system_input: SystemInit::load_checked_latest( + &self.database, + execution_config, + vm_init, + ), error_after_count, }; let kernel_init = KernelInit::load(&self.database, system_init);