-
Notifications
You must be signed in to change notification settings - Fork 283
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(aztec-noir): align public and private execution patterns #1515
Changes from 5 commits
4219b44
ccd42d8
cac043f
d6b4437
9b95458
d8f1700
b3c336d
d92fc4e
2447886
79c1e7a
0fa5c7a
5289f2b
25f90a1
b97c244
17b666c
106615a
e2dacf2
8c448f9
e56d3e5
1bf4a23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ import { createDebugLogger } from '@aztec/foundation/log'; | |
import { to2Fields } from '@aztec/foundation/serialize'; | ||
import { FunctionL2Logs, NotePreimage, NoteSpendingInfo } from '@aztec/types'; | ||
|
||
import { extractPublicInputs, frToAztecAddress, frToSelector } from '../acvm/deserialize.js'; | ||
import { extractPrivateCircuitPublicInputs, frToAztecAddress, frToSelector } from '../acvm/deserialize.js'; | ||
import { | ||
ZERO_ACVM_FIELD, | ||
acvm, | ||
|
@@ -53,7 +53,7 @@ export class PrivateFunctionExecution { | |
this.log(`Executing external function ${this.contractAddress.toString()}:${selector}`); | ||
|
||
const acir = Buffer.from(this.abi.bytecode, 'base64'); | ||
const initialWitness = this.writeInputs(); | ||
const initialWitness = this.getInitialWitness(); | ||
|
||
// TODO: Move to ClientTxExecutionContext. | ||
const newNotePreimages: NewNoteData[] = []; | ||
|
@@ -182,7 +182,7 @@ export class PrivateFunctionExecution { | |
}, | ||
}); | ||
|
||
const publicInputs = extractPublicInputs(partialWitness, acir); | ||
const publicInputs = extractPrivateCircuitPublicInputs(partialWitness, acir); | ||
|
||
// TODO(https://github.com/AztecProtocol/aztec-packages/issues/1165) --> set this in Noir | ||
publicInputs.encryptedLogsHash = to2Fields(encryptedLogs.hash()); | ||
|
@@ -221,7 +221,7 @@ export class PrivateFunctionExecution { | |
* Writes the function inputs to the initial witness. | ||
* @returns The initial witness. | ||
*/ | ||
private writeInputs() { | ||
private getInitialWitness() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
const contractDeploymentData = this.context.txContext.contractDeploymentData ?? ContractDeploymentData.empty(); | ||
|
||
const blockData = this.context.constantHistoricBlockData; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ import { | |
ZERO_ACVM_FIELD, | ||
acvm, | ||
convertACVMFieldToBuffer, | ||
extractReturnWitness, | ||
extractPublicCircuitPublicInputs, | ||
frToAztecAddress, | ||
frToSelector, | ||
fromACVMField, | ||
|
@@ -160,7 +160,9 @@ export class PublicExecutor { | |
}, | ||
}); | ||
|
||
const returnValues = extractReturnWitness(acir, partialWitness).map(fromACVMField); | ||
// TODO: get the rest of everything from here, this should also be used to get the new Commitments, Nullifiers etc. | ||
const publicInputs = extractPublicCircuitPublicInputs(partialWitness, acir); | ||
const { returnValues } = publicInputs; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason you are not doing? Don't seems like you are using the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The use of the public inputs is dependant on some ongoing discussions around how we will handle context in the vm setting. I am unsure if we will use the publicInputs further. When i started the work I was under the impression that I was going to. I will update |
||
|
||
const [contractStorageReads, contractStorageUpdateRequests] = storageActions.collect(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,44 +5,58 @@ contract Child { | |
use dep::aztec::abi; | ||
use dep::aztec::abi::PrivateContextInputs; | ||
use dep::aztec::abi::PublicContextInputs; | ||
use dep::aztec::context::Context; | ||
use dep::aztec::context::{ | ||
PrivateContext, | ||
PublicContext | ||
}; | ||
use crate::storage::Storage; | ||
|
||
fn constructor( | ||
inputs: PrivateContextInputs, | ||
) -> distinct pub abi::PrivateCircuitPublicInputs { | ||
// Return private circuit public inputs. All private functions need to return this as it is part of the input of the private kernel. | ||
Context::new(inputs, 0).finish() | ||
PrivateContext::new(inputs, 0).finish() | ||
} | ||
|
||
// Returns a sum of the input and the chain id and version of the contract in private circuit public input's return_values. | ||
fn value( | ||
inputs: PrivateContextInputs, | ||
input: Field, | ||
) -> distinct pub abi::PrivateCircuitPublicInputs { | ||
let mut context = Context::new(inputs, abi::hash_args([input])); | ||
let mut context = PrivateContext::new(inputs, abi::hash_args([input])); | ||
|
||
context.return_values.push(input + inputs.private_global_variables.chain_id + inputs.private_global_variables.version); | ||
context.return_values.push(input + context.chain_id() + context.version()); | ||
|
||
// Return private circuit public inputs. All private functions need to return this as it is part of the input of the private kernel. | ||
context.finish() | ||
} | ||
|
||
// Returns base_value + 42. | ||
open fn pubValue(inputs: PublicContextInputs, base_value: Field) -> pub Field { | ||
base_value + inputs.public_global_variables.chain_id + inputs.public_global_variables.version + inputs.public_global_variables.block_number + inputs.public_global_variables.timestamp | ||
open fn pubValue(inputs: PublicContextInputs, base_value: Field) -> pub abi::PublicCircuitPublicInputs { | ||
let mut context = PublicContext::new(inputs, abi::hash_args([base_value])); | ||
|
||
// TODO: make these available on context | ||
let returnValue = base_value + context.chain_id() + context.version() + context.block_number() + context.timestamp(); | ||
|
||
context.return_values.push(returnValue); | ||
// TODO(MADDIAA): MAYBE we put the return values inside the finish object? That could have nice UX | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think return_values are part of the PublicCircuitPublicInputs, although I think there are plans to instead return a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah but it's true that even if we do return_values_hash we don't need to keep them inside the context, they can instead be passed as arguments to finish() both in public and private, and when we have return_values_hash then finish(hash_returns(whatever)) |
||
context.finish() | ||
} | ||
|
||
// Increments `current_value` by `new_value` and returns `new_value` + 1. | ||
open fn pubStoreValue(_inputs: PublicContextInputs, new_value: Field) -> pub Field { | ||
open fn pubStoreValue(inputs: PublicContextInputs, new_value: Field) -> pub abi::PublicCircuitPublicInputs { | ||
let mut context = PublicContext::new(inputs, abi::hash_args([new_value])); | ||
|
||
let storage = Storage::init(); | ||
let old_value = storage.current_value.read(); | ||
// Compiler complains if we don't assign the result to anything | ||
let _void1 = storage.current_value.write(old_value + new_value); | ||
// Compiler fails with "we do not allow private ABI inputs to be returned as public outputs" if we try to | ||
// return new_value as-is, but then it also complains if we add `pub` to `new_value` in the args, so we | ||
// just assign it to another variable and tweak it so it's not the same value, and the compiler is happy. | ||
let ret_value = new_value + 1; | ||
ret_value | ||
let return_value = new_value + 1; | ||
|
||
context.return_values.push(return_value); | ||
context.finish() | ||
} | ||
} |
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.
Do you need to pad it when reading array of length
RETURN_VALUES_LENGTH
? Won't you be fillingRETURN_VALUES_LENGTH - RETURN_VALUES_LENGTH = 0
elements withFr.ZERO
values.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.
It is there as a sanity check, i can remove
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.
Just seemed strange when no others had it.