fix: Correct soundness bug in keccak circuit

[CPerezz]

Once Keccak was migrated to the Challenge API in #925, there was a column containing data_rlcs which was getting stored values that were the result of an RLC.

Therefore, this swaps the data_rlcs colum Phase to SecondPhase.

Resolves: #1016

[ed255]

It's not clear to me that this function will always return a cell belonging to a column at phase. Notice the path when column_idx < self.columns.len(), the phase parameter is ignored, and we don't know which is the phase of self.columns[column_idx].

AFAIK, the cell manager here returns cells in this order (assume height = 3)

Col1	Col2	Col3
0	3	6
1	4	7
2	5	8

The queries 0, 3 and 6 cause the meta.advice_column_in(phase). And the rest of the queries will reuse the column at whichever phase was chosen before.

I think the cell manager needs to be duplicated. One cell manager for phase1, one cell manager for phase2. Each one will create columns in the corresponding phase. And then cells for data_rlcs will be allocated with cell_manager_phase2.query_cell(meta), where as the rest of the cells will be queried with cell_manager_phase1.query_cell(meta).

[lispc]

Hi @CPerezz is the pr (almost) done？we may consider directly pick this patch to our fork to make a super circuit with real non mock challenge api

[CPerezz]

@lispc indeed this work was left aside as it implies a significant refactor. And we prefer to pospone it until we have the hi-lo specs ready.
So that we can evaluate how worth is to update this or refactor specs directly.

The issue in here is that duplicating the cellmanager isn't enough. THerefore we need to make deep changes on how the circuit is coded to allow this to work propperly.

[ed255]

I've added the wontfix label following this comment #1016 (comment)

[aguzmant103]

Closed in favor a future PR that Scroll team will send