Add functional-style methods for keys

[Kixunil]

Methods like add_assign are annoying to use because the variable changes meaning after the operation and renaming it is awkward. This could be trivially fixed by adding methods copying the input and performing the operation.

[apoelstra]

Agreed. The add_assign etc come from the upstream C API but there's no reason to preserve this wart.

[tcharding]

Methods like add_assign are annoying to use because the variable changes meaning after the operation and renaming it is awkward.

Is resolution of this issue achieved by removing the implementations of ops::AddAssign?

This could be trivially fixed by adding methods copying the input and performing the operation.

We have implementations of ops::Add and ops::Sub already that do this, right? Or am I missing something?

FTR I don't see why implementing add_assign is any more clunky for an Amount than it is for any other integer type? Am I missing something?

[apoelstra]

This issue is about SecretKey and PublicKey. I don't think we've implemented any of the ops traits on these, though I wouldn't be opposed to it.

I think the issue would be resolved by adding add methods alongside add_assign, and/or removing add_assign in favor of using the ops traits.

[tcharding]

oh my bad, I looked at Amount, I see now the issue title says 'keys'.

EDIT: oh boy, I wasn't even in the right repo, face palm.

[Kixunil]

I was thinking about ops too. One issue is the operations currently accept slice and return Result. I think we should change the parameter to take Scalar or PublicKey. It probably would have prevented mistake that happened recently. Once it's changed the only error remaining is key being invalid. I think it'd be OK to panic since invalid keys should be unlikely for reasonable scalars. But maybe we're creating a DoS footgun for people who don't properly validate input data?

[apoelstra]

The reason we take a slice is that this API is intended for use where you are adding a (hashed) tweak to a key. This is for BIP32 and Taproot. We regret exposing such a low-level API for these two applications.

Zero is a valid tweak so we cannot take a SecretKey, but also, conceptually this API was not meant to add two keys together. It was meant to tweak keys.

We explicitly do not want to support adding public keys together, as part of our general "don't expose primitives to roll your own crypto" philosophy in libsecp. Unfortunately, we do expose this in the pubkey_combine function, which was added as part of a broken Schnorr multisignature scheme many years ago, and which we cannot remove because the LN people found it and started using it.

[Kixunil]

Zero is a valid tweak so we cannot take a SecretKey

That's why I suggested adding Scalar

We explicitly do not want to support adding public keys together, as part of our general "don't expose primitives to roll your own crypto" philosophy in libsecp.

Interesting. I definitely see value in discouraging people from implementing their own crypto. I wonder if the approach of not exposing some fundamental operations is the best one. The thing is there are also people who roll well-reviewed crypto and those have then harder time using it and implementing it, paradoxically worsening security because of increased implementation complexity.

I could get behind avoiding operators and instead have methods with some reasonable name and documentation warning about potential pitfalls.