fix: store identity CIDs in CARs for online deals #749
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rvagg
force-pushed
the
rvagg/store-identity-cids
branch
from
5fb90f8
to
c0372f1
Compare
Closed
5 tasks
|
updated this to use the newly released go-car/v2@2.5.0 which has the necessary changes to handle identity CIDs like we want for this |
dirkmc
reviewed
Sep 13, 2022
retrievalmarket/impl/provider.go
Outdated
| // for each link, query the dagstore for pieces that contain it | ||
| for i, link := range links { | ||
| piecesWithThisCid, err := p.dagStore.GetPiecesContainingBlock(link) | ||
| if len(piecesWithThisCid) == 0 { |
There was a problem hiding this comment.
Is it possible for GetPiecesContainingBlock to return a zero-length piecesWithThisCid, and for err to also be nil?
There was a problem hiding this comment.
Good call, I think it's probably unsafe to not explicitly deal with err even though I believe zero-length should also ways be coupled with an error (comes out of GetShardsForMultihash in the inverted index which requires an entry for that CID and an ErrNotFound otherwise). Addressed in #750
rvagg
force-pushed
the
rvagg/store-identity-cids
branch
from
069f57e
to
a7c442f
Compare
|
This becomes considerably simpler with #747 merged, the big changes were in there. Rebased now so there's only two commits left. |
dirkmc
approved these changes
Sep 13, 2022
rvagg
force-pushed
the
rvagg/store-identity-cids
branch
2 times, most recently
from
49ff77d
to
f559821
Compare
matches historical precedent, without doing this we get CommP failures where deals are made with intermediate inline CIDs and the client stores them in the CAR used to make CommP. car.SelectiveCar is the historical way of doing this. e.g. https://github.com/filecoin-project/lotus/blob/a843c52e38da13da489cbe6b290ea49b2660b3fb/node/impl/client/client.go#L1405-L1412 Ref: ipld/go-car#332
rvagg
force-pushed
the
rvagg/store-identity-cids
branch
from
f559821
to
ee565ed
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on #747 and relies on ipld/go-car#332
This matches historical precedent, without doing this we get CommP failures where
deals are made with intermediate inline CIDs and the client stores them in
the CAR used to make CommP. car.SelectiveCar is the historical way of doing
this.
e.g. https://github.com/filecoin-project/lotus/blob/a843c52e38da13da489cbe6b290ea49b2660b3fb/node/impl/client/client.go#L1405-L1412
More discussion in ipld/go-car#332, and I'll also open a Lotus PR to bring this in and test it there and will link back here from that.