Integrate CARv2 blockstore in the retrieval market #560

dirkmc · 2021-06-28T15:05:17Z

WIP

TODO

Unit Tests for the lazy store.

aarshkshah1992 · 2021-07-06T04:35:17Z

@dirkmc Please can you write some unit tests for the lazyBlockstore and we can merge all the work we've done in the Storage & Retrieval Markets to the feature branch and open seperate PRs for the Sharded DAG Store integration work ?

* dagstore lotus mount impl * refactor: nicer error messages * mount api tests * refactor: integrate dag store (#565) Co-authored-by: Dirk McCormick <dirkmdev@gmail.com>

storagemarket/impl/provider_environments.go

storagemarket/impl/provider.go

aarshkshah1992 · 2021-07-11T06:30:26Z

retrievalmarket/types.go

@@ -77,7 +77,8 @@ func (deal *ClientDealState) NextInterval() uint64 {
 // of a retrieval provider
 type ProviderDealState struct {
 	DealProposal
-	StoreID         multistore.StoreID
+	StoreID multistore.StoreID


Please can we get rid of this completely ? We don't need to support inflight storage deals after the upgrade. It'd be great if Markets does NOT depend on go-multistore after the upgrade.

Also, this will enable Lotus to completely get rid of the go-multistore dep and have a green CI.

The migrations depend on this field being here.
We could get rid of it but we'd also have to get rid of those migrations. I'd suggest we do that in a separate PR.

aarshkshah1992 · 2021-07-11T06:33:46Z

filestore/types.go

+
+// CarFileStore provides the path at which to store CAR files created during
+// storage or retrieval
+type CarFileStore interface {


Why do we need a separate abstraction/impl here ? I think we can simply use the existing Filestore here.

The existing fileStore in filestore/filestore.go is for a different use case - it's used to create a file at a path, and delete it later.
Whereas the carStore is just a way for code that uses the retrieval client to be able to tell the retrieval client where to create CAR files. The actual creation of the CAR files is managed by the go-car package.

storagemarket/impl/providerstates/provider_states.go

storagemarket/impl/client_environments.go

carstore/read_write_blockstore.go

aarshkshah1992

Great !

retrievalmarket/impl/client.go

retrievalmarket/impl/clientstates/client_fsm.go

aarshkshah1992 · 2021-07-11T11:50:30Z

retrievalmarket/impl/lazyblockstore_test.go

+
+	"github.com/filecoin-project/go-fil-markets/shared_testutil"
+)
+


Please can you also write a test to verify that the lazy blockstore loads the underlying blockstore only once ? Can pass a CountingBlockstore wrapper to the lazy blockstore...

retrievalmarket/impl/provider_environments.go

aarshkshah1992 · 2021-07-11T11:53:27Z

retrievalmarket/impl/provider.go

@@ -16,11 +16,12 @@ import (
 	datatransfer "github.com/filecoin-project/go-data-transfer"
 	versioning "github.com/filecoin-project/go-ds-versioning/pkg"
 	versionedfsm "github.com/filecoin-project/go-ds-versioning/pkg/fsm"
-	"github.com/filecoin-project/go-multistore"


Please can we get rid of go-multistore completely from go.mod ?

codecov-commenter · 2021-07-12T13:22:32Z

Codecov Report

Merging #560 (6c5210e) into feat/lotus-dagstore-mount (468dae9) will increase coverage by 0.02%.
The diff coverage is 57.64%.

@@                      Coverage Diff                      @@
##           feat/lotus-dagstore-mount     #560      +/-   ##
=============================================================
+ Coverage                      65.69%   65.71%   +0.02%     
=============================================================
  Files                             56       65       +9     
  Lines                           3756     4487     +731     
=============================================================
+ Hits                            2467     2948     +481     
- Misses                          1039     1264     +225     
- Partials                         250      275      +25

Impacted Files	Coverage Δ
dagstore/dagstorewrapper.go	`0.00% <0.00%> (ø)`
dagstore/readonlyblockstore.go	`0.00% <0.00%> (ø)`
filestore/carfilestore.go	`0.00% <0.00%> (ø)`
retrievalmarket/dealstatus.go	`0.00% <ø> (-75.00%)`	⬇️
retrievalmarket/events.go	`0.00% <ø> (-75.00%)`	⬇️
...market/impl/requestvalidation/requestvalidation.go	`88.10% <ø> (+0.45%)`	⬆️
retrievalmarket/types.go	`58.03% <ø> (+0.89%)`	⬆️
storagemarket/impl/client_environments.go	`0.00% <0.00%> (ø)`
storagemarket/impl/clientstates/client_states.go	`89.03% <0.00%> (-1.18%)`	⬇️
storagemarket/types.go	`85.37% <ø> (-2.51%)`	⬇️
... and 83 more

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 05738c3...6c5210e. Read the comment docs.

aarshkshah1992

SHIP.

* dag store lotus mount * storage client code complete-tests remain * storage miner first draft * second draft * provider state tests are now working * most unit tests working and more unit tests * refactor: car store trackers (#559) * Apply suggestions from code review Co-authored-by: dirkmc <dirkmdev@gmail.com> * nits and review * integration testing changes * storage test harness changes * added more TODOs * fix itests for storage * get offline deal tests working * tests work * more tests * integration tests * fix blockstore finalize * Integrate CARv2 blockstore in the retrieval market (#560) * refactor: integrate dag store into retrieval market * fix all tests and the padding issue * refactor: move mount from shared testutil to dagstore dir * refactor: add tests for lazy blockstore * refactor: code cleanup * feat: update go-car to latest * Dagstore lotus mount Implementation with tests (#564) * dagstore lotus mount impl * refactor: nicer error messages * mount api tests * refactor: integrate dag store (#565) Co-authored-by: Dirk McCormick <dirkmdev@gmail.com> * some storage market fixes by aarsh * fix: better error messages in DAG store wrapper * refactor: simplify mock dag store wrapper * fix: TestBounceConnectionDealTransferOngoing * refactor: remove some commented out code * refactor: closable blockstore interface to use full blockstore * fix: TestBounceConnectionDealTransferUnsealing * refactor: add comment explaining lotus mount template * test: verify that the lazy blockstore is only initialized once * fix: comment * fix: always finalize blockstore before reaching complete state (#567) Co-authored-by: aarshkshah1992 <aarshkshah1992@gmail.com> Co-authored-by: dirkmc <dirkmdev@gmail.com>

dirkmc force-pushed the feat/dagstore-retrieval branch 16 times, most recently from 14562e0 to 2b0175b Compare July 2, 2021 07:16

aarshkshah1992 changed the title ~~integrate dag store into retrieval market~~ Integrate CARv2 blockstore into retrieval market Jul 4, 2021

aarshkshah1992 changed the title ~~Integrate CARv2 blockstore into retrieval market~~ Integrate CARv2 blockstore in the retrieval market Jul 4, 2021

aarshkshah1992 mentioned this pull request Jul 4, 2021

[Meta Issue] Replace Badger in Deal Making with CARv2 and the Sharded DAG Store and test with MRA filecoin-project/lotus#6673

Closed

20 tasks

dirkmc force-pushed the feat/dagstore-retrieval branch 2 times, most recently from 0edae9f to bbc081d Compare July 5, 2021 12:35

refactor: integrate dag store into retrieval market

dd70879

dirkmc force-pushed the feat/dagstore-retrieval branch from bbc081d to dd70879 Compare July 5, 2021 12:46

fix all tests and the padding issue

7435c9f

dirkmc added 2 commits July 6, 2021 08:20

refactor: move mount from shared testutil to dagstore dir

932f6c0

refactor: add tests for lazy blockstore

b47f9e6

dirkmc force-pushed the feat/dagstore-retrieval branch from f8eabc1 to e3e351e Compare July 6, 2021 07:26

refactor: code cleanup

fb18a6b

dirkmc force-pushed the feat/dagstore-retrieval branch from e3e351e to fb18a6b Compare July 6, 2021 07:34

feat: update go-car to latest

07597f8

dirkmc force-pushed the feat/dagstore-retrieval branch from e95617a to 07597f8 Compare July 6, 2021 09:07

Dagstore lotus mount Implementation with tests (#564)

92e827d

* dagstore lotus mount impl * refactor: nicer error messages * mount api tests * refactor: integrate dag store (#565) Co-authored-by: Dirk McCormick <dirkmdev@gmail.com>

aarshkshah1992 reviewed Jul 11, 2021

View reviewed changes

some storage market fixes by aarsh

9c440e2

aarshkshah1992 force-pushed the feat/dagstore-retrieval branch from 0b1481b to 9c440e2 Compare July 11, 2021 10:49

aarshkshah1992 reviewed Jul 11, 2021

View reviewed changes

carstore/read_write_blockstore.go Show resolved Hide resolved

aarshkshah1992 reviewed Jul 11, 2021

View reviewed changes

dirkmc added 8 commits July 12, 2021 10:14

fix: better error messages in DAG store wrapper

9e66010

refactor: simplify mock dag store wrapper

79305a7

fix: TestBounceConnectionDealTransferOngoing

4907775

refactor: remove some commented out code

7063c15

refactor: closable blockstore interface to use full blockstore

5e21789

fix: TestBounceConnectionDealTransferUnsealing

7d01608

refactor: add comment explaining lotus mount template

7c81e47

test: verify that the lazy blockstore is only initialized once

296d14c

fix: comment

9a2f61b

dirkmc mentioned this pull request Jul 12, 2021

Always finalize blockstore before reaching end state #567

Merged

fix: always finalize blockstore before reaching complete state (#567)

6c5210e

aarshkshah1992 approved these changes Jul 13, 2021

View reviewed changes

dirkmc marked this pull request as ready for review July 13, 2021 06:57

dirkmc merged commit 2cbcf20 into feat/lotus-dagstore-mount Jul 13, 2021

dirkmc deleted the feat/dagstore-retrieval branch July 13, 2021 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate CARv2 blockstore in the retrieval market #560

Integrate CARv2 blockstore in the retrieval market #560

dirkmc commented Jun 28, 2021 •

edited

aarshkshah1992 commented Jul 6, 2021

aarshkshah1992 Jul 11, 2021

aarshkshah1992 Jul 11, 2021

dirkmc Jul 12, 2021

aarshkshah1992 Jul 11, 2021

dirkmc Jul 12, 2021

aarshkshah1992 left a comment

aarshkshah1992 Jul 11, 2021

dirkmc Jul 12, 2021

aarshkshah1992 Jul 11, 2021

codecov-commenter commented Jul 12, 2021 •

edited

aarshkshah1992 left a comment


		"github.com/filecoin-project/go-fil-markets/shared_testutil"
		)

Integrate CARv2 blockstore in the retrieval market #560

Integrate CARv2 blockstore in the retrieval market #560

Conversation

dirkmc commented Jun 28, 2021 • edited

TODO

aarshkshah1992 commented Jul 6, 2021

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

aarshkshah1992 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

codecov-commenter commented Jul 12, 2021 • edited

Codecov Report

aarshkshah1992 left a comment

Choose a reason for hiding this comment

dirkmc commented Jun 28, 2021 •

edited

codecov-commenter commented Jul 12, 2021 •

edited