Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug of potential orphan input cells #81

Merged
merged 1 commit into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
name: Tests

# We only run these lints on trial-merges of PRs to reduce noise.
on:
merge_group:
pull_request:
types: [synchronize, opened, reopened, ready_for_review]
push:
branches:
- master

jobs:
skip_check:
runs-on: ubuntu-latest
outputs:
should_skip: ${{ steps.skip_check.outputs.should_skip }}
steps:
- id: skip_check
uses: fkirc/skip-duplicate-actions@v5
with:
cancel_others: 'true'
concurrent_skipping: 'same_content_newer'
paths_ignore: '["**/README.md"]'

tests:
needs: [skip_check]
if: |
github.event.pull_request.draft == false &&
(github.event.action == 'ready_for_review' || needs.skip_check.outputs.should_skip != 'true')
name: Run Tests
timeout-minutes: 30
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Cargo cache
uses: actions/cache@v3
with:
path: |
~/.cargo/bin/
~/.cargo/registry/index/
~/.cargo/registry/cache/
~/.cargo/git/db/
target/
key: lint-${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}

- name: Install cargo make
run: |
cargo install --force cargo-make
- name: run test
uses: actions-rs/cargo@v1
with:
command: make
args: tests
4 changes: 2 additions & 2 deletions Makefile.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ CARGO_MAKE_EXTEND_WORKSPACE_MAKEFILE = true
CORE = { script = ["grep ^cpu\\scores /proc/cpuinfo | uniq | awk '{print $4}'"] }
RAYON_NUM_THREADS = "${CORE}"

[tasks.test]
[tasks.tests]
command = "cargo"
args = ["test", "--release", "--all", "--all-features"]
args = ["test", "--lib", "--release", "--all", "--all-features"]

[tasks.fmt-check]
command = "cargo"
Expand Down
63 changes: 61 additions & 2 deletions gkr/src/circuit/circuit_witness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -440,13 +440,13 @@ impl<F: SmallField> Debug for CircuitWitness<F> {

#[cfg(test)]
mod test {
use std::collections::HashMap;
use std::{collections::HashMap, ops::Neg};

use ff::Field;
use ff_ext::ExtensionField;
use goldilocks::GoldilocksExt2;
use itertools::Itertools;
use simple_frontend::structs::{ChallengeConst, ChallengeId, CircuitBuilder};
use simple_frontend::structs::{ChallengeConst, ChallengeId, CircuitBuilder, ConstantType};

use crate::{
structs::{Circuit, CircuitWitness, LayerWitness},
Expand Down Expand Up @@ -977,4 +977,63 @@ mod test {

assert_eq!(circuit_wits, expect_circuit_wits);
}

#[test]
fn test_orphan_const_input() {
// create circuit
let mut circuit_builder = CircuitBuilder::<GoldilocksExt2>::new();

let (_, leaves) = circuit_builder.create_witness_in(3);
let mul_0_1_res = circuit_builder.create_cell();

// 2 * 3 = 6
circuit_builder.mul2(
mul_0_1_res,
leaves[0],
leaves[1],
<GoldilocksExt2 as ExtensionField>::BaseField::ONE,
);

let (_, out) = circuit_builder.create_witness_out(2);
// like a bypass gate, passing 6 to output out[0]
circuit_builder.add(
out[0],
mul_0_1_res,
<GoldilocksExt2 as ExtensionField>::BaseField::ONE,
);

// assert const 2
circuit_builder.assert_const(leaves[2], 5);

// 5 + -5 = 0, put in out[1]
circuit_builder.add(
out[1],
leaves[2],
<GoldilocksExt2 as ExtensionField>::BaseField::ONE,
);
circuit_builder.add_const(
out[1],
<GoldilocksExt2 as ExtensionField>::BaseField::from(5).neg(), // -5
);

// assert out[1] == 0
circuit_builder.assert_const(out[1], 0);

circuit_builder.configure();
let circuit = Circuit::new(&circuit_builder);

let mut circuit_wits = CircuitWitness::new(&circuit, vec![]);
let witness_in = vec![LayerWitness {
instances: vec![vec![i64_to_field(2), i64_to_field(3), i64_to_field(5)]],
}];
circuit_wits.add_instances(&circuit, witness_in, 1);

println!("circuit_wits {:?}", circuit_wits);
let output_layer_witness = &circuit_wits.layers[0];
for gate in circuit.assert_consts.iter() {
if let ConstantType::Field(constant) = gate.scalar {
assert_eq!(output_layer_witness.instances[0][gate.idx_out], constant);
}
}
}
}
11 changes: 10 additions & 1 deletion simple-frontend/src/circuit_builder/base_opt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,16 @@ impl<Ext: ExtensionField> CircuitBuilder<Ext> {
}

pub fn assert_const(&mut self, out: CellId, constant: i64) {
self.mark_cells(CellType::Out(OutType::AssertConst(constant)), &[out]);
// check cell and if it's belong to input cell, we must create an intermediate cell
// and avoid edit input layer cell_type, otherwise it will be orphan cell with no fan-in.
let out_cell = if let Some(CellType::In(_)) = self.cells[out].cell_type {
let out_cell = self.create_cell();
self.add(out_cell, out, Ext::BaseField::ONE);
out_cell
} else {
out
};
self.mark_cells(CellType::Out(OutType::AssertConst(constant)), &[out_cell]);
}

pub fn add_cell_expr(&mut self, out: CellId, in_0: MixedCell<Ext>) {
Expand Down
4 changes: 0 additions & 4 deletions singer/examples/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ use gkr::structs::LayerWitness;
use goldilocks::GoldilocksExt2;
use itertools::Itertools;

const NUM_SAMPLES: usize = 10;
#[from_env]
const RAYON_NUM_THREADS: usize = 8;

use singer::{
instructions::{add::AddInstruction, Instruction, InstructionGraph, SingerCircuitBuilder},
scheme::GKRGraphProverState,
Expand Down
17 changes: 2 additions & 15 deletions singer/src/instructions/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,12 +332,8 @@ mod test {
// The actual challenges used is:
// challenges
// { ChallengeConst { challenge: 1, exp: i }: [Goldilocks(c^i)] }
let c: u64 = 6;
let circuit_witness_challenges = vec![
GoldilocksExt2::from(c),
GoldilocksExt2::from(c),
GoldilocksExt2::from(c),
];
let c = GoldilocksExt2::from(6u64);
let circuit_witness_challenges = vec![c; 3];

let circuit_witness = test_opcode_circuit(
&inst_circuit,
Expand All @@ -346,15 +342,6 @@ mod test {
&phase0_values_map,
circuit_witness_challenges,
);

// check the correctness of add operation
// stack_push = RLC([stack_ts=3, RAMType::Stack=0, stack_top=98, result=0,1,0,0,0,0,0,0, len=11])
// = 3 (stack_ts) + c^2 * 98 (stack_top) + c^4 * 1 + c^11
let add_stack_push_wire_id = inst_circuit.layout.chip_check_wire_id[1].unwrap().0;
let add_stack_push =
&circuit_witness.witness_out_ref()[add_stack_push_wire_id as usize].instances[0][1];
let add_stack_push_value: u64 = 3 + c.pow(2_u32) * 98 + c.pow(4u32) * 1 + c.pow(11_u32);
assert_eq!(*add_stack_push, Goldilocks::from(add_stack_push_value));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This assertion are not valid for extension field which occupied multiple elements, therefore clean up

}

fn bench_add_instruction_helper<E: ExtensionField>(instance_num_vars: usize) {
Expand Down
2 changes: 2 additions & 0 deletions sumcheck/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ fn test_sumcheck_internal<E: ExtensionField>(
}

#[test]
#[ignore = "temporarily not supporting degree > 2"]
fn test_trivial_polynomial() {
test_trivial_polynomial_helper::<GoldilocksExt2>();
}
Expand All @@ -109,6 +110,7 @@ fn test_trivial_polynomial_helper<E: ExtensionField>() {
}

#[test]
#[ignore = "temporarily not supporting degree > 2"]
fn test_normal_polynomial() {
test_normal_polynomial_helper::<GoldilocksExt2>();
}
Expand Down
Loading