feat(blockifier): cairo1 revert stack now part of ErrorStack

[dorimedini-starkware]

No description provided.

[reviewable-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 👈 (View in Graphite)
• 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.

[codecov]

Codecov Report

Attention: Patch coverage is 91.42857% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.32%. Comparing base (e3165c4) to head (736d0dc).
Report is 417 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/blockifier/src/execution/stack_trace.rs	89.09%	6 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1490       +/-   ##
===========================================
+ Coverage   40.10%   69.32%   +29.22%     
===========================================
  Files          26      105       +79     
  Lines        1895    13804    +11909     
  Branches     1895    13804    +11909     
===========================================
+ Hits          760     9570     +8810     
- Misses       1100     3831     +2731     
- Partials       35      403      +368     

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

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

because this cairo1 revert stack cannot contain entry point frames or vm frames in it; once we hit a Cairo1 revert stack, it must be by definition the tail of the stack.
since this is a variant of Frame, and the cairo1 revert stack contains it's own concept of frames, I thought it wise to be explicit in the variant name

What is a frame?

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

why not? it is part of the data; the cairo1 revert stack always starts comes from one of two sources (validate/execute)

the stack is the same regardless of if the source is validate or execute

[dorimedini-starkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

What is a frame?

up until now, two frames basically: entry point frame (one per syscall) and VM frame (one per inner call in a contract, one PC location per frame).
I am open to renaming this type if you think it's confusing

---

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

Previously, ilyalesokhin-starkware wrote…

the stack is the same regardless of if the source is validate or execute

not it's string formatting. we provide a header indicating whether this is from validate or execute.

[dorimedini-starkware]

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

---

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

Previously, dorimedini-starkware wrote…

not it's string formatting. we provide a header indicating whether this is from validate or execute.

the TransactionExecutionInfo needs this info, because it will store a non-stringified error stack object.
when converted to a string we need to print the respective header

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

the TransactionExecutionInfo needs this info, because it will store a non-stringified error stack object.
when converted to a string we need to print the respective header

can you store it in the TansactionExecutionInfo?

[dorimedini-starkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

can you store it in the TansactionExecutionInfo?

why?
this means that ErrorStack cannot implement Display, because part of the data needed to format the string is not in the struct

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

up until now, two frames basically: entry point frame (one per syscall) and VM frame (one per inner call in a contract, one PC location per frame).
I am open to renaming this type if you think it's confusing

Putting a stack and a frame in stack in the same enum is strange.

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

why?
this means that ErrorStack cannot implement Display, because part of the data needed to format the string is not in the struct

If you add it it is no longer a just a revert stack.

Maybe we should rename it to
Cairo1RevertSummery?

[dorimedini-starkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

Putting a stack and a frame in stack in the same enum is strange.

renamed

---

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

Previously, ilyalesokhin-starkware wrote…

If you add it it is no longer a just a revert stack.

Maybe we should rename it to
Cairo1RevertSummery?

Done.

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

renamed

What about the variant name?

[ilyalesokhin-starkware]

Reviewed 4 of 8 files at r2, 3 of 6 files at r4.
Reviewable status: 4 of 9 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

[dorimedini-starkware]

Reviewable status: 4 of 9 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @ilyalesokhin-starkware)

---

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

Previously, ilyalesokhin-starkware wrote…

What about the variant name?

what do you suggest?

[ilyalesokhin-starkware]

Reviewable status: 4 of 9 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

---

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

Previously, dorimedini-starkware wrote…

what do you suggest?

Cairo1RevertSummery
or just
RevertSummery

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: 8 of 9 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[dorimedini-starkware]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 8 of 9 files reviewed, all discussions resolved

[dorimedini-starkware]

Reviewed 1 of 8 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Merge activity

• Nov 17, 5:09 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Nov 17, 5:09 AM EST: A user merged this pull request with Graphite.