Remove dependency on curv in multisig #1819
Conversation
AlastairHolmes
force-pushed
the
chore/remove-curv
branch
from
df99b04
to
1bba639
Compare
| // Wrapping in `Option` to make it easier to keep track | ||
| // of "zero" scalars which often need special treatment | ||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub struct Scalar(Option<SK>); |
There was a problem hiding this comment.
curv's impl wraps this Option in a Zeroize, wouldn't we like to do the same?
There was a problem hiding this comment.
Without this we will not zeroize on drop
There was a problem hiding this comment.
Actually, we do zeroize on drop. See the drop implementation in derive_scalar_impls. Adding another level of inderection creates problems like requiring SK to be ZeroizeOnDrop (which is probably why curv had to use a newtype rather than an alias for SK, as it doesn't implement it natively). Considering how easy it is to implement ourselves, I don't think it is worth it.
| let x_bytes = x.to_bytes_be(); | ||
| let mut array = [0u8; SECRET_KEY_SIZE]; | ||
| array[SECRET_KEY_SIZE - x_bytes.len()..].copy_from_slice(&x_bytes); | ||
|
|
||
| // Safe because `x` is within the group | ||
| // and `array` is correct size by construction | ||
| Scalar(Some(SK::from_slice(&array).unwrap())) |
There was a problem hiding this comment.
I think it would be better to factor this part out into a function and inline the rest of this function (As in the invert function the BigUint you pass to this function cannot be zero (As I understand things))
There was a problem hiding this comment.
Are you suggesting changing from_reduced_bigint to from_reduced_non_zero_bigint? I agree that the inverse shouldn't produce zero, but I don't think it is a big deal if we check anyway and have a slightly nicer name.
closes @1648
Instead of depending on the wrapper from
kzen-curv, we now use primitives from secp256k1 directly. See #1648 for more context. Apart from reducing multisig's memory footprint (I measured a 25% reduction), it is nice to remove this somewhat obscure dependency and gain more control over the implementation (and it better matches what we did for curve25519).Some other improvements:
Scalar::from_bytesis nowScalar::from_bytes_mod_order, which better conveys its expected behaviour. curve25519's implementation is the same, but secp256k1's was wrong as it didn't do modulo reduction (it would result in a crash with a probability of ~1/(2^128), so negligible). Now fixed.From<u32> for Scalarused to create a temporary bigint and doing reduction modulo curve order, which was wasteful as u32 is always smaller and can be trivially converted to Scalar directly, which is the change I made here.Note to reviewers:
secp255k1.rs(the old file, had a typo) andsecp256k1.rs(the new file) are different enough that it makes more sense to review the new file "from scratch" (of course referencing the old file can occasionally be helpful).