Blobkzg Circuit for EIP 4844 #106

mabbamOG · 2023-06-05T11:49:19Z

Description

This PR adds the circuit required to integrate Taiko with EIP 4844. Basically it takes data stored in the circuit and uses it for function evaluation. This is then compared with the blob opening checked in the smart contract with EIP 4844. Make sure the data fed into this circuit is checked to be the same as all the other claims of the proof.

Issue Link

[link issue here]

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Rationale

Required for the L2 to make use of EIP 4844 Blobs, greatly improving costs for publishing new proofs

How Has This Been Tested?

Unit tests (more to come)

xiaodino · 2023-06-06T16:25:59Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+
+    fn configure(meta: &mut ConstraintSystem<N>) -> Self::Config {
+        let integer_config = {
+            const BIT_LEN_LIMB: usize = 68;


Move constant variable declarations BIT_LEN_LIMB and NUMBER_OF_LIMBS to the top of a file?

xiaodino · 2023-06-06T16:32:58Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+            ctx: &mut RegionCtx<N>,
+            x: W
+        ) -> Result<AssignedInteger<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB>, Error> {
+            let rc_rns: std::rc::Rc<Rns<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB>> = std::rc::Rc::new(Rns::<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB>::construct());


Could we change the code format to

let rc_rns = std::rc::Rc::new(Rns::<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB>::construct());

?

sure, was just using explicit typing bc in this part of the code I had to be really careful to the types

xiaodino · 2023-06-06T16:35:04Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+            let v: Value<Integer<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB>> = Value::known(i);
+            let u: UnassignedInteger<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB> = UnassignedInteger::from(v);
+            // TODO: is Range::Operand a correct range here?
+            let a: AssignedInteger<W, N, NUMBER_OF_LIMBS, BIT_LEN_LIMB> = integer_chip.assign_integer(ctx, u, Range::Operand)?;


What is a for?

assigned integer

xiaodino · 2023-06-06T16:40:08Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+use halo2_proofs::circuit::{Layouter,SimpleFloorPlanner,Value};
+use halo2_proofs::halo2curves::ff::PrimeField as FieldExt;
+use halo2wrong_integer::{IntegerConfig, rns::Rns, IntegerChip, IntegerInstructions, rns::Integer, UnassignedInteger, Range, AssignedInteger};
+use halo2wrong_maingate::{RangeChip, MainGate, RegionCtx};


What is the motivation to use halo2wrong_maingate here?

it's required for some operations, I think to allow me to setup the IntegerConfig with its underlying Range gate Config and Maingate Config. also I think that's where the RegionCtx operations come from.

AlekseiVambol · 2023-06-07T14:44:04Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+            }
+            x2is
+        };
+        let nbits: [W; 256] = {


Unfortunately, I do not no Halo2Wrong API and do not sure that I have correctly understood the related parts. However, if they work as I suppose and bit factorization of n (log2 of the order of w) is predefined, than I think that it should work correctly. This part of the code (the lines 54-71) can be written in a shorter way (defining nbits[j*8+i] directly without the boolean array), but it is not a problem.

Brechtpd

Please use cargo fmt to format the code. Yeah I know it messes up the code how you want it to look, but it does make it easier to see the code in a nice standard way.

It seems like some tests did not pass as expected:

running 6 tests
test tests::wrong_witness_data ... FAILED
test tests::wrong_x - should panic ... ok
test tests::try_default has been running for over 60 seconds
test tests::try_random_datapoints has been running for over 60 seconds
test tests::wrong_n has been running for over 60 seconds
test tests::wrong_w has been running for over 60 seconds
test tests::try_default ... ok
test tests::wrong_n - should panic ... FAILED
test tests::wrong_w - should panic ... FAILED
test tests::try_random_datapoints ... ok

failures:

---- tests::wrong_witness_data stdout ----
thread 'tests::wrong_witness_data' panicked at 'not implemented', src/main.rs:519:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::wrong_n stdout ----
note: test did not panic as expected
---- tests::wrong_w stdout ----
note: test did not panic as expected

failures:
    tests::wrong_n
    tests::wrong_w
    tests::wrong_witness_data

test result: FAILED. 3 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 144.90s

wrong_witness_data makes sense how it is now (but please change it so all tests pass even if still to do), but the others seems like unexpected.

Brechtpd · 2023-06-11T16:22:57Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+
+#[derive(Debug)]
+pub struct BlobKZGCircuit<W: PrimeField, N: PrimeField>{ // W is the wrong field, N is the native field
+    datapoints: [W; 4096],


Will be good to use const variables everywhere like NUM_POINTS instead of the constants like 4096/256/... themselves.

Brechtpd · 2023-06-11T16:24:15Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+use halo2wrong_maingate::{RangeChip, MainGate, RegionCtx};
+
+/// THIS CIRCUIT PERFORMS A NON-NATIVE FUNCTION EVAL USING BARYCENTRIC-FORMULA
+/// P(X) = (const) * sum (d_i * (w^i/x-w^i)) where const = (x^N - 1)/N with w^N = 1 and X^N != 1


Will be nice to have some more comments explaining about the exact approach, and which values are calculated in the circuit and how, and which values are precomputed. Some comments which variable is what because the same variable names are used in the code would also be nice.

Brechtpd · 2023-06-11T16:50:04Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+impl<W: PrimeField> WitnessValues<W> {
+    fn new(x:&W, n:&W, w:&W, datapoints: &[W; 4096]) -> WitnessValues<W> {
+        let x2is: [W; 256] = {
+            let mut x2is = [W::default(); 256];


Instead of W::default(), let's just use W::ZERO everywhere, that makes things more clear.

Brechtpd · 2023-06-11T17:06:40Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+    }
+}
+impl<W: PrimeField> WitnessValues<W> {
+    fn new(x:&W, n:&W, w:&W, datapoints: &[W; 4096]) -> WitnessValues<W> {


Can use some simple comments for each step here so it's more clear what it being done and why. For example one big one could be // Split up n into bits so we can calculate the exponent using the square and multiply. and the x2is code would be // Calculate the exponent for each bit position. You get the idea.

Brechtpd · 2023-06-11T17:53:24Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+    fn new(x:&W, n:&W, w:&W, datapoints: &[W; 4096]) -> WitnessValues<W> {
+        let x2is: [W; 256] = {
+            let mut x2is = [W::default(); 256];
+            let mut state = *x;


Instead of the name state, acc or something like that is a more standard name I think.

Brechtpd · 2023-06-11T21:53:27Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+            let assigned_x = assign_wrong_integer(&integer_chip, &mut ctx, self.x)?;
+            let assigned_w = assign_wrong_integer(&integer_chip, &mut ctx, self.w)?;
+            let assigned_zero = assign_wrong_integer(&integer_chip, &mut ctx, W::ZERO)?;
+            let assigned_one = assign_wrong_integer(&integer_chip, &mut ctx, W::ONE)?;
+            let assigned_n = assign_wrong_integer(&integer_chip, &mut ctx, self.n)?;


All of these values (and some below here as well) are being assigned using assign_wrong_integer which internally will use assign_integer. But some of these are constants and need to have a fixed value, so shouldn't those use assign_constant?

Brechtpd · 2023-06-11T21:54:03Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+            }).collect::<Result<Vec<_>, Error>>()?;
+            assert!(assigned_datapoints.len() == 4096);
+
+            // ctx = RegionCtx::new(ctx.into_region(), 0);


It looks like this is some old comment?

Brechtpd · 2023-06-11T21:56:03Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+                    // TODO: check _div_result?
+                    integer_chip.add(&mut ctx, &assigned_evals[i], &summand)?
+                };
+                integer_chip.assert_equal(&mut ctx, &assigned_evals[i+1], &eval_next)?;


Do we need to do all these assigned_evals? Can't we just use eval_next in the next loop iteration instead of assigned_evals[i]?

Brechtpd · 2023-06-11T21:56:23Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+            // TODO: integer_chip operations (.mul, .sub) actually assigned NEW values to the circuit!!! how to fix? Because .assert_equal only seems to work between AssignedInteger<> values...
+            // ctx = RegionCtx::new(ctx.into_region(), 0);


Is this still a TODO?

Brechtpd · 2023-06-11T22:19:32Z

zkevm-circuits/src/blobkzg_circuit/src/main.rs

+        use halo2wrong_maingate::RangeInstructions;
+        integer_chip.range_chip().load_table(&mut layouter)?;
+
+        layouter.assign_region(|| "BlobKZGCircuit Region", | region| {


Some more comments here on what does what would also be nice.

AlekseiVambol · 2023-10-09T13:15:17Z

An important remark about the correct way of choosing the pseudorandom evaluation point x for the polynomial encoding the data. The PR is aimed at implementing protocol on the screenshot.

Source: https://notes.ethereum.org/@vbuterin/proto_danksharding_faq#Moderate-approach-works-with-any-ZK-SNARK

Note that the first 2 steps insist on choosing the value of x as the hash of both the polynomial outside the circuit and its evaluations inside the circuit. This means that (in the case of one-phase synthesis) the circuit must contain the constraints representing the hashing of the data polynomial evaluations. This inner hash value should be passed as a public input to the circuit to make the result of the in-circuit hashing accessible to the verifier. When a smart contract verifies the proof for the circuit, it will be given this inner hash and use it along with the hash of the polynomial outside the circuit to compute the evaluation point x, which is also a public input.

Why do we need this inner hashing? To force the prover to choose the value of x strictly after the data polynomial evaluations in the circuit are fixed. If prover is allowed to change these evaluations after x is chosen, he can interpolate the polynomial, which is not equal to the one outside the circuit, but coincide with it at x. He can do it with polynomial time and space complexity and is free to choose the values at any 4095 of 4096 evaluation points!

How could we simplify this approach? I think we can avoid writing the hash constraints, if we have a tricky proving system that allows two-phase synthesis, has the challenge API (returning the hash of the witnesses committed at the first phase) and allows us to generate some public input data between the phases of the synthesis. In this case the challenge value can be used instead of the aforesaid inner hash value - it can be used (along with the public input part representing the hash of the polynomial outside the circuit) to compute x between the synthesis phases. The value of x will be added to the public input before the second phase of synthesis. Unfortunately, it seems that Halo2 does not support this dynamic public input generation.

Brechtpd · 2023-10-11T11:52:52Z

@AlekseiVambol Could we reuse the KZG commitment for the column storing the data that could already be available for the verifier (?) instead of explicitly committing to the data in-circuit?

AlekseiVambol · 2023-10-16T14:16:25Z

Yes.

folder

e956357

github-actions bot added the crate-zkevm-circuits label Jun 5, 2023

Brechtpd requested review from xiaodino and AlekseiVambol June 5, 2023 11:58

xiaodino reviewed Jun 6, 2023

View reviewed changes

added root-of-unity and tests (not all pass!)

f4365a2

AlekseiVambol reviewed Jun 7, 2023

View reviewed changes

Brechtpd reviewed Jun 11, 2023

View reviewed changes

Merge branch 'taikoxyz:main' into main

d6bbd57

mabbamOG deleted the branch taikoxyz:main June 26, 2023 12:15

mabbamOG closed this Jun 26, 2023

mabbamOG deleted the main branch June 26, 2023 12:15

Brechtpd mentioned this pull request Jul 4, 2023

Eip 4844 #125

Open

4 tasks

Brechtpd mentioned this pull request Oct 24, 2023

feat(protocol): allow using both blob & calldata for DA taikoxyz/taiko-mono#15011

Merged

mratsim mentioned this pull request Mar 6, 2024

EIP-4844: KZG verification acceleration via proof of equivalence taikoxyz/raiko#47

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blobkzg Circuit for EIP 4844 #106

Blobkzg Circuit for EIP 4844 #106

mabbamOG commented Jun 5, 2023

xiaodino Jun 6, 2023

xiaodino Jun 6, 2023

mabbamOG Jun 7, 2023

xiaodino Jun 6, 2023

mabbamOG Jun 7, 2023

xiaodino Jun 6, 2023

mabbamOG Jun 7, 2023

AlekseiVambol Jun 7, 2023

Brechtpd left a comment

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

Brechtpd Jun 11, 2023

AlekseiVambol commented Oct 9, 2023 •

edited

Loading

Brechtpd commented Oct 11, 2023

AlekseiVambol commented Oct 16, 2023

		// TODO: integer_chip operations (.mul, .sub) actually assigned NEW values to the circuit!!! how to fix? Because .assert_equal only seems to work between AssignedInteger<> values...
		// ctx = RegionCtx::new(ctx.into_region(), 0);

Blobkzg Circuit for EIP 4844 #106

Blobkzg Circuit for EIP 4844 #106

Conversation

mabbamOG commented Jun 5, 2023

Description

Issue Link

Type of change

Contents

Rationale

How Has This Been Tested?

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

Brechtpd 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

Choose a reason for hiding this comment

AlekseiVambol commented Oct 9, 2023 • edited Loading

Brechtpd commented Oct 11, 2023

AlekseiVambol commented Oct 16, 2023

AlekseiVambol commented Oct 9, 2023 •

edited

Loading