Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(blockifier): get alias from last state diff #2946

Merged
merged 1 commit into from
Dec 29, 2024

Conversation

yoavGrs
Copy link
Contributor

@yoavGrs yoavGrs commented Dec 25, 2024

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

yoavGrs commented Dec 25, 2024

@yoavGrs yoavGrs marked this pull request as ready for review December 25, 2024 11:12
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: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 183 at r1 (raw file):

struct AliasCompressor<'a, S: StateReader> {
    state: &'a S,
    alias_contract_address: ContractAddress,

Why is this needed? use the constant

Code quote:

    alias_contract_address: ContractAddress,

crates/blockifier/src/state/stateful_compression.rs line 194 at r1 (raw file):

            new_aliases: state_diff
                .storage
                .iter()

Since you need to iterate the entire diff again, how about a more straightforward suggestion (that would be compatible with the execution):

In state_diff_with_alias_allocation:

  • Compute state_diff_without_aliases
  • Let the AliasUpdater insert the new aliases directly to the state.
  • Compute the state diff again.

Now the AliasCompressor has all the info in state.

Code quote:

                .storage
                .iter()

@yoavGrs yoavGrs force-pushed the yoav/compression/look_at_alias_contract branch from 5937694 to 8f1025d Compare December 25, 2024 13:10
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 3 files at r2.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 84 at r2 (raw file):

/// Updates the alias contract with the new keys.
struct AliasUpdater<'a, S: StateReader> {
    state: &'a mut CachedState<S>,

Try this

Suggestion:

struct AliasUpdater<'a, S: State> {
    state: &'a mut S,

crates/blockifier/src/blockifier/transaction_executor.rs line 171 at r2 (raw file):

        log::debug!("Final block weights: {:?}.", self.bouncer.get_accumulated_weights());
        let mut block_state = self.block_state.take().expect(BLOCK_STATE_ACCESS_ERR);

Why not?

Suggestion:

self.block_state.as_mut().expect(BLOCK_STATE_ACCESS_ERR);

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 3 files at r2.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)

@yoavGrs yoavGrs force-pushed the yoav/compression/look_at_alias_contract branch 2 times, most recently from 5956f90 to 3b6e5a0 Compare December 25, 2024 14:03
Copy link
Contributor Author

@yoavGrs yoavGrs 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: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)


crates/blockifier/src/blockifier/transaction_executor.rs line 171 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why not?

Done.


crates/blockifier/src/state/stateful_compression.rs line 183 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Why is this needed? use the constant

I removed the constant when I saw it in the versioned_constants


crates/blockifier/src/state/stateful_compression.rs line 194 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Since you need to iterate the entire diff again, how about a more straightforward suggestion (that would be compatible with the execution):

In state_diff_with_alias_allocation:

  • Compute state_diff_without_aliases
  • Let the AliasUpdater insert the new aliases directly to the state.
  • Compute the state diff again.

Now the AliasCompressor has all the info in state.

Done.


crates/blockifier/src/state/stateful_compression.rs line 84 at r2 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

Try this

Done.

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.

:lgtm:

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

@yoavGrs yoavGrs removed the request for review from dorimedini-starkware December 25, 2024 20:57
Copy link
Contributor Author

yoavGrs commented Dec 26, 2024

Merge activity

  • Dec 26, 7:50 AM EST: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
  • Dec 29, 4:40 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 29, 4:40 AM EST: Graphite couldn't add this PR to the GitHub merge queue because it failed for an unknown reason.

@yoavGrs yoavGrs force-pushed the yoav/compression/look_at_alias_contract branch from 3b6e5a0 to 9a0991c Compare December 29, 2024 07:38
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.

:lgtm:

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

@yoavGrs yoavGrs force-pushed the yoav/compression/look_at_alias_contract branch from 9a0991c to e9b7969 Compare December 29, 2024 13:10
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 all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 114 at r4 (raw file):

                Some(alias) => alias,
                None => INITIAL_AVAILABLE_ALIAS,
            };

non-blocking; but this should be equivalent

Suggestion:

            let alias_to_allocate = self.next_free_alias.unwrap_or(INITIAL_AVAILABLE_ALIAS);

@yoavGrs yoavGrs force-pushed the yoav/compression/look_at_alias_contract branch from e9b7969 to 7e93dec Compare December 29, 2024 14:15
Copy link
Contributor Author

@yoavGrs yoavGrs 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: 2 of 3 files reviewed, all discussions resolved (waiting on @Yoni-Starkware)


crates/blockifier/src/state/stateful_compression.rs line 114 at r4 (raw file):

Previously, dorimedini-starkware wrote…

non-blocking; but this should be equivalent

Done.

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 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

@yoavGrs yoavGrs merged commit 2a7d97c into main-v0.13.4 Dec 29, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2024
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.

4 participants