feat: support holes in range_check96 segment

[ilyalesokhin-starkware]

This change is 

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 110 at r1 (raw file):

        nonce: nonce_manager.next(account_address)
    };
    let tx_execution_info = run_invoke_tx(

it seems that this is passing without the fix :(

Code quote:

run_invoke_tx(

[dorimedini-starkware]

Reviewed 2 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 558 at r1 (raw file):

    // Add drop for AddInputResult as it only has PanicDesturct.

typo

Code quote:

Desturct

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 565 at r1 (raw file):

        let in1 = CircuitElement::<CircuitInput<0>> {};
        (in1,).new_inputs();
    }

is this builtin available for cairo0?
if so please add a cairo0 implementation
(non blocking in case this builtin is unavailable)

Code quote:

    // Add drop for AddInputResult as it only has PanicDesturct.
    impl AddInputResultDrop<C> of Drop<core::circuit::AddInputResult<C>>;

    #[external(v0)]
    fn test_missing_rc96_values(ref self: ContractState) {
        let in1 = CircuitElement::<CircuitInput<0>> {};
        (in1,).new_inputs();
    }

---

crates/blockifier/src/execution/entry_point_execution.rs line 286 at r1 (raw file):

    program_segment_size: usize,
) -> EntryPointExecutionResult<()> {
    let verify_secure = false;

what's this...? anyone who now calls run_entry_point should not verify security? how is this related to the memory holes issue?

Code quote:

let verify_secure = false;

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 286 at r1 (raw file):

Previously, dorimedini-starkware wrote…

what's this...? anyone who now calls run_entry_point should not verify security? how is this related to the memory holes issue?

I call it manually.
I need to fill the holes before calling it.

[dorimedini-starkware]

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 286 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

I call it manually.
I need to fill the holes before calling it.

is calling it manually after the run_from_entrypoint call equivalent to previous behavior...? even with recursive syscalls (call contract / libcall)?

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 286 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is calling it manually after the run_from_entrypoint call equivalent to previous behavior...? even with recursive syscalls (call contract / libcall)?

yes, look at the implementation of run_from_entrypoint
https://github.com/lambdaclass/cairo-vm/blob/9ef2ab88d89a9b879553b55e045db1f4e148ac26/vm/src/cairo_run.rs#L178

[ilyalesokhin-starkware]

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 565 at r1 (raw file):

Previously, dorimedini-starkware wrote…

is this builtin available for cairo0?
if so please add a cairo0 implementation
(non blocking in case this builtin is unavailable)

you can't declare new cairo0 contracts.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 286 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

yes, look at the implementation of run_from_entrypoint
https://github.com/lambdaclass/cairo-vm/blob/9ef2ab88d89a9b879553b55e045db1f4e148ac26/vm/src/cairo_run.rs#L178

For some reason, it does behave in a different manner I have to call compute_effective_sizes

[ilyalesokhin-starkware]

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 558 at r1 (raw file):

Previously, dorimedini-starkware wrote…

typo

done

[Yoni-Starkware]

Reviewable status: 0 of 4 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 301 at r2 (raw file):

    // Fill holes in the rc96 segment.
    if let Some((rc_96_idx, rc96_builtin_runner)) =

either rc96 or rc_96 throughout the code

Code quote:

(rc_96_idx

---

crates/blockifier/src/execution/entry_point_execution.rs line 313 at r2 (raw file):

    {
        // 'EntryPointReturnValues' is returned after the implicits and its size is 5.
        let implicits_offsets = 5;

Suggestion:

let implicits_offset = 5;

---

crates/blockifier/src/execution/entry_point_execution.rs line 318 at r2 (raw file):

            .map_err(|err| CairoRunError::VirtualMachine(VirtualMachineError::Math(err)))?;

        let base = rc96_builtin_runner.base() as isize;

Can you use try_into?

Code quote:

base() as isize;

---

crates/blockifier/src/execution/entry_point_execution.rs line 322 at r2 (raw file):

        let Relocatable { segment_index, offset: stop_offset } =
            runner.vm.get_relocatable(rc_96_stop_ptr).map_err(CairoRunError::MemoryError)?;
        assert_eq!(segment_index, base);

Suggestion:

        let rc96_base = rc96_builtin_runner.base() as isize;

        let Relocatable { segment_index, offset: stop_offset } =
            runner.vm.get_relocatable(rc_96_stop_ptr).map_err(CairoRunError::MemoryError)?;
        assert_eq!(segment_index, rc96_base, "Unexpected segment index of range_check96.");

---

crates/blockifier/src/transaction/account_transactions_test.rs line 110 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

it seems that this is passing without the fix :(

Still?

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 110 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Still?

No, the problem was that holes at the end didn't trigger a failure.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 330 at r2 (raw file):

        }
    }
    runner.vm.segments.compute_effective_sizes();

Any idea why this is needed?

There is no such call inside run_from_entrypoint

Code quote:

  runner.vm.segments.compute_effective_sizes();

[liorgold2]

Reviewable status: 0 of 4 files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware, @ilyalesokhin-starkware, and @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 302 at r3 (raw file):

    // Fill holes in the rc96 segment.
    if let Some((rc96_offset, rc96_builtin_runner)) =
        runner.vm.get_builtin_runners().iter().find_map(|builtin| {

For code simplicity, I suggest putting the result of this find_map call in a variable.

---

crates/blockifier/src/execution/entry_point_execution.rs line 303 at r3 (raw file):

    if let Some((rc96_offset, rc96_builtin_runner)) =
        runner.vm.get_builtin_runners().iter().find_map(|builtin| {
            let Some(rc96_offset) = opt_rc96_idx else { return None };

Isn't this line identical to rc96_offset? ?

[ilyalesokhin-starkware]

Reviewable status: 0 of 4 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware, @liorgold2, and @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 301 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

either rc96 or rc_96 throughout the code

Done.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 303 at r3 (raw file):

Previously, liorgold2 wrote…

Isn't this line identical to rc96_offset? ?

Done.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 330 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

Any idea why this is needed?

There is no such call inside run_from_entrypoint

Ok, the issue was early return

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 286 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

For some reason, it does behave in a different manner I have to call compute_effective_sizes

The issue was early return.

[ilyalesokhin-starkware]

Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware, @liorgold2, and @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 302 at r3 (raw file):

Previously, liorgold2 wrote…

For code simplicity, I suggest putting the result of this find_map call in a variable.

Done.

[orizi]

Reviewed 4 of 4 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware, @ilyalesokhin-starkware, @liorgold2, and @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 315 at r4 (raw file):

        let implicits_offset = 6;
        let rc_96_stop_ptr = (runner.vm.get_ap() - (implicits_offset + rc96_offset))
            .map_err(|err| CairoRunError::VirtualMachine(VirtualMachineError::Math(err)))?;

Suggestion:

        const IMPLICITS_OFFSET: usize = 6;
        let rc_96_stop_ptr = (runner.vm.get_ap() - (IMPLICITS_OFFSET + rc96_offset))
            .map_err(|err| CairoRunError::VirtualMachine(VirtualMachineError::Math(err)))?;

[Yoni-Starkware]

Reviewed 2 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 338 at r5 (raw file):

                .vm
                .insert_value(Relocatable { segment_index: rc96_segment, offset }, Felt::zero());
        }

Please match the result, and ignore only MemoryError::InconsistentMemory
(since this function raises more errors)

Alternatively, check with get before the insertion

Code quote:

            // If the value is already set, ignore the error.
            let _ = runner
                .vm
                .insert_value(Relocatable { segment_index: rc96_segment, offset }, Felt::zero());
        }

[Yoni-Starkware]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 331 at r5 (raw file):

            .as_mut()
            .expect("Segments used sizes should be calculated at this point")[rc96_base] =
            stop_offset;

Please add a comment about this

Code quote:

        runner
            .vm
            .segments
            .segment_used_sizes
            .as_mut()
            .expect("Segments used sizes should be calculated at this point")[rc96_base] =
            stop_offset;

[Yoni-Starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 121 at r5 (raw file):

    .unwrap();

    assert_eq!(tx_execution_info.revert_error, None);

Suggestion:

assert!(!tx_execution_info.is_reverted());

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 121 at r5 (raw file):

    .unwrap();

    assert_eq!(tx_execution_info.revert_error, None);

assert_eq! prints the failure in case of error.

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/account_transactions_test.rs line 121 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

assert_eq! prints the failure in case of error.

but I guess we can fix all tests in a future PR.

[ilyalesokhin-starkware]

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @orizi, and @Yoni-Starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 315 at r4 (raw file):

        let implicits_offset = 6;
        let rc_96_stop_ptr = (runner.vm.get_ap() - (implicits_offset + rc96_offset))
            .map_err(|err| CairoRunError::VirtualMachine(VirtualMachineError::Math(err)))?;

Done.

---

crates/blockifier/src/execution/entry_point_execution.rs line 331 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please add a comment about this

done

---

crates/blockifier/src/execution/entry_point_execution.rs line 338 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Please match the result, and ignore only MemoryError::InconsistentMemory
(since this function raises more errors)

Alternatively, check with get before the insertion

Done.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 338 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

Done.

What happens if I get a different error?

[orizi]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

[Yoni-Starkware]

Reviewed 2 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 338 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

What happens if I get a different error?

Panic I guess. Is there a recoverable error?

[Yoni-Starkware]

Reviewed 1 of 3 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/stack_trace_test.rs line 262 at r6 (raw file):

#[case(CairoVersion::Cairo0, "fail", "An ASSERT_EQ instruction failed: 1 != 0.", 0_u8, 0_u8, (37_u16, 1093_u16, 1184_u16, 1188_u16))]
#[case(CairoVersion::Cairo0, "fail", "An ASSERT_EQ instruction failed: 1 != 0.", 0_u8, 1_u8, (49_u16, 1111_u16, 1184_u16, 1188_u16))]
#[case(CairoVersion::Cairo1, "invoke_call_chain", "0x4469766973696f6e2062792030 ('Division by 0')", 1_u8, 0_u8, (9602_u16, 9602_u16, 0_u16, 0_u16))]

@dorimedini-starkware does this test always fail when changing test_contract.cairo?

[ilyalesokhin-starkware]

crates/blockifier/src/execution/entry_point_execution.rs line 338 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Panic I guess. Is there a recoverable error?

I don't know how to recover from that, that's why I ignore it in the previous solution.

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/entry_point_execution.rs line 338 at r5 (raw file):

Previously, ilyalesokhin-starkware wrote…

I don't know how to recover from that, that's why I ignore it in the previous solution.

So panic is good

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/stack_trace_test.rs line 262 at r6 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

@dorimedini-starkware does this test always fail when changing test_contract.cairo?

if PC locations of previous functions change, then yes

[ilyalesokhin-starkware]

Reviewed 2 of 4 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[dorimedini-starkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[Yoni-Starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/stack_trace_test.rs line 262 at r6 (raw file):

Previously, dorimedini-starkware wrote…

if PC locations of previous functions change, then yes

Can we use a constant cairo contract for this, that won't be changed and recompiled?
(i.e., can you add this to Monday?)

[Yoni-Starkware]

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

[ilyalesokhin-starkware]

Reviewed 1 of 4 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)

not available

[Yoni-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 564 at r7 (raw file):

    fn test_rc96_holes(ref self: ContractState) {
        test_rc96_holes_helper();
        test_rc96_holes_helper();

Add a call with eval here

Suggestion:

    fn test_rc96_holes(ref self: ContractState) {
        test_rc96_holes_helper();
        // here
        test_rc96_holes_helper();