-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_os_runner: aviv refactor the rpc state reader to compute the compiled class hash #11302
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
starknet_os_runner: aviv refactor the rpc state reader to compute the compiled class hash #11302
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
4840bd7 to
393f1fb
Compare
5312554 to
731f5de
Compare
393f1fb to
b7e8194
Compare
731f5de to
b6c4c9b
Compare
078c1d2 to
37f3cf2
Compare
b6c4c9b to
e4db4a2
Compare
e4db4a2 to
d76b6ad
Compare
37f3cf2 to
5b82bfd
Compare
d76b6ad to
6b263bc
Compare
5b82bfd to
29c4a84
Compare
avi-starkware
left a comment
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.
@avi-starkware reviewed 2 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 164 at r2 (raw file):
/// Use the default implementation of get_compiled_class_hash_v2. /// Which compute the hash.
This doesn't mean much to people who don't have the context.
Why not keep this dockstring as it was?
Code quote:
/// Use the default implementation of get_compiled_class_hash_v2.
/// Which compute the hash.crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 292 at r2 (raw file):
let mut vc = VersionedConstants::get(&self.get_starknet_version()?)?.clone(); // The rpc state reader does not support casm hash migration, which requires compiled class // hashes. .
Suggestion:
// Casm hash migration is not supported. It requires compiled class hashes, and the RPC state reader does not have them.crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 333 at r2 (raw file):
/// Get the commitment state diff of the current block. /// Notice: we ignore migrated_compiled_classes.
Suggestion:
/// Get the commitment state diff of the current block.
/// Note: the commitment state diff does not include migrated_compiled_classes.crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 164 at r2 (raw file):
/// need to be migrated from v1 to v2 format. /// In reexecution we use a dummy value for both get_compiled_class_hash and /// get_compiled_class_hash_v2, to avoid the migration process.
Why is this no longer true?
Code quote:
/// Returns a dummy compiled class hash for reexecution purposes.
///
/// This method is required since v0.14.1 for checking if compiled class hashes
/// need to be migrated from v1 to v2 format.
/// In reexecution we use a dummy value for both get_compiled_class_hash and
/// get_compiled_class_hash_v2, to avoid the migration process.6b263bc to
73f5b9d
Compare
29c4a84 to
34ccb7e
Compare
AvivYossef-starkware
left a comment
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.
@AvivYossef-starkware made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avi-starkware and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 164 at r2 (raw file):
Previously, avi-starkware (Avi Cohen) wrote…
Why is this no longer true?
since now it is unimplemented for the get_compiled_class_hash
and returns the actual v2 hash
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 164 at r2 (raw file):
Previously, avi-starkware (Avi Cohen) wrote…
This doesn't mean much to people who don't have the context.
Why not keep this dockstring as it was?
Since now it does not return the same value as the get compiled hash, which is unimplemented.
It just computes it
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 292 at r2 (raw file):
let mut vc = VersionedConstants::get(&self.get_starknet_version()?)?.clone(); // The rpc state reader does not support casm hash migration, which requires compiled class // hashes. .
Done.
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 333 at r2 (raw file):
/// Get the commitment state diff of the current block. /// Notice: we ignore migrated_compiled_classes.
Done.
avi-starkware
left a comment
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.
@avi-starkware reviewed 3 files and all commit messages, made 2 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @noaov1).
crates/blockifier_reexecution/src/state_reader/rpc_state_reader.rs line 164 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
Since now it does not return the same value as the get compiled hash, which is unimplemented.
It just computes it
Let's remove the docstring if that is the case
(the function does exactly what its name says it does)
avi-starkware
left a comment
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 @AvivYossef-starkware and @noaov1).
34ccb7e to
97378aa
Compare
73f5b9d to
99daea0
Compare
99daea0 to
a4817b4
Compare
97378aa to
d3e5d61
Compare
Merge activity
|
d3e5d61 to
da38f4f
Compare
AvivYossef-starkware
left a comment
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.
@AvivYossef-starkware reviewed 1 file and resolved 1 discussion.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @noaov1).
AvivYossef-starkware
left a comment
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.
@AvivYossef-starkware reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1).
… compiled class hash
da38f4f to
75355c7
Compare
AvivYossef-starkware
left a comment
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.
@AvivYossef-starkware reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1).

No description provided.