chore!: Remove open keyword from Noir

[spalladino]

Removes the unused open keyword from Noir, as requested here.

Breaking change: This PR changes the format of Noir contract build artifacts, so a full rebuild of contracts is needed after this PR is merged. No other action is needed by the user. If they fail to do it, they'll see a message along the lines of No custom attributes found for contract function foo. Try rebuilding the contract with the latest nargo version. (cc @AztecProtocol/devrel @rahul-kothari)

[TomAFrench]

Is the "open" concept completely unused by aztec then? If so can you remove it from the compiler?

[AztecBot]

Benchmark results

Metrics with a significant change:

• tx_sequencer_processing_time_ms (0): 11.6 (+51%)
• note_successful_decrypting_time_in_ms (32): 574 (+20%)
• note_trial_decrypting_time_in_ms (64): 113 (+222%)

Detailed results

All benchmarks are run on txs on the Benchmarking contract on the repository. Each tx consists of a batch call to create_note and increment_balance, which guarantees that each tx has a private call, a nested private call, a public call, and a nested public call, as well as an emitted private note, an unencrypted log, and public storage read and write.

This benchmark source data is available in JSON format on S3 here.

Values are compared against data from master at commit 7f216eb0 and shown if the difference exceeds 1%.

L2 block published to L1

Each column represents the number of txs on an L2 block published to L1.

Metric	8 txs	32 txs	64 txs
l1_rollup_calldata_size_in_bytes	5,668	18,820	36,356
l1_rollup_calldata_gas	65,992	238,696	467,900
l1_rollup_execution_gas	666,472	953,405	1,335,056
l2_block_processing_time_in_ms	1,328	4,787	9,427
note_successful_decrypting_time_in_ms	232 (+4%)	⚠️ 574 (+20%)	981 (-2%)
note_trial_decrypting_time_in_ms	37.5 (+39%)	87.5 (-10%)	⚠️ 113 (+222%)
l2_block_building_time_in_ms	12,362 (+1%)	48,922 (+1%)	98,322 (+1%)
l2_block_rollup_simulation_time_in_ms	7,305	29,050	58,536 (+1%)
l2_block_public_tx_process_time_in_ms	5,027 (+2%)	19,808 (+2%)	39,618 (+2%)

L2 chain processing

Each column represents the number of blocks on the L2 chain where each block has 16 txs.

Metric	5 blocks	10 blocks
node_history_sync_time_in_ms	14,377 (+1%)	27,722 (-1%)
note_history_successful_decrypting_time_in_ms	1,219 (-5%)	2,425 (-2%)
note_history_trial_decrypting_time_in_ms	63.3 (-45%)	152 (-20%)
node_database_size_in_bytes	18,636,880	35,315,792
pxe_database_size_in_bytes	29,859	59,414

Circuits stats

Stats on running time and I/O sizes collected for every circuit run across all benchmarks.

Circuit	circuit_simulation_time_in_ms	circuit_input_size_in_bytes	circuit_output_size_in_bytes
private-kernel-init	272	44,338	27,700
private-kernel-ordering	206	52,324	14,326
base-rollup	666 (+1%)	177,083	925
root-rollup	68.7 (+1%)	4,176	789
private-kernel-inner	359	73,197	27,700
public-kernel-app-logic	179	34,582	27,671
public-kernel-tail	91.9 (+1%)	34,206	27,671
merge-rollup	8.34	2,696	925

Tree insertion stats

The duration to insert a fixed batch of leaves into each tree type.

Metric	1 leaves	16 leaves	64 leaves	128 leaves	512 leaves	1024 leaves	2048 leaves	4096 leaves	32 leaves
batch_insert_into_append_only_tree_16_depth_ms	10.0 (+1%)	16.0 (-1%)	N/A	N/A	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_16_depth_hash_count	16.8	31.6	N/A	N/A	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_16_depth_hash_ms	0.585 (+1%)	0.494 (-1%)	N/A	N/A	N/A	N/A	N/A	N/A	N/A
batch_insert_into_append_only_tree_32_depth_ms	N/A	N/A	46.8 (+2%)	73.5	232 (+1%)	450 (+1%)	866	1,723 (-1%)	N/A
batch_insert_into_append_only_tree_32_depth_hash_count	N/A	N/A	96.0	159	543	1,055	2,079	4,127	N/A
batch_insert_into_append_only_tree_32_depth_hash_ms	N/A	N/A	0.481 (+2%)	0.454	0.423 (+1%)	0.422 (+1%)	0.413	0.414	N/A
batch_insert_into_indexed_tree_20_depth_ms	N/A	N/A	54.2 (+1%)	106	336 (+1%)	661	1,315	2,621 (+1%)	N/A
batch_insert_into_indexed_tree_20_depth_hash_count	N/A	N/A	104	207	691	1,363	2,707	5,395	N/A
batch_insert_into_indexed_tree_20_depth_hash_ms	N/A	N/A	0.483 (+1%)	0.481	0.458 (+1%)	0.456	0.456	0.456 (+1%)	N/A
batch_insert_into_indexed_tree_40_depth_ms	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	61.8 (+1%)
batch_insert_into_indexed_tree_40_depth_hash_count	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	109
batch_insert_into_indexed_tree_40_depth_hash_ms	N/A	N/A	N/A	N/A	N/A	N/A	N/A	N/A	0.542 (+1%)

Miscellaneous

Transaction sizes based on how many contract classes are registered in the tx.

Metric	0 registered classes
tx_size_in_bytes	19,191

Transaction processing duration by data writes.

Metric	0 new note hashes	1 new note hashes
tx_pxe_processing_time_ms	2,834	1,435

Metric	0 public data writes	1 public data writes
tx_sequencer_processing_time_ms	⚠️ 11.6 (+51%)	614 (+1%)

[spalladino]

I understand the is_open flag for a function is still in use, as it affects its target (ACIR vs Brillig/AVM) (@Maddiaa0 please correct me if I'm wrong). The only difference is that this is now specified by using the aztec(public) macro over the open keyword.

[spalladino]

@TomAFrench I've had to add an is_unconstrained flag to the function output. The custom_attributes alone were not enough to differentiate between an "unconstrained contract function" (ie a getter, which is signalled by marking the function with the unconstrained noir keyword) and a manually-written private function (ie one written manually and without the usage of macros). Let me know what you think.

Long term, maybe a better approach would be to require all contract functions to be decorated with one of aztec(private|public|getter), and functions that do not have any of these modifiers are deemed internal to the contract (what today is contract_library_function).

[spalladino]

@TomAFrench this PR is failing because some contracts started exceeding the max bytecode length. I checked and indeed public functions are now about 10-15% bigger. For instance, in the token contract compiled in this PR:

$ yarn aztec-cli inspect-contract TokenContractArtifact | grep -A3 transfer
open transfer_public(from: (inner: Field), to: (inner: Field), amount: Field, nonce: Field) 
	function signature: transfer_public((Field),(Field),Field,Field)
	selector: 0x7db8f449
	bytecode: 22515 bytes (sha256 81af6e02824d206d4a1d87e084bb3ff6bf04bfabae2686ac0d5379c00f7250fa)
--
secret transfer(from: (inner: Field), to: (inner: Field), amount: Field, nonce: Field) 
	function signature: transfer((Field),(Field),Field,Field)
	selector: 0xb1aa668a
	bytecode: 694454 bytes (sha256 8a01f0f8da7f8aba6551de8c2bd4f42e1176eca140a39bb631d42f6ac7bd5698)

Whereas in master:

$  yarn aztec-cli inspect-contract TokenContractArtifact | grep -A3 transfer
secret transfer(from: (inner: Field), to: (inner: Field), amount: Field, nonce: Field) 
	function signature: transfer((Field),(Field),Field,Field)
	selector: 0xb1aa668a
	bytecode: 693619 bytes (sha256 bb9b31696b92afdfb36e8aff8f7704689fcdce932f177c1cabad08931363fda1)
--
open transfer_public(from: (inner: Field), to: (inner: Field), amount: Field, nonce: Field) 
	function signature: transfer_public((Field),(Field),Field,Field)
	selector: 0x7db8f449
	bytecode: 19712 bytes (sha256 e520866d0d4c5655b48e4aa6a79f3182aaa74514e20a99de238c1f019916df6b)

Is it possible that by removing the open flag in functions we're accidentally compiling public functions as ACIR instead of Brillig? How could I verify this easily? If not, what do you think could be the issue?

[TomAFrench]

@spalladino, the selectors of the two functions are swapping between the two branches, is that expected? I'm wondering if there's some confounding factor here as I can't see anything that would result in an open function being compiled to acir. Misread, nevermind.

[TomAFrench]

I've compiled the contract

contract Counter {
    // docs:start:imports
    use dep::aztec::prelude::{AztecAddress, Map};
    use dep::aztec::context::Context;
    use dep::value_note::{balance_utils, value_note::{ValueNote, VALUE_NOTE_LEN}};
    use dep::easy_private_state::EasyPrivateUint;
    // docs:end:imports

    // docs:start:storage_struct
    // struct Storage {
    //     counters: Map<AztecAddress, EasyPrivateUint>,
    // }
    // docs:end:storage_struct

    // docs:start:constructor
    #[aztec(private)]
    #[aztec(initializer)]
    fn constructor(headstart: u64, owner: AztecAddress) {
        // let counters = storage.counters;
        // counters.at(owner).add(headstart, owner);
    }

    // docs:start:increment
    // #[aztec(private)]
    //   fn increment(owner: AztecAddress) {
    //     let counters = storage.counters;
    //     counters.at(owner).add(1, owner);
    // }
    // docs:end:increment

    // docs:start:get_counter
    #[aztec(public-vm)]
    fn get_counter(owner: AztecAddress) -> pub Field {
        assert(owner == AztecAddress::empty());
        2
    }
    // docs:end:get_counter
}

This gives the snipped output below (can be done with the --print-acir flag) which consists of a single brillig opcode so we're definitely not accidentally compiling it as if it were constrained.

Compiled ACIR for Counter::avm_get_counter (unoptimized):
current witness index : 1
public parameters indices : []
return value indices : [1]
BRILLIG: inputs: [Single(Expression { mul_terms: [], linear_combinations: [(1, Witness(0))], q_c: 0 })]
outputs: [Simple(Witness(1))]
[Const { destination: MemoryAddress(0), bit_size: 64, value: Value { inner: 1026 } }, CalldataCopy { destination_address: MemoryAddress(1024), size: 1, offset: 0 }, Mov { destination: MemoryAddress(2), source: MemoryAddress(1024) }, Call { location: 6 }, Mov { destination: MemoryAddress(1025), source: MemoryAddress(2) }, Stop { return_data_offset: 1025, return_data_size: 1 }, Const { destination: MemoryAddress(4), bit_size: 254, value: Value { inner: 0 } }, BinaryFieldOp { destination: MemoryAddress(3), op: Equals, lhs: MemoryAddress(2), rhs: MemoryAddress(4) }, JumpIf { condition: MemoryAddress(3), location: 10 }, Trap, Const { destination: MemoryAddress(3), bit_size: 254, value: Value { inner: 2 } }, Mov { destination: MemoryAddress(5), source: MemoryAddress(3) }, Mov { destination: MemoryAddress(2), source: MemoryAddress(5) }, Return]

[TomAFrench]

What's counted as "contract bytecode"? We're obviously putting more data into the JSON file artifact but I'm assuming you're not just dumping that directly onchain right?

[spalladino]

What's counted as "contract bytecode"? We're obviously putting more data into the JSON file artifact but I'm assuming you're not just dumping that directly onchain right?

No, contract bytecode is the concatenation of the selector, internal flag, bytecode length, and bytecode for each public function:

export function packBytecode(publicFns: ContractClass['publicFunctions']): Buffer {
  return serializeArrayOfBufferableToVector(
    publicFns.map(fn => serializeToBuffer(fn.selector, fn.isInternal, numToInt32BE(fn.bytecode.length), fn.bytecode)),
  );
}

From circuits.js/src/contract/public_bytecode.ts

[spalladino]

Sorry, I'm a bit confused: isn't the public-vm attribute a new one, used for AVM transpilation, and still not used in the noir-contracts?

[TomAFrench]

I'm confused. I thought that open functions were functions that ran in an open context (i.e. on the public VM) what do they actually do?

[TomAFrench]

This implies that open is public vm.

[spalladino]

I'm confused. I thought that open functions were functions that ran in an open context (i.e. on the public VM) what do they actually do?

The public VM doesn't exist yet, so today public functions are just Brillig that gets run on the sequencer, but not proven.

[TomAFrench]

Ok, I'm not sure what's happening in that case. From looking at the build artifacts on this branch compared to master however we're getting the same bytecode out for public functions so it seems like the issue is on how aztec is ingesting these artifacts.

[spalladino]

Got it, I'll look it from Aztec's side on Monday. Thanks for looking into this Tom!

[TomAFrench]

Ah, good catch!

[TomAFrench]

Looks like circuits.js isn't recompiling some contracts it's using?