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.

Sign up for GitHub

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

Jump to bottom

refactor: plain struct flavor entities #3277

Merged
merged 83 commits into from
Nov 13, 2023
Merged

refactor: plain struct flavor entities #3277

merged 83 commits into from
Nov 13, 2023

Conversation

ludamad
Copy link
Collaborator

@ludamad ludamad commented Nov 9, 2023 •
edited
Loading

Remove array backing, propagating necessary changes. While we do use structs as arrays, it's simpler conceptually to model this as just another getter view instead of coupling strongly to an array storage model. This gives more flexibility to how these are defined, and give less gotchas via reference copying.

This requires more definition of pointer_view(). Note that the main mechanism for ensuring pointer_view() is correct is checking that the number of listed members is the one expected (e.g. equals NUM_ALL_ENTITIES).

Review notes:

  • A bunch of loops had to rethought as they previously did random access. Random access is possible in a sense, but only after constructing a pointer_view which, while cheap, is not free. As a result there's been some refactoring
  • zip_view has been used where possible, especially to get around the awkwardness of first creating the pointer_view and then using it as a structure to index into

@ludamad0
refactor(flavor): try to remove operator[]
50a0aba
@ludamad0
REVERTME comment out all array usage of polynomials
a96aa39
@ludamad0
Merge remote-tracking branch 'origin/master' into ad/chore/flavor-ref…
061ccd2 
…actor
@ludamad0
try to remove operator[]
844c310
@ludamad0
try to remove operator[]
a02ca70
@ludamad0
try to remove operator[]
264dc18
@ludamad0
try to remove operator[]
2444a59
@ludamad0
try to remove operator[]
6ff2adb
@ludamad0
Start of implementing
ad99141
@ludamad0
Start of implementing
e57c811
@ludamad0
Start of implementing
82bae80
@ludamad0
Start of implementing
acf9c75
@ludamad0
Start of implementing
fd819a8
@ludamad0
Start of implementing
6616189
@ludamad0
more implementation
9815845
@ludamad0
refactor: finish extend_edges
0831ddf
@ludamad0
refactor: finish extend_edges
cd10687
@ludamad0
comment
414eb9a
@ludamad0
more implementation
488a23f
@ludamad0
more implementation
4d5ac93
@ludamad0
more implementation
51690f8
@ludamad0
fix
2c10140
@ludamad0
add zip_view utility
012cb5f
@ludamad0
renaming
df2f1ff
@ludamad0
more addressing TODOs
12f72fe
@ludamad0
more addressing TODOs
5649093
@ludamad0
more addressing TODOs
82aa856
@ludamad0
more addressing TODOs
ab62aa1
@ludamad0
more addressing TODOs
c0bdb36
@ludamad0
more addressing TODOs
a6346ac
@ludamad ludamad marked this pull request as ready for review November 10, 2023 14:40
Base automatically changed from ad/chore/flavor-refactor to master November 10, 2023 16:31
@ludamad ludamad mentioned this pull request Nov 12, 2023
Closed
@codygunton codygunton self-requested a review November 12, 2023 09:57
@ludamad ludamad changed the title refactor: towards plain struct flavor entities part 2 refactor: plain struct flavor entities Nov 12, 2023
ledwards2225
ledwards2225 approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

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

LGTM. Glad the references are going away. I'm definitely conscious of how verbose these flavors are getting but I'm sure you're sick of thinking about it for the time being. Something worth addressing (by someone) in the near future I think.

barretenberg/cpp/src/barretenberg/flavor/goblin_ultra.hpp Outdated
@@ -425,7 +448,7 @@ class GoblinUltra {
public:
using Base = AllEntities<FF, FF>;
using Base::Base;
AllValues(std::array<FF, NUM_ALL_ENTITIES> _data_in) { this->_data = _data_in; }
// AllValues(std::array<FF, NUM_ALL_ENTITIES> _data_in) { this->_data = _data_in; }
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

@ludamad
Copy link
Collaborator Author

ludamad commented Nov 13, 2023

Yeah, we need to make progress from here, but the array-backing made it awkward to refactor these much. I chose to increase verbosity and help semantics. I still think the ultimate verbosity killer might be a small custom format that generates the code here.

@ludamad ludamad enabled auto-merge (squash) November 13, 2023 12:48
@ludamad ludamad merged commit f109512 into master Nov 13, 2023
76 of 78 checks passed
@ludamad ludamad deleted the ad/chore/flavor-refactor-part2 branch November 13, 2023 13:15
@AztecBot AztecBot mentioned this pull request Nov 12, 2023
Merged
ludamad pushed a commit that referenced this pull request Nov 17, 2023
@AztecBot
chore(master): Release 0.15.0 (#3269)
dd8ea9c 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.15.0</summary>

##
[0.15.0](aztec-packages-v0.14.2...aztec-packages-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))

### Features

* **bb:** Add msan preset
([#3284](#3284))
([bcf025c](bcf025c))
* Enable merge and root rollup circuits in noir
([#3248](#3248))
([68555fc](68555fc))
* Protogalaxy combiner quotient
([#3245](#3245))
([db0f3ab](db0f3ab))
* Public kernel in noir
([#3186](#3186))
([15a522b](15a522b))
* Ultra honk arith from ultra
([#3274](#3274))
([ec2b805](ec2b805))


### Bug Fixes

* Debug build
([#3283](#3283))
([aca2624](aca2624))
* Fix block constraint key divergence bug.
([#3256](#3256))
([1c71a0c](1c71a0c))
* Main.md typo
([#3278](#3278))
([cb87c4d](cb87c4d))
* Typo fix roundup
([#3302](#3302))
([9dd778d](9dd778d))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](#3318))
([4229173](4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](#3303))
([13f1a1d](13f1a1d))
* Clean up Plonk widgets
([#3305](#3305))
([4623d91](4623d91))
* **docs:** Aztec.nr logging page
([#3281](#3281))
([11e6ca7](11e6ca7))
* **docs:** Update netlify.toml and fix build
([#3304](#3304))
([df76636](df76636))
* Explicitly instantiate Goblin translator relations
([#3239](#3239))
([e3b5fb0](e3b5fb0))
* Plain struct flavor entities
([#3277](#3277))
([f109512](f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](#3330))
([23d1e2d](23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))
([87eeb71](87eeb71))
* Revert build-debug folder for debug preset
([#3324](#3324))
([43a2e6b](43a2e6b))
* Towards plain struct flavor entities
([#3216](#3216))
([3ba89cf](3ba89cf))
* Typo fixes based on cspell
([#3319](#3319))
([8ae44dd](8ae44dd))
</details>

<details><summary>barretenberg.js: 0.15.0</summary>

##
[0.15.0](barretenberg.js-v0.14.2...barretenberg.js-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))

### Bug Fixes

* Fix block constraint key divergence bug.
([#3256](#3256))
([1c71a0c](1c71a0c))


### Miscellaneous

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))
([87eeb71](87eeb71))
* Typo fixes based on cspell
([#3319](#3319))
([8ae44dd](8ae44dd))
</details>

<details><summary>barretenberg: 0.15.0</summary>

##
[0.15.0](barretenberg-v0.14.2...barretenberg-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))

### Features

* **bb:** Add msan preset
([#3284](#3284))
([bcf025c](bcf025c))
* Protogalaxy combiner quotient
([#3245](#3245))
([db0f3ab](db0f3ab))
* Ultra honk arith from ultra
([#3274](#3274))
([ec2b805](ec2b805))


### Bug Fixes

* Debug build
([#3283](#3283))
([aca2624](aca2624))
* Fix block constraint key divergence bug.
([#3256](#3256))
([1c71a0c](1c71a0c))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](#3318))
([4229173](4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](#3303))
([13f1a1d](13f1a1d))
* Clean up Plonk widgets
([#3305](#3305))
([4623d91](4623d91))
* Explicitly instantiate Goblin translator relations
([#3239](#3239))
([e3b5fb0](e3b5fb0))
* Plain struct flavor entities
([#3277](#3277))
([f109512](f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](#3330))
([23d1e2d](23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](#3114))
([87eeb71](87eeb71))
* Revert build-debug folder for debug preset
([#3324](#3324))
([43a2e6b](43a2e6b))
* Towards plain struct flavor entities
([#3216](#3216))
([3ba89cf](3ba89cf))
* Typo fixes based on cspell
([#3319](#3319))
([8ae44dd](8ae44dd))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
AztecBot added a commit to AztecProtocol/barretenberg that referenced this pull request Nov 19, 2023
@AztecBot
chore(master): Release 0.15.0 (#3269)
ffdc351 
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-packages: 0.15.0</summary>

##
[0.15.0](AztecProtocol/aztec-packages@aztec-packages-v0.14.2...aztec-packages-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))

### Features

* **bb:** Add msan preset
([#3284](AztecProtocol/aztec-packages#3284))
([bcf025c](AztecProtocol/aztec-packages@bcf025c))
* Enable merge and root rollup circuits in noir
([#3248](AztecProtocol/aztec-packages#3248))
([68555fc](AztecProtocol/aztec-packages@68555fc))
* Protogalaxy combiner quotient
([#3245](AztecProtocol/aztec-packages#3245))
([db0f3ab](AztecProtocol/aztec-packages@db0f3ab))
* Public kernel in noir
([#3186](AztecProtocol/aztec-packages#3186))
([15a522b](AztecProtocol/aztec-packages@15a522b))
* Ultra honk arith from ultra
([#3274](AztecProtocol/aztec-packages#3274))
([ec2b805](AztecProtocol/aztec-packages@ec2b805))


### Bug Fixes

* Debug build
([#3283](AztecProtocol/aztec-packages#3283))
([aca2624](AztecProtocol/aztec-packages@aca2624))
* Fix block constraint key divergence bug.
([#3256](AztecProtocol/aztec-packages#3256))
([1c71a0c](AztecProtocol/aztec-packages@1c71a0c))
* Main.md typo
([#3278](AztecProtocol/aztec-packages#3278))
([cb87c4d](AztecProtocol/aztec-packages@cb87c4d))
* Typo fix roundup
([#3302](AztecProtocol/aztec-packages#3302))
([9dd778d](AztecProtocol/aztec-packages@9dd778d))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](AztecProtocol/aztec-packages#3318))
([4229173](AztecProtocol/aztec-packages@4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](AztecProtocol/aztec-packages#3303))
([13f1a1d](AztecProtocol/aztec-packages@13f1a1d))
* Clean up Plonk widgets
([#3305](AztecProtocol/aztec-packages#3305))
([4623d91](AztecProtocol/aztec-packages@4623d91))
* **docs:** Aztec.nr logging page
([#3281](AztecProtocol/aztec-packages#3281))
([11e6ca7](AztecProtocol/aztec-packages@11e6ca7))
* **docs:** Update netlify.toml and fix build
([#3304](AztecProtocol/aztec-packages#3304))
([df76636](AztecProtocol/aztec-packages@df76636))
* Explicitly instantiate Goblin translator relations
([#3239](AztecProtocol/aztec-packages#3239))
([e3b5fb0](AztecProtocol/aztec-packages@e3b5fb0))
* Plain struct flavor entities
([#3277](AztecProtocol/aztec-packages#3277))
([f109512](AztecProtocol/aztec-packages@f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](AztecProtocol/aztec-packages#3330))
([23d1e2d](AztecProtocol/aztec-packages@23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))
([87eeb71](AztecProtocol/aztec-packages@87eeb71))
* Revert build-debug folder for debug preset
([#3324](AztecProtocol/aztec-packages#3324))
([43a2e6b](AztecProtocol/aztec-packages@43a2e6b))
* Towards plain struct flavor entities
([#3216](AztecProtocol/aztec-packages#3216))
([3ba89cf](AztecProtocol/aztec-packages@3ba89cf))
* Typo fixes based on cspell
([#3319](AztecProtocol/aztec-packages#3319))
([8ae44dd](AztecProtocol/aztec-packages@8ae44dd))
</details>

<details><summary>barretenberg.js: 0.15.0</summary>

##
[0.15.0](AztecProtocol/aztec-packages@barretenberg.js-v0.14.2...barretenberg.js-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))

### Bug Fixes

* Fix block constraint key divergence bug.
([#3256](AztecProtocol/aztec-packages#3256))
([1c71a0c](AztecProtocol/aztec-packages@1c71a0c))


### Miscellaneous

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))
([87eeb71](AztecProtocol/aztec-packages@87eeb71))
* Typo fixes based on cspell
([#3319](AztecProtocol/aztec-packages#3319))
([8ae44dd](AztecProtocol/aztec-packages@8ae44dd))
</details>

<details><summary>barretenberg: 0.15.0</summary>

##
[0.15.0](AztecProtocol/aztec-packages@barretenberg-v0.14.2...barretenberg-v0.15.0)
(2023-11-16)


### ⚠ BREAKING CHANGES

* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))

### Features

* **bb:** Add msan preset
([#3284](AztecProtocol/aztec-packages#3284))
([bcf025c](AztecProtocol/aztec-packages@bcf025c))
* Protogalaxy combiner quotient
([#3245](AztecProtocol/aztec-packages#3245))
([db0f3ab](AztecProtocol/aztec-packages@db0f3ab))
* Ultra honk arith from ultra
([#3274](AztecProtocol/aztec-packages#3274))
([ec2b805](AztecProtocol/aztec-packages@ec2b805))


### Bug Fixes

* Debug build
([#3283](AztecProtocol/aztec-packages#3283))
([aca2624](AztecProtocol/aztec-packages@aca2624))
* Fix block constraint key divergence bug.
([#3256](AztecProtocol/aztec-packages#3256))
([1c71a0c](AztecProtocol/aztec-packages@1c71a0c))


### Miscellaneous

* **bb:** Remove -Wfatal-errors
([#3318](AztecProtocol/aztec-packages#3318))
([4229173](AztecProtocol/aztec-packages@4229173))
* Clarify that barretenberg mirror should not take PRs
([#3303](AztecProtocol/aztec-packages#3303))
([13f1a1d](AztecProtocol/aztec-packages@13f1a1d))
* Clean up Plonk widgets
([#3305](AztecProtocol/aztec-packages#3305))
([4623d91](AztecProtocol/aztec-packages@4623d91))
* Explicitly instantiate Goblin translator relations
([#3239](AztecProtocol/aztec-packages#3239))
([e3b5fb0](AztecProtocol/aztec-packages@e3b5fb0))
* Plain struct flavor entities
([#3277](AztecProtocol/aztec-packages#3277))
([f109512](AztecProtocol/aztec-packages@f109512))
* Remove bn254 instantiation of eccvm plus naming changes
([#3330](AztecProtocol/aztec-packages#3330))
([23d1e2d](AztecProtocol/aztec-packages@23d1e2d))
* Replace computing hashes in circuits wasm, with computing them in ts
via bb.js pedersen call.
([#3114](AztecProtocol/aztec-packages#3114))
([87eeb71](AztecProtocol/aztec-packages@87eeb71))
* Revert build-debug folder for debug preset
([#3324](AztecProtocol/aztec-packages#3324))
([43a2e6b](AztecProtocol/aztec-packages@43a2e6b))
* Towards plain struct flavor entities
([#3216](AztecProtocol/aztec-packages#3216))
([3ba89cf](AztecProtocol/aztec-packages@3ba89cf))
* Typo fixes based on cspell
([#3319](AztecProtocol/aztec-packages#3319))
([8ae44dd](AztecProtocol/aztec-packages@8ae44dd))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ledwards2225 ledwards2225 ledwards2225 approved these changes

@codygunton codygunton Awaiting requested review from codygunton

Assignees

@ludamad ludamad

Labels
None yet
Projects
A3
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ludamad @AztecBot @ledwards2225 @ludamad0