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

feat(rust): add KzgSettings::clone #358

Closed

Conversation

mattsse
Copy link
Contributor

@mattsse mattsse commented Sep 11, 2023

Adds Clone impl for KzgSettings

since c_kzg_4844.c does alloc+free, Clone must alloc accordingly, free occurs on drop.

This is only possible because the number of G1 and G2 are constant and enforced by KzgSettings API.

@asn-d6
Copy link
Contributor

asn-d6 commented Sep 12, 2023

Hey @mattsse, thanks for the PR!

I naturally have an aversion towards unsafe code, so allow me to ask some questions.

Would this feature be possible without unsafe? And if yes, what would be the drawbacks of that approach?

Also, what do you currently do in your codebase, now that Clone is not available?

@mattsse
Copy link
Contributor Author

mattsse commented Sep 12, 2023

Would this feature be possible without unsafe? And if yes, what would be the drawbacks of that approach?

it is not, because copying/dereferencing a pointer always requires unsafe

Also, what do you currently do in your codebase, now that Clone is not available?

Currently, we're required to use Arc<KzgSettings>, because we need it in a few places.
though even with a clone impl, I'd continue using an Arc where possible, because Clone requires a few allocations.

but having the option to clone it would make it easier to use under some circumstances where an API does not support Arc<KzgSettings>

@mattsse
Copy link
Contributor Author

mattsse commented Sep 14, 2023

I thought about this a bit, and this feature is actually not sound, but KzgSettings can still be created with arbitrary length via unsafe external calls, so there's no real guarantee the num of points are constant

@mattsse mattsse closed this Sep 14, 2023
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.

None yet

2 participants