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

Enable using transcripts other than merlin::Transcript. #161

Closed
6 tasks
JayWhite2357 opened this issue Sep 13, 2024 · 1 comment · Fixed by #180
Closed
6 tasks

Enable using transcripts other than merlin::Transcript. #161

JayWhite2357 opened this issue Sep 13, 2024 · 1 comment · Fixed by #180
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@JayWhite2357
Copy link
Contributor

JayWhite2357 commented Sep 13, 2024

Background and Motivation

Currently, the Proof of SQL prover uses the merlin crate to provide it's public-coin transcript, created here:

.
However, merlin does not support no_std (see #117 and zkcrypto/merlin#8) and is not conducive to use within the EVM/Solidity.

#123 and #159 add a new Transcript trait that allows for using different transcripts. (Such as the simple Keccak256Transcript.)

If we replace all uses of merlin::Transcript with impl Transcript, we enable the ability to use any transcript implementation that is desired.

Requested Changes

We need to replace all usage of merlin::Transcript with impl Transcript. Tests can still leverage merlin::Transcript because they need a concrete implementation, but all other code should be written generically.

  • extend_serialize_as_le can replace append_auto

  • extend_canonical_serialize_as_le can replace append_canonical_serialize

  • scalar_challenge_as_be can replace challenge_scalar_single ( and challenge_scalars )

  • other methods can be better replacements as relevant

  • CommitmentEvaluationProof::new, CommitmentEvaluationProof::verify_proof, CommitmentEvaluationProof::verify_batched_proof should accept impl crate::base::proof::Transcript instead of merlin::Transcript.

  • All implementers of CommitmentEvaluationProof will need to be updated to implement the changes properly.

    • InnerProductProof - this will still internally need to use merlin::Transcript. This is handled via wrap_transcript.
    • DoryEvaluationProof - the usages of the transcript bubble down to DoryMessages.
  • Either QueryProof::new and QueryProof::verify can be made generic w.r.t. Transcript, or QueryProof itself can be made generic w.r.t. Transcript. It is unclear which is a better design choice. Both solutions will require replacing the old transcript usage with the new within those functions.

  • Tests will likely need to be refactored, but should just used merlin::Transcript as the concrete type. In the future, we may wish to migrate tests to use Keccak256Transcript instead. This can be a separate issue.

@JayWhite2357 JayWhite2357 added enhancement New feature or request good first issue Good for newcomers help wanted labels Sep 13, 2024
@JayWhite2357
Copy link
Contributor Author

@jvff Here is the issue that we discussed. This should complete the work to support wasm32-unknown-unknown without JS.

@JayWhite2357 JayWhite2357 self-assigned this Sep 24, 2024
@JayWhite2357 JayWhite2357 linked a pull request Sep 24, 2024 that will close this issue
JayWhite2357 added a commit that referenced this issue Sep 26, 2024
# Rationale for this change

See #161 for rationale.

# What changes are included in this PR?

NOTE: the individual commits may be good to review individually

* `DoryMessages` accepts `impl Transcript` instead of
`merlin::Transcript`. This is a breaking change, making the proofs
non-backward compatible. The API deos not change.
* Changed `CommitmentEvaluationProof` to accept `impl Transcript` rather
than `merlin::Transcript`.
* Replaced `merlin::Transcript` with `Keccak256Transcript` within
`QueryProof`. Although the proof could be made generic, there is no real
use-case for `merlin`. In the future, we can easily support other
transcripts, but I think it is best to keep it concrete for now.
* Dropped dead code and renamed the old `merlin` module.
* Put `merlin` behind the `blitzar` feature flag since it is still
required for the `blitzar::InnerProductProof`.

* The tests are _NOT_ refactored to remove `merlin`. This can be done in
a separate PR.

# Are these changes tested?

Yes. Existing tests cover the changes.
JayWhite2357 added a commit that referenced this issue Mar 7, 2025
# Rationale for this change

See #161 for rationale.

# What changes are included in this PR?

NOTE: the individual commits may be good to review individually

* `DoryMessages` accepts `impl Transcript` instead of
`merlin::Transcript`. This is a breaking change, making the proofs
non-backward compatible. The API deos not change.
* Changed `CommitmentEvaluationProof` to accept `impl Transcript` rather
than `merlin::Transcript`.
* Replaced `merlin::Transcript` with `Keccak256Transcript` within
`QueryProof`. Although the proof could be made generic, there is no real
use-case for `merlin`. In the future, we can easily support other
transcripts, but I think it is best to keep it concrete for now.
* Dropped dead code and renamed the old `merlin` module.
* Put `merlin` behind the `blitzar` feature flag since it is still
required for the `blitzar::InnerProductProof`.

* The tests are _NOT_ refactored to remove `merlin`. This can be done in
a separate PR.

# Are these changes tested?

Yes. Existing tests cover the changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant