-
Notifications
You must be signed in to change notification settings - Fork 31
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): replace the aliases in the state diff #2766
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this 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 r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/blockifier/src/state/stateful_compression_test.rs
line 0 at r1 (raw file):
can you implement a decompress
test util and use it to compare original and compressed+decompressed diff?
crates/blockifier/src/state/stateful_compression_test.rs
line 271 at r1 (raw file):
.into_iter() .collect(), declared_contracts: vec![(ClassHash(Felt::THREE), true)].into_iter().collect(),
put large values here? to make sure these are not compressed, but for the correct reason
Code quote:
compiled_class_hashes: vec![(ClassHash(Felt::ONE), CompiledClassHash(Felt::ZERO))]
.into_iter()
.collect(),
declared_contracts: vec![(ClassHash(Felt::THREE), true)].into_iter().collect(),
crates/blockifier/src/state/stateful_compression.rs
line 161 at r1 (raw file):
.class_hashes .insert(alias_compressor.compress_address(address)?, *class_hash); }
we always compress class hashes? @Yoni-Starkware @nimrod-starkware
Code quote:
for (address, class_hash) in state_diff.class_hashes.iter() {
compressed_state_diff
.class_hashes
.insert(alias_compressor.compress_address(address)?, *class_hash);
}
crates/blockifier/src/state/stateful_compression.rs
line 174 at r1 (raw file):
compressed_state_diff.declared_contracts.extend(state_diff.declared_contracts.iter()); Ok(compressed_state_diff)
for forward-compatibility (if fields are added to the state diff) better to use ..
notation and explicitly replace specific compressed fields. WDYT?
Suggestion:
let alias_compressor = AliasCompressor { state, alias_contract_address };
let nonces: ... = state_diff.nonces.iter().map(|(address, nonce)| {
(alias_compressor.compress_address(address)?, *nonce)
}).collect();
let class_hashes: ... = state_diff.class_hashes.iter().map(|(address, class_hash)| {
(alias_compressor.compress_address(address)?, *class_hash)
}).collect();
let storage: ... = state_diff.storage.iter().map(|((address, key), value)| {
(
(
alias_compressor.compress_address(address)?,
alias_compressor.compress_storage_key(key, address)?,
),
*value,
)
}).collect();
Ok(StateMaps {
nonces,
class_hashes,
storage,
..state_diff
})
ff8e757
to
3bf82ca
Compare
af1d7cb
to
86efe96
Compare
There was a problem hiding this 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, 4 unresolved discussions (waiting on @dorimedini-starkware and @nimrod-starkware)
crates/blockifier/src/state/stateful_compression.rs
line 174 at r1 (raw file):
Previously, dorimedini-starkware wrote…
for forward-compatibility (if fields are added to the state diff) better to use
..
notation and explicitly replace specific compressed fields. WDYT?
Done.
(Without ?
in closures)
crates/blockifier/src/state/stateful_compression_test.rs
line 271 at r1 (raw file):
Previously, dorimedini-starkware wrote…
put large values here? to make sure these are not compressed, but for the correct reason
Done.
crates/blockifier/src/state/stateful_compression_test.rs
line at r1 (raw file):
Previously, dorimedini-starkware wrote…
can you implement a
decompress
test util and use it to compare original and compressed+decompressed diff?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @yoavGrs)
There was a problem hiding this 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 @Yoni-Starkware)
crates/blockifier/src/state/stateful_compression.rs
line 161 at r1 (raw file):
Previously, dorimedini-starkware wrote…
we always compress class hashes? @Yoni-Starkware @nimrod-starkware
In this code, we compress only the contract address.
3bf82ca
to
ff60390
Compare
86efe96
to
3a1c429
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @Yoni-Starkware)
crates/blockifier/src/state/stateful_compression.rs
line 161 at r1 (raw file):
Previously, yoavGrs wrote…
In this code, we compress only the contract address.
what I meant to ask: if only class hash changed, do we compress the contract address?
ff60390
to
ae7f69c
Compare
3a1c429
to
1644fd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @Yoni-Starkware)
There was a problem hiding this 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 @Yoni-Starkware)
crates/blockifier/src/state/stateful_compression.rs
line 161 at r1 (raw file):
Previously, dorimedini-starkware wrote…
what I meant to ask: if only class hash changed, do we compress the contract address?
This is what Nimrod wrote to me:
for address in address_to_class_hash.keys() | address_to_nonce.keys() | storage_updates.keys():
if address in storage_updates:
// allocate aliases for storage keys.
// allocate alias for address
There was a problem hiding this 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 @nimrod-starkware and @yoavGrs)
crates/blockifier/src/state/stateful_compression.rs
line 174 at r1 (raw file):
Previously, yoavGrs wrote…
Done.
(Without?
in closures)
@yoavGrs can you please follow Dori's suggestion re collect
instead of new
+ for
? (convention)
crates/blockifier/src/state/stateful_compression.rs
line 183 at r7 (raw file):
) -> CompressionResult<ContractAddress> { if contract_address.0 >= MIN_VALUE_FOR_ALIAS_ALLOC { Ok(self.get_alias(StorageKey(contract_address.0))?.try_into()?)
StorageKey -> AliasKey
ae7f69c
to
e41ec4a
Compare
1644fd5
to
f7862bb
Compare
There was a problem hiding this 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 r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @yoavGrs)
crates/blockifier/src/state/stateful_compression.rs
line 174 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
@yoavGrs can you please follow Dori's suggestion re
collect
instead ofnew
+for
? (convention)
@Yoni-Starkware issue is ?
within the iterator closures.
@yoavGrs you can collect::<Result<_, _>>()?
to solve this though (collect
is smart enough to collect an Iterator<Result<_, _>>
into a Result<_, _>
)
e41ec4a
to
cf2d3f0
Compare
f7862bb
to
edbf88b
Compare
cf2d3f0
to
01d697a
Compare
edbf88b
to
814abff
Compare
01d697a
to
7b96768
Compare
814abff
to
f46dab6
Compare
There was a problem hiding this 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 r11, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @yoavGrs)
c781a55
to
44a7c63
Compare
f46dab6
to
e25383e
Compare
44a7c63
to
43c73ff
Compare
e25383e
to
5433ec4
Compare
4f92b53
to
e6f35c8
Compare
There was a problem hiding this 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, 1 unresolved discussion (waiting on @dorimedini-starkware, @nimrod-starkware, and @Yoni-Starkware)
crates/blockifier/src/state/stateful_compression.rs
line 174 at r1 (raw file):
Previously, dorimedini-starkware wrote…
@Yoni-Starkware issue is
?
within the iterator closures.
@yoavGrs you cancollect::<Result<_, _>>()?
to solve this though (collect
is smart enough to collect anIterator<Result<_, _>>
into aResult<_, _>
)
Done!
I should return Result
from the closures, to allow using ?
within them.
crates/blockifier/src/state/stateful_compression.rs
line 183 at r7 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
StorageKey -> AliasKey
Compilation error: can't use a type alias as a constructor
e6f35c8
to
199ab60
Compare
There was a problem hiding this 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 r13, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware and @Yoni-Starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 2 files at r13.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)
No description provided.