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

Drop zeroization, impl Copy for everything #56

Merged
merged 1 commit into from Jun 25, 2020

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Jun 20, 2020

As shown by paritytech/parity-common#400 you can't have reliable memory erasure without keeping it on the heap. Since this is less than acceptable here, we might as well drop zeroization altogether and implement Copy for all types.

This is a conservative PR. Another one that breaks the APIs by replacing reference arguments with values will follow.

@sorpaas
Copy link
Member

sorpaas commented Jun 20, 2020

Another one that breaks the APIs by replacing reference arguments with values will follow.

Why would we want to do this? Wouldn't that unnecessarily copy some stuff when passing function parameters?

@vorot93
Copy link
Contributor Author

vorot93 commented Jun 21, 2020

There's a lot of cloning going on already, and they are cheap most of the time, and the Rust compiler should be able to optimize when not. In terms of semantics using references to what is essentially numbers is very awkward.

@sorpaas
Copy link
Member

sorpaas commented Jun 25, 2020

I think this is okay, per Rust docs:

Generally speaking, if your type can implement Copy, it should. Keep in mind, though, that implementing Copy is part of the public API of your type. If the type might become non-Copy in the future, it could be prudent to omit the Copy implementation now, to avoid a breaking API change.

We should be conducting some benchmarking before the next release though, avoid any regressions.

@sorpaas sorpaas merged commit 7deec29 into paritytech:master Jun 25, 2020
@LLFourn
Copy link

LLFourn commented Jun 26, 2020

As shown by paritytech/parity-common#400 you can't have reliable memory erasure without keeping it on the heap.

Hi I read the PR thread but I didn't understand where this claim comes from. Can you elaborate?

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

3 participants