Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

feat!: added hash index to pedersen #281

Merged
merged 5 commits into from
May 30, 2023
Merged

Conversation

sirasistant
Copy link
Contributor

@sirasistant sirasistant commented May 11, 2023

Related issue(s)

Related issue: noir-lang/noir#1341

Description

Adds the hash index constant parameter to pedersen bb func call.

Summary of changes

(Describe the changes in this PR. Point out breaking changes if any.)

Dependency additions / changes

(If applicable.)

Test additions / changes

(If applicable.)

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

(If applicable.)

@sirasistant sirasistant changed the title feat: added hash index to pedersen feat!: added hash index to pedersen May 11, 2023
Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

What this is doing?
Do we have a corresponding GitHub issue to motivate the change?

@TomAFrench
Copy link
Member

Same, I'm missing a lot of context around this change so it would be helpful to have a full issue on what this is trying to solve.

@sirasistant
Copy link
Contributor Author

Related issue: noir-lang/noir#1341

Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

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

This new field indicates which set of generators to use. The fact that we use it in Aztec for domain separation is not relevant in this context. A better name would be something like generators_set or generators_index.

acir/src/circuit/opcodes/black_box_function_call.rs Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor

This new field indicates which set of generators to use. The fact that we use it in Aztec for domain separation is not relevant in this context. A better name would be something like generators_set or generators_index.

Its main purpose is domain separation, so this name is fine

@kevaundray
Copy link
Contributor

@guipublic

The naming of generator_set vs domain_separation is also related to noir-lang/noir#4760 ;

If we had a homomorphic pedersen implementation, then generator_set would be the correct choice, but since we are using it as a HashToGroup, domain_separation works better here.

@guipublic
Copy link
Contributor

In any cases (homomorphic or not) it indicates which group elements to use, so generator_set is ok.
When the opcode will be renamed, then maybe another name will be better but we can (and should) change it at that time, not now.

@kevaundray
Copy link
Contributor

In any cases (homomorphic or not) it indicates which group elements to use, so generator_set is ok.
When the opcode will be renamed, then maybe another name will be better but we can (and should) change it at that time, not now.

generator_set is not a detail that matters here, what we need is domain separation and knowing what the generator_set is, does not help in anyway due to this function not being homomorphic right now.

Lets go with domain_separatorand we can come back to it, if it proves to not be sufficient.

@guipublic
Copy link
Contributor

generator_set is not a detail that matters here, what we need is domain separation

generator_set is a detail that does not matter for Aztec, what we need for Aztec is domain separation. When we will set this value coming from a noir contract, it will be called domain_index or domain_separator.

But at the acir level, domain_separator for a pedersen opcode is meaningless.

and knowing what the generator_set is, does not help in anyway due to this function not being homomorphic right now.

Knowing what the generators are allows someone to know what this opcode is doing, not what is his purpose, but what exact computation it is proving.

@sirasistant sirasistant requested a review from vezenovm May 29, 2023 14:53
@kevaundray kevaundray added this pull request to the merge queue May 30, 2023
Merged via the queue into master with commit 61820b6 May 30, 2023
@github-actions github-actions bot mentioned this pull request May 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants