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

Allow the output of NonceAgg to be infinity #21

Merged
merged 2 commits into from Jun 21, 2022
Merged

Conversation

jonasnick
Copy link
Owner

@jonasnick jonasnick commented May 24, 2022

Fixes #20 and #14

Copy link
Collaborator

@robot-dreams robot-dreams left a comment

Choose a reason for hiding this comment

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

Concept ACK

It might be worth making a bigger deal about the modified serialization format that can handle infinity.

bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2/reference.py Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2/reference.py Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
Copy link
Collaborator

@robot-dreams robot-dreams left a comment

Choose a reason for hiding this comment

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

ACK e2bc4e1

bip-musig2.mediawiki Show resolved Hide resolved
@jonasnick
Copy link
Owner Author

jonasnick commented Jun 9, 2022

Rebased (which required separating the parts of the inf_aggnonce_test and moving them into the existing tests) and added commit to fix #14.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

Here are a few nits.

Ensuring associativity in NonceAgg is certainly an improvement. But I'd like to think about this exact approach a little bit more before ACKing this. When I proposed this, what I had in mind is that b will still ensure that the final nonce cannot be infinity but that's obviously wrong...

bip-musig2/reference.py Outdated Show resolved Hide resolved
bip-musig2.mediawiki Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
bip-musig2.mediawiki Outdated Show resolved Hide resolved
@real-or-random
Copy link
Collaborator

real-or-random commented Jun 10, 2022

ACK on the commit addressing the nits

Ensuring associativity in NonceAgg is certainly an improvement. But I'd like to think about this exact approach a little bit more before ACKing this. When I proposed this, what I had in mind is that b will still ensure that the final nonce cannot be infinity but that's obviously wrong...

So the PR currently switches to a variant of the scheme where the final nonce R can end up being infinity (=0). This is a little bit strange because it's no longer true that "if all partial sigs verify, then the final sig verifies". This property won't hold if R=0. It's only possible to produce all partial sigs if all signers are malicious but this may still be a relevant case (e.g., the aggregator or an auditor could be honest).

There's another possible variant, where we replace R=0 by R=G (but only as the very last step in signing, not during Nonce Aggregation).

Pros:

  • We keep the property "if all partial sigs verify, then the final sig verifies".
  • We don't need bytes_extended.
  • No need to fail in ''PartialSigAgg'' if R=0 (though strictly speaking, there's also no need to do this with the current variant).

Cons:

  • We need another if in signing. But failing to implement this if will not result in forgeability, and it will be detectable by the test vectors...
  • We need to keep (part of) the explanation why this is okay for unforgeability. The reason here again is that we don't trust the aggregator, and an aggregator that sends an aggregated nonce (pair) (R1, R2) that results in R=0 could instead send (R1+G, R2) which will result in R=G.

I slightly lean towards this variant. It will be nice to hear your opinions on this.

By the way, I'm not exactly sure how either of these variants interact with adaptor sigs. I think both are fine but it would be nice to hear your comments.

@jonasnick
Copy link
Owner Author

jonasnick commented Jun 10, 2022

Pro:

  • We do write if ''PartialSigVerify'' succeeds for all partial signatures then ''PartialSigAgg'' will return a valid Schnorr signature right now. So we wouldn't have to add a paragraph explaining that this only holds if also "aggnonce != bytes(66, 0)" or "there's an honest signer" (*).
  • It's possible to imagine the following adaptor signature setup: there's a non-signer Charlie that obtains a partial signature and an adaptor signature. Charlie verifies the partial signature and the adaptor signature thinking that with the adaptor secret he can extract a partial signature from the adaptor signature. This would allow him to add the partial signatures to arrive at a final Schnorr signature. However, this does not work if the final nonce is the point at infinity and therefore Charlie needs to check for this. It seems hard to communicate that this check is necessary (only for a third-party) in the adaptor signature docs (*).

Con:

  • This is another hack that could bite us back later.

(*) If non-signer Charlie would forget the "aggnonce != bytes(66,0)" check but has valid partial signatures with final nonce equal to infinity, he can still proceed! That's because Charlie knows the secret nonce of the signature (0), can extract the secret key from the sum of the partial signatures and create a Schnorr signature himself. But I suppose we don't want to create an API for this.

@real-or-random
Copy link
Collaborator

Charlie knows the secret nonce of the signature (0), can extract the secret key

Lol, in principle we could also keep R=0, and let PartialSigAgg extract the secret key and create a signature with it. This also fulfills "if all partial sigs verify, then the final sig verifies".

@jonasnick
Copy link
Owner Author

To help imagine the consequences, I added a commit that implements the R=G variant in the reference code.

@jonasnick
Copy link
Owner Author

Converted the whole PR to the R=G variant.

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK mod nits

Comment on lines 539 to 541
Given a successful adversary against the security game (EUF-CMA) for the modified scheme, a reduction can win the security game for the original scheme by simulating the modification towards the adversary.
When the adversary provides ''aggnonce' '' such that ''R' '' in ''GetSessionValues'' is the point at infinity, the reduction sets ''aggnonce = cbytes_extended(G) || bytes(33, 0)''.
For any other ''aggnonce' '', the reduction sets ''aggnonce = aggnonce' ''.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the reduction would be clearer if it turned aggnonce' = bytes(33, 0) || bytes(33, 0) into aggnonce = cbytes_extended(G) || bytes(33, 0). This is equivalent to your proposal because the only way to reach R'=0 is aggnonce' = bytes(33, 0) || bytes(33, 0) (except with negligible probability) but it's more direct. Suggestion:

Suggested change
Given a successful adversary against the security game (EUF-CMA) for the modified scheme, a reduction can win the security game for the original scheme by simulating the modification towards the adversary.
When the adversary provides ''aggnonce' '' such that ''R' '' in ''GetSessionValues'' is the point at infinity, the reduction sets ''aggnonce = cbytes_extended(G) || bytes(33, 0)''.
For any other ''aggnonce' '', the reduction sets ''aggnonce = aggnonce' ''.
Given a successful adversary against the unforgeability game (EUF-CMA) for the modified scheme, a reduction can win the unforgeability game for the original scheme by simulating the modification towards the adversary:
When the adversary provides ''aggnonce' = bytes(33, 0) || bytes(33, 0)'', the reduction sets ''aggnonce = cbytes_extended(G) || bytes(33, 0)''.
For any other ''aggnonce' '', the reduction sets ''aggnonce = aggnonce' ''.
(The case that the adversary provides an ''aggnonce' ≠ bytes(33, 0) || bytes(33, 0) '' but nevertheless ''R' '' in ''GetSessionValues'' is the point at infinity happens only with negligible probability.)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point. Done

The <code>MINOR</code> version is incremented whenever the inputs or the output of an algorithm changes in a backward-compatible way or new backward-compatible functionality is added.
The <code>PATCH</code> version is incremented for other changes that are noteworthy (bug fixes, test vectors, important clarifications, etc.).

* '''0.4.0''' (2022-06-09): Allow the output of NonceAgg to be infinity and add test vectors
Copy link
Collaborator

Choose a reason for hiding this comment

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

(maybe update this)

Copy link
Owner Author

Choose a reason for hiding this comment

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

ok

If it happens that ''is_infinite(R'<sub>i</sub>)'' inside ''[[#NonceAgg infinity|NonceAgg]]'' there is at least one dishonest signer (except with negligible probability).
If we fail here, we will never be able to determine who it is.
Therefore, we continue so that the culprit is revealed when collecting and verifying partial signatures.
If it happens that ''is_infinite(R')'' inside ''[[#NonceAgg infinity|GetSessionValues]]'', either the nonce aggregator is dishonest or there is at least one dishonest signer (except with negligible probability).
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the nonce aggregator is dishonest, then strictly speaking, it doesn't make sense to talk about it running the honest "GetSessionValues" algorithm, so I think it's better to drop the sentence.

Maybe a better way is to rephrase it using the aggnonce:

Suggested change
If it happens that ''is_infinite(R')'' inside ''[[#NonceAgg infinity|GetSessionValues]]'', either the nonce aggregator is dishonest or there is at least one dishonest signer (except with negligible probability).
If it happens ''aggnonce = bytes(33,0) || bytes(33,0)'' for the aggregate nonce returned by the signature aggregator, either the nonce aggregator is dishonest or there is at least one dishonest signer (except with negligible probability).

Copy link
Owner Author

@jonasnick jonasnick Jun 20, 2022

Choose a reason for hiding this comment

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

I applied your suggestion (after slightly rephrasing it). Do you still propose to drop a sentence?

Comment on lines 530 to 531
If the signer aborted in this case, it would be impossible to determine who is dishonest.
Therefore, the signer continues so that the culprit is revealed when collecting and verifying partial signatures.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
If the signer aborted in this case, it would be impossible to determine who is dishonest.
Therefore, the signer continues so that the culprit is revealed when collecting and verifying partial signatures.
If the signing aborted in this case, it would be impossible to determine who is dishonest.
Therefore, the signing continues so that the culprit is revealed when collecting and verifying partial signatures.

or could also say "the signers". I just thought that "the signer" is a little bit misleading. (Which signer?)

Copy link
Owner Author

Choose a reason for hiding this comment

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

ok

Comment on lines 533 to 535
However, the final nonce ''R'' of a BIP340 Schnorr signature can not be the point at infinity.
If this specification would nonetheless allow the final nonce to be the point at infinity, then the scheme would lose the following property:
if ''PartialSigVerify'' succeeds for all partial signatures then ''PartialSigAgg'' will return a valid Schnorr signature.
Copy link
Collaborator

Choose a reason for hiding this comment

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

grammar nits:

Suggested change
However, the final nonce ''R'' of a BIP340 Schnorr signature can not be the point at infinity.
If this specification would nonetheless allow the final nonce to be the point at infinity, then the scheme would lose the following property:
if ''PartialSigVerify'' succeeds for all partial signatures then ''PartialSigAgg'' will return a valid Schnorr signature.
However, the final nonce ''R'' of a BIP340 Schnorr signature cannot be the point at infinity.
If this specification would nonetheless allow the final nonce to be the point at infinity, then the scheme would lose the following property:
if ''PartialSigVerify'' succeeds for all partial signatures, then ''PartialSigAgg'' will return a valid Schnorr signature.

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

* Let ''R<sub>1</sub> = cpoint_extended(aggnonce[0:33]), R<sub>2</sub> = cpoint_extended(aggnonce[33:66])''; fail if that fails
* Let ''R' = R<sub>1</sub> + b⋅R<sub>2</sub>''
* If ''is_infinite(R'):
** <div id="NonceAgg infinity"></div>Let ''R = G'' (see [[#dealing-with-infinity-in-nonce-aggregation|Dealing with Infinity in Nonce Aggregation]])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
** <div id="NonceAgg infinity"></div>Let ''R = G'' (see [[#dealing-with-infinity-in-nonce-aggregation|Dealing with Infinity in Nonce Aggregation]])
** Let ''R = G'' (see [[#dealing-with-infinity-in-nonce-aggregation|Dealing with Infinity in Nonce Aggregation]])

(could drop this div in case you accept the suggestion below)

Copy link
Owner Author

Choose a reason for hiding this comment

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

yeah

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK 20ba031

@real-or-random real-or-random merged commit 59e7f94 into musig2 Jun 21, 2022
@real-or-random real-or-random deleted the inf_aggnonce branch June 21, 2022 11:20
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.

Deal with Infinity in NonceAgg by Outputting Point at Infinity
3 participants