Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use CAR and padding for piece data #27

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Use CAR and padding for piece data #27

merged 4 commits into from
Jan 10, 2020

Conversation

ingar
Copy link
Contributor

@ingar ingar commented Jan 8, 2020

Summary

The Provider now writes out a padded CAR file of the graph fetched from the client. It uses this file to calculate CommP and verify that it matches the CommP in the proposal. It then saves the path to this file in the MinerDeal, so the node can pass this information on to the mining/binpacking module.

Resolves https://github.com/filecoin-project/go-storage-market/issues/15

@ingar ingar marked this pull request as ready for review January 8, 2020 22:39
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about how OnDealComplete is called.

Otherwise LGTM

@@ -153,6 +169,7 @@ func (p *Provider) staged(ctx context.Context, deal MinerDeal) (func(*MinerDeal)

return func(deal *MinerDeal) {
deal.SectorID = sectorID
deal.PiecePath = file.Path()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe (could be wrong) that in order for this to work, PiecePath also needs to be passed into OnDealComplete

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also also, I forgot that OnDealComplete has a third parameter for the piece path, and given that, I wonder if we need to retain it in the miner deal. (this part is non-blocking, the above is blocking)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, also, also, you should check with @laser, but I believe the miner implementation is not gonna be able to return us the SectorID, and we have an alternate mechanism for that, and, we should probably change that signature (though, no need to do that just yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, this makes sense. I think we do need to store the piece path between deal states, as (upcoming) we will have a few states between writing the piece to the FileStore and when we call OnDealComplete().

@codecov-io
Copy link

Codecov Report

Merging #27 into master will decrease coverage by 0.08%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage    14.7%   14.62%   -0.07%     
==========================================
  Files          26       26              
  Lines        1756     1765       +9     
==========================================
  Hits          258      258              
- Misses       1448     1457       +9     
  Partials       50       50
Impacted Files Coverage Δ
storagemarket/impl/provider.go 0% <0%> (ø) ⬆️
storagemarket/impl/client.go 0% <0%> (ø) ⬆️
storagemarket/impl/client_utils.go 30.31% <0%> (ø) ⬆️
storagemarket/impl/provider_states.go 0% <0%> (ø) ⬆️
pieceio/pieceio.go 100% <100%> (ø) ⬆️

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 ca439f3...b28a3ad. Read the comment docs.

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this needs a rebase on master but LGTM whenever that happens.

@ingar ingar merged commit ad2b1a2 into master Jan 10, 2020
@ingar ingar deleted the feat/car-piece branch January 13, 2020 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants