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

Resolve #84 #98

Merged
merged 8 commits into from
Feb 27, 2024
Merged

Resolve #84 #98

merged 8 commits into from
Feb 27, 2024

Conversation

skaunov
Copy link
Collaborator

@skaunov skaunov commented Feb 23, 2024

That was a larger one. 😅
Among other it

  • bumps non-breaking versions,
  • finalizes documentation including the example,
  • improves test(s),
  • adds signing capability.

use rand_core::CryptoRng;
use signature::RandomizedSigner;

const message: &[u8; 29] = b"An example app message string";
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a note in docs or something that a protocol should set this message to be unique, and unique per nullifier? I.e. a protocol might put the name of the question being voted on here along with their protocol name for a ZK vote.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good one! What does message reuse brings? Is it only replay or would it let signing other messages without having the secret; or maybe anything else?

Copy link
Member

Choose a reason for hiding this comment

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

No replay but you can deanonymize someones nullifier from the other place they posted and correlate that their identity must overlap with the other groups nullifier anonymity set.

Copy link
Collaborator Author

@skaunov skaunov Feb 23, 2024

Choose a reason for hiding this comment

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

Why the point $r$ isn't included into the hash then? It would make each signature unique and move this foot-gun away from those employing the protocol... 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Because then the nullifier wouldn't be deterministic! We need that property. We can potentially set a default in smart contracts to add the blickhash as part of the message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I totally trust you just trying to understand.

Deterministic as "to know c beforehand of signing"; or is it just the fact that V2 wouldn't work since it lacks the point $r$ value? Or something else?

I mean I kind of don't see yet the essential difference if signer adds to the message point $r$ instead of the info/data you mentioned. 🤔

}
}
impl<'signing> RandomizedSigner<PlumeSignature> for PlumeSigner<'signing> {
fn try_sign_with_rng(
Copy link
Member

@Divide-By-0 Divide-By-0 Feb 23, 2024

Choose a reason for hiding this comment

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

Is there a reason the user has to pass in the RNG? I wonder if it would help ease of implementation to have a version in which the RNG is determined inside of the function itself? Maybe I'm not as familiar with how RNG is treated in rust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This particular part is conforming to RustCrypto, outside of the module I preserved choice of the CRNG as they're sensitive thing, and it's a good practice to offer downstream to use whatever they see fit (according to the trait bounds).

On the other hand if you feel like simplifying it further I see no problem to just hard-code the system CRNG like the example shows inputting it. I guess downstream wouldn't have the control for the thread in which that will be running. But that path/choice is mainly for no-std (embedded) environment where instantiating the CRNG is custom to the platform. Anyway downstream would be able to sign with the PlumeSigner which offers the choice.

Copy link
Member

@Divide-By-0 Divide-By-0 Feb 23, 2024

Choose a reason for hiding this comment

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

What if we exposd a fn for signing with no RNG argument that filled in a preset RNG as a call to the existing fn, kind of like a default arg?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's exactly what I'm talking about. Just initializing the system RNG in sign_v1 leaving PlumeSigner for those who need detailed control.

Copy link
Member

Choose a reason for hiding this comment

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

For sure!

@Divide-By-0
Copy link
Member

Divide-By-0 commented Feb 23, 2024

Is there some confirmation that this signature function corresponds exactly to the compute all inputs function in JavaScript?

@skaunov
Copy link
Collaborator Author

skaunov commented Feb 23, 2024

Is there some confirmation that this signature function corresponds exactly to the compute all inputs function in JavaScript?

Only the fact that I have checked manually. X) Basically the values are taken from there. But everybody are invited to have another look; it's always better.

@Divide-By-0 Divide-By-0 merged commit 57d5b39 into 67 Feb 27, 2024
1 check passed
skaunov added a commit that referenced this pull request Feb 28, 2024
* current progress

* current progress

* current progress

* current progress

* current progress

* semantically finished

* `fmt`

* that's for another issue and is more complex
@skaunov skaunov deleted the 84 branch November 9, 2024 03:57
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.

2 participants