Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

refactor(execution): add messages info to ExecutionSummary #1666

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

Yael-Starkware
Copy link
Contributor

@Yael-Starkware Yael-Starkware commented Mar 13, 2024

This change is Reviewable

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1, all commit messages.
Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)


crates/blockifier/src/transaction/objects.rs line 229 at r1 (raw file):

    /// Returns a summary of transaction execution, including executed class hashes, visited storage
    /// entries, l2_to_l1_payload_lengths, and the number of emitted events.

Suggestion:

L2-to-L1 payload lengths,

crates/blockifier/src/transaction/objects.rs line 234 at r1 (raw file):

            self.non_optional_call_infos().map(|call_info| call_info.summarize()).collect();
        Ok(execution_summary_vec?.into_iter().sum())
    }

Why do you need the variable? can't you do it in one line?
I think you can keep it as-is

Code quote:

        let execution_summary_vec: TransactionExecutionResult<Vec<ExecutionSummary>> =
            self.non_optional_call_infos().map(|call_info| call_info.summarize()).collect();
        Ok(execution_summary_vec?.into_iter().sum())
    }

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 5 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)


crates/blockifier/src/transaction/objects.rs line 234 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why do you need the variable? can't you do it in one line?
I think you can keep it as-is

Anyway, this function

return Err(TransactionExecutionError::InvalidOrder {

need not return a Result. You can use assert! there.

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @Yael-Starkware)


crates/blockifier/src/bouncer.rs line 144 at r1 (raw file):

/// Calculates the L1 resources used by L1<>L2 messages.
/// Returns the total message segment length and the L1 gas usage.
pub fn calc_message_l1_resources(

calculate


crates/blockifier/src/bouncer.rs line 145 at r1 (raw file):

/// Returns the total message segment length and the L1 gas usage.
pub fn calc_message_l1_resources(
    execution_summary: &ExecutionSummary,

Pass just l2_to_l1_payload_lengths

Code quote:

execution_summary: &ExecutionSummary,

@Yael-Starkware Yael-Starkware force-pushed the yael/add_messages_info_to_summarize branch from ad2c8e2 to 6a8b976 Compare March 14, 2024 09:50
Copy link
Contributor Author

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 5 files reviewed, 4 unresolved discussions (waiting on @Yoni-Starkware)


crates/blockifier/src/bouncer.rs line 144 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

calculate

Done.


crates/blockifier/src/bouncer.rs line 145 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Pass just l2_to_l1_payload_lengths

Done.


crates/blockifier/src/transaction/objects.rs line 229 at r1 (raw file):

    /// Returns a summary of transaction execution, including executed class hashes, visited storage
    /// entries, l2_to_l1_payload_lengths, and the number of emitted events.

Done.


crates/blockifier/src/transaction/objects.rs line 234 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Anyway, this function

return Err(TransactionExecutionError::InvalidOrder {

need not return a Result. You can use assert! there.

as discussed on huddles, I removed the Result return value, and changed get_sorted_l2_to_l1_payload_lengths() to be unsorted.

@Yael-Starkware Yael-Starkware force-pushed the yael/add_messages_info_to_summarize branch from 6a8b976 to 0b73e36 Compare March 14, 2024 11:52
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 12 files at r2, all commit messages.
Reviewable status: 3 of 12 files reviewed, 3 unresolved discussions (waiting on @Yael-Starkware)


crates/blockifier/src/execution/call_info.rs line 196 at r3 (raw file):

            .map(|message| message.message.payload.0.len())
            .collect()
    }

Make it a function that gets l2_to_l1_messages (not self)
(Why? function names are too complicated)

Code quote:

    pub fn get_call_info_l2_to_l1_payload_lengths(&self) -> Vec<usize> {
        self.execution
            .l2_to_l1_messages
            .iter()
            .map(|message| message.message.payload.0.len())
            .collect()
    }

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 12 files at r2, 1 of 2 files at r3.
Reviewable status: 8 of 12 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)


crates/blockifier/src/transaction/objects.rs line 229 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Done.

Not exactly :)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 12 files at r2.
Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)

@Yael-Starkware Yael-Starkware force-pushed the yael/add_messages_info_to_summarize branch from 0b73e36 to 9c5a532 Compare March 14, 2024 12:26
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

Codecov Report

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

Project coverage is 73.98%. Comparing base (1912b42) to head (317d008).
Report is 7 commits behind head on main.

Files Patch % Lines
crates/blockifier/src/bouncer.rs 75.00% 5 Missing ⚠️
.../blockifier/src/blockifier/transaction_executor.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1666      +/-   ##
==========================================
+ Coverage   73.16%   73.98%   +0.82%     
==========================================
  Files          59       59              
  Lines        8572     8712     +140     
  Branches     8572     8712     +140     
==========================================
+ Hits         6272     6446     +174     
+ Misses       1809     1787      -22     
+ Partials      491      479      -12     

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

Yoni-Starkware
Yoni-Starkware previously approved these changes Mar 14, 2024
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 12 files at r2, 3 of 3 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

@Yael-Starkware Yael-Starkware merged commit b5c4339 into main Mar 14, 2024
10 checks passed
@Yael-Starkware Yael-Starkware deleted the yael/add_messages_info_to_summarize branch March 14, 2024 13:34
@Yael-Starkware Yael-Starkware restored the yael/add_messages_info_to_summarize branch March 14, 2024 13:51
apoorvsadana pushed a commit to apoorvsadana/blockifier that referenced this pull request May 13, 2024
apoorvsadana pushed a commit to apoorvsadana/blockifier that referenced this pull request May 13, 2024
apoorvsadana pushed a commit to apoorvsadana/blockifier that referenced this pull request May 13, 2024
apoorvsadana pushed a commit to apoorvsadana/blockifier that referenced this pull request May 13, 2024
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants