A6 public input circuit #161

CeciliaZ030 · 2023-10-09T17:03:33Z

Description

Migrated from #149

Brechtpd

Haven't rechecked the circuit in detail again, but generally looks okay, just some questions.

There's also still some clippy errors I think.

zkevm-circuits/src/table/block_table.rs

zkevm-circuits/src/taiko_super_circuit.rs

zkevm-circuits/src/taiko_pi_circuit.rs

smtmfft

PI circuit part seems ok as the most of logic is moved from a5, however, maybe separating this one to 2 PRs(1 for circuit_tool & 1 for pi change) is better for reviewing & commitment organization.

smtmfft · 2023-10-30T01:45:22Z

bus-mapping/src/circuit_input_builder/protocol_instance.rs

+            FixedBytes(self.graffiti, 32),
+        ];
+        match evidence_type {
+            EvidenceType::Sgx { prover, new_pubkey } => {


Do we need sgx type here?

Copied from Raiko? Otherwise indeed not necessary here.

Yea it's copied.

smtmfft · 2023-10-30T01:46:54Z

bus-mapping/src/circuit_input_builder/protocol_instance.rs

+        result.extend_from_slice(self.block_evidence.blockMetadata.l1Hash.as_slice());
+        result.extend_from_slice(self.block_evidence.signalRoot.as_slice());
+        result.extend_from_slice(self.block_evidence.blockMetadata.l1Hash.as_slice());
+        result.extend_from_slice(&[0]);


This is parent_gas_used which got removed from this version. I'll change it to a param.

smtmfft · 2023-10-30T01:50:57Z

bus-mapping/src/circuit_input_builder/protocol_instance.rs

+});
+
+///
+pub static TREASURY: Lazy<Address> = Lazy::new(|| {


in case of l1 change, hardcode is inflexible, I think client already gives all of them from API, why you need hardcode here?

I think these constants should be taken from Raiko-host? Maybe we should align the API. For now the only place of getting taiko specific params are through ProtocolInstance, if anything is removed from BlockEvidence in our contract, I'll have to scrape it from somewhere else or hardcode.

smtmfft · 2023-10-30T02:08:40Z

zkevm-circuits/src/taiko_pi_circuit.rs


-/// PublicData contains all the values that the PiCircuit receives as input
+const S1: PiCellType = PiCellType::StoragePhase1;


SP1 or SPhase1 makes more sense.

smtmfft · 2023-10-30T02:12:54Z

gadgets/src/util.rs

@@ -173,6 +203,7 @@ impl_expr!(bool);
 impl_expr!(u8);
 impl_expr!(u64);
 impl_expr!(usize);
+impl_expr!(isize);


which data is in isize type?

Default type of negative numbers, was added for the MPT circuit.

smtmfft · 2023-10-30T02:18:12Z

zkevm-circuits/src/taiko_pi_circuit.rs

@@ -540,11 +454,11 @@ impl<F: Field> SubCircuit<F> for TaikoPiCircuit<F> {
    fn unusable_rows() -> usize {
        // No column queried at more than 3 distinct rotations, so returns 6 as
        // minimum unusable rows.
-        6
+        CM_HEIGHT + 3


is CM_HEIGHT enough? as you don't have cross CM rotation I think.

Seems okay to me because 3 is the base number of unusable rows. Or what do you mean with cross CM rotation?

I was thinking that the max rotation is CM_HEIGHT because there is no rotation to the other cm region, not a real problem with +3.

I think the 3 comes from the random values that are required to make the prover zero knowledge that need to be stored in the last 3 rows. Not sure though, and I also don't know why it's 3 to make that happen (I would think 1 random value would be good enough, but seems like not).

Total cells needed for 7 FieldGadget are 7 * 32 = 224. CM height is 15 and has 15 columns available = 225 which fits nicely :)

Brechtpd

Would indeed have been a bit easier to have separate PRs so it's more clear what needs to be reviewed more closely. Now I just went over everything pretty quickly. :)

Brechtpd · 2023-10-30T17:40:26Z

zkevm-circuits/src/evm_circuit/execution/jump.rs

@@ -34,7 +34,6 @@ impl<F: Field> ExecutionGadget<F> for JumpGadget<F> {

        // Pop the value from the stack
        cb.stack_pop(destination.expr());
-


Random new line removed diff?

Brechtpd · 2023-10-30T17:48:31Z

zkevm-circuits/src/taiko_pi_circuit.rs

@@ -540,11 +454,11 @@ impl<F: Field> SubCircuit<F> for TaikoPiCircuit<F> {
    fn unusable_rows() -> usize {
        // No column queried at more than 3 distinct rotations, so returns 6 as
        // minimum unusable rows.
-        6
+        CM_HEIGHT + 3


Seems okay to me because 3 is the base number of unusable rows. Or what do you mean with cross CM rotation?

Brechtpd · 2023-10-30T17:50:08Z

gadgets/src/util.rs

@@ -173,6 +203,7 @@ impl_expr!(bool);
 impl_expr!(u8);
 impl_expr!(u64);
 impl_expr!(usize);
+impl_expr!(isize);


Default type of negative numbers, was added for the MPT circuit.

Brechtpd · 2023-10-30T17:51:55Z

bus-mapping/src/circuit_input_builder/protocol_instance.rs

+            FixedBytes(self.graffiti, 32),
+        ];
+        match evidence_type {
+            EvidenceType::Sgx { prover, new_pubkey } => {


Copied from Raiko? Otherwise indeed not necessary here.

CeciliaZ030 added 2 commits October 10, 2023 00:19

added circuit-tools

de1d474

done

d2e5d9d

github-actions bot added crate-gadgets crate-zkevm-circuits labels Oct 9, 2023

CeciliaZ030 added 4 commits October 10, 2023 19:56

new circuit tools

c43017d

clippy & fmt

4a0f298

Merge commit '939a5753b562f0612dc25e4f76f27eb1539e726b' into a6-pi

daac441

meta_data

d748edd

CeciliaZ030 marked this pull request as ready for review October 10, 2023 12:09

CeciliaZ030 requested review from Brechtpd and smtmfft October 10, 2023 12:10

Brechtpd reviewed Oct 10, 2023

View reviewed changes

CeciliaZ030 added 7 commits October 13, 2023 20:12

update

e9aaa8b

fix

b9949ae

clippy

c25777b

change some test

b1df725

fmt

4411ea4

new pi skeleton

a356761

contestable rollup modification

4b2cf72

github-actions bot added crate-bus-mapping crate-mock labels Oct 26, 2023

CeciliaZ030 added 4 commits October 26, 2023 18:18

done

eb295e1

fmt & fix

7e18abf

clippy

cbd0c59

fix all targets clippy

5228f57

github-actions bot added the crate-integration-tests label Oct 26, 2023

CeciliaZ030 added 2 commits October 29, 2023 22:39

add prover to Protocol Instance

7d7d7c7

fmt & clippy

a310fd5

smtmfft reviewed Oct 30, 2023

View reviewed changes

Brechtpd reviewed Oct 30, 2023

View reviewed changes

CeciliaZ030 added 8 commits November 1, 2023 14:12

address github comments

d6e2ec3

Merge branch 'for-a6-release' into a6-pi

12e81a0

update lock

ede391d

fmt & clippy

dfd3d4a

fix merge error

75af7b8

fmt again

e217934

update

a95c28c

update

8dffa59

CeciliaZ030 merged commit 49ac689 into feat/for-a6-release Nov 1, 2023
15 of 20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A6 public input circuit #161

A6 public input circuit #161

CeciliaZ030 commented Oct 9, 2023

Brechtpd left a comment

smtmfft left a comment

smtmfft Oct 30, 2023

Brechtpd Oct 30, 2023

CeciliaZ030 Nov 1, 2023

smtmfft Oct 30, 2023

CeciliaZ030 Nov 1, 2023

smtmfft Oct 30, 2023

CeciliaZ030 Nov 1, 2023 •

edited

Loading

smtmfft Oct 30, 2023

smtmfft Oct 30, 2023

Brechtpd Oct 30, 2023

smtmfft Oct 30, 2023

Brechtpd Oct 30, 2023

smtmfft Oct 31, 2023

Brechtpd Oct 31, 2023

CeciliaZ030 Nov 1, 2023 •

edited

Loading

Brechtpd left a comment

Brechtpd Oct 30, 2023

Brechtpd Oct 30, 2023

Brechtpd Oct 30, 2023

Brechtpd Oct 30, 2023


		/// PublicData contains all the values that the PiCircuit receives as input
		const S1: PiCellType = PiCellType::StoragePhase1;

		@@ -34,7 +34,6 @@ impl<F: Field> ExecutionGadget<F> for JumpGadget<F> {

		// Pop the value from the stack
		cb.stack_pop(destination.expr());

A6 public input circuit #161

A6 public input circuit #161

Conversation

CeciliaZ030 commented Oct 9, 2023

Description

Brechtpd left a comment

Choose a reason for hiding this comment

smtmfft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CeciliaZ030 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CeciliaZ030 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Brechtpd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CeciliaZ030 Nov 1, 2023 •

edited

Loading

CeciliaZ030 Nov 1, 2023 •

edited

Loading