Skip to content

chore(sales): log error when datasets size is different then expected #1155

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AuHau
Copy link
Member

@AuHau AuHau commented Mar 11, 2025

This is a safeguard for when the downloaded Slot's size does not match what was expected. I want to add this, because back when I was working on IPFS I discovered a security bug where you could forge the manifest in a way that it would report a different size than the actual data's size. You could have then stored an "infinite" amount of data with the pinning services because they relied on the manifest info and did not double-check actual data size.

Originally part of #798 but that PR got reworked by Arnoud at #1099 so just pulling this part out of it.

@AuHau AuHau requested review from emizzle and markspanbroek March 11, 2025 16:06
@AuHau AuHau added the Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details label Mar 11, 2025
@AuHau AuHau force-pushed the chore/download-size-mismatch branch from 08a7bb4 to 125cf9b Compare March 11, 2025 16:35
Copy link
Contributor

@emizzle emizzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the data is smaller than StorageRequest.ask.slotSize then unused capacity in a Reservation gets returned to the Availability on clean up. If the data is greater than the Reservation capacity, we should get a BytesOutOfBoundsError exception in the result. So from an accounting perspective, I believe we're already ok. However, from a data integrity perspective, wouldn't it be better to recreate the Cid from the dataset and see if it matches? Slower, yes, but more accurate.

Also, maybe node.onStore might be a better place to implement that.

@dryajov
Copy link
Contributor

dryajov commented Apr 16, 2025

However, from a data integrity perspective, wouldn't it be better to recreate the Cid from the dataset and see if it matches? Slower, yes, but more accurate.

This is already the case, we always check the integrity of the downloaded data.

error "Couldn't get reservation key", id = $reservation.id, error = error.msg

if updatedReservation =? await reservations.get(keyId, Reservation):
if updatedReservation.size != 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not fail the contract tho, if this is whats going on. We should either return that extra capacity back, or just ignore it, since the user is allegedly already paying for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants