changes as per review

cmd/lotus-storage-miner/allinfo_test.go

@@ -61,7 +61,14 @@ func TestMinerAllInfo(t *testing.T) {
 	t.Run("pre-info-all", run)
 
 	dh := kit.NewDealHarness(t, client, miner)
-	dh.MakeFullDeal(context.Background(), 6, false, false, 0)
+	_, _, _ = dh.MakeFullDeal(kit.MakeFullDealParams{
+		Ctx:         context.Background(),
+		Rseed:       6,
+		CarExport:   false,
+		FastRet:     false,
+		StartEpoch:  0,
+		DoRetrieval: true,
+	})
 
 	t.Run("post-info-all", run)
 }

itests/ccupgrade_test.go

@@ -94,7 +94,14 @@ func runTestCCUpgrade(t *testing.T, b kit.APIBuilder, blocktime time.Duration, u
 
 	dh := kit.NewDealHarness(t, client, miner)
 
-	dh.MakeFullDeal(context.Background(), 6, false, false, 0)
+	_, _, _ = dh.MakeFullDeal(kit.MakeFullDealParams{
+		Ctx:         context.Background(),
+		Rseed:       6,
+		CarExport:   false,
+		FastRet:     false,
+		StartEpoch:  0,
+		DoRetrieval: true,
+	})
 
 	// Validate upgrade
 

itests/deals_test.go

@@ -106,10 +106,24 @@ func runQuotePriceForUnsealedRetrieval(t *testing.T, b kit.APIBuilder, blocktime
 
 	dh := kit.NewDealHarness(t, client, miner)
 
-	_, info, fcid := dh.MakeFullDealNoRetrieval(ctx, 6, false, startEpoch)
+	_, info, fcid := dh.MakeFullDeal(kit.MakeFullDealParams{
+		Ctx:         ctx,
+		Rseed:       6,
+		CarExport:   false,
+		FastRet:     false,
+		StartEpoch:  startEpoch,
+		DoRetrieval: false,
+	})
 
 	// one more storage deal for the same data
-	_, _, fcid2 := dh.MakeFullDealNoRetrieval(ctx, 6, false, startEpoch)
+	_, _, fcid2 := dh.MakeFullDeal(kit.MakeFullDealParams{
+		Ctx:         ctx,
+		Rseed:       6,
+		CarExport:   false,
+		FastRet:     false,
+		StartEpoch:  startEpoch,
+		DoRetrieval: false,
+	})
 	require.Equal(t, fcid, fcid2)
 
 	// fetch quote -> zero for unsealed price since unsealed file already exists.
@@ -495,7 +509,14 @@ func runFullDealCycles(t *testing.T, n int, b kit.APIBuilder, blocktime time.Dur
 
 	baseseed := 6
 	for i := 0; i < n; i++ {
-		dh.MakeFullDeal(context.Background(), baseseed+i, carExport, fastRet, startEpoch)
+		_, _, _ = dh.MakeFullDeal(kit.MakeFullDealParams{
+			Ctx:         context.Background(),
+			Rseed:       baseseed + i,
+			CarExport:   carExport,
+			FastRet:     fastRet,
+			StartEpoch:  startEpoch,
+			DoRetrieval: true,
+		})
 	}
 }
 
@@ -601,5 +622,12 @@ func runZeroPricePerByteRetrievalDealFlow(t *testing.T, b kit.APIBuilder, blockt
 	err = miner.MarketSetRetrievalAsk(ctx, ask)
 	require.NoError(t, err)
 
-	dh.MakeFullDeal(ctx, 6, false, false, startEpoch)
+	_, _, _ = dh.MakeFullDeal(kit.MakeFullDealParams{
+		Ctx:         ctx,
+		Rseed:       6,
+		CarExport:   false,
+		FastRet:     false,
+		StartEpoch:  startEpoch,
+		DoRetrieval: true,
+	})
 }

itests/gateway_test.go

@@ -206,7 +206,14 @@ func TestGatewayDealFlow(t *testing.T) {
 	dealStartEpoch := abi.ChainEpoch(2 << 12)
 
 	dh := kit.NewDealHarness(t, nodes.lite, nodes.miner)
-	dh.MakeFullDeal(ctx, 6, false, false, dealStartEpoch)
+	dh.MakeFullDeal(kit.MakeFullDealParams{
+		Ctx:         ctx,
+		Rseed:       6,
+		CarExport:   false,
+		FastRet:     false,
+		StartEpoch:  dealStartEpoch,
+		DoRetrieval: true,
+	})
 }
 
 func TestGatewayCLIDealFlow(t *testing.T) {

itests/kit/deals.go

@@ -34,6 +34,15 @@ type DealHarness struct {
 	miner  TestMiner
 }
 
+type MakeFullDealParams struct {
+	Ctx         context.Context
+	Rseed       int
+	CarExport   bool
+	FastRet     bool
+	StartEpoch  abi.ChainEpoch
+	DoRetrieval bool
+}
+
 // NewDealHarness creates a test harness that contains testing utilities for deals.
 func NewDealHarness(t *testing.T, client api.FullNode, miner TestMiner) *DealHarness {
 	return &DealHarness{
@@ -43,9 +52,9 @@ func NewDealHarness(t *testing.T, client api.FullNode, miner TestMiner) *DealHar
 	}
 }
 
-func (dh *DealHarness) MakeFullDealNoRetrieval(ctx context.Context, rseed int, fastRet bool, startEpoch abi.ChainEpoch) ([]byte,
+func (dh *DealHarness) MakeFullDeal(params MakeFullDealParams) ([]byte,
 	*api.DealInfo, cid.Cid) {
-	res, _, data, err := CreateImportFile(ctx, dh.client, rseed, 0)
+	res, _, data, err := CreateImportFile(params.Ctx, dh.client, params.Rseed, 0)
 	if err != nil {
 		dh.t.Fatal(err)
 	}
@@ -53,41 +62,23 @@ func (dh *DealHarness) MakeFullDealNoRetrieval(ctx context.Context, rseed int, f
 	fcid := res.Root
 	fmt.Println("FILE CID: ", fcid)
 
-	deal := dh.StartDeal(ctx, fcid, fastRet, startEpoch)
+	deal := dh.StartDeal(params.Ctx, fcid, params.FastRet, params.StartEpoch)
 
 	// TODO: this sleep is only necessary because deals don't immediately get logged in the dealstore, we should fix this
 	time.Sleep(time.Second)
-	dh.WaitDealSealed(ctx, deal, false, false, nil)
+	dh.WaitDealSealed(params.Ctx, deal, false, false, nil)
 
 	// Retrieval
-	info, err := dh.client.ClientGetDealInfo(ctx, *deal)
+	info, err := dh.client.ClientGetDealInfo(params.Ctx, *deal)
 	require.NoError(dh.t, err)
 
+	if params.DoRetrieval {
+		dh.TestRetrieval(params.Ctx, fcid, &info.PieceCID, params.CarExport, data)
+	}
+
 	return data, info, fcid
 }
 
-func (dh *DealHarness) MakeFullDeal(ctx context.Context, rseed int, carExport, fastRet bool, startEpoch abi.ChainEpoch) {
-	res, _, data, err := CreateImportFile(ctx, dh.client, rseed, 0)
-	if err != nil {
-		dh.t.Fatal(err)
-	}
-
-	fcid := res.Root
-	fmt.Println("FILE CID: ", fcid)
-
-	deal := dh.StartDeal(ctx, fcid, fastRet, startEpoch)
-
-	// TODO: this sleep is only necessary because deals don't immediately get logged in the dealstore, we should fix this
-	time.Sleep(time.Second)
-	dh.WaitDealSealed(ctx, deal, false, false, nil)
-
-	// Retrieval
-	info, err := dh.client.ClientGetDealInfo(ctx, *deal)
-	require.NoError(dh.t, err)
-
-	dh.TestRetrieval(ctx, fcid, &info.PieceCID, carExport, data)
-}
-
 func (dh *DealHarness) StartDeal(ctx context.Context, fcid cid.Cid, fastRet bool, startEpoch abi.ChainEpoch) *cid.Cid {
 	maddr, err := dh.miner.ActorAddress(ctx)
 	if err != nil {

markets/retrievaladapter/provider.go

@@ -123,15 +123,9 @@ func (rpn *retrievalProviderNode) IsUnsealed(ctx context.Context, sectorID abi.S
 	return rpn.pp.IsUnsealed(ctx, ref, storiface.UnpaddedByteIndex(offset), length)
 }
 
-// `storageDeals` param here is the list of storage deals made for the `payloadCID` the retrieval client is looking for.
+// GetRetrievalPricingInput takes a set of candidate storage deals that can serve a retrieval request,
-//
+// and returns an minimally populated PricingInput. This PricingInput should be enhanced
-// `pieceCID` is the CID of the specific Piece we want to retrieve the payload from. The client can either mandate that
+// with more data, and passed to the pricing function to determine the final quoted price.
-// we retrieve the payload from a specific piece or we choose a Piece to retrieve the payload from, prioritizing
-// a Piece for which an unsealed sector file already exists if possible.
-//
-// 1. For the `VerifiedDeal` flag in the response `PricingInput`, we are looking to answer the question "does there exist any verified storage deal for this `payloadCID`" ?
-//
-// 2. We also want to ensure that we return the `PieceSize` for the actual piece we want to retrieve the deal from.
 func (rpn *retrievalProviderNode) GetRetrievalPricingInput(ctx context.Context, pieceCID cid.Cid, storageDeals []abi.DealID) (retrievalmarket.PricingInput, error) {
 	resp := retrievalmarket.PricingInput{}
 
@@ -154,13 +148,17 @@ func (rpn *retrievalProviderNode) GetRetrievalPricingInput(ctx context.Context,
 			resp.PieceSize = ds.Proposal.PieceSize.Unpadded()
 		}
 
+		// If we've discovered a verified deal with the required PieceCID, we don't need
+		// to lookup more deals and we're done.
 		if resp.VerifiedDeal && resp.PieceSize != 0 {
 			break
 		}
 	}
 
+	// Note: The piece size can never actually be zero. We only use it to here
+	// to assert that we didn't find a matching piece.
 	if resp.PieceSize == 0 {
-		return resp, xerrors.New("failed to find matching piece, PieceSize is zero")
+		return resp, xerrors.New("failed to find matching piece")
 	}
 
 	return resp, nil