blockifier: set missing config validations#12695
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
e19f5e5 to
f250931
Compare
e3dc405 to
16ac2a2
Compare
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on ArniStarkware and nadin-Starkware).
crates/blockifier/Cargo.toml line 49 at r1 (raw file):
cairo-native = { workspace = true, optional = true } cairo-vm.workspace = true dashmap.workspace = true
Why is dashmap needed?
Code quote:
dashmap.workspace = truecrates/blockifier/src/blockifier/config.rs line 143 at r1 (raw file):
pub struct ContractClassManagerConfig { pub cairo_native_run_config: CairoNativeRunConfig, #[validate(range(min = 1, message = "`contract_cache_size` must be greater than 0"))]
Can we just have an assertion instead?
(In the constructor of the class manager)
Code quote:
#[validate(range(min = 1, message = "`contract_cache_size` must be greater than 0"))]|
Previously, avi-starkware (Avi Cohen) wrote…
is was moved (alphabetized). |
|
@avi-starkware, a short explanation regarding the |
f250931 to
2450e69
Compare
16ac2a2 to
ef2714e
Compare
ArniStarkware
left a comment
There was a problem hiding this comment.
@ArniStarkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on avi-starkware and nadin-Starkware).
crates/blockifier/src/blockifier/config.rs line 143 at r1 (raw file):
Previously, avi-starkware (Avi Cohen) wrote…
Can we just have an assertion instead?
(In the constructor of the class manager)
I can remove this validation. We can discuss it in a different context.
It never existed, and it worked just fine.
|
Previously, ArniStarkware (Arnon Hod) wrote…
reverted. |
2450e69 to
2d428b5
Compare
ef2714e to
cd4cd2f
Compare
2d428b5 to
ca8c30c
Compare
cd4cd2f to
5047911
Compare
5047911 to
432618e
Compare
avi-starkware
left a comment
There was a problem hiding this comment.
@avi-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on nadin-Starkware).
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArniStarkware and nadin-Starkware).
crates/apollo_batcher_config/src/config.rs line 240 at r2 (raw file):
pub contract_class_manager_config: ContractClassManagerConfig, pub commitment_manager_config: CommitmentManagerConfig, pub max_l1_handler_txs_per_block_proposal: usize,
Why aren't all the fields validated?
Code quote:
pub struct BatcherStaticConfig {
// TODO(Arni): Set nested validation for storage config.
pub storage: StorageConfig,
pub outstream_content_buffer_size: usize,
pub input_stream_content_buffer_size: usize,
pub block_builder_config: BlockBuilderConfig,
pub pre_confirmed_block_writer_config: PreconfirmedBlockWriterConfig,
#[validate(nested)]
pub contract_class_manager_config: ContractClassManagerConfig,
pub commitment_manager_config: CommitmentManagerConfig,
pub max_l1_handler_txs_per_block_proposal: usize,|
Previously, Itay-Tsabary-Starkware wrote…
Spectacular question. I partially answered it in #12703. |
|
Previously, ArniStarkware (Arnon Hod) wrote…
|
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
@Itay-Tsabary-Starkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on nadin-Starkware).
Itay-Tsabary-Starkware
left a comment
There was a problem hiding this comment.
@Itay-Tsabary-Starkware reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on nadin-Starkware).

Note
Low Risk
Config-only validation changes; main risk is newly enforced validation causing previously-accepted configs to fail at startup.
Overview
Adds missing
validatorsupport and nested validation to ensureContractClassManagerConfigand itsnative_compiler_configare validated when included in higher-level configs.Updates
BatcherStaticConfigandGatewayStaticConfigto markcontract_class_manager_configas#[validate(nested)], and updatesblockifierto deriveValidateforContractClassManagerConfig(plus adds thevalidatordependency).Written by Cursor Bugbot for commit 432618e. This will update automatically on new commits. Configure here.