From 6e5862177bb505a38b72e0cb181c4eab8d2fc4bc Mon Sep 17 00:00:00 2001 From: hannahhoward Date: Fri, 19 Aug 2022 16:19:15 -0700 Subject: [PATCH 1/4] test(itests): demonstrate failure to retrieve --- itests/dummydeal_offline_test.go | 3 +- itests/dummydeal_test.go | 33 +++++++-------- itests/framework/framework.go | 72 ++++++++++++++++++++------------ 3 files changed, 63 insertions(+), 45 deletions(-) diff --git a/itests/dummydeal_offline_test.go b/itests/dummydeal_offline_test.go index 031ad16a7..ecb87070b 100644 --- a/itests/dummydeal_offline_test.go +++ b/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) diff --git a/itests/dummydeal_test.go b/itests/dummydeal_test.go index 5dd3c1b6c..8bdd0b6e6 100644 --- a/itests/dummydeal_test.go +++ b/itests/dummydeal_test.go @@ -56,7 +56,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,7 +66,7 @@ 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 @@ -74,20 +74,17 @@ func TestDummydealOnline(t *testing.T) { 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) + + outCar := f.RetrieveDirect(ctx, t, rootCid, &res.DealParams.ClientDealProposal.Proposal.PieceCID, true) + kit.AssertFilesEqual(t, carFilepath, outCar) } diff --git a/itests/framework/framework.go b/itests/framework/framework.go index f5c8ccfba..880e9f6eb 100644 --- a/itests/framework/framework.go +++ b/itests/framework/framework.go @@ -477,7 +477,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 @@ -543,7 +548,11 @@ func (f *TestFramework) MakeDummyDeal(dealUuid uuid.UUID, carFilepath string, ro }, } - 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) { @@ -667,6 +676,39 @@ 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) + + 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) @@ -680,7 +722,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 { @@ -708,7 +750,7 @@ consumeEvents: require.NoError(t, f.FullNode.ClientExport(ctx, lapi.ExportRef{ - Root: root, + Root: offer.Root, DealID: retrievalRes.DealID, }, lapi.FileRef{ @@ -723,25 +765,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 -} From 0deee0bf05850b940ae1cbe25228a46a965ea942 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Tue, 23 Aug 2022 17:36:43 +1000 Subject: [PATCH 2/4] fix: cleanup test & handle identity roots --- go.mod | 6 ++++++ itests/dummydeal_test.go | 8 ++++++-- itests/framework/framework.go | 18 ++++++++++++++++-- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 92bda19ca..a449c4137 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,12 @@ replace github.com/filecoin-project/go-jsonrpc => github.com/nonsense/go-jsonrpc // replace github.com/filecoin-project/lotus => ../lotus +// replace github.com/filecoin-project/go-fil-markets => ../go-fil-markets + +// replace github.com/filecoin-project/dagstore => ../dagstore + +// replace github.com/ipld/go-car/v2 => ../../ipld/go-car/v2 + require ( contrib.go.opencensus.io/exporter/prometheus v0.4.0 github.com/BurntSushi/toml v1.1.0 diff --git a/itests/dummydeal_test.go b/itests/dummydeal_test.go index 8bdd0b6e6..7bec559b9 100644 --- a/itests/dummydeal_test.go +++ b/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) @@ -85,6 +88,7 @@ func TestDummydealOnline(t *testing.T) { err = f.WaitForDealAddedToSector(passingDealUuid) require.NoError(t, err) - outCar := f.RetrieveDirect(ctx, t, rootCid, &res.DealParams.ClientDealProposal.Proposal.PieceCID, true) - kit.AssertFilesEqual(t, carFilepath, outCar) + // rootCid is an identity CID + outFile := f.RetrieveDirect(ctx, t, rootCid, &res.DealParams.ClientDealProposal.Proposal.PieceCID, true) + kit.AssertFilesEqual(t, randomFilepath, outFile) } diff --git a/itests/framework/framework.go b/itests/framework/framework.go index 880e9f6eb..8a9291d23 100644 --- a/itests/framework/framework.go +++ b/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" @@ -58,7 +59,12 @@ import ( unixfile "github.com/ipfs/go-unixfs/file" "github.com/ipld/go-car" "github.com/libp2p/go-libp2p" +<<<<<<< HEAD "github.com/libp2p/go-libp2p/core/host" +======= + "github.com/libp2p/go-libp2p-core/host" + "github.com/multiformats/go-multihash" +>>>>>>> 108ee6c (fix: cleanup test & handle identity roots) "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" ) @@ -684,8 +690,16 @@ func (f *TestFramework) ExtractFileFromCAR(ctx context.Context, t *testing.T, fi ch, err := car.LoadCar(ctx, bserv.Blockstore(), file) require.NoError(t, err) - b, err := bserv.GetBlock(ctx, ch.Roots[0]) - 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) From 9b370faf28a747c54e3f0ef759e2e4d1e8a89a27 Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Thu, 1 Sep 2022 16:49:49 +1000 Subject: [PATCH 3/4] chore: update go-fil-markets with identity CID fix Ref: https://github.com/filecoin-project/go-fil-markets/pull/747 --- go.mod | 6 ------ itests/framework/framework.go | 4 ---- 2 files changed, 10 deletions(-) diff --git a/go.mod b/go.mod index a449c4137..92bda19ca 100644 --- a/go.mod +++ b/go.mod @@ -8,12 +8,6 @@ replace github.com/filecoin-project/go-jsonrpc => github.com/nonsense/go-jsonrpc // replace github.com/filecoin-project/lotus => ../lotus -// replace github.com/filecoin-project/go-fil-markets => ../go-fil-markets - -// replace github.com/filecoin-project/dagstore => ../dagstore - -// replace github.com/ipld/go-car/v2 => ../../ipld/go-car/v2 - require ( contrib.go.opencensus.io/exporter/prometheus v0.4.0 github.com/BurntSushi/toml v1.1.0 diff --git a/itests/framework/framework.go b/itests/framework/framework.go index 8a9291d23..5bd62c441 100644 --- a/itests/framework/framework.go +++ b/itests/framework/framework.go @@ -59,12 +59,8 @@ import ( unixfile "github.com/ipfs/go-unixfs/file" "github.com/ipld/go-car" "github.com/libp2p/go-libp2p" -<<<<<<< HEAD - "github.com/libp2p/go-libp2p/core/host" -======= "github.com/libp2p/go-libp2p-core/host" "github.com/multiformats/go-multihash" ->>>>>>> 108ee6c (fix: cleanup test & handle identity roots) "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" ) From 0412500a82e252a16577a5dd84779ccb3796699d Mon Sep 17 00:00:00 2001 From: Rod Vagg Date: Thu, 1 Sep 2022 17:15:04 +1000 Subject: [PATCH 4/4] doc: make it clear InlineBuilder isn't recommended i.e. don't copypasta this for your filecoin deal prep --- car/car_offset_writer_test.go | 1 + itests/framework/framework.go | 2 +- testutil/car.go | 1 + transport/httptransport/http_transport_test.go | 1 + 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/car/car_offset_writer_test.go b/car/car_offset_writer_test.go index 0ad70fa20..bd3091223 100644 --- a/car/car_offset_writer_test.go +++ b/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, diff --git a/itests/framework/framework.go b/itests/framework/framework.go index 5bd62c441..770e8f0c2 100644 --- a/itests/framework/framework.go +++ b/itests/framework/framework.go @@ -59,7 +59,7 @@ import ( unixfile "github.com/ipfs/go-unixfs/file" "github.com/ipld/go-car" "github.com/libp2p/go-libp2p" - "github.com/libp2p/go-libp2p-core/host" + "github.com/libp2p/go-libp2p/core/host" "github.com/multiformats/go-multihash" "github.com/stretchr/testify/require" "golang.org/x/sync/errgroup" diff --git a/testutil/car.go b/testutil/car.go index 6c8d7071b..3e85a72b7 100644 --- a/testutil/car.go +++ b/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, diff --git a/transport/httptransport/http_transport_test.go b/transport/httptransport/http_transport_test.go index 0683f83ca..b5b0f1b2d 100644 --- a/transport/httptransport/http_transport_test.go +++ b/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,