chore: Refactor FileMetadata struct (#381)

* refactor file metadata struct; no pub fields, validate fields in `new` * use existing as_hash method for fingerprint to fixed array * handle `to_file_metadata` Result * fix: 🩹 Avoid impossible panic in file metadata * feat: ✨ Add check for empty files metadata to rocskdb as well * fix: 🩹 Remove unused error --------- Co-authored-by: Facundo Farall <37149322+ffarall@users.noreply.github.com>
Moonsong-Labs · Feb 26, 2025 · f9a7ac4 · f9a7ac4
1 parent d43e13e
commit f9a7ac4
Show file tree

Hide file tree

Showing 50 changed files with 920 additions and 646 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/api-augment/dist/interfaces/lookup.js b/api-augment/dist/interfaces/lookup.js
diff --git a/api-augment/dist/interfaces/lookup.js.map b/api-augment/dist/interfaces/lookup.js.map
diff --git a/api-augment/dist/types/interfaces/augment-api-errors.d.ts b/api-augment/dist/types/interfaces/augment-api-errors.d.ts
@@ -226,6 +226,14 @@ declare module "@polkadot/api-base/types/errors" {
        * Failed to verify proof: required to provide a proof of non-inclusion.
        **/
       ExpectedNonInclusionProof: AugmentedError<ApiType>;
+      /**
+       * Failed to compute file key
+       **/
+      FailedToComputeFileKey: AugmentedError<ApiType>;
+      /**
+       * Failed to create file metadata
+       **/
+      FailedToCreateFileMetadata: AugmentedError<ApiType>;
       /**
        * Failed to decode threshold.
        **/

diff --git a/api-augment/dist/types/interfaces/storagehubclient/types.d.ts b/api-augment/dist/types/interfaces/storagehubclient/types.d.ts
@@ -177,7 +177,12 @@ export interface QueryBucketsForMspError extends Enum {
 /** @name QueryConfirmChunksToProveForFileError */
 export interface QueryConfirmChunksToProveForFileError extends Enum {
   readonly isChallengedChunkToChunkIdError: boolean;
-  readonly type: "ChallengedChunkToChunkIdError";
+  readonly isFailedToCreateFileMetadata: boolean;
+  readonly isFailedToGenerateChunkChallenges: boolean;
+  readonly type:
+    | "ChallengedChunkToChunkIdError"
+    | "FailedToCreateFileMetadata"
+    | "FailedToGenerateChunkChallenges";
 }
 /** @name QueryEarliestChangeCapacityBlockError */
 export interface QueryEarliestChangeCapacityBlockError extends Enum {

diff --git a/api-augment/dist/types/interfaces/types-lookup.d.ts b/api-augment/dist/types/interfaces/types-lookup.d.ts
@@ -5725,6 +5725,8 @@ declare module "@polkadot/types/lookup" {
     readonly isNoPrivacyChange: boolean;
     readonly isOperationNotAllowedForInsolventProvider: boolean;
     readonly isOperationNotAllowedWhileBucketIsNotStoredByMsp: boolean;
+    readonly isFailedToComputeFileKey: boolean;
+    readonly isFailedToCreateFileMetadata: boolean;
     readonly type:
       | "StorageRequestAlreadyRegistered"
       | "StorageRequestNotFound"
@@ -5804,7 +5806,9 @@ declare module "@polkadot/types/lookup" {
       | "RootNotUpdated"
       | "NoPrivacyChange"
       | "OperationNotAllowedForInsolventProvider"
-      | "OperationNotAllowedWhileBucketIsNotStoredByMsp";
+      | "OperationNotAllowedWhileBucketIsNotStoredByMsp"
+      | "FailedToComputeFileKey"
+      | "FailedToCreateFileMetadata";
   }
   /** @name PalletProofsDealerProofSubmissionRecord (468) */
   interface PalletProofsDealerProofSubmissionRecord extends Struct {

diff --git a/api-augment/src/interfaces/augment-api-errors.ts b/api-augment/src/interfaces/augment-api-errors.ts
@@ -234,6 +234,14 @@ declare module "@polkadot/api-base/types/errors" {
        * Failed to verify proof: required to provide a proof of non-inclusion.
        **/
       ExpectedNonInclusionProof: AugmentedError<ApiType>;
+      /**
+       * Failed to compute file key
+       **/
+      FailedToComputeFileKey: AugmentedError<ApiType>;
+      /**
+       * Failed to create file metadata
+       **/
+      FailedToCreateFileMetadata: AugmentedError<ApiType>;
       /**
        * Failed to decode threshold.
        **/

diff --git a/api-augment/src/interfaces/lookup.ts b/api-augment/src/interfaces/lookup.ts
@@ -4714,7 +4714,9 @@ export default {
       "RootNotUpdated",
       "NoPrivacyChange",
       "OperationNotAllowedForInsolventProvider",
-      "OperationNotAllowedWhileBucketIsNotStoredByMsp"
+      "OperationNotAllowedWhileBucketIsNotStoredByMsp",
+      "FailedToComputeFileKey",
+      "FailedToCreateFileMetadata"
     ]
   },
   /**

diff --git a/api-augment/src/interfaces/storagehubclient/types.ts b/api-augment/src/interfaces/storagehubclient/types.ts
@@ -208,7 +208,12 @@ export interface QueryBucketsForMspError extends Enum {
 /** @name QueryConfirmChunksToProveForFileError */
 export interface QueryConfirmChunksToProveForFileError extends Enum {
   readonly isChallengedChunkToChunkIdError: boolean;
-  readonly type: "ChallengedChunkToChunkIdError";
+  readonly isFailedToCreateFileMetadata: boolean;
+  readonly isFailedToGenerateChunkChallenges: boolean;
+  readonly type:
+    | "ChallengedChunkToChunkIdError"
+    | "FailedToCreateFileMetadata"
+    | "FailedToGenerateChunkChallenges";
 }
 
 /** @name QueryEarliestChangeCapacityBlockError */

diff --git a/api-augment/src/interfaces/types-lookup.ts b/api-augment/src/interfaces/types-lookup.ts
@@ -6003,6 +6003,8 @@ declare module "@polkadot/types/lookup" {
     readonly isNoPrivacyChange: boolean;
     readonly isOperationNotAllowedForInsolventProvider: boolean;
     readonly isOperationNotAllowedWhileBucketIsNotStoredByMsp: boolean;
+    readonly isFailedToComputeFileKey: boolean;
+    readonly isFailedToCreateFileMetadata: boolean;
     readonly type:
       | "StorageRequestAlreadyRegistered"
       | "StorageRequestNotFound"
@@ -6082,7 +6084,9 @@ declare module "@polkadot/types/lookup" {
       | "RootNotUpdated"
       | "NoPrivacyChange"
       | "OperationNotAllowedForInsolventProvider"
-      | "OperationNotAllowedWhileBucketIsNotStoredByMsp";
+      | "OperationNotAllowedWhileBucketIsNotStoredByMsp"
+      | "FailedToComputeFileKey"
+      | "FailedToCreateFileMetadata";
   }
 
   /** @name PalletProofsDealerProofSubmissionRecord (468) */

diff --git a/api-augment/storagehub.json b/api-augment/storagehub.json
diff --git a/client/blockchain-service/Cargo.toml b/client/blockchain-service/Cargo.toml
@@ -58,6 +58,7 @@ storage-hub-runtime = { workspace = true }
 shc-actors-framework = { workspace = true }
 shc-forest-manager = { workspace = true }
 shc-common = { workspace = true }
+shp-constants = { workspace = true }
 shp-file-key-verifier = { workspace = true }
 shp-file-metadata = { workspace = true }
 

diff --git a/client/blockchain-service/src/utils.rs b/client/blockchain-service/src/utils.rs
@@ -1194,8 +1194,8 @@ where
                                     continue;
                                 }
 
-                                trace!(target: LOG_TARGET, "Applying on-chain Forest root mutations to BSP [{:?}]", provider_id);
-                                trace!(target: LOG_TARGET, "Mutations: {:?}", mutations);
+                                debug!(target: LOG_TARGET, "Applying on-chain Forest root mutations to BSP [{:?}]", provider_id);
+                                debug!(target: LOG_TARGET, "Mutations: {:?}", mutations);
 
                                 // Apply forest root changes to the BSP's Forest Storage.
                                 // At this point, we only apply the mutation of this file and its metadata to the Forest of this BSP,
@@ -1332,10 +1332,12 @@ where
         old_root: ForestRoot,
         new_root: ForestRoot,
     ) -> Result<()> {
+        debug!(target: LOG_TARGET, "Applying Forest mutations to Forest key [{:?}], reverting: {}, old root: {:?}, new root: {:?}", forest_key, revert, old_root, new_root);
+
         for (file_key, mutation) in mutations {
             // If we are reverting the Forest root changes, we need to revert the mutation.
             let mutation = if revert {
-                trace!(target: LOG_TARGET, "Reverting mutation [{:?}] with file key [{:?}]", mutation, file_key);
+                debug!(target: LOG_TARGET, "Reverting mutation [{:?}] with file key [{:?}]", mutation, file_key);
                 match self.revert_mutation(mutation) {
                     Ok(mutation) => mutation,
                     Err(e) => {
@@ -1344,7 +1346,7 @@ where
                     }
                 }
             } else {
-                trace!(target: LOG_TARGET, "Applying mutation [{:?}] with file key [{:?}]", mutation, file_key);
+                debug!(target: LOG_TARGET, "Applying mutation [{:?}] with file key [{:?}]", mutation, file_key);
                 mutation.clone()
             };
 
@@ -1370,7 +1372,7 @@ where
 
         let local_new_root = fs.read().await.root();
 
-        trace!(target: LOG_TARGET, "Mutations applied. New local Forest root: {:?}", local_new_root);
+        debug!(target: LOG_TARGET, "Mutations applied. New local Forest root: {:?}", local_new_root);
 
         if revert {
             if old_root != local_new_root {
@@ -1416,12 +1418,12 @@ where
                 value: encoded_metadata,
             }) => {
                 // Metadata comes encoded, so we need to decode it first to apply the mutation and add it to the Forest.
-                let metadata = FileMetadata::decode(&mut &encoded_metadata[..]).map_err(|e| {
+                let metadata = <FileMetadata<{shp_constants::H_LENGTH}, {shp_constants::FILE_CHUNK_SIZE}, {shp_constants::FILE_SIZE_TO_CHALLENGES}> as Decode>::decode(&mut &encoded_metadata[..]).map_err(|e| {
                     error!(target: LOG_TARGET, "CRITICAL❗️❗️ Failed to decode metadata from encoded metadata when applying mutation to Forest storage. This may result in a mismatch between the Forest root on-chain and in this node. \nThis is a critical bug. Please report it to the StorageHub team. \nError: {:?}", e);
                     anyhow!("Failed to decode metadata from encoded metadata: {:?}", e)
                 })?;
 
-                fs.write()
+                let inserted_file_keys = fs.write()
                     .await
                     .insert_files_metadata(vec![metadata].as_slice()).map_err(|e| {
                         error!(target: LOG_TARGET, "CRITICAL❗️❗️ Failed to apply mutation to Forest storage. This may result in a mismatch between the Forest root on-chain and in this node. \nThis is a critical bug. Please report it to the StorageHub team. \nError: {:?}", e);
@@ -1430,6 +1432,8 @@ where
                             e
                         )
                     })?;
+
+                debug!(target: LOG_TARGET, "Inserted file keys: {:?}", inserted_file_keys);
             }
             TrieMutation::Remove(_) => {
                 fs.write().await.delete_file_key(file_key).map_err(|e| {

diff --git a/client/common/src/types.rs b/client/common/src/types.rs
@@ -165,18 +165,27 @@ pub struct FileProof {
 }
 
 impl FileProof {
-    pub fn to_file_key_proof(&self, file_metadata: FileMetadata) -> FileKeyProof {
+    pub fn to_file_key_proof(
+        &self,
+        file_metadata: FileMetadata,
+    ) -> Result<FileKeyProof, FileProofError> {
         FileKeyProof::new(
-            file_metadata.owner.clone(),
-            file_metadata.bucket_id.clone(),
-            file_metadata.location.clone(),
-            file_metadata.file_size,
-            file_metadata.fingerprint,
+            file_metadata.owner().clone(),
+            file_metadata.bucket_id().clone(),
+            file_metadata.location().clone(),
+            file_metadata.file_size(),
+            file_metadata.fingerprint().clone(),
             self.proof.clone(),
         )
+        .map_err(|_| FileProofError::InvalidFileMetadata)
     }
 }
 
+#[derive(Debug, Clone)]
+pub enum FileProofError {
+    InvalidFileMetadata,
+}
+
 #[derive(Clone, Eq, Hash, PartialEq, Debug)]
 pub struct DownloadRequestId(u64);