feat(papyrus_storage): get versioned casm #2420
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
AvivYossef-starkware
requested review from
alonh5,
dorimedini-starkware,
DvirYo-starkware,
TzahiTaub and
Yoni-Starkware
and removed request for
alonh5
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
4466854 to
c8f3cb5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2420 +/- ##
===========================================
+ Coverage 40.10% 77.22% +37.12%
===========================================
Files 26 393 +367
Lines 1895 42535 +40640
Branches 1895 42535 +40640
===========================================
+ Hits 760 32849 +32089
- Misses 1100 7389 +6289
- Partials 35 2297 +2262 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 66 at r1 (raw file):
/// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the Cairo assembly of a class and his sierra version given its Sierra class hash.
Suggestion:
itscrates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
} } Ok(None)
Suggestion:
match (self.get_casm(class_hash)?, self.get_class(class_hash)?) {
(Some(casm), Some(sierra)) => {
let sierra_version = SierraVersion::extract_from_program(&sierra.sierra_program)?;
Ok(Some((casm, sierra_version)))
}
(_, _) => Ok(None)
}crates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
} } Ok(None)
@AvivYossef-starkware will this function be called in the blockifier for every entry point in a running tx?
if so, @Yoni-Starkware is this an ok performance hit?
Code quote:
fn get_versioned_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<VersionedCasm>> {
// TODO(Aviv): Consider adding a dedicated table to store Sierra versions for better
// organization and performance.
if let Some(casm) = self.get_casm(class_hash)? {
if let Some(sierra) = self.get_class(class_hash)? {
let sierra_version = SierraVersion::extract_from_program(&sierra.sierra_program)?;
return Ok(Some((casm, sierra_version)));
}
}
Ok(None)crates/papyrus_storage/src/serialization/serializers.rs line 756 at r1 (raw file):
serde_json::from_value(serde_json::Value::deserialize_from(bytes)?).ok() } }
- this is not a primitive type, why is it in the primitive types section?
- why not use
auto_storage_serde!?
Code quote:
impl StorageSerde for SierraVersion {
fn serialize_into(&self, res: &mut impl std::io::Write) -> Result<(), StorageSerdeError> {
serde_json::to_value(self)?.serialize_into(res)
}
fn deserialize_from(bytes: &mut impl std::io::Read) -> Option<Self> {
serde_json::from_value(serde_json::Value::deserialize_from(bytes)?).ok()
}
}
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
c8f3cb5 to
002ea4b
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
Previously, dorimedini-starkware wrote…
@AvivYossef-starkware will this function be called in the blockifier for every entry point in a running tx?
if so, @Yoni-Starkware is this an ok performance hit?
No, it called only after Chache miss,
This struct has a cache of
RunnableCompiledClass and I'll add it also the SierraVersion
crates/papyrus_storage/src/serialization/serializers.rs line 756 at r1 (raw file):
Previously, dorimedini-starkware wrote…
- this is not a primitive type, why is it in the primitive types section?
- why not use
auto_storage_serde!?
For using auto_storage_serde! I should copy the struct definition.
crates/papyrus_storage/src/compiled_class.rs line 66 at r1 (raw file):
/// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the Cairo assembly of a class and his sierra version given its Sierra class hash.
Done.
crates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
} } Ok(None)
Done.
There was a problem hiding this comment.
You need to support get_sierra
Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
There was a problem hiding this comment.
@avi-starkware FYI
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/papyrus_storage/src/serialization/serializers.rs line 756 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
For using
auto_storage_serde!I should copy the struct definition.
this is the standard practice in this module iiuc
AvivYossef-starkware
force-pushed
the
aviv/add_sierra_version
branch
2 times, most recently
from
1bbe7f1 to
aa5b760
Compare
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
e0a4c01 to
865d1dc
Compare
There was a problem hiding this comment.
Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/papyrus_storage/src/compiled_class.rs line 66 at r3 (raw file):
/// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the Cairo assembly of a class and its sierra contract class given its Sierra class hash.
Suggestion:
d its Sierra con
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
865d1dc to
2fdc7e4
Compare
AvivYossef-starkware
changed the base branch from
aviv/add_sierra_version
to
aviv/refactor_default_sierra_contract_class
There was a problem hiding this comment.
Reviewable status: 6 of 11 files reviewed, 9 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class_test.rs line 48 at r12 (raw file):
Previously, DvirYo-starkware wrote…
do like
LocationInFile::get\_test\_instance(&mut get\_rng());
Done.
crates/papyrus_storage/src/compiled_class_test.rs line 58 at r12 (raw file):
Previously, DvirYo-starkware wrote…
Use
get_test_instancealso for Sierra. The fact that it is an invalid object doesn't matter here. If you have a very strong opinion use the Default
Done
crates/papyrus_storage/src/compiled_class_test.rs line 88 at r12 (raw file):
Previously, dorimedini-starkware wrote…
necessary?
No
crates/papyrus_storage/src/compiled_class_test.rs line 99 at r12 (raw file):
Previously, DvirYo-starkware wrote…
why read here again?
removed
crates/papyrus_storage/src/compiled_class_test.rs line 90 at r12 (raw file):
dbg!(&result); // If Sierra XOR CASM exists assert!(matches!(
Removed
There was a problem hiding this comment.
Reviewed 5 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @AvivYossef-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class_test.rs line 40 at r13 (raw file):
#[case::only_sierra_exists(false, true)] #[case::neither_exists(false, false)] fn test_casm_and_sierra(#[case] has_casm: bool, #[case] has_sierra: bool) {
now you can use cartesian testing :)
non-blocking
Suggestion:
fn test_casm_and_sierra(#[values(true, false)] has_casm: bool, #[values(true, false)] has_sierra: bool) {crates/papyrus_storage/src/compiled_class.rs line 103 at r13 (raw file):
let casm = self.get_casm(class_hash)?; let sierra = self.get_class(class_hash)?; Ok((casm, sierra))
non-blocking
Suggestion:
Ok((self.get_casm(class_hash)?, self.get_class(class_hash)?))
There was a problem hiding this comment.
Must have a Py side here, the get_sierra_and_casm should break python.
Reviewed 1 of 1 files at r9, 1 of 5 files at r13.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/papyrus_storage/src/compiled_class.rs line 69 at r13 (raw file):
/// If both exist, returns `(Some(casm), Some(sierra))`. If neither, returns `(None, None)`. /// If only one exists, returns `(Some, None)` or `(None, Some)`. fn get_casm_and_sierra(
Should we use your util inside this func and let it return
Option<(CasmContractClass, SierraContractClass)>
?
Code quote:
/// Returns the CASM and Sierra contract classes for the given hash.
/// If both exist, returns `(Some(casm), Some(sierra))`. If neither, returns `(None, None)`.
/// If only one exists, returns `(Some, None)` or `(None, Some)`.
fn get_casm_and_sierra(crates/papyrus_state_reader/src/papyrus_state.rs line 62 at r13 (raw file):
if class_is_declared { let (option_casm_compiled_class, option_sierra) = self
Suggestion:
let (option_casm, opcrates/blockifier/src/state/errors.rs line 39 at r13 (raw file):
/// Converts CASM and Sierra options into a Result, raising a `CasmAndSierraMismatch` error /// when there is an inconsistency. pub fn validate_casm_and_sierra(
Suggestion:
/// Ensures that the CASM and Sierra classes are coupled - Meaning that they both exist or are missing.
/// Returns a `CasmAndSierraMismatch` error when there is an inconsistency in their existence.
pub fn couple_casm_and_sierra(crates/blockifier/src/state/errors.rs line 42 at r13 (raw file):
class_hash: ClassHash, casm_opt: Option<CasmContractClass>, sierra_opt: Option<SierraContractClass>,
Be consistent
Suggestion:
option_casm: Option<CasmContractClass>,
option_sierra: Option<SierraContractClass>,
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
7763b88 to
0999dfe
Compare
There was a problem hiding this comment.
Reviewable status: 5 of 11 files reviewed, 9 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 69 at r13 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Should we use your util inside this func and let it return
Option<(CasmContractClass, SierraContractClass)>
?
This function is defined in Papyrus Storage and returns a StorageResult.
@DvirYo-starkware mentioned that if either Sierra or Casm exists independently, it should not be considered a storage error since the storage allows them to be saved independently. Therefore, I use this utility in functions that return a StateResult.
crates/blockifier/src/state/errors.rs line 39 at r13 (raw file):
/// Converts CASM and Sierra options into a Result, raising a `CasmAndSierraMismatch` error /// when there is an inconsistency. pub fn validate_casm_and_sierra(
Done
crates/papyrus_state_reader/src/papyrus_state.rs line 62 at r13 (raw file):
if class_is_declared { let (option_casm_compiled_class, option_sierra) = self
Done.
There was a problem hiding this comment.
Reviewed 6 of 6 files at r14, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewed 1 of 6 files at r14, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/blockifier/src/state/errors.rs line 39 at r13 (raw file):
Previously, AvivYossef-starkware wrote…
Done
Rename the func name as well
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
0637840 to
447a92d
Compare
There was a problem hiding this comment.
Why?
Reviewable status: 8 of 11 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status: 8 of 11 files reviewed, 7 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/blockifier/src/state/errors.rs line 39 at r13 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Rename the func name as well
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewed 1 of 6 files at r14, 1 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class_test.rs line 46 at r15 (raw file):
let ((reader, mut writer), _temp_dir) = get_test_storage(); let expected_casm = CasmContractClass::get_test_instance(&mut rng); let expected_sierra = <SierraContractClass as GetTestInstance>::get_test_instance(&mut rng);
Why do you need the <SierraContractClass as GetTestInstance>? I think doing the same as in the CASM case will be enough
crates/papyrus_storage/src/compiled_class.rs line 65 at r15 (raw file):
pub trait CasmStorageReader { /// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>;
Add a new line here
crates/papyrus_storage/src/compiled_class.rs line 67 at r15 (raw file):
fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the CASM and Sierra contract classes for the given hash. /// If both exist, returns `(Some(casm), Some(sierra))`. If neither, returns `(None, None)`.
Consider each if in a new line
crates/papyrus_storage/src/compiled_class.rs line 97 at r15 (raw file):
} fn get_casm_and_sierra(
Just to make it clear, the call
reader.get_casm_and_sierra(class_hash)?
is totally equivalent to
(reader.get_casm(class_hash)?, reader.get_sierra(class_hash)?)
If I understand it correctly, I don't see any reason for this function. @Yoni-Starkware, if you approve, I'm fine with adding this.
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
ceffe13 to
12d69a6
Compare
There was a problem hiding this comment.
Reviewable status: 9 of 11 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 97 at r15 (raw file):
Previously, DvirYo-starkware wrote…
Just to make it clear, the call
reader.get_casm_and_sierra(class_hash)?
is totally equivalent to
(reader.get_casm(class_hash)?, reader.get_sierra(class_hash)?)If I understand it correctly, I don't see any reason for this function. @Yoni-Starkware, if you approve, I'm fine with adding this.
Yes, its the same.
crates/papyrus_storage/src/compiled_class_test.rs line 46 at r15 (raw file):
Previously, DvirYo-starkware wrote…
Why do you need the
<SierraContractClass as GetTestInstance>? I think doing the same as in the CASM case will be enough
The compiler is uncertain whether to use the function defined in GetTestInstance or another option. Therefore, I needed to add it.
Yoni-Starkware
requested review from
dorimedini-starkware,
DvirYo-starkware and
Yoni-Starkware
There was a problem hiding this comment.
Reviewed 1 of 6 files at r14, 2 of 3 files at r15, 1 of 1 files at r16.
Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware and @TzahiTaub)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status:
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
12d69a6 to
1236dca
Compare
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
1236dca to
4471084
Compare
No description provided.