fix(engine): only scope check new references for component state (#990)

Description
---
fix(engine): only scope check new references for component state
fix(engine): limit initial scope to transaction inputs
fix(engine): depositing buckets now also includes the resource in scope

Motivation and Context
---
A bug in the engine caused some transactions to fail unless all vaults
and resources contained in e.g an account were in scope regardless if
they were used. This PR changes the scope check to only check that newly
added references in component state are in scope, without requiring that
_every_ reference is in scope.

Explicitly limiting scope to inputs allows us to pass in a store that
contains more than just the transaction inputs for the current
transaction. This should be useful in future when we want to maintain a
store for potentially dependent transactions within a block. The
thinking is that we pass in a wrapped read-only persistent store, the
wrapper will track substate changes made by executing transactions in a
block and be able to return a commit diff for the block.

How Has This Been Tested?
---
Engine tests, manually by executing a transaction that previously failed
due to the scope issue.

What process can a PR reviewer use to test or verify this change?
---
In the wallet web UI, create an initial free coins account, then create
a new empty account, then click create free test coins on the empty
account. This transaction previously failed.

Breaking Changes
---

- [x] None
- [ ] Requires data directory to be deleted
- [ ] Other - Please specify

dan_layer/engine/src/runtime/impl.rs

@@ -114,7 +114,6 @@ use crate::{
         RuntimeInterface,
         RuntimeModule,
     },
-    state_store::AtomicDb,
     template::LoadedTemplate,
     transaction::TransactionProcessor,
 };
@@ -156,24 +155,10 @@ impl<TTemplateProvider: TemplateProvider<Template = LoadedTemplate>> RuntimeInte
             max_call_depth,
             network,
         };
-        runtime.initialize_initial_scope()?;
         runtime.invoke_modules_on_initialize()?;
         Ok(runtime)
     }
 
-    fn initialize_initial_scope(&self) -> Result<(), RuntimeError> {
-        self.tracker.write_with(|state| {
-            let store = state.store().state_store().clone();
-            let tx = store.read_access()?;
-            let scope_mut = state.current_call_scope_mut()?;
-            for (k, _) in tx.iter_raw() {
-                let address = SubstateId::from_bytes(k)?;
-                scope_mut.add_substate_to_owned(address);
-            }
-            Ok(())
-        })
-    }
-
     fn invoke_modules_on_initialize(&self) -> Result<(), RuntimeError> {
         for module in &self.modules {
             module.on_initialize(&self.tracker)?;
@@ -1090,8 +1075,8 @@ impl<TTemplateProvider: TemplateProvider<Template = LoadedTemplate>> RuntimeInte
                 args.assert_no_args("CreateVault")?;
 
                 self.tracker.write_with(|state| {
-                    let substate_id = SubstateId::Resource(*resource_address);
-                    let resource_lock = state.lock_substate(&substate_id, LockFlag::Read)?;
+                    let resource_substate_id = SubstateId::Resource(*resource_address);
+                    let resource_lock = state.lock_substate(&resource_substate_id, LockFlag::Read)?;
                     let resource = state.get_resource(&resource_lock)?;
 
                     // Require deposit permissions on the resource to create the vault (even if empty)
@@ -1124,8 +1109,10 @@ impl<TTemplateProvider: TemplateProvider<Template = LoadedTemplate>> RuntimeInte
                     );
                     state.unlock_substate(resource_lock)?;
 
-                    // The resource has been "claimed" by an empty vault
-                    state.current_call_scope_mut()?.move_node_to_owned(&substate_id)?;
+                    // The resource is not orphaned because of the new vault.
+                    state
+                        .current_call_scope_mut()?
+                        .move_node_to_owned(&resource_substate_id)?;
 
                     Ok(InvokeResult::encode(&vault_id)?)
                 })

dan_layer/engine/src/runtime/scope.rs

@@ -50,7 +50,7 @@ impl CallScope {
         this
     }
 
-    pub(super) fn set_auth_scope(&mut self, scope: AuthorizationScope) {
+    pub(crate) fn set_auth_scope(&mut self, scope: AuthorizationScope) {
         self.auth_scope = scope;
     }
 
@@ -168,7 +168,7 @@ impl CallScope {
     }
 
     /// Add a substate to the owned nodes set without checking if it is already in the scope. This is used when
-    /// initializing the root scope from the state store.
+    /// initializing the root scope from the state store and for resources used in buckets.
     pub fn add_substate_to_owned(&mut self, address: SubstateId) {
         self.referenced.swap_remove(&address);
         self.orphans.swap_remove(&address);
@@ -202,9 +202,19 @@ impl CallScope {
         self.auth_scope.update_from_child(child.auth_scope);
     }
 
-    pub fn include_in_scope(&mut self, values: &IndexedWellKnownTypes) {
+    pub fn include_owned_in_scope(&mut self, values: &IndexedWellKnownTypes) {
         for addr in values.referenced_substates() {
-            // These are never able to be brought into scope
+            // These are never able to bring these into scope
+            if addr.is_public_key_identity() || addr.is_transaction_receipt() {
+                continue;
+            }
+            self.add_substate_to_owned(addr);
+        }
+    }
+
+    pub fn include_refs_in_scope(&mut self, values: &IndexedWellKnownTypes) {
+        for addr in values.referenced_substates() {
+            // These are never able to bring these into scope
             if addr.is_public_key_identity() || addr.is_vault() || addr.is_transaction_receipt() {
                 continue;
             }
@@ -333,6 +343,7 @@ pub enum PushCallFrame {
     ForComponent {
         template_address: TemplateAddress,
         module_name: String,
+        component_scope: IndexedWellKnownTypes,
         component_lock: LockedSubstate,
         arg_scope: IndexedWellKnownTypes,
         entity_id: EntityId,
@@ -365,12 +376,14 @@ impl PushCallFrame {
             Self::ForComponent {
                 template_address,
                 module_name,
+                component_scope,
                 component_lock,
                 arg_scope,
                 entity_id,
             } => {
                 let mut frame = CallFrame::for_component(template_address, module_name, component_lock, entity_id);
-                frame.scope_mut().include_in_scope(&arg_scope);
+                frame.scope_mut().include_owned_in_scope(&component_scope);
+                frame.scope_mut().include_refs_in_scope(&arg_scope);
                 frame
             },
             Self::Static {
@@ -380,7 +393,7 @@ impl PushCallFrame {
                 entity_id,
             } => {
                 let mut frame = CallFrame::for_static(template_address, module_name, entity_id);
-                frame.scope_mut().include_in_scope(&arg_scope);
+                frame.scope_mut().include_refs_in_scope(&arg_scope);
                 frame
             },
         }

dan_layer/engine/src/runtime/tracker.rs

@@ -51,10 +51,9 @@ use tari_template_lib::{
 use crate::{
     runtime::{
         locking::LockedSubstate,
-        scope::PushCallFrame,
+        scope::{CallScope, PushCallFrame},
         working_state::WorkingState,
         workspace::Workspace,
-        AuthorizationScope,
         RuntimeError,
     },
     state_store::memory::MemoryStateStore,
@@ -72,14 +71,14 @@ impl StateTracker {
     pub fn new(
         state_store: MemoryStateStore,
         virtual_substates: VirtualSubstates,
-        initial_auth_scope: AuthorizationScope,
+        initial_call_scope: CallScope,
         transaction_hash: Hash,
     ) -> Self {
         Self {
             working_state: Arc::new(RwLock::new(WorkingState::new(
                 state_store,
                 virtual_substates,
-                initial_auth_scope,
+                initial_call_scope,
                 transaction_hash,
             ))),
             fee_checkpoint: Arc::new(Mutex::new(None)),
@@ -192,7 +191,7 @@ impl StateTracker {
             }
 
             let indexed = IndexedWellKnownTypes::from_value(&component.body.state)?;
-            state.validate_component_state(&indexed, true)?;
+            state.validate_component_state(None, &indexed)?;
 
             state.new_substate(substate_id.clone(), SubstateValue::Component(component))?;
 

dan_layer/engine/src/runtime/working_state.rs

@@ -61,7 +61,6 @@ use crate::{
         state_store::WorkingStateStore,
         tracker_auth::Authorization,
         ActionIdent,
-        AuthorizationScope,
         RuntimeError,
         TransactionCommitError,
     },
@@ -90,7 +89,7 @@ pub(super) struct WorkingState {
     last_instruction_output: Option<IndexedValue>,
     workspace: Workspace,
     call_frames: Vec<CallFrame>,
-    base_call_scope: CallScope,
+    initial_call_scope: CallScope,
 
     fee_state: FeeState,
 }
@@ -99,12 +98,9 @@ impl WorkingState {
     pub fn new(
         state_store: MemoryStateStore,
         virtual_substates: VirtualSubstates,
-        initial_auth_scope: AuthorizationScope,
+        initial_call_scope: CallScope,
         transaction_hash: Hash,
     ) -> Self {
-        let mut base_call_scope = CallScope::new();
-        base_call_scope.set_auth_scope(initial_auth_scope);
-
         Self {
             transaction_hash,
             events: Vec::new(),
@@ -123,7 +119,7 @@ impl WorkingState {
             virtual_substates,
             new_fee_claims: HashMap::default(),
             call_frames: Vec::new(),
-            base_call_scope,
+            initial_call_scope,
             fee_state: FeeState::new(),
             object_ids: ObjectIds::new(1000),
         }
@@ -193,17 +189,8 @@ impl WorkingState {
         let before = IndexedWellKnownTypes::from_value(component_mut.state())?;
         let ret = f(component_mut);
 
-        let indexed = IndexedWellKnownTypes::from_value(component_mut.state())?;
-
-        for existing_vault in before.vault_ids() {
-            // Vaults can never be removed from components
-            if !indexed.vault_ids().contains(existing_vault) {
-                return Err(RuntimeError::OrphanedSubstate {
-                    address: (*existing_vault).into(),
-                });
-            }
-        }
-        self.validate_component_state(&indexed, false)?;
+        let after = IndexedWellKnownTypes::from_value(component_mut.state())?;
+        self.validate_component_state(Some(&before), &after)?;
 
         Ok(ret)
     }
@@ -395,10 +382,19 @@ impl WorkingState {
         if !self.current_call_scope()?.is_bucket_in_scope(bucket_id) {
             return Err(RuntimeError::BucketNotFound { bucket_id });
         }
-        self.current_call_scope_mut()?.remove_bucket_from_scope(bucket_id);
-        self.buckets
+        let bucket = self
+            .buckets
             .remove(&bucket_id)
-            .ok_or(RuntimeError::BucketNotFound { bucket_id })
+            .ok_or(RuntimeError::BucketNotFound { bucket_id })?;
+
+        // Use of the bucket adds the resource to the scope
+        let resource_addr = *bucket.resource_address();
+        {
+            let scope_mut = self.current_call_scope_mut()?;
+            scope_mut.remove_bucket_from_scope(bucket_id);
+            scope_mut.add_substate_to_owned(resource_addr.into());
+        }
+        Ok(bucket)
     }
 
     pub fn burn_bucket(&mut self, bucket: Bucket) -> Result<(), RuntimeError> {
@@ -772,28 +768,42 @@ impl WorkingState {
 
     pub fn validate_component_state(
         &mut self,
-        indexed: &IndexedWellKnownTypes,
-        require_in_scope: bool,
+        previous_state: Option<&IndexedWellKnownTypes>,
+        next_state: &IndexedWellKnownTypes,
     ) -> Result<(), RuntimeError> {
-        let mut dup_check = HashSet::with_capacity(indexed.vault_ids().len());
-        for vault_id in indexed.vault_ids() {
+        // Check that no vaults were dropped
+        if let Some(prev_state) = previous_state {
+            for existing_vault in prev_state.vault_ids() {
+                // Vaults can never be removed from components
+                if !next_state.vault_ids().contains(existing_vault) {
+                    return Err(RuntimeError::OrphanedSubstate {
+                        address: (*existing_vault).into(),
+                    });
+                }
+            }
+        }
+
+        // Check that no vaults are duplicated
+        let mut dup_check = HashSet::with_capacity(next_state.vault_ids().len());
+        for vault_id in next_state.vault_ids() {
             if !dup_check.insert(vault_id) {
                 return Err(RuntimeError::DuplicateReference {
                     address: (*vault_id).into(),
                 });
             }
         }
-        // TODO: I think that we can clean this up a bit. We should always be checking the scope but there are edge
-        //       cases and it was just easier to have this conditional
-        if require_in_scope {
-            self.check_all_substates_in_scope(indexed)?;
-        } else {
-            self.check_all_substates_known(indexed)?;
-        }
+
+        let diff_values = previous_state.map(|prev_state| next_state.diff(prev_state));
+
+        // We only require newly added values to be in scope since previous values were already checked. For instance,
+        // if a transaction uses an account does not have to input all vaults and resources just to transact on a
+        // single vault.
+        let new_values = diff_values.as_ref().unwrap_or(next_state);
+        self.check_all_substates_in_scope(new_values)?;
 
         let scope_mut = self.current_call_scope_mut()?;
-        for address in indexed.referenced_substates() {
-            // Move orphaned objects to owned
+        for address in next_state.referenced_substates() {
+            // Mark any orphaned objects as owned
             scope_mut.move_node_to_owned(&address)?
         }
 
@@ -889,15 +899,15 @@ impl WorkingState {
             .call_frames
             .last_mut()
             .map(|s| s.scope_mut())
-            .unwrap_or(&mut self.base_call_scope))
+            .unwrap_or(&mut self.initial_call_scope))
     }
 
     pub fn current_call_scope(&self) -> Result<&CallScope, RuntimeError> {
         Ok(self
             .call_frames
             .last()
             .map(|f| f.scope())
-            .unwrap_or(&self.base_call_scope))
+            .unwrap_or(&self.initial_call_scope))
     }
 
     pub fn call_frame_depth(&self) -> usize {
@@ -958,7 +968,7 @@ impl WorkingState {
             // base to the first call scope)
             new_frame
                 .scope_mut()
-                .set_auth_scope(self.base_call_scope.auth_scope().clone());
+                .set_auth_scope(self.initial_call_scope.auth_scope().clone());
         }
 
         self.call_frames.push(new_frame);
@@ -994,14 +1004,14 @@ impl WorkingState {
     }
 
     pub fn base_call_scope(&self) -> &CallScope {
-        &self.base_call_scope
+        &self.initial_call_scope
     }
 
     pub fn take_state(&mut self) -> Self {
         let new_state = WorkingState::new(
             self.store.state_store().clone(),
-            Default::default(),
-            AuthorizationScope::new(vec![]),
+            VirtualSubstates::new(),
+            CallScope::new(),
             self.transaction_hash,
         );
         mem::replace(self, new_state)
@@ -1045,6 +1055,7 @@ impl WorkingState {
 
     pub fn check_all_substates_in_scope(&self, value: &IndexedWellKnownTypes) -> Result<(), RuntimeError> {
         let scope = self.current_call_scope()?;
+
         for addr in value.referenced_substates() {
             // You are allowed to reference existing root substates
             if addr.is_root() {

dan_layer/engine/src/transaction/processor.rs

@@ -52,7 +52,7 @@ use tari_utilities::ByteArray;
 
 use crate::{
     runtime::{
-        scope::PushCallFrame,
+        scope::{CallScope, PushCallFrame},
         AuthParams,
         AuthorizationScope,
         Runtime,
@@ -110,7 +110,14 @@ impl<TTemplateProvider: TemplateProvider<Template = LoadedTemplate> + 'static> T
         } = self;
 
         let initial_auth_scope = AuthorizationScope::new(auth_params.initial_ownership_proofs);
-        let tracker = StateTracker::new(state_db, virtual_substates, initial_auth_scope, transaction.hash());
+        let mut initial_call_scope = CallScope::new();
+        initial_call_scope.set_auth_scope(initial_auth_scope);
+        for input in transaction.all_inputs_iter() {
+            initial_call_scope.add_substate_to_owned(input.substate_id.clone());
+        }
+
+        let tracker = StateTracker::new(state_db, virtual_substates, initial_call_scope, transaction.hash());
+
         let runtime_interface = RuntimeInterfaceImpl::initialize(
             tracker,
             template_provider.clone(),
@@ -425,9 +432,12 @@ impl<TTemplateProvider: TemplateProvider<Template = LoadedTemplate> + 'static> T
             .map(IndexedWellKnownTypes::from_value)
             .collect::<Result<_, _>>()?;
 
+        let component_scope = IndexedWellKnownTypes::from_value(component.state())?;
+
         runtime.interface().push_call_frame(PushCallFrame::ForComponent {
             template_address,
             module_name: template.template_name().to_string(),
+            component_scope,
             component_lock: component_lock.clone(),
             arg_scope,
             entity_id: component.entity_id,