Keccak circuit #1

SupremoUGH · 2023-08-24T16:18:27Z

Adds functions for the Saturn Keccak circuit. Goes together with: https://github.com/NebraZKP/Saturn/pull/99

dtebbs

Overall makes sense, with a few little details that are not clear. I think a few comments could help clarify (even though the original code is pretty much devoid of comments), and it might be clearer to replace rather than copy the large phase0 function, if that's feasible.

I was also expecting to see some changes in the test files. (I get errors when running cargo test - it's probably worth doing some basic checks).

It's not obvious to me whether the remaining code still makes sense in the context of some of these changes, but let's keep this diff as small as possible to make the changes obvious.

dtebbs · 2023-08-24T18:36:34Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

@@ -1189,7 +1212,9 @@ impl<F: Field> KeccakCircuitConfig<F> {
        }
        info!("- Post chi:");
        info!("Lookups: {}", lookup_counter);
-        info!("Columns: {}", cell_manager.get_width());
+        let width = cell_manager.get_width();
+        std::env::set_var("COL_TO_ENABLE", (width + 4).to_string());


Could we have a comment explaining why this must be an env var, instead of something we pass as a param somewhere?

Also the magic +4

Basically I didn't want to break the functions' signatures, so I borrowed a page from their book. I agree ideally we should change the defn of CellManager, etc.

About the magic +4: I want to enforce copy constraints on the column on which the output bytes of keccak are stored. This happens 4 columns from now. Maybe I should define a constant and explain its meaning.

Thanks. Yeah, constant couldn't hurt, but the main thing would be to add a comment in the code so the next reader will also understand.

Now it is explained

hashes/zkevm-keccak/src/keccak_packed_multi.rs

dtebbs · 2023-08-25T10:15:24Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

@@ -1546,6 +1510,43 @@ impl<F: Field> KeccakCircuitConfig<F> {
        }
    }

+    /// Assigns the cells in `KeccakRow` to `region`.


Could you explain the extra with_flags part here as well?

hashes/zkevm-keccak/src/keccak_packed_multi.rs

stechu · 2023-09-06T00:14:43Z

hashes/zkevm-keccak/src/keccak_packed_multi/tests.rs

@@ -121,7 +119,6 @@ fn packed_multi_keccak_simple() {
    verify::<Fr>(k, inputs, true);
 }

-/// Cmdline: KECCAK_DEGREE=14 RUST_LOG=info cargo test -- --nocapture packed_multi_keccak_prover


why deleting this?

I deleted the whole testing framework (all relevant tests are in Saturn)

stechu · 2023-09-06T00:39:35Z

The test packed_multi_keccak_prover failed. It complains about:

        KeccakCircuitConfig::new(meta, challenge)

feed two argument, while where it defined:
https://github.com/NebraZKP/halo2-lib/blob/keccak_for_saturn/hashes/zkevm-keccak/src/keccak_packed_multi.rs#L873

only take one.

stechu · 2023-09-06T00:56:08Z

Additional Q: where is keccak_phase0_with_flags getting used?

stechu · 2023-09-06T01:02:52Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

@@ -1954,6 +1950,350 @@ pub fn keccak_phase0<F: Field>(
    }
 }

+/// Witness generation in `FirstPhase` for a keccak hash digest without
+/// computing RLCs, which are deferred to `SecondPhase`.


Can you comment here on what do the flags do?

stechu · 2023-09-06T03:13:09Z

I think I clicked approve accidentally.

SupremoUGH · 2023-09-20T19:59:56Z

Overall makes sense, with a few little details that are not clear. I think a few comments could help clarify (even though the original code is pretty much devoid of comments), and it might be clearer to replace rather than copy the large phase0 function, if that's feasible.

I was also expecting to see some changes in the test files. (I get errors when running cargo test - it's probably worth doing some basic checks).

It's not obvious to me whether the remaining code still makes sense in the context of some of these changes, but let's keep this diff as small as possible to make the changes obvious.

I removed the testing module altogether, it doesn't make sense for us any longer. All relevant tests are in Saturn

dtebbs

That's a lot easier to reason about after the refactor. Thanks.
Just a couple of queries, but otherwise looks good.

hashes/zkevm-keccak/src/keccak_packed_multi.rs

dtebbs · 2023-09-21T13:59:34Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

@@ -1189,7 +1212,9 @@ impl<F: Field> KeccakCircuitConfig<F> {
        }
        info!("- Post chi:");
        info!("Lookups: {}", lookup_counter);
-        info!("Columns: {}", cell_manager.get_width());
+        let width = cell_manager.get_width();
+        std::env::set_var("COL_TO_ENABLE", (width + 4).to_string());


Thanks. Yeah, constant couldn't hurt, but the main thing would be to add a comment in the code so the next reader will also understand.

GhostOfGauss · 2023-09-22T16:56:29Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

+                let cell_offset = absorb_data.rotation as usize;
+                let cell_column_idx = absorb_data.column_idx;
+                // We compute the row index from the cell offset, the current
+                // round and the current chunk
+                let row_index = idx * (NUM_ROUNDS + 1) * num_rows_per_round
+                    + round * num_rows_per_round
+                    + cell_offset;


This computation happens in at least one other place so let's extract it into its own function.

(I know, I know, it seems futile to start doing this kind of thing when you've inherited a 400+ line function to begin with...)

GhostOfGauss · 2023-09-22T16:56:58Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

+                    let cell_offset = cell.rotation as usize;
+                    let cell_column_idx = cell.column_idx;
+                    // We compute the row index from the round number and the cell offset.
+                    let row_index = (num_chunks - 1) * (NUM_ROUNDS + 1) * num_rows_per_round
+                        + round_number * num_rows_per_round
+                        + cell_offset;


See above comment; make this its own function.

GhostOfGauss · 2023-09-22T17:11:17Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

+    pub fn set_row(&self, region: &mut Region<'_, F>, offset: usize, row: &KeccakRow<F>) {
+        self.set_row_with_flags(region, offset, row, &[]);


set_row_with_flags returns a Vec<KeccakAssignedValue<F>>, why don't we use it here?

Or maybe my question is why set_row_with_flags returns it?

GhostOfGauss · 2023-09-22T17:27:41Z

hashes/zkevm-keccak/src/keccak_packed_multi.rs

+            if let Ok(num_flex_advice) = var("FLEX_GATE_NUM_ADVICE").map(|s| {
+                s.parse::<usize>().expect("Cannot parse FLEX_GATE_NUM_ADVICE env var as usize")
+            }) {
+                let col_to_enable =
+                    if num_flex_advice == 1 { num_flex_advice + 1 } else { num_flex_advice + 2 };
+                if advice.index() == col_to_enable {
+                    meta.enable_equality(advice);
+                }
+            }
+            // This is the column where the output bytes are stored. It happens at the last keccak column,
+            // whose index will be shifted by the number of flex gate columns.
+            if let Ok(last_keccak_column) = var("LAST_KECCAK_COLUMN").map(|s| {
+                s.parse::<usize>().expect("Cannot parse LAST_KECCAK_COLUMN env var as usize")
+            }) {
+                if let Ok(num_flex_advice) = var("FLEX_GATE_NUM_ADVICE").map(|s| {
+                    s.parse::<usize>().expect("Cannot parse FLEX_GATE_NUM_ADVICE env var as usize")
+                }) {
+                    let col_to_enable = last_keccak_column
+                        + if num_flex_advice == 1 { num_flex_advice } else { num_flex_advice + 1 };
+                    if advice.index() == col_to_enable {
+                        meta.enable_equality(advice);
+                    }
+                }
+            }


These two blocks are pretty similar, there's probably a function that could be abstracted out and called here. Maybe in the docs of that function you could elaborate a bit on what's going on, because I don't quite follow.

SupremoUGH added 2 commits August 24, 2023 18:17

keccak stuff

fd1b5fc

removed unnecessary stuff

088c3c6

SupremoUGH marked this pull request as ready for review August 24, 2023 16:41

SupremoUGH requested review from dtebbs and GhostOfGauss August 24, 2023 16:41

dtebbs requested changes Aug 25, 2023

View reviewed changes

stechu reviewed Sep 6, 2023

View reviewed changes

stechu approved these changes Sep 6, 2023

View reviewed changes

SupremoUGH added 3 commits September 19, 2023 15:32

changes for the new version

bebdb5e

refactor the code

c52036c

remvoed tests and added comments

75fe331

SupremoUGH requested review from dtebbs and stechu September 20, 2023 20:00

dtebbs requested changes Sep 21, 2023

View reviewed changes

SupremoUGH added 4 commits September 22, 2023 11:49

dep update

af356b6

column issue fix

337a991

fix enabling last keccak column

0640148

fix the fix

6649c6d

GhostOfGauss reviewed Sep 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keccak circuit #1

Keccak circuit #1

SupremoUGH commented Aug 24, 2023 •

edited

Loading

dtebbs left a comment

dtebbs Aug 24, 2023

dtebbs Aug 25, 2023

SupremoUGH Sep 7, 2023

dtebbs Sep 21, 2023

SupremoUGH Sep 22, 2023

dtebbs Aug 25, 2023

SupremoUGH Sep 20, 2023

stechu Sep 6, 2023

SupremoUGH Sep 20, 2023

stechu commented Sep 6, 2023

stechu commented Sep 6, 2023

stechu Sep 6, 2023

SupremoUGH Sep 20, 2023

stechu commented Sep 6, 2023

SupremoUGH commented Sep 20, 2023

dtebbs left a comment

dtebbs Sep 21, 2023

GhostOfGauss Sep 22, 2023

GhostOfGauss Sep 22, 2023

GhostOfGauss Sep 22, 2023

GhostOfGauss Sep 22, 2023

GhostOfGauss Sep 22, 2023

		pub fn set_row(&self, region: &mut Region<'_, F>, offset: usize, row: &KeccakRow<F>) {
		self.set_row_with_flags(region, offset, row, &[]);

Keccak circuit #1

Are you sure you want to change the base?

Keccak circuit #1

Conversation

SupremoUGH commented Aug 24, 2023 • edited Loading

dtebbs 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stechu commented Sep 6, 2023

stechu commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stechu commented Sep 6, 2023

SupremoUGH commented Sep 20, 2023

dtebbs 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

SupremoUGH commented Aug 24, 2023 •

edited

Loading