search

paritytech / libsecp256k1

Pure Rust Implementation of secp256k1.
Apache License 2.0
175 stars 84 forks source link

Drop zeroization, impl Copy for everything #56

Closed vorot93 closed 4 years ago

vorot93 commented 4 years ago

As shown by https://github.com/paritytech/parity-common/pull/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

LLFourn commented 4 years ago

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?