test: validate retrieve when root payload CID is an identity CID (#715)

* test(itests): demonstrate failure to retrieve

* fix: cleanup test & handle identity roots

* chore: update go-fil-markets with identity CID fix

Ref: filecoin-project/go-fil-markets#747

* doc: make it clear InlineBuilder isn't recommended
i.e. don't copypasta this for your filecoin deal prep

Co-authored-by: Rod Vagg <rod@vagg.org>

car/car_offset_writer_test.go

@@ -244,6 +244,7 @@ func DAGImport(dserv format.DAGService, fi io.Reader) (format.Node, error) {
 		Maxlinks:  1024,
 		RawLeaves: true,
 
+		// NOTE: InlineBuilder not recommended, we are using this to test identity CIDs
 		CidBuilder: cidutil.InlineBuilder{
 			Builder: prefix,
 			Limit:   32,

itests/dummydeal_offline_test.go

@@ -34,7 +34,8 @@ func TestDummydealOffline(t *testing.T) {
 
 	// make an offline deal
 	offlineDealUuid := uuid.New()
-	res, err := f.MakeDummyDeal(offlineDealUuid, carFilepath, rootCid, "", true)
+	dealRes, err := f.MakeDummyDeal(offlineDealUuid, carFilepath, rootCid, "", true)
+	res := dealRes.Result
 	require.NoError(t, err)
 	require.True(t, res.Accepted)
 	res, err = f.Boost.BoostOfflineDealWithData(context.Background(), offlineDealUuid, carFilepath)

itests/dummydeal_test.go

@@ -39,6 +39,9 @@ func TestDummydealOnline(t *testing.T) {
 	failingFilepath, err := testutil.CreateRandomFile(tempdir, 5, 2000000)
 	require.NoError(t, err)
 
+	// NOTE: these calls to CreateDenseCARv2 have the identity CID builder enabled so will
+	// produce a root identity CID for this case. So we're testing deal-making and retrieval
+	// where a DAG has an identity CID root
 	rootCid, carFilepath, err := testutil.CreateDenseCARv2(tempdir, randomFilepath)
 	require.NoError(t, err)
 
@@ -56,7 +59,7 @@ func TestDummydealOnline(t *testing.T) {
 	// Make a deal
 	res, err := f.MakeDummyDeal(dealUuid, carFilepath, rootCid, server.URL+"/"+filepath.Base(carFilepath), false)
 	require.NoError(t, err)
-	require.True(t, res.Accepted)
+	require.True(t, res.Result.Accepted)
 	log.Debugw("got response from MarketDummyDeal", "res", spew.Sdump(res))
 
 	time.Sleep(2 * time.Second)
@@ -66,28 +69,26 @@ func TestDummydealOnline(t *testing.T) {
 	failingDealUuid := uuid.New()
 	res2, err2 := f.MakeDummyDeal(failingDealUuid, failingCarFilepath, failingRootCid, server.URL+"/"+filepath.Base(failingCarFilepath), false)
 	require.NoError(t, err2)
-	require.Contains(t, res2.Reason, "no space left", res2.Reason)
+	require.Contains(t, res2.Result.Reason, "no space left", res2.Result.Reason)
 	log.Debugw("got response from MarketDummyDeal for failing deal", "res2", spew.Sdump(res2))
 
 	// Wait for the first deal to be added to a sector and cleaned up so space is made
 	err = f.WaitForDealAddedToSector(dealUuid)
 	require.NoError(t, err)
 	time.Sleep(100 * time.Millisecond)
 
-	// *********************************************************************
-	// Disable this part of the test for now because there is a bug in lotus
-	// introduced in this PR that causes the test to fail:
-	// https://github.com/filecoin-project/lotus/pull/9174/files#top
-	// *********************************************************************
-
-	//// Make a third deal - it should succeed because the first deal has been cleaned up
-	//passingDealUuid := uuid.New()
-	//res2, err2 = f.MakeDummyDeal(passingDealUuid, failingCarFilepath, failingRootCid, server.URL+"/"+filepath.Base(failingCarFilepath), false)
-	//require.NoError(t, err2)
-	//require.True(t, res2.Accepted)
-	//log.Debugw("got response from MarketDummyDeal", "res2", spew.Sdump(res2))
-	//
-	//// Wait for the deal to be added to a sector
-	//err = f.WaitForDealAddedToSector(passingDealUuid)
-	//require.NoError(t, err)
+	// Make a third deal - it should succeed because the first deal has been cleaned up
+	passingDealUuid := uuid.New()
+	res2, err2 = f.MakeDummyDeal(passingDealUuid, failingCarFilepath, failingRootCid, server.URL+"/"+filepath.Base(failingCarFilepath), false)
+	require.NoError(t, err2)
+	require.True(t, res2.Result.Accepted)
+	log.Debugw("got response from MarketDummyDeal", "res2", spew.Sdump(res2))
+
+	// Wait for the deal to be added to a sector
+	err = f.WaitForDealAddedToSector(passingDealUuid)
+	require.NoError(t, err)
+
+	// rootCid is an identity CID
+	outFile := f.RetrieveDirect(ctx, t, rootCid, &res.DealParams.ClientDealProposal.Proposal.PieceCID, true)
+	kit.AssertFilesEqual(t, randomFilepath, outFile)
 }

itests/framework/framework.go

@@ -48,6 +48,7 @@ import (
 	"github.com/filecoin-project/lotus/storage/paths"
 	"github.com/filecoin-project/lotus/storage/pipeline/sealiface"
 	"github.com/google/uuid"
+	blocks "github.com/ipfs/go-block-format"
 	"github.com/ipfs/go-cid"
 	"github.com/ipfs/go-datastore"
 	files "github.com/ipfs/go-ipfs-files"
@@ -59,6 +60,7 @@ import (
 	"github.com/ipld/go-car"
 	"github.com/libp2p/go-libp2p"
 	"github.com/libp2p/go-libp2p/core/host"
+	"github.com/multiformats/go-multihash"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sync/errgroup"
 )
@@ -477,7 +479,12 @@ func (f *TestFramework) WaitForDealAddedToSector(dealUuid uuid.UUID) error {
 	}
 }
 
-func (f *TestFramework) MakeDummyDeal(dealUuid uuid.UUID, carFilepath string, rootCid cid.Cid, url string, isOffline bool) (*api.ProviderDealRejectionInfo, error) {
+type DealResult struct {
+	DealParams types.DealParams
+	Result     *api.ProviderDealRejectionInfo
+}
+
+func (f *TestFramework) MakeDummyDeal(dealUuid uuid.UUID, carFilepath string, rootCid cid.Cid, url string, isOffline bool) (*DealResult, error) {
 	cidAndSize, err := storagemarket.GenerateCommP(carFilepath)
 	if err != nil {
 		return nil, err
@@ -545,7 +552,11 @@ func (f *TestFramework) MakeDummyDeal(dealUuid uuid.UUID, carFilepath string, ro
 		SkipIPNIAnnounce:   false,
 	}
 
-	return f.Client.StorageDeal(f.ctx, dealParams, peerID)
+	res, err := f.Client.StorageDeal(f.ctx, dealParams, peerID)
+	return &DealResult{
+		DealParams: dealParams,
+		Result:     res,
+	}, err
 }
 
 func (f *TestFramework) signProposal(addr address.Address, proposal *market.DealProposal) (*market.ClientDealProposal, error) {
@@ -669,6 +680,47 @@ func (f *TestFramework) Retrieve(ctx context.Context, t *testing.T, deal *cid.Ci
 	require.NoError(t, err)
 	require.NotEmpty(t, offers, "no offers")
 
+	return f.retrieve(ctx, t, offers[0], carExport)
+}
+
+func (f *TestFramework) ExtractFileFromCAR(ctx context.Context, t *testing.T, file *os.File) string {
+	bserv := dstest.Bserv()
+	ch, err := car.LoadCar(ctx, bserv.Blockstore(), file)
+	require.NoError(t, err)
+
+	var b blocks.Block
+	if ch.Roots[0].Prefix().MhType == multihash.IDENTITY {
+		mh, err := multihash.Decode(ch.Roots[0].Hash())
+		require.NoError(t, err)
+		b, err = blocks.NewBlockWithCid(mh.Digest, ch.Roots[0])
+		require.NoError(t, err)
+	} else {
+		b, err = bserv.GetBlock(ctx, ch.Roots[0])
+		require.NoError(t, err)
+	}
+
+	nd, err := ipld.Decode(b)
+	require.NoError(t, err)
+
+	dserv := dag.NewDAGService(bserv)
+	fil, err := unixfile.NewUnixfsFile(ctx, dserv, nd)
+	require.NoError(t, err)
+
+	tmpFile := path.Join(t.TempDir(), fmt.Sprintf("file-in-car-%d", rand.Uint32()))
+	err = files.WriteTo(fil, tmpFile)
+	require.NoError(t, err)
+
+	return tmpFile
+}
+
+func (f *TestFramework) RetrieveDirect(ctx context.Context, t *testing.T, root cid.Cid, pieceCid *cid.Cid, carExport bool) string {
+	offer, err := f.FullNode.ClientMinerQueryOffer(ctx, f.MinerAddr, root, pieceCid)
+	require.NoError(t, err)
+
+	return f.retrieve(ctx, t, offer, carExport)
+}
+
+func (f *TestFramework) retrieve(ctx context.Context, t *testing.T, offer lapi.QueryOffer, carExport bool) string {
 	p := path.Join(t.TempDir(), "ret-car-"+t.Name())
 	carFile, err := os.Create(p)
 	require.NoError(t, err)
@@ -682,7 +734,7 @@ func (f *TestFramework) Retrieve(ctx context.Context, t *testing.T, deal *cid.Ci
 	updates, err := f.FullNode.ClientGetRetrievalUpdates(updatesCtx)
 	require.NoError(t, err)
 
-	retrievalRes, err := f.FullNode.ClientRetrieve(ctx, offers[0].Order(caddr))
+	retrievalRes, err := f.FullNode.ClientRetrieve(ctx, offer.Order(caddr))
 	require.NoError(t, err)
 consumeEvents:
 	for {
@@ -710,7 +762,7 @@ consumeEvents:
 
 	require.NoError(t, f.FullNode.ClientExport(ctx,
 		lapi.ExportRef{
-			Root:   root,
+			Root:   offer.Root,
 			DealID: retrievalRes.DealID,
 		},
 		lapi.FileRef{
@@ -725,25 +777,3 @@ consumeEvents:
 
 	return ret
 }
-
-func (f *TestFramework) ExtractFileFromCAR(ctx context.Context, t *testing.T, file *os.File) string {
-	bserv := dstest.Bserv()
-	ch, err := car.LoadCar(ctx, bserv.Blockstore(), file)
-	require.NoError(t, err)
-
-	b, err := bserv.GetBlock(ctx, ch.Roots[0])
-	require.NoError(t, err)
-
-	nd, err := ipld.Decode(b)
-	require.NoError(t, err)
-
-	dserv := dag.NewDAGService(bserv)
-	fil, err := unixfile.NewUnixfsFile(ctx, dserv, nd)
-	require.NoError(t, err)
-
-	tmpFile := path.Join(t.TempDir(), fmt.Sprintf("file-in-car-%d", rand.Uint32()))
-	err = files.WriteTo(fil, tmpFile)
-	require.NoError(t, err)
-
-	return tmpFile
-}

testutil/car.go

@@ -139,6 +139,7 @@ func WriteUnixfsDAGTo(path string, into ipldformat.DAGService) (cid.Cid, error)
 	params := ihelper.DagBuilderParams{
 		Maxlinks:  unixfsLinksPerLevel,
 		RawLeaves: true,
+		// NOTE: InlineBuilder not recommended, we are using this to test identity CIDs
 		CidBuilder: cidutil.InlineBuilder{
 			Builder: prefix,
 			Limit:   126,

transport/httptransport/http_transport_test.go

@@ -509,6 +509,7 @@ func dagImport(dserv format.DAGService, fi io.Reader) (format.Node, error) {
 		Maxlinks:  1024,
 		RawLeaves: true,
 
+		// NOTE: InlineBuilder not recommended, we are using this to test identity CIDs
 		CidBuilder: cidutil.InlineBuilder{
 			Builder: prefix,
 			Limit:   32,