-
Notifications
You must be signed in to change notification settings - Fork 184
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: Add generic hyrax scheme #387
base: main
Are you sure you want to change the base?
Conversation
90df800
to
3bdb90c
Compare
3bdb90c
to
e725313
Compare
a4a2356
to
53f74c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes so that this PR is blocked until I approve @Dustin-Ray and @jacobtrombetta can you give an initial review.
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_helpers.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_helpers.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_helpers.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_helpers.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_helpers.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_helpers.rs
Outdated
Show resolved
Hide resolved
@stuarttimwhite Its really cool you were able to spin this up so quickly, nice work. Right off the bat I would ask that we add unit testing for any new functionality being added, particularly in the RE your comment about the helper module containing duplicate code from dory, if its not a huge lift you could try to just use the dory code, but if you want to save that for later I think it would be ok. |
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_commitment_evaluation_proof.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_commitment_evaluation_proof.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_commitment_evaluation_proof.rs
Outdated
Show resolved
Hide resolved
Are there any tests that were used to confirm the Hyrax commitment scheme is working as expected? I don't see any, which makes me nervous. Since these are commitments we should really include tests. |
Can you add any more to the description about the Hyrax scheme or what sp1 is? For example, I don't know what sp1 is or why it is important. Additional information will also help other people when looking at the code. Here is an example from when the Hyrax multilinear PCS was added to Arkworks: arkworks-rs/poly-commit#130. (The PR also includes tests in the PR, if you were looking to add tests to this PR.) |
This is a nice comment, @stuarttimwhite if you look through the dory code, there are specific references to the paper involved the implementation and the specific sections where certain algorithms appear. But even for a brief description or background, I think referring to the hyrax paper could be a good place to start |
51e8fac
to
84feda9
Compare
@@ -0,0 +1,5 @@ | |||
/// The hyrax scheme in generic form. | |||
pub(super) mod base; | |||
/// A valid scheme that can be used for testing. This could become its own scheme. It's a reasonable scheme. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesnt make a lot of sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a change. Should be more clear now.
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_configuration.rs
Outdated
Show resolved
Hide resolved
crates/proof-of-sql/src/proof_primitive/hyrax/base/hyrax_configuration.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im a bit wary of the complexity introduced with the associated types, but I think its ok for now, and they work for the use case here.
What would be interesting to see is if you can please add a test here that shows a round trip usage of hyrax evaluation proof, I think that would put us in a pretty good place
75452e5
to
7fd46e9
Compare
Unfortunately test code can't be used within integration_tests. I do plan to add a test using the Sp1Configuration in the next PR here, so this would be remedied in relatively short order. And I have run a simple query locally using the test scheme within integration_tests (temporarily unmarking code as test code as needed), and that test passed. Would it be acceptable to wait for the next PR to add the round trip tests? |
7fd46e9
to
a7bac0a
Compare
Please be sure to look over the pull request guidelines here: https://github.com/spaceandtimelabs/sxt-proof-of-sql/blob/main/CONTRIBUTING.md#submit-pr.
Please go through the following checklist
!
is used if and only if at least one breaking change has been introduced.source scripts/run_ci_checks.sh
.Rationale for this change
There is a need for a Hyrax scheme specifically for implementing proof of sql with the sp1 zkvm (docs here).
What changes are included in this PR?
Are these changes tested?
There will be a follow up PR with a specific Sp1 implementation, which will have sp1 specific tests.