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

feat: allow for window post proving on a partition basis #1526

Merged
merged 10 commits into from
Oct 25, 2021

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Oct 13, 2021

This is based on #1497 and attempts to reduce the changes to a minimum needed.

TODOs

  • improve naming
  • improve documentation
  • review for more things that can be simplified

@dignifiedquire
Copy link
Contributor Author

@cryptonemo any thoughts why only darwin would show the testfailure?

@cryptonemo
Copy link
Collaborator

@cryptonemo any thoughts why only darwin would show the testfailure?

CI flakiness is my guess. The error is that it's missing parameters. I suspect a re-run may resolve it(?)

@dignifiedquire
Copy link
Contributor Author

CI flakiness is my guess. The error is that it's missing parameters. I suspect a re-run may resolve it(?)

I am not sure, the same error happened on the original PR, but no other PRs

@vmx
Copy link
Contributor

vmx commented Oct 14, 2021

Rerunning CI should do the trick: cryptocorrosion/cryptocorrosion#48

@dignifiedquire
Copy link
Contributor Author

didn’t work

@vmx
Copy link
Contributor

vmx commented Oct 14, 2021

It's a different error though.

@cryptonemo
Copy link
Collaborator

Very odd. And specifically because it's a window post test failing for code that's changed those parts, it's suspect. I'll see if I can find anything locally at some point. This is not the kind of code I'd normally quickly merge in any case, as everything touching PoSt needs careful review.

cryptonemo
cryptonemo previously approved these changes Oct 22, 2021
Copy link
Collaborator

@cryptonemo cryptonemo left a comment

Choose a reason for hiding this comment

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

As a first cut, this looks good. I was able to generate the parameters on darwin, so CI is good, but haven't been able to hook up the save/restore code, so it's done each time. It's not ideal, but if this can land in Proofs v10.1.0, it can be resolved after that.

@cryptonemo
Copy link
Collaborator

@cryptonemo any thoughts why only darwin would show the testfailure?

Resolved -- the test parameters are now generated on darwin.

@DrPeterVanNostrand
Copy link
Contributor

Looks good to me, my only question is regarding the mixed use of get_partitions_for_window_post and get_num_partition_for_fallback_post.

@cryptonemo cryptonemo merged commit c7e58dd into master Oct 25, 2021
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.

4 participants