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

feat(blockifier): aliases updater struct #2534

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Conversation

yoavGrs
Copy link
Contributor

@yoavGrs yoavGrs commented Dec 8, 2024

fix(blockifier): fix test_write_at_validate_and_execute (#2529)

feat(blockifier): aliases updater struct

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

yoavGrs commented Dec 8, 2024

@yoavGrs yoavGrs marked this pull request as ready for review December 8, 2024 16:02
@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch from f20a030 to 4a4d28d Compare December 8, 2024 16:12
@yoavGrs yoavGrs changed the title fix(blockifier): fix test_write_at_validate_and_execute (#2529) feat(blockifier): aliases updater struct Dec 8, 2024
Copy link
Contributor

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


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

type Alias = Felt;
type AliasKey = StorageKey;

I think it's more suitable because ContractAddress & StorageKey wrap PatriciaKey.

Suggestion:

type AliasKey = PatriciaKey;

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

pub fn get_alias_counter_storage_key() -> StorageKey {
    StorageKey::from(ALIAS_COUNTER_STORAGE_KEY)
}

More readable, IMO, and doesn't need to recompute every time get_alias_contract_address & get_alias_counter_storage_key are called.

Suggestion:

// The address of the alias contract.
const ALIAS_CONTRACT_ADDRESS: ContractAddress = ContractAddress::from(2);
// The storage key of the alias counter in the alias contract.
const ALIAS_COUNTER_STORAGE_KEY: StorageKey = StorageKey::from(0);
// The minimal value for a key to be allocated an alias. Smaller keys are serialized as is (their
// alias is identical to the key).
const MIN_VALUE_FOR_ALIAS_ALLOC: Felt = Felt::from_hex_unchecked("0x80");

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

    state: &'a mut CachedState<S>,
    next_free_alias: Alias,
    was_updated: bool,

I don't think you need this field. IIUC, you use it when you need to decide if the counter should be updated or not.
I suggest always setting the counter. You can use the Drop trait by doing something like:


impl Drop for AliasUpdater {
    fn drop(&mut self) {
        self.state.set_storage_at(ALIAS_CONTRACT_ADDRESS, ALIAS_COUNTER_STORAGE_KEY, self.next_free_alias);
    }
}

Code quote:

was_updated: bool,

@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch from 4a4d28d to dd3c882 Compare December 10, 2024 09:18
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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)


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

Previously, nimrod-starkware wrote…

I think it's more suitable because ContractAddress & StorageKey wrap PatriciaKey.

On the other hand, this type is a storage key in the aliases contract.
Look at the calls to get_storage_at and set_storage_at.


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

Previously, nimrod-starkware wrote…

More readable, IMO, and doesn't need to recompute every time get_alias_contract_address & get_alias_counter_storage_key are called.

I can't use from in a const context.
Maybe @dorimedini-starkware will have a more elegant solution than mine.


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

Previously, nimrod-starkware wrote…

I don't think you need this field. IIUC, you use it when you need to decide if the counter should be updated or not.
I suggest always setting the counter. You can use the Drop trait by doing something like:


impl Drop for AliasUpdater {
    fn drop(&mut self) {
        self.state.set_storage_at(ALIAS_CONTRACT_ADDRESS, ALIAS_COUNTER_STORAGE_KEY, self.next_free_alias);
    }
}

Removed.
Note that for the first block we will write the counter to the storage, even if it doesn't contain aliases.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.

Project coverage is 79.64%. Comparing base (e3165c4) to head (d9c83da).
Report is 809 commits behind head on main.

Files with missing lines Patch % Lines
crates/starknet_api/src/core.rs 0.00% 6 Missing ⚠️
...rates/blockifier/src/state/stateful_compression.rs 89.28% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2534       +/-   ##
===========================================
+ Coverage   40.10%   79.64%   +39.53%     
===========================================
  Files          26      102       +76     
  Lines        1895    13726    +11831     
  Branches     1895    13726    +11831     
===========================================
+ Hits          760    10932    +10172     
- Misses       1100     2308     +1208     
- Partials       35      486      +451     

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

Copy link
Contributor

@nimrod-starkware nimrod-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 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


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

Previously, yoavGrs wrote…

I can't use from in a const context.
Maybe @dorimedini-starkware will have a more elegant solution than mine.

How does this work?..
const MIN_VALUE_FOR_ALIAS_ALLOC: Felt = Felt::from_hex_unchecked("0x80")


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

Previously, yoavGrs wrote…

Removed.
Note that for the first block we will write the counter to the storage, even if it doesn't contain aliases.

That's ok; in the OS it's also the case

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 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)


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

Previously, nimrod-starkware wrote…

How does this work?..
const MIN_VALUE_FOR_ALIAS_ALLOC: Felt = Felt::from_hex_unchecked("0x80")

you should be able to make the first two const as well, e.g.

impl PatriciaKey {
    const fn new(v: Felt) -> Self { Self(v) }
}

const x: StorageKey = StorageKey(PatriciaKey::new(Felt::ZERO))

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

#[cfg(test)]
#[path = "stateful_compression_test.rs"]
pub mod stateful_compression_test;

do you need the #[path] if the module name is identical to the file name?

Code quote:

#[path = "stateful_compression_test.rs"]
pub mod stateful_compression_test;

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

const ALIAS_CONTRACT_ADDRESS: u8 = 2;
// The storage key of the alias counter in the alias contract.
const ALIAS_COUNTER_STORAGE_KEY: u8 = 0;

can you make these typed? maybe you can add const definitions in the types themselves

Suggestion:

const ALIAS_CONTRACT_ADDRESS: ContractAddress = ContractAddress(2);
// The storage key of the alias counter in the alias contract.
const ALIAS_COUNTER_STORAGE_KEY: StorageKey = StorageKey(0);

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

// The minimal value for a key to be allocated an alias. Smaller keys are serialized as is (their
// alias is identical to the key).
const MIN_VALUE_FOR_ALIAS_ALLOC: Felt = Felt::from_hex_unchecked("0x80");

we need to verify consistency with python-side constants.
please expose these constants with pyfunctions in the native blockifier, and open a py side PR to test that these values are identical to there python counterparts

Code quote:

// The address of the alias contract.
const ALIAS_CONTRACT_ADDRESS: u8 = 2;
// The storage key of the alias counter in the alias contract.
const ALIAS_COUNTER_STORAGE_KEY: u8 = 0;
// The minimal value for a key to be allocated an alias. Smaller keys are serialized as is (their
// alias is identical to the key).
const MIN_VALUE_FOR_ALIAS_ALLOC: Felt = Felt::from_hex_unchecked("0x80");

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

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

why operate directly on the state?
I would prefer starting a transactional state, and then you can have a fn finalize_and_commit

Code quote:

state: &'a mut CachedState<S>,

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

        if alias_key.0 >= PatriciaKey::try_from(MIN_VALUE_FOR_ALIAS_ALLOC)?
            && self.state.get_storage_at(get_alias_contract_address(), *alias_key)? == Felt::ZERO
        {

non-blocking, but seems nicer

Suggestion:

    fn in_aliasing_range(alias_key: &AliasKey) -> bool {
        alias_key.0 >= MIN_VALUE_FOR_ALIAS_ALLOC
    }
    
    fn get_alias(&mut self, alias_key: &AliasKey) -> StateResult<Felt> {
        self.state.get_storage_at(get_alias_contract_address(), *alias_key)
    }
    
    /// Inserts the alias key to the updates if it's not already aliased.
    fn set_alias(&mut self, alias_key: &AliasKey) -> StateResult<()> {
        if Self::in_aliasing_range(alias_key) && self.get_alias(alias_key)? == Felt::ZERO {

crates/blockifier/src/state/stateful_compression_test.rs line 18 at r2 (raw file):

fn insert_to_alias_contract(
    storage: &mut HashMap<(ContractAddress, StorageKey), Felt>,

how about using an existing type?

Suggestion:

storage: &mut StorageView,

crates/blockifier/src/state/stateful_compression_test.rs line 25 at r2 (raw file):

}

fn initial_state(n_exist_aliases: u8) -> CachedState<DictStateReader> {

Suggestion:

n_existing_aliases

crates/blockifier/src/state/stateful_compression_test.rs line 82 at r2 (raw file):

    // Test the new aliases.
    let mut expected_storage_diff = HashMap::new();
    let mut next_alias = MIN_VALUE_FOR_ALIAS_ALLOC + Felt::from(n_exist_aliases);

Suggestion:

expected_next_alias

@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch from dd3c882 to d9c83da Compare December 10, 2024 15:13
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 4 files reviewed, 8 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)


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

Previously, dorimedini-starkware wrote…

you should be able to make the first two const as well, e.g.

impl PatriciaKey {
    const fn new(v: Felt) -> Self { Self(v) }
}

const x: StorageKey = StorageKey(PatriciaKey::new(Felt::ZERO))

Done.


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

Previously, dorimedini-starkware wrote…

do you need the #[path] if the module name is identical to the file name?

Yes. Checking why.


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

Previously, dorimedini-starkware wrote…

can you make these typed? maybe you can add const definitions in the types themselves

Done.


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

Previously, dorimedini-starkware wrote…

why operate directly on the state?
I would prefer starting a transactional state, and then you can have a fn finalize_and_commit

I made some changes to give aliases only for addresses and keys with nontrivial changes.
The state is immutable now.


crates/blockifier/src/state/stateful_compression_test.rs line 18 at r2 (raw file):

Previously, dorimedini-starkware wrote…

how about using an existing type?

I don't need the wrapping type.


crates/blockifier/src/state/stateful_compression_test.rs line 25 at r2 (raw file):

}

fn initial_state(n_exist_aliases: u8) -> CachedState<DictStateReader> {

Done.


crates/blockifier/src/state/stateful_compression_test.rs line 82 at r2 (raw file):

    // Test the new aliases.
    let mut expected_storage_diff = HashMap::new();
    let mut next_alias = MIN_VALUE_FOR_ALIAS_ALLOC + Felt::from(n_exist_aliases);

Done.

Copy link
Contributor

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


crates/blockifier/src/state/stateful_compression.rs line 23 at r3 (raw file):

// The minimal value for a key to be allocated an alias. Smaller keys are serialized as is (their
// alias is identical to the key).
const MIN_VALUE_FOR_ALIAS_ALLOC: Felt = Felt::from_hex_unchecked("0x80");

please also add INITIAL_AVAILABLE_ALIAS: Felt as a constant, so you don't need to recompute PatriciaKey::try_from(MIN_VALUE_FOR_ALIAS_ALLOC) every time you check wether to allocate or not.

Suggestion:

// The minimal value for a key to be allocated an alias. Smaller keys are serialized as is (their
// alias is identical to the key).
const INITIAL_AVAILABLE_ALIAS: Felt = Felt::from_hex_unchecked("0x80");

// ....
const MIN_VALUE_FOR_ALIAS_ALLOC: PatriciaKey = PatriciaKey::new_unchecked(INITIAL_AVAILABLE_ALIAS);

crates/blockifier/src/state/stateful_compression.rs line 50 at r3 (raw file):

    /// Inserts the alias key to the updates if it's not already aliased.
    fn insert_alias(&mut self, alias_key: &AliasKey) -> StateResult<()> {
        if alias_key.0 >= PatriciaKey::try_from(MIN_VALUE_FOR_ALIAS_ALLOC)?

Suggestion:

MIN_VALUE_FOR_ALIAS_ALLOC

crates/starknet_api/src/core.rs line 125 at r3 (raw file):

    }

    pub const fn new(val: Felt) -> Self {

Shouldn't that be also named new_unchecked?

Code quote:

pub const fn new

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 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)


crates/starknet_api/src/core.rs line 125 at r3 (raw file):

Previously, nimrod-starkware wrote…

Shouldn't that be also named new_unchecked?

no, no type conversions are done here


crates/starknet_api/src/core.rs line 367 at r3 (raw file):

    pub const fn new_unchecked(val: StarkHash) -> Self {
        Self(val)
    }

you are not performing conversions here - nothing is "unchecked"

Suggestion:

    pub const fn new(val: StarkHash) -> Self {
        Self(val)
    }

crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

    /// contract.
    fn finalize_updates(mut self) -> HashMap<StorageEntry, Felt> {
        if !self.new_aliases.is_empty() || self.next_free_alias == MIN_VALUE_FOR_ALIAS_ALLOC {

if no new aliases are allocated and the next free alias is not zero, why set the counter value?
shouldn't you be checking if the next free alias is zero (not initialized yet)?

Code quote:

self.next_free_alias == MIN_VALUE_FOR_ALIAS_ALLOC

Copy link
Contributor

@nimrod-starkware nimrod-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, 7 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, dorimedini-starkware wrote…

if no new aliases are allocated and the next free alias is not zero, why set the counter value?
shouldn't you be checking if the next free alias is zero (not initialized yet)?

In the OS, if the counter is 0, we set it to 128 right away, regardless of whether new aliases were allocated or not.


crates/starknet_api/src/core.rs line 125 at r3 (raw file):

Previously, dorimedini-starkware wrote…

no, no type conversions are done here

What if val >= 2**251? the contract address will be invalid.

@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch from d9c83da to 48faf8d Compare December 15, 2024 10:36
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 6 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)


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

Previously, dorimedini-starkware wrote…

we need to verify consistency with python-side constants.
please expose these constants with pyfunctions in the native blockifier, and open a py side PR to test that these values are identical to there python counterparts

OK, but bot in this PR


crates/blockifier/src/state/stateful_compression.rs line 23 at r3 (raw file):

Previously, nimrod-starkware wrote…

please also add INITIAL_AVAILABLE_ALIAS: Felt as a constant, so you don't need to recompute PatriciaKey::try_from(MIN_VALUE_FOR_ALIAS_ALLOC) every time you check wether to allocate or not.

Done.


crates/starknet_api/src/core.rs line 125 at r3 (raw file):

Previously, nimrod-starkware wrote…

What if val >= 2**251? the contract address will be invalid.

Reverted.


crates/starknet_api/src/core.rs line 367 at r3 (raw file):

Previously, dorimedini-starkware wrote…

you are not performing conversions here - nothing is "unchecked"

Reverted.


crates/blockifier/src/state/stateful_compression.rs line 50 at r3 (raw file):

    /// Inserts the alias key to the updates if it's not already aliased.
    fn insert_alias(&mut self, alias_key: &AliasKey) -> StateResult<()> {
        if alias_key.0 >= PatriciaKey::try_from(MIN_VALUE_FOR_ALIAS_ALLOC)?

Done.

Copy link

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.831 ms 34.907 ms 34.985 ms]
change: [+1.2989% +1.5409% +1.7889%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

Copy link
Contributor

@nimrod-starkware nimrod-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: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @yoavGrs)


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

        PatriciaKey::try_from(Felt::TWO).unwrap()
    );
    // const ALIAS_CONTRACT_ADDRESS: ContractAddress = ContractAddress::new(Felt::TWO);

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 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, nimrod-starkware wrote…

In the OS, if the counter is 0, we set it to 128 right away, regardless of whether new aliases were allocated or not.

yes but it seems to me like the counter is not zero here, i.e. already set to 128 previously. why set again?


crates/blockifier/Cargo.toml line 38 at r4 (raw file):

itertools.workspace = true
keccak.workspace = true
lazy_static.workspace = true

remove - use LazyLock

Code quote:

lazy_static.workspace = true

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

    static ref MIN_VALUE_FOR_ALIAS_ALLOC: PatriciaKey =
        PatriciaKey::try_from(INITIAL_AVAILABLE_ALIAS).unwrap();
}

please use LazyLock, not lazy_static. it doesn't require an external dep (we have examples in code)

Code quote:

lazy_static! {
    // The address of the alias contract.
    static ref ALIAS_CONTRACT_ADDRESS: ContractAddress = ContractAddress(
        PatriciaKey::try_from(Felt::TWO).unwrap()
    );
    // const ALIAS_CONTRACT_ADDRESS: ContractAddress = ContractAddress::new(Felt::TWO);
    // The storage key of the alias counter in the alias contract.
    static ref ALIAS_COUNTER_STORAGE_KEY: StorageKey = StorageKey(
        PatriciaKey::try_from(Felt::ZERO).unwrap()
    );
    // The minimal value for a key to be allocated an alias. Smaller keys are serialized as is (their
    // alias is identical to the key).
    static ref MIN_VALUE_FOR_ALIAS_ALLOC: PatriciaKey =
        PatriciaKey::try_from(INITIAL_AVAILABLE_ALIAS).unwrap();
}

@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch from 48faf8d to dd494d2 Compare December 15, 2024 12:10
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: 3 of 6 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, dorimedini-starkware wrote…

yes but it seems to me like the counter is not zero here, i.e. already set to 128 previously. why set again?

Only here we write the counter in the storage.


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

Previously, dorimedini-starkware wrote…

please use LazyLock, not lazy_static. it doesn't require an external dep (we have examples in code)

Done.


crates/blockifier/Cargo.toml line 38 at r4 (raw file):

Previously, dorimedini-starkware wrote…

remove - use LazyLock

Done.


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

        PatriciaKey::try_from(Felt::TWO).unwrap()
    );
    // const ALIAS_CONTRACT_ADDRESS: ContractAddress = ContractAddress::new(Felt::TWO);

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 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, yoavGrs wrote…

Only here we write the counter in the storage.

it is theoretically possible for the next available alias to be the initial available alias, even after we initialize it in a previous block, no? what if the first block after upgrade does not introduce any alias?

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


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

Previously, yoavGrs wrote…

Yes. Checking why.

It can be removed if the test module is declared in state.rs.
In cached_state.rs we did the same as here.


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, dorimedini-starkware wrote…

it is theoretically possible for the next available alias to be the initial available alias, even after we initialize it in a previous block, no? what if the first block after upgrade does not introduce any alias?

You're right.
If there are no aliases in the first two blocks, there is unnecessary writing. Fixed.

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, 1 unresolved discussion (waiting on @nimrod-starkware and @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, yoavGrs wrote…

You're right.
If there are no aliases in the first two blocks, there is unnecessary writing. Fixed.

where? I don't see a diff

@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch 2 times, most recently from c8cb536 to 5edecb2 Compare December 17, 2024 07:34
@yoavGrs yoavGrs force-pushed the yoav/aliasing/alias_updater branch from 5edecb2 to 7bde263 Compare December 17, 2024 08:24
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 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 78 at r6 (raw file):

                }
            }
        }

does this match OS behavior? @nimrod-starkware

Code quote:

        match self.next_free_alias {
            None => {
                self.new_aliases.insert(*ALIAS_COUNTER_STORAGE_KEY, INITIAL_AVAILABLE_ALIAS);
            }
            Some(alias) => {
                if !self.new_aliases.is_empty() {
                    self.new_aliases.insert(*ALIAS_COUNTER_STORAGE_KEY, alias);
                }
            }
        }

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: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs)


crates/blockifier/src/state/stateful_compression.rs line 63 at r3 (raw file):

Previously, dorimedini-starkware wrote…

where? I don't see a diff

Done, sorry

Copy link
Contributor

@nimrod-starkware nimrod-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 (waiting on @dorimedini-starkware)


crates/blockifier/src/state/stateful_compression.rs line 78 at r6 (raw file):

Previously, dorimedini-starkware wrote…

does this match OS behavior? @nimrod-starkware

yes :)

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.

:lgtm:

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

Copy link
Contributor

@nimrod-starkware nimrod-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:

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

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 @yoavGrs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants