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

feat(avm): new public inputs witgen #10179

Merged
merged 3 commits into from
Nov 26, 2024

Conversation

IlyasRidhuan
Copy link
Contributor

@IlyasRidhuan IlyasRidhuan commented Nov 25, 2024

This PR does a few things

  1. Disconnects the kernel trace constraints (but keeps the file for future reference when we re-constrain public inputs)
  2. Replaces the old public inputs with the new ones from AvmCircuitPublicInputs, however unconstrained
  3. All merkle checks are now performed in witgen

It's still a bit brittle and probably needs a refactor to clean it up but will suffice for now as we are getting all the pieces together

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@IlyasRidhuan IlyasRidhuan force-pushed the ir/11-14-feat_avm_new_public_inputs_witgen branch 2 times, most recently from 4431f66 to d05ccb0 Compare November 25, 2024 17:17
@IlyasRidhuan IlyasRidhuan marked this pull request as ready for review November 25, 2024 17:28
@@ -16,6 +16,17 @@ include "gadgets/mem_slice.pil";
include "gadgets/merkle_tree.pil";

namespace main(256);
//===== PUBLIC INPUT POLYNOMIALS ======================================
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are moved over to just keep the existing public inputs structure and is not representative of the final version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Were these copied from another PIL file? Should they be removed from their original file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - the old pil file (kernel.pil) is commented out - it will eventually be deleted entirely, but ive kept it because we will need to reference / copy stuff from it when we constrain the new inputs

@@ -13,6 +13,7 @@ enum Poseidon2Caller {
NONE = 0,
BYTECODE_HASHING = 1,
MERKLE_TREE = 2,
SILO = 3,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These will be used when we constrain silo-ing in the circuit

@@ -12,7 +14,7 @@ class AvmMerkleTreeTraceBuilder {
struct MerkleEntry {
uint32_t clk;
FF leaf_value{};
uint32_t leaf_index;
uint64_t leaf_index;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2^40 trees at this stage require bigger indices

Copy link
Collaborator

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

Looks good! Handful of comments. I'd expect all the merkle checks in witgen to break for transactions that include side effects from private since (i think) you aren't inserting any of the stuff from private, so the start and end roots should be mismatched. How is this working?

@@ -16,6 +16,17 @@ include "gadgets/mem_slice.pil";
include "gadgets/merkle_tree.pil";

namespace main(256);
//===== PUBLIC INPUT POLYNOMIALS ======================================
Copy link
Collaborator

Choose a reason for hiding this comment

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

Were these copied from another PIL file? Should they be removed from their original file?

Comment on lines 78 to 84
return AvmExecutionTests::gen_trace(bytecode, calldata, public_inputs_vec, returndata, execution_hints);
auto p_i = public_inputs;
return AvmExecutionTests::gen_trace(bytecode, calldata, p_i, returndata, execution_hints);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: no need to rename to p_i here?

barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp Outdated Show resolved Hide resolved
barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp Outdated Show resolved Hide resolved
barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp Outdated Show resolved Hide resolved
Comment on lines +264 to 274
// These are the tracked roots for intermediate steps
TreeSnapshots intermediate_tree_snapshots;
// These are some counters for the tree acceess hints that we probably dont need in the future
uint32_t note_hash_read_counter = 0;
uint32_t note_hash_write_counter = 0;
uint32_t nullifier_read_counter = 0;
uint32_t nullifier_write_counter = 0;
uint32_t l1_to_l2_msg_read_counter = 0;
uint32_t storage_read_counter = 0;
uint32_t storage_write_counter = 0;

Copy link
Collaborator

Choose a reason for hiding this comment

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

We actually might need these in the future because we need to raise error flag if these surpass some limits.

@IlyasRidhuan IlyasRidhuan force-pushed the ir/11-14-feat_avm_new_public_inputs_witgen branch from d05ccb0 to 7bbeeee Compare November 26, 2024 12:30
@IlyasRidhuan IlyasRidhuan force-pushed the ir/11-14-feat_avm_new_public_inputs_witgen branch from 7bbeeee to dcdb3f0 Compare November 26, 2024 15:25
@@ -10,7 +10,7 @@ services:
exec anvil --silent -p "$$ANVIL_PORT" --host 0.0.0.0 --chain-id 31337
fi'
ports:
- "${ANVIL_PORT:-8545}:${ANVIL_PORT:-8545}"
- '${ANVIL_PORT:-8545}:${ANVIL_PORT:-8545}'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some leftover formatting from master i think

@IlyasRidhuan IlyasRidhuan requested a review from jeanmon November 26, 2024 15:26
@IlyasRidhuan IlyasRidhuan merged commit ac8f13e into master Nov 26, 2024
74 checks passed
@IlyasRidhuan IlyasRidhuan deleted the ir/11-14-feat_avm_new_public_inputs_witgen branch November 26, 2024 17:32
critesjosh pushed a commit that referenced this pull request Nov 26, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.65.0</summary>

##
[0.65.0](aztec-package-v0.64.0...aztec-package-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](#10179))
([ac8f13e](ac8f13e))
</details>

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

##
[0.65.0](barretenberg.js-v0.64.0...barretenberg.js-v0.65.0)
(2024-11-26)


### Bug Fixes

* **bb.js:** Don't minify bb.js - webpack config
([#10170](#10170))
([6e7fae7](6e7fae7))
</details>

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

##
[0.65.0](aztec-packages-v0.64.0...aztec-packages-v0.65.0)
(2024-11-26)


### ⚠ BREAKING CHANGES

* remove SharedImmutable
([#10183](#10183))
* rename sharedimmutable methods
([#10164](#10164))

### Features

* **avm:** New public inputs witgen
([#10179](#10179))
([ac8f13e](ac8f13e))
* Blobs.
([#9302](#9302))
([03b7e0e](03b7e0e))
* One liner for nodes to join rough-rhino
([#10168](#10168))
([3a425e9](3a425e9))
* Origin tags implemented in biggroup
([#10002](#10002))
([c8696b1](c8696b1))
* Remove SharedImmutable
([#10183](#10183))
([a9f3b5f](a9f3b5f))
* Rename sharedimmutable methods
([#10164](#10164))
([ef7cd86](ef7cd86))
* UltraRollupRecursiveFlavor
([#10088](#10088))
([4418ef2](4418ef2))


### Bug Fixes

* Aztec-nargo curl in the earthfile also
([#10199](#10199))
([985a678](985a678))
* **bb.js:** Don't minify bb.js - webpack config
([#10170](#10170))
([6e7fae7](6e7fae7))
* Docker compose aztec up fix
([#10197](#10197))
([d7ae959](d7ae959))
* Increase test timeouts
([#10205](#10205))
([195aa3d](195aa3d))
* Release l1-contracts
([#10095](#10095))
([29f0d7a](29f0d7a))
* Revert "feat: blobs.
([#9302](#9302))"
([#10187](#10187))
([a415f65](a415f65))


### Miscellaneous

* Added ref to env variables
([#10193](#10193))
([b51fc43](b51fc43))
* **avm:** Operands reordering
([#10182](#10182))
([69bdf4f](69bdf4f)),
closes
[#10136](#10136)
* Fix devbox
([#10201](#10201))
([323eaee](323eaee))
* Misc cleanup
([#10194](#10194))
([dd01417](dd01417))
* Reinstate docs-preview, fix doc publish
([#10213](#10213))
([ed9a0e3](ed9a0e3))
* Replace relative paths to noir-protocol-circuits
([1650446](1650446))
</details>

<details><summary>barretenberg: 0.65.0</summary>

##
[0.65.0](barretenberg-v0.64.0...barretenberg-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](#10179))
([ac8f13e](ac8f13e))
* Origin tags implemented in biggroup
([#10002](#10002))
([c8696b1](c8696b1))
* UltraRollupRecursiveFlavor
([#10088](#10088))
([4418ef2](4418ef2))


### Miscellaneous

* **avm:** Operands reordering
([#10182](#10182))
([69bdf4f](69bdf4f)),
closes
[#10136](#10136)
</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 27, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>aztec-package: 0.65.0</summary>

##
[0.65.0](AztecProtocol/aztec-packages@aztec-package-v0.64.0...aztec-package-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](AztecProtocol/aztec-packages#10179))
([ac8f13e](AztecProtocol/aztec-packages@ac8f13e))
</details>

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

##
[0.65.0](AztecProtocol/aztec-packages@barretenberg.js-v0.64.0...barretenberg.js-v0.65.0)
(2024-11-26)


### Bug Fixes

* **bb.js:** Don't minify bb.js - webpack config
([#10170](AztecProtocol/aztec-packages#10170))
([6e7fae7](AztecProtocol/aztec-packages@6e7fae7))
</details>

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

##
[0.65.0](AztecProtocol/aztec-packages@aztec-packages-v0.64.0...aztec-packages-v0.65.0)
(2024-11-26)


### ⚠ BREAKING CHANGES

* remove SharedImmutable
([#10183](AztecProtocol/aztec-packages#10183))
* rename sharedimmutable methods
([#10164](AztecProtocol/aztec-packages#10164))

### Features

* **avm:** New public inputs witgen
([#10179](AztecProtocol/aztec-packages#10179))
([ac8f13e](AztecProtocol/aztec-packages@ac8f13e))
* Blobs.
([#9302](AztecProtocol/aztec-packages#9302))
([03b7e0e](AztecProtocol/aztec-packages@03b7e0e))
* One liner for nodes to join rough-rhino
([#10168](AztecProtocol/aztec-packages#10168))
([3a425e9](AztecProtocol/aztec-packages@3a425e9))
* Origin tags implemented in biggroup
([#10002](AztecProtocol/aztec-packages#10002))
([c8696b1](AztecProtocol/aztec-packages@c8696b1))
* Remove SharedImmutable
([#10183](AztecProtocol/aztec-packages#10183))
([a9f3b5f](AztecProtocol/aztec-packages@a9f3b5f))
* Rename sharedimmutable methods
([#10164](AztecProtocol/aztec-packages#10164))
([ef7cd86](AztecProtocol/aztec-packages@ef7cd86))
* UltraRollupRecursiveFlavor
([#10088](AztecProtocol/aztec-packages#10088))
([4418ef2](AztecProtocol/aztec-packages@4418ef2))


### Bug Fixes

* Aztec-nargo curl in the earthfile also
([#10199](AztecProtocol/aztec-packages#10199))
([985a678](AztecProtocol/aztec-packages@985a678))
* **bb.js:** Don't minify bb.js - webpack config
([#10170](AztecProtocol/aztec-packages#10170))
([6e7fae7](AztecProtocol/aztec-packages@6e7fae7))
* Docker compose aztec up fix
([#10197](AztecProtocol/aztec-packages#10197))
([d7ae959](AztecProtocol/aztec-packages@d7ae959))
* Increase test timeouts
([#10205](AztecProtocol/aztec-packages#10205))
([195aa3d](AztecProtocol/aztec-packages@195aa3d))
* Release l1-contracts
([#10095](AztecProtocol/aztec-packages#10095))
([29f0d7a](AztecProtocol/aztec-packages@29f0d7a))
* Revert "feat: blobs.
([#9302](AztecProtocol/aztec-packages#9302))"
([#10187](AztecProtocol/aztec-packages#10187))
([a415f65](AztecProtocol/aztec-packages@a415f65))


### Miscellaneous

* Added ref to env variables
([#10193](AztecProtocol/aztec-packages#10193))
([b51fc43](AztecProtocol/aztec-packages@b51fc43))
* **avm:** Operands reordering
([#10182](AztecProtocol/aztec-packages#10182))
([69bdf4f](AztecProtocol/aztec-packages@69bdf4f)),
closes
[#10136](AztecProtocol/aztec-packages#10136)
* Fix devbox
([#10201](AztecProtocol/aztec-packages#10201))
([323eaee](AztecProtocol/aztec-packages@323eaee))
* Misc cleanup
([#10194](AztecProtocol/aztec-packages#10194))
([dd01417](AztecProtocol/aztec-packages@dd01417))
* Reinstate docs-preview, fix doc publish
([#10213](AztecProtocol/aztec-packages#10213))
([ed9a0e3](AztecProtocol/aztec-packages@ed9a0e3))
* Replace relative paths to noir-protocol-circuits
([1650446](AztecProtocol/aztec-packages@1650446))
</details>

<details><summary>barretenberg: 0.65.0</summary>

##
[0.65.0](AztecProtocol/aztec-packages@barretenberg-v0.64.0...barretenberg-v0.65.0)
(2024-11-26)


### Features

* **avm:** New public inputs witgen
([#10179](AztecProtocol/aztec-packages#10179))
([ac8f13e](AztecProtocol/aztec-packages@ac8f13e))
* Origin tags implemented in biggroup
([#10002](AztecProtocol/aztec-packages#10002))
([c8696b1](AztecProtocol/aztec-packages@c8696b1))
* UltraRollupRecursiveFlavor
([#10088](AztecProtocol/aztec-packages#10088))
([4418ef2](AztecProtocol/aztec-packages@4418ef2))


### Miscellaneous

* **avm:** Operands reordering
([#10182](AztecProtocol/aztec-packages#10182))
([69bdf4f](AztecProtocol/aztec-packages@69bdf4f)),
closes
[#10136](AztecProtocol/aztec-packages#10136)
</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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants