halo2 specification

[lassebramer]

This is a specification of steps 4 through 26 in the halo2 protocol.

There is also a spec of a polynomial ring over the base field for the Vesta curve. It is included here but should be moved to another crate when we make a generic implementation of polynomial rings over fields.

It passes the hacspec-v1 typechecker without warnings or errors, and the included tests all pass.

Info on the halo2 protocol here

[franziskuskiefer]

Thanks, two preliminary comments.

[Hvassaa]

We also forgot to mention that this specification includes modifications from this PR. Since these modifications are required for an implementation/specification, the PR should hopefully be merged, so that the specification corresponds to the published protocol description.

[spitters]

It would be good to include it, so we have everything in one place.

…

On Thu, Jun 15, 2023 at 10:36 AM Rasmus T. Bjerg ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In halo2/src/halo2.rs <#4 (comment)>: > @@ -0,0 +1,5825 @@ +#![doc = include_str!("../table.md")] Oh yes, we forgot. That file has the protocol description (from the link in the original comment) with some extra annotations. It has a lot of latex math-mode, which requires a katex <https://katex.org/> header file. I believe it needs to be in the root of the workspace, for rustdoc to render the math. Should that be included as well, or would it be more suitable to just remove that line? — Reply to this email directly, view it on GitHub <#4 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABTNTWLQBNCZGQSTCBBECLXLLCRTANCNFSM6AAAAAAZGSY3HY> . You are receiving this because your review was requested.Message ID: ***@***.***>

[lassebramer]

It would be good to include it, so we have everything in one place.
…
On Thu, Jun 15, 2023 at 10:36 AM Rasmus T. Bjerg @.> wrote: @.* commented on this pull request. ------------------------------ In halo2/src/halo2.rs <#4 (comment)>: > @@ -0,0 +1,5825 @@ +#![doc = include_str!("../table.md")] Oh yes, we forgot. That file has the protocol description (from the link in the original comment) with some extra annotations. It has a lot of latex math-mode, which requires a katex https://katex.org/ header file. I believe it needs to be in the root of the workspace, for rustdoc to render the math. Should that be included as well, or would it be more suitable to just remove that line? — Reply to this email directly, view it on GitHub <#4 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABTNTWLQBNCZGQSTCBBECLXLLCRTANCNFSM6AAAAAAZGSY3HY . You are receiving this because your review was requested.Message ID: @.***>

We have pushed the table.md file and the katex-header.html. The katex-header has to be in the root directory for it to work. The rustdoc can be compiled using the command
RUSTDOCFLAGS="--html-in-header katex-header.html" cargo doc --no-deps --document-private-items --open

We use the approach from here to include latex type math in the rustdoc.

[spitters]

Rustdoc is here:
https://hvassaa.github.io/hacspec_halo2/

[spitters]

The code is well documented and has very good test coverage.

[franziskuskiefer]

Thanks, lgtm!

[franziskuskiefer]

@lassebramer looks like tests are sometimes failing. Can you figure out what's going on? Otherwise we'll have to revert the merge for now.

[lassebramer]

@lassebramer looks like tests are sometimes failing. Can you figure out what's going on? Otherwise we'll have to revert the merge for now.

Will look into it now, looks like some logic in the testing

[lassebramer]

@lassebramer looks like tests are sometimes failing. Can you figure out what's going on? Otherwise we'll have to revert the merge for now.

Will look into it now, looks like some logic in the testing

Should be fixed now. Should I make a new PR?(unsure what the flow is once the commit have already been merged)

[franziskuskiefer]

Should be fixed now. Should I make a new PR?(unsure what the flow is once the commit have already been merged)

Yes, please file a follow-up PR.
Thanks for the quick fix!