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

Pedersen cleanup #3029

Closed
kevaundray opened this issue Oct 25, 2023 · 8 comments
Closed

Pedersen cleanup #3029

kevaundray opened this issue Oct 25, 2023 · 8 comments
Labels
C-barretenberg Component: barretenberg cryptography library

Comments

@kevaundray
Copy link
Contributor

Problem

Now that Pedersen has been merged, the codebase is a lot cleaner however we still have a bit of tech debt related to the old pedersen implementations.

This is a tracking issue to note down all of these tech debt tasks

@kevaundray kevaundray added the C-barretenberg Component: barretenberg cryptography library label Oct 25, 2023
@github-project-automation github-project-automation bot moved this to Todo in A3 Oct 25, 2023
@kevaundray
Copy link
Contributor Author

kevaundray commented Oct 25, 2023

The first is use of compress and the plookup prefixed function names

#3033 removes plookup from the namespace
#3038 removes compress from the interface

@kevaundray
Copy link
Contributor Author

We also have c_bind and c_bind_new -- since the generator runs on c_bind_new, we can get rid of c_bind and parties which rely on c_bind should switch to that calling convention

@kevaundray
Copy link
Contributor Author

General investigation of hash/commit methods which are not used

@kevaundray
Copy link
Contributor Author

kevaundray commented Oct 25, 2023

We have a pattern of exposing hash methods which can be implemented by other exposed hash methods. One example is plookupCompress and PlookupCompressWithHashIndex, where the former is the latter with an index of 0

#3038 wittles down the interface to only have HashWithHashIndex and Hash

@kevaundray
Copy link
Contributor Author

kevaundray commented Oct 25, 2023

This can be a separate issue.

Investigate implementation of hash_buffer:

template <typename Curve>
typename Curve::BaseField pedersen_hash_base<Curve>::hash_buffer(const std::vector<uint8_t>& input,
                                                                 const GeneratorContext context)
{
    std::vector<Fq> converted = convert_buffer(input);

    if (converted.size() < 2) {
        return hash(converted, context);
    }
    auto result = hash({ converted[0], converted[1] }, context);
    for (size_t i = 2; i < converted.size(); ++i) {
        result = hash({ result, converted[i] }, context);
    }
    return result;
}

Intuitively, I would have expected it to convert a buffer into fields, then call hash with those fields and the context.

EDIT: this was is actually better because we re-use the same generators, instead of having N different generators

@kevaundray
Copy link
Contributor Author

check for stale comments/TODOs in some of the pedersen files, the most obvious would be to grep for delete this file

@kevaundray
Copy link
Contributor Author

Investigate whether we should get rid of pedersen__init -- it currently does nothing in the cbinds

kevaundray added a commit that referenced this issue Oct 26, 2023
Related to #3029 

We can merge this into `kw/pedersen-cleanup-remove-unused-cbinds` as
part of the cleanup

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
kevaundray added a commit that referenced this issue Oct 26, 2023
Related to #3029 

This removes all of the unused c_binds. The typescript code has been
modified to not use any of these, so at this point its not a breaking
change for yarn-packages. The bb.js codegen were using these inside of
tests that I added to make sure that they were computing the same thing,
these tests have been removed.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).

---------

Co-authored-by: Leila Wang <leizciw@gmail.com>
kevaundray added a commit that referenced this issue Oct 26, 2023
…an Fr element (#3072)

Commit history was a bit messed up in #3071 

Related to #3029 
# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
kevaundray added a commit that referenced this issue Oct 26, 2023
This method is no longer being used, we can bring it back if it is
deemed necessary.

Related to #3029 

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
kevaundray added a commit that referenced this issue Oct 26, 2023
…and ts (#3074)

This removes all mention of the phrase compress in relation to pedersen,
execpt for one place because the code has been commented out.

I think we should likely delete that file and re-add tests. File is
`barretenberg/cpp/src/barretenberg/stdlib/commitment/pedersen/pedersen.test.cpp`

Related to #3029 


# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
kevaundray added a commit that referenced this issue Oct 26, 2023
When we merged the mega PR, we commented out the stdlib pedersen
commitment code. This was okay for the most part because we had tests
for pedersen hashing tests. This adds back smoke pedersen commitment
tests.

Also happy to just remove the file, if folks want.

Related to #3029 

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
@kevaundray
Copy link
Contributor Author

Marking this as completed, #3076 and #3077 have been spun out into their own issues for further discussion

@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Oct 26, 2023
kevaundray added a commit that referenced this issue Oct 27, 2023
Related to #3029 and resolves #3077 .

The generator context is initializing and extending generator data which
outlives the lifetime of the function (it stays alive for the lifetime
of wasm instance). There may be an argument in the future to add a
method that will arbitrarily extend the generators, so that they do not
get extended in the middle of a pedersen call, if this is wanted we can
open up a separate issue.

# Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if
the PR is ready to merge.
- [ ] If the pull request requires a cryptography review (e.g.
cryptographic algorithm implementations) I have added the 'crypto' tag.
- [ ] I have reviewed my diff in github, line by line and removed
unexpected formatting changes, testing logs, or commented-out code.
- [ ] Every change is related to the PR description.
- [ ] I have
[linked](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
this pull request to relevant issues (if any exist).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-barretenberg Component: barretenberg cryptography library
Projects
Archived in project
Development

No branches or pull requests

1 participant