feat: Enable more optional tables #724

sai-deng · 2024-10-14T16:16:05Z

In addition to Keccak tables #620 , this PR enables BytePacking, Logic, MemAfter, and Poseidon tables as optional tables in the root circuit, reducing table proving time and recursion time.

Root circuit size change:

Before this PR

Degree before blinding & padding: 39137
Degree after blinding & padding: 65536

After this PR

Degree before blinding & padding: 43738
Degree after blinding & padding: 65536

Recursion Proving Time After this PR:

CPU halted after 255 cycles
CPU halted after 255 cycles
CPU trace padded to 256 cycles
Keccak table not in use
KeccakSponge table not in use
Logic table not in use
Trace lengths (before padding): TraceCheckpoint { arithmetic_len: 65, byte_packing_len: 2, cpu_len: 256, keccak_len: 0, keccak_sponge_len: 0, logic_len: 0, memory_len: 65443 }, mem_before_len: 64547, mem_after_len: 64557
4.2065s to Recursion Time
 | 0.9084s to Normal recursion
 | 0.3007s to Normal recursion
 | 0.2974s to Normal recursion
 | 0.0007s to Dummy recursion
 | 0.0001s to Dummy recursion
 | 0.0002s to Dummy recursion
 | 0.8890s to Normal recursion
 | 0.8868s to Normal recursion
 | 0.9231s to Normal recursion

CPU halted after 255 cycles
CPU halted after 255 cycles
CPU trace padded to 256 cycles
Trace lengths (before padding): TraceCheckpoint { arithmetic_len: 62, byte_packing_len: 4, cpu_len: 256, keccak_len: 24, keccak_sponge_len: 1, logic_len: 5, memory_len: 65455 }, mem_before_len: 64557, mem_after_len: 64558
9.5262s to Recursion Time
 | 0.9100s to Normal recursion
 | 0.3236s to Normal recursion
 | 0.3210s to Normal recursion
 | 2.8789s to Normal recursion
 | 1.5076s to Normal recursion
 | 0.8986s to Normal recursion
 | 0.9040s to Normal recursion
 | 0.8762s to Normal recursion
 | 0.9063s to Normal recursion

This reverts commit f63363b.

…le_keccak_optional_proving

sai-deng · 2024-10-16T15:44:02Z

I have successfully tested this PR on block 448 in the test chain.

LindaGuiga

LGTM, thanks! Just a couple of nits

evm_arithmetization/src/fixed_recursive_verifier.rs

evm_arithmetization/src/generation/mod.rs

evm_arithmetization/src/proof.rs

evm_arithmetization/src/testing_utils.rs

sai-deng · 2024-10-18T00:33:08Z

LGTM, thanks! Just a couple of nits

Thank you for the careful review!

evm_arithmetization/src/generation/mod.rs

evm_arithmetization/src/all_stark.rs

Nashtare

Thanks Sai, nice improvement overall! Just a couple nitpicky comments for readabily, otherwise looks good

Nashtare · 2024-10-18T14:44:47Z

evm_arithmetization/src/generation/mod.rs

+    let mut table_in_use = [true; NUM_TABLES];
+    if state.traces.keccak_inputs.is_empty() && OPTIONAL_TABLE_INDICES.contains(&Table::Keccak) {
+        assert!(OPTIONAL_TABLE_INDICES.contains(&Table::KeccakSponge));
+        info!("Keccak and KeccakSponge tables not in use");


I think my outdated comment got lost during refactoring, I was suggesting to remove all these info! messages as they can get a bit noisy and are sharing information that is already guessable from the TraceCheckpoint displayed on every segment proof.

Actually, they serve different purposes. We have the config to choose which table should be optional, but I’m fine with changing them to log::debug.

Yeah I'd rather at least put them one level lower, to Debug. Info should really be only for meaningful outputs.

Nashtare · 2024-10-18T14:56:08Z

evm_arithmetization/src/generation/mod.rs

+    if final_len == 0 && OPTIONAL_TABLE_INDICES.contains(&MemAfter) {
+        info!("MemAfter table not in use");
+        table_in_use[*MemAfter] = false;
+    }


nit: we don't need to expose final_len. MemAfter is empty when there is nothing to propagate (final segment), which can be retrieved as so:

Suggested change

if final_len == 0 && OPTIONAL_TABLE_INDICES.contains(&MemAfter) {

info!("MemAfter table not in use");

table_in_use[*MemAfter] = false;

}

let is_last_segment = next_data.registers_after.program_counter == KERNEL.global_labels["halt"];

if is_last_segment && OPTIONAL_TABLE_INDICES.contains(&MemAfter) {

table_in_use[*MemAfter] = false;

}

The bool table_in_use is directly dependent on the length of trace rows. If there’s any change between is_last_segment and the length of trace rows (although it’s unlikely), then mem_after will not be empty, and we might encounter some bugs.

Hmm I can't see why we would ever get a non-empty final MemAfter, as there is nothing to propagate though, and we would catch the discrepancy anyway as we'd have conflicts in the challenges

Nashtare · 2024-10-18T14:57:59Z

evm_arithmetization/src/prover.rs

-                    &ctl_data_per_table[*$table],
-                    ctl_challenges,
-                    challenger,
+            if !OPTIONAL_TABLE_INDICES.contains(&$table) || table_in_use[*$table] {


Suggested change

if !OPTIONAL_TABLE_INDICES.contains(&$table) || table_in_use[*$table] {

if table_in_use[*$table] {

slightly more concise, and the shortcut from the first bool condition is not significant enough

Nashtare · 2024-10-18T15:57:46Z

I forgot to mention it in the review, but a next step could be to also skip altogether the commit phase for each of these tables, especially as some of these tables are padded to a default value larger than the cap length due to their permutation argument.

sai-deng · 2024-10-18T16:07:03Z

I forgot to mention it in the review, but a next step could be to also skip altogether the commit phase for each of these tables, especially as some of these tables are padded to a default value larger than the cap length due to their permutation argument.

Yes, this would require a significant refactor of how CTL works, including changes to Plonky2, with minimal performance gains. I’ll work on it, but I believe it’s a low priority for now

Nashtare · 2024-10-18T16:45:09Z

I forgot to mention it in the review, but a next step could be to also skip altogether the commit phase for each of these tables, especially as some of these tables are padded to a default value larger than the cap length due to their permutation argument.

Yes, this would require a significant refactor of how CTL works, including changes to Plonky2, with minimal performance gains. I’ll work on it, but I believe it’s a low priority for now

Yeah we can just make it a tracking ticket for now.

sai-deng · 2024-10-18T18:23:00Z

Thanks for reviewing! Comments are addressed, and the PR is merged.

sai-deng added 30 commits September 21, 2024 09:13

refactoring

662c8db

fix clippy

d82b1a8

fix clippy

4498870

wip

b14417f

wip

eabcf5e

Merge branch 'develop' into sai/skip_table_in_root_circuit

2ad3857

wip

2b2e737

cleanup

f63363b

prove one segment

cc9d41b

polish the code

fe88819

cleanup

7ff2fb1

Revert "cleanup"

6ce3cbe

This reverts commit f63363b.

add [ignore]

8c940b0

add challenger state check

d6af500

add more checks

f5deedb

cleanup

e39894a

add empty keccak tables bool

948d742

enable no keccak recursion

49bb96b

refactor

bbba820

fix tests

8c4a362

rename

5cdb37d

fix pis

2f0129e

fix

36ace10

resolve conflicts

5b7f818

done

56deaa3

polish the comments

7a3fe08

Merge branch 'sai/conditionally_verify_in_root_circuit' into sai/enab…

cfc6f24

…le_keccak_optional_proving

fix ci

4d288f8

add comments

e6bed69

fix ci

0bd1448

sai-deng self-assigned this Oct 15, 2024

sai-deng marked this pull request as ready for review October 15, 2024 17:56

sai-deng requested review from wborgeaud, muursh, Nashtare and LindaGuiga as code owners October 15, 2024 17:56

sai-deng mentioned this pull request Oct 15, 2024

Optimize zkVM Proving by Skipping Unused Keccak Tables #690

Merged

sai-deng added 5 commits October 15, 2024 14:43

address comments

564b950

fix a bug with index_verifier_data

c8f03fc

Merge branch 'develop' into sai/refactor_optional_table_proving

3581d50

merge

70e2f5b

fmt

f696d57

Base automatically changed from sai/refactor_optional_table_proving to develop October 15, 2024 22:20

sai-deng requested a review from atanmarko as a code owner October 15, 2024 22:20

Merge branch 'develop' into sai/more_optional_tables

7e1a4dc

LindaGuiga approved these changes Oct 17, 2024

View reviewed changes

sai-deng added 3 commits October 17, 2024 17:20

address comments

4ff6a52

fix

b66cdba

merge develop

7c8cde6

Nashtare reviewed Oct 18, 2024

View reviewed changes

evm_arithmetization/src/generation/mod.rs Outdated Show resolved Hide resolved

evm_arithmetization/src/all_stark.rs Show resolved Hide resolved

Nashtare approved these changes Oct 18, 2024

View reviewed changes

address comments

6c8c8b9

sai-deng merged commit ae6c3c4 into develop Oct 18, 2024
20 checks passed

sai-deng deleted the sai/more_optional_tables branch October 18, 2024 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable more optional tables #724

feat: Enable more optional tables #724

sai-deng commented Oct 14, 2024 •

edited

Loading

sai-deng commented Oct 16, 2024

LindaGuiga left a comment

sai-deng commented Oct 18, 2024

Nashtare left a comment

Nashtare Oct 18, 2024

sai-deng Oct 18, 2024

Nashtare Oct 18, 2024

Nashtare Oct 18, 2024

sai-deng Oct 18, 2024

Nashtare Oct 18, 2024

Nashtare Oct 18, 2024

Nashtare commented Oct 18, 2024

sai-deng commented Oct 18, 2024

Nashtare commented Oct 18, 2024

sai-deng commented Oct 18, 2024

	if !OPTIONAL_TABLE_INDICES.contains(&$table) \|\| table_in_use[*$table] {
	if table_in_use[*$table] {

feat: Enable more optional tables #724

feat: Enable more optional tables #724

Conversation

sai-deng commented Oct 14, 2024 • edited Loading

sai-deng commented Oct 16, 2024

LindaGuiga left a comment

Choose a reason for hiding this comment

sai-deng commented Oct 18, 2024

Nashtare left a comment

Choose a reason for hiding this comment

Nashtare Oct 18, 2024

Choose a reason for hiding this comment

sai-deng Oct 18, 2024

Choose a reason for hiding this comment

Nashtare Oct 18, 2024

Choose a reason for hiding this comment

Nashtare Oct 18, 2024

Choose a reason for hiding this comment

sai-deng Oct 18, 2024

Choose a reason for hiding this comment

Nashtare Oct 18, 2024

Choose a reason for hiding this comment

Nashtare Oct 18, 2024

Choose a reason for hiding this comment

Nashtare commented Oct 18, 2024

sai-deng commented Oct 18, 2024

Nashtare commented Oct 18, 2024

sai-deng commented Oct 18, 2024

sai-deng commented Oct 14, 2024 •

edited

Loading