Skip to content
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

fix: 🚑 add missing update_payment_stream calls when deleting files #300

Merged
merged 23 commits into from
Dec 27, 2024

Conversation

TDemeco
Copy link
Contributor

@TDemeco TDemeco commented Dec 24, 2024

This PR fixes some lingering issues in the runtime regarding payment streams not being correctly updated for Buckets and BSPs when deleting files. Those fixes are:

  • In bsp_confirm_stop_storing, after the proof has been verified and the used capacity of the BSP has been decreased, added a calculation of the new amount provided from the BSP to the corresponding user and the update (or delete if this new amount is zero) of the payment stream between them. Since the user of the file was not available at this time I added it to the tuple found in PendingStopStoringRequests, which is set in the bsp_request_stop_storing extrinsic.
  • In the delete_file extrinsic, if the user supplied an inclusion proof for the file key it wants to delete, the bucket size was being decreased (and the payment stream was updated accordingly) but the new bucket root after the file deletion was not being calculated nor updated. Added that.
  • In the pending_file_deletion_request_submit_proof (used by MSPs when a user requests a file deletion), if the proof submitted by the MSP includes the file key to delete, the bucket size nor root were being updated (and the payment stream wasn't either). Added the bucket root update and the bucket size update (which also updates the payment stream).
    • To do this we needed the file size which was not readily available at the time the extrinsic was executing, so I added the file size to the tuple stored under PendingFileDeletionRequests. This gets set when the user calls the delete_file extrinsic and gets verified in the pending_file_deletion_request_submit_proof call.
  • Changed the update_provider_after_key_removal logic to delete the payment stream if the new amount provided after the file key deletion is zero. Previously, it was always calling update instead of delete and the call would fail. This function gets called after the mutations get applied in a submit_proof extrinsic call, to update the BSP's used capacity and payment streams.
  • Updated the forest verifier's apply_delta logic to first get the value (encoded file metadata) under the key to remove with .get() and then remove it with .remove(). This was needed as calling .remove() directly and handling the returned value as the leaf value was not giving correct results, because the .remove() call from the TrieDBMut structure returns the literal value under the key in the trie, which is almost always a node that contains the hash that points to the actual leaf value, and since the actual leaf value gets removed in the call, calling .get() afterwards for the obtained hash returned None. I believe this behaviour is quite confusing and should be fixed (or at least clarified) in trie-db.
  • Finally, after these last two fixes, now payment streams were correctly updating for trie remove mutations done in submit_proof, which meant that there was a new worst-case scenario to handle for this extrinsic (updating a payment stream for each mutation). I added a payment stream creation in the setup of the submit_proof benchmarks and recalculated the results.

@TDemeco TDemeco requested a review from ffarall December 24, 2024 03:31
@TDemeco TDemeco changed the title fix: 🚑 add missing update_payment_stream calls when adding/deleting files fix: 🚑 add missing update_payment_stream calls when deleting files Dec 24, 2024
@santikaplan
Copy link
Contributor

Fantastic PR! Thank you for spotting all these issues and taking the time to fix them! 💪

pallets/file-system/src/lib.rs Outdated Show resolved Hide resolved
pallets/file-system/src/lib.rs Outdated Show resolved Hide resolved
pallets/file-system/src/lib.rs Outdated Show resolved Hide resolved
pallets/file-system/src/utils.rs Show resolved Hide resolved
pallets/file-system/src/utils.rs Show resolved Hide resolved
pallets/proofs-dealer/src/utils.rs Show resolved Hide resolved
pallets/proofs-dealer/src/benchmark_proofs.rs Show resolved Hide resolved
pallets/proofs-dealer/Cargo.toml Outdated Show resolved Hide resolved
pallets/proofs-dealer/Cargo.toml Outdated Show resolved Hide resolved
@TDemeco TDemeco requested a review from ffarall December 27, 2024 13:25
test/util/bspNet/waits.ts Show resolved Hide resolved
@ffarall ffarall merged commit 20c947e into main Dec 27, 2024
24 checks passed
@ffarall ffarall deleted the fix/update-payment-stream-stop-storing branch December 27, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants