Add ordinary and x-only tweaking to spec and simplify implementation

[jonasnick]

This is a draft because I still need to add some comment to the implementation that explains how exactly we make tweak identical to t in tweak_add.

Based on #171

[robot-dreams]

Reviewed up to dcc485d, maybe those first few commits can be split out and merged soon?

Overall the changes (in that range) look good, I just have some cosmetic comments. It was straightforward to directly adapt the spec updates to make the reference implementation use a "Session Context", see 9e773e6

[robot-dreams]

Would it be helpful to add a line such as:

* Let ''(aggnonce, u, pk<sub>1..u</sub>, m) = session_ctx''

or maybe a comment such as "elements of the Session Context are considered in scope"?

(Also in GetSessionKeyCoef below)

[jonasnick]

Added the assignmen, an explanation what that means and a definition for tuples.

[robot-dreams]

1. Should this be GetSessionKeyCoef?
2. P isn't in scope here yet, should this either be changed to P' or switched with the next line?

[jonasnick]

fixed

[robot-dreams]

Nit: Coeff for consistency?

[jonasnick]

fixed

[jonasnick]

After receiving some feedback (H/T @brandonblack) and looking at this PR again, I'm not sure that this is the right way to specify tweaking. Yes, the spec itself is elegant, but the implementation is absolutely not.

In short, the spec is elegant because it only has one keygen algorithm that does keygen and all tweaks at once. Therefore, we can avoid having keygen state. However, in an implementation we want to be more flexible. For example, we want to apply two different tweaks on the same aggregate key without having to recompute the aggregate key. This requires deviating quite a bit from the spec pseudocode. I made it extra complicated in order to be able to say that parity_factor always equals g in the spec. Perhaps we should change the spec to be more practical, at the cost of introducing key aggregation/tweaking state. I'll have to try this out.

maybe those first few commits can be split out and merged soon?

@robot-dreams done, see #171

[brandonblack]

I'm futzing with my implementation of tweaking to see if I can reverse it back to something that would make sense as a specification. So far all I've accomplished is introducing ~21 million state variables.

[robot-dreams]

Quick question: can you give an example of where you'd want to interleave ordinary (BIP32) tweaks and x-only (BIP341) tweaks, as opposed to doing (1) all the ordinary tweaks, followed by (2) all the x-only tweaks?

[jonasnick]

@robot-dreams I can't imagine an example for this. But I don't think allowing interleaving adds any complexity.

[brandonblack]

OK, sorry for the first false start there -- I thought I had a fully working tweak function, but discovered bugs when testing it against the vectors in tests_impl.h instead of just my own happy path (thanks for those).

Here's a specification for my working (really this time) algorithm:

The algorithm ''Tweak(Q, tweak<sub>1..v</sub>, is_xonly_t<sub>1..v</sub>)'' is defined as:
* Let ''Q<sub>0</sub> = Q''
* Let ''g<sub>0</sub> = not has_even_y(Q)''
* Let ''t<sub>0</sub> = 0''
* For ''i = 1 .. v'':
** Let ''t'<sub>i</sub> = int(tweak<sub>i</sub>)''; fail if ''t<sub>i</sub> &ge; n''
** Let ''Q'<sub>i</sub> = -Q<sub>i-1</sub> if ''is_xonly_t<sub>i</sub>'' and ''not has_even_y(Q<sub>i-1</sub>)'' otherwise let ''Q'<sub>i</sub> = Q<sub>i-1</sub>
** Let ''Q<sub>i</sub> = Q'<sub>i</sub> + t'<sub>i</sub>⋅G''
** If ''is_xonly_t<sub>i</sub>'' or ''not has_even_y(Q<sub>i-1</sub>)'':
*** Let ''t"<sub>i</sub> = t<sub>i-1</sub> + t'<sub>i</sub> mod n''
*** Let ''g'<sub>i</sub> = g<sub>i-1</sub>''
** Else:
*** Let ''t"<sub>i</sub> = n - t<sub>i-1</sub> + t'<sub>i</sub> mod n''
*** Let ''g'<sub>i</sub> = not g<sub>i-1</sub>''
** Let ''(t<sub>i</sub>, g<sub>i</sub>) = (t"<sub>i</sub>, g'<sub>i</sub>)'' if ''has_even_y(Q<sub>i</sub>)'' otherwise let ''(t<sub>i</sub>, g<sub>i</sub>) = (n - t"<sub>i</sub>, not g'<sub>i</sub>)''
* Return ''Q<sub>v</sub>, g<sub>v</sub>, t<sub>v</sub>''

This treats g as parity, rather than introducing the "parity factor" concept. It can also be easily adapted to adding a tweak given Q_p, g_p, t_p, which would require more changes elsewhere. This gist has both this version, and the "add tweak" version: https://gist.github.com/brandonblack/8862eed68619dc52555d77027f962a1b

Here's my implementation that logically matches the "add tweak"-style spec:
https://github.com/brandonblack/musig-js/blob/0d67928b6263096c9655304b3d370fbb47c3fe3b/index.ts#L283

[jonasnick]

I'm not sure that this is the right way to specify tweaking. Yes, the spec itself is elegant, but the implementation is absolutely not.

Ok, now I have a much better solution that is also just slightly different to the previous proposal. It allows having a simple implementation that is close to the spec and the spec doesn't require to include a lot of keygen state. This was achieved by not including g[v] in g. I archived the old branch here.

@brandonblack Thank you! I think a downside of your proposal is that it's not as easy to justify as the current algorithm in the "Negation Of The Secret Key When Signing" section. In particular, switching the g values to booleans and letting g[i] depend on g[i-1] seems harder to deal with.

[brandonblack]

Removed comment about mis-push

[real-or-random]

I haven't read the algorithms in detail (and I think it will be easier to do this once you've addressed some of the comments) but I think it's a very good idea to have this tweaking section that explains why the algorithms are as they are.

[jonasnick]

I'm terribly sorry @brandonblack @real-or-random but I pushed the wrong branch yesterday! I fixed that now, so my comment above actually applies now.

[brandonblack]

Spec tACK (if that's a thing).

I modified my KeyAgg, Tweak, PartialSign, PartialVerify, SignAgg, and Session code to match this latest spec, and it was indeed much easier to get right.

Should the Abstract be updated to reflect that this document describes much more than Aggregation?

[jonasnick]

@brandonblack Great to hear that it's now easier to get right. Thanks for the feedback. I fixed the sentence in the abstract.

[jonasnick]

As @brandonblack suggested I moved the compution of t from KeyAggInternal to Tweak which demonstrates how to incrementally apply tweaks like we do in the implementation. I renamed t to tacc[v] and adjusted the explanation. Similarly, I moved the g computation inside Tweak and renamed it gacc[v].

[jonasnick]

I added a few more fixups and pushed the squashed branch. I'd be all ears if anyone has good ideas for a more consistent/readable approach to variable naming

[brandonblack]

ACK f1fad27

Two nits.

[real-or-random]

This is very nice.

ACK eac0df1 -- I haven't checked all the indices etc, so this is more of a Concept ACK than a "pseudocode review ACK" but we we have the ACK by Brandon and this is anyway still a draft, so I think this is good to be merged.

Here are a few high-level comments that you could address here or in a later PR (whatever you feel seems easier):

• The tweaking section should remind the reader that the purpose of specifying tweaking here is only to ensure compatibility with existing uses of tweaking. Draft: "The purpose of specifying tweaking here is only to ensure compatibility with existing uses of tweaking, i.e., that the result of the signing is a valid signature with the tweaks applied. The algorithms here take as input arbitrary tweaks but allowing for arbitrary tweaks will render signatures insecure, and so the tweaks itself still need to obtained according to some other specification (e.g., [insert here]), which preserves security of tweaked signatures even if MuSig2 is used to generate the signatures. This typically involves deriving the tweaks from a hash of the public key given as input to the tweaking algorithm and some other information."
• When describing the algorithms, we could write "x-only public keys" instead of "public keys".
• Since we have so many algorithms now, it's more elegant to drop the "the algorithm ... is defined as:" and just have the name of the algorithm in bold.

I share your impression that mediawiki is not optimal for algorithms but I have no good suggestion currently.

edit: I had the wrong commit id behind the "ACK", fixed now.

[jonasnick]

@real-or-random Good points, but I agree that it's better to address this in a separate PR given that this one is already pretty extensive and has ACKs. I'll put this on the todo list.

Here's a short summary of design decisions taken during the lifetime of this PR:

• Why don't we compute g_v already in KeyAggInternal and multiply it into g? There is an important difference between the spec and an (efficient) implementation: in the spec we do tweaking inside keyagg, but an implementation would have tweaking functions separate from key aggregation. As a result, such an implementation does not know v when applying tweaks. It's much (!) simpler for an implementation to compute g[v] in the signing phase. The spec simply reflects that. Why don't we separate tweaking from key aggregation in the spec as well? That's possible, but would introduce a bit more bloat.
• Why do we compute tacc inside Tweak and not in KeyAggInternal which would be simpler? Again, this is to show that tweaking can easily be done separately from key aggregation.