Add functional style methods to various keys

[tcharding]

The various _assign methods (add_assign, add_expr_assign, mul_assign, tweak_add_assign) are cumbersome to use because a local variable that uses these methods changes meaning but keeps the same identifier. It would be more useful if we had methods that consumed self and returned the newly modified type.

We notice also that this API is for adding/multiplying tweaks not arbitraryly adding keys.

• Patch 1: Changes add/mul_assign -> add/mul_tweak for PublicKey and SecretKey (incl. re-working unit tests)
• Patch 2: Changes tweak_add_assign -> add_tweak for KeyPair and XOnlyPublicKey
• Patch 3: Changes negate_assign -> negate

All methods changed include:

• New method consumes self and returns the tweaked key
• Original method remains with a deprecated attribute, however I've left a TODO in there for adding the since field.

Close: #415

[apoelstra]

ACK 739dc2e

I think these are all positive changes.

[sanket1729]

utACK 739dc2e. Did not do a deep review here, but looks good at a high level. Left some comments that are not directly related to the objective of this PR.

[sanket1729]

Here and below for all APIs:

We should re-read the underlying secp documentation to confirm these. I think(without referring to docs) that the invalid secret key will only happen if the output secret key was negative.

In this case, we can then add in the documentation stating users can safely unwrap this if they sample tweak randomly.

[sanket1729]

In the long run, it might be better to create a new type: https://docs.rs/secp256k1-zkp/0.6.0/secp256k1_zkp/struct.Tweak.html#

I slightly prefer the name Scalar tough.

[sanket1729]

nit: (Not a strong perference)
I would rather have both methods. add_assign feels more natural when you are doing cryptography math whereas "tweaking" is more a term common in the bitcoin protocol world, but not in the cryptography math world.

For example, while implementing some custom protocol(say bulletproofs), I would be looking for the add_assign method. Whereas while implementing bitcoin taproot or sign to contract APIs, I would look for the tweak API.

[tcharding]

Crypto-geeks, this assertion sometimes fails. Is it correct or do I not understand what is going on here? Thanks

[tcharding]

See the failing test: https://github.com/rust-bitcoin/rust-secp256k1/runs/6806870441?check_suite_focus=true

[apoelstra]

cc @real-or-random do you know offhand how these APIs work?

This test code looks suspicious to me but I can't follow all the tweaking and tweak-dropping to tell what's going wrong.

[real-or-random]

I haven't fully thought it through but I think it should pass like this:

Suggested change

	assert_eq!(parity, other_parity); // Sanity check.
	assert_eq!(_parity ^ parity, other_parity); // Sanity check.

[tcharding]

Thanks for looking @real-or-random, the suggestion did not work (at least locally). I just remove the whole 'sanity check' and passed the parity returned by add_tweak back to tweak_add_check. Does this mean there is some bug in the parity handling or that we should not be returning the parity from XOnlyPublicKey::from_keypair?

[real-or-random]

            // kp = P
            let kp = KeyPair::new(&s, &mut thread_rng());

            // xonly = x(P), i.e., lift_x(x(P)) = ±P
            let (xonly, _) = XOnlyPublicKey::from_keypair(&kp);

            // tweaked_kp = ±P + tG
            let tweaked_kp = kp.add_tweak(&s, &tweak).expect("keypair tweak add failed");

            // tweaked_xonly = x(±P + tG)
            let (tweaked_xonly, parity) = xonly.add_tweak(&s, &tweak).expect("xonly pubkey tweak failed");

            // want_tweaked_xonly = x(±P + tG)
            let (want_tweaked_xonly, tweaked_kp_parity) = XOnlyPublicKey::from_keypair(&tweaked_kp);

            // x(±P + tG) == x(±P + tG)
            assert_eq!(tweaked_xonly, want_tweaked_xonly);

            // lift_x(x(±P + tG)) == lift_x(x(±P + tG))
            assert_eq!(parity, tweaked_kp_parity);

So KeyPair::add_tweak confused me a lot... It calls secp256k1_keypair_xonly_tweak_add, which does two things: It converts the current pubkey to xonly and then tweaks the resulting xonly key. From the name KeyPair::add_tweak, I expect it to tweak the keypair directly without converting to xonly first. So maybe this should be called KeyPair::add_xonly_tweak. (There's no secp256k1_keypair_tweak_add upstream but this is simply because noone has needed this function so far.)

What adds to the confusion here is that KeyPair::add_tweak doesn't output the parity resulting from internal xonly conversation (because secp256k1_keypair_xonly_tweak_add doesn't output it either...) This is what should be equal to _.

So the way to think about tweaking is that there are basically two modes of tweaking: Ordinary tweaking and x-only tweaking (see https://github.com/jonasnick/bips/blob/musig2/bip-musig2.mediawiki#tweaking-definition where we introduce this terminology). This keeps confusing people, including ourselves. We should probably write a blog post on this.

h/t @jonasnick who help me understand what's going on here.

[tcharding]

Changes in force push are non-trivial:

• Rebase on top of the new Scalar type.
• Fixed incorrect example code on the XOnlyPublicKey::add_tweak method.
• Use add_exp_tweak as the replacement name for add_exp_assign (previously I had used add_tweak).
• Removed the parity assertion from unit test as discussed above

[tcharding]

Changes in force push:

• Use name KeyPair::add_xonly_tweak as suggested by @real-or-random
• Add back in a sanity check asserting the matching parities are equal, thanks Tim!

[tcharding]

The parity assertion fails when fuzzing, I don't know if this is to be expected. I added a #[cfg(not(fuzzing))] guarding the parity assertion.

[sanket1729]

The fuzzing logic here is carefully hand-crafted to allow signature forging. I would not be surprised if it broke randomly.

I was also hit with similar stuff when dealing with this is rust-miniscript. rust-bitcoin/rust-miniscript#258

I did a half-hearted attempt for compressed keys #322. Did not look into this particular bug in detail. Maybe, it's related to that

[tcharding]

Does it need investigating more thoroughly or are we ok to just skip the assertion when fuzzing, its not the first time I've done something like this so probably good to know once and for all if I should or should no be doing it?

[real-or-random]

I had a look at this. The fuzzing functions are written with parity in mind [1] and simply return the parity of pk[32]. However, there was a bug when doing this for a keypair, where the correct offset is 64 (because the keypair internally stores sk32||pk64, so we need to add 32 taking the sk into account).

Cherry-picking this should fix it: real-or-random@b1897f3

Does it need investigating more thoroughly or are we ok to just skip the assertion when fuzzing, its not the first time I've done something like this so probably good to know once and for all if I should or should no be doing it?

If you ask me, it's hard to say in general. The "pseudocrypto" in fuzzing tries to emulate a lot but it can't handle everything, so I think this needs to be seen on a case-by-case basis. But @apoelstra knows more about the fuzzing functions, maybe he can suggest a policy.

[1] Well, at least partly... they support getting parity but won't emulate that converting to xonly destroys the parity information

[real-or-random]

Ah by the way, I think the comment lift_x(x(±P + tG)) == lift_x(x(±P + tG)), which I suggested, is nonsense, better to remove it. (The logic should be correct though.)

[tcharding]

Thanks man, I actually think it was the fuzzing stuff that caused the problems in the first place. I learned bunch from your digging so thanks a bunch for doing that.

[tcharding]

Force push is removal of two comments as suggested above. No other changes.

[apoelstra]

ACK 12d4583

[apoelstra]

Yeah, the fuzzing crypto is super ad-hoc. Happy to answer specific questions but if it breaks wit hnew functionality the answer is probably to just hack around it.