feat(blockifier): add struct StateChangesCountForFee

[yoavGrs]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yoavGrs)

---

crates/blockifier/src/transaction/transactions_test.rs line 0 at r1 (raw file):
same as above: does it make sense to have nonzero values for n_allocations in this test?

---

crates/blockifier/src/state/cached_state.rs line 731 at r1 (raw file):

                n_modified_contracts: modified_contracts.len(),
            },
            n_allocated_keys: if enable_stateful_compression {

hmmm... maybe this flag is a bit confusing...
I would expect enable_stateful_compression to indicate whether or not contract 0x2 appears in the state diff,
not whether or not we charge for allocations (which is what is happening here).
should we have two separate flags for this? @Yoni-Starkware

Code quote:

n_allocated_keys: if enable_stateful_compression {

---

crates/blockifier/src/state/cached_state.rs line 755 at r1 (raw file):

pub struct StateChangesCountForFee {
    pub state_changes_count: StateChangesCount,
    pub n_allocated_keys: usize,

just to be clear: this field will be computed, regardless of the enable_stateful_compression flag, correct?

Code quote:

 pub n_allocated_keys: usize,

---

crates/blockifier/src/fee/resources.rs line 153 at r1 (raw file):

        sender_address: Option<ContractAddress>,
        fee_token_address: ContractAddress,
        enable_stateful_compression: bool,

consider this rename, everywhere (see below - maybe a separate flag for stateful compression?)

Suggestion:

charge_for_allocations: bool,

---

crates/blockifier/src/fee/resources.rs line 181 at r1 (raw file):

        // TODO(Nimrod, 29/3/2024): delete `get_da_gas_cost` and move it's logic here.
        // TODO(Yoav): Add the cost of allocating keys.
        get_da_gas_cost(&self.state_changes_for_fee.state_changes_count, use_kzg_da)

did we decide the allocation should be part of DA gas?
we show DA gas separately in the receipt, we want to charge for allocation but not necessarily make it show up as part of the DA gas vector
@Yoni-Starkware WDYT?

Code quote:

        // TODO(Yoav): Add the cost of allocating keys.
        get_da_gas_cost(&self.state_changes_for_fee.state_changes_count, use_kzg_da)

---

crates/blockifier/src/fee/receipt_test.rs line 0 at r1 (raw file):
any coverage benefit from making some of these values non-zero?
if so, please make some non-zero
(if tests fail: why?)

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1385 at r1 (raw file):

            block_context.versioned_constants.enable_stateful_compression,
        )
        .state_changes_count;

why take this field, and not compare the entire object?

X3 (same Q twice below)

Code quote:

.state_changes_count

---

crates/blockifier/src/state/cached_state_test.rs line 351 at r1 (raw file):

            n_modified_contracts: 2,
        },
        n_allocated_keys: 0,

can you update create_state_changes_for_test to create some allocations and use some nonzero X here?

Suggestion:

n_allocated_keys: if enable_stateful_compression { X } else { 0 },

[yoavGrs]

• fix(blockifier): merge state diff with squash #2310 : 2 dependent PRs (#2456 , #2471 )
• feat(blockifier): add comprehensive state diff versioned constant #2407
• feat(blockifier): compute allocation cost #2301
• feat(starknet_api): checked mul for gas vector #2300
• feat(blockifier): two allocation_cost gas vectors #2293
• feat(blockifier): implement the allocated keys logic #2150
• feat(blockifier): add struct StateChangesCountForFee #2175 👈 (View in Graphite)
• feat(blockifier): add n_allocated_keys #2149
• feat(blockifier): add allocation cost versioned constant #2200
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[yoavGrs]

Reviewable status: 5 of 8 files reviewed, 9 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/fee/receipt_test.rs line at r1 (raw file):

Previously, dorimedini-starkware wrote…

any coverage benefit from making some of these values non-zero?
if so, please make some non-zero
(if tests fail: why?)

Done, where it makes sense.
Some tests will fail when we include the allocations in the gas computation.

---

crates/blockifier/src/state/cached_state.rs line 755 at r1 (raw file):

Previously, dorimedini-starkware wrote…

just to be clear: this field will be computed, regardless of the enable_stateful_compression flag, correct?

No.
Under a ForFee struct, this field will be 0 if not enable_stateful_compression.

---

crates/blockifier/src/state/cached_state_test.rs line 351 at r1 (raw file):

Previously, dorimedini-starkware wrote…

can you update create_state_changes_for_test to create some allocations and use some nonzero X here?

It will create allocations when we implement the allocation logic. Look at this test here:
https://reviewable.io/reviews/starkware-libs/sequencer/2150

---

crates/blockifier/src/transaction/account_transactions_test.rs line 1385 at r1 (raw file):

Previously, dorimedini-starkware wrote…

why take this field, and not compare the entire object?

X3 (same Q twice below)

Changing it.
I'll set the correct number of allocations when we implement the allocation logic (2150).

---

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

Previously, dorimedini-starkware wrote…

same as above: does it make sense to have nonzero values for n_allocations in this test?

No allocations in invoke return_result / declare tx.
This is the expected value.

---

crates/blockifier/src/state/cached_state.rs line 732 at r1 (raw file):

            },
            n_allocated_keys: if enable_stateful_compression {
                self.allocated_keys.count()

Sorry, moving this to the next PR.

Code quote:

self.allocated_keys.count()

[github-actions]

Artifacts upload triggered. View details here

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.70%. Comparing base (e3165c4) to head (99c2be9).
Report is 588 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2175       +/-   ##
===========================================
+ Coverage   40.10%   68.70%   +28.60%     
===========================================
  Files          26      108       +82     
  Lines        1895    13936    +12041     
  Branches     1895    13936    +12041     
===========================================
+ Hits          760     9575     +8815     
- Misses       1100     3950     +2850     
- Partials       35      411      +376     

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

[dorimedini-starkware]

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

---

crates/blockifier/src/state/cached_state.rs line 755 at r1 (raw file):

Previously, yoavGrs wrote…

No.
Under a ForFee struct, this field will be 0 if not enable_stateful_compression.

ah, right.
but the StateChanges will count this? just not the ForFee flavor?

[github-actions]

Artifacts upload triggered. View details here

[Yoni-Starkware]

Reviewable status: 1 of 8 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/state/cached_state.rs line 731 at r1 (raw file):

Previously, dorimedini-starkware wrote…

hmmm... maybe this flag is a bit confusing...
I would expect enable_stateful_compression to indicate whether or not contract 0x2 appears in the state diff,
not whether or not we charge for allocations (which is what is happening here).
should we have two separate flags for this? @Yoni-Starkware

We should count the allocated keys here regardless of this flag - only charge according to it.
And yes, more readable to have two flags.

[Yoni-Starkware]

Reviewable status: 1 of 8 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/state/cached_state.rs line 731 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

We should count the allocated keys here regardless of this flag - only charge according to it.
And yes, more readable to have two flags.

Even better - let the second config be the cost of an allocated key, and set it to 0 for old VCs. That way we save a flag.

[Yoni-Starkware]

Reviewable status: 1 of 8 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/fee/resources.rs line 181 at r1 (raw file):

Previously, dorimedini-starkware wrote…

did we decide the allocation should be part of DA gas?
we show DA gas separately in the receipt, we want to charge for allocation but not necessarily make it show up as part of the DA gas vector
@Yoni-Starkware WDYT?

Hmmm yes but I don't want to split this part into another struct. And soon we will have L2 cost for state changes, so I think putting this under StateResources makes sense.

[dorimedini-starkware]

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yoavGrs)

---

crates/blockifier/src/fee/gas_usage_test.rs line 82 at r3 (raw file):

        },
        19,
>>>>>>> 868b6b68c (feat(blockifier): add struct StateChangesCountForFee)

oops

Code quote:

>>>>>>> 868b6b68c (feat(blockifier): add struct StateChangesCountForFee)

[github-actions]

Artifacts upload triggered. View details here

[dorimedini-starkware]

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

---

crates/blockifier/src/fee/resources.rs line 181 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Hmmm yes but I don't want to split this part into another struct. And soon we will have L2 cost for state changes, so I think putting this under StateResources makes sense.

unblocking here, but I'm saying that get_da_gas_cost should ignore allocations even in future PRs.
this function should return get_da_gas_cost(...).checked_add(get_allocations_gas_cost(...)).expect(...), so we can still use get_da_gas_cost to represent the cost of data availability (and not starknet state allocation costs)

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-25T13:58:41Z
• Started at 2024-11-25T13:58:44Z
• Started at 2024-11-25T15:07:39Z
• Started at 2024-11-25T15:07:41Z
• Started at 2024-11-25T16:02:25Z
• Started at 2024-11-26T09:10:03Z
• Started at 2024-11-26T09:10:05Z
• Started at 2024-11-26T09:24:50Z
• Started at 2024-11-26T10:18:28Z
• Started at 2024-11-26T10:54:35Z
• Started at 2024-11-26T11:13:40Z
• Started at 2024-11-26T11:14:41Z

[Yoni-Starkware]

Reviewable status: 1 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/state/cached_state.rs line 731 at r5 (raw file):

            },
            // TODO: Set number of allocated keys.
            n_allocated_keys: { 0 },

What's the plan?

Code quote:

            // TODO: Set number of allocated keys.
            n_allocated_keys: { 0 },

[yoavGrs]

Reviewable status: 1 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)

---

crates/blockifier/src/fee/gas_usage_test.rs line 82 at r3 (raw file):

Previously, dorimedini-starkware wrote…

oops

:(

---

crates/blockifier/src/fee/resources.rs line 153 at r1 (raw file):

Previously, dorimedini-starkware wrote…

consider this rename, everywhere (see below - maybe a separate flag for stateful compression?)

removed.

---

crates/blockifier/src/fee/resources.rs line 181 at r1 (raw file):

Previously, dorimedini-starkware wrote…

unblocking here, but I'm saying that get_da_gas_cost should ignore allocations even in future PRs.
this function should return get_da_gas_cost(...).checked_add(get_allocations_gas_cost(...)).expect(...), so we can still use get_da_gas_cost to represent the cost of data availability (and not starknet state allocation costs)

👍

---

crates/blockifier/src/state/cached_state.rs line 731 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Even better - let the second config be the cost of an allocated key, and set it to 0 for old VCs. That way we save a flag.

https://reviewable.io/reviews/starkware-libs/sequencer/2200

---

crates/blockifier/src/state/cached_state.rs line 755 at r1 (raw file):

Previously, dorimedini-starkware wrote…

ah, right.
but the StateChanges will count this? just not the ForFee flavor?

Sorry, even in theForFee struct, the real amount will appear.

[yoavGrs]

Reviewable status: 1 of 9 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/blockifier/src/state/cached_state.rs line 731 at r5 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

What's the plan?

I changed the order of the branches, and removed the TODO.

[dorimedini-starkware]

Reviewed 1 of 4 files at r4, 1 of 6 files at r5, 4 of 5 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

---

crates/blockifier/src/fee/resources.rs line 17 at r7 (raw file):

};
#[cfg(any(test, feature = "testing"))]
use crate::state::cached_state::StateChangesCount;

delete (see below)
non-blocking, it's a question of style

Code quote:

#[cfg(any(test, feature = "testing"))]
use crate::state::cached_state::StateChangesCount;

---

crates/blockifier/src/fee/resources.rs line 183 at r7 (raw file):

    ) -> Self {
        Self {
            state_changes_for_fee: StateChangesCountForFee {

I suggest this, looks better than gating the use (see above)

Suggestion:

state_changes_for_fee: crate::state::cached_state::StateChangesCountForFee

[Yoni-Starkware]

Reviewed 1 of 4 files at r4, 1 of 6 files at r5, 1 of 5 files at r6, 1 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

[dorimedini-starkware]

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

[yoavGrs]

Merge activity

• Nov 26, 5:53 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Nov 26, 6:15 AM EST: Graphite rebased this pull request as part of a merge.
• Nov 26, 6:34 AM EST: A user merged this pull request with Graphite.

[dorimedini-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

[dorimedini-starkware]

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