feat: Correct circuit construction from acir

[ledwards2225]

This work attempts to add a robust means for construction of bberg circuits from acir data. The difficulty comes primarily from different assumptions about constant variables added in the builder constructors.

The problem is essentially that constraints are constructed from acir in two ways: directly and indirectly. Directly means the witness values themselves are known at the point of acir generation. In this case gates are specified directly via indices into a "witness" array. Most constraints are indirect in the sense that bberg is presented with an acir opcode, a sha hash say, and has to turn that into constraints using internal bberg mechanisms. This leads to issues when the "witness" array (and acir object) and the "variables" array (a bberg builder object) are offset from one another, as can happen when constant variables are added in the bberg builder constructors. The issue is further complicated by the fact that noir applies a PRE-offset (see here) to the indices in the directly specified gates to account for the fact that the Ultra builder historically added a single constant in its constructor. The GUH builder adds 3 more, which is how this issue arose in the first place. However the issue would have come up regardless once we removed that +1 from noir which everyone agrees should not be there.

The solution was to add a new builder constructor that takes some data from acir (witness, public_inputs, etc.), populates the analogous structures in the builder, and then adds the necessary constant variables. This means that the indices encoded into explicit gates from acir correctly index into builder.variables, regardless of how many constants are added subsequently. This solution will also work once we remove the +1 from noir, with one additional tweak in bberg that is clearly indicated in the code.

Closes AztecProtocol/barretenberg#815

[AztecBot]

Benchmark results

No metrics with a significant change found.

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 c4153936 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	128 txs
l1_rollup_calldata_size_in_bytes	45,444	179,588	716,132
l1_rollup_calldata_gas	223,020	868,292	3,449,972
l1_rollup_execution_gas	867,129	3,746,345	23,306,737
l2_block_processing_time_in_ms	1,323 (-14%)	5,095 (-3%)	20,927
note_successful_decrypting_time_in_ms	357 (-5%)	1,108 (-1%)	4,254 (+4%)
note_trial_decrypting_time_in_ms	66.8 (-20%)	36.1 (-1%)	132 (-3%)
l2_block_building_time_in_ms	14,995 (+1%)	59,874 (+2%)	240,107 (+1%)
l2_block_rollup_simulation_time_in_ms	10,650 (+1%)	42,612 (+1%)	171,154 (+1%)
l2_block_public_tx_process_time_in_ms	4,315 (+1%)	17,185 (+2%)	68,694 (+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	15,266 (+2%)	29,333 (+2%)
note_history_successful_decrypting_time_in_ms	2,765 (+5%)	5,632 (+6%)
note_history_trial_decrypting_time_in_ms	117 (+41%)	149 (+3%)
node_database_size_in_bytes	3,621,947	3,725,396
pxe_database_size_in_bytes	29,923	59,478

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	211 (+1%)	44,709	25,561
private-kernel-ordering	138 (+1%)	30,953	14,809
base-rollup	1,080 (+1%)	128,374	881
root-rollup	84.8 (+3%)	4,088	889
private-kernel-inner	289 (+1%)	71,236	25,561
public-kernel-private-input	204 (+1%)	31,891	25,561
public-kernel-non-first-iteration	202 (+2%)	31,933	25,561
merge-rollup	9.90 (+2%)	2,608	881

Miscellaneous

Transaction sizes based on how many contracts are deployed in the tx.

Metric	0 deployed contracts	1 deployed contracts
tx_size_in_bytes	15,487	31,879

[ludamad]

LGTM other than two minor notes, nice work

[kevaundray]

Suggested change

	// properly indexes into witness bc we're about to make calls like add_variable(witness[public_inputs[idx]]). Once
	// properly indexes into witness because we're about to make calls like add_variable(witness[public_inputs[idx]]). Once

[ledwards2225]

thx

[kevaundray]

Is there an issue open for this? Its not blocking this PR as it was already there before this PR was merged.

Would be good to clean this up in a near-future PR as having TestingUtils being used anywhere except for tests seems like a footgun

[ledwards2225]

Added the relevant issue. The hope is to get all of this sorted out in the near future

[kevaundray]

I think this else branch should just be removed if there is nothing inside of it

[ledwards2225]

ditto

[kevaundray]

same here about empty else branch

[ledwards2225]

Swapped the print with the relevant issue

[kevaundray]

nit: could probably change this to hardcoded_grumpkin_subgroup_size_hack so there is consistency between bb.js and bb

[ledwards2225]

done

[kevaundray]

Left some small nits -- there seems to be changes that are unrelated to the circuit construction fix, I didn't look at these in too much detail -- If they are indeed orthogonal, in the future, ideally these are put into separate PRs with descriptions, so they can be reviewed separately!