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: introduce avm circuit public inputs #9759

Merged
merged 12 commits into from
Nov 7, 2024

Conversation

LeilaWang
Copy link
Collaborator

@LeilaWang LeilaWang commented Nov 5, 2024

No description provided.

@LeilaWang LeilaWang added the e2e-all CI: Enables this CI job. label Nov 5, 2024
@@ -111,7 +101,6 @@ impl PrivateCallDataValidator {
self.validate_call();
self.validate_private_call_requests();
self.validate_public_call_requests();
self.validate_teardown_call_request();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved into validate_public_call_requests.

} else {
1
};
validate_incrementing_counters_within_range(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't need to check it as teardown_call_request does not have a counter anymore.

@@ -85,10 +85,6 @@ impl PrivateKernelCircuitOutputValidator {
private_call.historical_header,
"mismatch historical_header",
);
assert(
is_empty(self.output.constants.global_variables),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this check as a new struct TxConstantData is now used in private land, which includes all fields in CombinedConstantData except for global_variables.

@@ -34,6 +31,5 @@ pub fn meter_gas_used(data: CombinedAccumulatedData, gas_settings: GasSettings)
metered_da_bytes += data.unencrypted_log_preimages_length as u32;
metered_l2_gas += data.unencrypted_log_preimages_length as u32 * L2_GAS_PER_LOG_BYTE;

let teardown_gas = gas_settings.teardown_gas_limits;
Gas::new(metered_da_bytes * DA_GAS_PER_BYTE, metered_l2_gas) + Gas::tx_overhead() + teardown_gas
Copy link
Collaborator Author

@LeilaWang LeilaWang Nov 5, 2024

Choose a reason for hiding this comment

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

Don't include teardown_gas for private-only tx.

Before this, the users would have to set it to empty in GasSettings to avoid paying extra fees, which was quite annoying.

Copy link
Contributor

github-actions bot commented Nov 5, 2024

Changes to public function bytecode sizes

Generated at commit: 3effc7935de1aee3aaed18783322fb1e12cac6eb, compared to commit: d77e473219d1628b2045100a55c4073f9fa32c25

🧾 Summary (100% most significant diffs)

Program Bytecode size in bytes (+/-) %
Crowdfunding::public_dispatch +28 ❌ +0.45%
FeeJuice::public_dispatch -4 ✅ -0.06%
DocsExample::public_dispatch -4 ✅ -0.07%
StatefulTest::public_dispatch -6 ✅ -0.07%
Test::public_dispatch -27 ✅ -0.14%
Benchmarking::public_dispatch -10 ✅ -0.18%
Token::_finalize_transfer_to_private_unsafe -13 ✅ -0.18%
Token::finalize_transfer_to_private -13 ✅ -0.18%
Token::_finalize_mint_to_private_unsafe -13 ✅ -0.18%
Token::finalize_mint_to_private -13 ✅ -0.18%
Auth::public_dispatch -20 ✅ -0.19%
NFT::_finalize_transfer_to_private_unsafe -13 ✅ -0.20%
NFT::finalize_transfer_to_private -13 ✅ -0.20%
TokenBridge::public_dispatch -47 ✅ -0.21%
Token::complete_refund -13 ✅ -0.21%
Child::public_dispatch -13 ✅ -0.22%
EasyPrivateVoting::public_dispatch -16 ✅ -0.26%
Claim::public_dispatch -13 ✅ -0.31%
InclusionProofs::public_dispatch -13 ✅ -0.31%
PrivateFPC::public_dispatch -13 ✅ -0.32%
Lending::public_dispatch -92 ✅ -0.32%
Spam::public_dispatch -13 ✅ -0.34%
Uniswap::public_dispatch -89 ✅ -0.35%
AuthRegistry::public_dispatch -33 ✅ -0.39%
StaticChild::public_dispatch -13 ✅ -0.43%
TokenBlacklist::public_dispatch -137 ✅ -0.54%
CardGame::public_dispatch -92 ✅ -0.56%
NFT::public_dispatch -160 ✅ -0.61%
AvmTest::returndata_copy_oracle -13 ✅ -0.65%
AvmTest::public_dispatch -407 ✅ -0.67%
Token::public_dispatch -287 ✅ -0.78%
FPC::public_dispatch -76 ✅ -0.80%
PriceFeed::public_dispatch -39 ✅ -0.98%
StaticParent::public_dispatch -72 ✅ -0.99%
AuthWitTest::public_dispatch -21 ✅ -1.09%
AppSubscription::public_dispatch -57 ✅ -1.13%
TestLog::public_dispatch -39 ✅ -1.20%
Parent::public_dispatch -124 ✅ -1.46%
ImportTest::public_dispatch -26 ✅ -1.65%
Router::public_dispatch -62 ✅ -2.34%

Full diff report 👇
Program Bytecode size in bytes (+/-) %
Crowdfunding::public_dispatch 6,212 (+28) +0.45%
FeeJuice::public_dispatch 6,261 (-4) -0.06%
DocsExample::public_dispatch 5,838 (-4) -0.07%
StatefulTest::public_dispatch 8,122 (-6) -0.07%
Test::public_dispatch 19,751 (-27) -0.14%
Benchmarking::public_dispatch 5,687 (-10) -0.18%
Token::_finalize_transfer_to_private_unsafe 7,362 (-13) -0.18%
Token::finalize_transfer_to_private 7,315 (-13) -0.18%
Token::_finalize_mint_to_private_unsafe 7,152 (-13) -0.18%
Token::finalize_mint_to_private 7,105 (-13) -0.18%
Auth::public_dispatch 10,782 (-20) -0.19%
NFT::_finalize_transfer_to_private_unsafe 6,482 (-13) -0.20%
NFT::finalize_transfer_to_private 6,435 (-13) -0.20%
TokenBridge::public_dispatch 22,625 (-47) -0.21%
Token::complete_refund 6,044 (-13) -0.21%
Child::public_dispatch 5,981 (-13) -0.22%
EasyPrivateVoting::public_dispatch 6,085 (-16) -0.26%
Claim::public_dispatch 4,227 (-13) -0.31%
InclusionProofs::public_dispatch 4,169 (-13) -0.31%
PrivateFPC::public_dispatch 4,105 (-13) -0.32%
Lending::public_dispatch 28,339 (-92) -0.32%
Spam::public_dispatch 3,765 (-13) -0.34%
Uniswap::public_dispatch 25,046 (-89) -0.35%
AuthRegistry::public_dispatch 8,363 (-33) -0.39%
StaticChild::public_dispatch 3,033 (-13) -0.43%
TokenBlacklist::public_dispatch 25,197 (-137) -0.54%
CardGame::public_dispatch 16,234 (-92) -0.56%
NFT::public_dispatch 26,233 (-160) -0.61%
AvmTest::returndata_copy_oracle 1,998 (-13) -0.65%
AvmTest::public_dispatch 60,132 (-407) -0.67%
Token::public_dispatch 36,704 (-287) -0.78%
FPC::public_dispatch 9,396 (-76) -0.80%
PriceFeed::public_dispatch 3,947 (-39) -0.98%
StaticParent::public_dispatch 7,220 (-72) -0.99%
AuthWitTest::public_dispatch 1,907 (-21) -1.09%
AppSubscription::public_dispatch 4,973 (-57) -1.13%
TestLog::public_dispatch 3,219 (-39) -1.20%
Parent::public_dispatch 8,381 (-124) -1.46%
ImportTest::public_dispatch 1,551 (-26) -1.65%
Router::public_dispatch 2,583 (-62) -2.34%

Copy link
Contributor

github-actions bot commented Nov 6, 2024

Changes to circuit sizes

Generated at commit: 3effc7935de1aee3aaed18783322fb1e12cac6eb, compared to commit: 1b41d3855badb318e7a37bd9e4f18d62af626184

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_public +108,294 ❌ +31.30% +1,234,320 ❌ +53.32%
private_kernel_init +150 ❌ +0.70% +3,775 ❌ +12.14%
private_kernel_inner +132 ❌ +0.35% +3,761 ❌ +7.00%
private_kernel_reset -24 ✅ -0.03% -30 ✅ -0.01%
private_kernel_empty -63 ✅ -9.39% -63 ✅ -0.01%
private_kernel_reset_4_4_4_4_4_4_4_4_1 -24 ✅ -0.08% -39 ✅ -0.05%
private_kernel_tail -133 ✅ -2.88% -178 ✅ -1.35%
rollup_base_private -28,030 ✅ -8.12% -61,826 ✅ -1.89%
private_kernel_tail_to_public -6,597 ✅ -25.93% -8,984 ✅ -22.31%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_base_public 454,278 (+108,294) +31.30% 3,549,032 (+1,234,320) +53.32%
private_kernel_init 21,708 (+150) +0.70% 34,871 (+3,775) +12.14%
private_kernel_inner 38,070 (+132) +0.35% 57,511 (+3,761) +7.00%
private_kernel_reset 73,331 (-24) -0.03% 462,711 (-30) -0.01%
private_kernel_empty 608 (-63) -9.39% 942,835 (-63) -0.01%
private_kernel_reset_4_4_4_4_4_4_4_4_1 31,789 (-24) -0.08% 76,935 (-39) -0.05%
private_kernel_tail 4,492 (-133) -2.88% 13,043 (-178) -1.35%
rollup_base_private 317,095 (-28,030) -8.12% 3,210,904 (-61,826) -1.89%
private_kernel_tail_to_public 18,849 (-6,597) -25.93% 31,290 (-8,984) -22.31%

@@ -66,7 +67,7 @@ pub struct PrivateContext {
nullifiers: BoundedVec<Nullifier, MAX_NULLIFIERS_PER_CALL>,

private_call_requests: BoundedVec<PrivateCallRequest, MAX_PRIVATE_CALL_STACK_LENGTH_PER_CALL>,
public_call_requests: BoundedVec<PublicCallRequest, MAX_PUBLIC_CALL_STACK_LENGTH_PER_CALL>,
public_call_requests: BoundedVec<Counted<PublicCallRequest>, MAX_ENQUEUED_CALLS_PER_CALL>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove counter from PublicCallRequest and use Counted to add counter to any struct.

Will do the same for other side effects in another PR.

meter_gas_used(data) + Gas::tx_overhead()
}

pub fn meter_gas_used_revertible(data: PublicAccumulatedData, teardown_gas: Gas) -> Gas {
meter_gas_used(data) + Gas::new(teardown_gas.da_gas, teardown_gas.l2_gas)
Copy link
Collaborator Author

@LeilaWang LeilaWang Nov 6, 2024

Choose a reason for hiding this comment

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

Don't add the teardown_gas to revertible gas_used in private_tail_to_public.

It is used to compute the transaction fee, and should be added when actually computing the transaction fee. (I think) it was added here to make the code for computing fee slightly simpler - adding gas used from both non-revertible and revertible. But there were code in some places that subtracted the teardown gas to get the actual gas used, which seemed more confusing.

With this change, the gas_used from either private kernel tail or tail_to_public indicates the actual gas used. When computing transaction fee, it will be non_revertible_gas + revertible_gas + teardown_gas.

@LeilaWang LeilaWang requested a review from dbanks12 as a code owner November 6, 2024 17:54
self.hints.public_data_writes,
self.output.end.public_data_update_requests,
);
let original_array = self.hints.public_data_writes;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copied the code from assert_deduped_array, and tweaked it so that it operates on the deduped array that contains different types to the items in the original array.

Can't be bothered to change assert_deduped_array since we will remove the public kernel soon.

let all_public_data_update_requests =
self.calculate_all_public_data_update_requests(transaction_fee);
self.calculate_all_public_data_update_requests(self.transaction_fee);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

transaction_fee is either computed in private_base_rollup or in the AVM and passed in from public_base_rollup.


if read_hint.leaf_slot == 0 {
if existing_update_index != MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use the existing_update_index to decide whether to update the pending set or the tree.

This makes the code in the orchestrator simpler. It doesn't have to check whether the write exists in the pending set to generate the hint. It can always generate the hint. And let the circuit decide whether to use the hint or not.

@@ -393,20 +393,6 @@ export class BBNativeRollupProver implements ServerCircuitProver {
return emptyPrivateKernelProof;
}

public async getEmptyTubeProof(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this because it is exactly the same as getEmptyPrivateKernelProof.

/**
* Output data of the tx.
*/
txEffect: TxEffect;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

data: KernelCircuitPublicInputs is removed from ProcessedTx because it is no longer the type exported by all txs. Private-only txs still output KernelCircuitPublicInputs. But txs containing public calls are now associated with AvmCircuitPublicIpnuts.

We replace it with TxEffect, which achieves what KernelCircuitPublicInputs can do and more:

  • The finalPublicDataUpdateRequests field can be removed because txEffect.publicDataWrites contains all public data writes, including the feePaymentPublicDataWrite injected by the base rollup.
  • All 3 log preimages arrays can be removed because they are already in TxEffect.

* Is the struct empty?
* @returns whether all members are empty.
*/
isEmpty(): boolean {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removing this as it was only used in avm.test.ts, and I didn't want to add isEmpty to AvmCircuitPublicInputs and all its child types.
In real world we might never need to check if the inputs to the AVM is empty or not.

@LeilaWang LeilaWang removed the request for review from fcarreiro November 6, 2024 23:46
Copy link
Collaborator

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

Awesome work! Huge changeset....

@LeilaWang LeilaWang merged commit 4660381 into master Nov 7, 2024
101 of 106 checks passed
@LeilaWang LeilaWang deleted the lw/avm_circuit_public_inputs branch November 7, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-all CI: Enables this CI job.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants