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

decoupling post worker #7600

Merged

Conversation

siriusyim
Copy link
Contributor

based on:
#6943

@siriusyim siriusyim requested a review from a team as a code owner November 5, 2021 06:45
@siriusyim
Copy link
Contributor Author

siriusyim commented Nov 5, 2021

Hi,@jennijuju
These ci errors are due to the fact that the PR has not been merged: filecoin-project/filecoin-ffi#197

After this part is merged, I will modify it again


var GenerateSingleVanillaProof = ffi.GenerateSingleVanillaProof

var MergeWindowPoStPartitionProofs = ffi.MergeWindowPoStPartitionProofs
Copy link
Contributor

Choose a reason for hiding this comment

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

ffi types should not be used as part of the interface

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, resubmitted

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long on this, lets work on landing this soon.

(reviewing the ffiwrapper parts for now)

I'm not too sure if we need DeDuplicatePrivateSectorInfo, it seems like it should be easy to avoid it

}
body, err := ioutil.ReadAll(r.Body)
if err != nil {
failedRet(500, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use http.Error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Comment on lines 48 to 49
TTGenerateWindowPoSt: "GWDP",
TTGenerateWinningPoSt: "GWNP",
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks tiny bit cleaner:

Suggested change
TTGenerateWindowPoSt: "GWDP",
TTGenerateWinningPoSt: "GWNP",
TTGenerateWindowPoSt: "WDP",
TTGenerateWinningPoSt: "WNP",

Comment on lines 73 to 74
GenerateWinningPoSt(ctx context.Context, mid abi.ActorID, privsectors ffiwrapper.SortedPrivateSectorInfo, randomness abi.PoStRandomness, sectorChallenges *ffiwrapper.FallbackChallenges) ([]proof.PoStProof, error) //perm:admin
GenerateWindowPoSt(ctx context.Context, mid abi.ActorID, privsectors ffiwrapper.SortedPrivateSectorInfo, partitionIdx int, offset int, randomness abi.PoStRandomness, postChallenges *ffiwrapper.FallbackChallenges) (ffiwrapper.WindowPoStResult, error) //perm:admin
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: move above TaskDisable, so that calls which do work are grouped together.


privsectors := ffi.NewSortedPrivateSectorInfo(out...)

deDuplicationPrivsectors, err := ffi.DeDuplicatePrivateSectorInfo(ctx, &privsectors)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we, instead of adding this function, do the following:

  • Add a seen := map[abi.SectorNumber]struct{} map above
  • Keep track of what sectors we've added to out in that map
  • Skip appending to out if we've seen the sector.

This way we should be able to avoid adding this function in filecoin-ffi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, this is a good idea for deduplication, but after deduplication according to the underlying logic of ffi, the sector set must be sorted according to sectorNumber

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@jennijuju
Copy link
Member

Hi @siriusmz how is this going?

@siriusyim
Copy link
Contributor Author

Hi @siriusmz how is this going?
Based on the logic of the ffi library, we believe that the post cannot be modified in accordance with the review comments after the post is split. The reasons are as follows

image

@siriusyim
Copy link
Contributor Author

siriusyim commented Dec 15, 2021

Hi @siriusmz how is this going?

@jennijuju Others have been modified in accordance with the review comments

@jennijuju jennijuju added the P1 P1: Must be resolved label Dec 29, 2021
@jennijuju jennijuju modified the milestones: v1.13.3, v1.15.0 Dec 29, 2021
@siriusyim siriusyim force-pushed the feat/decoupling-post-worker branch 3 times, most recently from 95a787c to 057efae Compare January 5, 2022 11:57
@magik6k magik6k mentioned this pull request Jan 19, 2022
5 tasks
@magik6k magik6k self-assigned this Feb 10, 2022
@hail100
Copy link

hail100 commented Feb 21, 2022

hi @siriusmz @jennijuju @magik6k,can this feature merge before march 4th?

@magik6k magik6k merged commit cf7dd36 into filecoin-project:master Mar 26, 2022
@magik6k
Copy link
Contributor

magik6k commented Mar 26, 2022

Thanks for the PR!

Merged with #7971

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 P1: Must be resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants