-
Notifications
You must be signed in to change notification settings - Fork 270
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): basic AVM-ACVM interoperability #5595
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @fcarreiro and the rest of your teammates on Graphite |
Benchmark resultsMetrics with a significant change:
Detailed resultsAll benchmarks are run on txs on the This benchmark source data is available in JSON format on S3 here. Values are compared against data from master at commit L2 block published to L1Each column represents the number of txs on an L2 block published to L1.
L2 chain processingEach column represents the number of blocks on the L2 chain where each block has 16 txs.
Circuits statsStats on running time and I/O sizes collected for every circuit run across all benchmarks.
Tree insertion statsThe duration to insert a fixed batch of leaves into each tree type.
MiscellaneousTransaction sizes based on how many contract classes are registered in the tx.
Transaction size based on fee payment method
Transaction processing duration by data writes.
|
2e89a28
to
6ccd909
Compare
// Cannot work because ACVM does not support pending nullifiers. | ||
// it('AVM->ACVM nullifiers work (pending)', async () => { | ||
// await avmContract.methods.avm_to_acvm_nullifier().send().wait(); | ||
// }); | ||
|
||
it('AVM sees settled nullifiers by ACVM', async () => { | ||
const nullifier = new Fr(123456); | ||
await avmContract.methods.new_nullifier(nullifier).send().wait(); | ||
await avmContract.methods.assert_unsiloed_nullifier_acvm(nullifier).send().wait(); | ||
}); | ||
|
||
it('AVM nested call to ACVM sees settled nullifiers', async () => { | ||
const nullifier = new Fr(123456); | ||
await avmContract.methods.new_nullifier(nullifier).send().wait(); | ||
await avmContract.methods | ||
.avm_to_acvm_call(FunctionSelector.fromSignature('assert_unsiloed_nullifier_acvm(Field)'), nullifier) | ||
.send() | ||
.wait(); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, I wish we didn't have to go down this rabbit hole of accumulating/passing nullifiers between ACVM and AVM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done! It's unfortunate that we have to do all of this.... But I can't think of a better way. It will get even uglier when we properly handle nested calls to different contracts and pass their side-effects to the kernel.
6ccd909
to
dd50e6f
Compare
dd50e6f
to
258ed4e
Compare
fb1def3
to
8416597
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
||
// Cannot work because ACVM does not support pending nullifiers. | ||
// it('AVM->ACVM nullifiers work (pending)', async () => { | ||
// await avmContract.methods.avm_to_acvm_nullifier().send().wait(); | ||
// }); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this EVER work? If not, should we remove the commented code? If it SHOULD work someday, maybe we link a ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will never work. Removed.
public readonly contractCallDepth: Fr, | ||
|
||
public readonly header: Header, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might eventually want opcodes to directly get certain entries from this header (different from HEADERMEMBER which gets historic info)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can do all of it from DB at that point. This was the easy way out for now :)
BTW, when I discussed with Lasse, he thought that HEADERMEMBER only made sense w/the latest header, because all the rest you can do in private.
@@ -502,7 +547,7 @@ describe('AVM simulator: transpiled Noir contracts', () => { | |||
const context = initContext({ env: initExecutionEnvironment({ calldata }) }); | |||
jest | |||
.spyOn(context.persistableState.hostStorage.contractsDb, 'getBytecode') | |||
.mockReturnValueOnce(Promise.resolve(addBytecode)); | |||
.mockReturnValue(Promise.resolve(addBytecode)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is mockReturnValue preferred over mockReturnValueOnce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No! But since I'm now getting the bytecode first to decide if it's AVM or ACVM I needed to change it.
expect(results.reverted).toBe(true); // The outer call should revert. | ||
// TODO(fcarreiro): revertReason lost in translation between results. | ||
// expect(results.revertReason).toEqual(/StaticCallStorageAlterError/); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the revertReason lost? Also is there a ticket or more info on how/when this will be solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's because the way nested calls in PublicExecution* are handled. Nested executions in current public are not expected to revert and carry on, they would throw and everything dies. This should certainly work once we remove the usage of the ACVM, but I might need to get it working somehow before that.
|
||
/** | ||
* We want to keep track of all performed reads in the journal | ||
* This information is hinted to the avm circuit | ||
|
||
* @param contractAddress - | ||
* @param key - | ||
* @param value - | ||
*/ | ||
journalUpdate(map: Map<bigint, Map<bigint, Fr[]>>, contractAddress: Fr, key: Fr, value: Fr): void { | ||
let contractMap = map.get(contractAddress.toBigInt()); | ||
if (!contractMap) { | ||
contractMap = new Map<bigint, Array<Fr>>(); | ||
map.set(contractAddress.toBigInt(), contractMap); | ||
} | ||
|
||
let accessArray = contractMap.get(key.toBigInt()); | ||
if (!accessArray) { | ||
accessArray = new Array<Fr>(); | ||
contractMap.set(key.toBigInt(), accessArray); | ||
} | ||
accessArray.push(value); | ||
} | ||
|
||
// Create an instance of journalUpdate that appends to the read array | ||
private journalRead = this.journalUpdate.bind(this, this.publicStorageReads); | ||
// Create an instance of journalUpdate that appends to the writes array | ||
private journalWrite = this.journalUpdate.bind(this, this.publicStorageWrites); | ||
} | ||
|
||
/** | ||
* Merges two contract journalling maps together | ||
* For read maps, we just append the childMap arrays into the host map arrays, as the order is important | ||
* | ||
* @param hostMap - The map to be merged into | ||
* @param childMap - The map to be merged from | ||
*/ | ||
function mergeContractJournalMaps(hostMap: Map<bigint, Map<bigint, Fr[]>>, childMap: Map<bigint, Map<bigint, Fr[]>>) { | ||
for (const [key, value] of childMap) { | ||
const map1Value = hostMap.get(key); | ||
if (!map1Value) { | ||
hostMap.set(key, value); | ||
} else { | ||
mergeStorageJournalMaps(map1Value, value); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Merge two storage journalling maps together (for a particular contract). | ||
* | ||
* @param hostMap - The map to be merge into | ||
* @param childMap - The map to be merged from | ||
*/ | ||
function mergeStorageJournalMaps(hostMap: Map<bigint, Fr[]>, childMap: Map<bigint, Fr[]>) { | ||
for (const [key, value] of childMap) { | ||
const readArr = hostMap.get(key); | ||
if (!readArr) { | ||
hostMap.set(key, value); | ||
} else { | ||
hostMap.set(key, readArr?.concat(...value)); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you could delete all this! I've been hoping to for a while now!
8416597
to
e733a83
Compare
e733a83
to
8fc5b98
Compare
Merge activity
|
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.34.0</summary> ## [0.34.0](aztec-package-v0.33.0...aztec-package-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](#5622)) ([c496a10](c496a10)) </details> <details><summary>barretenberg.js: 0.34.0</summary> ## [0.34.0](barretenberg.js-v0.33.0...barretenberg.js-v0.34.0) (2024-04-10) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.34.0</summary> ## [0.34.0](aztec-cli-v0.33.0...aztec-cli-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](#5622)) ([c496a10](c496a10)) </details> <details><summary>aztec-packages: 0.34.0</summary> ## [0.34.0](aztec-packages-v0.33.0...aztec-packages-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](#5617)) ### Features * **acvm_js:** Execute program (noir-lang/noir#4694) ([ff28080](ff28080)) * Add `remove_enable_side_effects` SSA pass (noir-lang/noir#4224) ([ff28080](ff28080)) * Allow slices to brillig entry points (noir-lang/noir#4713) ([ff28080](ff28080)) * **avm:** Basic AVM-ACVM interoperability ([#5595](#5595)) ([d872445](d872445)) * **avm:** Make authwit work with avm ([#5594](#5594)) ([b02d1e1](b02d1e1)) * **docs:** Documenting noir codegen (noir-lang/noir#4454) ([ff28080](ff28080)) * Generalize protogalaxy to multiple instances ([#5510](#5510)) ([f038b70](f038b70)) * Improve nargo check cli with --override flag and feedback for existing files (noir-lang/noir#4575) ([ff28080](ff28080)) * Improve optimisations on range constraints (noir-lang/noir#4690) ([ff28080](ff28080)) * Improve SSA type-awareness in EQ and MUL instructions (noir-lang/noir#4691) ([ff28080](ff28080)) * **nargo:** Multiple circuits info for binary programs (noir-lang/noir#4719) ([ff28080](ff28080)) * Stdlib databus ([#5598](#5598)) ([633a711](633a711)) ### Bug Fixes * **acvm:** Mark outputs of Opcode::Call solvable (noir-lang/noir#4708) ([ff28080](ff28080)) * Do not retry RPC requests on 4xx errors ([#5634](#5634)) ([5af2b95](5af2b95)) * Field comparisons (noir-lang/noir#4704) ([ff28080](ff28080)) * Last use analysis & make it an SSA pass (noir-lang/noir#4686) ([ff28080](ff28080)) * **ssa:** Do not use get_value_max_num_bits when we want pure type information (noir-lang/noir#4700) ([ff28080](ff28080)) * Unknown slice lengths coming from as_slice (noir-lang/noir#4725) ([ff28080](ff28080)) * Use empty artifact in test ([#5640](#5640)) ([1d18a5e](1d18a5e)) ### Miscellaneous * Check for references to private functions during path resolution (noir-lang/noir#4622) ([ff28080](ff28080)) * **ci:** Fix cutting new versions of the docs (noir-lang/noir#4737) ([ff28080](ff28080)) * **ci:** Replace `yarn build:js:only` script (noir-lang/noir#4735) ([ff28080](ff28080)) * **ci:** Stop updating version list before cutting new docs version (noir-lang/noir#4726) ([ff28080](ff28080)) * Disable earthly cloud ([#5639](#5639)) ([47e9c25](47e9c25)) * Fix clippy errors (noir-lang/noir#4684) ([ff28080](ff28080)) * Reduce log verbosity in local e2e tests ([#5622](#5622)) ([c496a10](c496a10)) * Remove `FunctionInput::dummy` (noir-lang/noir#4723) ([ff28080](ff28080)) * Remove conditional compilation around `acvm_js` package (noir-lang/noir#4702) ([ff28080](ff28080)) * Remove docker CI flow (noir-lang/noir#4724) ([ff28080](ff28080)) * Remove fixed-length keccak256 ([#5617](#5617)) ([40480b3](40480b3)) * Remove last traces of nix (noir-lang/noir#4679) ([ff28080](ff28080)) * Remove unused env vars from `Cross.toml` (noir-lang/noir#4717) ([ff28080](ff28080)) * Replace relative paths to noir-protocol-circuits ([bc214c5](bc214c5)) * Simplify how `acvm_backend.wasm` is embedded (noir-lang/noir#4703) ([ff28080](ff28080)) * Simplify how blns is loaded into tests (noir-lang/noir#4705) ([ff28080](ff28080)) * Update condition for clearing warning comment on release PRs (noir-lang/noir#4739) ([ff28080](ff28080)) * Update from vulnerable version of h2 (noir-lang/noir#4714) ([ff28080](ff28080)) * Update JS publish workflow to upload build artifacts correctly. (noir-lang/noir#4734) ([ff28080](ff28080)) * Use is_entry_point helper on RuntimeType (noir-lang/noir#4678) ([ff28080](ff28080)) </details> <details><summary>barretenberg: 0.34.0</summary> ## [0.34.0](barretenberg-v0.33.0...barretenberg-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](#5617)) ### Features * Generalize protogalaxy to multiple instances ([#5510](#5510)) ([f038b70](f038b70)) * Stdlib databus ([#5598](#5598)) ([633a711](633a711)) ### Miscellaneous * Remove fixed-length keccak256 ([#5617](#5617)) ([40480b3](40480b3)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release *beep* *boop* --- <details><summary>aztec-package: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@aztec-package-v0.33.0...aztec-package-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](AztecProtocol/aztec-packages#5622)) ([c496a10](AztecProtocol/aztec-packages@c496a10)) </details> <details><summary>barretenberg.js: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@barretenberg.js-v0.33.0...barretenberg.js-v0.34.0) (2024-04-10) ### Miscellaneous * **barretenberg.js:** Synchronize aztec-packages versions </details> <details><summary>aztec-cli: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@aztec-cli-v0.33.0...aztec-cli-v0.34.0) (2024-04-10) ### Miscellaneous * Reduce log verbosity in local e2e tests ([#5622](AztecProtocol/aztec-packages#5622)) ([c496a10](AztecProtocol/aztec-packages@c496a10)) </details> <details><summary>aztec-packages: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@aztec-packages-v0.33.0...aztec-packages-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ### Features * **acvm_js:** Execute program (noir-lang/noir#4694) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Add `remove_enable_side_effects` SSA pass (noir-lang/noir#4224) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Allow slices to brillig entry points (noir-lang/noir#4713) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **avm:** Basic AVM-ACVM interoperability ([#5595](AztecProtocol/aztec-packages#5595)) ([d872445](AztecProtocol/aztec-packages@d872445)) * **avm:** Make authwit work with avm ([#5594](AztecProtocol/aztec-packages#5594)) ([b02d1e1](AztecProtocol/aztec-packages@b02d1e1)) * **docs:** Documenting noir codegen (noir-lang/noir#4454) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Generalize protogalaxy to multiple instances ([#5510](AztecProtocol/aztec-packages#5510)) ([f038b70](AztecProtocol/aztec-packages@f038b70)) * Improve nargo check cli with --override flag and feedback for existing files (noir-lang/noir#4575) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Improve optimisations on range constraints (noir-lang/noir#4690) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Improve SSA type-awareness in EQ and MUL instructions (noir-lang/noir#4691) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **nargo:** Multiple circuits info for binary programs (noir-lang/noir#4719) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Stdlib databus ([#5598](AztecProtocol/aztec-packages#5598)) ([633a711](AztecProtocol/aztec-packages@633a711)) ### Bug Fixes * **acvm:** Mark outputs of Opcode::Call solvable (noir-lang/noir#4708) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Do not retry RPC requests on 4xx errors ([#5634](AztecProtocol/aztec-packages#5634)) ([5af2b95](AztecProtocol/aztec-packages@5af2b95)) * Field comparisons (noir-lang/noir#4704) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Last use analysis & make it an SSA pass (noir-lang/noir#4686) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ssa:** Do not use get_value_max_num_bits when we want pure type information (noir-lang/noir#4700) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Unknown slice lengths coming from as_slice (noir-lang/noir#4725) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Use empty artifact in test ([#5640](AztecProtocol/aztec-packages#5640)) ([1d18a5e](AztecProtocol/aztec-packages@1d18a5e)) ### Miscellaneous * Check for references to private functions during path resolution (noir-lang/noir#4622) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ci:** Fix cutting new versions of the docs (noir-lang/noir#4737) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ci:** Replace `yarn build:js:only` script (noir-lang/noir#4735) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * **ci:** Stop updating version list before cutting new docs version (noir-lang/noir#4726) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Disable earthly cloud ([#5639](AztecProtocol/aztec-packages#5639)) ([47e9c25](AztecProtocol/aztec-packages@47e9c25)) * Fix clippy errors (noir-lang/noir#4684) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Reduce log verbosity in local e2e tests ([#5622](AztecProtocol/aztec-packages#5622)) ([c496a10](AztecProtocol/aztec-packages@c496a10)) * Remove `FunctionInput::dummy` (noir-lang/noir#4723) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove conditional compilation around `acvm_js` package (noir-lang/noir#4702) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove docker CI flow (noir-lang/noir#4724) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ([40480b3](AztecProtocol/aztec-packages@40480b3)) * Remove last traces of nix (noir-lang/noir#4679) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Remove unused env vars from `Cross.toml` (noir-lang/noir#4717) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Replace relative paths to noir-protocol-circuits ([bc214c5](AztecProtocol/aztec-packages@bc214c5)) * Simplify how `acvm_backend.wasm` is embedded (noir-lang/noir#4703) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Simplify how blns is loaded into tests (noir-lang/noir#4705) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Update condition for clearing warning comment on release PRs (noir-lang/noir#4739) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Update from vulnerable version of h2 (noir-lang/noir#4714) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Update JS publish workflow to upload build artifacts correctly. (noir-lang/noir#4734) ([ff28080](AztecProtocol/aztec-packages@ff28080)) * Use is_entry_point helper on RuntimeType (noir-lang/noir#4678) ([ff28080](AztecProtocol/aztec-packages@ff28080)) </details> <details><summary>barretenberg: 0.34.0</summary> ## [0.34.0](AztecProtocol/aztec-packages@barretenberg-v0.33.0...barretenberg-v0.34.0) (2024-04-10) ### ⚠ BREAKING CHANGES * remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ### Features * Generalize protogalaxy to multiple instances ([#5510](AztecProtocol/aztec-packages#5510)) ([f038b70](AztecProtocol/aztec-packages@f038b70)) * Stdlib databus ([#5598](AztecProtocol/aztec-packages#5598)) ([633a711](AztecProtocol/aztec-packages@633a711)) ### Miscellaneous * Remove fixed-length keccak256 ([#5617](AztecProtocol/aztec-packages#5617)) ([40480b3](AztecProtocol/aztec-packages@40480b3)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Makes it possible to call external public calls to/from AVM/ACVM.
I'll handle gas for nested calls in a separate PR.