chore(blockifier): add with_attr error message

[aner-starkware]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.45%. Comparing base (29f90d3) to head (95c4bc7).
Report is 8 commits behind head on main-v0.13.2.

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.2     #624      +/-   ##
================================================
+ Coverage         76.43%   76.45%   +0.02%     
================================================
  Files               316      316              
  Lines             34263    34306      +43     
  Branches          34263    34306      +43     
================================================
+ Hits              26188    26229      +41     
- Misses             5797     5799       +2     
  Partials           2278     2278              

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

[dorimedini-starkware]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @aner-starkware and @TzahiTaub)

---

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

        };
        // TODO(Aner): add test for error message in intermediate call"
        format!("{error_msg}{vm_exception_preamble}{vm_exception_traceback}")

if the error message exists you should add a newline, no?
or is the newline already in the error attr?

Code quote:

{error_msg}{

---

crates/blockifier/src/execution/stack_trace_test.rs line 173 at r2 (raw file):

        }
        CairoVersion::Cairo1 => String::new(),
    };

simpler

Suggestion:

    let expected_with_attr_error_msg = match (cairo_version, last_func_name) {
        (CairoVersion::Cairo0, "fail") => "Error message: You shall not pass!\n".to_string(),
        _ => String::new(),
    };

---

crates/blockifier/src/execution/stack_trace_test.rs line 248 at r2 (raw file):

2: Error in the called contract (contract address: {contract_address_felt:#064x}, class hash: \
                 {test_contract_hash:#064x}, selector: {invoke_call_chain_selector_felt:#064x}):
{expected_with_attr_error_msg}Error at pc=0:{expected_pc0}:

need a newline after this line

Code quote:

{expected_with_attr_error_msg}

---

crates/blockifier/src/execution/stack_trace_test.rs line 308 at r2 (raw file):

        }
        CairoVersion::Cairo1 => String::new(),
    };

see above for simplification

Code quote:

    let expected_with_attr_error_msg = match cairo_version {
        CairoVersion::Cairo0 => {
            if last_func_name == "fail" {
                "Error message: You shall not pass!\n".to_string()
            } else {
                String::new()
            }
        }
        CairoVersion::Cairo1 => String::new(),
    };

---

crates/blockifier/src/execution/stack_trace_test.rs line 411 at r2 (raw file):

3: {last_call_preamble}:
{expected_with_attr_error_msg}Error at pc=0:{expected_pc2}:

newline

Code quote:

{expected_with_attr_error_msg}

[aner-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

if the error message exists you should add a newline, no?
or is the newline already in the error attr?

I wrote it so that it passes the test, so I guess this is the correct way. While there might of course be other correct ways, just adding something, e.g., a newline, will cause it to fail the test.
So the other logical option seems to be that when there is no error message, there will just be a blank line - would it be a better option? (IMO, the current solution is better)

---

crates/blockifier/src/execution/stack_trace_test.rs line 173 at r2 (raw file):

Previously, dorimedini-starkware wrote…

simpler

Done.

---

crates/blockifier/src/execution/stack_trace_test.rs line 248 at r2 (raw file):

Previously, dorimedini-starkware wrote…

need a newline after this line

See above.

---

crates/blockifier/src/execution/stack_trace_test.rs line 308 at r2 (raw file):

Previously, dorimedini-starkware wrote…

see above for simplification

Done.

---

crates/blockifier/src/execution/stack_trace_test.rs line 411 at r2 (raw file):

Previously, dorimedini-starkware wrote…

newline

See above.

[dorimedini-starkware]

Reviewed 1 of 4 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware and @TzahiTaub)

---

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

Previously, aner-starkware wrote…

I wrote it so that it passes the test, so I guess this is the correct way. While there might of course be other correct ways, just adding something, e.g., a newline, will cause it to fail the test.
So the other logical option seems to be that when there is no error message, there will just be a blank line - would it be a better option? (IMO, the current solution is better)

OK, the below comments I had about newlines (in tests) - you are right, your way is better.
here, though, I don't understand how you get the newline for free?
in the with_attr, the string doesn't include a newline,
but somehow in tests you get a newline in the formatted frame.
how does this happen?
does the with_attr automatically append a newline?

[aner-starkware]

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

Previously, dorimedini-starkware wrote…

OK, the below comments I had about newlines (in tests) - you are right, your way is better.
here, though, I don't understand how you get the newline for free?
in the with_attr, the string doesn't include a newline,
but somehow in tests you get a newline in the formatted frame.
how does this happen?
does the with_attr automatically append a newline?

Apparently it's automatically appended, as I didn't specifically add a newline in the Cairo code. Should I investigate it further? I guess it's worth verifying this is the intended behaviour.

[dorimedini-starkware]

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

---

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

Previously, aner-starkware wrote…

Apparently it's automatically appended, as I didn't specifically add a newline in the Cairo code. Should I investigate it further? I guess it's worth verifying this is the intended behaviour.

if you can, please do; it looks like this is the behavior but we should verify so we don't start seeing mangled error messages on public endpoints

[aner-starkware]

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

Previously, dorimedini-starkware wrote…

if you can, please do; it looks like this is the behavior but we should verify so we don't start seeing mangled error messages on public endpoints

OK, will do. Who should I ask?

[dorimedini-starkware]

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

---

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

Previously, aner-starkware wrote…

OK, will do. Who should I ask?

in the python VM it looks like that's what's happening;
for the rust VM - you will have to read to verify :)
I would clone the VM repo locally and search for with_attr handling, i.e. search for with_attr or the Error message: prefix

[aner-starkware]

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

Previously, dorimedini-starkware wrote…

in the python VM it looks like that's what's happening;
for the rust VM - you will have to read to verify :)
I would clone the VM repo locally and search for with_attr handling, i.e. search for with_attr or the Error message: prefix

Wouldn't that just be verifying the actual behaviour (which seems to be adding a newline). I meant how do I check this is the intended behaviour?

[dorimedini-starkware]

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

---

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

Previously, aner-starkware wrote…

Wouldn't that just be verifying the actual behaviour (which seems to be adding a newline). I meant how do I check this is the intended behaviour?

it is intended :) (request from product team)
unblocking