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

Fix some failed precommit handling #3445

Merged
merged 1 commit into from
Sep 30, 2020
Merged

Conversation

arajasek
Copy link
Contributor

@arajasek arajasek commented Sep 1, 2020

I think this is correct?

Thanks to @Arctic in the Filecoin slack for the report.

@arajasek arajasek self-assigned this Sep 1, 2020
@magik6k
Copy link
Contributor

magik6k commented Sep 1, 2020

Can you share more context on this issue?

@hyunmoon hyunmoon mentioned this pull request Sep 1, 2020
@arajasek
Copy link
Contributor Author

arajasek commented Sep 1, 2020

@magik6k I think it's just a question of whether this is the correct thing to be doing.

  • Is the boolean returned by checkPreCommitted an indication of whether the sector has already precommitted? If so, I think we should be returning false in the error cases, and true otherwise?

  • If I'm understanding it correctly, the log.Warn("sector %d is precommitted on chain, but we don't have precommit message", sector.SectorNumber) should be triggered if we didn't have the PreCommitMessage field in the SectorInfo (that is if it is nil, not if it isn't).

Might be wrong on both counts 🤷‍♀️

@whyrusleeping
Copy link
Member

@magik6k context is here: https://filecoinproject.slack.com/archives/C0179RNEMU4/p1598926390173100

someone claims theyve found a silly bug but doesnt really say what it is. they seem really intent on being 'helpful'

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.

This fix looks correct, not sure what how this code ended up in this shape

@magik6k magik6k merged commit 430897c into master Sep 30, 2020
@magik6k magik6k deleted the asr/precommit-failed-fix branch September 30, 2020 07:35
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