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

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

Merged
merged 2 commits into from
May 4, 2024

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented May 3, 2024

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
Copy link
Collaborator Author

gbotrel commented May 3, 2024

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

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.

go.sum Outdated Show resolved Hide resolved
@gbotrel gbotrel merged commit d9bfacd into master May 4, 2024
7 checks passed
@gbotrel gbotrel deleted the perf/pk_dump_read branch May 4, 2024 02:35
@doutv
Copy link
Contributor

doutv commented Jul 9, 2024

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants