starknet_os: fix blake2s finalize opcode command in naive blake

[meship-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[meship-starkware]

• starknet_os: change the naive unpack to only handle data of len 1 #9508
• starknet_os: change all encoding to le in naive blake #9487
• starknet_os: fix blake2s finalize opcode command in naive blake #9477 👈 (View in Graphite)
• main-v0.14.1

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

[meship-starkware]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 25 at r1 (raw file):

    const MESSAGE_SHIFT = 2 ** 32;
    const FLAGS_SHIFT = 2 ** 48;
    const OPCODE_EXT_SHIFT = 2 ** 63;

Not sure if these are needed, we can just write the numbers in the final constant, just added them to be consistent with the type rs implementation

Code quote:

    const COUNTER_SHIFT = 1;
    const STATE_SHIFT = 2 ** 16;
    const MESSAGE_SHIFT = 2 ** 32;
    const FLAGS_SHIFT = 2 ** 48;
    const OPCODE_EXT_SHIFT = 2 ** 63;

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 29 at r1 (raw file):

    const POS_STATE_OFFSET = 2 ** 15 + STATE_OFFSET;
    const POS_MESSAGE_OFFSET = 2 ** 15 + MESSAGE_OFFSET;
    const POS_COUNTER_OFFSET = 2 ** 15 + COUNTER_OFFSET;

Added this content because the format fix did not handle the parentheses well in the BLAKE2S_FINALIZE_INSTRUCTION constant

Code quote:

    const POS_STATE_OFFSET = 2 ** 15 + STATE_OFFSET;
    const POS_MESSAGE_OFFSET = 2 ** 15 + MESSAGE_OFFSET;
    const POS_COUNTER_OFFSET = 2 ** 15 + COUNTER_OFFSET;

[liorgold2]

@liorgold2 reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 25 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Not sure if these are needed, we can just write the numbers in the final constant, just added them to be consistent with the type rs implementation

Good idea, let's remove these constants.

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 29 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Added this content because the format fix did not handle the parentheses well in the BLAKE2S_FINALIZE_INSTRUCTION constant

Ok.

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

    // - State and data are referenced from fp
    // - Counter is referenced from ap
    // - Increment ap by 1 after instruction

Suggestion:

    // Flags: 000100000001010.
    // - State and data are referenced from fp.
    // - Counter is referenced from ap.
    // - Increment ap by 1 after the instruction.

[liorgold2]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

    // - State and data are referenced from fp
    // - Counter is referenced from ap
    // - Increment ap by 1 after instruction

Actually, you can comment on the flags:

    const OP0_REG = 1;  // State is fp-based.
    const OP1_FP = 3;  // Data is fp-based.
    const AP_ADD1 = 11;  // Increment ap by 1 after the instruction.

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @liorgold2 and @Yoni-Starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

Previously, liorgold2 wrote…

Actually, you can comment on the flags:

    const OP0_REG = 1;  // State is fp-based.
    const OP1_FP = 3;  // Data is fp-based.
    const AP_ADD1 = 11;  // Increment ap by 1 after the instruction.

Done.

[liorgold2]

@liorgold2 reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @Yoni-Starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Done.

See second reply - move comments to the constants.

[Yoni-Starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

// Computes blake2s of `input` of size 16 felts, representing 32 bits each.
// The initial state is the standard BLAKE2s IV XORed with the parameter block P[0] = 0x01010020.
func blake_with_opcode_for_single_16_length_word(data: felt*, out: felt*, initial_state: felt*) {

@meship-starkware I wonder if adding noise to this function (+-1 to one of the consts and removing the static asserts) makes some tests fail. In particular, some consistency test with Rust.

[meship-starkware]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

Previously, liorgold2 wrote…

See second reply - move comments to the constants.

Done.

[meship-starkware]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

@meship-starkware I wonder if adding noise to this function (+-1 to one of the consts and removing the static asserts) makes some tests fail. In particular, some consistency test with Rust.

Changing one of the constants would fail the test that checks the encryption, as the Blake opcode will return something else. Removing the static asserts won't fail anything, but can we make a test that will fail if someone removes the static assert? If someone removes the static assert, it will change the order of the function arguments; the Blake opcode would fail as well, it would just be a bit harder to understand the failure reason

[meship-starkware]

@ArniStarkware This Solve the merge conflict with main

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

[Yoni-Starkware]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @meship-starkware)

---

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Changing one of the constants would fail the test that checks the encryption, as the Blake opcode will return something else. Removing the static asserts won't fail anything, but can we make a test that will fail if someone removes the static assert? If someone removes the static assert, it will change the order of the function arguments; the Blake opcode would fail as well, it would just be a bit harder to understand the failure reason

Static asserts are sanity check. I told you to remove them because I didn't want them to catch your noise.
What about AP_ADD1?