Skip to content

feat(core): update read_zkey with new_unchecked#129

Merged
oskarth merged 3 commits intomainfrom
zkey
Apr 26, 2024
Merged

feat(core): update read_zkey with new_unchecked#129
oskarth merged 3 commits intomainfrom
zkey

Conversation

@vivianjeng
Copy link
Copy Markdown
Member

@vivianjeng vivianjeng commented Apr 21, 2024

According to #25 (comment)

loading zkey time ark-zkey with new_unchecked
multiplier2 872.00ns 1.21µs
keccak256 115.82s 1.56s

@oskarth
Copy link
Copy Markdown
Collaborator

oskarth commented Apr 21, 2024

I wonder if we want to combine ark-zkey with this? Because with ark-zkey we get 50% file size reduction due to point compression.

@vivianjeng vivianjeng marked this pull request as ready for review April 24, 2024 02:21
@vivianjeng vivianjeng requested a review from oskarth April 24, 2024 02:22
// read_zkey(&mut reader).expect("Failed to read zkey")
// });

static ARKZKEY: Lazy<(ProvingKey<Bn254>, ConstraintMatrices<Fr>)> = Lazy::new(|| {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do we have to remove the ARKZKEY stuff you think? or is this preferable?

thinking mostly about space savings which are quite significant

@oskarth
Copy link
Copy Markdown
Collaborator

oskarth commented Apr 24, 2024

If we remove/don't update arkzkey stuff, could we create an issue explaining problem and current state? So even if we don't do it now we can introduce this for arkzkey too

@vivianjeng
Copy link
Copy Markdown
Member Author

I think we can create the issue to describe the situation

I think arkzkey is still doable without the current serialization/deserialization
like in circom-compat
https://github.com/arkworks-rs/circom-compat/blob/170b10fc9ed182b5f72ecf379033dda023d0bf07/src/zkey.rs#L328

but I am figuring out what is the correct way to serialize/deserialize

@vivianjeng
Copy link
Copy Markdown
Member Author

Create an issue here
#130

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