refactor: remove selector from public call request by LeilaWang · Pull Request #12828 · AztecProtocol/aztec-packages

LeilaWang · 2025-03-18T08:46:45Z

Remove function_selector from PublicCallRequest as it is no longer used.
Remove public_functions from ContractClass.
ContractClass checks that there's at most 1 public function in the artifact.
Rename args to calldata when it's public calldata that includes the function selector.
Use different generator indexes for calldata and regular args.

aztec-nr

Rename oracle calls enqueue_public_function_call_internal to notify_enqueued_public_function_call , and set_public_teardown_function_call_internal to notify_set_public_teardown_function_call , to be consistent with other oracle calls.
Storing data to execution cache will need to provide the hash in addition to the preimage, making it possible to use it for different types of data. Currently it's used for calldata and args.

node

Rename getContractFunctionName to getDebugFunctionName, as the function name won't always be available. It's set by explicitly calling registerContractFunctionSignatures for debugging purpose.
Remove ExecutionRequest[] in Tx and replace it with calldata[]. We now loop through the publicCallRequests in the public inputs and pair each with an entry in calldata[].
- DataValidator checks that calldata are provided for all the non-empty public call requests, and the hash is correct.

…_selector

LeilaWang · 2025-03-18T15:03:56Z

yarn-project/pxe/src/private_kernel/private_kernel_oracle_impl.ts


  public async getContractAddressPreimage(address: AztecAddress) {
    const instance = await this.contractDataProvider.getContractInstance(address);
+    if (!instance) {


The functions in contractDataProvider behaved differently: some threw when the value was not found and some returned undefined. I've changed them all to be returning value | undefined, and check the result and throw specific error elsewhere.

LeilaWang · 2025-03-18T15:19:13Z

yarn-project/pxe/src/storage/contract_data_provider/contract_data_provider.ts

  public async getDebugFunctionName(contractAddress: AztecAddress, selector: FunctionSelector) {
-    const tree = await this.getTreeForAddress(contractAddress);
-    const { name: contractName } = tree.getArtifact();
-    const { name: functionName } = await tree.getFunctionArtifact(selector);


Moving artifact out of the PrivateFunctionsTree as it was confusing to fetch public functions from a component dealing with private functions. And the above didn't work for public functions except for the public dispatch.

LeilaWang · 2025-03-18T16:13:13Z

yarn-project/stdlib/src/tx/tx.ts

      feePaymentMethod:
        // needsSetup? then we pay through a fee payment contract
-        this.data.forPublic?.needsSetup
-          ? // if the first call is to `approve_public_authwit`, then it's a public payment


This function doesn't exist anymore. Changing the displayed text to be either 'fee_juice' or fpc as there's no reliable way to check what kind of fpc it is.

…_selector

yarn-project/stdlib/src/contract/artifact_hash.test.ts

Thunkar · 2025-03-20T08:34:53Z

Hell of a PR, aztec.js side LGTM. Tagging @nventuro to take a look at aztec-nr since he and Jan have been refactoring oracles these last few days.

One thing that worries me: whose responsibility is to call registerContractFunctionSignatures? I've not been able to solve callstacks and errors for a while if they happen in public functions 😢 After this change and ensuring that function is called, is that expected to work again?

LeilaWang · 2025-03-20T10:47:05Z

whose responsibility is to call registerContractFunctionSignatures?

Currently it's called by pxe when registering a contract. Think it makes sense for testing!

I've not been able to solve callstacks and errors for a while if they happen in public functions 😢 After this change and ensuring that function is called, is that expected to work again?

I don't think so 😞 On the public side, the data provided by calling this function is only used to show real function names in logs. Originally I wanted to remove it in this pr, but others think it's useful to keep it around. To solve callstacks should rely on the public dispatch function's debug metadata, and we don't have it in public right now. But maybe we can change this api to provide it!

Thunkar

Aztec.js side LGTM!

fcarreiro

Amazing work, thanks! LGTM for AVM. Left a few comments, the most important one being about making sure that we don't over-enforce the aztec (public) calling convention in places we shouldn't (outside aztec{nr,js}).

fcarreiro · 2025-03-21T09:47:11Z

noir-projects/aztec-nr/aztec/src/hash.nr

+    }
+}
+
+pub fn hash_calldata(calldata: [Field]) -> Field {


Does this make us free to change the hash for public independent of private? I know we might need to change it to something more avm-friendly soon (cc @IlyasRidhuan )

Yes! Just need to update this and the corresponding computeCalldataHash in ts :)

fcarreiro · 2025-03-21T09:48:48Z

noir-projects/aztec-nr/aztec/src/hash.nr

    }
 }

+pub fn hash_calldata_array<let N: u32>(calldata: [Field; N]) -> Field {
+    if calldata.len() == 0 {
+        0
+    } else {
+        poseidon2_hash_with_separator(calldata, GENERATOR_INDEX__PUBLIC_CALLDATA)
+    }
+}
+
+pub fn hash_calldata(calldata: [Field]) -> Field {


[Re: lines +92 to +108]

Could you maybe add a comment to the hash_{args,array,calldata,etc} functions? Basically IIUC saying which one is used for private and which one for public.

See this comment inline on Graphite.

fcarreiro · 2025-03-21T09:53:13Z

yarn-project/pxe/src/pxe_service/error_enriching.ts

 ) {
  const callStack = err.getCallStack();
  const originalFailingFunction = callStack[callStack.length - 1];
-  // TODO(https://github.com/AztecProtocol/aztec-packages/issues/8985): Properly fix this.


fcarreiro · 2025-03-21T10:01:07Z

yarn-project/simulator/src/public/fixtures/public_tx_simulation_tester.ts

+    const calldata = [fnSelector.toField(), ...encodedArgs];
+    const calldataHash = await computeCalldataHash(calldata);
+    const isStaticCall = call.isStaticCall ?? false;
+    const request = new PublicCallRequest(sender, address, isStaticCall, calldataHash);


You could consider having a static factory method in PublicCallRequest that takes the calldata, and internally computes the hash (then the choice of hash etc would be just in one place).

Good idea! Will do :)

fcarreiro · 2025-03-21T10:05:57Z

yarn-project/stdlib/src/tx/hashed_values.ts

+  static async fromArgs(args: Fr[]) {
+    return new HashedValues(args, await computeVarArgsHash(args));
+  }
+
+  static async fromCalldata(calldata: Fr[]) {
+    return new HashedValues(calldata, await computeCalldataHash(calldata));
+  }


Could you add a comment that one is for private and the other one for public?

fcarreiro · 2025-03-21T10:15:24Z

yarn-project/stdlib/src/tx/public_call_request_with_calldata.ts

+
+  // Public functions get routed through the dispatch function, whose first argument is the target function selector.
+  get functionSelector(): FunctionSelector {
+    return FunctionSelector.fromField(this.calldata[0]);


Just one comment here but it applies to the whole PR: while the aztec{nr,js} calling convention is to have the funcion selector first, args after that, in principle someone could craft a transaction that doesn't follow this.

Also, there could be contracts that don't follow this. That is, execution of a public contract should just receive the whole calldata that was sent in the TX and in principle nothing should prevent that. As extreme examples, the calldata could be empty, and I think our nodes/tx processing/validation should not fail (debugging and logs might not be able to display things but shouldn't crash). Another example is when there IS a first field, but the semantic is not that of a function selector (because eg the contract was written in asm and while it does have a public_dispatch it is not really dispatching).

I was going to remove any reference of function selector in public land. It's only used for debug logs and in the tests for fetching bytecode of individual public function. But decided to leave it as people think it's still useful to see the actual function name 😅
Regarding the tests, Alvaro mentioned that it's the plan running the tests with public dispatch function?

Alvaro mentioned that it's the plan running the tests with public dispatch function?

Hmm. For most tests that's completely true. The only case where this is undesirable is the avm_simulator.test.ts tests that test single opcodes. Right now they really test contract code that is basically JUST that opcode. If we use public_dispatch there, they will be testing many more opcodes from the dispatch itself. So the tests become less "unit". Not a blocker though.

…_selector

🤖 I have created a new Aztec Packages release --- ## [0.82.1](v0.82.0...v0.82.1) (2025-03-24) ### Features * **avm:** Port field gt to vm2 ([#12883](#12883)) ([0ae6891](0ae6891)) * use msgpack for ClientIvc::Proof in API ([#12911](#12911)) ([1a01602](1a01602)) ### Bug Fixes * disable proving on vite box ([#12971](#12971)) ([69a0fb6](69a0fb6)) * no hardcoded versions in bbup ([#12944](#12944)) ([397144f](397144f)) * pull CRS data ahead of time ([#12945](#12945)) ([43155d6](43155d6)) * Remove workaround ([#12952](#12952)) ([c3337af](c3337af)) * set the correct env var ([#12959](#12959)) ([bd0f4b2](bd0f4b2)) * yolo add bunch of test flakes ([13c19da](13c19da)) * yolo e2e_p2p tests now fully skipped due to huge speed regression ([9141410](9141410)) * yolo txe binds just to localhost by default. ([3933b35](3933b35)) ### Miscellaneous * Change `/bin/bash` shebang to be env based ([#12834](#12834)) ([7843a67](7843a67)) * clean up avm codeowners ([#12860](#12860)) ([35a8f46](35a8f46)) * deflake the kind smoke test ([#12955](#12955)) ([1a37d6d](1a37d6d)), closes [#11177](#11177) * fee cleanup ([#12941](#12941)) ([fdf1da4](fdf1da4)) * Increase bot count ([#12963](#12963)) ([16edd06](16edd06)) * L2 chain config for alpha testnet ([#12962](#12962)) ([e13edb8](e13edb8)) * Reduce bots ([#12953](#12953)) ([4bbc5da](4bbc5da)) * remove selector from public call request ([#12828](#12828)) ([18bcc1b](18bcc1b)) * replace relative paths to noir-protocol-circuits ([61cf4b6](61cf4b6)) * replace relative paths to noir-protocol-circuits ([4356c17](4356c17)) * replace relative paths to noir-protocol-circuits ([f73f47d](f73f47d)) * Set default proving config to true ([#12964](#12964)) ([75c1549](75c1549)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

Revert change from #12828 Unclear how this made tests pass

- Remove `function_selector` from `PublicCallRequest` as it is no longer used. - Remove `public_functions` from `ContractClass`. - `ContractClass` checks that there's at most 1 public function in the artifact. - Rename `args` to `calldata` when it's public calldata that includes the function selector. - Use different generator indexes for calldata and regular args. ### aztec-nr - Rename oracle calls `enqueue_public_function_call_internal ` to `notify_enqueued_public_function_call `, and `set_public_teardown_function_call_internal` to `notify_set_public_teardown_function_call `, to be consistent with other oracle calls. - Storing data to execution cache will need to provide the hash in addition to the preimage, making it possible to use it for different types of data. Currently it's used for calldata and args. ### node - Rename `getContractFunctionName` to `getDebugFunctionName`, as the function name won't always be available. It's set by explicitly calling `registerContractFunctionSignatures` for debugging purpose. - Remove `ExecutionRequest[]` in `Tx` and replace it with `calldata[]`. We now loop through the `publicCallRequests` in the public inputs and pair each with an entry in `calldata[]`. - `DataValidator` checks that calldata are provided for all the non-empty public call requests, and the hash is correct.

🤖 I have created a new Aztec Packages release --- ## [0.82.1](v0.82.0...v0.82.1) (2025-03-24) ### Features * **avm:** Port field gt to vm2 ([#12883](#12883)) ([0ae6891](0ae6891)) * use msgpack for ClientIvc::Proof in API ([#12911](#12911)) ([1a01602](1a01602)) ### Bug Fixes * disable proving on vite box ([#12971](#12971)) ([69a0fb6](69a0fb6)) * no hardcoded versions in bbup ([#12944](#12944)) ([397144f](397144f)) * pull CRS data ahead of time ([#12945](#12945)) ([43155d6](43155d6)) * Remove workaround ([#12952](#12952)) ([c3337af](c3337af)) * set the correct env var ([#12959](#12959)) ([bd0f4b2](bd0f4b2)) * yolo add bunch of test flakes ([13c19da](13c19da)) * yolo e2e_p2p tests now fully skipped due to huge speed regression ([9141410](9141410)) * yolo txe binds just to localhost by default. ([3933b35](3933b35)) ### Miscellaneous * Change `/bin/bash` shebang to be env based ([#12834](#12834)) ([7843a67](7843a67)) * clean up avm codeowners ([#12860](#12860)) ([35a8f46](35a8f46)) * deflake the kind smoke test ([#12955](#12955)) ([1a37d6d](1a37d6d)), closes [#11177](#11177) * fee cleanup ([#12941](#12941)) ([fdf1da4](fdf1da4)) * Increase bot count ([#12963](#12963)) ([16edd06](16edd06)) * L2 chain config for alpha testnet ([#12962](#12962)) ([e13edb8](e13edb8)) * Reduce bots ([#12953](#12953)) ([4bbc5da](4bbc5da)) * remove selector from public call request ([#12828](#12828)) ([18bcc1b](18bcc1b)) * replace relative paths to noir-protocol-circuits ([61cf4b6](61cf4b6)) * replace relative paths to noir-protocol-circuits ([4356c17](4356c17)) * replace relative paths to noir-protocol-circuits ([f73f47d](f73f47d)) * Set default proving config to true ([#12964](#12964)) ([75c1549](75c1549)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

- Remove `function_selector` from `PublicCallRequest` as it is no longer used. - Remove `public_functions` from `ContractClass`. - `ContractClass` checks that there's at most 1 public function in the artifact. - Rename `args` to `calldata` when it's public calldata that includes the function selector. - Use different generator indexes for calldata and regular args. ### aztec-nr - Rename oracle calls `enqueue_public_function_call_internal ` to `notify_enqueued_public_function_call `, and `set_public_teardown_function_call_internal` to `notify_set_public_teardown_function_call `, to be consistent with other oracle calls. - Storing data to execution cache will need to provide the hash in addition to the preimage, making it possible to use it for different types of data. Currently it's used for calldata and args. ### node - Rename `getContractFunctionName` to `getDebugFunctionName`, as the function name won't always be available. It's set by explicitly calling `registerContractFunctionSignatures` for debugging purpose. - Remove `ExecutionRequest[]` in `Tx` and replace it with `calldata[]`. We now loop through the `publicCallRequests` in the public inputs and pair each with an entry in `calldata[]`. - `DataValidator` checks that calldata are provided for all the non-empty public call requests, and the hash is correct.

🤖 I have created a new Aztec Packages release --- ## [0.82.1](v0.82.0...v0.82.1) (2025-03-24) ### Features * **avm:** Port field gt to vm2 ([#12883](#12883)) ([0ae6891](0ae6891)) * use msgpack for ClientIvc::Proof in API ([#12911](#12911)) ([1a01602](1a01602)) ### Bug Fixes * disable proving on vite box ([#12971](#12971)) ([69a0fb6](69a0fb6)) * no hardcoded versions in bbup ([#12944](#12944)) ([397144f](397144f)) * pull CRS data ahead of time ([#12945](#12945)) ([43155d6](43155d6)) * Remove workaround ([#12952](#12952)) ([c3337af](c3337af)) * set the correct env var ([#12959](#12959)) ([bd0f4b2](bd0f4b2)) * yolo add bunch of test flakes ([13c19da](13c19da)) * yolo e2e_p2p tests now fully skipped due to huge speed regression ([9141410](9141410)) * yolo txe binds just to localhost by default. ([3933b35](3933b35)) ### Miscellaneous * Change `/bin/bash` shebang to be env based ([#12834](#12834)) ([7843a67](7843a67)) * clean up avm codeowners ([#12860](#12860)) ([35a8f46](35a8f46)) * deflake the kind smoke test ([#12955](#12955)) ([1a37d6d](1a37d6d)), closes [#11177](#11177) * fee cleanup ([#12941](#12941)) ([fdf1da4](fdf1da4)) * Increase bot count ([#12963](#12963)) ([16edd06](16edd06)) * L2 chain config for alpha testnet ([#12962](#12962)) ([e13edb8](e13edb8)) * Reduce bots ([#12953](#12953)) ([4bbc5da](4bbc5da)) * remove selector from public call request ([#12828](#12828)) ([18bcc1b](18bcc1b)) * replace relative paths to noir-protocol-circuits ([61cf4b6](61cf4b6)) * replace relative paths to noir-protocol-circuits ([4356c17](4356c17)) * replace relative paths to noir-protocol-circuits ([f73f47d](f73f47d)) * Set default proving config to true ([#12964](#12964)) ([75c1549](75c1549)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

LeilaWang added 2 commits March 17, 2025 23:05

Remove selector from public call request.

24f334d

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

ebc807e

…_selector

LeilaWang requested review from IlyasRidhuan, dbanks12, fcarreiro and jeanmon as code owners March 18, 2025 08:46

LeilaWang added 2 commits March 18, 2025 09:52

Fix.

2846aaa

Fix.

a0ae1d2

LeilaWang commented Mar 18, 2025

View reviewed changes

Fix.

5abc49c

LeilaWang commented Mar 18, 2025

View reviewed changes

LeilaWang added 3 commits March 18, 2025 16:16

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

12a565d

…_selector

Fix.

cba5916

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

b85fb0b

…_selector

LeilaWang force-pushed the lw/deprecate_public_selector branch from a090a63 to b85fb0b Compare March 19, 2025 14:32

LeilaWang added 2 commits March 19, 2025 14:35

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

254e414

…_selector

Fix.

19f22f0

LeilaWang requested review from Thunkar and removed request for IlyasRidhuan and jeanmon March 19, 2025 19:26

Thunkar reviewed Mar 20, 2025

View reviewed changes

yarn-project/stdlib/src/contract/artifact_hash.test.ts Show resolved Hide resolved

Thunkar approved these changes Mar 20, 2025

View reviewed changes

fcarreiro approved these changes Mar 21, 2025

View reviewed changes

LeilaWang added 4 commits March 21, 2025 12:34

Address comments.

fcc4e14

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

bf78c94

…_selector

Fix.

d581191

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

caf0ab8

…_selector

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

a9b5f90

…_selector

LeilaWang requested a review from sirasistant as a code owner March 21, 2025 20:00

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

f78895e

…_selector

LeilaWang force-pushed the lw/deprecate_public_selector branch from dd5a267 to 5b19387 Compare March 22, 2025 12:47

Merge remote-tracking branch 'origin/master' into lw/deprecate_public…

b0ead53

…_selector

LeilaWang force-pushed the lw/deprecate_public_selector branch from 5b19387 to b0ead53 Compare March 22, 2025 22:15

Use new civc vks.

f1e9499

LeilaWang merged commit 18bcc1b into master Mar 23, 2025
7 checks passed

LeilaWang deleted the lw/deprecate_public_selector branch March 23, 2025 13:50

AztecBot mentioned this pull request Mar 22, 2025

chore(master): release 0.82.1 #12943

Merged

Thunkar mentioned this pull request Mar 24, 2025

fix: send serializable tx through the wire #12989

Merged

Thunkar added a commit that referenced this pull request Mar 24, 2025

fix: send serializable tx through the wire (#12989)

c6aa376

Revert change from #12828 Unclear how this made tests pass

LeilaWang mentioned this pull request Apr 12, 2025

Switch public execution to public dispatch function #8985

Closed

6 tasks

Conversation

LeilaWang commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

aztec-nr

node

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Thunkar commented Mar 20, 2025

Uh oh!

LeilaWang commented Mar 20, 2025

Uh oh!

Thunkar left a comment

Choose a reason for hiding this comment

Uh oh!

fcarreiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LeilaWang commented Mar 18, 2025 •

edited

Loading