Skip to content

Commit

Permalink
amend review
Browse files Browse the repository at this point in the history
  • Loading branch information
@snowmead
snowmead committed Nov 12, 2024
1 parent db7f52a commit 8008ca2
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 58 deletions.
1 change: 0 additions & 1 deletion Cargo.lock
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 22 additions & 4 deletions node/src/tasks/bsp_upload_file.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ where
)
.await?;

let new_root: Option<H256> = events.and_then(|events| {
let maybe_new_root: Option<H256> = events.and_then(|events| {
events.into_iter().find_map(|event| {
if let storage_hub_runtime::RuntimeEvent::FileSystem(
pallet_file_system::Event::BspConfirmedStoring {
Expand All @@ -433,16 +433,35 @@ where
}
Some(new_root)
} else {
debug!(
target: LOG_TARGET,
"Received confirmation for another BSP: {:?}",
bsp_id
);
None
}
} else {
debug!(
target: LOG_TARGET,
"Received unexpected event: {:?}",
event.event
);
None
}
})
});

let new_root = match maybe_new_root {
Some(new_root) => new_root,
None => {
let err_msg = "CRITICAL❗️❗️ This is a critical bug! Please report it to the StorageHub team. Failed to query BspConfirmedStoring new forest root after confirming storing.";
error!(target: LOG_TARGET, "{}", err_msg);
return Err(anyhow!(err_msg));
}
};

// Save `FileMetadata` of the successfully retrieved stored files in the forest storage (executed in closure to drop the read lock on the forest storage).
if !file_metadatas.is_empty() && new_root.is_some() {
if !file_metadatas.is_empty() {
fs.write().await.insert_files_metadata(
file_metadatas

Check warning on line 466 in node/src/tasks/bsp_upload_file.rs

View workflow job for this annotation

GitHub Actions / Check lint with clippy 

iterating on a map's values
.into_iter()
Expand All @@ -451,8 +470,7 @@ where
.as_slice(),
)?;

if fs.read().await.root() != new_root.expect("Impossible for new_root to be None; qed")
{
if fs.read().await.root() != new_root {
let err_msg =
"CRITICAL❗️❗️ This is a critical bug! Please report it to the StorageHub team. \nError forest root mismatch after confirming storing.";
error!(target: LOG_TARGET, err_msg);
Expand Down
86 changes: 42 additions & 44 deletions pallets/file-system/src/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1242,7 +1242,7 @@ where
)?;

// Create a queue to store the file keys and metadata to be processed.
let mut file_keys_and_metadatas: BoundedVec<
let mut file_keys_and_metadata: BoundedVec<
(MerkleHash<T>, Vec<u8>),
T::MaxBatchConfirmStorageRequests,
> = BoundedVec::new();
Expand Down Expand Up @@ -1361,7 +1361,7 @@ where
let file_metadata = storage_request_metadata.clone().to_file_metadata();
let encoded_trie_value = file_metadata.encode();
expect_or_err!(
file_keys_and_metadatas.try_push((file_key.0, encoded_trie_value)),
file_keys_and_metadata.try_push((file_key.0, encoded_trie_value)),
"Failed to push file key and metadata",
Error::<T>::FileMetadataProcessingQueueFull,
result
Expand Down Expand Up @@ -1418,55 +1418,53 @@ where
}
}

// Remove all the skipped file keys from file_keys_and_metadatas
file_keys_and_metadatas.retain(|(fk, _)| !skipped_file_keys.contains(fk));
// Remove all the skipped file keys from file_keys_and_metadata
file_keys_and_metadata.retain(|(fk, _)| !skipped_file_keys.contains(fk));

let new_root = if !file_keys_and_metadatas.is_empty() {
// Check if this is the first file added to the BSP's Forest. If so, initialise last tick proven by this BSP.
let old_root = expect_or_err!(
<T::Providers as shp_traits::ReadProvidersInterface>::get_root(bsp_id),
"Failed to get root for BSP, when it was already checked to be a BSP",
Error::<T>::NotABsp
);

if old_root == <T::Providers as shp_traits::ReadProvidersInterface>::get_default_root()
{
// This means that this is the first file added to the BSP's Forest.
<T::ProofDealer as shp_traits::ProofsDealerInterface>::initialise_challenge_cycle(
&bsp_id,
)?;

// Emit the corresponding event.
Self::deposit_event(Event::<T>::BspChallengeCycleInitialised {
who: sender.clone(),
bsp_id,
});
}
ensure!(
!file_keys_and_metadata.is_empty(),
Error::<T>::NoFileKeysToConfirm
);

let mutations = file_keys_and_metadatas
.iter()
.map(|(fk, metadata)| (*fk, TrieAddMutation::new(metadata.clone()).into()))
.collect::<Vec<_>>();
// Check if this is the first file added to the BSP's Forest. If so, initialise last tick proven by this BSP.
let old_root = expect_or_err!(
<T::Providers as shp_traits::ReadProvidersInterface>::get_root(bsp_id),
"Failed to get root for BSP, when it was already checked to be a BSP",
Error::<T>::NotABsp
);

// Compute new root after inserting new file keys in forest partial trie.
let new_root = <T::ProofDealer as shp_traits::ProofsDealerInterface>::apply_delta(
if old_root == <T::Providers as shp_traits::ReadProvidersInterface>::get_default_root() {
// This means that this is the first file added to the BSP's Forest.
<T::ProofDealer as shp_traits::ProofsDealerInterface>::initialise_challenge_cycle(
&bsp_id,
mutations.as_slice(),
&non_inclusion_forest_proof,
)?;

// Root should have changed.
ensure!(old_root != new_root, Error::<T>::RootNotUpdated);
// Emit the corresponding event.
Self::deposit_event(Event::<T>::BspChallengeCycleInitialised {
who: sender.clone(),
bsp_id,
});
}

let mutations = file_keys_and_metadata
.iter()
.map(|(fk, metadata)| (*fk, TrieAddMutation::new(metadata.clone()).into()))
.collect::<Vec<_>>();

// Compute new root after inserting new file keys in forest partial trie.
let new_root = <T::ProofDealer as shp_traits::ProofsDealerInterface>::apply_delta(
&bsp_id,
mutations.as_slice(),
&non_inclusion_forest_proof,
)?;

// Update root of BSP.
<T::Providers as shp_traits::MutateProvidersInterface>::update_root(bsp_id, new_root)?;
// Root should have changed.
ensure!(old_root != new_root, Error::<T>::RootNotUpdated);

new_root
} else {
return Err(Error::<T>::NoFileKeysToConfirm.into());
};
// Update root of BSP.
<T::Providers as shp_traits::MutateProvidersInterface>::update_root(bsp_id, new_root)?;

// This should not fail since `skipped_file_keys` purpously share the same bound as `file_keys_and_metadatas`.
// This should not fail since `skipped_file_keys` purposely share the same bound as `file_keys_and_metadata`.
let skipped_file_keys: BoundedVec<MerkleHash<T>, T::MaxBatchConfirmStorageRequests> = expect_or_err!(
skipped_file_keys.into_iter().collect::<Vec<_>>().try_into(),
"Failed to convert skipped_file_keys to BoundedVec",
Expand All @@ -1475,12 +1473,12 @@ where
);

let file_keys: BoundedVec<MerkleHash<T>, T::MaxBatchConfirmStorageRequests> = expect_or_err!(
file_keys_and_metadatas
file_keys_and_metadata
.into_iter()
.map(|(fk, _)| fk)
.collect::<Vec<_>>()
.try_into(),
"Failed to convert file_keys_and_metadatas to BoundedVec",
"Failed to convert file_keys_and_metadata to BoundedVec",
Error::<T>::TooManyStorageRequestResponses,
result
);
Expand Down
1 change: 0 additions & 1 deletion pallets/proofs-dealer/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ targets = ["x86_64-unknown-linux-gnu"]

[dependencies]
codec = { workspace = true }
log = { workspace = true }
scale-info = { workspace = true }

# Local
Expand Down
9 changes: 1 addition & 8 deletions pallets/proofs-dealer/src/utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1005,14 +1005,7 @@ impl<T: pallet::Config> ProofsDealerInterface for Pallet<T> {
<T::ForestVerifier as TrieProofDeltaApplier<T::MerkleTrieHashing>>::apply_delta(
&root, mutations, proof,
)
.map_err(|e| {
log::error!(
"Failed to apply delta for Provider {:?} with error: {:?}",
provider_id,
e
);
Error::<T>::FailedToApplyDelta
})?
.map_err(|_| Error::<T>::FailedToApplyDelta)?
.1,
)
}
Expand Down

0 comments on commit 8008ca2

Please sign in to comment.