refactor(native_blockifier): flat fields in bouncerinfo

[ayeletstarkware]

This change is 

[codecov-commenter]

Codecov Report

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

Comparison is base (29d2d5f) 70.37% compared to head (26cfc50) 70.38%.

Files	Patch %	Lines
...ve_blockifier/src/py_transaction_execution_info.rs	0.00%	45 Missing ⚠️
...ates/native_blockifier/src/transaction_executor.rs	0.00%	8 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           main-v0.13.1    #1394   +/-   ##
=============================================
  Coverage         70.37%   70.38%           
=============================================
  Files                60       60           
  Lines              7773     7772    -1     
  Branches           7773     7772    -1     
=============================================
  Hits               5470     5470           
+ Misses             1874     1873    -1     
  Partials            429      429           

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

[ArniStarkware]

Python side PR: https://github.com/starkware-industries/starkware/pull/33661

[ArniStarkware]

Reviewed 3 of 4 files at r1.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware)

---

crates/blockifier/src/utils.rs line 38 at r1 (raw file):

    }
    result
}

This is the only time this is used. Cool.
Will we not want to use this util ever again?

Code quote:

pub fn merge_hashmaps<K, V>(x: &HashMap<K, V>, y: &HashMap<K, V>) -> HashMap<K, V>
where
    K: Hash + Eq + Clone,
    V: Add<Output = V> + AddAssign + Default + Clone,
{
    let mut result = x.clone();
    for (key, value) in y {
        result
            .entry(key.clone())
            .and_modify(|v| v.add_assign(value.clone()))
            .or_insert(value.clone());
    }
    result
}

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 219 at r1 (raw file):

}

type ResourcesMap = HashMap<String, usize>;

Is this type only used in this context (like this)?
Probably not. Use this type throughout the code, (like other instances of actual_resources). Add a todo to do it another time (please link here the number of the corresponding PR) - or delete this type.

Code quote:

type ResourcesMap = HashMap<String, usize>

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r1 (raw file):

impl PyBouncerInfo {
    pub fn calculate(

1. Is this function never being called? Why does it exist?

2. The pyclasses should be very simple - they should not implement any logic. On the other hand - this is just a constructor - which is exactly the kind of logic that do makes sense.

3. Is the name calculate correct here? are there any computations happening other than some additions? (Maybe additions are enough of an operator to be considered calculations...)

Code quote:

 pub fn calculate(

[ArniStarkware]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

1. Is this function never being called? Why does it exist?

2. The pyclasses should be very simple - they should not implement any logic. On the other hand - this is just a constructor - which is exactly the kind of logic that do makes sense.

3. Is the name calculate correct here? are there any computations happening other than some additions? (Maybe additions are enough of an operator to be considered calculations...)

Discussed F2F. Done.

[ayeletstarkware]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/blockifier/src/utils.rs line 38 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

This is the only time this is used. Cool.
Will we not want to use this util ever again?

I added this function in the last PR (for calculate_tx_weights).

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 219 at r1 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Is this type only used in this context (like this)?
Probably not. Use this type throughout the code, (like other instances of actual_resources). Add a todo to do it another time (please link here the number of the corresponding PR) - or delete this type.

Done.

[ArniStarkware]

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r2 (raw file):

impl PyBouncerInfo {
    pub fn calculate(

The pyclasses should be very simple - they should not implement any logic. On the other hand - this is just a constructor - which is precisely the kind of logic that do makes sense.
On the other other hand - there is some computation here - So maybe it should be a util.

@giladchase WDYT?

Code quote:

impl PyBouncerInfo {
    pub fn calculate(

[ArniStarkware]

crates/native_blockifier/src/transaction_executor.rs line 116 at r2 (raw file):

                let actual_resources = blockifier::transaction::objects::ResourcesMapping(
                    tx_execution_info.actual_resources.0.clone(),
                );

Can you use the type?

Suggestion:

                // In imports section.
                use blockifier::transaction::objects::ResourcesMapping;
                // Here
                let ResourcesMapping(
                    tx_execution_info.actual_resources.0.clone(),
                );

[ayeletstarkware]

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/native_blockifier/src/transaction_executor.rs line 116 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

Can you use the type?

Done.

[ArniStarkware]

Reviewed 1 of 1 files at r3.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)

[ArniStarkware]

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)

[elintul]

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 224 at r3 (raw file):

#[derive(Clone, Default)]
// TODO(Ayelet, 24/01/2024): Consider remove message_segment_length, state_diff_size.
pub struct PyBouncerInfo {

See my comment in the Python side, I think we should work with VmExecutionResources.

[ArniStarkware]

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

The pyclasses should be very simple - they should not implement any logic. On the other hand - this is just a constructor - which is precisely the kind of logic that do makes sense.
On the other other hand - there is some computation here - So maybe it should be a util.

@giladchase WDYT?

@elintul ?

[ayeletstarkware]

Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @elintul)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 224 at r3 (raw file):

Previously, elintul (Elin) wrote…

See my comment in the Python side, I think we should work with VmExecutionResources.

Done.

[elintul]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

@elintul ?

Did you try to read what's the best practice?

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 229 at r4 (raw file):

    pub gas_weight: usize,
    #[pyo3(get)]
    pub range_check_builtin: usize,

Should be removed, exists in PyVmExecutionResources.

Code quote:

pub range_check_builtin: usize,

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 243 at r4 (raw file):

        state_diff_size: usize,
    ) -> TransactionExecutionResult<Self> {
        let gas_weight = actual_resources.0.get("l1_gas_usage").ok_or_else(|| {

Can you build a VmExecutionResources out of ResourcesMapping and add them?

[elintul]

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

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 243 at r4 (raw file):

Previously, elintul (Elin) wrote…

Can you build a VmExecutionResources out of ResourcesMapping and add them?

Maybe we can do it outside, like we do today (when building additional_os_resources).

[elintul]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware and @giladchase)

[ArniStarkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r2 (raw file):

Previously, elintul (Elin) wrote…

Did you try to read what's the best practice?

a. No. Doing it right now.
b. If you didn't have a strong feeling towards that - it means we are not against having this as a logic here. Unblocking this discussion.

[elintul]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

a. No. Doing it right now.
b. If you didn't have a strong feeling towards that - it means we are not against having this as a logic here. Unblocking this discussion.

I'll have an opinion once I understand best practices. :)
We can also move this offline and consult with Gilad.

[ArniStarkware]

crates/native_blockifier/src/py_transaction_execution_info.rs line 242 at r2 (raw file):

Previously, elintul (Elin) wrote…

I'll have an opinion once I understand best practices. :)
We can also move this offline and consult with Gilad.

It is fine to create impls for Rust-only methods.

Straight from documentation:
https://pyo3.rs/v0.18.1/class.html?highlight=pycell#:~:text=Since%20Rust%20allows%20any%20number%20of%20impl%20blocks%2C%20you%20can%20easily%20split%20methods%20between%20those%20accessible%20to%20Python%20(and%20Rust)%20and%20those%20accessible%20only%20to%20Rust.

[ayeletstarkware]

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @elintul)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 229 at r4 (raw file):

Previously, elintul (Elin) wrote…

Should be removed, exists in PyVmExecutionResources.

Done.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 243 at r4 (raw file):

Previously, elintul (Elin) wrote…

Maybe we can do it outside, like we do today (when building additional_os_resources).

Done.

[elintul]

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 267 at r5 (raw file):

            builtin_instance_counter,
            n_memory_holes: merged_resources.n_memory_holes,
        };

Let's pass actual_resources inside, pop the L1 usage and create VmExecutionResources from the rest. Then + between the two resources (the one we created and OS ones) and use PyVmExecutionResources::from.

Code quote:

        let execution_resources = PyVmExecutionResources {
            n_steps,
            builtin_instance_counter,
            n_memory_holes: merged_resources.n_memory_holes,
        };

[ayeletstarkware]

Reviewable status: 3 of 5 files reviewed, 2 unresolved discussions (waiting on @elintul)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 267 at r5 (raw file):

Previously, elintul (Elin) wrote…

Let's pass actual_resources inside, pop the L1 usage and create VmExecutionResources from the rest. Then + between the two resources (the one we created and OS ones) and use PyVmExecutionResources::from.

Done.

[elintul]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 239 at r6 (raw file):

    pub fn calculate(
        additional_os_resources: VmExecutionResources,
        actual_resources: ResourcesMapping,

Suggestion:

actual_resources: &ResourcesMapping,

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 247 at r6 (raw file):

                field: "l1_gas_usage".to_string(),
            }
        })?;

expect + informative error message, also lose the variable.

Code quote:

        let l1_gas_amount = actual_resources.0.get("l1_gas_usage").ok_or_else(|| {
            TransactionExecutionError::InvalidTransactionExecutionInfo {
                field: "l1_gas_usage".to_string(),
            }
        })?;

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 249 at r6 (raw file):

        })?;

        let actual_resources = execution_resources_from_hashmap(&actual_resources.0);

Suggestion:

        let actual_resources = VmExecutionResources {
            n_steps: actual_resources.0.get(constants::N_STEPS_RESOURCE).unwrap_or_default(),
            builtin_instance_counter: HashMap::new(
                (name, actual_resources.get(name).unwrap_of_default()) for name in BUILTIN_NAMES
            ),
            n_memory_holes: actual_resources.get("n_memory_holes").unwrap_or_default(),
        };

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 251 at r6 (raw file):

        let actual_resources = execution_resources_from_hashmap(&actual_resources.0);

        let mut merged_resources = additional_os_resources.add(&actual_resources);

I think adding them is enough.

Suggestion:

+

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 253 at r6 (raw file):

        let mut merged_resources = additional_os_resources.add(&actual_resources);

        merged_resources.n_steps += merged_resources.n_memory_holes;

Zero it after usage.

Code quote:

merged_resources.n_memory_holes;

[ayeletstarkware]

Reviewable status: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @ArniStarkware and @elintul)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 239 at r6 (raw file):

    pub fn calculate(
        additional_os_resources: VmExecutionResources,
        actual_resources: ResourcesMapping,

Done.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 247 at r6 (raw file):

Previously, elintul (Elin) wrote…

expect + informative error message, also lose the variable.

Done.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 249 at r6 (raw file):

        })?;

        let actual_resources = execution_resources_from_hashmap(&actual_resources.0);

Done.
unwrap_or_default didn't work beacuse "get" returns an Option<&usize>, and unwrap_or_default expects usize in order to create the default value.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 251 at r6 (raw file):

Previously, elintul (Elin) wrote…

I think adding them is enough.

didn't work

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 253 at r6 (raw file):

Previously, elintul (Elin) wrote…

Zero it after usage.

Done.

[elintul]

Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ayeletstarkware)

a discussion (no related file):
In a Python PR: please add an assertion that the new bouncer info we get from Rust is equivalent to the one we currently calculate in Python. Only after this CI passes, can we merge this feature.

---

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 249 at r6 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

Done.
unwrap_or_default didn't work beacuse "get" returns an Option<&usize>, and unwrap_or_default expects usize in order to create the default value.

I didn't understand the last part of your sentence.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 251 at r6 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

didn't work

Please explain what you tried and the errors you got.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 239 at r7 (raw file):

    pub fn calculate(
        additional_os_resources: VmExecutionResources,
        actual_resources: &ResourcesMapping,

Swap order, by importance.

Suggestion:

        tx_actual_resources: &ResourcesMapping,
        tx_additional_os_resources: VmExecutionResources,

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 252 at r7 (raw file):

            BuiltinName::keccak,
            BuiltinName::poseidon,
        ];

Consider defining a constant list and use in other such locations throughout the code.

Code quote:

        let builtin_ordered_list = vec![
            BuiltinName::output,
            BuiltinName::pedersen,
            BuiltinName::range_check,
            BuiltinName::ecdsa,
            BuiltinName::bitwise,
            BuiltinName::ec_op,
            BuiltinName::keccak,
            BuiltinName::poseidon,
        ];

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 260 at r7 (raw file):

            .collect();

        let actual_resources_execution_resources = VmExecutionResources {

Can the name be shadowed?

Code quote:

actual_resources_execution_resources

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 269 at r7 (raw file):

            additional_os_resources.add(&actual_resources_execution_resources);
        merged_resources.n_steps += merged_resources.n_memory_holes;
        merged_resources.n_memory_holes = 0;

Suggestion:

        
        // Memory holes are counted as steps.
        merged_resources.n_steps += merged_resources.n_memory_holes;
        merged_resources.n_memory_holes = 0;

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 278 at r7 (raw file):

                .0
                .get("l1_gas_usage")
                .expect("Invalid Transaction Execution Info. Field l1_gas_usage was not found."),

It's longer than I expected, let's introduce back the variable. Sorry for the back and forth.

Code quote:

            l1_gas_amount: *actual_resources
                .0
                .get("l1_gas_usage")
                .expect("Invalid Transaction Execution Info. Field l1_gas_usage was not found."),

---

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

                );

                Ok((raw_tx_execution_info, py_bouncer_info))

Suggestion:

                let state_diff_size = 0;
                
                // Finalize counting logic.
                let typed_tx_execution_info = TypedTransactionExecutionInfo {
                    info: tx_execution_info,
                    tx_type: tx_type.to_string(),
                };
                let actual_resources = &typed_tx_execution_info.info.actual_resources;
                let py_bouncer_info = PyBouncerInfo::calculate(
                    additional_os_resources,
                    actual_resources,
                    message_segment_length,
                    state_diff_size,
                )?;
                
                self.staged_for_commit_state = Some(
                    transactional_state.stage(tx_executed_class_hashes, tx_visited_storage_entries),
                );
                let raw_tx_execution_info = serde_json::to_vec(&typed_tx_execution_info)?;
                Ok((raw_tx_execution_info, py_bouncer_info))

[elintul]

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 239 at r8 (raw file):

    pub fn calculate(
        tx_actual_resources: &ResourcesMapping,
        tx_additional_os_resources: VmExecutionResources, // os

Remove?

Code quote:

// os

[ayeletstarkware]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @elintul)

a discussion (no related file):

Previously, elintul (Elin) wrote…

In a Python PR: please add an assertion that the new bouncer info we get from Rust is equivalent to the one we currently calculate in Python. Only after this CI passes, can we merge this feature.

I deleted Python's "_calculate_tx_weights" on the previous PR. I couldn't add it back because this function used to sum "tx_receipt.execution_info.actual_resources" and "tx_receipt.additionial_os_resources" (as we see in the code snippet below).
"tx_receipt" doesn't have "additionial_os_resources" anymore. It has but "bouncer_info", which contains the calculation already.

I believe the rust's calculation does the same thing:
1 - sums up additional_os_resources + actual_resources
2 - pops "l1_gas_usage" from actual_resources and add it to the dictionary

Also, I discovered a new field of 'actual_resources', which was added recently - "l1_blob_gas_usage".
Do we need to add it to the bouncer?

Code snippet:

def _calculate_tx_weights(self, tx_receipt: StateConsumptionReceipt) -> CountingBouncerWeights:
        gas_weight, cairo_usage = extract_l1_gas_and_cairo_usage(
            resources=tx_receipt.execution_info.actual_resources
        )
        os_cairo_usage = tx_receipt.additional_os_resources.to_dict()
        cairo_usage = add_counters(cairo_usage, os_cairo_usage)

        return {
            **cairo_usage,
            "gas_weight": gas_weight,
            "message_segment_length": tx_receipt.bouncer_info.message_segment_length,
            STATE_DIFF_SIZE_WEIGHT_NAME: tx_receipt.bouncer_info.state_diff_size,
        }
        
def extract_l1_gas_and_cairo_usage(resources: ResourcesMapping) -> Tuple[int, ResourcesMapping]:
    cairo_resource_usage = dict(resources)
    return cairo_resource_usage.pop("l1_gas_usage"), cairo_resource_usage

---

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 249 at r6 (raw file):

Previously, elintul (Elin) wrote…

I didn't understand the last part of your sentence.

get returns Option<&V>:
pub fn get(&self, k: &Q) -> Option<&V>

unwrap_or_default expect usize:
the trait bound &usize: std::default::Default is not satisfied
the trait std::default::Default is implemented for usize

It works this way if you prefer it:

Code snippet:

match tx_actual_resources.0.get(constants::N_STEPS_RESOURCE) {
                Some(&value) => value,
                None => Default::default(),
            }

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 251 at r6 (raw file):

Previously, elintul (Elin) wrote…

Please explain what you tried and the errors you got.

cannot add cairo_vm::vm::runners::cairo_runner::ExecutionResources to cairo_vm::vm::runners::cairo_runner::ExecutionResources

core::ops::arith::Add
pub fn add(self, rhs: Rhs) -> Self::Output
Performs the + operation.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 239 at r7 (raw file):

Previously, elintul (Elin) wrote…

Swap order, by importance.

Done

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 252 at r7 (raw file):

Previously, elintul (Elin) wrote…

Consider defining a constant list and use in other such locations throughout the code.

Will do so in a different PR.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 260 at r7 (raw file):

Previously, elintul (Elin) wrote…

Can the name be shadowed?

WDYM?

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 269 at r7 (raw file):

            additional_os_resources.add(&actual_resources_execution_resources);
        merged_resources.n_steps += merged_resources.n_memory_holes;
        merged_resources.n_memory_holes = 0;

Done

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 278 at r7 (raw file):

Previously, elintul (Elin) wrote…

It's longer than I expected, let's introduce back the variable. Sorry for the back and forth.

Done.

---

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

                );

                Ok((raw_tx_execution_info, py_bouncer_info))

Done

[elintul]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 249 at r6 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

get returns Option<&V>:
pub fn get(&self, k: &Q) -> Option<&V>

unwrap_or_default expect usize:
the trait bound &usize: std::default::Default is not satisfied
the trait std::default::Default is implemented for usize

It works this way if you prefer it:

Sorry, I don't follow. Let's take this offline please.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 251 at r6 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

cannot add cairo_vm::vm::runners::cairo_runner::ExecutionResources to cairo_vm::vm::runners::cairo_runner::ExecutionResources

core::ops::arith::Add
pub fn add(self, rhs: Rhs) -> Self::Output
Performs the + operation.

I think you should work with references.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 260 at r7 (raw file):

Previously, ayeletstarkware (Ayelet Zilber) wrote…

WDYM?

Use actual_resources.

[ayeletstarkware]

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

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 251 at r6 (raw file):

Previously, elintul (Elin) wrote…

I think you should work with references.

Done.

[elintul]

Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware)

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 265 at r10 (raw file):

            .collect();

        let tx_actual_resources = VmExecutionResources {

Suggestion:

            .collect();
        let tx_actual_resources = VmExecutionResources {

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 273 at r10 (raw file):

        let mut merged_resources = &tx_additional_os_resources + &tx_actual_resources;

        // Memory holes are counted as steps.

Suggestion:

        let mut merged_resources = &tx_additional_os_resources + &tx_actual_resources;
        // Memory holes are counted as steps.

---

crates/native_blockifier/src/py_transaction_execution_info.rs line 277 at r10 (raw file):

        merged_resources.n_memory_holes = 0;

        let execution_resources = PyVmExecutionResources::from(merged_resources);

I think we can lose this variable.

Code quote:

execution_resources

[elintul]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ayeletstarkware)

[ArniStarkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ayeletstarkware)

a discussion (no related file):

Previously, ArniStarkware (Arnon Hod) wrote…

Python side PR: https://github.com/starkware-industries/starkware/pull/33661

Please, some else block.

---

[elintul]

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

[elintul]

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