PI refactor #138

CeciliaZ030 · 2023-08-20T04:23:57Z

Description

Refactor the PI circuit in circuit-tool style.

Brechtpd · 2023-08-20T15:50:47Z

zkevm-circuits/src/taiko_pi_circuit.rs

+        meta.create_gate("keccak(rpi)", |meta| {
+            cb.restart();


Much cleaner to just move everything in a single gate like the MPT circuit, then also no restart necessary (and I guess not always restarted now, so constraints are added multiple times now per gate because they accumulate)?

Glad that you like it :) No restart needed.
What do you mean accumulate tho? If you're talking about block_acc it should be applied per row within the left field bytes section.

I meant that the multiple gates introduces extra things people can forget and make errors with. Because each gate does build_constraints, if you don't call restart between the different gates, constraints will keep getting added to the list in the constrain builder and those will keep being added when calling build_constraints, so that will also add the constraints in the previous gates again.

Conclusion: gates bad! haha

Brechtpd · 2023-08-20T15:51:44Z

zkevm-circuits/src/taiko_pi_circuit.rs

+                    require!(a!(rpi_field_bytes_acc) => a!(rpi_field_bytes));
+                });
+            });
+            cb.build_constraints(None)


Why did you add an option for an additional selector here? People should just use ifx instead no?

Because I don't like too much indentation with lots of ifx 😅 (aesthetically unpleasing given macro's ! and parentheses) and I feel like if there's must be a fixed selector enabling of circuits, why don't we generate the constraint "anchoring" this gate.

If you write normal rust code, would you introduce a gate like construct to make it cleaner? If the answer is no, why would you do it here? Does normal rust code have too many nested if's? I don't think so, you just split things up over multiple functions in a way that makes sense. So why not do exactly the same with circuits? The whole of MPT is written using a single gate, I don't think there was any problem there with this because it's split up mostly like you would split up a normal rust program.

why don't we generate the constraint "anchoring" this gate.

Because then the flow control is split into multiple parts and gates don't allow if/else statements, so I find it harder to read and it's not something normal programmers are used to.

It's also not possible to do gates inside gates. So even if you like using gates, it's impossible to use it except on the top level of the program. So it's a strange concept to rely on because if you like to use it to split up your program, it only works for tiny programs that have only 1 level complexity, anything more complex does require using if/else statement.

It's also not directly compatible with regions/context management, so this would require a different path to make it behave like the normal control flow using ifs/matches.

CeciliaZ030 · 2023-08-21T09:55:57Z

I have one concern tho... @Brechtpd do you think for bytes: [Cell<F>; N] within FieldBytesGadget I should directly fill in in increment with r?
Remember in the original accumulation column increment all the way and it was just one constraint like next = cur * r + byte which has low degree, but mine has block_accc[i+1] = ((byte0 * r) + byte1 * r) * ... + byte31 which causes high degree. I know we can reduce it with split_expression but that's also some amount of overhead.
For simple circuit like this refactoring with circuit-tools i just worry about performance degrade in general 😭 and whether it's worthy to tradeoff performance over readability.

Brechtpd · 2023-08-21T11:20:51Z

I have one concern tho... @Brechtpd do you think for bytes: [Cell<F>; N] within FieldBytesGadget I should directly fill in in increment with r? Remember in the original accumulation column increment all the way and it was just one constraint like next = cur * r + byte which has low degree, but mine has block_accc[i+1] = ((byte0 * r) + byte1 * r) * ... + byte31 which causes high degree. I know we can reduce it with split_expression but that's also some amount of overhead. For simple circuit like this refactoring with circuit-tools i just worry about performance degrade in general 😭 and whether it's worthy to tradeoff performance over readability.

Challenges behave like constants so they do not increase the expression degree when multiplying with them, so I think the expression degree here should still be low.
Yeah only use the cell manager when it makes sense. If something is just column based and easy to understand, no reason to force the cell manager into it.

Brechtpd

The version in taiko_pi_circuit_.rs I think could also potentially work, but yeah the cost of the current version will be much higher because of many more columns and A LOT more lookups, so the performance will be much worse. But to solve that it just needs to be possible to make the CellManager be a single column (or maybe two columns), which seems to be possible to me.

On the other hand, this will be very close to just using the taiko_pi_circuit.rs version where everything is still assumed to be on a single row, and just put every cell used on that row in a cellmanager. The cellmanager doesn't do much in this case, but it still does away with having to assign things to columns instead of allocated cells. I currently lean towards this approach because it will be easier.

To print the most important info for the circuit performance:

println!("max expression degree: {}", meta.degree());
println!("num lookups: {}", meta.lookups().len());
println!("num advices: {}", meta.num_advice_columns());
println!("num fixed: {}", meta.num_fixed_columns());

Brechtpd · 2023-08-21T23:27:11Z

zkevm-circuits/src/taiko_pi_circuit_.rs

+            cb.set_cell_manager(state_cm.clone());
+            let block_number = [();2].map(|_| cb.query_one(PiCellType::Storage1));
+            let keccak_output = [();2].map(|_| cb.query_one(PiCellType::Storage2));


Doe these need to be stored in a separate CellManager? Seems like these could just be stored somewhere in the main CellManager and it should still be okay? keccak_output requires a copy constraint I think which should be possible I think, block_number doesn't seem to require anything special, just some storage cell, unless I miss something.

Brechtpd · 2023-08-21T23:35:47Z

zkevm-circuits/src/taiko_pi_circuit_.rs

+    is_field: Cell<F>,
+    is_first: Cell<F>,
+    is_last: Cell<F>,


These are advice cells, but seems like they should be fixed cells? (should not be possible for the prover to set these values).

Brechtpd · 2023-08-21T23:38:54Z

zkevm-circuits/src/taiko_pi_circuit_.rs

+               );
+            let q_enable = meta.complex_selector();
+            let public_input = meta.instance_column();
+            let block_acc = meta.advice_column_in(SecondPhase);


Why not also put it in the CellManager and just use rot with the height?

Brechtpd · 2023-08-21T23:41:35Z

zkevm-circuits/src/taiko_pi_circuit_.rs

+                    (PiCellType::Storage1, 3, 1, false)
+                ],
+                0,
+                1,


I think with a single CellManager it will be easier to just make this height be whatever, and then the other code should be able to use the same height in rot to check between the different states, and in the witness assignment.

smtmfft · 2023-08-29T06:30:27Z

Does this support new PI format?? https://github.com/taikoxyz/taiko-mono/blob/85bef055c8778a473fff41318b06792c151efa52/packages/protocol/contracts/L1/libs/LibProving.sol#L166C42-L166C50
BTW: evidence: https://github.com/taikoxyz/taiko-mono/blob/85bef055c8778a473fff41318b06792c151efa52/packages/protocol/contracts/L1/TaikoData.sol#L120

smtmfft · 2023-09-01T01:06:26Z

For A5 integration, after functionality test, please merge this PR to feat/a5-testnet branch.

smtmfft · 2023-09-04T06:38:52Z

zkevm-circuits/src/taiko_pi_circuit.rs

+const SIGNAL_ROOT: usize = 3;
+const GRAFFITI: usize = 4;
+const PROVER: usize = 5;
+const S1: PiCellType = PiCellType::Storage1;


StoragePhase1

Sure of you like :)

smtmfft · 2023-09-04T06:39:25Z

zkevm-circuits/src/taiko_pi_circuit.rs

-
-/// PublicData contains all the values that the PiCircuit receives as input
+const PADDING_LEN: usize = 32;
+const META_HASH: usize = 0;


I think const is better cuz enum is wordy, you have to type somethingPiField::MetaHash.

smtmfft · 2023-09-04T06:41:17Z

zkevm-circuits/src/taiko_pi_circuit.rs

+        self.field.iter().map(|f| f.expr()).collect()
+    }
+
+    fn acc(&self, r: Expression<F>) -> Expression<F> {


rlc_acc or rlc?

smtmfft · 2023-09-04T06:41:18Z

zkevm-circuits/src/taiko_pi_circuit.rs

+
+#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
+enum PiCellType {
+    Storage1,


StoragePhase1/2

smtmfft · 2023-09-04T06:52:25Z

zkevm-circuits/src/taiko_pi_circuit.rs

+    fn mock_public_data() -> PublicData<Fr> {
+        let mut evidence = PublicData::default();
+        evidence.set_field(PARENT_HASH, OMMERS_HASH.to_fixed_bytes().to_vec());
+        evidence.set_field(BLOCK_HASH, OMMERS_HASH.to_fixed_bytes().to_vec());


init address once as it has different data format

You mean mock address for prover?

yes, at least test different type once

smtmfft · 2023-09-04T07:03:15Z

zkevm-circuits/src/taiko_pi_circuit.rs

+                assign!(region, self.block_hash.0, 0 => (evidence.block_context.number).as_u64().scalar());
+                assign!(region, self.block_hash.2, 0 => evidence.assignment_acc(BLOCK_HASH, evm_word));
+
+                let _acc = F::ZERO;


useless variable??

smtmfft · 2023-09-04T08:01:25Z

zkevm-circuits/src/taiko_pi_circuit.rs

-            field10,
+impl<F: Field> PublicData<F> {
+    fn new(block: &witness::Block<F>) -> Self {
+        let meta_hash = Token::FixedBytes(block.protocol_instance.meta_hash.hash().to_word().to_be_bytes().to_vec());


hash.hash() for what??

From protocol instance implementation

meta_hash here is already a hash, no need hash again.

In the old version meta_hash is a struct MetaHash which returns the actual hash when you call hash() on itself, but yea now it's already a hash.

CeciliaZ030 added 15 commits July 30, 2023 23:03

init

bcae2ed

runs

4b891d7

conversion macro

998d71a

fixes from mpt

0a66dfd

latest circuit tools

bddf633

rewrite

e803f77

apis done

ca4e47c

cb.equalities

0f52e56

assignments done

362346d

cb build_equalities & build_constraints with sel

d91e1b1

debug constraints

059eeba

query all cells ver

a7e7487

build with sel

8e845aa

update

9ce2b2a

keccak input output is wired

6ee13a8

github-actions bot added crate-gadgets crate-zkevm-circuits labels Aug 20, 2023

Brechtpd reviewed Aug 20, 2023

View reviewed changes

Brechtpd reviewed Aug 21, 2023

View reviewed changes

CeciliaZ030 added 5 commits August 22, 2023 19:23

debug tmp

7abe67e

debug tmp

c9f78dc

yeah

3bb26bb

cleanup

b3df73e

cargo fix

e723a84

CeciliaZ030 marked this pull request as ready for review August 24, 2023 13:12

rewrite for new A5 testnest

4f8d2c0

CeciliaZ030 added 17 commits September 3, 2023 02:07

update

a0c95db

fixing

e12af58

block_hash work

9de1a9a

block acc cell

4ac0a3f

acc

bbfd300

siganl root

530a553

fuckin offset is dumb

857bcf6

parent_hash

791ecff

last two

934773b

hi lo no instance

01ff54b

hi lo instance constraint

925a263

doen

760f642

const

caae2ea

more tests

3956c17

remove files

198adae

fix super

09cb220

cargo fix

54c1764

CeciliaZ030 force-pushed the taiko/pi-refactor branch from e6b14de to 54c1764 Compare September 3, 2023 22:32

CeciliaZ030 requested review from smtmfft and johntaiko September 4, 2023 03:52

smtmfft reviewed Sep 4, 2023

View reviewed changes

CeciliaZ030 mentioned this pull request Sep 4, 2023

PI circuit for A5 testnet #149

Closed

1 task

update

1fd6743

smtmfft approved these changes Sep 25, 2023

View reviewed changes

CeciliaZ030 added 4 commits September 25, 2023 19:32

fmt

e3910ba

fmt --all

f66a583

clippy --all

e884118

update

2cb3e17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PI refactor #138

PI refactor #138

CeciliaZ030 commented Aug 20, 2023

Brechtpd Aug 20, 2023

CeciliaZ030 Aug 21, 2023

Brechtpd Aug 21, 2023

Brechtpd Aug 20, 2023

CeciliaZ030 Aug 21, 2023

Brechtpd Aug 21, 2023

CeciliaZ030 commented Aug 21, 2023 •

edited

Loading

Brechtpd commented Aug 21, 2023

Brechtpd left a comment

Brechtpd Aug 21, 2023

Brechtpd Aug 21, 2023

Brechtpd Aug 21, 2023

Brechtpd Aug 21, 2023

smtmfft commented Aug 29, 2023 •

edited

Loading

smtmfft commented Sep 1, 2023 •

edited

Loading

smtmfft Sep 4, 2023

CeciliaZ030 Sep 4, 2023

smtmfft Sep 4, 2023

CeciliaZ030 Sep 4, 2023

smtmfft Sep 4, 2023

smtmfft Sep 4, 2023

smtmfft Sep 4, 2023

CeciliaZ030 Sep 4, 2023

smtmfft Sep 4, 2023

smtmfft Sep 4, 2023

smtmfft Sep 4, 2023

CeciliaZ030 Sep 4, 2023

smtmfft Sep 4, 2023

CeciliaZ030 Sep 4, 2023 •

edited

Loading

PI refactor #138

Are you sure you want to change the base?

PI refactor #138

Conversation

CeciliaZ030 commented Aug 20, 2023

Description

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 commented Aug 21, 2023 • edited Loading

Brechtpd commented Aug 21, 2023

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

smtmfft commented Aug 29, 2023 • edited Loading

smtmfft commented Sep 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

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 Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

CeciliaZ030 commented Aug 21, 2023 •

edited

Loading

smtmfft commented Aug 29, 2023 •

edited

Loading

smtmfft commented Sep 1, 2023 •

edited

Loading

CeciliaZ030 Sep 4, 2023 •

edited

Loading