Skip to content

Keccak-f[1600] gate#919

Closed
onesk wants to merge 1 commit intomainfrom
08-29-keccakf-gate
Closed

Keccak-f[1600] gate#919
onesk wants to merge 1 commit intomainfrom
08-29-keccakf-gate

Conversation

@onesk
Copy link
Copy Markdown
Contributor

@onesk onesk commented Sep 2, 2025

Introducing an intrinsic Keccakf1600 opcode, which instantiates an optimal 600 AND constraint circuit.

The keccak example has a --no-intrinsic flag to facilitate comparison with the gate fusion approach.

Copy link
Copy Markdown
Contributor Author

onesk commented Sep 2, 2025


How to use the Graphite Merge Queue

Add the label merge-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@onesk onesk force-pushed the 08-29-keccakf-gate branch 6 times, most recently from d0cca1a to 31d1a20 Compare September 3, 2025 08:40
@onesk onesk requested review from jimpo and pepyakin September 3, 2025 08:41
@onesk onesk marked this pull request as ready for review September 3, 2025 08:41
@onesk onesk force-pushed the 08-29-keccakf-gate branch from 31d1a20 to af2771c Compare September 3, 2025 08:47
Comment on lines +507 to +510
for val in a {
let reg = self.read_reg();
self.store(ctx, reg, val);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Bug: Incorrect State Writeback in Keccak-f[1600] Implementation

There's a significant issue with the state writeback logic in the exec_keccakf1600 function. Currently, the writeback loop (lines 507-510) is inside the round loop, causing state values to be written back after each round:

for &round_constant in &CONSTS[..24] {
    // θ, ρ, π, χ, ι steps...
    
    // writeback - THIS IS THE PROBLEM
    for val in a {
        let reg = self.read_reg();
        self.store(ctx, reg, val);
    }
}

This is incorrect because:

  1. It overwrites intermediate state values 24 times
  2. It expects 24×25 output registers when the gate only provides 25
  3. It produces incorrect permutation results

The writeback loop should be moved outside and after the round loop, so it executes only once after all 24 rounds are complete:

for &round_constant in &CONSTS[..24] {
    // θ, ρ, π, χ, ι steps...
}

// writeback (only once, after all rounds)
for val in a {
    let reg = self.read_reg();
    self.store(ctx, reg, val);
}

This change will ensure the correct final state is written to the output registers.

Suggested change
for val in a {
let reg = self.read_reg();
self.store(ctx, reg, val);
}
}
// Writeback (only once, after all rounds)
for val in a {
let reg = self.read_reg();
self.store(ctx, reg, val);
}

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onesk WDYT?


use super::gate::keccakf1600::{CONSTS, R};

fn idx(x: usize, y: usize) -> usize {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: Is there a reason not to reuse the public idx function from keccakf1600?

Copy link
Copy Markdown
Contributor

suggestion: Add a test in keccakf1600 that builds a circuit using the keccakf1600 function and compares the circuit output_state with the expected state as computed by the keccak_f1600_reference function.
https://github.com/IrreducibleOSS/monbijou/blob/bce5c61aecd456c227e3f0bf94a95a4013433046/crates/frontend/src/circuits/keccak/reference.rs#L146

Copy link
Copy Markdown
Collaborator

@jimpo jimpo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to do the job, thanks for plowing through this! I'd like to revert this once gate fusion is working properly. Or hold off on merging until we see if gate fusion produces the expected circuit.

// θ
let c: [Vec<RotWire>; 5] =
array::from_fn(|i| (0..5).map(|j| plain(pre[idx(i, j)])).collect());
let d: [Vec<RotWire>; 5] =
Copy link
Copy Markdown
Collaborator

@jimpo jimpo Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure committing the d wires would make the shift reduction more efficient. The main cost in shift reduction is the total number of different ShiftedWireIndices referenced (regardless of how many different constraints/operands reference them). When d is not committed, each rotation in the ρ steps applies to 5x as many wires.

@onesk
Copy link
Copy Markdown
Contributor Author

onesk commented Sep 9, 2025

This is superseded by gate fusion. Closing down.

@onesk onesk closed this Sep 9, 2025
@jimpo jimpo deleted the 08-29-keccakf-gate branch November 21, 2025 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants