musig-spec: Add naive Python reference implementation

[robot-dreams]

Analogous to BIP-340's naive reference implementation (I even copied some helper functions from that implementation). Besides being a demonstration, I think it would also be useful for quickly prototyping use cases, extensions, potential attacks.

We can also wait until the spec is moved a separate repo, per #157 (review)

[real-or-random]

ACK mod nits

[real-or-random]

We can also wait until the spec is moved a separate repo, per #157 (review)

Yes, we should move it to a separate repo but that should block meaningful work. How should we do this? I could just create a branch of bips and add you. Anything else?

[robot-dreams]

Sure, that makes sense!

[real-or-random]

ACK mod one more nit ;)

[real-or-random]

maybe we should have a set_infinite to hide the None, which was supposed to be an implementation detail.

[robot-dreams]

Sure! I added a definition infinity = None but left the part copied from the BIP-340 reference unchanged

[real-or-random]

Nice, this is even simpler.

[real-or-random]

@jonasnick This is roughly what I suggest for KeyAgg in the spec. It's the same as in our C code. I think we should give that algorithm to people, then they don't need to figure it out by themselves. It's not entirely trivial.

Suggested change

	for i in range(u):
	a_i = key_agg_coeff(pubkeys, pubkeys[i])
	P_i = lift_x(pubkeys[i])
	Q = point_add(Q, point_mul(P_i, a_i))
	second = None
	for i in range(1, u):
	if pubkeys[i] != pubkeys[1]:
	second = i
	break
	for i in range(u):
	a_i = 1 if i == second else key_agg_coeff(pubkeys, pubkeys[i])
	P_i = lift_x(pubkeys[i])
	Q = point_add(Q, point_mul(P_i, a_i))

@robot-dreams (not this PR, sorry for posting this here :P)

[jonasnick]

Is your suggestion to remove the IsSecond predicate? I'm not able to spot the advantages of your approach immediately. I think it is attractive that key_agg_coeff is the coefficient of the key (including the second one). Another downside is that overwriting a variable (second) has been criticized in the past (EDIT: sorry nvm, there would be no overwrite in the spec), which probably also applies to breaking.

Btw, it should be pubkeys[i] != pubkeys[0]:

[real-or-random]

My reasoning is that this algorithm has running O(n) whereas the current one has O(n^2). Implementers will notice and try to come up with the more efficient version (our own C code is the best example!). I believe that one of the main reasons for having pseudocode and even Python code in cryptographic specs is that people can just translate it to their target language without the need to think about the algorithm (and potentially screw up, see my mistake... -.-).

Is your suggestion to remove the IsSecond predicate? [...] I think it is attractive that key_agg_coeff is the coefficient of the key (including the second one).

I agree that it's is attractive that key_agg_coeff is a pure function currently, it's certainly more readable (I think there's a reason why we wrote it like this in the paper.) Maybe we can have the best of both worlds: We can write IsSecond and KeyAggCoeff in "mathematical" style such that it's easy to grasp and give algorithms for the more efficient version?

[jonasnick]

Oh sorry, I hadn't considered performance optimization at all since we had only discussed so far how IsSecond is confusing people. I generally prefer clarity over performance, but in this case I agree that we don't want to have O(N^2) in the spec unnecessarily.

Keeping the IsSecond calculation inside KeyAggCoef is not only more readable but also required if we want signing and partial sig verification to not deal with IsSecond directly, because both algorithms call KeyAggCoef to get the coef of a single signer.

Providing both a naive algorithm and a performant one is an interesting idea to explore.

[jonasnick]

By the way, I think the screw up potential when trying to optimize this yourself is neglible if we provide good test vectors.

[real-or-random]

utACK 01f62b2

[real-or-random]

ACK 134593e

nit is not blocking

[real-or-random]

We should later add a few test vectors where these are arguments are not empty (but this should not block the draft now.)

[robot-dreams]

Since this is a randomized test anyway it's easy to update the test to include these. I also randomly included some tweaks.

[real-or-random]

nit: Should we rename this to key_agg_coeff_internal then?

[robot-dreams]

Yes, done!

[jonasnick]

concept ACK

[jonasnick]

Isn't it better to just return False (similar to BIP-340)? In general there are a few assertions in this code (keyagg, partial sig agg) that would be triggerable by an attacker. I think that ideally we would assert only for function contracts or negligible edge cases. But we can do that later.

[robot-dreams]

Yes, I agree, though let's fix this in a future PR since there are a lot of assert here.

[jonasnick]

What's the significance of version 3.7? Did anyone run test this with 3.7? :)

[robot-dreams]

The significance is that it was copied from the BIP340 spec :) I can just say Python 3

[jonasnick]

ACK f71c297

[jonasnick]

ACK a5b1aaa

[jonasnick]

ACK 9acc936

[real-or-random]

utACK be9becb

[jonasnick]

ACK c235e50

[real-or-random]

utACK c235e50