-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier: add virtual os program hash validations in the gateway and blockifier #11413
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
263cfa5 to
834863c
Compare
avivg-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.
@avivg-starkware reviewed 19 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @meship-starkware and @noaov1).
crates/blockifier/src/transaction/account_transaction.rs line 251 at r2 (raw file):
fn validate_proof_facts( &self, tx_context: &TransactionContext,
Do we expect we'll need anything else other than the version constants? Perhaps better to pass only them?
Code quote:
tx_context: &TransactionContext,crates/blockifier/src/transaction/account_transaction.rs line 262 at r2 (raw file):
// TODO(Meshi/ AvivG): add proof facts validations. // Validates the proof facts program hash.
Remove or clarify what's missing
Suggestion:
// Validates the proof facts program hash.crates/apollo_starknet_os_program/src/constants_test.rs line 64 at r2 (raw file):
fn class_hash_list_to_felt_list(class_hashes: &[ClassHash]) -> Vec<Felt> { class_hashes.iter().map(|class_hash| class_hash.0).collect() }
WDYT?
Suggestion (i):
fn stringify_class_hash_list<T>(name: &str, class_hashes: &[T]) -> String
where
T: AsRef<Felt>,
{
class_hashes
.iter()
.enumerate()
.map(|(i, class_hash)| {
let felt = class_hash.as_ref();
// If the line ends up longer than 100 chars, wrap the value in parenthesis, so the
// formatter can split the lines.
let line = format!("const {name}_{i} = {:#066x};", felt);
if line.len() > 100 {
format!("const {name}_{i} = ({:#066x});", felt)
} else {
line
}
})
.chain(std::iter::once(format!("const {name}_LEN = {};", class_hashes.len())))
.collect::<Vec<String>>()
.join("\n")
}Code snippet (ii):
crates/starknet_api/src/core.rs
impl AsRef<Felt> for ClassHash {
fn as_ref(&self) -> &Felt {
&self.0
}
}crates/apollo_starknet_os_program/src/constants_test.rs line 216 at r2 (raw file):
&class_hash_list_to_felt_list(&os_constants.data_gas_accounts) ), );
See the suggestion above
Code quote:
// Backward compatibility accounts.
V1_BOUND_ACCOUNTS_CAIRO0 = stringify_class_hash_list(
"V1_BOUND_ACCOUNTS_CAIRO0",
&class_hash_list_to_felt_list(&os_constants.v1_bound_accounts_cairo0)
),
V1_BOUND_ACCOUNTS_CAIRO1 = stringify_class_hash_list(
"V1_BOUND_ACCOUNTS_CAIRO1",
&class_hash_list_to_felt_list(&os_constants.v1_bound_accounts_cairo1)
),
V1_BOUND_ACCOUNTS_MAX_TIP =
format!("{:#?}", Felt::from(os_constants.v1_bound_accounts_max_tip)),
DATA_GAS_ACCOUNTS = stringify_class_hash_list(
"DATA_GAS_ACCOUNTS",
&class_hash_list_to_felt_list(&os_constants.data_gas_accounts)
),
);crates/starknet_api/src/test_utils.rs line 377 at r2 (raw file):
proof_facts![ felt!(VIRTUAL_SNOS), felt!("0x030206a40921880628605041292e995870334451179c63090221210893986a2"),
Can we use a named const? Perhaps add a test comparing this with the latest version constant?
WDYT? to force this const to be more dependent in the code
Code quote:
"0x030206a40921880628605041292e995870334451179c63090221210893986a2"crates/blockifier/src/blockifier_versioned_constants.rs line 102 at r2 (raw file):
pub struct RawOsConstants { // Allowed virtual OS program hashes for client-side proving. pub allowed_virtual_os_program_hashes: Vec<StarkHash>,
Why is this a list? Do we expect more will be added?
Code quote:
// Allowed virtual OS program hashes for client-side proving.
pub allowed_virtual_os_program_hashes: Vec<StarkHash>,834863c to
2317066
Compare
|
Please add
|
2317066 to
b89f810
Compare
meship-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.
@meship-starkware made 6 comments.
Reviewable status: 16 of 22 files reviewed, 6 unresolved discussions (waiting on @avivg-starkware and @noaov1).
crates/apollo_starknet_os_program/src/constants_test.rs line 64 at r2 (raw file):
Previously, avivg-starkware wrote…
WDYT?
Done.
crates/apollo_starknet_os_program/src/constants_test.rs line 216 at r2 (raw file):
Previously, avivg-starkware wrote…
See the suggestion above
Done.
crates/blockifier/src/blockifier_versioned_constants.rs line 102 at r2 (raw file):
Previously, avivg-starkware wrote…
Why is this a list? Do we expect more will be added?
Yes Yoni wanted to make sure we can have small fixes to the virtual os without releasing a new version
crates/blockifier/src/transaction/account_transaction.rs line 251 at r2 (raw file):
Previously, avivg-starkware wrote…
Do we expect we'll need anything else other than the version constants? Perhaps better to pass only them?
Done.
crates/blockifier/src/transaction/account_transaction.rs line 262 at r2 (raw file):
Previously, avivg-starkware wrote…
Remove or clarify what's missing
Done.
crates/starknet_api/src/test_utils.rs line 377 at r2 (raw file):
Previously, avivg-starkware wrote…
Can we use a named const? Perhaps add a test comparing this with the latest version constant?
WDYT? to force this const to be more dependent in the code
Done.
avivg-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.
@avivg-starkware reviewed 6 files and all commit messages, made 4 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1).
crates/starknet_api/src/test_utils.rs line 377 at r2 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Done.
Making sure not adding a test is intentional(?)
crates/apollo_starknet_os_program/src/constants_test.rs line 60 at r4 (raw file):
.collect::<Vec<String>>() .join("\n") }
See names suggestion (function name and args). Regarding the documentation, I'm not so sure... do however you see fit
Suggestion:
/// Create Rust `const` definitions from a list of hashes (rendered as `Felt`s).
///
/// Example:
/// ```
/// let hashes = [ClassHash(1), ClassHash(2)];
///
/// let s = format_hash_consts("X", &hashes, |h| &h.0);
///
/// let expected = "\
/// const X_0 = 0x0000000000000000000000000000000000000000000000000000000000000001;\
/// \nconst X_1 = 0x0000000000000000000000000000000000000000000000000000000000000002;\
/// \nconst X_LEN = 2;";
///
/// assert_eq!(s, expected);
/// ```
///
/// Conceptual output (simplified for readability):
/// ```text
/// const X_0 = 0x1;
/// const X_1 = 0x2;
/// const X_LEN = 2;
/// ```
fn format_hash_consts<T, F>(name: &str, hash_list: &[T], to_felt: F) -> String
where
F: Fn(&T) -> &Felt,
{
hash_list
.iter()
.enumerate()
.map(|(i, hash)| {
let felt = to_felt(hash);
// If the line ends up longer than 100 chars, wrap the value in parentheses so the
// formatter can split the line.
let const_def = format!("const {name}_{i} = {:#066x};", felt);
if const_def.len() > 100 {
format!("const {name}_{i} = ({:#066x});", felt)
} else {
const_def
}
})
.chain(std::iter::once(format!("const {name}_LEN = {};", hash_list.len())))
.collect::<Vec<_>>()
.join("\n")
}crates/apollo_starknet_os_program/src/constants_test.rs line 103 at r4 (raw file):
"ALLOWED_VIRTUAL_OS_PROGRAM_HASHES", &os_constants.allowed_virtual_os_program_hashes, |f| f
Suggestion:
|felt| feltcrates/apollo_starknet_os_program/src/constants_test.rs line 202 at r4 (raw file):
"V1_BOUND_ACCOUNTS_CAIRO0", &os_constants.v1_bound_accounts_cairo0, |ch| &ch.0
Suggestion:
|class_hash| &class_hash.0
avivg-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.
@avivg-starkware resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1).
b89f810 to
b1e68fa
Compare
meship-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.
@meship-starkware reviewed 13 files, made 1 comment, and resolved 3 discussions.
Reviewable status: 21 of 22 files reviewed, all discussions resolved (waiting on @avivg-starkware and @noaov1).
crates/starknet_api/src/test_utils.rs line 377 at r2 (raw file):
Previously, avivg-starkware wrote…
Making sure not adding a test is intentional(?)
Talked f2f, it will create circular dependency, and the test will fail if the VC is different for this constant
avivg-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.
@avivg-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1).
31449a8 to
0ad0944
Compare
noaov1
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.
@noaov1 reviewed 23 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @meship-starkware).
0ad0944 to
cff48fd
Compare
cff48fd to
6f33ffc
Compare

No description provided.