feat(blockifier): add address and selector for intermediate cairo1 revert errors

[dorimedini-starkware]

No description provided.

[lotem-starkware]

This change is 

[dorimedini-starkware]

• test(blockifier): test negative flows of cairo1 revert trace #1469
• feat(blockifier): limit characters in cairo1 revert trace #1468
• feat(blockifier): structured cairo1 stack trace #1461
• test(blockifier): add class hash (if set) to cairo1 revert trace #1460
• feat(blockifier): add address and selector for intermediate cairo1 revert errors #1459 👈
• test(blockifier): named values in test #1458
• refactor(blockifier): format retdata in ExecutionFailed and PanicInValidate errors #1457
• main

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

Join @dorimedini-starkware and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 93.47826% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.23%. Comparing base (b0cfe82) to head (68e71d9).
Report is 436 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/execution/stack_trace.rs	91.17%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1459      +/-   ##
==========================================
- Coverage   74.18%   70.23%   -3.96%     
==========================================
  Files         359       95     -264     
  Lines       36240    13076   -23164     
  Branches    36240    13076   -23164     
==========================================
- Hits        26886     9184   -17702     
+ Misses       7220     3490    -3730     
+ Partials     2134      402    -1732     

Flag	Coverage Δ
	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Yoni-Starkware]

Reviewed 2 of 6 files at r1.
Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

crates/blockifier/src/execution/stack_trace.rs line 157 at r1 (raw file):

}

pub fn extract_trailing_cairo1_revert_trace(root_callinfo: &CallInfo) -> String {

call/call_info

Suggestion:

(root_call: &CallInfo)

---

crates/blockifier/src/execution/stack_trace.rs line 159 at r1 (raw file):

pub fn extract_trailing_cairo1_revert_trace(root_callinfo: &CallInfo) -> String {
    root_callinfo
        .tail_iter()

Document why is it enough to iterate the tail

Code quote:

    root_callinfo
        .tail_iter()

---

crates/blockifier/src/execution/stack_trace.rs line 160 at r1 (raw file):

    root_callinfo
        .tail_iter()
        .map(|callinfo| {

call/call_info

Code quote:

|callinfo

---

crates/blockifier/src/execution/stack_trace.rs line 168 at r1 (raw file):

            )
        })
        .join("\n\n")

You should stop iterating when you see call_info.failed == 0.
Also, you should pop out ENTRYPOINT_FAILED in every iteration.
If anything goes wrong (not enough ENTRYPOINT_FAILED, too much ENTRYPOINT_FAILED)
fallback the the flat retdata string.

Please add a simple test to extract_trailing_cairo1_revert_trace

Code quote:

        .tail_iter()
        .map(|callinfo| {
            format!(
                "Error in contract (contract address: {:#064x}, selector: {:#064x}):\n{}",
                callinfo.call.storage_address.0.key(),
                callinfo.call.entry_point_selector.0,
                format_panic_data(&callinfo.execution.retdata.0)
            )
        })
        .join("\n\n")

---

crates/blockifier/src/execution/stack_trace_test.rs line 229 at r1 (raw file):

Error in contract (contract address: {test_contract_address_2_felt:#064x}, selector: \
         {inner_entry_point_selector_felt:#064x}):
0x6661696c ('fail').

I think that this is the desired output

Suggestion:

Error in contract (contract address: {account_address_felt:#064x}, selector: \
         {execute_selector_felt:#064x}):

Error in contract (contract address: {test_contract_address_felt:#064x}, selector: \
         {external_entry_point_selector_felt:#064x}):
         
Error in contract (contract address: {test_contract_address_2_felt:#064x}, selector: \
         {inner_entry_point_selector_felt:#064x}):
0x6661696c ('fail').

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 159 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Document why is it enough to iterate the tail

Why aren't you looking at the ret_data?

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 159 at r1 (raw file):

Previously, ilyalesokhin-starkware wrote…

Why aren't you looking at the ret_data?

You're right, see my comment below.
I meant here - why is it enough to look at the rightmost call branch (given that the failure flag is on and the retdata is legit)

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 159 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You're right, see my comment below.
I meant here - why is it enough to look at the rightmost call branch (given that the failure flag is on and the retdata is legit)

better?

---

crates/blockifier/src/execution/stack_trace.rs line 168 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

You should stop iterating when you see call_info.failed == 0.
Also, you should pop out ENTRYPOINT_FAILED in every iteration.
If anything goes wrong (not enough ENTRYPOINT_FAILED, too much ENTRYPOINT_FAILED)
fallback the the flat retdata string.

Please add a simple test to extract_trailing_cairo1_revert_trace

Done.
Added negative tests only (top of stack), positive cases are covered by the other stack trace tests.
If you have a particular edge case in mind that I haven;t thought of PLMK

---

crates/blockifier/src/execution/stack_trace_test.rs line 229 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

I think that this is the desired output

Done.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 164 at r2 (raw file):

    // a source or error.
    let mut error_calls: Vec<&CallInfo> = vec![];
    for call_info in root_call.tail_iter() {

shouldn't tail_iter be in this pr?

Code quote:

tail_iter()

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 169 at r2 (raw file):

        if !call_info.execution.failed {
            break;
        }

if you have
a -> b -> c -> d
and c fails after succesfully calling d.

what is error_calls going to contain after the loop?

Code quote:

        if !call_info.execution.failed {
            break;
        }

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 185 at r2 (raw file):

        let retdata = &error_calls[depth].execution.retdata.0;
        let expected_retdata_len = error_calls.len() - depth;
        if retdata.len() != expected_retdata_len || retdata[1..] != entrypoint_failed_tail[depth..]

what are you skipping here?

Code quote:

retdata[1..]

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 164 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

shouldn't tail_iter be in this pr?

it is, it's in call_info.rs, above

---

crates/blockifier/src/execution/stack_trace.rs line 169 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

if you have
a -> b -> c -> d
and c fails after succesfully calling d.

what is error_calls going to contain after the loop?

this function will be called with c as the root_call.
c is failed, so c is added to the error_calls,
and d is not failed, so in the end error_calls is [c]

---

crates/blockifier/src/execution/stack_trace.rs line 185 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

what are you skipping here?

the failure reason.
if a->b->c fails at c with error x
then

1. retdata of c is [x]
2. retdata of b is [x, ENTRYPOINT_FAILED]
3. retdata of a is [x, ENTRYPOINT_FAILED, ENTRYPOINT_FAILED]

[ilyalesokhin-starkware]

crates/blockifier/src/execution/call_info.rs line 194 at r2 (raw file):

pub struct CallInfoTailIter<'a> {
    call_infos: Vec<&'a CallInfo>,

do you need to save more than one call_info for this logic?

Code quote:

Vec<&'a CallInfo>,

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 169 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this function will be called with c as the root_call.
c is failed, so c is added to the error_calls,
and d is not failed, so in the end error_calls is [c]

why is it called from c and not from a?

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 185 at r2 (raw file):

Previously, dorimedini-starkware wrote…

the failure reason.
if a->b->c fails at c with error x
then

1. retdata of c is [x]
2. retdata of b is [x, ENTRYPOINT_FAILED]
3. retdata of a is [x, ENTRYPOINT_FAILED, ENTRYPOINT_FAILED]

but x can be more than 1 felt.

[ilyalesokhin-starkware]

Reviewed 2 of 6 files at r1.
Reviewable status: 2 of 6 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 171 at r2 (raw file):

        }
        error_calls.push(call_info);
    }

Suggestion:

     
    loop {
        if !call_info.execution.failed || call_info.execution.retdata.0.last() != Some(ENTRYPOINT_FAILED_ERROR) {
            break;
        }
        error_calls.push(call_info);
        if let Some(last_call) = error_calls.last.unwrap().inner_calls {
           call_info = last_call;
        } else {
           break;
        }
    }

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 169 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

why is it called from c and not from a?

I'm assuming a, b are cairo0.
if the entire chain is cairo1 then the root will be a and error_calls will be [a, b, c]

---

crates/blockifier/src/execution/stack_trace.rs line 171 at r2 (raw file):

        }
        error_calls.push(call_info);
    }

Done, kinda.
last call is a special case: we don't expect to see the EF constant in the innermost retdata

---

crates/blockifier/src/execution/stack_trace.rs line 185 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

but x can be more than 1 felt.

Done.

---

crates/blockifier/src/execution/call_info.rs line 194 at r2 (raw file):

Previously, ilyalesokhin-starkware wrote…

do you need to save more than one call_info for this logic?

right, nope. Done.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 182 at r3 (raw file):

        if let Some(ref inner_call) = next_call {
            if inner_call.execution.failed && !last_felt_is_entrypoint_error {
                return fallback_value;

I think we do want to print the stack trace in this case.

Suggestion:

break

[ilyalesokhin-starkware]

Reviewed 1 of 6 files at r1, 2 of 3 files at r2, 1 of 2 files at r3.
Reviewable status: 5 of 6 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 182 at r3 (raw file):

Previously, ilyalesokhin-starkware wrote…

I think we do want to print the stack trace in this case.

Done.

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 182 at r3 (raw file):

Previously, dorimedini-starkware wrote…

Done.

... and updated tests

[Yoni-Starkware]

Reviewed 1 of 6 files at r1, 2 of 3 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @ilyalesokhin-starkware)