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: Inject fee payment update in base rollup #6403

Merged
merged 26 commits into from
May 22, 2024

Conversation

spalladino
Copy link
Collaborator

@spalladino spalladino commented May 14, 2024

Implements the fee payment update spec by charging the user's fee in the base rollup circuit. Involves the following changes:

  • Transactions now have a set of protocol-public-data-writes, which are data writes triggered by the protocol and not the user. We populate this in the base rollup circuit to charge the fee_payer. Note that if no fee_payer is sent, the tx goes through without paying (this will change once we enforce fees).
  • The base rollup circuit now needs a public data read hint to be able to read the fee_payer's current balance.
  • The GasToken contract now has no pay_fee function anymore. The pay_fee functions in the FPC are now renamed to pay_refund
  • The GasToken address is now a constant, since the base rollup circuit needs to know it to derive the slot for users balances. Because of this, the portal_address is removed from the GasToken constructor, so the address of the GasToken does not depend on its L1 counterpart, which would have made deployments more cumbersome.
  • Added an ACVM_FORCE_WASM flag to tests to force using acvm wasm instead of binary, since the binary failed to report the assertion failed messages.
  • The sequencer now validates that the fee_payer (if set) has enough balance to pay for the tx before picking it up, assuming it consumes all its gas limits.

Coming up on future PRs:

Copy link
Contributor

github-actions bot commented May 17, 2024

Changes to circuit sizes

Generated at commit: 8cccc4cb2d63e0c9587adedb40a5bf7f1cc28c7e, compared to commit: d3c3d4a565568329ca55f24e7f5e8d3e293e1366

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base +9,914 ❌ +5.47% +29,611 ❌ +1.67%
private_kernel_tail -2 ✅ -0.00% -4 ✅ -0.00%
private_kernel_tail_to_public -4 ✅ -0.00% -8 ✅ -0.00%
public_kernel_teardown -160 ✅ -0.07% -300 ✅ -0.04%
public_kernel_setup -160 ✅ -0.07% -300 ✅ -0.04%
public_kernel_app_logic -200 ✅ -0.08% -360 ✅ -0.05%
private_kernel_tail_to_public_simulated 0 ➖ 0.00% -4 ✅ -0.11%
public_kernel_app_logic_simulated 0 ➖ 0.00% -4 ✅ -0.11%
public_kernel_setup_simulated 0 ➖ 0.00% -4 ✅ -0.11%
public_kernel_teardown_simulated 0 ➖ 0.00% -4 ✅ -0.11%
public_kernel_tail -2,126 ✅ -0.21% -4,928 ✅ -0.13%
private_kernel_tail_simulated 0 ➖ 0.00% -2 ✅ -0.75%
public_kernel_tail_simulated 0 ➖ 0.00% -2 ✅ -0.75%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base 191,041 (+9,914) +5.47% 1,806,511 (+29,611) +1.67%
private_kernel_tail 195,031 (-2) -0.00% 1,316,885 (-4) -0.00%
private_kernel_tail_to_public 621,552 (-4) -0.00% 2,147,286 (-8) -0.00%
public_kernel_teardown 223,363 (-160) -0.07% 667,581 (-300) -0.04%
public_kernel_setup 223,157 (-160) -0.07% 667,288 (-300) -0.04%
public_kernel_app_logic 251,697 (-200) -0.08% 796,596 (-360) -0.05%
private_kernel_tail_to_public_simulated 1 (0) 0.00% 3,584 (-4) -0.11%
public_kernel_app_logic_simulated 1 (0) 0.00% 3,584 (-4) -0.11%
public_kernel_setup_simulated 1 (0) 0.00% 3,584 (-4) -0.11%
public_kernel_teardown_simulated 1 (0) 0.00% 3,584 (-4) -0.11%
public_kernel_tail 1,027,305 (-2,126) -0.21% 3,704,913 (-4,928) -0.13%
private_kernel_tail_simulated 1 (0) 0.00% 265 (-2) -0.75%
public_kernel_tail_simulated 1 (0) 0.00% 265 (-2) -0.75%

@AztecBot
Copy link
Collaborator

AztecBot commented May 17, 2024

Docs Preview

Hey there! 👋 You can check your preview at https://664de539634e2bf5377a7c27--aztec-docs-dev.netlify.app

@spalladino spalladino force-pushed the palla/inject-fee-payment-update branch from 5104f8f to 74e0ae1 Compare May 20, 2024 20:02
@spalladino spalladino force-pushed the palla/inject-fee-payment-update branch from 624e6a4 to e1ec553 Compare May 21, 2024 14:36
@@ -26,6 +26,8 @@ pub fn batch_insert<Value, Leaf, SubtreeWidth, SiblingPathLength, SubtreeHeight,
// A permutation to the values is provided to make the insertion use only one insertion strategy
check_permutation(values_to_insert, sorted_values, sorted_values_indexes);

// TODO: Shouldn't we check `sorted_values` is sorted?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@LeilaWang do you know if this is a missing check or it's indeed not needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't necessary have to be sorted in increasing order. But the values need to be arranged in a way that the low leaf will be inserted before a value if both are pending. It's on the prover to decide how to create the array, as long as it's a permutation of values_to_insert, which is checked above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably need better names cause I was confused for a bit 😂

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, thanks! I'll remove this comment on the next PR.

@spalladino spalladino marked this pull request as ready for review May 21, 2024 14:50
Copy link
Collaborator

@LeilaWang LeilaWang left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

spalladino added a commit that referenced this pull request May 21, 2024
Had been disabled after #6403
@spalladino spalladino force-pushed the palla/inject-fee-payment-update branch from 87bf5e1 to 0ae0240 Compare May 21, 2024 17:27
spalladino added a commit that referenced this pull request May 21, 2024
Had been disabled after #6403
Copy link
Collaborator

@just-mitch just-mitch left a comment

Choose a reason for hiding this comment

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

Nice!

@spalladino spalladino force-pushed the palla/inject-fee-payment-update branch from 0ae0240 to 03677ab Compare May 21, 2024 20:40
spalladino added a commit that referenced this pull request May 21, 2024
Had been disabled after #6403
Copy link
Collaborator Author

spalladino commented May 21, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @spalladino and the rest of your teammates on Graphite Graphite

@AztecBot
Copy link
Collaborator

AztecBot commented May 21, 2024

Benchmark results

No base data found for comparison.

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.

Proof generation

Each column represents the number of threads used in proof generation.

Metric 1 threads 4 threads 16 threads 32 threads 64 threads
proof_construction_time_sha256 5,681 1,543 715 749 777

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 772 772 772
l1_rollup_calldata_gas 6,868 6,880 6,880
l1_rollup_execution_gas 587,408 587,420 587,420
l2_block_processing_time_in_ms 1,355 5,101 10,130
l2_block_building_time_in_ms 32,315 127,003 253,779
l2_block_rollup_simulation_time_in_ms 32,142 126,371 252,530
l2_block_public_tx_process_time_in_ms 16,838 69,146 141,952

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,100 28,226
node_database_size_in_bytes 21,221,456 37,896,272
pxe_database_size_in_bytes 29,868 12,535

Circuits stats

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

Circuit protocol_circuit_simulation_time_in_ms protocol_circuit_witness_generation_time_in_ms protocol_circuit_proving_time_in_ms protocol_circuit_input_size_in_bytes protocol_circuit_output_size_in_bytes protocol_circuit_proof_size_in_bytes protocol_circuit_num_public_inputs protocol_circuit_size_in_gates
private-kernel-init 158 3,686 21,825 19,985 61,999 86,720 2,643 1,048,576
private-kernel-inner 602 4,784 41,001 89,053 61,999 86,720 2,643 2,097,152
private-kernel-reset-small 571 2,386 23,098 117,961 61,999 86,720 2,643 1,048,576
private-kernel-tail 526 3,905 37,750 86,849 78,838 10,624 265 2,097,152
base-parity 6.53 1,043 3,164 128 64.0 2,208 2.00 131,072
root-parity 49.2 62.2 40,377 27,064 64.0 2,720 18.0 2,097,152
base-rollup 720 2,553 43,775 112,602 957 3,136 31.0 2,097,152
root-rollup 93.2 48.8 8,357 11,518 821 3,456 41.0 524,288
public-kernel-app-logic 241 135 745 96,850 84,967 116,320 3,568 4,096
public-kernel-tail 861 668 1,160 388,084 7,691 10,112 249 512
public-kernel-setup 218 220 918 143,141 84,967 116,320 3,568 4,096
public-kernel-teardown 224 239 1,151 143,141 84,967 116,320 3,568 4,096
merge-rollup 6.45 19.8 1,615 2,760 957 3,136 31.0 65,536
private-kernel-tail-to-public N/A 9,744 72,039 N/A N/A 116,832 3,584 4,194,304

Stats on running time collected for app circuits

Function app_circuit_proof_size_in_bytes app_circuit_proving_time_in_ms app_circuit_size_in_gates app_circuit_num_public_inputs
SchnorrAccount:entrypoint 16,128 47,755 2,097,152 437
Test:emit_nullifier 16,128 2,786 65,536 437
FPC:fee_entrypoint_public 16,128 16,708 524,288 437
FPC:fee_entrypoint_private 16,128 9,005 524,288 437
Token:unshield 16,128 48,097 2,097,152 437
SchnorrAccount:spend_private_authwit 16,128 2,690 131,072 437
Token:transfer 16,128 35,047 2,097,152 437

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 11.2 18.2 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.7 31.8 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.654 0.561 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 52.2 80.5 261 502 989 1,960 N/A
batch_insert_into_append_only_tree_32_depth_hash_count N/A N/A 95.9 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.534 0.497 0.475 0.469 0.469 0.468 N/A
batch_insert_into_indexed_tree_20_depth_ms N/A N/A 62.4 119 378 737 1,474 2,932 N/A
batch_insert_into_indexed_tree_20_depth_hash_count N/A N/A 106 208 692 1,363 2,707 5,395 N/A
batch_insert_into_indexed_tree_20_depth_hash_ms N/A N/A 0.543 0.534 0.511 0.507 0.510 0.509 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 67.4
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 108
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.591

Miscellaneous

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

Metric 0 registered classes 1 registered classes
tx_size_in_bytes 84,613 664,966

Transaction size based on fee payment method

| Metric | |
| - | |

Transaction processing duration by data writes.

Metric 0 new note hashes 1 new note hashes 2 new note hashes
tx_pxe_processing_time_ms 27,546 4,239 122,868
Metric 0 public data writes 1 public data writes 2 public data writes 3 public data writes 4 public data writes 8 public data writes
tx_sequencer_processing_time_ms 1,370 2,205 1,509 4,068 1,691 1,989

@spalladino spalladino force-pushed the palla/inject-fee-payment-update branch from 03677ab to 3082c4d Compare May 22, 2024 11:52
spalladino added a commit that referenced this pull request May 22, 2024
Had been disabled after #6403
@spalladino spalladino enabled auto-merge (squash) May 22, 2024 11:53
@spalladino spalladino merged commit 4991188 into master May 22, 2024
71 checks passed
@spalladino spalladino deleted the palla/inject-fee-payment-update branch May 22, 2024 12:40
spalladino added a commit that referenced this pull request May 22, 2024
Had been disabled after #6403
spalladino added a commit that referenced this pull request May 22, 2024
Had been disabled after #6403
spalladino added a commit that referenced this pull request May 23, 2024
Had been disabled after #6403
spalladino added a commit that referenced this pull request May 23, 2024
Had been disabled after #6403
spalladino added a commit that referenced this pull request May 23, 2024
Had been disabled after #6403
spalladino added a commit that referenced this pull request May 23, 2024
Had been disabled after #6403
spalladino added a commit that referenced this pull request May 23, 2024
Had been disabled after #6403

Builds on top of #6403
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants