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

feat!: add block opcode #114

Merged
merged 19 commits into from
Feb 28, 2023
Merged

feat!: add block opcode #114

merged 19 commits into from
Feb 28, 2023

Conversation

guipublic
Copy link
Contributor

Description

Summary of changes

This PR adds an opcode which abstract memory operations as read and write to blocks.
It is required for the dynamic array feature because the solver is not able to solve dynamic reads except for simple cases.
The opcode uses the memory operation of a block to emulate the block value during solving in order to resolve the read operations.

Dependency additions / changes

Solving does not return an error anymore when it cannot solve a gate, the backend should be adapted to the new solve signature.

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

We will be able to use this new opcode for interfacing with barretenberg when it will expose dynamic memory.

@guipublic guipublic changed the title feat: add block opcode feat!: add block opcode Feb 22, 2023
@TomAFrench TomAFrench mentioned this pull request Feb 22, 2023
Merged
5 tasks
TomAFrench
TomAFrench reviewed Feb 22, 2023
View reviewed changes
acir/src/native_types/arithmetic.rs Outdated Show resolved Hide resolved
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
acvm/src/pwg/block.rs Show resolved Hide resolved
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
TomAFrench
TomAFrench reviewed Feb 22, 2023
View reviewed changes
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
jfecher
jfecher reviewed Feb 22, 2023
View reviewed changes
acir/src/circuit/opcodes.rs Outdated Show resolved Hide resolved
acir/src/native_types/arithmetic.rs Outdated Show resolved Hide resolved
acvm/src/lib.rs Outdated Show resolved Hide resolved
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
TomAFrench
TomAFrench previously requested changes Feb 23, 2023
View reviewed changes
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.

The addition of GateResolution seems unrelated to the purpose of this PR. I think it would be best to split off that change to be reviewed/merged separately.

kevaundray
kevaundray reviewed Feb 23, 2023
View reviewed changes
acvm/src/lib.rs Outdated Show resolved Hide resolved
kevaundray
kevaundray reviewed Feb 23, 2023
View reviewed changes
acvm/src/lib.rs Outdated Show resolved Hide resolved
kevaundray
kevaundray suggested changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

This PR is doing more than just adding a block opcode. These changes should be done in a separate PR and or an issue should be opened up for them.

I also left pending questions

@kevaundray kevaundray mentioned this pull request Feb 23, 2023
Closed
5 tasks
jfecher
jfecher reviewed Feb 24, 2023
View reviewed changes
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
TomAFrench
TomAFrench reviewed Feb 24, 2023
View reviewed changes
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
TomAFrench
TomAFrench reviewed Feb 24, 2023
View reviewed changes
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
TomAFrench
TomAFrench reviewed Feb 24, 2023
View reviewed changes
acir/src/circuit/opcodes.rs Outdated Show resolved Hide resolved
acvm/src/pwg/block.rs Outdated Show resolved Hide resolved
TomAFrench
TomAFrench reviewed Feb 24, 2023
View reviewed changes
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.

Should be gtg after this.

acir/src/circuit/opcodes.rs Show resolved Hide resolved
guipublic and others added 3 commits February 27, 2023 09:55
@guipublic @TomAFrench
Code review
dc1f418 
Update acir/src/circuit/opcodes.rs

Co-authored-by: Tom French <[email protected]>
@guipublic @TomAFrench
Code review
aff783d 
Update acvm/src/pwg/block.rs

Co-authored-by: Tom French <[email protected]>
@guipublic
fix clippy
1b73484
@TomAFrench TomAFrench dismissed their stale review February 28, 2023 13:05

issues addressed

kevaundray
kevaundray reviewed Feb 28, 2023
View reviewed changes
acvm/src/pwg.rs Outdated Show resolved Hide resolved
kevaundray
kevaundray reviewed Feb 28, 2023
View reviewed changes
acvm/src/pwg.rs Outdated Show resolved Hide resolved
kevaundray
kevaundray approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@kevaundray kevaundray left a comment

Choose a reason for hiding this comment

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

LGTM

@kevaundray kevaundray added this pull request to the merge queue Feb 28, 2023
Merged via the queue into master with commit 097cfb0 Feb 28, 2023
@github-actions github-actions bot mentioned this pull request Feb 28, 2023
Merged
@guipublic guipublic deleted the gd/acvm_block branch February 28, 2023 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomAFrench TomAFrench TomAFrench left review comments

@jfecher jfecher jfecher approved these changes

@kevaundray kevaundray kevaundray approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@guipublic @jfecher @TomAFrench @kevaundray