perf(native_blockifier): transaction execution info add serialize

[MohammadNassar1]

This change is 

[codecov-commenter]

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (bb30f51) 70.69% compared to head (3179a58) 70.95%.
Report is 2 commits behind head on main-v0.13.1.

Files	Patch %	Lines
...ates/native_blockifier/src/transaction_executor.rs	0.00%	17 Missing ⚠️
crates/blockifier/src/execution/call_info.rs	0.00%	1 Missing and 6 partials ⚠️
crates/native_blockifier/src/py_validator.rs	0.00%	3 Missing ⚠️
crates/blockifier/src/execution/entry_point.rs	0.00%	0 Missing and 2 partials ⚠️
crates/blockifier/src/transaction/objects.rs	0.00%	0 Missing and 2 partials ⚠️
...es/blockifier/src/transaction/transaction_types.rs	0.00%	0 Missing and 1 partial ⚠️
crates/native_blockifier/src/py_block_executor.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##           main-v0.13.1    #1315      +/-   ##
================================================
+ Coverage         70.69%   70.95%   +0.25%     
================================================
  Files                60       62       +2     
  Lines              7794     7980     +186     
  Branches           7794     7980     +186     
================================================
+ Hits               5510     5662     +152     
- Misses             1856     1898      +42     
+ Partials            428      420       -8     

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

[elintul]

Reviewed 10 of 14 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 16 at r2 (raw file):

use crate::transaction::objects::TransactionExecutionResult;

#[derive(Clone, Debug, Default, Eq, PartialEq, Serialize)]

What is the default impl. of this trait?

Code quote:

Serialize

---

crates/blockifier/src/execution/call_info.rs line 59 at r2 (raw file):

#[derive(Debug, Default, Eq, PartialEq)]
pub struct VmExecutionResourcesWrapper(pub VmExecutionResources);

As VmExecutionResources is already an alias, consider renaming it to VmExecutionResourcesInner and then you won't have to change the type in CallInfo.
If in the future LC adds the trait to their struct, we'd have less changes.

Code quote:

VmExecutionResourcesWrapper

---

crates/blockifier/src/execution/call_info.rs line 59 at r2 (raw file):

#[derive(Debug, Default, Eq, PartialEq)]
pub struct VmExecutionResourcesWrapper(pub VmExecutionResources);

Consider adding new and implementing Deref for less changes throughout the code. Perhaps Gilad will have a good suggestion.

Code quote:

pub struct VmExecutionResourcesWrapper

---

crates/blockifier/src/execution/call_info.rs line 62 at r2 (raw file):

impl Serialize for VmExecutionResourcesWrapper {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>

Can we ask LC to add this trait to their repo.?
Also, while currently implementing here, why can't we add Serialize to the wrapper? Is repeating the inner structure necessary?

Code quote:

fn serialize

---

crates/blockifier/src/execution/call_info.rs line 75 at r2 (raw file):

        // Finish serializing the struct
        state.end()

Suggestion:

        // Create a serialize struct with the appropriate number of fields.
        let mut state = serializer.serialize_struct("ExecutionResourcesWrapper", 3)?;

        // Serialize each field individually.
        state.serialize_field("builtin_instance_counter", &self.0.builtin_instance_counter)?;
        state.serialize_field("n_memory_holes", &self.0.n_memory_holes)?;
        state.serialize_field("n_steps", &self.0.n_steps)?;

        // Finish serializing the struct.
        state.end()

---

crates/blockifier/src/execution/call_info.rs line 93 at r2 (raw file):

impl Serialize for CallInfo {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>

Why implement manually?

Code quote:

fn serialize<

---

crates/blockifier/src/execution/entry_point_execution.rs line 14 at r2 (raw file):

use starknet_api::stark_felt;

use super::call_info::VmExecutionResourcesWrapper;

Suggestion:

crate::...

[elintul]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

a discussion (no related file):
Just to make sure: only the structs required to allow serialization of TransactionExecutionInfo now implement Serialize, right?

---

[MohammadNassar1]

Reviewable status: 11 of 14 files reviewed, 6 unresolved discussions (waiting on @elintul and @giladchase)

a discussion (no related file):

Previously, elintul (Elin) wrote…

Just to make sure: only the structs required to allow serialization of TransactionExecutionInfo now implement Serialize, right?

Goal: to enable serialization for TransactionExecutionInfo.
I've derived serialize to all the inner structs, and implemented Serialize for the VmExecutionResources as it's an LC struct.

---

---

crates/blockifier/src/execution/call_info.rs line 16 at r2 (raw file):

Previously, elintul (Elin) wrote…

What is the default impl. of this trait?

something like:
self.0.serialize(...)
Which is the same like my below implementation for VmExecutionResourcesWrapper.

---

crates/blockifier/src/execution/call_info.rs line 59 at r2 (raw file):

Previously, elintul (Elin) wrote…

As VmExecutionResources is already an alias, consider renaming it to VmExecutionResourcesInner and then you won't have to change the type in CallInfo.
If in the future LC adds the trait to their struct, we'd have less changes.

In all the files?
Many files are using this alias. example

---

crates/blockifier/src/execution/call_info.rs line 59 at r2 (raw file):

Previously, elintul (Elin) wrote…

Consider adding new and implementing Deref for less changes throughout the code. Perhaps Gilad will have a good suggestion.

Done.

---

crates/blockifier/src/execution/call_info.rs line 62 at r2 (raw file):

Previously, elintul (Elin) wrote…

Can we ask LC to add this trait to their repo.?
Also, while currently implementing here, why can't we add Serialize to the wrapper? Is repeating the inner structure necessary?

Adding Serialize to the Wrapper requires an implementation in the inner fields.

---

crates/blockifier/src/execution/call_info.rs line 93 at r2 (raw file):

Previously, elintul (Elin) wrote…

Why implement manually?

You are right; no need for it.

[elintul]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 13 of 14 files reviewed, 4 unresolved discussions (waiting on @giladchase)

[giladchase]

Reviewed 8 of 14 files at r1, 3 of 4 files at r2, 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @elintul and @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 16 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

something like:
self.0.serialize(...)
Which is the same like my below implementation for VmExecutionResourcesWrapper.

It'll ignore the wrapper types and just print the primitives in json format, so in this case, an array of [u8;32] arrays.

Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=569bdb8b3bc4d77d369b1e591ae83a7d

---

crates/blockifier/src/execution/call_info.rs line 68 at r4 (raw file):

        &self.0
    }
}

The derive generates the code you wrote

Suggestion:

#[derive(Debug, Default, Eq, PartialEq, derive_more::Deref)]
pub struct VmExecutionResourcesWrapper(pub VmExecutionResources);

---

crates/blockifier/src/execution/deprecated_entry_point_execution.rs line 256 at r4 (raw file):

            gas_consumed: 0,
        },
        vm_resources: VmExecutionResourcesWrapper(full_call_vm_resources.filter_unused_builtins()),

nit: Consider adding derive_more::From to this type and replacing it with full_call_vm_resources.filter_unused_builtins().into(), this hides the newtype which is generally a good thing, since it only exists due to technical constraints and doesn't have any significance on its own.
(DItto for other constructors in other files.)

But totally not critical, can stay as is.

Code quote:

VmExecutionResourcesWrapper(full_call_vm_resources.filter_unused_builtins())

[MohammadNassar1]

Reviewable status: 8 of 14 files reviewed, 5 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/blockifier/src/execution/call_info.rs line 68 at r4 (raw file):

Previously, giladchase wrote…

The derive generates the code you wrote

Done.

---

crates/blockifier/src/execution/deprecated_entry_point_execution.rs line 256 at r4 (raw file):

Previously, giladchase wrote…

nit: Consider adding derive_more::From to this type and replacing it with full_call_vm_resources.filter_unused_builtins().into(), this hides the newtype which is generally a good thing, since it only exists due to technical constraints and doesn't have any significance on its own.
(DItto for other constructors in other files.)

But totally not critical, can stay as is.

Liked it.
Code was updated accordingly.

[giladchase]

Reviewed 6 of 6 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @elintul)

[elintul]

Reviewed all commit messages.
Reviewable status: 15 of 17 files reviewed, 5 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 16 at r2 (raw file):

Previously, giladchase wrote…

It'll ignore the wrapper types and just print the primitives in json format, so in this case, an array of [u8;32] arrays.

Example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=569bdb8b3bc4d77d369b1e591ae83a7d

Thanks!

---

crates/blockifier/src/execution/call_info.rs line 58 at r7 (raw file):

}

#[derive(Debug, Default, Eq, PartialEq, derive_more::Deref, derive_more::From)]

Do we still need .0-s throughout the code?

Code quote:

derive_more::Deref

---

crates/blockifier/src/execution/call_info.rs line 73 at r7 (raw file):

    {
        // Create a serialize struct with the appropriate number of fields.
        let mut state = serializer.serialize_struct("ExecutionResourcesWrapper", 3)?;

What does this name represent?

Code quote:

"ExecutionResourcesWrapper"

[MohammadNassar1]

Reviewable status: 11 of 17 files reviewed, 5 unresolved discussions (waiting on @elintul and @giladchase)

---

crates/blockifier/src/execution/call_info.rs line 58 at r7 (raw file):

Previously, elintul (Elin) wrote…

Do we still need .0-s throughout the code?

I used .0 in one case when using PyVmExecutionResources::from, which doesn't call deref automatically. The other option is to call it explicitly with .deref() instead of .0. I prefer the second option.

---

crates/blockifier/src/execution/call_info.rs line 73 at r7 (raw file):

Previously, elintul (Elin) wrote…

What does this name represent?

It's the name of the struct.
It's not used during the serialization. An example of an output:

"vm_resources":{"builtin_instance_counter":{"range_check_builtin":1},"n_memory_holes":0,"n_steps":62}

[giladchase]

Reviewable status: 11 of 17 files reviewed, 5 unresolved discussions (waiting on @elintul and @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 62 at r2 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

Adding Serialize to the Wrapper requires an implementation in the inner fields.

More specifically, adding derive(Serialize) requires that all fields also implement Serialize, but since LC don't implement it and it's their type, we are forced to wrap it in a newType.

However, like you said, if LC adds derive(Serialize) it'll render this wrapper unnecessary.

---

crates/blockifier/src/execution/call_info.rs line 58 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

I used .0 in one case when using PyVmExecutionResources::from, which doesn't call deref automatically. The other option is to call it explicitly with .deref() instead of .0. I prefer the second option.

Deref is only for method calls unfortunately, so it doesn't apply for field accessors unless they are via getters :(

A common solution for this is to add an inner() method which returns self.0 (we haven't done this alot thus far but it's idiomatic).

This allows you to make the inner field private and communicates the NewType'iness of the instance better than .0 does (because the reader can't tell for certain if this is a newtype or a larger tuple struct with .1,.2,... .

(BTW performance-wise this method will likely get inlined anyway)

---

crates/blockifier/src/execution/call_info.rs line 73 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

It's the name of the struct.
It's not used during the serialization. An example of an output:

"vm_resources":{"builtin_instance_counter":{"range_check_builtin":1},"n_memory_holes":0,"n_steps":62}

I think she meant the syntax, it's the serialization API that serde exposes for defining a custom serialization scheme for a struct that doesn't derive(Serialize) in the normal way.
I think a better api for it could have been let mut custom_serializer = serializer.begin_custom_serialization_for("NameOfStructYouWantToSerializeNow",...)

[MohammadNassar1]

Reviewable status: 11 of 17 files reviewed, 4 unresolved discussions (waiting on @elin, @elintul, and @giladchase)

---

crates/blockifier/src/execution/call_info.rs line 58 at r7 (raw file):

Previously, giladchase wrote…

Deref is only for method calls unfortunately, so it doesn't apply for field accessors unless they are via getters :(

A common solution for this is to add an inner() method which returns self.0 (we haven't done this alot thus far but it's idiomatic).

This allows you to make the inner field private and communicates the NewType'iness of the instance better than .0 does (because the reader can't tell for certain if this is a newtype or a larger tuple struct with .1,.2,... .

(BTW performance-wise this method will likely get inlined anyway)

@elin, what so you think?

---

crates/blockifier/src/execution/call_info.rs line 73 at r7 (raw file):

Previously, giladchase wrote…

I think she meant the syntax, it's the serialization API that serde exposes for defining a custom serialization scheme for a struct that doesn't derive(Serialize) in the normal way.
I think a better api for it could have been let mut custom_serializer = serializer.begin_custom_serialization_for("NameOfStructYouWantToSerializeNow",...)

@giladchase, can you please tell me how to use this API?
I couldn't find such a method in the serializer.

[giladchase]

Reviewable status: 6 of 17 files reviewed, 4 unresolved discussions (waiting on @elin and @elintul)

---

crates/blockifier/src/execution/call_info.rs line 73 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

@giladchase, can you please tell me how to use this API?
I couldn't find such a method in the serializer.

Ahh this doesn't exist, i was just giving an example for a better name for this that would have made this action more obvious that "serialize_struct" (which doesn't imply that it is opening a state).

[elintul]

Reviewed 2 of 3 files at r10, all commit messages.
Reviewable status: 7 of 17 files reviewed, 7 unresolved discussions (waiting on @elin, @giladchase, and @MohammadNassar1)

---

crates/native_blockifier/src/py_block_executor.rs line 88 at r10 (raw file):

        tx: &PyAny,
        raw_contract_class: Option<&str>,
    ) -> NativeBlockifierResult<(Vec<u8>, PyBouncerInfo)> {

Please define an alias: RawTransactionExecutionInfo.

Code quote:

Vec<u8>

---

crates/native_blockifier/src/py_validator.rs line 74 at r10 (raw file):

        // so they are skipped here.
        if let AccountTransaction::DeployAccount(_deploy_account_tx) = account_tx {
            let (_bytes_tx_execution_info, _py_bouncer_info) =

In other locations as well.

Suggestion:

_raw_tx_execution_info

---

crates/native_blockifier/src/transaction_executor.rs line 90 at r10 (raw file):

                tx_visited_storage_entries.extend(tx_execution_info.get_visited_storage_entries());
                let bytes_tx_execution_info =
                    serde_json::to_string(&tx_execution_info).unwrap().into_bytes();

Please return SerdeError instead of unwraping.

Code quote:

unwrap()

[MohammadNassar1]

Reviewable status: 4 of 17 files reviewed, 6 unresolved discussions (waiting on @elin, @elintul, and @giladchase)

---

crates/native_blockifier/src/py_block_executor.rs line 88 at r10 (raw file):

Previously, elintul (Elin) wrote…

Please define an alias: RawTransactionExecutionInfo.

Done.

---

crates/native_blockifier/src/transaction_executor.rs line 90 at r10 (raw file):

Previously, elintul (Elin) wrote…

Please return SerdeError instead of unwraping.

Done.

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 89 at r18 (raw file):

        wrapper.0
    }
}

The name is somewhat confusing, it doens't generate an "Into", it generates the exact from you wrote.
The derive_more::From before it genreates the reverse From (bad api on their part...)

Suggestion:

#[derive(Debug, Default, derive_more::Deref, derive_more::From, derive_more::Into Eq, PartialEq)]
pub struct VmExecutionResourcesWrapper(pub VmExecutionResources);

[giladchase]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 58 at r7 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

For using inner method, we still need to add .inner in the code.
I implement from, to be able to use .into().
However, I still need to use .0 in one place in the code.

You mean in calculate_tx_weights which ownership on the inner vlaue?
I think the function can be changed to get the value by ref (doens't seem to do anything with the ownership), and then Deref can be used.

[giladchase]

For future reference: i found this serde trick which allows you to implement [De]Serialize on extetrnal types (which side steps the need for a wrapper), but i'm not sure it's worth it to scrap your work thus far for that
https://serde.rs/remote-derive.html#deriving-deserialize-for-type-in-a-different-crate.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

[elintul]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 6 of 18 files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)

---

crates/blockifier/src/execution/call_info.rs line 58 at r7 (raw file):

Previously, giladchase wrote…

You mean in calculate_tx_weights which ownership on the inner vlaue?
I think the function can be changed to get the value by ref (doens't seem to do anything with the ownership), and then Deref can be used.

Following @giladchase suggestion, I removed the wrapper and used Serde to derive Serialize as explained here.

---

crates/blockifier/src/execution/call_info.rs line 89 at r18 (raw file):

Previously, giladchase wrote…

The name is somewhat confusing, it doens't generate an "Into", it generates the exact from you wrote.
The derive_more::From before it genreates the reverse From (bad api on their part...)

This code was removed.
See the above comment.

[elintul]

Reviewed 12 of 12 files at r19, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @giladchase and @MohammadNassar1)

---

crates/blockifier/src/execution/call_info.rs line 83 at r19 (raw file):

// This struct is just a copy of the remote `VmExecutionResources` Struct.
// The remote attribute gives the path to the actual type we intend to derive
// Serialize and Deserialize for.

I think it's enough to say it is used to implement serde functionality on VmExecutionResources which is not defined inside the crate.

Code quote:

// This struct is just a copy of the remote `VmExecutionResources` Struct.
// The remote attribute gives the path to the actual type we intend to derive
// Serialize and Deserialize for.

---

crates/blockifier/src/execution/call_info.rs line 98 at r19 (raw file):

    pub execution: CallExecution,
    // It gives the path to the definition for the remote type.
    #[serde(with = "VmExecutionResourcesDef")]

I think the comment doesn't add information.

Suggestion:

    #[serde(with = "VmExecutionResourcesDef")]

[giladchase]

Reviewable status: 9 of 18 files reviewed, 1 unresolved discussion (waiting on @elintul and @MohammadNassar1)

---

crates/native_blockifier/src/transaction_executor.rs line 39 at r20 (raw file):

    #[serde(flatten)]
    info: TransactionExecutionInfo,
    tx_type: String,

Can this be TransactionType?

Code quote:

    tx_type: String,

---

crates/native_blockifier/src/transaction_executor.rs line 153 at r20 (raw file):

                };
                let raw_tx_execution_info = serde_json::to_vec(&typed_tx_execution_info)
                    .map_err(NativeBlockifierError::SerdeError)?;

I think this will still work, the ? runs into behind the scenes on the error

Suggestion:

                let raw_tx_execution_info = serde_json::to_vec(&typed_tx_execution_info)?;

[giladchase]

Reviewed 1 of 10 files at r9, 1 of 6 files at r12, 1 of 9 files at r13, 5 of 12 files at r19, 9 of 9 files at r20, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @elintul and @MohammadNassar1)

[elintul]

Reviewed 9 of 9 files at r20, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MohammadNassar1)

[MohammadNassar1]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @elintul and @giladchase)

---

crates/native_blockifier/src/transaction_executor.rs line 39 at r20 (raw file):

Previously, giladchase wrote…

Can this be TransactionType?

It doesn't work.

[giladchase]

Reviewed 1 of 1 files at r21, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)

[elintul]

Reviewed 1 of 1 files at r21, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @MohammadNassar1)