feat(aztec-noir): align public and private execution patterns #1515

Maddiaa0 · 2023-08-10T19:54:27Z

Metadata

fixes

feat(noir-libs): unify Private and Public Context Apis #1484

Relevant discussions

Remove PrivateContextInputs as a required parameter to a private noir contract function #1358 (comment)

Overview

This PR aims to align the implementations of Public and Private state.

Currently public circuits do not have a context object, this will cause developers to have to learn more than one code path.

Other work I am thinking of completing within this PR
Right now, PublicCircuitPublicInputs (The object which is fed to the public kernel after circuit completion) is not created within the application circuit. It is created within the sequencer. If we zoom out, this is not a direct security concern as execution is occuring within the sequencer, however it does break away from the pattern seen within PrivateExecution where the result of each execution is the PrivateCircuitPublicInputs object.

We could leave this as is, (alot of this code will be dumped when the vm arrives). However similar code paths will make building the library easier. For example, for some library methods we have two implementations, one for public and one for private. More and more standardisation would allow these methods to take in a generic context and utilise the same code paths. (A goal I think is worth pursuing).

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
Every change is related to the PR description.
I have linked this pull request to relevant issues (if any exist).

… md/p-context

iAmMichaelConnor · 2023-08-14T11:58:12Z

Tagging @sirasistant, as this touches on some design decisions you've made in the past.

iAmMichaelConnor · 2023-08-14T12:25:24Z

yarn-project/noir-contracts/src/contracts/child_contract/src/main.nr

+        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


I think return_values are part of the PublicCircuitPublicInputs, although I think there are plans to instead return a return_values_hash. When that change happens, the simulator will keep track of the return_values, and the simulator could return those.

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))

LHerskind

Nice job. Got some minor comments.

LHerskind · 2023-08-15T06:26:03Z

circuits/cpp/src/aztec3/circuits/abis/private_circuit_public_inputs.hpp

How come the ordering change in here? Was there a mismatch or was this to follow the structure of historic_block_data?

Ive made a followup pr that does some reorg to prevent structuring mistakes like this: #1567.

It can be merged into this one or run as a standalone pr. It was a big change so Ive broken it up

LHerskind · 2023-08-15T06:28:20Z

circuits/cpp/src/aztec3/circuits/apps/notes/default_private_note/note_preimage.hpp

Is the cpp using a different formatter? Some of these files have different formats without other stuff changing.

Yep, i think these changes came from adams commits where he was using clion as his ide

Should the tidy not have been angry about it? 👀

good question, im i suppose it exists within both rulesets

LHerskind · 2023-08-15T06:31:05Z

...p/src/aztec3/circuits/kernel/public/native_public_kernel_circuit_private_previous_kernel.cpp

@@ -62,7 +62,7 @@ KernelCircuitPublicInputs<NT> native_public_kernel_circuit_private_previous_kern
    // validate the kernel execution common to all invocation circumstances
    common_validate_kernel_execution(builder, public_kernel_inputs);

-    // vallidate our public call hash
+    // valLidate our public call hash


Suggested change

// valLidate our public call hash

// validate our public call hash

LHerskind · 2023-08-15T06:36:11Z

yarn-project/acir-simulator/src/acvm/deserialize.ts

+  );
+
+  const argsHash = witnessReader.readField();
+  const returnValues = padArrayEnd(witnessReader.readFieldArray(RETURN_VALUES_LENGTH), Fr.ZERO, RETURN_VALUES_LENGTH);


Do you need to pad it when reading array of length RETURN_VALUES_LENGTH? Won't you be filling RETURN_VALUES_LENGTH - RETURN_VALUES_LENGTH = 0 elements with Fr.ZERO values.

It is there as a sanity check, i can remove

Just seemed strange when no others had it.

LHerskind · 2023-08-15T06:39:34Z

yarn-project/acir-simulator/src/public/executor.ts

@@ -160,7 +160,8 @@ export class PublicExecutor {
      },
    });

-    const returnValues = extractReturnWitness(acir, partialWitness).map(fromACVMField);
+    const publicInputs = extractPublicCircuitPublicInputs(partialWitness, acir);
+    const { returnValues } = publicInputs;


What is the reason you are not doing? Don't seems like you are using the publicInputs

const { returnValues } = extractPublicCircuitPublicInputs(partialWitness, acir);

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

LHerskind · 2023-08-15T07:04:15Z

yarn-project/noir-libs/noir-aztec/src/abi.nr

@@ -261,6 +261,10 @@ impl ContractStorageRead {
    fn hash(self) -> Field {
        dep::std::hash::pedersen_with_separator(self.serialize(), GENERATOR_INDEX__PUBLIC_DATA_READ)[0]
    }
+
+    fn empty() -> Self {


Do we need empty_contract_storage_read() if we have empty here, seems like we should use the empty instead for clarity.

In call_public_function_with_packed_args in the context we populate contract_storage_read with the empty ones.

LHerskind · 2023-08-15T07:06:08Z

yarn-project/noir-libs/noir-aztec/src/abi.nr

        ]
    }
 }

 fn empty_block_data() -> HistoricBlockData {
-    HistoricBlockData{ private_data_tree_root: 0, nullifier_tree_root: 0, contract_tree_root: 0, l1_to_l2_messages_tree_root: 0, blocks_tree_root: 0, prev_global_variables_hash: 0, public_data_tree_root: 0 }
+    HistoricBlockData{ private_data_tree_root: 0, nullifier_tree_root: 0, contract_tree_root: 0, l1_to_l2_messages_tree_root: 0, blocks_tree_root: 0, public_data_tree_root: 0,  global_variables_hash: 0 }


I think we should replace this with an HistoricBlockData::empty() instead? Seems better for consistency. Only used in context.call_public_function_with_packed_args.

LHerskind · 2023-08-15T07:10:16Z

yarn-project/noir-libs/noir-aztec/src/abi.nr

+
+    fn empty() -> Self {
+        Self { storage_slot: 0, old_value: 0, new_value: 0 }
+    }
 }

 fn empty_contract_storage_update_request() -> ContractStorageUpdateRequest {


My brain want us to use ContractStorageUpdateRequest::empty() where this was used and get rid of this one.

LHerskind · 2023-08-15T07:10:44Z

yarn-project/noir-libs/noir-aztec/src/abi.nr

    prover_address: Field,
+
+
+    // TODO: include globals in here and check them elsewhere


Will you do this as part of this pr or create an issue for it?

Lets create an issue, it is out of scope
edit: #1567

LHerskind · 2023-08-15T07:16:17Z

yarn-project/sequencer-client/src/sequencer/public_processor.ts

@@ -211,7 +214,7 @@ export class PublicProcessor {
    const unencryptedLogsHash = to2Fields(result.unencryptedLogs.hash());
    const unencryptedLogPreimagesLength = new Fr(result.unencryptedLogs.getSerializedLength());

-    return PublicCircuitPublicInputs.from({
+    const pub = PublicCircuitPublicInputs.from({


Seems like you printed pub for debugging, but as not done anymore, you might as well return it directly here 👍.

iAmMichaelConnor · 2023-08-15T10:27:42Z

yarn-project/noir-contracts/src/contracts/public_token_contract/src/main.nr

    open fn mint(
-        _inputs: PublicContextInputs,
+        inputs: PublicContextInputs,
        amount: Field,
        recipient: Field,
-    ) -> pub Field {
+    ) -> pub abi::PublicCircuitPublicInputs {
+        let mut context = PublicContext::new(inputs, abi::hash_args([amount, recipient]));
+
        let storage = Storage::init();
        let recipient_balance = storage.balances.at(recipient);
        let new_amount = recipient_balance.read() + amount;
        // TODO: Remove return value.
        let _hash = emit_unencrypted_log("Coins minted");
        recipient_balance.write(new_amount);
-        new_amount
+
+        context.return_values.push(new_amount);
+
+        context.finish()
    }


Highlighting this diff, so I can link to it elsewhere, for a discussion.

… md/p-context

LHerskind

Looks good to me.

fixes dirty merge between #1570 and #1515

…ts (#1571) Add a bit more consistency across the code base, I came into these issues when working on #1515. Running this in a separate pr as it is not directly related. And to minimise the diff between the two ## Overview Replaces 6 seperate roots with one single data structure that is used literally everywhere else

feat: seperate public and private contexts

4219b44

Maddiaa0 changed the title ~~feat: align public and private execution patterns~~ feat(aztec-noir): align public and private execution patterns Aug 10, 2023

spalladino mentioned this pull request Aug 11, 2023

feat(noir): autogenerate contract interface for calling from external contracts #1487

Merged

Maddiaa0 added 4 commits August 11, 2023 14:22

feat: public processor consuming public context

ccd42d8

feat: Add to other contracts

cac043f

feat: use context methods

d6b4437

fix: use context rather than inputs more often

9b95458

Maddiaa0 force-pushed the md/p-context branch from 61afd38 to 9b95458 Compare August 11, 2023 23:08

Merge branch 'master' of github.com:AztecProtocol/aztec-packages into…

d8f1700

… md/p-context

Maddiaa0 mentioned this pull request Aug 14, 2023

fix: return DecodedReturn instead of any[] #1540

Merged

4 tasks

iAmMichaelConnor reviewed Aug 14, 2023

View reviewed changes

Maddiaa0 added 7 commits August 14, 2023 13:51

Merge branch 'master' into md/p-context

b3c336d

fix: codegen merge issues

d92fc4e

fix: unify foreign call syntax

2447886

feat: include block hash in public circuit public input shape

79c1e7a

temp

0fa5c7a

Add blockdata into public hashing

5289f2b

lint

25f90a1

Maddiaa0 requested a review from LHerskind August 14, 2023 19:08

Maddiaa0 marked this pull request as ready for review August 14, 2023 19:08

Maddiaa0 added 2 commits August 14, 2023 19:12

snapshot

b97c244

remove log

17b666c

LHerskind reviewed Aug 15, 2023

View reviewed changes

iAmMichaelConnor reviewed Aug 15, 2023

View reviewed changes

Maddiaa0 added 5 commits August 15, 2023 12:08

fix: typo

106615a

Merge branch 'master' of github.com:AztecProtocol/aztec-packages into…

e2dacf2

… md/p-context

chore: remove noirup script unused var

8c448f9

chore: refactor empty in noir

e56d3e5

fix: ts nits

1bf4a23

Maddiaa0 enabled auto-merge (squash) August 15, 2023 15:01

Maddiaa0 requested a review from LHerskind August 15, 2023 15:06

Maddiaa0 mentioned this pull request Aug 15, 2023

chore: remove individual historic roots from privateCircuitPublicInputs #1571

Merged

LHerskind approved these changes Aug 15, 2023

View reviewed changes

Maddiaa0 merged commit 35a81c3 into master Aug 15, 2023

Maddiaa0 deleted the md/p-context branch August 15, 2023 16:29

Maddiaa0 mentioned this pull request Aug 15, 2023

chore: fix dirty merge #1574

Merged

Maddiaa0 added a commit that referenced this pull request Aug 15, 2023

chore: fix dirty merge (#1574)

58dc9bf

fixes dirty merge between #1570 and #1515

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aztec-noir): align public and private execution patterns #1515

feat(aztec-noir): align public and private execution patterns #1515

Maddiaa0 commented Aug 10, 2023 •

edited

Loading

iAmMichaelConnor commented Aug 14, 2023

iAmMichaelConnor Aug 14, 2023

sirasistant Aug 14, 2023

LHerskind left a comment

LHerskind Aug 15, 2023

Maddiaa0 Aug 15, 2023

LHerskind Aug 15, 2023

Maddiaa0 Aug 15, 2023

LHerskind Aug 15, 2023

Maddiaa0 Aug 15, 2023

LHerskind Aug 15, 2023

LHerskind Aug 15, 2023

Maddiaa0 Aug 15, 2023

LHerskind Aug 15, 2023

LHerskind Aug 15, 2023

Maddiaa0 Aug 15, 2023

LHerskind Aug 15, 2023

LHerskind Aug 15, 2023

LHerskind Aug 15, 2023

LHerskind Aug 15, 2023

Maddiaa0 Aug 15, 2023 •

edited

Loading

LHerskind Aug 15, 2023

iAmMichaelConnor Aug 15, 2023

LHerskind left a comment

	// valLidate our public call hash
	// validate our public call hash

		prover_address: Field,


		// TODO: include globals in here and check them elsewhere

feat(aztec-noir): align public and private execution patterns #1515

feat(aztec-noir): align public and private execution patterns #1515

Conversation

Maddiaa0 commented Aug 10, 2023 • edited Loading

Metadata

Overview

Checklist:

iAmMichaelConnor commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maddiaa0 Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LHerskind left a comment

Choose a reason for hiding this comment

Maddiaa0 commented Aug 10, 2023 •

edited

Loading

Maddiaa0 Aug 15, 2023 •

edited

Loading