fix: 🐛 correctly emit mutations in events (#613)

* fix: 🐛 correctly emit mutations in events

This makes it so we emit the mutations with their value in the `MutationsApplied` and `MutationsAppliedForProvider` events, so providers that listen to them can process them accordingly and, for example, revert them during a reorg.

* fix: 🐛 correctly match trie remove mutations in task

Author: TDemeco

client/src/tasks/bsp_delete_file.rs

@@ -7,7 +7,7 @@ use shc_blockchain_service::events::{
 use shc_common::{
     consts::CURRENT_FOREST_KEY,
     traits::StorageEnableRuntime,
-    types::{FileKey, TrieMutation, TrieRemoveMutation},
+    types::{FileKey, TrieMutation},
 };
 use shc_file_manager::traits::FileStorage;
 use shc_forest_manager::traits::{ForestStorage, ForestStorageHandler};
@@ -202,7 +202,7 @@ where
             let file_key = FileKey::from(mutation.0);
 
             // Only process remove mutations in this task.
-            if mutation.1 != TrieMutation::Remove(TrieRemoveMutation::new()) {
+            if !matches!(mutation.1, TrieMutation::Remove(_)) {
                 debug!(target: LOG_TARGET, "Skipping non-remove mutation for file key {:?}", file_key);
                 continue;
             }

client/src/tasks/msp_delete_file.rs

@@ -6,7 +6,7 @@ use shc_blockchain_service::events::{
 };
 use shc_common::{
     traits::StorageEnableRuntime,
-    types::{FileKey, TrieMutation, TrieRemoveMutation},
+    types::{FileKey, TrieMutation},
 };
 use shc_file_manager::traits::FileStorage;
 use shc_forest_manager::traits::{ForestStorage, ForestStorageHandler};
@@ -106,8 +106,8 @@ where
             // Get the file key from the mutation.
             let file_key = FileKey::from(mutation.0);
 
-            // Only process remove mutations in this task..
-            if mutation.1 != TrieMutation::Remove(TrieRemoveMutation::new()) {
+            // Only process remove mutations in this task.
+            if !matches!(mutation.1, TrieMutation::Remove(_)) {
                 debug!(target: LOG_TARGET, "Skipping non-remove mutation for file key {:?}", file_key);
                 continue;
             }

pallets/file-system/src/mock.rs

@@ -27,7 +27,7 @@ use sp_runtime::{
     traits::{BlakeTwo256, Convert, ConvertBack, IdentifyAccount, IdentityLookup, Verify, Zero},
     BuildStorage, DispatchError, MultiSignature, Perbill, SaturatedConversion,
 };
-use sp_std::collections::btree_set::BTreeSet;
+use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet};
 use sp_trie::{CompactProof, LayoutV1, MemoryDB, TrieConfiguration, TrieLayout};
 use sp_weights::FixedFee;
 use std::{
@@ -557,7 +557,7 @@ where
         (
             MemoryDB<T::Hash>,
             Self::Key,
-            Vec<(Self::Key, Option<Vec<u8>>)>,
+            BTreeMap<Self::Key, TrieMutation>,
         ),
         DispatchError,
     > {
@@ -567,7 +567,7 @@ where
                 0 => *root,
                 _ => mutations.last().unwrap().0,
             },
-            Vec::new(),
+            BTreeMap::new(),
         ))
     }
 }

pallets/payment-streams/src/mock.rs

@@ -25,7 +25,7 @@ use sp_runtime::{
 use sp_runtime::{traits::Convert, BoundedBTreeSet};
 use sp_trie::{CompactProof, LayoutV1, MemoryDB, TrieConfiguration, TrieLayout};
 use sp_weights::Weight;
-use std::collections::BTreeSet;
+use std::collections::{BTreeMap, BTreeSet};
 
 type Block = frame_system::mocking::MockBlock<Test>;
 type Balance = u128;
@@ -381,12 +381,12 @@ where
         (
             MemoryDB<T::Hash>,
             Self::Key,
-            Vec<(Self::Key, Option<Vec<u8>>)>,
+            BTreeMap<Self::Key, TrieMutation>,
         ),
         DispatchError,
     > {
         // Just return the root as is with no mutations
-        Ok((MemoryDB::<T::Hash>::default(), *root, Vec::new()))
+        Ok((MemoryDB::<T::Hash>::default(), *root, BTreeMap::new()))
     }
 }
 

pallets/proofs-dealer/src/mock.rs

@@ -14,15 +14,15 @@ use frame_system::{pallet_prelude::BlockNumberFor, EnsureRoot, EnsureSigned};
 use shp_file_metadata::{FileMetadata, Fingerprint};
 use shp_traits::{
     CommitRevealRandomnessInterface, CommitmentVerifier, MaybeDebug, ProofSubmittersInterface,
-    TrieMutation, TrieProofDeltaApplier,
+    TrieMutation, TrieProofDeltaApplier, TrieRemoveMutation,
 };
 use shp_treasury_funding::NoCutTreasuryCutCalculator;
 use sp_core::{hashing::blake2_256, ConstU128, ConstU32, ConstU64, Hasher, H256};
 use sp_runtime::{
     traits::{BlakeTwo256, Convert, ConvertBack, IdentityLookup},
     BuildStorage, DispatchError, Perbill, SaturatedConversion,
 };
-use sp_std::collections::btree_set::BTreeSet;
+use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet};
 use sp_trie::{CompactProof, LayoutV1, MemoryDB, TrieConfiguration, TrieLayout};
 
 type Block = frame_system::mocking::MockBlock<Test>;
@@ -422,53 +422,57 @@ where
         (
             MemoryDB<T::Hash>,
             Self::Key,
-            Vec<(Self::Key, Option<Vec<u8>>)>,
+            BTreeMap<Self::Key, TrieMutation>,
         ),
         DispatchError,
     > {
         let last_key = mutations.last().unwrap().0;
 
         let db = MemoryDB::<T::Hash>::default();
 
-        let mutated_keys_and_values = mutations
-            .iter()
-            .map(|(key, mutation)| {
-                let value = match mutation {
-                    TrieMutation::Add(add_mutation) => Some(add_mutation.value.clone()),
-                    TrieMutation::Remove(_) => {
-                        let file_metadata: FileMetadata<
-                            { shp_constants::H_LENGTH },
-                            { shp_constants::FILE_CHUNK_SIZE },
-                            { shp_constants::FILE_SIZE_TO_CHALLENGES },
-                        > = match FileMetadata::new(
-                            1_u64.encode(),
-                            blake2_256(b"bucket").as_ref().to_vec(),
-                            b"path/to/file".to_vec(),
-                            1,
-                            Fingerprint::default().into(),
-                        ) {
-                            Ok(file_metadata) => file_metadata,
-                            Err(_) => {
-                                return Err(DispatchError::Other("Failed to create file metadata"))
-                            }
-                        };
-
-                        if key.as_ref() != [0; H_LENGTH] {
-                            Some(file_metadata.encode())
-                        } else {
-                            Some(vec![1, 2, 3, 4, 5, 6]) // We make it so the metadata is invalid for the empty key
+        let mut applied_mutations = BTreeMap::new();
+
+        for (key, mutation) in mutations {
+            match mutation {
+                TrieMutation::Add(add_mutation) => {
+                    applied_mutations.insert(*key, add_mutation.clone().into());
+                }
+                TrieMutation::Remove(_) => {
+                    let file_metadata: FileMetadata<
+                        { shp_constants::H_LENGTH },
+                        { shp_constants::FILE_CHUNK_SIZE },
+                        { shp_constants::FILE_SIZE_TO_CHALLENGES },
+                    > = match FileMetadata::new(
+                        1_u64.encode(),
+                        blake2_256(b"bucket").as_ref().to_vec(),
+                        b"path/to/file".to_vec(),
+                        1,
+                        Fingerprint::default().into(),
+                    ) {
+                        Ok(file_metadata) => file_metadata,
+                        Err(_) => {
+                            return Err(DispatchError::Other("Failed to create file metadata"))
                         }
+                    };
+
+                    if key.as_ref() != [0; H_LENGTH] {
+                        applied_mutations.insert(
+                            *key,
+                            TrieRemoveMutation::with_value(file_metadata.encode()).into(),
+                        );
+                    } else {
+                        applied_mutations.insert(
+                            *key,
+                            TrieRemoveMutation::with_value(vec![1, 2, 3, 4, 5, 6]).into(),
+                        ); // We make it so the metadata is invalid for the empty key
                     }
-                };
-                Ok((*key, value))
-            })
-            .collect::<Result<Vec<_>, _>>()?;
-
-        let mutated_keys_and_values = mutated_keys_and_values;
+                }
+            }
+        }
 
         // Return default db, the last key in mutations as the new root, and a
         // vector holding the supposedly mutated keys and values, so it is deterministic for testing.
-        Ok((db, last_key, mutated_keys_and_values))
+        Ok((db, last_key, applied_mutations))
     }
 }
 

pallets/proofs-dealer/src/tests.rs

@@ -17,6 +17,7 @@ use frame_system::{
     limits::BlockWeights, pallet_prelude::BlockNumberFor, BlockWeight, ConsumedWeight,
 };
 use pallet_storage_providers::HoldReason;
+use shp_file_metadata::{FileMetadata, Fingerprint};
 use shp_traits::{ProofsDealerInterface, ReadChallengeableProvidersInterface, TrieRemoveMutation};
 use sp_core::{blake2_256, Get, Hasher, H256};
 use sp_runtime::{
@@ -1628,6 +1629,21 @@ fn submit_proof_with_checkpoint_challenges_mutations_success() {
         // Dispatch submit proof extrinsic.
         assert_ok!(ProofsDealer::submit_proof(user.clone(), proof, None));
 
+        // Construct the file metadata that the mock returns for remove mutations.
+        // This must match the file metadata constructed in the mock's apply_delta.
+        let file_metadata: FileMetadata<
+            { shp_constants::H_LENGTH },
+            { shp_constants::FILE_CHUNK_SIZE },
+            { shp_constants::FILE_SIZE_TO_CHALLENGES },
+        > = FileMetadata::new(
+            1_u64.encode(),
+            blake2_256(b"bucket").as_ref().to_vec(),
+            b"path/to/file".to_vec(),
+            1,
+            Fingerprint::default().into(),
+        )
+        .expect("Failed to create file metadata");
+
         // Check that the event for mutations applied is emitted.
         System::assert_has_event(
             Event::MutationsAppliedForProvider {
@@ -1636,7 +1652,10 @@ fn submit_proof_with_checkpoint_challenges_mutations_success() {
                     .iter()
                     .filter_map(|custom_challenge| {
                         if custom_challenge.should_remove_key {
-                            Some((custom_challenge.key, TrieRemoveMutation::default().into()))
+                            Some((
+                                custom_challenge.key,
+                                TrieRemoveMutation::with_value(file_metadata.encode()).into(),
+                            ))
                         } else {
                             None
                         }

pallets/proofs-dealer/src/utils.rs

@@ -323,7 +323,7 @@ where
 
             if !mutations.is_empty() {
                 // Apply the mutations to the Forest.
-                let (_, new_root, mutated_keys_and_values) = <T::ForestVerifier as TrieProofDeltaApplier<
+                let (_, new_root, applied_mutations) = <T::ForestVerifier as TrieProofDeltaApplier<
                     T::MerkleTrieHashing,
                 >>::apply_delta(
                     &root, mutations.as_slice(), forest_proof
@@ -332,21 +332,24 @@ where
 
                 // Check that the number of mutated keys is the same as the mutations expected.
                 ensure!(
-                    mutated_keys_and_values.len() == mutations.len(),
+                    applied_mutations.len() == mutations.len(),
                     Error::<T>::UnexpectedNumberOfRemoveMutations
                 );
 
-                for (key, maybe_value) in mutated_keys_and_values.iter() {
+                for (key, mutation) in applied_mutations.iter() {
                     // Remove the mutated key from the list of `forest_keys_proven` to avoid having to verify the key proof.
                     forest_keys_proven.remove(key);
 
                     // Use the interface exposed by the Providers pallet to update the submitting Provider
                     // after the key removal if the key had a value.
-                    if let Some(trie_value) = maybe_value {
-                        ProvidersPalletFor::<T>::update_provider_after_key_removal(
-                            submitter, trie_value,
-                        )
-                        .map_err(|_| Error::<T>::FailedToApplyDelta)?;
+                    if let TrieMutation::Remove(trie_remove_mutation) = mutation {
+                        if let Some(trie_value) = &trie_remove_mutation.maybe_value {
+                            ProvidersPalletFor::<T>::update_provider_after_key_removal(
+                                submitter,
+                                &trie_value,
+                            )
+                            .map_err(|_| Error::<T>::FailedToApplyDelta)?;
+                        }
                     }
                 }
 
@@ -363,7 +366,7 @@ where
                 // Emit event of mutation applied.
                 Self::deposit_event(Event::MutationsAppliedForProvider {
                     provider_id: *submitter,
-                    mutations: mutations.to_vec(),
+                    mutations: applied_mutations.into_iter().collect(),
                     old_root: root,
                     new_root,
                 });
@@ -1097,16 +1100,15 @@ impl<T: pallet::Config> ProofsDealerInterface for Pallet<T> {
         let root = ProvidersPalletFor::<T>::get_root(*provider_id)
             .ok_or(Error::<T>::ProviderRootNotFound)?;
 
-        let (_, new_root, _) =
-            <T::ForestVerifier as TrieProofDeltaApplier<T::MerkleTrieHashing>>::apply_delta(
-                &root, mutations, proof,
-            )
-            .map_err(|_| Error::<T>::FailedToApplyDelta)?;
+        let (_, new_root, applied_mutations) = <T::ForestVerifier as TrieProofDeltaApplier<
+            T::MerkleTrieHashing,
+        >>::apply_delta(&root, mutations, proof)
+        .map_err(|_| Error::<T>::FailedToApplyDelta)?;
 
         // Emit event of mutation applied.
         Self::deposit_event(Event::MutationsAppliedForProvider {
             provider_id: *provider_id,
-            mutations: mutations.to_vec(),
+            mutations: applied_mutations.into_iter().collect(),
             old_root: root,
             new_root,
         });
@@ -1120,15 +1122,14 @@ impl<T: pallet::Config> ProofsDealerInterface for Pallet<T> {
         proof: &Self::ForestProof,
         event_info: Option<Vec<u8>>,
     ) -> Result<Self::MerkleHash, DispatchError> {
-        let (_, new_root, _) =
-            <T::ForestVerifier as TrieProofDeltaApplier<T::MerkleTrieHashing>>::apply_delta(
-                &root, mutations, proof,
-            )
-            .map_err(|_| Error::<T>::FailedToApplyDelta)?;
+        let (_, new_root, applied_mutations) = <T::ForestVerifier as TrieProofDeltaApplier<
+            T::MerkleTrieHashing,
+        >>::apply_delta(&root, mutations, proof)
+        .map_err(|_| Error::<T>::FailedToApplyDelta)?;
 
         // Emit event of mutation applied.
         Self::deposit_event(Event::MutationsApplied {
-            mutations: mutations.to_vec(),
+            mutations: applied_mutations.into_iter().collect(),
             old_root: *root,
             new_root,
             event_info,