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

feat: Generic Lookup Relations for AVM #3774

Closed
wants to merge 12 commits into from

Conversation

Rumata888
Copy link
Contributor

@Rumata888 Rumata888 commented Dec 26, 2023

Adds generic lookup relation with range constraint and toy examples.
The relation is base on logderivative and can be parametrised for most of the common usecases

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

@Rumata888 Rumata888 self-assigned this Dec 26, 2023
@AztecBot
Copy link
Collaborator

AztecBot commented Dec 26, 2023

Benchmark results

Metrics with a significant change:

  • circuit_simulation_time_in_ms (base-rollup): 1,061 (-48%)
  • circuit_input_size_in_bytes (base-rollup): 123,254 (-50%)
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 e83dd2b3 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,244 3,449,708
l1_rollup_execution_gas 867,129 (+3%) 3,746,297 (+4%) 23,306,473 (+5%)
l2_block_processing_time_in_ms 1,368 (+3%) 5,110 (+1%) 21,100 (+1%)
note_successful_decrypting_time_in_ms 339 (-1%) 1,169 (+1%) 3,834
note_trial_decrypting_time_in_ms 47.1 (-11%) 36.3 (-8%) 143 (+4%)
l2_block_building_time_in_ms 14,047 (+3%) 55,543 (+3%) 224,335 (+3%)
l2_block_rollup_simulation_time_in_ms 10,465 (+4%) 41,520 (+4%) 167,995 (+4%)
l2_block_public_tx_process_time_in_ms 3,556 (+1%) 13,968 56,164

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,208 (-3%) 28,756 (-3%)
note_history_successful_decrypting_time_in_ms 2,464 5,112 (+6%)
note_history_trial_decrypting_time_in_ms 81.1 (+3%) 191 (-1%)
node_database_size_in_bytes 3,618,825 (-7%) 3,729,456 (-11%)
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 200 43,109 20,441
private-kernel-ordering 115 25,833 9,689
base-rollup ⚠️ 1,061 (-48%) ⚠️ 123,254 (-50%) 881
root-rollup 82.4 (-2%) 4,088 889
private-kernel-inner 262 64,516 20,441
public-kernel-private-input 172 25,203 20,441
public-kernel-non-first-iteration 171 25,245 20,441
merge-rollup 9.71 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 10,331 26,317

@Rumata888 Rumata888 force-pushed the is/bonobus_generic_lookup_relations branch from f3fbf69 to c2ba5ee Compare December 31, 2023 17:39
@Rumata888 Rumata888 marked this pull request as ready for review January 2, 2024 13:59
// 2) WRITE_TERMS number of polynomials representing how much each write term has been read
// 3) READ_TERMS number of polynomials enabling the addition of a particular read term in this row (should we lookup
// or not)
// 4) WRITE_TERMS number of polynomials enabling a particular write term in this row (should we add it to
Copy link
Member

Choose a reason for hiding this comment

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

are 2 and 4 meant to be labelled the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not labels, this is how many polynomials there are of each type (WRITE_TERMS) is how many table entries can be added per row

@@ -52,6 +52,12 @@ template <typename Flavor> class ToyAVMCircuitBuilder {
const auto num_gates_log2 = static_cast<size_t>(numeric::get_msb64(num_gates));
size_t num_gates_pow2 = 1UL << (num_gates_log2 + (1UL << num_gates_log2 == num_gates ? 0 : 1));

// We need at least 256 values for the range constraint
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this? do we have precomputed columns that have a minimum size of 256?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just that in this implementation we add a table from 0-255 in compute_polynomials. It is not relation-generic, just for this concrete example.

* + READ - the action of looking up the value in the table
* + WRITE - the action of adding the value to the lookup table
*
* TODO(@Rumata888): Talk to Zac why "lookup_read_count" refers to the count of the looked up element in the multiset.
Copy link
Contributor

Choose a reason for hiding this comment

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

The relation evaluates the following (with X replaced by a random challenge):

_ 
\  ____1____ -  ____read_count[I]____
/  X + reads[I]     X + writes[I]
¯

The reason why we multiple lookup_read_count by the write predicate, is because we are also multiplying lookup_read_count with (1 / X + writes[i]) from the above equation.

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood the formula, it was more the semantics that confused me, because we were using read count for the number of times we wrote

polys.lookup_xor_argument_1[i] = wires[6][i];
polys.lookup_xor_argument_2[i] = wires[7][i];
polys.lookup_xor_result[i] = wires[8][i];
polys.lookup_xor_accumulated_argument_1[i] = wires[9][i];
Copy link
Contributor

Choose a reason for hiding this comment

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

what do lookup_xor_accumulated_argument_1 and lookup_xor_accumulated_argument_2 represent?

How many columns are we adding per distinct lookup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are trying to prove that lookup_xor_accumulated_argument_1 ^ lookup_xor_accumulated_argument_2 = lookup_xor_accumulated_result. However they are 8-bit, so we decompose them. The top 4 bits of lookup_xor_accumulated_argument_1 will be in lookup_xor_argument_1.

For this particular case we added 6 columns for arguments. 2 3-element tuples.

@@ -0,0 +1,469 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

One potential problem with defining lookups as independent relations is the very large number of inverse polynomials that are going to be created.

This will be expensive

Important question: does the relation algebra ensure that, if a given row is NOT performing a lookup, the value of the inverse polynomial is 0? If not, I think this will be needed or committing to 1 inverse polynomial per lookup column will be very painful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, the polynomial should be zero when we are not performing lookups

@Rumata888
Copy link
Contributor Author

Merged as a part of #3880

@Rumata888 Rumata888 closed this Feb 1, 2024
@ludamad ludamad deleted the is/bonobus_generic_lookup_relations branch August 22, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants