fix(blockifier): merge state diff with squash

[yoavGrs]

No description provided.

[reviewable-StarkWare]

This change is 

[yoavGrs]

• fix(blockifier): merge state diff with squash #2310 : 2 dependent PRs (#2456 , #2471 ) 👈 (View in Graphite)
• 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
• 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.

[github-actions]

Artifacts upload workflows:

• Started at 2024-11-27T14:28:21Z
• Started at 2024-11-27T14:39:09Z
• Started at 2024-12-02T10:56:06Z
• Started at 2024-12-02T12:42:09Z
• Started at 2024-12-02T12:42:11Z
• Started at 2024-12-02T12:46:33Z
• Started at 2024-12-03T09:28:41Z
• Started at 2024-12-03T10:23:11Z
• Started at 2024-12-03T11:42:29Z
• Started at 2024-12-04T14:00:47Z
• Started at 2024-12-04T14:21:53Z
• Started at 2024-12-04T16:44:08Z
• Started at 2024-12-04T16:43:52Z
• Started at 2024-12-04T16:44:08Z
• Started at 2024-12-04T16:44:38Z
• Started at 2024-12-04T16:44:43Z
• Started at 2024-12-05T09:02:17Z
• Started at 2024-12-05T09:13:10Z
• Started at 2024-12-05T10:00:32Z
• Started at 2024-12-05T10:53:24Z

[codecov]

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.16%. Comparing base (e3165c4) to head (5861d2a).
Report is 711 commits behind head on main.

Files with missing lines	Patch %	Lines
.../blockifier/src/transaction/account_transaction.rs	77.77%	0 Missing and 2 partials ⚠️
crates/blockifier/src/state/cached_state.rs	97.05%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2310       +/-   ##
===========================================
+ Coverage   40.10%   71.16%   +31.05%     
===========================================
  Files          26      102       +76     
  Lines        1895    13739    +11844     
  Branches     1895    13739    +11844     
===========================================
+ Hits          760     9777     +9017     
- Misses       1100     3547     +2447     
- Partials       35      415      +380     

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

[dorimedini-starkware]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @yoavGrs and @Yoni-Starkware)

---

a discussion (no related file):
nice! much cleaner, and nice squashing algorithm :)

---

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

    pub fn to_state_cache(&mut self) -> StateResult<StateCache> {
        self.update_initial_values_of_write_only_access()?;

what is this?

Code quote:

self.update_initial_values_of_write_only_access()?;

---

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

// Invariant: keys cannot be deleted from fields (only used internally by the cached state).
#[derive(Clone, Debug, Default, PartialEq, Eq)]

I'd rather not ad this Clone if possible - the struct size is not O(1).
can you return a reference in to_state_cache?
or, change the signature to accept mut self instead of &mut self; to_X functions are consuming by convension

Code quote:

(Clone,

---

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

    /// is important.
    pub fn merge(state_caches: Vec<Self>) -> Self {
        let mut merged_state_cache = StateCache::default();

i think this is the terminology

Suggestion:

    /// Squashes the given state caches into a single one. Note that the order of the state caches
    /// is important.
    pub fn squash(state_caches: Vec<Self>) -> Self {
        let mut squashed_state_cache = StateCache::default();

---

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

#[case(true, vec![felt!("0x7"), felt!("0x0")], false)]
#[case(false, vec![felt!("0x7"), felt!("0x1")], false)]
#[case(false, vec![felt!("0x0"), felt!("0x8")], false)]

nice

Code quote:

#[case(false, vec![felt!("0x0"), felt!("0x8")], false)]

[yoavGrs]

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

---

a discussion (no related file):

Previously, dorimedini-starkware wrote…

nice! much cleaner, and nice squashing algorithm :)

:)

---

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

Previously, dorimedini-starkware wrote…

what is this?

It fills missing initial_reads, as done in to_state_diff.

---

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

Previously, dorimedini-starkware wrote…

I'd rather not ad this Clone if possible - the struct size is not O(1).
can you return a reference in to_state_cache?
or, change the signature to accept mut self instead of &mut self; to_X functions are consuming by convension

I don't see an immediate solution to this.
In account_transaction.rs, we should save a "snapshot" of the validation state cache and update the state for the execution stage.

(Conceptually, only the fn commit needs a mutable base state, so it is possible to own a reference to the state cache up to this point.)

---

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

Previously, dorimedini-starkware wrote…

i think this is the terminology

Done.

[dorimedini-starkware]

Reviewed 8 of 8 files at r4, all commit messages.
Reviewable status: 8 of 17 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)

---

crates/blockifier/src/fee/receipt.rs line 107 at r5 (raw file):

            .starknet_resources
            .state
            .da_gas_vector(tx_context.block_context.block_info.use_kzg_da);

this change - and others here - are from 2301, please rebase

Code quote:

.da_gas_vector(tx_context.block_context.block_info.use_kzg_da);

---

crates/blockifier/resources/versioned_constants_0_13_0.json line 11 at r5 (raw file):

    "disable_cairo0_redeclaration": false,
    "enable_stateful_compression": false,
    "comprehensive_state_diff": false,

@Yoni-Starkware any opinion on the name?

Code quote:

"comprehensive_state_diff"

[dorimedini-starkware]

Reviewed 1 of 1 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)

[Yoni-Starkware]

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

---

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

Previously, dorimedini-starkware wrote…

ok, I think it won't hit performance too much.
@Yoni-Starkware thoughts?
tl;dr extra clone of the state cache of a tx is now required

If you define let mut execution_state = CachedState::new(MutRefState::new(validate_state));, you could access the cache of validate after execution. But it might be too ugly, so I'm okay with this clone.

---

crates/blockifier/src/state/cached_state.rs line 414 at r9 (raw file):

    /// If 'comprehensive_state_diff' is false, opposite updates may not be canceled out. Used for
    /// backward compatibility.
    pub fn squashed_state_diff(

I think it's better to have a separate function that only returns a squashed StateCache (in your smart ordering)

---

crates/blockifier/src/state/cached_state.rs line 415 at r9 (raw file):

    /// backward compatibility.
    pub fn squashed_state_diff(
        state_caches: Vec<Self>,

Can you at least save the clone of execute? it is much heavier

Suggestion:

state_caches: Vec<&Self>,

[Yoni-Starkware]

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

---

crates/blockifier/src/transaction/account_transaction.rs line 601 at r9 (raw file):

            ),
            execution_steps_consumed,
        );

@dorimedini-starkware any fancy way to save this iteration over the cache if not needed? (I.e., if the tx is not reverted)

Suggestion:

        // Save the state changes resulting from running `validate_tx`, to be used later for
        // resource and fee calculation.
        let validate_state_cache = state.to_state_cache()?;

        // Create copies of state and validate_resources for the execution.
        // Both will be rolled back if the execution is reverted or committed upon success.
        let mut execution_state = TransactionalState::create_transactional(state);

        let execution_result =
            self.run_execute(&mut execution_state, &mut execution_context, remaining_gas);

        // Pre-compute cost in case of revert.
        // TODO(tzahi): add reverted_l2_gas to the receipt.
        let execution_steps_consumed =
            n_allotted_execution_steps - execution_context.n_remaining_steps();
        let revert_receipt = TransactionReceipt::from_account_tx(
            self,
            &tx_context,
            &validate_state_cache.to_state_diff(),
            CallInfo::summarize_many(
                validate_call_info.iter(),
                &tx_context.block_context.versioned_constants,
            ),
            execution_steps_consumed,
        );

[yoavGrs]

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

---

crates/blockifier/src/state/cached_state.rs line 414 at r9 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

I think it's better to have a separate function that only returns a squashed StateCache (in your smart ordering)

Done.
(If I understood correctly what you asked me)

---

crates/blockifier/src/state/cached_state.rs line 415 at r9 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Can you at least save the clone of execute? it is much heavier

Yes. Done.

---

crates/blockifier/src/transaction/account_transaction.rs line 601 at r9 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

@dorimedini-starkware any fancy way to save this iteration over the cache if not needed? (I.e., if the tx is not reverted)

If we don't return one big match, it's possible.

---

crates/blockifier/resources/versioned_constants_0_13_0.json line 11 at r5 (raw file):

Previously, dorimedini-starkware wrote…

@Yoni-Starkware any opinion on the name?

Approved in the previous PR

[Yoni-Starkware]

Reviewable status: 14 of 17 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/state/cached_state.rs line 69 at r10 (raw file):

/// It is not guaranteed that every write has a corresponding initial read.
Why are we okay with this?

Code quote:

    pub fn to_state_cache(&mut self) -> StateResult<StateCache> {
        self.update_initial_values_of_write_only_access()?;
        Ok(self.cache.borrow().clone())
    }

    /// It is not guaranteed that every write has a corresponding initial read.
    pub fn raw_state_cache(&self) -> Ref<'_, StateCache> {
        self.cache.borrow()
    }

[Yoni-Starkware]

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

---

crates/blockifier/src/state/cached_state.rs line 414 at r9 (raw file):

Previously, yoavGrs wrote…

Done.
(If I understood correctly what you asked me)

Almost - just move the .to_state_diff outside (and rename squashed_state_diff() -> squash_state_caches

[Yoni-Starkware]

Reviewable status: 14 of 17 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

---

crates/blockifier/src/state/cached_state.rs line 69 at r10 (raw file):

    pub fn raw_state_cache(&self) -> Ref<'_, StateCache> {
        self.cache.borrow()
    }

Oh, I see that you moved it outside. If there is not particular reason, I think it's better to have a single function like this. You can clone the result outside for validate.

Suggestion:

    pub fn borrow_updated_state_cache(&mut self) -> StateResult<Ref<'_, StateCache>> {
        self.update_initial_values_of_write_only_access()?;
        Ok(self.cache.borrow())
    }

[yoavGrs]

Reviewable status: 14 of 17 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/blockifier/src/state/cached_state.rs line 414 at r9 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Almost - just move the .to_state_diff outside (and rename squashed_state_diff() -> squash_state_caches

Done.

---

crates/blockifier/src/state/cached_state.rs line 69 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

/// It is not guaranteed that every write has a corresponding initial read.
Why are we okay with this?

Removed.

---

crates/blockifier/src/state/cached_state.rs line 69 at r10 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Oh, I see that you moved it outside. If there is not particular reason, I think it's better to have a single function like this. You can clone the result outside for validate.

Nice. Done.

[yoavGrs]

Reviewable status: 14 of 17 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

crates/blockifier/src/transaction/account_transaction.rs line 601 at r9 (raw file):

Previously, yoavGrs wrote…

If we don't return one big match, it's possible.

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

[dorimedini-starkware]

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Yoni-Starkware)

---

crates/blockifier/src/state/cached_state_test.rs line 447 at r12 (raw file):

    }

    let merged_changes = StateCache::squash_state_diff_backward_compatible(

I don't think it's necessary to append _backward_compatible to the name; you have the (documented) boolean parameter
non-blocking

Code quote:

squash_state_diff_backward_compatible

[yoavGrs]

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

---

crates/blockifier/src/state/cached_state_test.rs line 447 at r12 (raw file):

Previously, dorimedini-starkware wrote…

I don't think it's necessary to append _backward_compatible to the name; you have the (documented) boolean parameter
non-blocking

Done.

[dorimedini-starkware]

Reviewed 3 of 3 files at r13, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Yoni-Starkware)

[Yoni-Starkware]

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

[Yoni-Starkware]

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

[Yoni-Starkware]

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

[yoavGrs]

Merge activity

• Dec 5, 7:08 AM EST: A user started a stack merge that includes this pull request via Graphite.
• Dec 5, 7:09 AM EST: A user merged this pull request with Graphite.