feat(blockifier): limit characters in cairo1 revert trace

[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 👈 (View in Graphite)
• 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 89.55224% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.35%. Comparing base (e3165c4) to head (44f85b6).
Report is 36 commits behind head on main.

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

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1468       +/-   ##
===========================================
+ Coverage   40.10%   67.35%   +27.24%     
===========================================
  Files          26      103       +77     
  Lines        1895    13800    +11905     
  Branches     1895    13800    +11905     
===========================================
+ Hits          760     9295     +8535     
- Misses       1100     4104     +3004     
- Partials       35      401      +366     

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

[Yoni-Starkware]

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

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

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

---

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

            .chars()
            .take(TRACE_LENGTH_CAP)
            .collect::<String>()

I really don't like the fact that we need this logic to support an edge-case character limitation...
a frame is 264 characters, so 56 frames fit in the cap bound, not including the innermost retdata which should be short (unless there are use cases for many felts in the failure reason?)...
@ilyalesokhin-starkware if it's worth the extra code, do you have any ideas for improvements / helpful assumptions that will simplify the logic?

Code quote:

        // Total string length is limited by TRACE_LENGTH_CAP.

        // Prioritize the failure reason felts over the frames.
        // If the failure reason is too long to include a minimal frame trace, display only the
        // failure reason (truncated if necessary).
        let failure_reason = format_panic_data(&self.last_retdata.0);
        if failure_reason.len() >= TRACE_LENGTH_CAP - *MIN_CAIRO1_FRAMES_STACK_LENGTH {
            let output = if failure_reason.len() <= TRACE_LENGTH_CAP {
                failure_reason
            } else {
                failure_reason
                    .chars()
                    .take(TRACE_LENGTH_CAP - Self::TRUNCATION_SEPARATOR.len())
                    .collect::<String>()
                    + &format!("{}", Self::TRUNCATION_SEPARATOR)
            };
            return write!(f, "{}", output);
        }

        let untruncated_string = self
            .stack
            .iter()
            .map(|frame| frame.to_string())
            .chain([failure_reason.clone()])
            .join("\n");
        if untruncated_string.len() <= TRACE_LENGTH_CAP {
            return write!(f, "{}", untruncated_string);
        }

        // If the number of frames is too large, drop frames above the last frame (two frames are
        // not too many, as checked above with MIN_CAIRO1_FRAMES_STACK_LENGTH).
        let n_frames_to_drop = (untruncated_string.len() - TRACE_LENGTH_CAP
            + Self::TRUNCATION_SEPARATOR.len())
        .div_ceil(*MIN_CAIRO1_FRAME_LENGTH);

        // Expect the number of frames to be at least the number of frames to drop + 2. If not,
        // fall back to the failure reason.
        if self.stack.len() < n_frames_to_drop + 2 {
            return write!(f, "{}", failure_reason);
        }

        write!(
            f,
            "{}",
            self.stack[..self.stack.len() - n_frames_to_drop - 1]
            .iter()
            .map(|frame| frame.to_string())
            .chain([
                String::from(Self::TRUNCATION_SEPARATOR),
                self.stack
                    .last()
                    .expect("Stack has at least two frames at this point.")
                    .to_string(),
                failure_reason,
            ])
            .join("\n")
            // Truncate again as a failsafe.
            .chars()
            .take(TRACE_LENGTH_CAP)
            .collect::<String>()

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

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

 .expect("Stack has at least two frames at this point.")

I'd rather not use expect her.

[ilyalesokhin-starkware]

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

Previously, dorimedini-starkware wrote…

this is getting a bit ugly... both the stack and the failure reason can hit this limit, we may need to truncate them both.
when would we realistically have a trace longer than 15K chars?
anyway, modified, LMK what you think

I guess I didn't think it was going to be that complicated.

but if you already wrote it lets keep it.

[ilyalesokhin-starkware]

Reviewed 1 of 1 files at r5.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

[dorimedini-starkware]

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

---

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

Previously, ilyalesokhin-starkware wrote…

 .expect("Stack has at least two frames at this point.")

I'd rather not use expect her.

done

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

crates/blockifier/src/execution/stack_trace_test.rs line 851 at r6 (raw file):

    assert!(
        format!("{}", extract_trailing_cairo1_revert_trace(&next_call_info)).len()
            <= TRACE_LENGTH_CAP

Is putting the actual string here bad?

as it is written the length is the only thing being tested.

Code quote:

        format!("{}", extract_trailing_cairo1_revert_trace(&next_call_info)).len()
            <= TRACE_LENGTH_CAP

[dorimedini-starkware]

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

---

crates/blockifier/src/execution/stack_trace_test.rs line 851 at r6 (raw file):

Previously, ilyalesokhin-starkware wrote…

Is putting the actual string here bad?

as it is written the length is the only thing being tested.

Done.

[github-actions]

Artifacts upload triggered. View details here

[ilyalesokhin-starkware]

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