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

feat(fee): add linear combination for gas and blob_gas, modify relevant tests #1360

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

aner-starkware
Copy link
Contributor

@aner-starkware aner-starkware commented Jan 22, 2024

Modified the tests:
test_calculate_tx_gas_usage_basic, test_invoke_tx, test_declare_tx, test_deploy_account_tx, test_l1_handler, test_calculate_tx_gas_usage


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

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

Comparison is base (0a5416b) 70.43% compared to head (e9e2e0a) 70.57%.

Files Patch % Lines
crates/blockifier/src/fee/gas_usage.rs 81.08% 5 Missing and 2 partials ⚠️
crates/blockifier/src/utils.rs 66.66% 0 Missing and 2 partials ⚠️
crates/blockifier/src/fee/fee_checks.rs 80.00% 0 Missing and 1 partial ⚠️
crates/blockifier/src/fee/fee_utils.rs 90.90% 0 Missing and 1 partial ⚠️
crates/blockifier/src/transaction/errors.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           main-v0.13.1    #1360      +/-   ##
================================================
+ Coverage         70.43%   70.57%   +0.13%     
================================================
  Files                59       59              
  Lines              7713     7754      +41     
  Branches           7713     7754      +41     
================================================
+ Hits               5433     5472      +39     
- Misses             1853     1854       +1     
- Partials            427      428       +1     

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

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context5 branch from b7681f4 to 43164c5 Compare January 22, 2024 09:33
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from 9f34a92 to 296dd81 Compare January 22, 2024 09:42
@aner-starkware aner-starkware self-assigned this Jan 22, 2024
@ArniStarkware
Copy link
Contributor

crates/blockifier/src/fee/actual_cost.rs line 144 at r2 (raw file):

            true => calculate_tx_blob_gas_usage(state_changes_count),
            false => 0,
        };

I am confused.

Maybe there is a mistake in the previous commit?

Suggestion:

        # We return the blob_gas here?
        let (l1_gas_usage, _l1_blob_gas_usage) = calculate_tx_gas_and_blob_gas_usage(
            non_optional_call_infos,
            state_changes_count,
            self.l1_payload_size,
            self.block_context.block_info.use_kzg_da,
        )?;
        # We also return the blob gas here?
        let l1_blob_gas_usage = match self.block_context.block_info.use_kzg_da {
            true => calculate_tx_blob_gas_usage(state_changes_count),
            false => 0,
        };

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context5 branch from 43164c5 to 7055020 Compare January 22, 2024 12:26
Copy link
Contributor

@ArniStarkware ArniStarkware 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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @aner-starkware)


crates/blockifier/src/fee/actual_cost.rs line 144 at r2 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

I am confused.

Maybe there is a mistake in the previous commit?

There is no need to call calculate_tx_blob_gas_usage. It happens inside calculate_tx_gas_and_blob_gas_usage in your last PR.

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from 296dd81 to 2ed9893 Compare January 22, 2024 12:26
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context5 branch 7 times, most recently from 3f6e54b to eac6bb4 Compare January 22, 2024 14:35
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch 2 times, most recently from 14f7542 to ac49da3 Compare January 22, 2024 15:05
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context5 branch 7 times, most recently from f42debe to 8f04db1 Compare January 23, 2024 13:06
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from ac49da3 to 67fee51 Compare January 23, 2024 14:03
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context5 branch from 8f04db1 to 8e5bc5c Compare January 24, 2024 09:22
@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch 3 times, most recently from e0bd69e to e89fb1b Compare January 24, 2024 10:15
Copy link
Collaborator

@dorimedini-starkware dorimedini-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 4 files at r6, 1 of 5 files at r8, 6 of 6 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @aner-starkware)


crates/blockifier/src/utils.rs line 50 at r12 (raw file):

pub fn u128_from_usize(val: usize) -> Result<u128, String> {
    val.try_into().map_err(|_| format!("Error converting value {val} to u128."))
}
  1. I want the error string to be defined where the function is called, not in the function implementation itself, so the error string can be specific (usize_from_u128 doesn't know if it's input is a gas price, a fee, or what).
  2. The return type Result<u128, TryFromIntError> was just fine, no need to add map_err

Code quote:

/// Conversion from usize to u128. Currently, usize has 64 bits, so this conversion should never
/// fail.
pub fn usize_from_u128(val: u128) -> Result<usize, String> {
    val.try_into().map_err(|_| format!("Error converting value {val} to usize."))
}

/// Conversion from u128 to usize. This conversion should only be used if the value came from a
/// usize.
pub fn u128_from_usize(val: usize) -> Result<u128, String> {
    val.try_into().map_err(|_| format!("Error converting value {val} to u128."))
}

crates/blockifier/src/fee/fee_utils.rs line 76 at r11 (raw file):

        gas_usage: total_l1_gas_usage.ceil() as u128,
        blob_gas_usage: u128_from_usize(l1_blob_gas_usage)
            .expect("Conversion from usize to u128 should not fail."),

I want to see l1_blob_gas_usage in the output

Code quote:

.expect("Conversion from usize to u128 should not fail."),

crates/blockifier/src/fee/gas_usage.rs line 36 at r11 (raw file):

            use_kzg_da,
        )?)
        .expect("Conversion from usize to u128 should not fail."),

please print out all four inputs to calculate_tx_gas_usage in the error string

Code quote:

.expect("Conversion from usize to u128 should not fail."),

crates/blockifier/src/fee/gas_usage.rs line 41 at r11 (raw file):

            use_kzg_da,
        ))
        .expect("Conversion from usize to u128 should not fail."),

print out inputs (state changes count and KZG DA flag) in the failure string

Code quote:

.expect("Conversion from usize to u128 should not fail."),

crates/blockifier/src/fee/gas_usage.rs line 45 at r11 (raw file):

}

/// Returns the blob-gas (data-gas) needed to publish the transaction's state diff in a blob.

suggestion

Suggestion:

/// Returns the blob-gas (data-gas) needed to publish the transaction's state diff (if use_kzg_da is
/// false, zero blob is needed).

crates/blockifier/src/fee/gas_usage.rs line 61 at r11 (raw file):

/// the verifier following the addition of a transaction with the given parameters to a batch; e.g.,
/// a message from L2 to L1 is followed by a storage write operation in Starknet L1 contract which
/// requires gas.

close paren

Suggestion:

/// Returns an estimation of the L1 gas amount that will be used (by Starknet's state update and
/// the verifier following the addition of a transaction with the given parameters to a batch; e.g.,
/// a message from L2 to L1 is followed by a storage write operation in Starknet L1 contract which
/// requires gas).

crates/blockifier/src/fee/gas_usage_test.rs line 117 at r11 (raw file):

        u128_from_usize(manual_sharp_blob_gas_usage).unwrap(),
        deploy_account_blob_gas_usage
    );

suggestion (you may need to add some derives to GasAndBlobGasUsages)

Suggestion:

    assert_eq!(
        GasAndBlobGasUsages {
            gas_usage: u128_from_usize(manual_starknet_gas_usage + manual_sharp_gas_usage).unwrap(),
            blob_gas_usage: u128_from_usize(manual_sharp_blob_gas_usage).unwrap(),
        },
        deploy_account_gas_and_blob_gas_usage
    );

crates/blockifier/src/fee/gas_usage_test.rs line 132 at r11 (raw file):

        gas_usage: l1_handler_gas_usage,
        blob_gas_usage: l1_handler_blob_gas_usage,
    } = l1_handler_gas_and_blob_gas_usage;

if you only need the destructuring for the assert_eq!s below, consider comparing GasAndBlobGasUsages object instances directly (see above comment).

Code quote:

    let GasAndBlobGasUsages {
        gas_usage: l1_handler_gas_usage,
        blob_gas_usage: l1_handler_blob_gas_usage,
    } = l1_handler_gas_and_blob_gas_usage;

crates/blockifier/src/fee/gas_usage_test.rs line 195 at r11 (raw file):

        gas_usage: l2_to_l1_messages_gas_usage,
        blob_gas_usage: l2_to_l1_messages_blob_gas_usage,
    } = l2_to_l1_messages_gas_and_blob_gas_usage;

consider object comparison (see above)

Code quote:

    let GasAndBlobGasUsages {
        gas_usage: l2_to_l1_messages_gas_usage,
        blob_gas_usage: l2_to_l1_messages_blob_gas_usage,
    } = l2_to_l1_messages_gas_and_blob_gas_usage;

crates/blockifier/src/fee/gas_usage_test.rs line 208 at r11 (raw file):

            true => 0,
            false => get_onchain_data_cost(l2_to_l1_state_changes_count),
        };

will this be less lines...? non-blocking

Suggestion:

        + if use_kzg_da { 0 } else { get_onchain_data_cost(l2_to_l1_state_changes_count) };

crates/blockifier/src/fee/gas_usage_test.rs line 241 at r11 (raw file):

        gas_usage: storage_writings_gas_usage,
        blob_gas_usage: storage_writings_blob_gas_usage,
    } = storage_writings_gas_and_blob_gas_usage;

consider manual object comparison (see above)

Code quote:

    let GasAndBlobGasUsages {
        gas_usage: storage_writings_gas_usage,
        blob_gas_usage: storage_writings_blob_gas_usage,
    } = storage_writings_gas_and_blob_gas_usage;

crates/blockifier/src/fee/gas_usage_test.rs line 282 at r11 (raw file):

            eth_gas_constants::GAS_PER_MEMORY_WORD - eth_gas_constants::get_calldata_word_cost(12)
        }
    };

consider this (if formatter makes it nicer than how it currently is)

Suggestion:

    let fee_balance_discount = if use_kzg_da {
        0
    } else {
        eth_gas_constants::GAS_PER_MEMORY_WORD - eth_gas_constants::get_calldata_word_cost(12)
    };

crates/blockifier/src/transaction/transaction_utils.rs line 28 at r11 (raw file):

) -> TransactionExecutionResult<ResourcesMapping> {
    let l1_gas_usage = usize_from_u128(l1_gas_usages.gas_usage)
        .expect("This conversion should not fail as the value is a converted usize.");

print out the gas usage

Code quote:

.expect("This conversion should not fail as the value is a converted usize.");

crates/blockifier/src/transaction/transaction_utils.rs line 30 at r11 (raw file):

        .expect("This conversion should not fail as the value is a converted usize.");
    let l1_blob_gas_usage = usize_from_u128(l1_gas_usages.blob_gas_usage)
        .expect("This conversion should not fail as the value is a converted usize.");

print out the blob gas usage

Code quote:

.expect("This conversion should not fail as the value is a converted usize.")

crates/blockifier/src/transaction/transactions_test.rs line 296 at r11 (raw file):

}

#[rstest]

thanks for this refactor... I hate test_case


crates/blockifier/src/transaction/transactions_test.rs line 1093 at r11 (raw file):

    #[values(false, true)] use_kzg_da: bool,
) {
    let block_context = &&BlockContext::create_for_account_testing_with_kzg(use_kzg_da);

why extra &?

Code quote:

&

crates/blockifier/src/transaction/transactions_test.rs line 1205 at r11 (raw file):

    #[values(false, true)] use_kzg_da: bool,
) {
    let block_context = &&BlockContext::create_for_account_testing_with_kzg(use_kzg_da);

why extra &?

Code quote:

&

crates/blockifier/src/transaction/transactions_test.rs line 1491 at r11 (raw file):

    let account_cairo_version = CairoVersion::Cairo0;
    let test_contract_cairo_version = CairoVersion::Cairo0;
    let block_context = &&BlockContext::create_for_account_testing_with_kzg(use_kzg_da);

why extra &?

Code quote:

&

crates/blockifier/src/transaction/transactions_test.rs line 1530 at r11 (raw file):

        u128_from_usize(tx_execution_info.actual_resources.blob_gas_usage()).unwrap(),
        l1_blob_gas_usage
    );

consider using object comparison instead of destructuring (see above)

Code quote:

    let GasAndBlobGasUsages { gas_usage: l1_gas_usage, blob_gas_usage: l1_blob_gas_usage } =
        l1_gas_and_blob_gas_usage;
    assert_eq!(
        u128_from_usize(tx_execution_info.actual_resources.gas_usage()).unwrap(),
        l1_gas_usage
    );
    assert_eq!(
        u128_from_usize(tx_execution_info.actual_resources.blob_gas_usage()).unwrap(),
        l1_blob_gas_usage
    );

crates/blockifier/src/transaction/transactions_test.rs line 1581 at r11 (raw file):

        l1_blob_gas_usage
    );
}

consider object comparison

Code quote:

    let GasAndBlobGasUsages { gas_usage: l1_gas_usage, blob_gas_usage: l1_blob_gas_usage } =
        l1_gas_and_blob_gas_usage;
    assert_eq!(
        u128_from_usize(tx_execution_info.actual_resources.gas_usage()).unwrap(),
        l1_gas_usage
    );
    assert_eq!(
        u128_from_usize(tx_execution_info.actual_resources.blob_gas_usage()).unwrap(),
        l1_blob_gas_usage
    );
}

crates/blockifier/src/transaction/transactions_test.rs line 1700 at r11 (raw file):

fn test_l1_handler(#[values(false, true)] use_kzg_da: bool) {
    let state = &mut create_test_state();
    let block_context = &&BlockContext::create_for_account_testing_with_kzg(use_kzg_da);

why extra &?

Code quote:

&

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from cec3224 to 210542f Compare January 29, 2024 08:50
@aner-starkware
Copy link
Contributor Author

crates/blockifier/src/utils.rs line 50 at r12 (raw file):

Previously, dorimedini-starkware wrote…
  1. I want the error string to be defined where the function is called, not in the function implementation itself, so the error string can be specific (usize_from_u128 doesn't know if it's input is a gas price, a fee, or what).
  2. The return type Result<u128, TryFromIntError> was just fine, no need to add map_err

OK, but then you don't have the value (it's not part of the error).

@aner-starkware
Copy link
Contributor Author

crates/blockifier/src/fee/fee_utils.rs line 76 at r11 (raw file):

Previously, dorimedini-starkware wrote…

I want to see l1_blob_gas_usage in the output

It's shown because the error contains the value. I think it's better to put a TODO here until you figure out exactly what it is we want.

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 12 of 12 files at r13, all commit messages.
Reviewable status: all files reviewed, 24 unresolved discussions (waiting on @aner-starkware)

@ArniStarkware
Copy link
Contributor

crates/blockifier/src/transaction/transaction_utils.rs line 30 at r13 (raw file):

        .expect("This conversion should not fail as the value is a converted usize.");
    let l1_blob_gas_usage = usize_from_u128(l1_gas_usages.blob_gas)
        .expect("This conversion should not fail as the value is a converted usize.");

@dorimedini-starkware Why not?
What am I missing?

Suggestion:

    let l1_blob_gas_usage = l1_gas_usages.blob_gas.try_into()
        .expect("This conversion should not fail as the value is a converted usize.");

Copy link
Collaborator

@dorimedini-starkware dorimedini-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: all files reviewed, 20 unresolved discussions (waiting on @aner-starkware and @ArniStarkware)


crates/blockifier/src/utils.rs line 50 at r12 (raw file):

Previously, aner-starkware wrote…

OK, but then you don't have the value (it's not part of the error).

good point. too bad TryFromIntError doesn't include it.
however, String is not really a good option (not an Error enum); how about a NumericConversionError?


crates/blockifier/src/fee/fee_utils.rs line 76 at r11 (raw file):

Previously, aner-starkware wrote…

It's shown because the error contains the value. I think it's better to put a TODO here until you figure out exactly what it is we want.

right, this is fine how it is :) no need for TODO


crates/blockifier/src/fee/gas_usage.rs line 36 at r11 (raw file):

Previously, dorimedini-starkware wrote…

please print out all four inputs to calculate_tx_gas_usage in the error string

nvm


crates/blockifier/src/fee/gas_usage.rs line 41 at r11 (raw file):

Previously, dorimedini-starkware wrote…

print out inputs (state changes count and KZG DA flag) in the failure string

nvm


crates/blockifier/src/transaction/transaction_utils.rs line 28 at r11 (raw file):

Previously, dorimedini-starkware wrote…

print out the gas usage

nvm


crates/blockifier/src/transaction/transaction_utils.rs line 30 at r11 (raw file):

Previously, dorimedini-starkware wrote…

print out the blob gas usage

nvm


crates/blockifier/src/transaction/transaction_utils.rs line 30 at r13 (raw file):

Previously, ArniStarkware (Arnon Hod) wrote…

@dorimedini-starkware Why not?
What am I missing?

in case the type of blob_gas changes in the future, and the behavior of try_into will silently change. using usize_from_u128 ensures we get compile-time errors if the underlying type changes

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from 210542f to e9e2e0a Compare January 29, 2024 11:28
Copy link
Contributor Author

@aner-starkware aner-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: 13 of 15 files reviewed, 19 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/utils.rs line 50 at r12 (raw file):

Previously, dorimedini-starkware wrote…

good point. too bad TryFromIntError doesn't include it.
however, String is not really a good option (not an Error enum); how about a NumericConversionError?

Done.


crates/blockifier/src/fee/fee_utils.rs line 95 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Sorry, I meant this:

pub struct GasVector { 
        pub l1_gas: u128, 
        pub blob_gas: u128, 
        // (In the future:) 
        pub l2_gas: u128, 
}

Done.


crates/blockifier/src/fee/gas_usage.rs line 34 at r9 (raw file):

Previously, dorimedini-starkware wrote…

function should return result, use unwrap_or_else(|_| panic!(<MESSAGE>)) where the message is informative

OK. Used expect.


crates/blockifier/src/fee/gas_usage.rs line 60 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

.

Done.


crates/blockifier/src/fee/gas_usage.rs line 66 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

.

Done.


crates/blockifier/src/fee/gas_usage.rs line 67 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

.

Done.


crates/blockifier/src/fee/gas_usage.rs line 103 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Ignore the Ignore

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1093 at r11 (raw file):

Previously, dorimedini-starkware wrote…

why extra &?

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1205 at r11 (raw file):

Previously, dorimedini-starkware wrote…

why extra &?

Done.


crates/blockifier/src/transaction/transactions_test.rs line 1700 at r11 (raw file):

Previously, dorimedini-starkware wrote…

why extra &?

Done.

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from e9e2e0a to d5c3452 Compare January 29, 2024 11:32
Copy link
Contributor Author

@aner-starkware aner-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: 12 of 15 files reviewed, 19 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/fee/gas_usage.rs line 59 at r10 (raw file):

/// the verifier following the addition of a transaction with the given parameters to a batch; e.g.,
/// a message from L2 to L1 is followed by a storage write operation in Starknet L1 contract which
/// requires gas.

Done.


crates/blockifier/src/fee/gas_usage.rs line 61 at r11 (raw file):

Previously, dorimedini-starkware wrote…

close paren

Done.

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 2 of 2 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @aner-starkware and @Yoni-Starkware)


crates/blockifier/src/fee/gas_usage.rs line 36 at r15 (raw file):

            use_kzg_da,
        )?)
        .expect("Conversion from usize to u128 should not fail."),

Suggestion:

"Result of calculate_tx_l1_gas_usage failed conversion to u128."

crates/blockifier/src/fee/gas_usage.rs line 38 at r15 (raw file):

        .expect("Conversion from usize to u128 should not fail."),
        blob_gas: u128_from_usize(calculate_tx_blob_gas_usage(state_changes_count, use_kzg_da))
            .expect("Conversion from usize to u128 should not fail."),

Suggestion:

"Result of calculate_tx_blob_gas_usage failed conversion to u128."

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from d5c3452 to 781f8f7 Compare January 29, 2024 11:50
Copy link
Contributor Author

@aner-starkware aner-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: all files reviewed, 19 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/fee/gas_usage.rs line 77 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

.

Done.


crates/blockifier/src/fee/gas_usage.rs line 36 at r15 (raw file):

            use_kzg_da,
        )?)
        .expect("Conversion from usize to u128 should not fail."),

Done. Although I personally disagree here - I think the message should explain why we expect it to pass, not to inform what failed.


crates/blockifier/src/fee/gas_usage.rs line 38 at r15 (raw file):

        .expect("Conversion from usize to u128 should not fail."),
        blob_gas: u128_from_usize(calculate_tx_blob_gas_usage(state_changes_count, use_kzg_da))
            .expect("Conversion from usize to u128 should not fail."),

Done. Same comment as above.


crates/blockifier/src/fee/gas_usage_test.rs line 117 at r11 (raw file):

Previously, dorimedini-starkware wrote…

suggestion (you may need to add some derives to GasAndBlobGasUsages)

Added TODO.


crates/blockifier/src/fee/gas_usage_test.rs line 132 at r11 (raw file):

Previously, dorimedini-starkware wrote…

if you only need the destructuring for the assert_eq!s below, consider comparing GasAndBlobGasUsages object instances directly (see above comment).

Added TODO.


crates/blockifier/src/fee/gas_usage_test.rs line 195 at r11 (raw file):

Previously, dorimedini-starkware wrote…

consider object comparison (see above)

Added TODO


crates/blockifier/src/fee/gas_usage_test.rs line 208 at r11 (raw file):

Previously, dorimedini-starkware wrote…

will this be less lines...? non-blocking

Added TODO


crates/blockifier/src/fee/gas_usage_test.rs line 241 at r11 (raw file):

Previously, dorimedini-starkware wrote…

consider manual object comparison (see above)

Added TODO


crates/blockifier/src/fee/gas_usage_test.rs line 282 at r11 (raw file):

Previously, dorimedini-starkware wrote…

consider this (if formatter makes it nicer than how it currently is)

Added TODO


crates/blockifier/src/transaction/transactions_test.rs line 1530 at r11 (raw file):

Previously, dorimedini-starkware wrote…

consider using object comparison instead of destructuring (see above)

Added TODO


crates/blockifier/src/transaction/transactions_test.rs line 1581 at r11 (raw file):

Previously, dorimedini-starkware wrote…

consider object comparison

Added TODO

Copy link
Collaborator

@dorimedini-starkware dorimedini-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 2 of 2 files at r16, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @aner-starkware and @Yoni-Starkware)


crates/blockifier/src/fee/gas_usage.rs line 36 at r15 (raw file):

Previously, aner-starkware wrote…

Done. Although I personally disagree here - I think the message should explain why we expect it to pass, not to inform what failed.

Sure, I didn't mean to insist on grammar, just wanted calculate_tx_l1_gas_usage to appear in the string.
Same as below

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: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware)


crates/blockifier/src/transaction/account_transaction.rs line 181 at r16 (raw file):

    ) -> TransactionPreValidationResult<()> {
        // TODO(Aner, 21/01/24) modify for 4844 (blob_gas).
        let minimal_l1_gas_and_blob_gas_amount = estimate_minimal_l1_gas(block_context, self)?;

Non-blocking for this PR

Suggestion:

let minimal_gas_vector = estimate_minimal_gas_vector(block_context, self)?;

crates/blockifier/src/transaction/account_transaction.rs line 215 at r16 (raw file):

            AccountTransactionContext::Deprecated(context) => {
                let max_fee = context.max_fee;
                let min_fee = get_fee_by_l1_gas_usages(

Non-blocking for this PR

Suggestion:

get_fee_by_gas_vector

@aner-starkware
Copy link
Contributor Author

crates/blockifier/src/transaction/account_transaction.rs line 181 at r16 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Non-blocking for this PR

OK. There's a TODO, and we should verify that there isn't any logic that should be changed as well here (specifically, it seems that the function estimate\_minimal\_l1\_gas should be modified for 4844)

@aner-starkware
Copy link
Contributor Author

crates/blockifier/src/transaction/account_transaction.rs line 215 at r16 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Non-blocking for this PR

But it's a blocking comment :)

@aner-starkware aner-starkware force-pushed the aa/data-gas-prices-in-block-context6 branch from 781f8f7 to 7ae9f72 Compare January 29, 2024 13:53
Copy link
Collaborator

@dorimedini-starkware dorimedini-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 7 of 7 files at r17, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aner-starkware and @Yoni-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.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware)

@aner-starkware aner-starkware marked this pull request as ready for review January 29, 2024 15:40
Copy link
Contributor Author

@aner-starkware aner-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: all files reviewed, 1 unresolved discussion

a discussion (no related file):
Let me know if there's anything else


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 r13, 3 of 7 files at r17.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-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.

Reviewed 1 of 12 files at r13, 1 of 7 files at r17.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware)


crates/blockifier/src/transaction/transaction_utils.rs line 23 at r17 (raw file):

pub fn calculate_tx_resources(
    execution_resources: &ExecutionResources,
    l1_gas_usages: GasVector,

In the next PR, please rename things like this to gas_vector (also in func names)

Code quote:

l1_gas_usages

Copy link
Collaborator

@dorimedini-starkware dorimedini-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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware)

@aner-starkware aner-starkware merged commit 2e445ae into main-v0.13.1 Jan 30, 2024
8 checks passed
@aner-starkware aner-starkware deleted the aa/data-gas-prices-in-block-context6 branch January 30, 2024 07:54
rodrigo-pino added a commit to NethermindEth/blockifier that referenced this pull request Mar 7, 2024
* Run cargo update

Signed-off-by: Dori Medini <[email protected]>

* Detach native_blockifier

* Bump cairo-vm and cairo compiler

New cairo-vm version includes performance optimizations.
New cairo version only includes the new vm version.

* Release 0.4.1-rc.0

* Re-attach native_blockifier, bump papyrus_storage

* native_blockifier: Allow querying for `Blockifier` version. (starkware-libs#1249)

This allows one to do:
```
import native_blockifier
native_blockifier.blockifier_version()
```
And get the version the native was compiled with.

Co-Authored-By: Gilad Chase <[email protected]>

* Add updated version of estimate_casm_hash_computation_resources(). (starkware-libs#1314)

* Resolve conflicts.

* Reconnect nnative-blockifier

Signed-off-by: Dori Medini <[email protected]>

* Add the field kzg_resources to PyBouncerInfo. (starkware-libs#1321)

* Add use_kzg_da flag to PyBlockInfo. (starkware-libs#1319)

* Merge `main-v0.13.0` into `main` (resolve conflicts).

* Remove gas price bounds from general config (starkware-libs#1329)

Signed-off-by: Dori Medini <[email protected]>

* Bump cairo-vm version (starkware-libs#1330)

Signed-off-by: Dori Medini <[email protected]>

* Update cairo compiler version and fix related TODOs. (starkware-libs#1332)

* Small fix in into_block_context. (starkware-libs#1337)

* Data gas prices in block context (starkware-libs#1336)

Signed-off-by: Dori Medini <[email protected]>

* Use try_from instead of as (starkware-libs#1322)

* Use into instead of as (starkware-libs#1333)

* Add MAX_L1_GAS_AMOUNT_U128 (starkware-libs#1335)

* Refactor calculate_tx_gas_usage (starkware-libs#1320)

* Change additional as to try_from and try_to (starkware-libs#1342)

* Remove PyKzgResources. Reveal the state diff size. (starkware-libs#1341)

* Make `BlockContext` a newtype around `BlockInfo` (starkware-libs#1323)

The crux of this commit is the change in `block_context.rs`, which
converts `BlockContext` into a wrapper around `BlockInfo` (which used to be
`BlockContext`).

Everything else is just delegating the accessors to the internal block_info, made
by search-and-replace (no other changes).

Motivation:
- This change the groundwork for future refactor and consolidation of
contexts.
- Subsequent commits will extract non block-specific constants from
`BlockInfo` into separate `ChainInfo` and `VersionedConstants` structs,
which will be siblings of `block_info` inside `BlockContext`.

Co-Authored-By: Gilad Chase <[email protected]>

* Extract from `BlockInfo` into `ChainInfo` (starkware-libs#1344)

`BlockInfo` should only contain block-level info, whereas chain-level
information should be held at the `BlockContext` level, encapsulated in
a dedicated `ChainInfo` struct.

Changes:
Only extract from `BlockInfo` and store in `BlockContext.chain_info:
ChainInfo`. All other changes are search-replace + moving the
fee_token_address getter into `ChainInfo`.

TODO: extract more stuff from BlockInfo: version-dependant constants should
be extracted into a dedicated VersionedConstants, which will be a member of BlockContext

Co-Authored-By: Gilad Chase <[email protected]>

* Revert changes of 776d7a5 (starkware-libs#1352)

* fix(native_blockifier): Remove `PyGasPrices` (starkware-libs#1353)

`PyX` types are intended to represent (a subset) of the real
structure of object in Python.
Currently Python is broken do to PyBlockInfo's FromPyObject expecting
there to be a `PyGasPrices`, which doesn't exist in Ptyhon

Co-Authored-By: Gilad Chase <[email protected]>

* Rename `{into,to}_actual_cost_builder` (starkware-libs#1355)

`into` implies `owned -> owned` cast, whereas `to` can imply `borrowed
-> owned`.

See this reference for naming convention: https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Co-Authored-By: Gilad Chase <[email protected]>

* add missing internal panic in cairo1 (starkware-libs#1340)

* add missing internal panic in cairo1

* update `test_stack_trace` and make it work with Cairo1 as well as Cairo0

* Align block info fields (starkware-libs#1358)

Signed-off-by: Dori Medini <[email protected]>

* ci: add commitlint in CI (starkware-libs#1313)

Signed-off-by: Dori Medini <[email protected]>

* Create the function calculate_tx_blob_gas_usage. (starkware-libs#1346)

* Add direct cast from PyFelt into ContractAddress + Deref (starkware-libs#1345)

Co-Authored-By: Gilad Chase <[email protected]>

* fix: revert "Add direct cast from PyFelt into ContractAddress + Deref (starkware-libs#1345)" (starkware-libs#1364)

This reverts commit 6b524c9.

Python is fussy about derive_more, will TAL at it again later

Co-Authored-By: Gilad Chase <[email protected]>

* refactor(fee): split tx l1 gas computation to da and non-da sum, add fee test for empty transaction (starkware-libs#1339)

* test: add stack trace regressions tests in Cairo1 for various call chains (starkware-libs#1367)

* feat(fee): preparation-add the calldata length to the resource calcaultion (starkware-libs#1361)

* build(fee): added new struct and field in preparation for using blob_gas (starkware-libs#1356)

* feat(fee): modified calculate_tx_l1_gas_usage(s) added calculate_tx_gas_and_blob_gas_usage (starkware-libs#1357)

* style: remove some unnecessary variables (starkware-libs#1366)

* feat(fee): modified calculate_tx_l1_gas_usage(s), added calculate_tx_gas_and_blob_gas_usage (starkware-libs#1370)

* refactor(state): replace GlobalContractCache default with GlobalContractCache new (starkware-libs#1368)

* feat(fee): check gas amount including discounted data gas (starkware-libs#1374)

Signed-off-by: Dori Medini <[email protected]>

* test: add cairo0 variants to the new stack trace regression tests (starkware-libs#1371)

* feat(fee): calculates messages size field (starkware-libs#1290)

* fix(native_blockifier): change failure_flag type to bool (starkware-libs#1334)

* fix(execution): convert all local errors to stack trace (starkware-libs#1378)

Signed-off-by: Dori Medini <[email protected]>

* fix(execution): change additional as to from (starkware-libs#1373)

* Merge `main-v0.13.1` into `main` (resolve conflicts).

* fix(native_blockifier): use PyCallType and PyEntryPointType instead of as (starkware-libs#1383)

* ci: prevent PR title check on merges

Signed-off-by: Dori Medini <[email protected]>

* ci: ignore merge PRs when linting PR titles (starkware-libs#1388)

Signed-off-by: Dori Medini <[email protected]>

* refactor(native_blockifier): pybouncerinfo tx_weights field (starkware-libs#1347)

* style(fee): fix error handle unpack (starkware-libs#1390)

* feat(fee): add linear combination for gas and blob_gas (starkware-libs#1360)

* refactor: change CustomHint error usage into VirtualMachineError::Other(anyhow::Error) (starkware-libs#1397)

* refactor(state): use get_nonce_updates (starkware-libs#1398)

* refactor: make the BlockContext constructor private as preparation for pre_process_block changes (starkware-libs#1385)

* refactor: change variable and function names for gas_vector (starkware-libs#1400)

* fix(fee): modified estimate_minimal_gas_vector, extracted compute_discounted_gas_from_gas_vector (starkware-libs#1401)

* refactor: modified extraction in extract_l1_blob_gas_usage (from unwrap_or(0) to expect) (starkware-libs#1402)

* test: update test_call_contract to new infra (starkware-libs#1404)

Signed-off-by: Dori Medini <[email protected]>

* feat: add VersionedConstants

- Currently only covers the constants previously contained in `BlockInfo`.
TODO: Add check if this covers all chain ids, which might have different
contstants
- Remove `BlockContextArgs`: no longer needed now that BlockContext can
  be initialized by 3 args of public types.

* test: update test_replace_class to new infra (starkware-libs#1405)

Signed-off-by: Dori Medini <[email protected]>

* refactor: restructure old_block_number_and_hash as a struct (starkware-libs#1403)

* chore(fee): gas computation refactor (starkware-libs#1408)

Changes:
1. Implemented get_da_gas_cost, which returns a GasVector (depending on
   use_kzg_da flag).
2. Deleted calculate_tx_gas_usage_vector, because
   calculate_tx_l1_gas_usage does the same thing.
3. Derived derive_more::Add for GasVector to easily sum up gas costs.

Signed-off-by: Dori Medini <[email protected]>

* chore(fee): use GasVector as a return type for gas computations (starkware-libs#1409)

Signed-off-by: Dori Medini <[email protected]>

* refactor: rename context modules

- Rename block_context.rs -> context.rs. This will hold all context
  types.
- Rename block_execution.rs -> block.rs and move `BlockInfo` and
  `GasPrices` there (`GasPrices` is only used inside `BlockInfo`).

No other changes.

* chore(fee): integer VM gas usage (starkware-libs#1410)

* chore(fee): use GasVector as a return type for gas computations

Signed-off-by: Dori Medini <[email protected]>

* chore(fee): integer VM gas usage

Signed-off-by: Dori Medini <[email protected]>

* perf(native_blockifier): transaction execution info add serialize (starkware-libs#1315)

* feat(fee): adding a slope paramters to the os resources (starkware-libs#1362)

* feat(fee): set the resources slope values (starkware-libs#1363)

* refactor: make `AccountTransactionContext` hold BlockContext

- Rename `AccountTransactionContext` into `TransactionInfo`: i want to
  use `Context` for something else, and `Account` is a misnomer, since
  L1Transactions also generate this instance.
- Create a new `AccountTransactionContext`, which holds `BlockContext`
  and `TransactionInfo`: This mirrors `BlockContext`, which holds
  `BlockInfo` as well as higher level contexts.
- Remove all unnecessary `get_account_tx` calls.
- Replace all functions that take both `block_context` and `tx_info` with
a single `TransactionContext`.
- Make `EntryPointExecutionContext` hold an `Arc<TransactionContext>`
  instead of both a block_context and `tx_info`: previously every entry
  point cloned the blockContext and generated a new tx_info, now they
  all share the same (identical) one.

* feat: add more constants to`VersionedConstants`

- Add gateway constants, these are only for transparency, and aren't
  used directly by the Blockifier whatsoever.
- Pass `validate_max_n_steps` into the blockifier via native_blockifier,
  to override versioned constants defaults.
- Sort json

* fix: remove dead code `block_context.rs` (starkware-libs#1423)

The module has already taken out of lib.rs in
`8ba2662f93999b526ef7fd0a7fc49d1314657184` (making this module
dead-code), but the file wasn't actually deleted there.

Co-Authored-By: Gilad Chase <[email protected]>

* fix: delete dead code block_execution.rs (starkware-libs#1425)

Was already removed from lib.rs.

Co-Authored-By: Gilad Chase <[email protected]>

* chore(execution): enforce nonzero gas prices (starkware-libs#1391)

Signed-off-by: Dori Medini <[email protected]>

* refactor(execution)!: pre_process_block refactor for full_nodes simulate transaction (starkware-libs#1387)

* refactor(native_blockifier): flat fields in bouncerinfo (starkware-libs#1394)

* feat: add Starknet OS constants to `VersionedConstants` - unused

1. Currently not using the new consts, just adding
the deserialization logic and tests.
In upcoming commits this will replace the gas costs in `constants.rs`.

The idea is that values inside `cairo_constants` that are json objects,
that is, have the form:
```json
"step_gas_cost": {
  foo_cost: x,
  bar_cost: y,
  ...
}
```
should be parsed as:
```
step_gas_cost = foo_cost*x + bar_cost*y + ...,
```
where {x,y} must be unsigned integers (otherwise an error is thrown),
and where {foo_cost, bar_cost,...} must correspond to keys that
have already* been parsed in the JSON.

Note: JSON standard doesn't enforce order on the keys, but our
implementation does; we should consider using a format that ensures
order, rather than relying on serde's implementation and `IndexMap`.

2. validate the os costs on creation (except for in tests): under
this assumption `get_gas_cost` will _panic_ if given a non-whitelisted
gas cost name.

3. hide the two hashmaps inside `VersionedConstants`, they have
invariants, set accessors accordingly.

* chore(execution): saturate step bound on overflow (starkware-libs#1429)

Signed-off-by: Dori Medini <[email protected]>

* feat(fee): add signature length to ActualCostBuilder (starkware-libs#1431)

* feat(execution): use gas costs only from `VersionedConstants`

- Remove from constants module and replace all usages with
  `VersionedConstants#gas_cost(..)`
  - Move all comments from the constants module into the const whitelist
    in `VersionedConstants`.
- Add gas cost getter to `EntryPointExecutionContext`, for readability
  - enclose in closure inside hint_processor.rs for even more brevity.
- Move `Transaction::Initial_gas` into `VersionedConstants`: it is now
  derived from the constants json.
- No other logic changes.

* feat(fee): add os resources to versioned constants

- Move hardcoded os resources json into the versioned constants json,
  move `OsResources` into into `OsResources` module.
- Indent versioned_constants.json at 4.
- Delete os_usage.rs: all logic is now called from methods in
  `VersionedConstants`(todo: extract into submodules, currently
  just a big module).
- Delete os_usage_test.rs: these are not post-deserialize validations of
  `OsResources`.
- sorted versioned_constants (where possible, which is everywhere except
  for inside os_constants keys).

* chore(execution): fix the tests using create_state_changes_for_test for readability and correctness (starkware-libs#1430)

* refactor(execution, native_blockifier): make TransactionExecutor.execute() work w/o Py objects (starkware-libs#1414)

* refactor(execution, native_blockifier): make TransactionExecutor.finalize() work without Py objects (starkware-libs#1418)

* refactor(execution, native_blockifier): decouple BouncerInfo from Python (starkware-libs#1428)

* refactor: split InnerCallExecutionError into CallContractExecutionError & LibraryCallExecutionError.
Add storage_address to both new types and class_hash to LibraryCallExecutionError (starkware-libs#1432)

* feat(execution): moved global max step limit to input validation (starkware-libs#1415)

Signed-off-by: Dori Medini <[email protected]>

* test(execution): improve covrage of test_count_actual_storage_changes (starkware-libs#1436)

* refactor(execution): the struct state changes and state changes count (starkware-libs#1417)

* chore(fee): update os resources and fix expected

* fix(native_blockifier): fix unused imports (starkware-libs#1442)

* chore(execution): consume state_changes on state_changes_count from (starkware-libs#1443)

* feat(execution): calculate the syscall resources for every entry point (starkware-libs#1437)

* feat(execution): add entry_point_syscall_counter (starkware-libs#1407)

* refactor(execution): split get_additional_os_resources (starkware-libs#1411)

* feat(execution): add the syscall resources to vm_resources (starkware-libs#1412)

* fix(fee): fix doc of usize u128 conversions (starkware-libs#1446)

* fix: memory holes tests (starkware-libs#1451)

Co-Authored-By: Gilad Chase <[email protected]>

* fix: remove unused dep ctor (starkware-libs#1455)

Co-Authored-By: Gilad Chase <[email protected]>

* refactor(execution): replace StateChangesCount::from with state_changes.count() (starkware-libs#1452)

* refactor(transaction): make DeclareTransaction fields public (starkware-libs#1441)

* refactor: remove enum VirtualMachineExecutionError (starkware-libs#1453)

* fix(native_blockifier): fix build for mac + unused import (starkware-libs#1406)

* feat(fee): add ClassInfo struct (starkware-libs#1434)

* Merge `main-v0.13.1` into `main` (resolve conflicts).

* feat(fee): add GasVector to TransactionExecutionInfo (starkware-libs#1399)

* feat(fee): add DA GasVector to TransactionExecutionInfo

Signed-off-by: Dori Medini <[email protected]>

* chore(fee): rename blob_gas to l1_data_gas

Signed-off-by: Dori Medini <[email protected]>

* chore(fee): delete unused PyGasVector

Signed-off-by: Dori Medini <[email protected]>

* refactor(execution): remove syscall_counter from ExecutionResources (starkware-libs#1424)

* Update syscall resources (starkware-libs#1459)

* fix: os resources (starkware-libs#1465)

Co-Authored-By: Gilad Chase <[email protected]>

* feat(state): bouncer DA count: define StateWriteKeys (starkware-libs#1461)

* chore: alphabetize top level keys in versioned constants (starkware-libs#1466)

Co-authored-by: Gilad Chase <[email protected]>

* feat(state): bouncer DA count: use StateWriteKeys to count state diff size (starkware-libs#1462)

* refactor(execution, native_blockifier): move Bouncer to blockifier crate (starkware-libs#1445)

* refactor(transaction): use ClassInfo in Declare transaction (starkware-libs#1456)

* feat(fee): add gas cost for calldata bytes (starkware-libs#1426)

* chore: bump CI versions (starkware-libs#1473)

- Remove the deprecated `actions-rs/toolchain@v1` wherever it is still
  used.
- Bump everything to latest versions.

Co-Authored-By: Gilad Chase <[email protected]>

* refactor(execution): delete enrty point ExecutionResources (starkware-libs#1447)

* fix: limit the syscall emit_event resources, keys, data and number of events (starkware-libs#1463)

* fix(transaction): set l1 hanlder payload size to None for non-l1 hanlder txs (bouncer count) (starkware-libs#1470)

* feat(fee): charge for signatures per byte (starkware-libs#1474)

* Merge `main-v0.13.0-hotfix` into `main-v0.13.1` (resolve conflicts).

* feat(execution): round block_number and timestamp for the execution info syscall in validate_mode (starkware-libs#1467)

* feat(fee): add gas cost for code bytes (starkware-libs#1475)

* feat(fee): add events gas cost (starkware-libs#1458)

* chore(execution): increase invoke_tx_max_n_steps (starkware-libs#1477)

Co-Authored-By: Gilad Chase <[email protected]>

* chore(fee): update vm resource and l2 resource costs (starkware-libs#1479)

* chore: upgrade cairo and sn-api versions (starkware-libs#1454)

* chore: upgrade cairo and sn-api versions

* chore: upgrade cairo and sn-api versions (starkware-libs#1460)

* fix: fix cargo.lock - remove cargo update (starkware-libs#1481)

* fix: undo cargo update from previous commit 984431a

* fix: update cargo.lock to take into account changes in 984431a

they wered reverted in previous commit.

Co-Authored-By: Gilad Chase <[email protected]>

* fix: fix versioned_constants.json

* chore(fee): small fixes (starkware-libs#1484)

* refactor(fee): moving calculate_tx_gas_usage_vector to ActualCostBuilder (starkware-libs#1478)

* Release v0.5.0-rc.1

* refactor(execution): remove fee weights from config (starkware-libs#1487)

* fix: update the event limit constants (starkware-libs#1483)

* Merge `main-v0.13.0-hotfix` into `main-v0.13.1` (resolve conflicts).

* refactor(execution, native_blockifier): move TransactionExecutor to blockifier (starkware-libs#1497)

* refactor(state): remove unnecessary derive (starkware-libs#1505)

* refactor(execution): move bouncer, block and block_test to blockifier dir (starkware-libs#1502)

* feat(fee): add n_events field to BouncerInfo (starkware-libs#1489)

* refactor: move the event size limit constants into versioned_constants (starkware-libs#1490)

* refactor(transaction): remove public fields, add new function (starkware-libs#1480)

* refactor(execution): type of additional_data in test_validate_accounts_tx (starkware-libs#1485)

* refactor: move validate_block_number_rounding and validate_timestamp_rounding to versioned_constants (starkware-libs#1506)

* test(execution): add get_execution_info scenario to test_validate_accounts_tx (starkware-libs#1486)

* test(execution): get_sequencer_address in test_validate_accounts_tx (starkware-libs#1496)

* refactor: use versioned_constants function when possible (starkware-libs#1508)

* test(execution): get_block_number and get_block_timestamp in test_validate_accounts_tx for Cairo0 (starkware-libs#1512)

* chore(fee): Update os resources to include 4844 calculations (starkware-libs#1498)

* refactor: replaced default_invoke_tx_args with macro that enforces to explicitly define max_fee (starkware-libs#1510)

* feat(fee): calculate n_events field for BouncerInfo in a naive way (starkware-libs#1499)

* fix: state trait does not require &mut self (starkware-libs#1325)

* fix: state trait does not require &mut self

fix: sachedState use interior mutability

* chore: remove clippy allow

* fix: small review fix

* fix: remove to_state_diff from StateApi (starkware-libs#1326)

* chore: merge branch main-v0.13.1 into main (resolve conflicts)

* feat: add merge_branches script (starkware-libs#1513)

* chore: bump compiler version to 2.6.0-rc.1 (starkware-libs#1525)

Signed-off-by: Dori Medini <[email protected]>

* chore: move validate consts into os constants (starkware-libs#1523)

* chore: move validate consts into os constants

Had to bump serde_json due to some apparent bug with the recent stable
rust.

* feat: make 13_1 consts backwards compatible

Assign the following defaults for keys missing in versioned_constants
files (for example, they are missing in versioned_constants_13_0.json).

- EventSizeLimit => max values
- L2ResourceGasCosts => 0 values
- validate rounding numbers => 1 (to preserve past no-rounding
  behavior). This required bundling them up in a specialized struct in
  order to define a custom default as 1 (rather than 0).
- Add test for default values: required extracting validation logic of
  `OsResources` so it won't trigger automatically in tests.

Co-Authored-By: Gilad Chase <[email protected]>

* chore!: limit pointer width in both crates (starkware-libs#1527)

Signed-off-by: Dori Medini <[email protected]>

* feat: backwards compatibility for calldata_factor

If os resources' inner tx is missing `constant` and `calldata_factor`
keys, then:
- assume it is a flat `ExecutionResources` instance, and put its
  contents inside a `constant` key.
- initialize calldata_factor as default (all zeros).

* feat: add versioned_constants_13_0.json (starkware-libs#1520)

Changes between this and the current versioned_constants:
- no `event_size_limit`
- invoke_tx_max_n_steps is 3_000_000 instead of 4_000_000
- no `l2_resource_gas_costs`
- multiple value changes in os_resources
- `vm_resource_fee_cost` is X2 for all values
- no constants for validate's block number and timestamp rounding

No other changes (in particular, os constants is indeed _unchanged_)

Co-Authored-By: Gilad Chase <[email protected]>

* Merge `main-v0.13.1` into `main` (resolve conflicts).

* fix: versioned constants validate (starkware-libs#1537)

- validate invocation had a syntax error, which was hidden by the cfg
- remove `testing` from the cfg, since we all compile with `testing`
  during development, and it's just not what we want.

Co-Authored-By: Gilad Chase <[email protected]>

* fix: update the tx event limit constants (starkware-libs#1517)

* chore: release 0.5.0-rc.2: 13_0.json support

* fix: generate_starknet_os_resources python target fix (starkware-libs#1541)

* No conflicts in main-v0.13.1 -> main merge, this commit is for any change needed to pass the CI.

* fix: generate_starknet_os_resources python target fix (starkware-libs#1548)

* refactor: remove some magic numbers from tests (starkware-libs#1547)

* fix!: update max_contract_bytecode_size to 81k (starkware-libs#1549)

* chore: add merge_status.py script (starkware-libs#1543)

Signed-off-by: Dori Medini <[email protected]>

* chore: merge branch main-v0.13.1 into main (resolve conflicts)

* ci: advance setup-python action version (starkware-libs#1555)

* refactor(native_blockifier): rename l1_gas_amount to gas_weight in bouncerInfo (starkware-libs#1524)

* chore: release v0.5.0-rc.3

* chore: panic if u128_from_usize fails (starkware-libs#1522)

Signed-off-by: Dori Medini <[email protected]>

* refactor: make StateError::UndeclaredClassHash a one-line error (starkware-libs#1563)

Change previous error which prints out as:
```
Class with hash ClassHash(
    StarkFelt(
        "0x00000000000000000000000000000000000000000000000000000000000004d2",
    ),
) is not declared.
```
into:
```
Class with hash 0x00000000000000000000000000000000000000000000000000000000000004d2 is not declared.
```

* test(fee): test get_message_segment_length function (starkware-libs#1471)

* refactor: add BlockWeights for the future bouncer (starkware-libs#1444)

* refactor: remove f64 usage at blockifier (starkware-libs#1519)

* fix: unwrap called on u128 (starkware-libs#1570)

Signed-off-by: Dori Medini <[email protected]>

* test: use FeatureContract in create_test_state and tests which uses it (starkware-libs#1556)

* No conflicts in main-v0.13.1 -> main merge, this commit is for any change needed to pass the CI.

* chore: remove final f64s (starkware-libs#1574)

Signed-off-by: Dori Medini <[email protected]>

* chore: remove some 'as' conversions and enforce no 'as' conversions (starkware-libs#1575)

* chore: remove final f64s

Signed-off-by: Dori Medini <[email protected]>

* chore: remove some 'as' conversions and enforce no 'as' conversions

Signed-off-by: Dori Medini <[email protected]>

* Restore workflow

* Rename ci

* Add commit lint rules

* nightly format

* update .env

* Test global env and composite action

* attemp fix to composite action

* Minor fix

* Modify .github structure

* Add composite action to jobs?

* Minor yaml fix

* Fix compilation errors

* WIP

* Storage_read_write_gas

* add tests for vm & native

* modify test_get_execution_info

* Review tests

* rename syscalls_test.rs -> syscall_tests.rs

* Fix minor compilation bug

* Update dependencies

---------

Signed-off-by: Dori Medini <[email protected]>
Co-authored-by: Dori Medini <[email protected]>
Co-authored-by: Gilad Chase <[email protected]>
Co-authored-by: giladchase <[email protected]>
Co-authored-by: Lior Goldberg <[email protected]>
Co-authored-by: liorgold2 <[email protected]>
Co-authored-by: Arnon Hod <[email protected]>
Co-authored-by: Elin Tulchinsky <[email protected]>
Co-authored-by: OriStarkware <[email protected]>
Co-authored-by: Ayelet Zilber <[email protected]>
Co-authored-by: zuphitf <[email protected]>
Co-authored-by: aner-starkware <[email protected]>
Co-authored-by: Noa Oved <[email protected]>
Co-authored-by: Yoni <[email protected]>
Co-authored-by: YaelD <[email protected]>
Co-authored-by: mohammad-starkware <[email protected]>
Co-authored-by: barak-b-starkware <[email protected]>
Co-authored-by: Lucas @ StarkWare <[email protected]>
Co-authored-by: dafnamatsry <[email protected]>
Co-authored-by: avi-starkware <[email protected]>
Co-authored-by: Tzahi <[email protected]>
Co-authored-by: nimrod-starkware <[email protected]>
Co-authored-by: Timothée Delabrouille <[email protected]>
Co-authored-by: Barak <[email protected]>
Co-authored-by: Yonatan Iluz <[email protected]>
Co-authored-by: Rodrigo <[email protected]>
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.

5 participants