-
Notifications
You must be signed in to change notification settings - Fork 11
fix: 🚑 make it so is_in_bucket is consistent across same file key records
#598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| -- This migration cannot be safely reversed because we don't track which specific | ||
| -- file records had incorrect is_in_bucket=false values before the normalization. | ||
| -- | ||
| -- Reverting this migration would require arbitrarily setting some records back to | ||
| -- is_in_bucket=false, which could recreate the inconsistent state we're trying to fix. | ||
| -- | ||
| -- If you need to rollback, the safest approach is to restore from a database backup | ||
| -- taken before this migration was applied. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| -- Normalize is_in_bucket status across all file records with the same file_key | ||
| -- | ||
| -- If ANY file record with a given file_key has is_in_bucket=true, then ALL | ||
| -- records for that file_key should have is_in_bucket=true. This is because | ||
| -- the bucket forest only contains one instance of each file_key, so if it's | ||
| -- in the bucket, all storage request records for that file should reflect that. | ||
|
|
||
| UPDATE file | ||
| SET is_in_bucket = true | ||
| WHERE file_key IN ( | ||
| -- Find all file_keys where at least one record has is_in_bucket=true | ||
| SELECT DISTINCT file_key | ||
| FROM file | ||
| WHERE is_in_bucket = true | ||
| ) | ||
| AND is_in_bucket = false; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -385,6 +385,13 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| let block_hash_bytes = block_hash.as_bytes().to_vec(); | ||
| let tx_hash_bytes = evm_tx_hash.map(|h| h.as_bytes().to_vec()); | ||
|
|
||
| // Check if this file key is already present in the bucket of the MSP | ||
| // In this scenario, this will always return false, since there's no other file record | ||
| // in the DB, but it's still good practice to check it. | ||
| let is_in_bucket = | ||
| File::is_file_key_in_bucket(conn, file_key.as_ref().to_vec()) | ||
| .await?; | ||
|
|
||
| // Create file with Requested step since we will change it to Stored when the storage request is fulfilled | ||
| File::create( | ||
| conn, | ||
|
|
@@ -399,6 +406,7 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| vec![], // No peer_ids available from confirmation event | ||
| block_hash_bytes, | ||
| tx_hash_bytes, | ||
| is_in_bucket, | ||
| ) | ||
| .await? | ||
| } | ||
|
|
@@ -445,6 +453,14 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| // Convert EVM tx hash to bytes if present | ||
| let tx_hash_bytes = evm_tx_hash.map(|h| h.as_bytes().to_vec()); | ||
|
|
||
| // Check if this file key is already present in the bucket of the MSP | ||
| // This could happen if there was a previous storage request for this file key that | ||
| // the MSP accepted, and the new storage request was issued by the user to add redundancy to it. | ||
| // We do this check because in this scenario,the `MutationsApplied` event won't be emitted for this | ||
| // file key when the MSP accepts it, as the MSP is already storing it. | ||
| let is_in_bucket = | ||
| File::is_file_key_in_bucket(conn, file_key.as_ref().to_vec()).await?; | ||
|
|
||
| File::create( | ||
| conn, | ||
| who, | ||
|
|
@@ -458,6 +474,7 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| sql_peer_ids, | ||
| block_hash_bytes, | ||
| tx_hash_bytes, | ||
| is_in_bucket, | ||
| ) | ||
| .await?; | ||
| } | ||
|
|
@@ -547,6 +564,12 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| let block_hash_bytes = block_hash.as_bytes().to_vec(); | ||
| let tx_hash_bytes = evm_tx_hash.map(|h| h.as_bytes().to_vec()); | ||
|
|
||
| // Check if this file key is already present in the bucket of the MSP | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So when the user ask for more redundancy the MSP needs to "re-accept" it, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, otherwise the new storage request will expire without an MSP response, so it will be considered rejected. |
||
| // In this scenario, this will always return false, since there's no other file record | ||
| // in the DB, but it's still a good practice to check it. | ||
| let is_in_bucket = | ||
| File::is_file_key_in_bucket(conn, file_key.as_ref().to_vec()).await?; | ||
|
|
||
| // Create file with Requested step since we will change it to Stored when the storage request is fulfilled | ||
| File::create( | ||
| conn, | ||
|
|
@@ -561,6 +584,7 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| vec![], // No peer_ids available from acceptance event | ||
| block_hash_bytes, | ||
| tx_hash_bytes, | ||
| is_in_bucket, | ||
| ) | ||
| .await? | ||
| } | ||
|
|
@@ -732,9 +756,9 @@ impl<Runtime: StorageEnableRuntime> IndexerService<Runtime> { | |
| // No storage, safe to delete immediately | ||
| File::delete(conn, file_record.id).await?; | ||
| log::debug!( | ||
| "Incomplete storage request for file key {:?} and id {:?} is not being stored, deleted immediately", | ||
| file_key, file_record.id | ||
| ); | ||
| "Incomplete storage request for file key {:?} and id {:?} is not being stored, deleted immediately", | ||
| file_key, file_record.id, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the only way this can happen is if the SR is already accepted by the MSP and the user send other one to increase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as that's the only way to have more than one file record for the same file key.