From 984f7dfdaf474aabcede56f48a72ccc41af62093 Mon Sep 17 00:00:00 2001 From: David Edey Date: Mon, 11 Nov 2024 04:37:07 +0000 Subject: [PATCH] tweak: Markups for #1910, and some auth-zone neatenings --- .../src/types/non_fungible_global_id.rs | 10 +- .../resource/auth_zone/auth_zone_substates.rs | 56 +++--- .../src/blueprints/resource/package.rs | 2 +- radix-engine/src/system/actor.rs | 9 + radix-engine/src/system/system_callback.rs | 7 + .../system/system_modules/auth/auth_module.rs | 179 ++++++++++++------ .../system_modules/auth/authorization.rs | 28 +-- .../system_modules/costing/costing_module.rs | 12 +- .../multithread_intent_processor.rs | 3 + 9 files changed, 208 insertions(+), 98 deletions(-) diff --git a/radix-common/src/types/non_fungible_global_id.rs b/radix-common/src/types/non_fungible_global_id.rs index f0a3f89cf2d..652ea449db8 100644 --- a/radix-common/src/types/non_fungible_global_id.rs +++ b/radix-common/src/types/non_fungible_global_id.rs @@ -110,9 +110,13 @@ impl From for GlobalCaller { } impl GlobalCaller { - // Check if actor is a frame-owned object. - // See auth_module.rs for details. - pub fn is_frame_owned(&self) -> bool { + /// Due to a workaround in SystemV1, frame-owned objects were inadvertently assigned a `GlobalCaller`, + /// and for backwards compatibility had it replaced by `FRAME_OWNED_GLOBAL_MARKER`. + /// + /// This function checks for that marker, to verify if the `GlobalCaller` is valid. + /// + /// See auth_module.rs for more details. + pub fn is_actually_frame_owned(&self) -> bool { match self { GlobalCaller::GlobalObject(x) => x.eq(&FRAME_OWNED_GLOBAL_MARKER), GlobalCaller::PackageBlueprint(_) => false, diff --git a/radix-engine/src/blueprints/resource/auth_zone/auth_zone_substates.rs b/radix-engine/src/blueprints/resource/auth_zone/auth_zone_substates.rs index 727d5cf01c5..d6f954e77bc 100644 --- a/radix-engine/src/blueprints/resource/auth_zone/auth_zone_substates.rs +++ b/radix-engine/src/blueprints/resource/auth_zone/auth_zone_substates.rs @@ -5,30 +5,42 @@ use radix_engine_interface::blueprints::resource::*; pub struct AuthZone { pub proofs: Vec, - // Virtualized resources, note that one cannot create proofs with virtual resources but only be used for AuthZone checks + pub simulate_all_proofs_under_resources: BTreeSet, + pub implicit_non_fungible_proofs: BTreeSet, + + pub direct_caller_package_address: Option, + pub global_caller: Option<(GlobalCaller, Reference)>, + + pub parent: Option, +} + +#[derive(ScryptoSbor)] +#[sbor(type_name = "AuthZone")] +/// This is just the same as `AuthZone`, but with old field names. +/// This allows us to have a fixed genesis schema for the resource package. +pub struct GenesisSchemaAuthZone { + pub proofs: Vec, pub virtual_resources: BTreeSet, pub virtual_non_fungibles: BTreeSet, - pub local_caller_package_address: Option, pub global_caller: Option<(GlobalCaller, Reference)>, - pub parent: Option, } impl AuthZone { pub fn new( proofs: Vec, - virtual_resources: BTreeSet, - virtual_non_fungibles: BTreeSet, - local_caller_package_address: Option, + simulate_all_proofs_under_resources: BTreeSet, + implicit_non_fungible_proofs: BTreeSet, + direct_caller_package_address: Option, global_caller: Option<(GlobalCaller, Reference)>, parent: Option, ) -> Self { Self { proofs, - virtual_resources, - virtual_non_fungibles, - local_caller_package_address, + simulate_all_proofs_under_resources, + implicit_non_fungible_proofs, + direct_caller_package_address, global_caller, parent, } @@ -38,34 +50,34 @@ impl AuthZone { &self.proofs } - pub fn virtual_resources(&self) -> &BTreeSet { - &self.virtual_resources + pub fn simulate_all_proofs_under_resources(&self) -> &BTreeSet { + &self.simulate_all_proofs_under_resources } - pub fn virtual_non_fungibles(&self) -> &BTreeSet { - &self.virtual_non_fungibles + pub fn implicit_non_fungible_proofs(&self) -> &BTreeSet { + &self.implicit_non_fungible_proofs } - pub fn local_virtual_non_fungibles(&self) -> BTreeSet { - let mut virtual_proofs = BTreeSet::new(); + pub fn local_implicit_non_fungible_proofs(&self) -> BTreeSet { + let mut local_implicit_non_fungible_proofs = BTreeSet::new(); // Local Caller package address - if let Some(local_package_address) = self.local_caller_package_address { + if let Some(local_package_address) = self.direct_caller_package_address { let non_fungible_global_id = NonFungibleGlobalId::package_of_direct_caller_badge(local_package_address); - virtual_proofs.insert(non_fungible_global_id); + local_implicit_non_fungible_proofs.insert(non_fungible_global_id); } // Global Caller if let Some((global_caller, _global_caller_reference)) = &self.global_caller { - if !global_caller.is_frame_owned() { + if !global_caller.is_actually_frame_owned() { let non_fungible_global_id = NonFungibleGlobalId::global_caller_badge(global_caller.clone()); - virtual_proofs.insert(non_fungible_global_id); + local_implicit_non_fungible_proofs.insert(non_fungible_global_id); } } - virtual_proofs + local_implicit_non_fungible_proofs } pub fn push(&mut self, proof: Proof) { @@ -77,9 +89,9 @@ impl AuthZone { } pub fn remove_signature_proofs(&mut self) { - self.virtual_resources + self.simulate_all_proofs_under_resources .retain(|x| x != &SECP256K1_SIGNATURE_RESOURCE && x != &ED25519_SIGNATURE_RESOURCE); - self.virtual_non_fungibles.retain(|x| { + self.implicit_non_fungible_proofs.retain(|x| { x.resource_address() != SECP256K1_SIGNATURE_RESOURCE && x.resource_address() != ED25519_SIGNATURE_RESOURCE }); diff --git a/radix-engine/src/blueprints/resource/package.rs b/radix-engine/src/blueprints/resource/package.rs index 680faf54fe5..7105d9e04b1 100644 --- a/radix-engine/src/blueprints/resource/package.rs +++ b/radix-engine/src/blueprints/resource/package.rs @@ -798,7 +798,7 @@ impl ResourceNativePackage { let mut fields = Vec::new(); fields.push(FieldSchema::static_field( - aggregator.add_child_type_and_descendents::(), + aggregator.add_child_type_and_descendents::(), )); let mut functions = index_map_new(); diff --git a/radix-engine/src/system/actor.rs b/radix-engine/src/system/actor.rs index 8c593c423e2..93f3c7c9d38 100644 --- a/radix-engine/src/system/actor.rs +++ b/radix-engine/src/system/actor.rs @@ -69,12 +69,21 @@ pub struct BlueprintHookActor { #[derive(Debug, Clone, ScryptoSbor, PartialEq, Eq)] pub enum Actor { + /// In System V1, there was an explicit call to initialize the transaction processor. + /// This call has to have an actor making the call, which is the Root. + /// + /// From V2 onwards, we don't have an explicit function call to initialize the transaction + /// processor - but we still temporarily set a CallFrameInit with a `Root` actor. + /// This is used to set up the initial AuthZone in [`MultiThreadIntentProcessor::init`]. + /// + /// [`MultiThreadIntentProcessor::init`]: crate::system::transaction::multithread_intent_processor::MultiThreadIntentProcessor::init Root, Method(MethodActor), Function(FunctionActor), BlueprintHook(BlueprintHookActor), } +// This is only used by `kernel_create_kernel_for_testing` in the testing framework. impl Default for Actor { fn default() -> Self { Self::Root diff --git a/radix-engine/src/system/system_callback.rs b/radix-engine/src/system/system_callback.rs index 5bb06a21d24..c99b620bb2e 100644 --- a/radix-engine/src/system/system_callback.rs +++ b/radix-engine/src/system/system_callback.rs @@ -241,6 +241,13 @@ impl SystemVersion { true } + pub fn should_inject_transaction_processor_proofs_in_call_function(&self) -> bool { + match self { + SystemVersion::V1 => true, + SystemVersion::V2 => false, + } + } + pub fn should_charge_for_transaction_intent(&self) -> bool { match self { SystemVersion::V1 => false, diff --git a/radix-engine/src/system/system_modules/auth/auth_module.rs b/radix-engine/src/system/system_modules/auth/auth_module.rs index f703e60ec86..3977f2215f5 100644 --- a/radix-engine/src/system/system_modules/auth/auth_module.rs +++ b/radix-engine/src/system/system_modules/auth/auth_module.rs @@ -45,9 +45,10 @@ pub struct Unauthorized { #[derive(Debug, Clone)] pub struct AuthModule { - /// Special-case the initial transaction processor function call and - /// add virtual resources to the transaction processor call frame - pub generate_transaction_processor_auth_zone: Option, + /// SystemV1 only - we special-case the initial transaction processor + /// function call and add virtual resources to the transaction processor + /// call frame + pub v1_transaction_processor_proofs_for_injection: Option, } pub enum AuthorizationCheckResult { @@ -73,17 +74,22 @@ pub enum ResolvedPermission { impl AuthModule { pub fn new() -> Self { Self { - generate_transaction_processor_auth_zone: None, + v1_transaction_processor_proofs_for_injection: None, } } pub fn new_with_transaction_processor_auth_zone(auth_zone_init: AuthZoneInit) -> Self { Self { - generate_transaction_processor_auth_zone: Some(auth_zone_init), + v1_transaction_processor_proofs_for_injection: Some(auth_zone_init), } } - fn on_call_function_auth_zone_params( + // In SystemV1, the transaction processor is initiated via a call_function, and we + // used this to inject the signature proofs and resource simulation. + // + // In SystemV2 and later, we initialize the auth zone directly, so no longer have + // need to do this check. + fn system_v1_resolve_injectable_transaction_processor_proofs( system: &mut SystemService, blueprint_id: &BlueprintId, ) -> Result<(BTreeSet, BTreeSet), RuntimeError> { @@ -94,7 +100,9 @@ impl AuthModule { let is_root_thread = system.kernel_get_current_stack_id_uncosted() == 0; if is_root_call_frame && is_root_thread { let auth_module = &system.kernel_get_system().modules.auth; - if let Some(auth_zone_init) = &auth_module.generate_transaction_processor_auth_zone { + if let Some(auth_zone_init) = &auth_module.v1_transaction_processor_proofs_for_injection + { + // This is an extra sanity check / defense in depth which I believe isn't strictly needed. let is_transaction_processor_blueprint = blueprint_id .package_address .eq(&TRANSACTION_PROCESSOR_PACKAGE) @@ -120,9 +128,25 @@ impl AuthModule { ) -> Result { // Create AuthZone let auth_zone = { - let (virtual_resources, virtual_non_fungibles) = - Self::on_call_function_auth_zone_params(system, blueprint_id)?; - Self::create_auth_zone(system, None, virtual_resources, virtual_non_fungibles)? + if system + .system() + .versioned_system_logic + .should_inject_transaction_processor_proofs_in_call_function() + { + let (simulate_all_proofs_under_resources, implicit_non_fungible_proofs) = + Self::system_v1_resolve_injectable_transaction_processor_proofs( + system, + blueprint_id, + )?; + Self::create_auth_zone( + system, + None, + simulate_all_proofs_under_resources, + implicit_non_fungible_proofs, + )? + } else { + Self::create_auth_zone(system, None, Default::default(), Default::default())? + } }; // Check authorization @@ -204,36 +228,45 @@ impl AuthModule { pub fn on_call_fn_mock( system: &mut SystemService, receiver: Option<(&NodeId, bool)>, - virtual_resources: BTreeSet, - virtual_non_fungibles: BTreeSet, + simulate_all_proofs_under_resources: BTreeSet, + implicit_non_fungible_proofs: BTreeSet, ) -> Result { - Self::create_auth_zone(system, receiver, virtual_resources, virtual_non_fungibles) + Self::create_auth_zone( + system, + receiver, + simulate_all_proofs_under_resources, + implicit_non_fungible_proofs, + ) } fn copy_global_caller( system: &mut SystemService, - node_id: &NodeId, + direct_caller_auth_zone_id: &NodeId, ) -> Result<(Option<(GlobalCaller, Reference)>, Option), RuntimeError> { - let handle = system.kernel_open_substate( - node_id, + let direct_caller_auth_zone_handle = system.kernel_open_substate( + direct_caller_auth_zone_id, MAIN_BASE_PARTITION, &AuthZoneField::AuthZone.into(), LockFlags::read_only(), SystemLockData::default(), )?; - let auth_zone = system - .kernel_read_substate(handle)? + let direct_caller_auth_zone = system + .kernel_read_substate(direct_caller_auth_zone_handle)? .as_typed::>() .unwrap(); - Ok((auth_zone.into_payload().global_caller, Some(handle))) + + Ok(( + direct_caller_auth_zone.into_payload().global_caller, + Some(direct_caller_auth_zone_handle), + )) } pub(crate) fn create_auth_zone( system: &mut SystemService, receiver: Option<(&NodeId, bool)>, - virtual_resources: BTreeSet, - virtual_non_fungibles: BTreeSet, + simulate_all_proofs_under_resources: BTreeSet, + implicit_non_fungible_proofs: BTreeSet, ) -> Result { let (auth_zone, parent_lock_handle) = { let is_global_context_change = if let Some((receiver, direct_access)) = receiver { @@ -243,58 +276,88 @@ impl AuthModule { true }; - let current_actor = system.current_actor(); - let local_package_address = current_actor.package_address(); + let direct_caller = system.current_actor(); + let direct_caller_package_address = direct_caller.package_address(); // Retrieve global caller property of next auth zone - let (global_caller, parent_lock_handle) = match current_actor { + let (global_caller, parent_lock_handle) = match direct_caller { Actor::Root | Actor::BlueprintHook(..) => (None, None), - Actor::Method(current_method_actor) => { - let node_visibility = - system.kernel_get_node_visibility_uncosted(¤t_method_actor.node_id); - let current_ref_origin = node_visibility - .reference_origin(current_method_actor.node_id) + Actor::Method(direct_caller_method_actor) => { + let direct_caller_reference_origin = system + .kernel_get_node_visibility_uncosted(&direct_caller_method_actor.node_id) + .reference_origin(direct_caller_method_actor.node_id) .unwrap(); - let self_auth_zone = current_method_actor.auth_zone; - match (current_ref_origin, is_global_context_change) { - // Actor is part of the global component state tree AND next actor is a global context change - (ReferenceOrigin::Global(address), true) => { - (Some((address.into(), Reference(self_auth_zone))), None) + let direct_caller_auth_zone = direct_caller_method_actor.auth_zone; + + match (direct_caller_reference_origin, is_global_context_change) { + // The direct caller is global AND this call is a global context change + (ReferenceOrigin::Global(direct_caller_global_address), true) => { + let global_caller_address = direct_caller_global_address.into(); + let global_caller_leaf_auth_zone_reference = + Reference(direct_caller_auth_zone); + ( + Some(( + global_caller_address, + global_caller_leaf_auth_zone_reference, + )), + None, + ) } - // Actor is part of the global component state tree AND next actor is NOT a global context change + // The direct caller is global AND this call is NOT a global context change + // e.g. the receiver is internal (ReferenceOrigin::Global(..), false) => { - Self::copy_global_caller(system, &self_auth_zone)? + Self::copy_global_caller(system, &direct_caller_auth_zone)? } - // Actor is a direct access reference + // The direct caller is a direct access reference (ReferenceOrigin::DirectlyAccessed, _) => (None, None), - // Actor is a non-global reference + // The direct caller is a borrowed non-global reference (ReferenceOrigin::SubstateNonGlobalReference(..), _) => (None, None), - // Actor is a frame-owned object + // The direct caller is a frame-owned object (ReferenceOrigin::FrameOwned, _) => { - // In the past frame-owned objects were inheriting the AuthZone of the caller. - // It was a critical issue, which could allow called components to eg. - // withdraw resources from the signing account. - // To prevent this we use TRANSACTION_TRACKER NodeId as a marker, that we are dealing with a frame-owned object. - // It is checked later on when virtual proofs for AuthZone are verified. - // Approach with such marker allows to keep backward compatibility with substate database. + // In the past, all frame-owned direct callers copied their global caller to their callee. + // This was a mistake, as it could allow frame-owned objects to use proofs from e.g. + // the transaction processor. + // + // A fix needed to be backwards-compatible (without changing the size of substates, which would + // affect the fee costs), and whilst the auth zone reference could be fixed by using a `Reference` + // to `self_auth_zone`, the global caller was harder. + // + // As a work-around, the `FRAME_OWNED_GLOBAL_MARKER = TRANSACTION_TRACKER` was used as a marker + // that the global caller was invalid and shouldn't be used. It is checked used to avoid adding + // a global caller implicit proof in this case. + let (caller, lock_handle) = - Self::copy_global_caller(system, &self_auth_zone)?; - ( - caller.map(|_| { - (FRAME_OWNED_GLOBAL_MARKER.into(), Reference(self_auth_zone)) - }), - lock_handle, - ) + Self::copy_global_caller(system, &direct_caller_auth_zone)?; + + // To avoid changing the size of the substate, we need to make that we replace Some + // with Some and None with None. + let global_caller = match caller { + Some(_) => { + let global_caller_address = FRAME_OWNED_GLOBAL_MARKER.into(); + let global_caller_leaf_auth_zone_reference = + Reference(direct_caller_auth_zone); + Some(( + global_caller_address, + global_caller_leaf_auth_zone_reference, + )) + } + None => None, + }; + + (global_caller, lock_handle) } } } Actor::Function(function_actor) => { - let self_auth_zone = function_actor.auth_zone; + let direct_caller_auth_zone = function_actor.auth_zone; let global_caller = function_actor.as_global_caller(); if is_global_context_change { - (Some((global_caller, Reference(self_auth_zone))), None) + ( + Some((global_caller, Reference(direct_caller_auth_zone))), + None, + ) } else { - Self::copy_global_caller(system, &self_auth_zone)? + Self::copy_global_caller(system, &direct_caller_auth_zone)? } } }; @@ -310,9 +373,9 @@ impl AuthModule { let auth_zone = AuthZone::new( vec![], - virtual_resources, - virtual_non_fungibles, - local_package_address, + simulate_all_proofs_under_resources, + implicit_non_fungible_proofs, + direct_caller_package_address, global_caller, auth_zone_parent, ); diff --git a/radix-engine/src/system/system_modules/auth/authorization.rs b/radix-engine/src/system/system_modules/auth/authorization.rs index af387165cb4..2d7c6cd459b 100644 --- a/radix-engine/src/system/system_modules/auth/authorization.rs +++ b/radix-engine/src/system/system_modules/auth/authorization.rs @@ -71,18 +71,20 @@ impl Authorization { handles.push(handle); { - let mut virtual_non_fungible_global_ids = BTreeSet::new(); - let virtual_resources = auth_zone.virtual_resources(); + let mut implicit_non_fungible_proofs = BTreeSet::new(); + let simulate_all_proofs_under_resources = + auth_zone.simulate_all_proofs_under_resources(); - virtual_non_fungible_global_ids.extend(auth_zone.virtual_non_fungibles().clone()); + implicit_non_fungible_proofs + .extend(auth_zone.implicit_non_fungible_proofs().clone()); let proofs = auth_zone.proofs(); // Check if check( proofs, - virtual_resources, - virtual_non_fungible_global_ids, + simulate_all_proofs_under_resources, + implicit_non_fungible_proofs, api, )? { pass = true; @@ -132,17 +134,21 @@ impl Authorization { .unwrap() .into_payload(); - // Check Local virtual non fungibles - let virtual_proofs = auth_zone.local_virtual_non_fungibles(); - if !virtual_proofs.is_empty() { - if check(&[], &btreeset!(), virtual_proofs, api)? { + // Check local implicit non fungible proofs + let local_implicit_non_fungible_proofs = auth_zone.local_implicit_non_fungible_proofs(); + if !local_implicit_non_fungible_proofs.is_empty() { + if check(&[], &btreeset!(), local_implicit_non_fungible_proofs, api)? { return Ok(true); } } // Check global caller's full auth zone - if let Some((_global_caller, global_caller_reference)) = &auth_zone.global_caller { - if Self::global_auth_zone_matches(api, &global_caller_reference.0, &check)? { + if let Some((_, global_caller_leaf_auth_zone_reference)) = &auth_zone.global_caller { + if Self::global_auth_zone_matches( + api, + &global_caller_leaf_auth_zone_reference.0, + &check, + )? { return Ok(true); } } diff --git a/radix-engine/src/system/system_modules/costing/costing_module.rs b/radix-engine/src/system/system_modules/costing/costing_module.rs index a7a77c45717..c5e0be27a7b 100644 --- a/radix-engine/src/system/system_modules/costing/costing_module.rs +++ b/radix-engine/src/system/system_modules/costing/costing_module.rs @@ -405,7 +405,9 @@ impl PrivilegedSystemModule for CostingModule { api: &mut impl SystemBasedKernelApi, invocation: &KernelInvocation, ) -> Result<(), RuntimeError> { - // Skip invocation costing for transaction processor + // This check only applies in SystemV1. + // In this case, there was a call from Root => Transaction Processor, which this check avoids charging for. + // From SystemV2 onwards, there is no explicit call, and the Root actor is simply overwritten. if api.kernel_get_system_state().current_call_frame.is_root() { return Ok(()); } @@ -495,7 +497,9 @@ impl> SystemModule for CostingMod }); } - // Skip invocation costing for transaction processor + // This check only applies in SystemV1. + // In this case, there was a call from Root => Transaction Processor, which this check avoids charging for. + // From SystemV2 onwards, there is no explicit call, and the Root actor is simply overwritten. if is_root { return Ok(()); } @@ -527,7 +531,9 @@ impl> SystemModule for CostingMod }); } - // Skip invocation costing for transaction processor + // This check only applies in SystemV1. + // In this case, there was a call from Root => Transaction Processor, which this check avoids charging for. + // From SystemV2 onwards, there is no explicit call, and the Root actor is simply overwritten. if api.system_state().current_call_frame.is_root() { return Ok(()); } diff --git a/radix-engine/src/system/transaction/multithread_intent_processor.rs b/radix-engine/src/system/transaction/multithread_intent_processor.rs index 41aabea673d..2271e9a6f4b 100644 --- a/radix-engine/src/system/transaction/multithread_intent_processor.rs +++ b/radix-engine/src/system/transaction/multithread_intent_processor.rs @@ -44,6 +44,9 @@ impl<'e> MultiThreadIntentProcessor<'e> { for (thread_id, intent) in executable.all_intents().enumerate() { api.kernel_switch_stack(thread_id)?; + // We create the auth zone for the transaction processor. + // This needs a `SystemService` to resolve the current actor (Root) to get the global caller (None), + // and also to create the node and substates. let mut system_service = SystemService::new(api); let simulate_every_proof_under_resources = intent .auth_zone_init