Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change IsSecond to match the design section #164

Closed

Conversation

kallerosenbaum
Copy link
Contributor

The third bullet in the design section says that the second unique key gets coefficient 1, but the algorithm doesn't match that.

This change will make the algo description match the design section.

The third bullet in the design section says that the second unique key gets coefficient 1, but the algorithm doesn't match that.

This change will make the algo description match the design section.
@real-or-random
Copy link
Collaborator

It should be correct as it is. Say we have pk1 != pk2

Then if I'm not mistaken IsSecond([pk1, pk2], pk2) = true.

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, no the algorithm is correct. With your change, IsSecond(pk<sub>1..u</sub>, 1) will return true which is not what we intend.

@kallerosenbaum
Copy link
Contributor Author

Run KeyAggCoeff([77, 43], 2) // 1-based index
IsSecond([77,43],2) will return false and a_2 will become != 1
The result is that the second unique key will not get coefficient 1, which contradicts the Design section.

@kallerosenbaum
Copy link
Contributor Author

Oops, forget about this. IsSecond is correct.

@jonasnick
Copy link
Contributor

This part of the spec has already confused more people #157 (comment) and others. I'm wondering if we can write this down more clearly.

@real-or-random
Copy link
Collaborator

Maybe we can write a FindSecondUniqueKey which differs because

  • it will return a single pubkey and not work on indices (I think this is be clearer)
  • it's more natural because it iterates over they keys only once (this is how you'd implement it)

But I think the core of the problem is that the algorithm looks wrong at first glance. (I thought the same when I first saw it). But that's hard to fix. The algorithm itself is correct and as simple as it gets. Our brains here are wrong, not the spec. ;)

@real-or-random
Copy link
Collaborator

No matter how we write the algorithm, we could add a few input/output examples

@kallerosenbaum
Copy link
Contributor Author

One slightly related question:

If the keys are [44, 80, 80], then IsSecond([44, 80, 80], 3) would return true, right? This means that if the second unique key is repeated, all of them will get coefficient 1. Is that intentional? It seems intentional because the check compares actual keys, and not indices in "Return true if pkj = pki, otherwise return false"

@jonasnick
Copy link
Contributor

@kallerosenbaum Yes this is intentional and the variant that is proven correct in the appendix of the MuSig2 paper (we call this variant MuSig2*).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants