Properly Implement Retrieval Lookups Based on CIDs #57

hannahhoward · 2020-01-24T01:46:44Z

Goals

Allow Retrieval deals to work with actual Payload CIDs

Implementation

-- add a second mapping to the PieceStore that translates from a CID to pieces its a part of
-- support unsealling pieces from CIDs as needed (theoretically a dag could span multiple pieces in the future where pieces are not whole dags -- not super important though)
-- add very preliminary writes of CID -> Piece mapping on storage provider side
--- IMPORTANT: Not included is mapping any CIDs in the IPLD graph other than the root to Piece, or recording where in the piece blocks actually live. This can definitely be done by recording this information as we write the CAR file. This is definitely temporary implementation meant to get us to working retrieval by payload CID fast, with allowing retrievals based off of CIDs deeper in the dag later.

seperate block infos to their own table -- mapping links with pieceCIDs

modify markets to use actual payload CIDs instead of treating payload and piece CIDs as equavalent

very simple implementation of storing a map between payload root and pieceCID. to be augmented later with actual block refs

shannonwells

primarily editorial comments and peanut-gallery stuff

shannonwells · 2020-01-24T18:04:04Z

piecestore/piecestore.go

-	if err := ps.store.Get(newKey(pieceCID)).Get(&out); err != nil {
-		return PieceInfo{}, err
+func (ps *pieceStore) ensurePieceInfo(pieceCID []byte) error {
+	has, err := ps.pieces.Has(newKey(pieceCID))


Maybe call it canHasPieceInfo /jk

shannonwells · 2020-01-24T18:13:03Z

piecestore/piecestore_test.go

+		err := ps.AddDealForPiece(pieceCid, dealInfo)
+		assert.NoError(t, err)
+
+		pi, err := ps.GetPieceInfo(pieceCid)


nit: I should think you could skip the error/value checking on the first try since it's tested above.

I just follow past of least resistance to get no lint-stage errors

shannonwells · 2020-01-24T18:37:02Z

retrievalmarket/impl/blockunsealing/blockunsealing.go

@@ -92,3 +87,22 @@ func (lu *loaderWithUnsealing) attemptUnseal() error {

 	return nil
 }
+
+func (lu *loaderWithUnsealing) firstSuccessfulUnseal(cidInfo piecestore.CIDInfo) (io.ReadCloser, error) {


I'm a little fuzzy on this: If it's already unsealed do you expect the unsealer func not to return an error?

so the logic changed a bit here. theoretically since we're basing our queries on payload CIDs, I guess it's possible that a query could span multiple pieces now (if a piece was a partial but not complete DAG -- something we intend to support I believe).

The point of this function is: for a given Cid, find the first piece that it's contained in that we can successfully unseal.

since we unseal and read the whole piece, in theory this function should not get called unless the query we make somehow crosses a piece boundary, which again, I think is possible.

If unsealing fails completely the first time, it's probably alright cause the loader calling code will immediately error and stop the retrieval deal.

shannonwells · 2020-01-24T18:42:46Z

retrievalmarket/impl/integration_test.go

@@ -82,12 +82,22 @@ func requireSetupTestClientAndProvider(bgCtx context.Context, t *testing.T, payC
 	providerNode := testnodes.NewTestRetrievalProviderNode()
 	pieceStore := tut.NewTestPieceStore()
 	expectedCIDs := tut.GenerateCids(3)
-	missingPiece := tut.GenerateCids(1)[0]
+	expectedPieces := [][]byte{[]byte("applesuace"), []byte("jam"), []byte("apricot")}


I don't know how you resisted using "pork chops" after "applesauce"

shannonwells · 2020-01-24T19:39:51Z

storagemarket/impl/provider_states.go

 	sectorID, offset, length, err := p.spn.LocatePieceForDealWithinSector(ctx, deal.DealID)
 	if err != nil {
 		return nil, err
 	}
+	// TODO: Record actual block locations for all CIDs in piece by improving car writing


SHould this be captured in an issue?

yes, it will be soon, that's the intent.

codecov-io · 2020-01-24T22:19:52Z

Codecov Report

Merging #57 into master will increase coverage by 0.51%.
The diff coverage is 57.58%.

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   34.12%   34.62%   +0.51%     
==========================================
  Files          36       36              
  Lines        2615     2713      +98     
==========================================
+ Hits          892      939      +47     
- Misses       1588     1617      +29     
- Partials      135      157      +22

Impacted Files	Coverage Δ
storagemarket/impl/provider_states.go	`0% <0%> (ø)`	⬆️
...ievalmarket/impl/providerstates/provider_states.go	`94.32% <100%> (ø)`	⬆️
...rievalmarket/impl/blockunsealing/blockunsealing.go	`85.37% <100%> (-2.13%)`	⬇️
piecestore/types_cbor_gen.go	`28.58% <27.96%> (-0.87%)`	⬇️
piecestore/piecestore.go	`83.88% <83.68%> (-2.49%)`	⬇️
retrievalmarket/impl/provider.go	`75.19% <96.16%> (+2.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aa65e2...582fa86. Read the comment docs.

ingar

Looks good so far.

ingar · 2020-01-24T19:44:33Z

piecestore/piecestore.go

 	}
 }

 type pieceStore struct {
-	store *statestore.StateStore
+	pieces   *statestore.StateStore
+	cidInfos *statestore.StateStore
 }

 func (ps *pieceStore) AddDealForPiece(pieceCID []byte, dealInfo DealInfo) error {
 	// Do we need to de-dupe or anything here?


Guess we can remove this comment now.

ingar · 2020-01-24T22:06:57Z

storagemarket/impl/provider_states.go

 	sectorID, offset, length, err := p.spn.LocatePieceForDealWithinSector(ctx, deal.DealID)
 	if err != nil {
 		return nil, err
 	}
+	// TODO: Record actual block locations for all CIDs in piece by improving car writing
+	err = p.pieceStore.AddPieceBlockLocations(deal.Proposal.PieceRef, map[cid.Cid]piecestore.BlockLocation{
+		deal.Ref: {},


What is needed to store the root CID location here?

even for the root, we need to figure out where it sits within the CAR file, so it will be addressed at the same time we address the todo above

ingar · 2020-01-24T22:22:49Z

The idea is we can store an index to arbitrary blocks in the IPLD graph of a deal payload, because a CAR file is just blocks that are concatenated together?

hannahhoward · 2020-01-25T00:58:09Z

@ingar -- basically yes -- when we write the car, we can basically record for any CID:

the piece its in
- the relative offset within the piece
- the size of the block
  And then when we combine that with pieces mapping, we can effectively get, for any CID:
a sector it's in
an offset within the sector
a length of bytes within the sector.

And theoretically we might start partial unsealling even pieces based on that, except that requires unsealing to be super fast which... I'm not sure where things are at with that.

hannahhoward requested review from shannonwells and ingar and removed request for shannonwells January 24, 2020 01:48

hannahhoward added 5 commits January 24, 2020 09:47

feat(piecestore): maintain CID map

39a4f78

seperate block infos to their own table -- mapping links with pieceCIDs

feat(retrievalmarket): translate payload CIDs

4ca08b5

modify markets to use actual payload CIDs instead of treating payload and piece CIDs as equavalent

feat(storagemarket): save cid info for piece

b0d3c0c

very simple implementation of storing a map between payload root and pieceCID. to be augmented later with actual block refs

fix(deps): update to rebased go-car

9a31d7a

fix(deps): go mod tidy

9211de0

hannahhoward force-pushed the feat/implement-payload-CIDs branch from 03eb634 to 9211de0 Compare January 24, 2020 17:48

shannonwells approved these changes Jan 24, 2020

View reviewed changes

fix(discovery): fix key encoding

7b74974

ingar approved these changes Jan 24, 2020

View reviewed changes

fix(retrievalmarket): set default provider params

d94a1cf

style(piecestore): remove outdated comment

582fa86

hannahhoward merged commit 2249baf into master Jan 25, 2020

mishmosh mentioned this pull request Jan 27, 2020

Retrieval by Payload CID vs PieceCID filecoin-project/specs#853

Closed

hannahhoward deleted the feat/implement-payload-CIDs branch April 30, 2020 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly Implement Retrieval Lookups Based on CIDs #57

Properly Implement Retrieval Lookups Based on CIDs #57

hannahhoward commented Jan 24, 2020

shannonwells left a comment

shannonwells Jan 24, 2020

shannonwells Jan 24, 2020

hannahhoward Jan 25, 2020

shannonwells Jan 24, 2020

hannahhoward Jan 25, 2020

shannonwells Jan 24, 2020

shannonwells Jan 24, 2020

hannahhoward Jan 25, 2020

codecov-io commented Jan 24, 2020 •

edited

ingar left a comment

ingar Jan 24, 2020

hannahhoward Jan 25, 2020

ingar Jan 24, 2020

hannahhoward Jan 25, 2020

ingar commented Jan 24, 2020

hannahhoward commented Jan 25, 2020

Properly Implement Retrieval Lookups Based on CIDs #57

Properly Implement Retrieval Lookups Based on CIDs #57

Conversation

hannahhoward commented Jan 24, 2020

Goals

Implementation

shannonwells left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jan 24, 2020 • edited

Codecov Report

ingar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingar commented Jan 24, 2020

hannahhoward commented Jan 25, 2020

codecov-io commented Jan 24, 2020 •

edited