Skip to content

perf,feat: groth16.ProvingKey implements BinaryDumper using gnark-crypto unsafe #1124

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.

Sign up for GitHub

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

Jump to bottom
Merged
gbotrel merged 2 commits into from
May 4, 2024
Merged

perf,feat: groth16.ProvingKey implements BinaryDumper using gnark-crypto unsafe #1124

gbotrel merged 2 commits into from
May 4, 2024

Conversation

@gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented May 3, 2024 •
edited
Loading

Sister PR to Consensys/gnark-crypto#501 .

Essentially, addresses the issue of slow deserialization for (large) proving keys. ReadFrom and UnsafeReadFrom can take many seconds (if not minutes) due to subgroup checks, point decompression and "safe" point reconstruction (involve at least converting []byte canonical field elements into Montgomery repr).

This PR introduces pk.WriteDump and pk.ReadDump for scenarios where want to minimize loading time, don't need portability (uses unsafe; pk.ReadDump must read a stream generated by pk.WriteDump on a similar architecture: 32/64 bit little/big endian (most popular platforms these days are 64bits little endian)), and don't need to perform any sanity checks.

A quick bench for a medium sized groth16.ProvingKey for bls12-377; divides by 100x decoding time 🔥 (but since we use unsafe we are io bound, here we ... kind of benchmark memcopy for 80% of the pk).

BenchmarkSerialization/deserialize_pk_from_dump
BenchmarkSerialization/deserialize_pk_from_dump-10                     6         213271979 ns/op        2314815138 B/op      141 allocs/op
BenchmarkSerialization/deserialize_pk_unsafereadfrom
BenchmarkSerialization/deserialize_pk_unsafereadfrom-10                1        19682179709 ns/op       3226044160 B/op 14597433 allocs/op

@gbotrel gbotrel requested review from ivokub and yelhousni May 3, 2024 02:33
@gbotrel
Copy link
Collaborator Author

gbotrel commented May 3, 2024

Considering a rename to WriteMemDump , and remove UnsafeReadFrom (i.e. put it behind UnsafeRead(WithoutSubgroupChecks()) ).

ivokub
ivokub approved these changes May 4, 2024
View reviewed changes
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Looks perfect.

@gbotrel gbotrel merged commit d9bfacd into master May 4, 2024
@gbotrel gbotrel deleted the perf/pk_dump_read branch May 4, 2024 02:35
@doutv
Copy link
Contributor

doutv commented Jul 9, 2024 •
edited
Loading

Any security issues if we skip subgroup checks?

I want to use UnsafeReadFrom to speed up proving. What it the security assumption and how should I make the system secure?

Use with caution, as crafted points from an untrusted source can lead to crypto-attacks.
https://github.com/Consensys/gnark-crypto/blob/f483b7d9c9b49276f284441ce91af8a278c01079/ecc/bn254/marshal.go#L416

@gbotrel
Copy link
Collaborator Author

gbotrel commented Jul 9, 2024

yes, you should do subgroup checks if you don't trust the source of the points you are reading. this method (ReadDump / WriteDump) is intended for scenarios where you store some points / key on a trusted destination / have other mechanism to verify integrity (checksum, ...).

@doutv
Copy link
Contributor

doutv commented Jul 9, 2024 •
edited
Loading

In the worst case, the prover read a malicious proving key, and generate an invalid proof. An honest verifier will detect the invalid proof and reject it. Soundness property of SNARK.

It seems ok to skip subgroup checks in such case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivokub ivokub ivokub approved these changes

@yelhousni yelhousni Awaiting requested review from yelhousni

Assignees

No one assigned

Labels

type: new feature type: perf

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gbotrel @doutv @ivokub