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

Consider QueryOffer Error & Status #2042

Merged
merged 2 commits into from
Jun 16, 2020
Merged

Consider QueryOffer Error & Status #2042

merged 2 commits into from
Jun 16, 2020

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Jun 16, 2020

Consider QueryOffer.Status before calling ClientRetrieve, and be aware of non-errored Retrieval.Query calls that have non-success .Status.

Continuation of #2040
cc @whyrusleeping

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
var errStr string
switch queryResponse.Status {
case retrievalmarket.QueryResponseAvailable:
errStr = ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Despite it's the default value, preferred to be explicit about the case.

@jsign jsign marked this pull request as ready for review June 16, 2020 20:39
@jsign jsign marked this pull request as draft June 16, 2020 20:46
@jsign jsign marked this pull request as ready for review June 16, 2020 20:47
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@whyrusleeping whyrusleeping merged commit c6a8fe1 into filecoin-project:master Jun 16, 2020
@mikeal
Copy link

mikeal commented Jun 23, 2020

This may be a bigger/broader issue than just this PR but it’s relevant so I’m going to bring it up.

The PayloadCID is not secure and whenever we use it without the PieceCID there are possible attacks.

The PayloadCID is the hash of a root block inside a CAR file.

It’s actually not difficult at all to take that CAR file, store only the root block, and do a new deal on that CAR file.

The only place we have assurances that the rest of the data for the graph is actually stored in the CAR file is in the default lotus dealflow (client and miner agree on CommP), so it’s possible to do deals for CAR files that don’t have all of the data using their own deal flow (the offline deal flow would allow this).

What happens here when a miner has two storage deals for the same PayloadCID?

@hannahhoward
Copy link
Contributor

@mikeal offline deals are specified with a PieceCID and the car file that is received is validated to match the PieceCID before it's accepted for the deal.

@hannahhoward
Copy link
Contributor

also: the reason retrieval uses payload CIDs is you're not paying for "the piece" you're paying for data -- and in fact, what you care about is that you can verify that data incrementally, because you pay for it incrementally. If the miner does not serve the whole payload (or the payload + requested selector) you don't pay for it.

You can however, also specify the pieceCID, so you're saying "I want this payload, served specifically from this piece".

However, in general, retrieval is a proof is in the pudding protocol -- the miner can say they have anything, and the intent is that retrieval providers may not be miners at all -- they may just be people holding unsealed data. What you pay for is what they send you. And the good thing about a payload is we can use graphsync & selectors to know everything we're getting is part of the payload as we get it.

@hannahhoward
Copy link
Contributor

But long and short, the required parameter is payload CID -- otherwise you have no way to know till the whole deal is transferred that you've gotten what you asked for.

@hannahhoward
Copy link
Contributor

Important Follow-up: I see now that we never exposed the parameter for PieceCID on query. I'm sorry about that. I will elevated that ticket.

@mikeal
Copy link

mikeal commented Jun 23, 2020

I may need to hold off on this until we can have a deeper call on how payment works in the retrieval market, but here’s a few questions I have (feel free to wait to answer them until we have time for a larger brain dump).

  • Since this transaction is trustless:
    • How does the client cancel payment if it only receives partial data?
    • How do we trust that the client isn’t lying in order to avoid payment?
  • If we don’t use PieceCID along with PayloadCID for retrievals can’t I craft a denial of service attack on a piece of content by creating new storage deals with that miner for CAR files of incomplete data w/ the targeted PayloadCID?

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.

4 participants