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: Recursion in public kernels and rollup circuits #6425

Merged
merged 73 commits into from
May 22, 2024

Conversation

PhilWindle
Copy link
Collaborator

@PhilWindle PhilWindle commented May 15, 2024

Copy link
Contributor

github-actions bot commented May 15, 2024

Changes to circuit sizes

Generated at commit: e55736687f9e1a7c5e42a5fb24d95430fc57ba56, compared to commit: 0b95eb38cdb71eaec1f9ff077bbbbf0237b0b83a

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_merge +1,015 ❌ +333.88% +514,411 ❌ +1119.01%
rollup_root +902 ❌ +64.11% +548,366 ❌ +183.15%
public_kernel_setup +451 ❌ +0.20% +826,189 ❌ +124.03%
public_kernel_teardown +451 ❌ +0.20% +826,190 ❌ +123.98%
public_kernel_app_logic +451 ❌ +0.18% +826,390 ❌ +103.92%
rollup_base_simulated 0 ➖ 0.00% +11 ❌ +30.56%
public_kernel_tail +451 ❌ +0.04% +825,788 ❌ +22.29%
rollup_base +451 ❌ +0.24% +301,691 ❌ +16.71%
private_kernel_tail_simulated 0 ➖ 0.00% +11 ❌ +4.33%
public_kernel_tail_simulated 0 ➖ 0.00% +11 ❌ +4.33%
private_kernel_init_simulated 0 ➖ 0.00% +11 ❌ +0.42%
private_kernel_inner_simulated 0 ➖ 0.00% +11 ❌ +0.42%
private_kernel_reset_simulated 0 ➖ 0.00% +11 ❌ +0.42%
private_kernel_reset_simulated_big 0 ➖ 0.00% +11 ❌ +0.42%
private_kernel_reset_simulated_medium 0 ➖ 0.00% +11 ❌ +0.42%
private_kernel_reset_simulated_small 0 ➖ 0.00% +11 ❌ +0.42%
private_kernel_tail_to_public_simulated 0 ➖ 0.00% +11 ❌ +0.31%
public_kernel_app_logic_simulated 0 ➖ 0.00% +11 ❌ +0.31%
public_kernel_setup_simulated 0 ➖ 0.00% +11 ❌ +0.31%
public_kernel_teardown_simulated 0 ➖ 0.00% +11 ❌ +0.31%
parity_base 0 ➖ 0.00% +10 ❌ +0.01%
private_kernel_reset 0 ➖ 0.00% -2,803 ✅ -0.11%
private_kernel_tail_to_public 0 ➖ 0.00% -3,147 ✅ -0.15%
private_kernel_reset_big 0 ➖ 0.00% -3,027 ✅ -0.18%
private_kernel_tail 0 ➖ 0.00% -2,742 ✅ -0.21%
private_kernel_reset_medium 0 ➖ 0.00% -3,139 ✅ -0.25%
private_kernel_reset_small 0 ➖ 0.00% -3,195 ✅ -0.31%
private_kernel_inner 0 ➖ 0.00% -5,787 ✅ -0.34%
private_kernel_init 0 ➖ 0.00% -3,117 ✅ -0.37%
parity_root 0 ➖ 0.00% -13,425 ✅ -1.26%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_merge 1,319 (+1,015) +333.88% 560,381 (+514,411) +1119.01%
rollup_root 2,309 (+902) +64.11% 847,768 (+548,366) +183.15%
public_kernel_setup 223,608 (+451) +0.20% 1,492,311 (+826,189) +124.03%
public_kernel_teardown 223,814 (+451) +0.20% 1,492,605 (+826,190) +123.98%
public_kernel_app_logic 252,148 (+451) +0.18% 1,621,622 (+826,390) +103.92%
rollup_base_simulated 1 (0) 0.00% 47 (+11) +30.56%
public_kernel_tail 1,027,756 (+451) +0.04% 4,529,936 (+825,788) +22.29%
rollup_base 191,492 (+451) +0.24% 2,107,682 (+301,691) +16.71%
private_kernel_tail_simulated 1 (0) 0.00% 265 (+11) +4.33%
public_kernel_tail_simulated 1 (0) 0.00% 265 (+11) +4.33%
private_kernel_init_simulated 1 (0) 0.00% 2,643 (+11) +0.42%
private_kernel_inner_simulated 1 (0) 0.00% 2,643 (+11) +0.42%
private_kernel_reset_simulated 1 (0) 0.00% 2,643 (+11) +0.42%
private_kernel_reset_simulated_big 1 (0) 0.00% 2,643 (+11) +0.42%
private_kernel_reset_simulated_medium 1 (0) 0.00% 2,643 (+11) +0.42%
private_kernel_reset_simulated_small 1 (0) 0.00% 2,643 (+11) +0.42%
private_kernel_tail_to_public_simulated 1 (0) 0.00% 3,584 (+11) +0.31%
public_kernel_app_logic_simulated 1 (0) 0.00% 3,584 (+11) +0.31%
public_kernel_setup_simulated 1 (0) 0.00% 3,584 (+11) +0.31%
public_kernel_teardown_simulated 1 (0) 0.00% 3,584 (+11) +0.31%
parity_base 347 (0) 0.00% 79,821 (+10) +0.01%
private_kernel_reset 257,443 (0) 0.00% 2,528,135 (-2,803) -0.11%
private_kernel_tail_to_public 621,552 (0) 0.00% 2,147,286 (-3,147) -0.15%
private_kernel_reset_big 188,976 (0) 0.00% 1,678,516 (-3,027) -0.18%
private_kernel_tail 195,031 (0) 0.00% 1,316,885 (-2,742) -0.21%
private_kernel_reset_medium 154,744 (0) 0.00% 1,253,708 (-3,139) -0.25%
private_kernel_reset_small 137,627 (0) 0.00% 1,041,303 (-3,195) -0.31%
private_kernel_inner 262,668 (0) 0.00% 1,712,375 (-5,787) -0.34%
private_kernel_init 228,119 (0) 0.00% 835,753 (-3,117) -0.37%
parity_root 2,140 (0) 0.00% 1,054,162 (-13,425) -1.26%

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.

Some minor nits and suggestions on possible future cleanup, but overall looks good to me! Can't wait to get functions working! 🔥

* @param inputs - The public kernel inputs.
* @returns The witness map
*/
export function convertPublicSetupRollupInputsToWitnessMap(inputs: PublicKernelCircuitPrivateInputs): WitnessMap {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: typo. looks like the functions above also call them rollups.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

// Verify the previous kernel proof
assert(verify_kernel_proof(self.kernel_data.proof), "kernel proof verification failed");
// Verify the kernel circuit proof
verify_kernel_proof(self.kernel_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice to have a trait like Verifiable that all kernel data structs implement, with getters for the vk and public inputs.

Then we could have a single generic verify_protocol_proof.


public async verifyProofForCircuit(circuit: ProtocolArtifact, proof: Proof) {
// Create random directory to be used for temp files
const bbWorkingDirectory = `${this.config.bbWorkingDirectory}/${randomBytes(8).toString('hex')}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use your foundations/fs utility here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, thought I had caught them all.

const vkData = await this.updateVerificationKeyAfterProof(provingResult.vkPath!, circuitType);

// Read the proof and then cleanup up our temporary directory
const rawProof = await fs.readFile(`${provingResult.proofPath!}/${PROOF_FILENAME}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line the only real difference between createProof and createRecursiveProof?

Copy link
Collaborator Author

@PhilWindle PhilWindle May 22, 2024

Choose a reason for hiding this comment

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

Oh, I think you are right actually. Well spotted. I hadn't realised they had converged to be so similar.

(await getTestContractOnPXE(1)).methods.emit_unencrypted(43),
(await getTestContractOnPXE(2)).methods.create_l2_to_l1_message_public(45, 46, EthAddress.random()),
(await getTokenContract(3)).methods.transfer(schnorrWalletAddress.address, recipient.address, 1000, 0),
//(await getTestContractOnPXE(1)).methods.emit_unencrypted(43),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you leaving this commented out just for when we have public working?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, once we have public functions working we should be able to exercise fee payment flows as well.

// stop the fake provers
proverAgents: 0,
realProofs: true,
minTxsPerBlock: 1, // min 2 txs per block
Copy link
Collaborator

Choose a reason for hiding this comment

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

comment bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, the comment is corrct, the code is wrong. Nice catch!

tx,
inputs,
treeSnapshots,
provingState.privateKernelVerificationKeys.privateKernelToPublicCircuit,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it doesn't matter based on the comment above, but I'm confused why we use the ToPublic keys here, but prepare for the base rollup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, if there are public functions, then this is the key that will be provided to the first public kernel to verify the previous private kernel. The key that gets given to the base rollup in this case will be the publicKernelTail and this is done once that circuit has been proven.

If there no public functions then it uses the priveKernelCircuit key provided further up to prepareBaseRollupInputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

@just-mitch IIUC the ToPublic key just gets ignored if there are no public function calls in this tx (see the if statement below

    if (!numPublicKernels) {
      // no public functions, go straight to the base rollup
      logger.debug(`Enqueueing base rollup for tx ${txIndex}`);
      this.enqueueBaseRollup(provingState, BigInt(txIndex), txProvingState);
      return;
    }

tx,
inputs,
treeSnapshots,
provingState.privateKernelVerificationKeys.privateKernelToPublicCircuit,
Copy link
Contributor

Choose a reason for hiding this comment

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

@just-mitch IIUC the ToPublic key just gets ignored if there are no public function calls in this tx (see the if statement below

    if (!numPublicKernels) {
      // no public functions, go straight to the base rollup
      logger.debug(`Enqueueing base rollup for tx ${txIndex}`);
      this.enqueueBaseRollup(provingState, BigInt(txIndex), txProvingState);
      return;
    }

@PhilWindle PhilWindle enabled auto-merge (squash) May 22, 2024 13:28
@PhilWindle PhilWindle disabled auto-merge May 22, 2024 14:56
alexghr and others added 5 commits May 22, 2024 17:17
This PR adds commands to generate the verification Solidity contract for
the RootRollup so that block proofs can be validated onchain

Stacked on top of #6425
@PhilWindle PhilWindle enabled auto-merge (squash) May 22, 2024 20:10
@PhilWindle PhilWindle merged commit 86fb999 into master May 22, 2024
72 of 73 checks passed
@PhilWindle PhilWindle deleted the pw/more-recursion branch May 22, 2024 20:37
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.

[Proving Phase 1] Update benchmarks
4 participants