Skip to content

Conversation

noaov1
Copy link
Collaborator

@noaov1 noaov1 commented Sep 14, 2025

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@noaov1 noaov1 force-pushed the noa/use_encrypt_method_for_da branch from e543d9c to ac81606 Compare September 14, 2025 08:26
@noaov1 noaov1 force-pushed the noa/add_encrypt_method branch from 7efa734 to d2ec44a Compare September 14, 2025 09:19
Copy link
Contributor

@einat-starkware einat-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


crates/starknet_os/src/hints/enum_definition.rs line 826 at r2 (raw file):

    ),
    (
        SetEncryptedStart,

Note that adding a new hint will break the unused hint test in python, so you will need to open a pyside PR as well

@noaov1 noaov1 force-pushed the noa/add_encrypt_method branch from d2ec44a to f73dd0e Compare September 14, 2025 15:22
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

@Yoni-Starkware reviewed 1 of 5 files at r1.
Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @einat-starkware and @noaov1)


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

    if (n_keys == 0) {
        return (da_start=compressed_start, da_end=compressed_dst);
    }

Suggestion:

    if (n_keys == 0) {
        // No public keys - skip the state updates encryption.
        return (da_start=compressed_start, da_end=compressed_dst);
    }

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

            # Assign a temporary segment, to be relocated into the output segment.
            ids.encrypted_start = segments.add_temp_segment()
    %}

Move this hint right above the use in encrypted_dst

Suggestion:

    local encrypted_start: felt*;
    %{
        if use_kzg_da:
            ids.encrypted_start = segments.add()
        else:
            # Assign a temporary segment, to be relocated into the output segment.
            ids.encrypted_start = segments.add_temp_segment()
    %}
    
    let encrypted_dst = encrypted_start;

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 5 files reviewed, 5 unresolved discussions (waiting on @einat-starkware and @noaov1)


crates/starknet_os/src/hints/hint_implementation/output.rs line 147 at r2 (raw file):

        x if x == Felt::ZERO => Ok(false),
        _ => Err(OsHintError::BooleanIdExpected { id: Ids::UseKzgDa, felt: use_kzg_da_felt }),
    }?;

Please share this code (felt_to_bool)

Code quote:

    let use_kzg_da = match use_kzg_da_felt {
        x if x == Felt::ONE => Ok(true),
        x if x == Felt::ZERO => Ok(false),
        _ => Err(OsHintError::BooleanIdExpected { id: Ids::UseKzgDa, felt: use_kzg_da_felt }),
    }?;

crates/starknet_os/src/hints/hint_implementation/output.rs line 149 at r2 (raw file):

    }?;

    if use_kzg_da || n_keys > Felt::ZERO {

Extract the insert_value_from_var_name outside the if-else. Also in the other hint

@noaov1 noaov1 force-pushed the noa/add_encrypt_method branch 2 times, most recently from 0c94207 to d1f6b58 Compare September 15, 2025 06:43
Copy link
Collaborator Author

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @einat-starkware and @Yoni-Starkware)


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

Previously, Yoni-Starkware (Yoni) wrote…

Move this hint right above the use in encrypted_dst

Done.


crates/starknet_os/src/hints/hint_implementation/output.rs line 147 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please share this code (felt_to_bool)

Done.

@noaov1 noaov1 force-pushed the noa/use_encrypt_method_for_da branch from ac81606 to 4369b9c Compare September 15, 2025 14:23
@noaov1 noaov1 changed the base branch from noa/add_encrypt_method to main-v0.14.1 September 15, 2025 14:23
Copy link

Artifacts upload workflows:

@noaov1 noaov1 force-pushed the noa/use_encrypt_method_for_da branch from 4369b9c to aca0107 Compare September 15, 2025 14:28
Copy link

Benchmark movements: No major performance changes detected.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Yoni-Starkware reviewed 1 of 1 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @einat-starkware and @noaov1)


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

Previously, noaov1 (Noa Oved) wrote…

Done.

Move this entire block above with encrypted_dst {... (non blocking)

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.

4 participants