Reduce the size of the Scalar and Point crypto primitives in memory

[msgmaxim]

The structs that underlie our (secp255k1) crypto are defined like this in our dependency curv:

pub struct Secp256k1Scalar {
    purpose: &'static str,
    fe: zeroize::Zeroizing<Option<SK>>,
}

pub struct Secp256k1Point {
    purpose: &'static str,
    ge: Option<PK>,
}

From what I can see, purpose is never actually used/read (the compiler will even give a warning). I can only assume that it is used for debugging: some non-sepc255k1 primitives in curv do use it in Debug implementation and nowhere else. That field doesn't get serialized either, so that information is lost when transmitted over network.

At the same time the field adds 16 bytes to each point/scalar element, adding something like 25-30% to the total memory usage by the multisig client. The only difficulty is that these structs are defined in curv, so we would need to write our own Secp256k1Scalar and Secp256k1Point which should be pretty straightforward as they are just thin wrappers over the primitives from the secp256k1 crate, which does all of the heavy lifting. The only reason we used curv's definitions in the frist place was that the multisig code was orignially loosely based on the KZen's (who designed curv) protocol implemenation, which is no longer the case.

It is possible that having this debug information is justified for KZen's purposes, but in our case the extra memory usage is not negligible.

[AlastairHolmes]

I think we should wait until we understand the memory usage problem before spending resources on this, otherwise this is a shot in the dark.

This would save ~100MB right?

[msgmaxim]

I think we should wait until we understand the memory usage problem before spending resources on this, otherwise this is a shot in the dark.

The motivation is not really to address the memory usage problem, or at least not anymore. Having done the support for polkadot, I realised just how unnecessary the curv wrapping is. All they did was implemented their own trait so they can do generic code with different curves. In certain cases the interface they provide is more awkward than that of the underlying primitive. It would be nice to not depend on this rather obscurecurv dependency and get some more control. I already mentioned that we could remove labels to reduce memory usage, but there would be further befefits. For example, I think we could also use libsecp256k1 instead of secp256k1 to match the verification code on SC (I believe the only the former has no-std support).

This would save ~100MB right?

I suppose, but to be fair this reduction would also affect other chains on the same curve (e.g. Bitcoin), and even signing (which doesn't use as much memory, but we could have many of them executed in parallel).

[AlastairHolmes]

I just don't want to re-impl curv's code ourselves (Because there is no benefit at the moment that isn't a pre-mature optimization i.e. label memory usage), but, if I understand correctly, what your saying is the wrapping basically does nothing so we're not re-impl'ing curv? If so, then I totally support removing the dependency (Especially if the curv interface is poorly built).

[morelazers]

@msgmaxim can we close this since removing curv?

[msgmaxim]

@msgmaxim can we close this since removing curv?

Yes! Closing