refactor: Update function selector computation #2001

LHerskind · 2023-09-05T10:45:37Z

Fixed #1990. Also adds an oracle to more easily compute selectors from inside the contracts for improved devex.

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

Maddiaa0 · 2023-09-05T11:03:03Z

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

@@ -29,6 +29,7 @@ export const ONE_ACVM_FIELD: ACVMField = `0x${'00'.repeat(Fr.SIZE_IN_BYTES - 1)}
 * The supported oracle names.
 */
 type ORACLE_NAMES =
+  | 'computeSelector'


despite the convinience, computing the selector is something that should be done at compile time, and not at runtime. A malicious app could alter the source code / calls being made without changing the verification key.

In the sandbox a malicious actor can do whatever they want. Calling other functions are also done through oracles where it could just return success independent of what happens. This is essentially just a helper to compute an input to another function that is completely controlled by a possibly malicious app so not sure it changes much there 🤷

I may be missing something, but I agree with @Maddiaa0 on this one. Calling other functions as you say can also return anything, but we then have a kernel circuit that ensures that the oracle behaved properly. But it's true it doesn't affect the security of sandbox as it is today.

I'm fine merging this, but we should add an issue to replace it with a Noir-only implementation in the future (ie before testnet).

I suppose if the sought-after pattern is to have the selector be a parameter to the function, then an oracle is ok. It would be akin to this Solidity pseudocode (because I've forgotten how to write solidity):

function do_an_arbitrary_call(bytes4 selector) { address contract_address = 0x1234...; call(contract_address, selector); }

But if a user actually wants to always call a specific selector, then an oracle call is dangerous, because it isn't constrained:

function do_a_specific_call() { address contract_address = 0x1234...; bytes4 selector = 0x12345678; // this cannot be achieved with an oracle call call(contract_address, selector); }

Allowing users to take over control flow is almost always a death-sin in solidity. I agree with both @spalladino and @Maddiaa0 that having an oracle is horrible, but since you can take over whenever through some of the other execution it seemed like a solution that was usable for this problem. #1550 should get rid of it, it is just used temporarily because it is too shit to compute selectors by hand when you want self-calls etc.

spalladino · 2023-09-05T12:12:57Z

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

@@ -29,6 +29,7 @@ export const ONE_ACVM_FIELD: ACVMField = `0x${'00'.repeat(Fr.SIZE_IN_BYTES - 1)}
 * The supported oracle names.
 */
 type ORACLE_NAMES =
+  | 'computeSelector'


I may be missing something, but I agree with @Maddiaa0 on this one. Calling other functions as you say can also return anything, but we then have a kernel circuit that ensures that the oracle behaved properly. But it's true it doesn't affect the security of sandbox as it is today.

I'm fine merging this, but we should add an issue to replace it with a Noir-only implementation in the future (ie before testnet).

spalladino · 2023-09-05T12:19:45Z

yarn-project/acir-simulator/src/client/simulator.ts

+    let abi: FunctionAbiWithDebugMetadata | undefined = undefined;
+
+    // Brute force
+    for (let i = 1; i < 5; i++) {


I believe we got a MAX_NOTE_FIELDS_LENGTH constant we could use here instead of the 5

spalladino · 2023-09-05T12:20:30Z

yarn-project/acir-simulator/src/client/function_selectors.ts

+export const computeNoteHashAndNullifierSignature = 'compute_note_hash_and_nullifier(Field,Field,Field,[Field;3])';

 export const computeNoteHashAndNullifierSelector = FunctionSelector.fromSignature(computeNoteHashAndNullifierSignature);


Can we remove these, given they are replaced by the loop?

spalladino · 2023-09-05T12:21:08Z

yarn-project/end-to-end/src/e2e_lending_contract.test.ts

@@ -88,6 +88,7 @@ describe('e2e_lending_contract', () => {
    stableCoin: NativeTokenContract,
    account: Account,
  ) => {
+    await sleep(5000);


Is this needed? If so, let's add a comment on why.

Ah, no was when debugging and thinking it was due to race-conditions, will remove it.

spalladino · 2023-09-05T12:27:27Z

yarn-project/foundation/src/abi/function_selector.ts

+    // If you are debugging, can be useful to uncomment the following line.
+    // console.log(`Function selector for ${signature} is ${selector}`);


Why not use a debug logger, so it can be turned on with an env var? Even better, we can have our users turn that on without having to change the code.

I'm not sure why. But if I use the debug logger in here. It breaks jest toEqual and typing in the server_world_state_synchroniser.

It probably breaks if you add it as an instance variable, but you can always create a new one as a local variable for this function and then ditch it.

spalladino · 2023-09-05T12:29:08Z

yarn-project/foundation/src/abi/decoder.ts

+export class FunctionSignatureDecoder {
+  constructor(private name: string, private parameters: ABIParameter[]) {}


I know this is a matter of personal preference, but I prefer to export plain functions rather than classes with a "do" method. A user of this API just wants to decodeFunctionSignature(name, parameters), and doesn't care about the FunctionSignatureDecoder that's doing the job.

Added decodeFunctionSignature to handle it

spalladino · 2023-09-05T12:30:42Z

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

@@ -355,7 +337,7 @@ contract NativeToken {
    // Note 2: Having it in all the contracts gives us the ability to compute the note hash and nullifier differently for different kind of notes.
    unconstrained fn compute_note_hash_and_nullifier(contract_address: Field, nonce: Field, storage_slot: Field, preimage: [Field; VALUE_NOTE_LEN]) -> [Field; 4] {
        let note_header = NoteHeader { contract_address, nonce, storage_slot };
-        if (storage_slot == 2) {
+        if (storage_slot == 3) {


Curious how you found this

Had failures because of missing compute_note_hash_and_nullifier so debugged from there and saw that the slot did not match what was in storage. The actual issue was then the different selectors 😬 leading to the loop.

Maddiaa0 · 2023-09-05T12:52:58Z

yarn-project/foundation/src/abi/decoder.ts

+   * @returns The function signature.
+   */
+  public decode(): string {
+    const hmm = this.parameters.map(param => this.decodeParameter(param.type));


guy skibbidy

LHerskind · 2023-09-05T14:14:18Z

yarn-project/acir-simulator/src/client/private_execution.test.ts

@@ -815,7 +819,7 @@ describe('Private Execution test suite', () => {
      );

      expect(result.enqueuedPublicFunctionCalls).toHaveLength(1);
-      expect(result.enqueuedPublicFunctionCalls[0]).toEqual(publicCallRequest);
+      expect(result.enqueuedPublicFunctionCalls[0].toBuffer()).toEqual(publicCallRequest.toBuffer());


For some reason, the toEqual was failing with the objects directly, even though they serialise to the same values 🤷

🤖 I have created a new Aztec Packages release --- ## [0.1.0-alpha60](v0.1.0-alpha59...v0.1.0-alpha60) (2023-09-06) ### Features * Goblin recursive verifier ([#1822](#1822)) ([f962cb6](f962cb6)) * initial `is_valid` eip1271 style wallet + minimal test changes ([#1935](#1935)) ([f264c54](f264c54)) ### Bug Fixes * benchmark git repo ([#2041](#2041)) ([3c696bb](3c696bb)) * cli canary & deployment ([#2053](#2053)) ([1ddd24a](1ddd24a)) * **rpc:** Fixes getNodeInfo serialisation ([#1991](#1991)) ([0a29fa8](0a29fa8)) ### Miscellaneous * **circuits:** - use msgpack for cbind routines of native private kernel circuits ([#1938](#1938)) ([3dc5c07](3dc5c07)) * **docs:** API docs stucture ([#2014](#2014)) ([9aab9dd](9aab9dd)) * Update function selector computation ([#2001](#2001)) ([e07ea1a](e07ea1a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

LHerskind force-pushed the lh/1990-function-selector branch from 95a0115 to 330ae13 Compare September 5, 2023 10:46

LHerskind requested review from spalladino, iAmMichaelConnor and Maddiaa0 September 5, 2023 10:48

Maddiaa0 reviewed Sep 5, 2023

View reviewed changes

LHerskind force-pushed the lh/1990-function-selector branch from 330ae13 to d391150 Compare September 5, 2023 12:12

spalladino reviewed Sep 5, 2023

View reviewed changes

Maddiaa0 reviewed Sep 5, 2023

View reviewed changes

LHerskind mentioned this pull request Sep 5, 2023

[Noir Contracts] Address cyclic dependency preventing generation of contract interface for an internal public function. #1550

Closed

LHerskind force-pushed the lh/1990-function-selector branch from cb42729 to 6732d6f Compare September 5, 2023 14:09

LHerskind commented Sep 5, 2023

View reviewed changes

LHerskind requested review from Maddiaa0 and spalladino September 5, 2023 15:20

spalladino approved these changes Sep 5, 2023

View reviewed changes

LHerskind added 5 commits September 5, 2023 21:27

feat: Update selector computation

c8e7913

feat: add oracle for selector computation

3af400a

feat: brute force compute_note_hash_and_nullifier selector

e6eeb2b

chore: address comments + minor test issue with toEqual

0a4b2fd

confusion: removing the debuglogger make jest works

92c0b18

LHerskind force-pushed the lh/1990-function-selector branch from ffe1078 to 92c0b18 Compare September 5, 2023 20:27

LHerskind enabled auto-merge (squash) September 5, 2023 20:38

LHerskind merged commit e07ea1a into master Sep 5, 2023

LHerskind deleted the lh/1990-function-selector branch September 5, 2023 20:45

github-actions bot mentioned this pull request Sep 5, 2023

chore(master): release 0.1.0-alpha60 #2040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Update function selector computation #2001

refactor: Update function selector computation #2001

LHerskind commented Sep 5, 2023

Maddiaa0 Sep 5, 2023

LHerskind Sep 5, 2023

spalladino Sep 5, 2023

iAmMichaelConnor Sep 5, 2023 •

edited

Loading

LHerskind Sep 5, 2023

spalladino Sep 5, 2023

spalladino Sep 5, 2023

spalladino Sep 5, 2023

spalladino Sep 5, 2023

LHerskind Sep 5, 2023

spalladino Sep 5, 2023

LHerskind Sep 5, 2023

spalladino Sep 5, 2023

spalladino Sep 5, 2023

LHerskind Sep 5, 2023

spalladino Sep 5, 2023

LHerskind Sep 5, 2023

Maddiaa0 Sep 5, 2023

LHerskind Sep 5, 2023

LHerskind Sep 5, 2023

		export const computeNoteHashAndNullifierSignature = 'compute_note_hash_and_nullifier(Field,Field,Field,[Field;3])';

		export const computeNoteHashAndNullifierSelector = FunctionSelector.fromSignature(computeNoteHashAndNullifierSignature);

		// If you are debugging, can be useful to uncomment the following line.
		// console.log(`Function selector for ${signature} is ${selector}`);

		export class FunctionSignatureDecoder {
		constructor(private name: string, private parameters: ABIParameter[]) {}

refactor: Update function selector computation #2001

refactor: Update function selector computation #2001

Conversation

LHerskind commented Sep 5, 2023

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iAmMichaelConnor Sep 5, 2023 • edited Loading

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

iAmMichaelConnor Sep 5, 2023 •

edited

Loading