Channel

[spapinistarkware]

This change is 

[spapinistarkware]

• Channel #5 👈
• QM31 #6
• MerkleVerifier #3 : 1 other dependent PR (#4 )
• main

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

Join @spapinistarkware and the rest of your teammates on Graphite

[shaharsamocha7]

Reviewed 2 of 6 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @spapinistarkware)

---

stwo_cairo_verifier/src/channel.cairo line 40 at r2 (raw file):

pub impl ChannelImpl of ChannelTrait {
    fn new(digest: felt252) -> Channel {
        Channel { digest: 0, channel_time: Default::default(), }

Suggestion:

        Channel { digest: digest, channel_time: Default::default(), }

---

stwo_cairo_verifier/src/channel.cairo line 54 at r2 (raw file):

    #[inline]
    fn draw_base_felts(ref self: Channel) -> [BaseField; FELTS_PER_HASH] {

add comment/TODO to check that this is sound.

Code quote:

    #[inline]
    fn draw_base_felts(ref self: Channel) -> [BaseField; FELTS_PER_HASH] {

---

stwo_cairo_verifier/src/channel.cairo line 88 at r2 (raw file):

                                * M31_IN_HASH_SHIFT
                                + x3.into()
                        );

Consider to extract to a function.
It is scary because it looks easy to have a bug with this calculation

Code quote:

                            ((x0.into() * M31_IN_HASH_SHIFT + x1.into()) * M31_IN_HASH_SHIFT
                                + x2.into())
                                * M31_IN_HASH_SHIFT
                                + x3.into()
                        );

---

stwo_cairo_verifier/src/channel.cairo line 135 at r2 (raw file):

                break;
            }
            let [a, b, c, d, e, f, g, h] = self.draw_base_felts();

I'm not familiar with those letters :)

Suggestion:

let [r0, r1, r2, r3, r4, r5, r6, r7] = self.draw_base_felts();

---

stwo_cairo_verifier/src/lib.cairo line 6 at r2 (raw file):

mod vcs;

pub use fields::{BaseField, SecureField};

Why is it here?
maybe you already told me that

Code quote:

pub use fields::{BaseField, SecureField};

[schouhy]

Hi! While working on the FRI verifier we had to make these changes to make the channel compatible with the rust implementation.
Let me know what you think!

[spapinistarkware]

looks good, thanks.

Reviewable status: 4 of 6 files reviewed, 9 unresolved discussions (waiting on @schouhy and @shaharsamocha7)

---

stwo_cairo_verifier/src/channel.cairo line 2 at r2 (raw file):

Previously, schouhy (Sergio Chouhy) wrote…

use core::poseidon::{poseidon_hash_span, hades_permutation};

Done.

---

stwo_cairo_verifier/src/channel.cairo line 40 at r2 (raw file):

pub impl ChannelImpl of ChannelTrait {
    fn new(digest: felt252) -> Channel {
        Channel { digest: 0, channel_time: Default::default(), }

Done.

---

stwo_cairo_verifier/src/channel.cairo line 51 at r2 (raw file):

Previously, schouhy (Sergio Chouhy) wrote…

    fn draw_felt252(ref self: Channel) -> felt252 {
        let (s0, _, _) = hades_permutation(self.digest, self.channel_time.n_sent.into(), 2);
        self.channel_time.inc_sent();
        s0
    }

Done.

---

stwo_cairo_verifier/src/channel.cairo line 54 at r2 (raw file):

Previously, shaharsamocha7 wrote…

add comment/TODO to check that this is sound.

Done.

---

stwo_cairo_verifier/src/channel.cairo line 71 at r2 (raw file):

Previously, schouhy (Sergio Chouhy) wrote…

    fn mix_digest(ref self: Channel, digest: felt252) {
        let (s0, _, _) = hades_permutation(self.digest, digest, 2);
        self.digest = s0;
        self.channel_time.inc_challenges();
    }

Done.

---

stwo_cairo_verifier/src/channel.cairo line 74 at r2 (raw file):

Previously, schouhy (Sergio Chouhy) wrote…

        let mut res = array![self.digest];

Done.

---

stwo_cairo_verifier/src/channel.cairo line 88 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Consider to extract to a function.
It is scary because it looks easy to have a bug with this calculation

It just looks scary because of the formatting here IMO, and usage of into.
It's just ((x0 * 2**31 + x1) * 2**31 + x2) * 2**31 + x3

---

stwo_cairo_verifier/src/channel.cairo line 135 at r2 (raw file):

Previously, shaharsamocha7 wrote…

I'm not familiar with those letters :)

Done.

---

stwo_cairo_verifier/src/lib.cairo line 6 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Why is it here?
maybe you already told me that

So it would be easier to import them?
Also, seems ok to export them from the lib.

[shaharsamocha7]

Reviewed 1 of 6 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @schouhy and @spapinistarkware)

---

stwo_cairo_verifier/src/channel.cairo line 88 at r2 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

It just looks scary because of the formatting here IMO, and usage of into.
It's just ((x0 * 2**31 + x1) * 2**31 + x2) * 2**31 + x3

but still you use this calculation multiple times.
Wouldn't it be better as an inline function?

---

stwo_cairo_verifier/src/channel.cairo line 124 at r3 (raw file):

    fn draw_felt(ref self: Channel) -> SecureField {
        let [a, b, c, d, _, _, _, _] = self.draw_base_felts();

r0, .., r3

Code quote:

a, b, c, d

---

stwo_cairo_verifier/src/channel.cairo line 222 at r3 (raw file):

        // Assert that all the random felts are unique.

remove extra line

---

stwo_cairo_verifier/src/channel.cairo line 223 at r3 (raw file):

        // Assert that all the random felts are unique.

        assert_ne!(random_felts[0], random_felts[5]);

Consider to create a set(random_felts) and check the lengths are equal

Code quote:

assert_ne!(random_felts[0], random_felts[5]);

---

stwo_cairo_verifier/src/channel.cairo line 238 at r3 (raw file):

        // Reseed channel and check the digest was changed.
        channel.mix_digest(0);

move two lines up

Code quote:

        channel.mix_digest(0);

[spapinistarkware]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @schouhy and @shaharsamocha7)

---

stwo_cairo_verifier/src/channel.cairo line 88 at r2 (raw file):

Previously, shaharsamocha7 wrote…

but still you use this calculation multiple times.
Wouldn't it be better as an inline function?

Done.

---

stwo_cairo_verifier/src/channel.cairo line 124 at r3 (raw file):

Previously, shaharsamocha7 wrote…

r0, .., r3

Done.

---

stwo_cairo_verifier/src/channel.cairo line 222 at r3 (raw file):

Previously, shaharsamocha7 wrote…

remove extra line

Done.

---

stwo_cairo_verifier/src/channel.cairo line 223 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Consider to create a set(random_felts) and check the lengths are equal

Kind hard. No set in cairo afaik.

---

stwo_cairo_verifier/src/channel.cairo line 238 at r3 (raw file):

Previously, shaharsamocha7 wrote…

move two lines up

Done.

[shaharsamocha7]

Reviewed all commit messages.
Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @schouhy and @spapinistarkware)

---

stwo_cairo_verifier/src/utils.cairo line 76 at r4 (raw file):

const M31_SHIFT: felt252 = 0x80000000; // 2**31.
pub fn pack4(cur: felt252, values: [BaseField; 4]) -> felt252 {

Consider changing to pack8
(or document cur :)

Code quote:

pub fn pack4(cur: felt252, values: [BaseField; 4]) -> felt252 {

[spapinistarkware]

Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @schouhy and @shaharsamocha7)

---

stwo_cairo_verifier/src/utils.cairo line 76 at r4 (raw file):

Previously, shaharsamocha7 wrote…

Consider changing to pack8
(or document cur :)

Too messy

[shaharsamocha7]

Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @schouhy and @spapinistarkware)

---

stwo_cairo_verifier/src/utils.cairo line 76 at r4 (raw file):

Previously, spapinistarkware (Shahar Papini) wrote…

Too messy

also about cur?
This function name is confusing because of it

[spapinistarkware]

Reviewable status: 5 of 7 files reviewed, 5 unresolved discussions (waiting on @schouhy and @shaharsamocha7)

---

stwo_cairo_verifier/src/utils.cairo line 76 at r4 (raw file):

Previously, shaharsamocha7 wrote…

also about cur?
This function name is confusing because of it

Done.

[shaharsamocha7]

Reviewed 1 of 1 files at r5.
Reviewable status: 6 of 7 files reviewed, 4 unresolved discussions (waiting on @schouhy)