feat(starknet_os): create panicking cached state #4554
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
nimrod-starkware
force-pushed
the
nimrod/hints/cached_state_input
branch
4 times, most recently
from
5074e9f to
609003c
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)
crates/starknet_os/src/hint_processor/execution_helper.rs line 48 at r2 (raw file):
.insert(class_hash, RunnableCompiledClass::V0(deprecated_class.try_into()?)); } // TODO(Nimrod): Fetch the actual sierra version somehow.
How can i get it? It's needed for the conversion, but maybe since we run in VM the it's not needed? Who should I ask?
Code quote:
// TODO(Nimrod): Fetch the actual sierra version somehow.crates/starknet_os/src/hint_processor/execution_helper.rs line 71 at r2 (raw file):
state_maps.compiled_class_hashes = state_input.class_hash_to_compiled_class_hash; // Insert anything to declared contracts???
?
Code quote:
// Insert anything to declared contracts???
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @nimrod-starkware)
crates/starknet_os/src/hint_processor/execution_helper.rs line 48 at r2 (raw file):
Previously, nimrod-starkware wrote…
How can i get it? It's needed for the conversion, but maybe since we run in VM the it's not needed? Who should I ask?
see below: change type to VersionedCasm to get the version
crates/starknet_os/src/hint_processor/execution_helper.rs line 71 at r2 (raw file):
Previously, nimrod-starkware wrote…
?
please discuss with yoni/noa - I doubt we need the declared classes mapping, but please understand what this mapping is for.
crates/starknet_os/src/io/os_input.rs line 91 at r2 (raw file):
pub(crate) class_hash_to_compiled_class_hash: HashMap<ClassHash, CompiledClassHash>, pub(crate) deprecated_compiled_classes: HashMap<ClassHash, ContractClass>, pub(crate) compiled_classes: HashMap<ClassHash, CasmContractClass>,
use this type instead
Suggestion:
pub(crate) compiled_classes: HashMap<ClassHash, VersionedCasm>,crates/starknet_os/src/hints/hint_implementation/deprecated_compiled_class.rs line 29 at r2 (raw file):
} }) .collect();
why is this change required? because this mapping is no longer in the OS input?
I think you can split this into a separate PR
Code quote:
pub(crate) fn load_deprecated_class_facts<S: StateReader>(
HintArgs { hint_processor, vm, exec_scopes, ids_data, ap_tracking, .. }: HintArgs<'_, S>,
) -> OsHintResult {
let deprecated_compiled_classes: HashMap<ClassHash, CompiledClassV0> = hint_processor
.execution_helper
.cached_state
.class_hash_to_class
.borrow()
.iter()
.filter_map(|(class_hash, class)| {
if let RunnableCompiledClass::V0(deprecated_class) = class {
Some((*class_hash, deprecated_class.clone()))
} else {
None
}
})
.collect();
nimrod-starkware
force-pushed
the
nimrod/hints/cached_state_input
branch
from
609003c to
cb0c0b6
Compare
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware)
crates/starknet_os/src/hint_processor/execution_helper.rs line 48 at r2 (raw file):
Previously, dorimedini-starkware wrote…
see below: change type to
VersionedCasmto get the version
Done.
crates/starknet_os/src/hints/hint_implementation/deprecated_compiled_class.rs line 29 at r2 (raw file):
Previously, dorimedini-starkware wrote…
why is this change required? because this mapping is no longer in the OS input?
I think you can split this into a separate PR
Yes, that's the reason, but will do it in a future PR. reverted it
crates/starknet_os/src/io/os_input.rs line 91 at r2 (raw file):
Previously, dorimedini-starkware wrote…
use this type instead
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)
crates/starknet_os/src/hint_processor/execution_helper.rs line 47 at r3 (raw file):
.insert(class_hash, RunnableCompiledClass::V0(deprecated_class.try_into()?)); } for (class_hash, class) in state_input.compiled_classes.into_iter() {
for clarity, non-blocking
Suggestion:
versioned_class
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/starknet_os/src/hint_processor/execution_helper.rs line 71 at r2 (raw file):
Previously, dorimedini-starkware wrote…
please discuss with yoni/noa - I doubt we need the declared classes mapping, but please understand what this mapping is for.
Talked to Noa, they needed it for the concurrency, it's used to indicate that a contract was declared. I don't think it's relevant as CachedState also holds the classes mapping. removed the comment.
crates/starknet_os/src/hint_processor/execution_helper.rs line 47 at r3 (raw file):
Previously, dorimedini-starkware wrote…
for clarity, non-blocking
Done
nimrod-starkware
force-pushed
the
nimrod/hints/cached_state_input
branch
from
cb0c0b6 to
c20b5ef
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)
crates/starknet_os/src/hint_processor/execution_helper.rs line 47 at r3 (raw file):
Previously, nimrod-starkware wrote…
Done
revert rename after you remove sierra version..
nimrod-starkware
force-pushed
the
nimrod/hints/cached_state_input
branch
from
c20b5ef to
5cec67a
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)
No description provided.