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

RPO STARK-based signature DSA (with zero knowledge) #349

Merged
merged 25 commits into from
Dec 13, 2024

Conversation

Al-Kindi-0
Copy link
Collaborator

Describe your changes

Same as #342 but uses the branch of Winterfell implementing zero-knowledge.

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! I left some comments inline.

Also, one thing that would be great to do is to add a section to the benchmarks section comparing Falcon and STARK signatures (key gen, sign, verify). For this we probably need to add benchmarks to measure these.

src/hash/rescue/rpo/digest.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpx/digest.rs Outdated Show resolved Hide resolved
src/dsa/mod.rs Show resolved Hide resolved
src/dsa/rpo_stark/signature/mod.rs Outdated Show resolved Hide resolved
src/dsa/rpo_stark/signature/mod.rs Show resolved Hide resolved
src/dsa/rpo_stark/stark/prover.rs Outdated Show resolved Hide resolved
Comment on lines +43 to +46
// generate the initial seed for the PRNG used for zero-knowledge
let mut seed = <ChaCha20Rng as SeedableRng>::Seed::default();
let mut rng = thread_rng();
rng.fill_bytes(&mut seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be able to do this in WASM (or any no-std context), right? I wonder what else can we do here (having a fully deterministic signature would have been nice - but we don't have this yet, right?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I have now made everything depend on one seed, except for the salted Merkle tree which we should discuss in the relevant issue.
Once this is fixed we should be able to de-randomize the signature e.g., make the initial seed a deterministic function of the inputs and some other fixed constants

src/hash/rescue/mod.rs Outdated Show resolved Hide resolved
src/dsa/rpo_stark/stark/air.rs Outdated Show resolved Hide resolved
src/dsa/rpo_stark/stark/prover.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some more comments inline.

Other outstanding things are:

  • Come up with a better name for the signature scheme.
  • Add a benchmark. This could be in a follow-up PR.

src/dsa/rpo_stark/signature/mod.rs Show resolved Hide resolved
src/dsa/rpo_stark/signature/mod.rs Show resolved Hide resolved
src/dsa/rpo_stark/stark/air.rs Outdated Show resolved Hide resolved
src/dsa/rpo_stark/stark/mod.rs Show resolved Hide resolved
src/dsa/rpo_stark/stark/prover.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpo/digest.rs Outdated Show resolved Hide resolved
src/hash/rescue/rpx/digest.rs Outdated Show resolved Hide resolved
src/rand/rpo.rs Show resolved Hide resolved
src/rand/rpx.rs Show resolved Hide resolved
@Al-Kindi-0
Copy link
Collaborator Author

Other outstanding things are:

* Come up with a better name for the signature scheme.

Opened the following issue for this #353

* Add a benchmark. This could be in a follow-up PR.

Opened the following PR for this #354

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few small comments inline.

I'm wondering that maybe instead of merging it into next, we should create a separate branch - e.g., rpo_dsa and merge this and subsequent related PRs into it. This way, we won't break the next branch until the relevant updates are made in winterfell (and I think the next step is to get the ZK PR in winterfell merged - right?).

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/dsa/rpo_stark/stark/mod.rs Show resolved Hide resolved
@Al-Kindi-0 Al-Kindi-0 changed the base branch from next to rpo-dsa December 11, 2024 16:52
@Al-Kindi-0
Copy link
Collaborator Author

I'm wondering that maybe instead of merging it into next, we should create a separate branch - e.g., rpo_dsa and merge this and subsequent related PRs into it. This way, we won't break the next branch until the relevant updates are made in winterfell

That's a must, otherwise things will break. Changed the base now.

(and I think the next step is to get the ZK PR in winterfell merged - right?).

That's the logical next step

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good! I added some more comments inline - mostly about dependencies and features.

One thing I'm still trying to understand how this will work in no-std environment (i.e., WASM).

Cargo.toml Outdated Show resolved Hide resolved
src/dsa/rpo_stark/signature/mod.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Copy link

sonarcloud bot commented Dec 12, 2024

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!

@bobbinth bobbinth merged commit 335c50f into rpo-dsa Dec 13, 2024
13 checks passed
@bobbinth bobbinth deleted the al-stark-signature-dev-masm branch December 13, 2024 03:33
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.

2 participants