Skip to content
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

refactor: Update function selector computation #2001

Merged
merged 5 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions yarn-project/acir-simulator/src/acvm/acvm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const ONE_ACVM_FIELD: ACVMField = `0x${'00'.repeat(Fr.SIZE_IN_BYTES - 1)}
* The supported oracle names.
*/
type ORACLE_NAMES =
| 'computeSelector'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@iAmMichaelConnor iAmMichaelConnor Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| 'packArguments'
| 'getSecretKey'
| 'getNote'
Expand Down
5 changes: 0 additions & 5 deletions yarn-project/acir-simulator/src/client/function_selectors.ts

This file was deleted.

5 changes: 5 additions & 0 deletions yarn-project/acir-simulator/src/client/private_execution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ export class PrivateFunctionExecution {
const unencryptedLogs = new FunctionL2Logs([]);

const { partialWitness } = await acvm(await AcirSimulator.getSolver(), acir, initialWitness, {
computeSelector: (...args) => {
const signature = oracleDebugCallToFormattedStr(args);
const returnValue = toACVMField(FunctionSelector.fromSignature(signature).toField());
return Promise.resolve(returnValue);
},
packArguments: async args => {
return toACVMField(await this.context.packedArgsCache.pack(args.map(fromACVMField)));
},
Expand Down
26 changes: 18 additions & 8 deletions yarn-project/acir-simulator/src/client/simulator.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CallContext, CircuitsWasm, FunctionData, TxContext } from '@aztec/circuits.js';
import { CallContext, CircuitsWasm, FunctionData, MAX_NOTE_FIELDS_LENGTH, TxContext } from '@aztec/circuits.js';
import { computeTxHash } from '@aztec/circuits.js/abis';
import { Grumpkin } from '@aztec/circuits.js/barretenberg';
import { ArrayType, FunctionType, encodeArguments } from '@aztec/foundation/abi';
import { ArrayType, FunctionSelector, FunctionType, encodeArguments } from '@aztec/foundation/abi';
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { EthAddress } from '@aztec/foundation/eth-address';
import { Fr } from '@aztec/foundation/fields';
Expand All @@ -15,7 +15,6 @@ import { PackedArgsCache } from '../common/packed_args_cache.js';
import { ClientTxExecutionContext } from './client_execution_context.js';
import { DBOracle, FunctionAbiWithDebugMetadata } from './db_oracle.js';
import { ExecutionResult } from './execution_result.js';
import { computeNoteHashAndNullifierSelector, computeNoteHashAndNullifierSignature } from './function_selectors.js';
import { PrivateFunctionExecution } from './private_execution.js';
import { UnconstrainedFunctionExecution } from './unconstrained_execution.js';

Expand Down Expand Up @@ -177,12 +176,23 @@ export class AcirSimulator {
storageSlot: Fr,
notePreimage: Fr[],
) {
let abi: FunctionAbiWithDebugMetadata;
try {
abi = await this.db.getFunctionABI(contractAddress, computeNoteHashAndNullifierSelector);
} catch (e) {
let abi: FunctionAbiWithDebugMetadata | undefined = undefined;

// Brute force
for (let i = 0; i < MAX_NOTE_FIELDS_LENGTH; i++) {
const signature = `compute_note_hash_and_nullifier(Field,Field,Field,[Field;${i}])`;
const selector = FunctionSelector.fromSignature(signature);
try {
abi = await this.db.getFunctionABI(contractAddress, selector);
if (abi !== undefined) break;
} catch (e) {
// ignore
}
}

if (abi == undefined) {
throw new Error(
`Mandatory implementation of "${computeNoteHashAndNullifierSignature}" missing in noir contract ${contractAddress.toString()}.`,
`Mandatory implementation of "compute_note_hash_and_nullifier" missing in noir contract ${contractAddress.toString()}.`,
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { CallContext, FunctionData } from '@aztec/circuits.js';
import { DecodedReturn, decodeReturnValues } from '@aztec/foundation/abi';
import { DecodedReturn, FunctionSelector, decodeReturnValues } from '@aztec/foundation/abi';
import { AztecAddress } from '@aztec/foundation/aztec-address';
import { Fr } from '@aztec/foundation/fields';
import { createDebugLogger } from '@aztec/foundation/log';
Expand Down Expand Up @@ -48,6 +48,11 @@ export class UnconstrainedFunctionExecution {
const initialWitness = toACVMWitness(1, this.args);

const { partialWitness } = await acvm(await AcirSimulator.getSolver(), acir, initialWitness, {
computeSelector: (...args) => {
const signature = oracleDebugCallToFormattedStr(args);
const returnValue = toACVMField(FunctionSelector.fromSignature(signature).toField());
return Promise.resolve(returnValue);
},
getSecretKey: ([ownerX], [ownerY]) => this.context.getSecretKey(this.contractAddress, ownerX, ownerY),
getPublicKey: async ([acvmAddress]) => {
const address = frToAztecAddress(fromACVMField(acvmAddress));
Expand Down
5 changes: 4 additions & 1 deletion yarn-project/acir-simulator/src/public/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,13 @@ export class PublicExecutor {
// We use this cache to hold the packed arguments.
const packedArgs = await PackedArgsCache.create([]);
const { partialWitness } = await acvm(await AcirSimulator.getSolver(), acir, initialWitness, {
computeSelector: (...args) => {
const signature = oracleDebugCallToFormattedStr(args);
return Promise.resolve(toACVMField(FunctionSelector.fromSignature(signature).toField()));
},
packArguments: async args => {
return toACVMField(await packedArgs.pack(args.map(fromACVMField)));
},

debugLog: (...args) => {
this.log(oracleDebugCallToFormattedStr(args));
return Promise.resolve(ZERO_ACVM_FIELD);
Expand Down
2 changes: 1 addition & 1 deletion yarn-project/end-to-end/src/e2e_lending_contract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('e2e_lending_contract', () => {
logger(`Tx sent with hash ${await tx.getTxHash()}`);
const receipt = await tx.wait();
expect(receipt.status).toBe(TxStatus.MINED);
logger(`Debt asset deployed to ${receipt.contractAddress}`);
logger(`Stable coin asset deployed to ${receipt.contractAddress}`);
stableCoin = await NativeTokenContract.at(receipt.contractAddress!, wallet);
}

Expand Down
52 changes: 51 additions & 1 deletion yarn-project/foundation/src/abi/decoder.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ABIType, FunctionAbi } from '@aztec/foundation/abi';
import { ABIParameter, ABIType, FunctionAbi } from '@aztec/foundation/abi';
import { Fr } from '@aztec/foundation/fields';

/**
Expand Down Expand Up @@ -86,3 +86,53 @@ class ReturnValuesDecoder {
export function decodeReturnValues(abi: FunctionAbi, returnValues: Fr[]) {
return new ReturnValuesDecoder(abi, returnValues.slice()).decode();
}

/**
* Decodes the signature of a function from the name and parameters.
*/
export class FunctionSignatureDecoder {
constructor(private name: string, private parameters: ABIParameter[]) {}
Comment on lines +93 to +94
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added decodeFunctionSignature to handle it


/**
* Decodes a single function parameter for the function signature.
* @param param - The parameter to decode.
* @returns A string representing the parameter type.
*/
private decodeParameter(param: ABIType): string {
switch (param.kind) {
case 'field':
return 'Field';
case 'integer':
if (param.sign === 'signed') {
throw new Error('Unsupported type: signed integer');
}
return `u${param.width}`;
case 'boolean':
return 'bool';
case 'array':
return `[${this.decodeParameter(param.type)};${param.length}]`;
case 'struct':
return `(${param.fields.map(field => `${this.decodeParameter(field.type)}`).join(',')})`;
default:
throw new Error(`Unsupported type: ${param.kind}`);
}
}

/**
* Decodes all the parameters and build the function signature
* @returns The function signature.
*/
public decode(): string {
return `${this.name}(${this.parameters.map(param => this.decodeParameter(param.type)).join(',')})`;
}
}

/**
* Decodes a function signature from the name and parameters.
* @param name - The name of the function-
* @param parameters - The parameters of the function.
* @returns - The function signature.
*/
export function decodeFunctionSignature(name: string, parameters: ABIParameter[]) {
return new FunctionSignatureDecoder(name, parameters).decode();
}
9 changes: 6 additions & 3 deletions yarn-project/foundation/src/abi/function_selector.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ABIParameter } from '@aztec/foundation/abi';
import { ABIParameter, decodeFunctionSignature } from '@aztec/foundation/abi';
import { toBigIntBE, toBufferBE } from '@aztec/foundation/bigint-buffer';
import { keccak } from '@aztec/foundation/crypto';
import { Fr } from '@aztec/foundation/fields';
Expand Down Expand Up @@ -97,8 +97,11 @@ export class FunctionSelector {
* @returns A Buffer containing the 4-byte function selector.
*/
static fromNameAndParameters(name: string, parameters: ABIParameter[]) {
const signature = name === 'constructor' ? name : `${name}(${parameters.map(p => p.type.kind).join(',')})`;
return FunctionSelector.fromSignature(signature);
const signature = decodeFunctionSignature(name, parameters);
const selector = FunctionSelector.fromSignature(signature);
// If using the debug logger here it kill the typing in the `server_world_state_synchroniser` and jest tests.
// console.log(`Function selector for ${signature} is ${selector}`);
return selector;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ contract Child {
use crate::storage::Storage;
use dep::aztec::{
abi::CallContext,
oracle::logs::emit_unencrypted_log,
oracle::{
logs::emit_unencrypted_log,
compute_selector::compute_selector,
}
};
use dep::std::option::Option;

Expand Down Expand Up @@ -34,7 +37,7 @@ contract Child {
input + context.chain_id() + context.version()
}

// Returns base_value + 42.
// Returns base_value + chain_id + version + block_number + timestamp
#[aztec(public)]
fn pubGetValue(base_value: Field) -> Field {
let returnValue = base_value + context.chain_id() + context.version() + context.block_number() + context.timestamp();
Expand Down Expand Up @@ -77,7 +80,7 @@ contract Child {

#[aztec(public)]
fn setValueTwiceWithNestedFirst() {
let pubSetValueSelector = 0x5b0f91b0;
let pubSetValueSelector = compute_selector("pubSetValue(Field)");
let _ret = context.call_public_function(context.this_address(), pubSetValueSelector, [10]);

let storage = Storage::init(Option::none(), Option::some(&mut context));
Expand All @@ -91,7 +94,7 @@ contract Child {
storage.current_value.write(20);
let _hash = emit_unencrypted_log(20);

let pubSetValueSelector = 0x5b0f91b0;
let pubSetValueSelector = compute_selector("pubSetValue(Field)");
let _ret = context.call_public_function(context.this_address(), pubSetValueSelector, [10]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use dep::aztec::context::{

use crate::storage::Asset;
use dep::aztec::constants_gen::RETURN_VALUES_LENGTH;
use dep::aztec::oracle::compute_selector::compute_selector;

struct PriceFeed {
address: Field,
Expand All @@ -20,7 +21,7 @@ impl PriceFeed {
fn get_price(self: Self, context: PublicContext) -> u120 {
let return_values = context.call_public_function(
self.address,
3359284436,
compute_selector("get_price(Field)"),
[0]
);

Expand All @@ -40,23 +41,23 @@ impl Token {
fn transfer_pub(self: Self, context: PublicContext, to: Field, amount: Field) {
let _transfer_return_values = context.call_public_function(
self.address,
1012824788,
compute_selector("transfer_pub(Field,Field)"),
[to, amount]
);
}

fn transfer_from_pub(self: Self, context: PublicContext, from: Field, to: Field, amount: Field) {
let _transfer_return_values = context.call_public_function(
self.address,
1602017294,
self.address,
compute_selector("transfer_from_pub(Field,Field,Field)"),
[from, to, amount]
);
}

fn owner_mint_pub(self: Self, context: PublicContext, to: Field, amount: Field) {
let _transfer_return_values = context.call_public_function(
self.address,
1071038680,
compute_selector("owner_mint_pub(Field,Field)"),
[to, amount]
);
}
Expand All @@ -65,7 +66,7 @@ impl Token {
fn unshield(self: Self, context: &mut PrivateContext, from: Field, to: Field, amount: Field) -> [Field; RETURN_VALUES_LENGTH] {
context.call_private_function(
self.address,
2423803924,
compute_selector("unshieldTokens(Field,Field,Field)"),
[from, to, amount]
)
}
Expand All @@ -82,8 +83,8 @@ impl Lending {

fn update_accumulator(self: Self, context: PublicContext) -> Asset {
let return_values = context.call_public_function_no_args(
self.address,
0x1873b536
self.address,
compute_selector("update_accumulator()"),
);

Asset {
Expand Down
Loading