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

[dorimedini-starkware]

No description provided.

[lotem-starkware]

This change is 

[dorimedini-starkware]

• feat(blockifier): keep revert error structure in TransactionExecutionInfo #1491
• feat(blockifier): cairo1 revert stack now part of ErrorStack #1490
• 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 👈 (View in Graphite)
• 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.

[codecov]

Codecov Report

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

Project coverage is 67.23%. Comparing base (b0cfe82) to head (251c7e1).
Report is 487 commits behind head on main.

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

❗ There is a different number of reports uploaded between BASE (b0cfe82) and HEAD (251c7e1). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (b0cfe82)	HEAD (251c7e1)
	3	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1459      +/-   ##
==========================================
- Coverage   74.18%   67.23%   -6.96%     
==========================================
  Files         359      102     -257     
  Lines       36240    13680   -22560     
  Branches    36240    13680   -22560     
==========================================
- Hits        26886     9198   -17688     
+ Misses       7220     4081    -3139     
+ Partials     2134      401    -1733     

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.

[Yoni-Starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 176 at r4 (raw file):

Previously, ilyalesokhin-starkware wrote…

Maybe we should do this according to the retdata of the outermost call?

Hmmm, I think it should be equivalent. Do you have a (natural) scenario where the result is different?

[Yoni-Starkware]

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

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 176 at r4 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Hmmm, I think it should be equivalent. Do you have a (natural) scenario where the result is different?

Code snippet:

let res = call_contract(...)
let _ =  call_contract(...)
res.unwrap_syscall(...)

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 202 at r6 (raw file):

            // Inner failure is either not unique, or does not exist (malformed retdata).
            _ => return fallback_value,
        };

Suggestion:

        let expected_inner_retdata = retdata[..retdata.len() - 1];
        let mut potential_inner_failures_iter: Vec<&CallInfo> = call
            .inner_calls
            .iter()
            .filter(|inner_call| {
                inner_call.execution.failed
                    && inner_call.execution.retdata.0 == expected_inner_retdata
            })
            .collect();
        call = potential_inner_failures_iter.next() {
            Some(unique_inner_failure) if potential_inner_failures_iter.next().is_none()  => unique_inner_failure,
            // Inner failure is either not unique, or does not exist (malformed retdata).
            _ => return fallback_value,
        };

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 176 at r4 (raw file):

        // Even if the next inner call is also in failed state, assume a scenario where the current
        // call panicked after ignoring the error result of the inner call.
        if call.execution.retdata.0.last() != Some(&entrypoint_failed_felt) {

Done.

---

crates/blockifier/src/execution/stack_trace.rs line 202 at r6 (raw file):

            // Inner failure is either not unique, or does not exist (malformed retdata).
            _ => return fallback_value,
        };

Done.

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

    let mut error_calls: Vec<&CallInfo> = vec![];
    let mut call = root_call;
    let mut expected_retdata = call.execution.retdata.0.clone();

is this clone nessiary?

Code quote:

let mut expected_retdata = call.execution.retdata.0.clone();

[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 166 at r7 (raw file):

Previously, ilyalesokhin-starkware wrote…

is this clone nessiary?

I don't want to mutate the original call's retdata, I need a new copy
If I drop the clone won't calling pop mutate call.execution.retdata?

[ilyalesokhin-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

Previously, dorimedini-starkware wrote…

I don't want to mutate the original call's retdata, I need a new copy
If I drop the clone won't calling pop mutate call.execution.retdata?

you can use last instead of pop

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

Previously, ilyalesokhin-starkware wrote…

you can use last instead of pop

like in r6

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

Previously, ilyalesokhin-starkware wrote…

like in r6

in r6 I had an addition expected_inner_retdata variable initialized once per iteration, with retdata[..X].as_vec(). Isn't this also a copy operation?
I think this way is better - I maintain the "current" expected retdata by popping once per loop, and only cloning data once before the loop starts

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

Previously, dorimedini-starkware wrote…

in r6 I had an addition expected_inner_retdata variable initialized once per iteration, with retdata[..X].as_vec(). Isn't this also a copy operation?
I think this way is better - I maintain the "current" expected retdata by popping once per loop, and only cloning data once before the loop starts

Yes, but the as_vec was unnecessary as well.

You can compare the relevant slices, there is no need to copy anything

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

Previously, ilyalesokhin-starkware wrote…

Yes, but the as_vec was unnecessary as well.

You can compare the relevant slices, there is no need to copy anything

ah, right
better?

[ilyalesokhin-starkware]

Reviewed 1 of 2 files at r5, 1 of 2 files at r6.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace.rs line 166 at r7 (raw file):

Previously, dorimedini-starkware wrote…

ah, right
better?

yes

[dorimedini-starkware]

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