migrate to DAG store + CARv2 blockstores for storage and retrieval #576
Conversation
There was a problem hiding this comment.
This is quite a large contribution, so it will take a few passes to review fully ❤️ This is a high-level/architectural pass.
- IMO, go-fil-markets shouldn't own the DAG store nor the CAR file trackers. go-fil-markets is supposed to be lightweight and reusable as a library, and I would expect no direct side effects on disk.
- Suggestion: have the DAG store injected into go-fil-markets from Lotus. This also makes parameter configuration (directories, throttling, etc.) easier, as well as future CLI/JSON-RPC operations.
- Similarly, I'm not convinced that the Lotus mount implementation should live here. go-fil-markets is not Lotus-specific, it's built to be used by other implementations (e.g. Venus), and even independently of Filecoin full node implementations, as a lightweight import (e.g. deal making robots, Estuary, etc.)
- Suggestion: can we move the Lotus mount to the lotus repo?
- go-fil-markets shouldn't be doing any file handling or management itself. go-fil-markets is intended to be used a library, and this complicates its intergration into third-party software.
- Suggestion: I think we need an abstraction functionally similar to the Multistore here. I can take a pass and see what I come up with.
- I'm worried about the profileration of CAR file trackers with apparent code duplication. Can we consolidate the read-only and read-write trackers into a single one?
- The CAR trackers won't remember state between restarts. Is this intended?
- The CAR trackers seem to simply abstract over maps. I wonder if they are truly necessary? I'm not sure they provide any value.
- The CarFileStore also seems a little bit unnecessary. I do see the case for a component that acts like an "on-disk CAR depot". I wonder if it make sense to merge the read-only tracker, read-write tracker, and the CarFileStore into such a component, taking the place of the Multistore?
I agree with this.
I agree with this.
Note: All file handling is now done by the
Hmmm.. how can we do this ?
Yes, this is in-memory. The CAR file path is persisted to disk as part of the deal state and we can always recreate/resume the CAR blockstore from CAR file.
It's nice to have. If we get rid of these trackers, we'd have to put this (deal -> CAR blockstore) creation, state management and the corresponding blockstore cleanup code in the Storage Client, Storage Provider, Retrieval Provider and Retrieval Client. We don't wanna copy the same code in all the four places.
This would be nice to have for sure. I am just worried about building a new component, testing it, integrating it and then testing the whole thing again in a span of 3 days while we are also focused on M1 testing. But, we can discuss this. |
Codecov Report
@@ Coverage Diff @@
## master #576 +/- ##
==========================================
- Coverage 66.18% 65.43% -0.74%
==========================================
Files 56 62 +6
Lines 4263 4443 +180
==========================================
+ Hits 2821 2907 +86
- Misses 1191 1256 +65
- Partials 251 280 +29
Continue to review full report at Codecov.
|
aarshkshah1992
force-pushed
the
feat/wip-markets-dagstore
branch
from
94f8a43
to
f8b9c7d
Compare
raulk
force-pushed
the
feat/wip-markets-dagstore
branch
from
b5fdee3
to
fae9419
Compare
There was a problem hiding this comment.
Mid review notes (through to storage market):
Almost all my comments are non blocking, however:
- I'd like to get the stores/filestore_test duplicate logic either fixed or understand why it's not duplicate
- I'd like to get answers to:
- why all the test voucher amounts changed
- whether it means anything that the variable used for PieceCID under piece info in the integration test changed
Stuff that I'm fine merging, but we should really consider seperate future cleanup tickets for:
- finding a simply solution to the lazy blockstore
- dropping the UnsealSector method if we don't use it
Will continue with more notes when I'm done.
| @@ -275,7 +280,7 @@ func TestClientCanMakeDealWithProvider(t *testing.T) { | |||
| {name: "multi-block file retrieval succeeds", | |||
| filename: "lorem.txt", | |||
| filesize: 19000, | |||
| voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, | |||
| voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10174000), abi.NewTokenAmount(19958000)}, | |||
There was a problem hiding this comment.
I must be dense but I've read through this multiple times and still cant figure out why the vouchers changed. It appears (I think) that everything went up by exactly 38000? Which happens to be 2x the filesize? I'd like to understand why.
There was a problem hiding this comment.
Good question -- @dirkmc could you investigate this? It does sound like an unexpected functional change.
There was a problem hiding this comment.
To be honest I'm not sure either. This is the commit where the change was made:
cf7e2a9
But I'm not sure why the amounts are different.
There was a problem hiding this comment.
@raulk please take a look 👁️👁️
There was a problem hiding this comment.
Following investigation, this change is correct. We modified the DagBuilderParams to match those used in lotus client import for a more realistic test, and that leads to a heavier DAG, thus higher cost.
If I revert that change, the voucher amounts revert to the previous ones.
Conclusion: side-effect of the test changing (for the better).
cf7e2a9#diff-26f3d1665dab327881c8abd11dfa4b1e249f730bddfefdec7363fe08290148fdR113-R122
There was a problem hiding this comment.
@raulk is correct here.
| @@ -61,7 +63,7 @@ func TestStorageRetrieval(t *testing.T) { | |||
| pricePerByte: abi.NewTokenAmount(1000), | |||
| paymentInterval: uint64(10000), | |||
| paymentIntervalIncrease: uint64(1000), | |||
| voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10136000), abi.NewTokenAmount(19920000)}, | |||
| voucherAmts: []abi.TokenAmount{abi.NewTokenAmount(10174000), abi.NewTokenAmount(19958000)}, | |||
There was a problem hiding this comment.
still confused about the voucher changes
There was a problem hiding this comment.
👍 @dirkmc let's follow up on this!
There was a problem hiding this comment.
See above. Nothing fishy here.
There was a problem hiding this comment.
Review at end:
NO additional blocking changes but I would like to get some answers to above questions about the retrieval side and resolve my understanding re: the duplicate tests in the one retrieval file.
I'd also like to see some of my suggestions both on retrieval and now on storage for post-merge cleanup acted upon, but I do not want to hold up merge and release any further.
| commP3 := genProviderCommP(t, carV2File1, pieceSize) | ||
| commP4 := genProviderCommP(t, carV2File2, pieceSize) | ||
|
|
||
| require.Equal(t, commP1, commP3) |
There was a problem hiding this comment.
I do not understand the value of this test -- are we surprised that to runs of the same function on the same file generate the same output while two runs different files produce different output? That seems somewhat self-evident. If the goal is to test that is actually produces "the right output" I feel like this test is incomplete. However, I also feed the loginc in the code itself is small enough that I'm not sure why we need to test this, especially since it primarily relies on code from commp-utils
There was a problem hiding this comment.
Good question!
Will follow up with @aarshkshah1992
There was a problem hiding this comment.
This might be a working test to validate that go-car/v2 was doing the right thing during development of that library. However, we can choose to keep it as a sentinel test.
raulk
force-pushed
the
feat/wip-markets-dagstore
branch
from
8e519d4
to
bd6c99c
Compare
This commit introduces changes in state machines, abstractions, and provider/client environments to remove the reliance on a monolithic blockstore, and instead migrate to a solution backed by the dagstore (https://github.com/filecoin-project/dagstore). Blockstores are not managed by this module, and are managed by the caller. When this module needs a blockstore in the context of a storage deal or a retrieval, it calls the relevant BlockstoreAccessor. We believe the test coverage to be pretty satisfactory, with a substantial portion of the LoC diff corresponding to tests. This commit is a squashed version of 89 commits developed by @aarshkshah1992, @dirkmc, and @raulk over the course of months. This contribution was thoroughly tested in the M1 milestone with minerX.2: filecoin-project/lotus#6852. Read more about the motivation here: - protocol/web3-dev-team#116 - https://github.com/filecoin-project/dagstore/blob/master/docs/design.md Co-authored-by: aarshkshah1992 <[email protected]> Co-authored-by: Dirk McCormick <[email protected]>
raulk
force-pushed
the
feat/wip-markets-dagstore
branch
from
bd6c99c
to
bcb4d1f
Compare
This was broken by #576 when the node orb was dropped for go only
This was broken by #576 when the node orb was dropped for go only
This PR introduces the go-fil-markets side of changes to migrate from a monolithic Badger-backed blockstore, to the dagstore, backed by indexed CARv2 files.
This contribution introduces changes in state machines, abstractions, and provider/client environments to remove the reliance on a monolithic blockstore, and instead migrate to a solution backed by the dagstore.
Blockstores are not managed by this module, and are managed by the caller. When this module needs a blockstore in the context of a storage deal or a retrieval, it calls the relevant BlockstoreAccessor.
We believe the test coverage to be pretty satisfactory, with a substantial portion of the LoC diff corresponding to tests.
This commit is a squashed version of 89 commits developed by @aarshkshah1992, @dirkmc, and @raulk over the course of months.
This contribution was thoroughly tested in the M1 milestone with minerX.2: filecoin-project/lotus#6852.
Read more about the motivation here:
Co-authored-by: @aarshkshah1992 @dirkmc @raulk.
For more information refer to the description in filecoin-project/lotus#6671.