Remove carv1/carv2 specific semantics from commP calculations

[willscott]

The commp should be directly over the bytes of the deal, it shouldn't care about car format.

[nonsense]

We should also fix Lotus to calculate commp in this form, at the moment AddPiece is failing because of this change at:

		ppi, err := m.sealer.AddPiece(sealer.WithPriority(ctx.Context(), DealSectorPriority),
			m.minerSector(sector.SectorType, sector.SectorNumber),
			pieceSizes,
			deal.size,
			deal.data)
		if !ppi.PieceCID.Equals(deal.deal.DealProposal.PieceCID) {
			err = xerrors.Errorf("got unexpected piece CID: expected:%s, got:%s", deal.deal.DealProposal.PieceCID, ppi.PieceCID)
			deal.accepted(sector.SectorNumber, offset, err)
			return ctx.Send(SectorAddPieceFailed{err})
		}

which refers to:

storage/sealer/ffiwrapper/sealer_cgo.go

func (sb *Sealer) pieceCid(spt abi.RegisteredSealProof, in []byte) (cid.Cid, error) {
	prf, werr, err := commpffi.ToReadableFile(bytes.NewReader(in), int64(len(in)))
	if err != nil {
		return cid.Undef, xerrors.Errorf("getting tee reader pipe: %w", err)
	}

	pieceCID, err := ffi.GeneratePieceCIDFromFile(spt, prf, abi.UnpaddedPieceSize(len(in)))
	if err != nil {
		return cid.Undef, xerrors.Errorf("generating piece commitment: %w", err)
	}

	_ = prf.Close()

	return pieceCID, werr()
}

[willscott]

I don't see anything in the commp utility / ffi calculations of commp that do more than a direct hash summing over the bytes of the piece/deal/file. Are you pointing to somewhere else where a carv2 would get unwrapped to only commp over the wrapped carv1?

[nonsense]

I just reviewed CI, and if you search for unexpected you come up with this error, which makes me think that we don't conform to the commp calculation that Lotus does (and the current codebase does so), which makes me think that Lotus also does more than direct hashing over the file.

[willscott]

I believe the remaining rough edge here is in piecedirectory.GetBlockstore, which tries to use the "absolute" index that boost generates with the carv2 blockstore interface - waiting on ipld/go-car#490 to be able to support that.

[willscott]

flagged by @rvagg that we should make sure there's a test over a carv2 deal to exercise that branch of graphsync GetBlockstore

[rvagg]

we should make sure there's a test over a carv2 deal to exercise that branch of graphsync GetBlockstore

Yep, it was broken. Tested and fixed here: #1662

[dirkmc]

@willscott @rvagg nice work! Thanks for diving deep into the code to figure this one out 🙌

Once we merge this PR, our restore functionality will also need to change, because at the moment it assumes CAR files. So we're going to hold off on merging until we've figured out how to modify the restore function.

[dirkmc]

Suggested change

	error: fmt.Errorf("failed to stat CARv1 file: %w", err),
	error: fmt.Errorf("failed to stat file: %w", err),

[dirkmc]

Let's add a unit test for this functionalty.

[dirkmc]

Let's also add an itest with a file that is not a car file to make sure that flow works as expected

[LexLuthr]

We need to account for this change in our LID recovery tool. We should hold off on merging this PR till we complete the following tasks.

• Add a test for NewMultiReaderAt() - @willscott or @rvagg

[willscott]

is the podsi itest sufficient for the itest ask?

e.g. we'll merge this first into #1495

[rvagg]

also needs to be updated to go-car/v2@v2.13.1 which will break the calculations here due to the "fix"

[LexLuthr]

@willscott I think we can marge this in data-segment indexing once unti test to NewMultiReaderAt() is done. I am moving the other 2 tasks to respective PRs.
@rvagg Is the carv2 update blocker for this merge?

[rvagg]

@LexLuthr yes, please consider it a blocker. I fixed a bug and released 2.12 for this branch but then we realised later we changed the behaviour in a braking way so fixed that in 2.13.1, now the values returned from BlockMetadata#Offset are referencing the wrong thing. We need to sort that out here first.
Either @willscott or myself will do it in the next few days . I'll do it my Monday if it's not don't by then.

[rvagg]

@LexLuthr go-car/v2 update done

[rvagg]

and a test for NewMultiReaderAt while I'm at it

[LexLuthr]

I think we are good to merge now. We just need to ensure that we are merging piecedirectory/segmentindex not merging in main. I will leave the rebase strategy upto the authors.

[LexLuthr]

@willscott Are we good to close this? I already see a commit with merge from this branch on #1495

[willscott]

yes. i think we can consider this 'merged' - there are failing tests in #1495 that i was going to take a look at addressing / identifying as fall-out from this merge or unrelated