feat: support reverts of inner calls

[ilyalesokhin-starkware]

This change is 

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/hint_processor.rs line 271 at r2 (raw file):

                .expect("Missing contract revert info.")
                .orig_values,
        );

This is not mandatory, but I felt it would be nicer to do it here than on every storage access.

Code quote:

        let orig_values = std::mem::take(
            &mut context
                .revert_infos
                .0
                .last_mut()
                .expect("Missing contract revert info.")
                .orig_values,
        );

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/transactions_test.rs line 1896 at r3 (raw file):

            ),
        ]),
        n_steps: get_tx_resources(TransactionType::L1Handler).n_steps + 164,

I have no idea why we save 7 steps here :(

Code quote:

 164,

[Yoni-Starkware]

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 80 at r3 (raw file):

        contract_address: ContractAddress,
        entry_point_selector: felt252,
        calldata: Array::<felt252>

Don't we have syntactic sugar for calling contracts in Cairo1?

Code quote:

        entry_point_selector: felt252,
        calldata: Array::<felt252>

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 94 at r3 (raw file):

            },
        };
        assert(self.my_storage_var.read() == 0, 'values should not change.');

Suggestion:

        };
        // TODO(Yoni, 1/12/2024): test replace class once get_class_hash_at syscall is supported.
        assert(self.my_storage_var.read() == 0, 'values should not change.');

---

crates/blockifier/src/execution/entry_point.rs line 50 at r3 (raw file):

    pub original_class_hash: ClassHash,
    /// The original storage values.
    pub orig_values: HashMap<StorageKey, Felt>,

Suggestion:

    pub original_class_hash: ClassHash,
    /// The original storage values.
    pub original_values: HashMap<StorageKey, Felt>,

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 271 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

This is not mandatory, but I felt it would be nicer to do it here than on every storage access.

Is it more efficient?

---

crates/blockifier/src/transaction/transactions_test.rs line 1896 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

I have no idea why we save 7 steps here :(

You compiled with 2.8.0 (the test contract was compiled with 2.7.0)

[Yoni-Starkware]

Very cool. Tiny issue is with concurrency - the set of writes might contain values that were not written by the tx.
Should be okay since there is a read before every write.
cc @noaov1 - is it safe to save the diff instead of the writes here and here?

Reviewable status: 0 of 8 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/hint_processor.rs line 271 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Is it more efficient?

yes

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/transactions.rs line 85 at r4 (raw file):

            Ok(value) => {
                if let Some(call_info) = &value.execute_call_info {
                    // If the execution of the outer call failed, revert the transction.

@dorimedini-starkware, does the following make sense?

Without it, the unwrap_err() here:

sequencer/crates/blockifier/src/transaction/transactions_test.rs

Line 1566 in 834eff6

let error = account_tx.execute(state, block_context, true, true).unwrap_err();

fails.

Code quote:

// If the execution of the outer call failed, revert the transction.

[ilyalesokhin-starkware]

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 80 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Don't we have syntactic sugar for calling contracts in Cairo1?

yes, but then you can't pass the selector in the calldata.

[ilyalesokhin-starkware]

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

---

crates/blockifier/src/transaction/transactions_test.rs line 1896 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You compiled with 2.8.0 (the test contract was compiled with 2.7.0)

fixed.

[ilyalesokhin-starkware]

"the set of writes might contain values that were not written by the tx."

can you elaborate? what writes?

[Yoni-Starkware]

Reviewable status: 0 of 9 files reviewed, 5 unresolved discussions (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 80 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

yes, but then you can't pass the selector in the calldata.

Why do you need it? why not calling test_revert_helper?

[ilyalesokhin-starkware]

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 80 at r3 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why do you need it? why not calling test_revert_helper?

I need to define a contract ABI for that. not sure it is worth the hustle,

we don't do it in other tests.

[Yoni-Starkware]

Reviewed 1 of 4 files at r4, 1 of 5 files at r5.
Reviewable status: 2 of 11 files reviewed, all discussions resolved

[dorimedini-starkware]

Reviewable status: 2 of 11 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/blockifier/src/transaction/transactions.rs line 85 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

@dorimedini-starkware, does the following make sense?

Without it, the unwrap_err() here:

sequencer/crates/blockifier/src/transaction/transactions_test.rs

Line 1566 in 834eff6

let error = account_tx.execute(state, block_context, true, true).unwrap_err();

fails.

when txs revert (with your new feature), we want to return an Err variant in the result of account_tx.execute, right @Yoni-Starkware ?
if this is the case, then yes, this seems to make sense to me

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/transactions.rs line 85 at r4 (raw file):

Previously, dorimedini-starkware wrote…

when txs revert (with your new feature), we want to return an Err variant in the result of account_tx.execute, right @Yoni-Starkware ?
if this is the case, then yes, this seems to make sense to me

Note that this is also temporary, in the future we would want to include prove reverted transactions and not use trusted reverts like we do today.

[Yoni-Starkware]

Reviewable status: 8 of 16 files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 855 at r10 (raw file):

    if failed {
        raw_retdata
            .push(Felt::from_hex(ENTRYPOINT_FAILED_ERROR).map_err(SyscallExecutionError::from)?);

Need to do this in the OS as well, right?

Code quote:

.push(Felt::from_hex(ENTRYPOINT_FAILED_ERROR)

[Yoni-Starkware]

Reviewable status: 8 of 16 files reviewed, 11 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/feature_contracts/cairo1/test_contract.cairo line 101 at r10 (raw file):

    #[external(v0)]
    fn test_revert_helper(ref self: ContractState) {
        self.my_storage_var.write(17);

Can you add message + event + replace_class/deploy here before the panic?

Code quote:

    #[external(v0)]
    fn test_revert_helper(ref self: ContractState) {
        self.my_storage_var.write(17);

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/hint_processor.rs line 190 at r10 (raw file):

}

/// Error codes returned by Cairo 1.0 code.

I think most of those are returned by the OS.

Code quote:

Error codes returned by Cairo 1.0 code.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/syscalls/hint_processor.rs line 855 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Need to do this in the OS as well, right?

yes

[ilyalesokhin-starkware]

crates/blockifier/src/transaction/account_transaction.rs line 461 at r7 (raw file):

Previously, ilyalesokhin-starkware wrote…

I'm not sure if this should be here or in some kind of wrapper and how to call the wrapper.

reverted

[Yoni-Starkware]

Reviewed 2 of 8 files at r11, all commit messages.
Reviewable status: 6 of 17 files reviewed, 12 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/call_info.rs line 206 at r11 (raw file):

            if call_info.execution.failed {
                continue;
            }

You have this, and this...
it's risky. I'll try to improve this in a preliminary PR

Code quote:

        for call_info in self.iter() {
            if call_info.execution.failed {
                continue;
            }

[Yoni-Starkware]

Reviewable status: 6 of 17 files reviewed, 12 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/call_info.rs line 206 at r11 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You have this, and this...
it's risky. I'll try to improve this in a preliminary PR

Oh, you cleared them. Great
So remove this skip - a reverted entrypoint should be considered here - we load its class and storage in the OS

[ilyalesokhin-starkware]

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

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 212 at r10 (raw file):

// "ENTRYPOINT_FAILED";
pub const ENTRYPOINT_FAILED_ERROR: &str =
    "0x000000000000000000000000000000454e545259504f494e545f4641494c4544";

Done.

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 851 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

We need to handle events and messages. Safer to remove them. Otherwise we need to change summarize here and python's equivalent

Done.

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 860 at r10 (raw file):

    let retdata_segment = create_retdata_segment(vm, syscall_handler, &raw_retdata)?;
    Ok(retdata_segment)

Done.

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 41 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Isn't it enough?

Done.

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 848 at r11 (raw file):

        // Delete events and l2_to_l1_messages from the reverted call.
        let execution = &mut syscall_handler.inner_calls.last_mut().unwrap().execution;

Oh basa. What about inner calls?
I'll ask @ArielElp

Code quote:

        // Delete events and l2_to_l1_messages from the reverted call.
        let execution = &mut syscall_handler.inner_calls.last_mut().unwrap().execution;

[ilyalesokhin-starkware]

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

---

crates/blockifier/src/execution/syscalls/hint_processor.rs line 848 at r11 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Oh basa. What about inner calls?
I'll ask @ArielElp

Done.

---

crates/blockifier/src/execution/call_info.rs line 206 at r11 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Oh, you cleared them. Great
So remove this skip - a reverted entrypoint should be considered here - we load its class and storage in the OS

Done.

[Yoni-Starkware]

Reviewed 2 of 13 files at r7, 3 of 8 files at r11, 6 of 6 files at r12, all commit messages.
Reviewable status: 16 of 17 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[Yoni-Starkware]

(Fix the OS with ENTRYPOINT_FAILED before merging)

Reviewable status: 16 of 17 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[Yoni-Starkware]

Reviewed 1 of 4 files at r4, 3 of 3 files at r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/entry_point_execution.rs line 108 at r13 (raw file):

    )?;

    if call_info.execution.failed && !context.versioned_constants().enable_reverts {

Revert new line

Suggestion:

    )?;
    if call_info.execution.failed && !context.versioned_constants().enable_reverts {

[Yoni-Starkware]

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

[Yoni-Starkware]

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

[Yoni-Starkware]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/syscalls/syscall_tests/call_contract.rs line 20 at r15 (raw file):

#[test]
fn test_call_contract_that_panics() {

@noaov1 TODO: test also with native once integrated

[Yoni-Starkware]

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

[ilyalesokhin-starkware]

Reviewed 1 of 4 files at r4, 2 of 13 files at r7, 1 of 10 files at r9, 2 of 8 files at r11, 2 of 6 files at r12, 1 of 3 files at r13, 7 of 9 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ilyalesokhin-starkware)