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

feat!: Remove backend solvable methods from the interface and solve them in ACVM #264

Merged
merged 10 commits into from
May 18, 2023

Conversation

guipublic
Copy link
Contributor

Related issue(s)

(If it does not already exist, first create a GitHub issue that describes the problem this Pull Request (PR) solves before creating the PR and link it here.)

Resolves (link to issue)

Description

Summary of changes

Use ACVM instead of calling the backend for solving blackbox functions, for the bb functions which do not use barretenberg for solving (the low hanging fruits).

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

Once this PR is merged, I will remove the related solve traits from pwg so that backends do not need to implement these solvers.

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

Much happier with this compared to original proposal as now we've defined a proper interface. This PR is leaving a lot of dead code on the PWG trait however which needs to be cleaned up.

acvm/src/pwg/blackbox.rs Outdated Show resolved Hide resolved
acvm/src/pwg/blackbox.rs Outdated Show resolved Hide resolved
@TomAFrench TomAFrench mentioned this pull request May 8, 2023
5 tasks
@guipublic guipublic requested a review from TomAFrench May 12, 2023 14:43
@TomAFrench
Copy link
Member

This PR is still leaving dead code on the PartialWitnessGenerator trait.

@guipublic
Copy link
Contributor Author

Yes, the idea is to remove this in another PR when this one is merged, we will be able to remove corresponding solves in the backend safely since they will not be called after this PR is merged.

@phated
Copy link
Contributor

phated commented May 15, 2023

I think we could rename the PR to feat!: Solve blackbox functions in ACVM instead of backends and then remove the functions from the PWG trait in this same PR. It'd communicate to implementors accurately, as this chore would be excluded from the changelog currently.

@guipublic
Copy link
Contributor Author

Since we want to have small PRs, I think it makes sense to have this one which does not call the backend and implement the solve methods, but does not break anything (and so it is a chore), and then have another one which will be a feat! and will break the interface, but it will only do that. This way there is no confusion between implementing the solvers and modifying the PWG trait.

@TomAFrench
Copy link
Member

Agreed with renaming PR, I don't really see the logic in doing this in two steps. We should only have one implementation of the black box functions in use at any one time.

@phated
Copy link
Contributor

phated commented May 15, 2023

I don't think this PR is non-breaking. If a backend was relying on their implementation of backend.schnorr_verify but you switch it in this PR, then they are broken. If we remove the PWG functions in the same PR, it is very clear that there isn't 2 separate implementations.

@guipublic
Copy link
Contributor Author

If a backend was relying on their implementation of backend.schnorr_verify but you switch it in this PR, then they are broken

No they are not be broken if I switch-in this PR, the schnorr verification should work for both implementations, if not that means their implementation was already broken.

@TomAFrench
Copy link
Member

Another example would be HashToField128Security, if a backend were using a hash function other than blake2s then this would be breaking.

@guipublic guipublic requested a review from kevaundray May 17, 2023 07:59
@kevaundray
Copy link
Contributor

If a backend was not doing the correct behavior for functions like hash_to_field etc, then I think thats on them -- and us for not having a spec.

I'm impartial as to this PR being breaking, but we should not release a version without the accompanying breaking change which removes the duplicates.

@kevaundray kevaundray changed the title chore: use acvm to solve bb functions when possible chore: use ACVM to solve BlackBox functions when ACVM has an implementation of it May 17, 2023
@kevaundray kevaundray changed the title chore: use ACVM to solve BlackBox functions when ACVM has an implementation of it chore: Remove backend solvable methods from the interface and solve them in ACVM May 18, 2023
@kevaundray kevaundray changed the title chore: Remove backend solvable methods from the interface and solve them in ACVM feat!: Remove backend solvable methods from the interface and solve them in ACVM May 18, 2023
@kevaundray kevaundray added this pull request to the merge queue May 18, 2023
Merged via the queue into master with commit 69916cb May 18, 2023
@github-actions github-actions bot mentioned this pull request May 18, 2023
@TomAFrench TomAFrench deleted the gd/acvm_solve branch May 19, 2023 05:08
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.

4 participants