Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

feat: add VersionedConstants #1348

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Conversation

giladchase
Copy link
Collaborator

@giladchase giladchase commented Jan 18, 2024

  • Currently only covers the constants previously contained in BlockInfo.
    TODO: Add check if this covers all chain ids, which might have different contstants.
  • Remove BlockContextArgs: no longer needed now that BlockContext can be initialized by 3 args of public types.

Python: https://reviewable.io/reviews/starkware-industries/starkware/33539

This change is Reviewable

@giladchase giladchase changed the title tmp Draft: add VersionedConstants Jan 18, 2024
@giladchase giladchase force-pushed the gilad/add-blockifier-config branch 3 times, most recently from 9c8970c to 5a2ac40 Compare January 18, 2024 11:20
Base automatically changed from gilad/chain-info to main January 18, 2024 12:32
@giladchase giladchase force-pushed the gilad/add-blockifier-config branch 2 times, most recently from 6aeec46 to 3efad5f Compare January 22, 2024 09:11
@giladchase giladchase changed the title Draft: add VersionedConstants Add VersionedConstants Jan 22, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2024

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (e5c746b) 70.57% compared to head (ef3526a) 70.64%.

Files Patch % Lines
crates/blockifier/src/versioned_constants.rs 58.33% 4 Missing and 1 partial ⚠️
crates/native_blockifier/src/py_validator.rs 0.00% 5 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs 72.72% 3 Missing ⚠️
crates/native_blockifier/src/py_utils.rs 0.00% 3 Missing ⚠️
crates/blockifier/src/fee/fee_utils.rs 60.00% 0 Missing and 2 partials ⚠️
crates/blockifier/src/execution/entry_point.rs 90.00% 0 Missing and 1 partial ⚠️
crates/blockifier/src/fee/fee_checks.rs 50.00% 0 Missing and 1 partial ⚠️
crates/blockifier/src/fee/gas_usage.rs 75.00% 0 Missing and 1 partial ⚠️
...ates/native_blockifier/src/transaction_executor.rs 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           main-v0.13.1    #1348      +/-   ##
================================================
+ Coverage         70.57%   70.64%   +0.07%     
================================================
  Files                59       60       +1     
  Lines              7813     7785      -28     
  Branches           7813     7785      -28     
================================================
- Hits               5514     5500      -14     
+ Misses             1871     1856      -15     
- Partials            428      429       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@giladchase giladchase changed the title Add VersionedConstants feat: add VersionedConstants Jan 22, 2024
@giladchase giladchase force-pushed the gilad/add-blockifier-config branch 4 times, most recently from 0716d4d to 4ad3696 Compare January 23, 2024 07:45
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 24 files at r1, 1 of 3 files at r2, all commit messages.
Reviewable status: 7 of 24 files reviewed, 5 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/resources/versioned_constants.json line 1 at r2 (raw file):

{

Can you please reference the origin of these values?


crates/blockifier/src/versioned_constants.rs line 26 at r2 (raw file):

impl VersionedConstants {
    pub fn latest_constants() -> &'static Self {

WDYM?

Code quote:

latest

crates/blockifier/src/versioned_constants.rs line 34 at r2 (raw file):

    type Error = VersionedConstantsError;

    fn try_from(path: &Path) -> Result<Self, Self::Error> {

Feels like a generic impl. and one that we already have; is this the case?

Code quote:

try_from

crates/blockifier/src/versioned_constants.rs line 35 at r2 (raw file):

    fn try_from(path: &Path) -> Result<Self, Self::Error> {
        let config_raw = std::fs::read_to_string(path)?;

Suggestion:

raw_constants

crates/blockifier/src/versioned_constants.rs line 41 at r2 (raw file):

#[derive(Debug, Error)]
pub enum VersionedConstantsError {

Please alphabetize; also, do we usually add a blank line between variants?

Code quote:

VersionedConstantsError

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 7 of 24 files reviewed, 3 unresolved discussions (waiting on @elintul and @noaov1)


crates/blockifier/resources/versioned_constants.json line 1 at r2 (raw file):

Previously, elintul (Elin) wrote…

Can you please reference the origin of these values?

Sure: you can see them in DEFAULT_CAIRO_RESOURCE_FEE_WEIGHTS in Python, they are the defaults.
Also in the same module you have build_general_config which is the one used instead of these defaults, and which calculates the exact same modules (i calculated it manually).
(Note that build_general_config is a bit funky, it deep_copies the argument then modifies it and returns it.)


crates/blockifier/src/versioned_constants.rs line 26 at r2 (raw file):

Previously, elintul (Elin) wrote…

WDYM?

Probably not the best of names 😅
Basically in the resources directory you'll have one file labeld verrsioned_constants.json and as we progress in blockifier versions, versioned_constants_0_13_1.json, versioned_constants_0_13_2.json, versioned_constants_0_14_0.json and so on.

In particular, there will always be a single versioned_constants.json in that directory, which i call latest, and which symbolizes the constants that the current version of blockifier has shipped with.

For example, when we publish blockifier version 1.2.3, versioned_constants.json will be the constants that are the "defaults" of this version.
I could have called this default_constants_for_current_blockifier_version, but default too much implies a trivial constructor in rust (with 0 values in everything), so i decided against it.

I'm not sure i explained it properly, i'll ping you offline if it's indeed the case.


crates/blockifier/src/versioned_constants.rs line 34 at r2 (raw file):

Previously, elintul (Elin) wrote…

Feels like a generic impl. and one that we already have; is this the case?

Can you explain a bit more what you mean plz?
I'm not sure what "feels like a generic impl" means here.


crates/blockifier/src/versioned_constants.rs line 41 at r2 (raw file):

Previously, elintul (Elin) wrote…

Please alphabetize; also, do we usually add a blank line between variants?

Done, and no :) so i removed blanks.
I also removed an unused variant (it remained from my attempt at support multiple chains.

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 15 of 24 files at r1, 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/resources/versioned_constants.json line 2 at r3 (raw file):

{
    "vm_resource_fee_cost": {

Please alphabetize the builtins.

Code quote:

vm_resource_fee_cost

crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

    },
    "invoke_tx_max_n_steps": 3000000,
    "validate_max_n_steps": 1000000,

And these?

Code quote:

    "invoke_tx_max_n_steps": 3000000,
    "validate_max_n_steps": 1000000,

crates/blockifier/src/versioned_constants.rs line 26 at r2 (raw file):

Previously, giladchase wrote…

Probably not the best of names 😅
Basically in the resources directory you'll have one file labeld verrsioned_constants.json and as we progress in blockifier versions, versioned_constants_0_13_1.json, versioned_constants_0_13_2.json, versioned_constants_0_14_0.json and so on.

In particular, there will always be a single versioned_constants.json in that directory, which i call latest, and which symbolizes the constants that the current version of blockifier has shipped with.

For example, when we publish blockifier version 1.2.3, versioned_constants.json will be the constants that are the "defaults" of this version.
I could have called this default_constants_for_current_blockifier_version, but default too much implies a trivial constructor in rust (with 0 values in everything), so i decided against it.

I'm not sure i explained it properly, i'll ping you offline if it's indeed the case.

I understood, consider adding a short docstring to this method.


crates/blockifier/src/versioned_constants.rs line 34 at r2 (raw file):

Previously, giladchase wrote…

Can you explain a bit more what you mean plz?
I'm not sure what "feels like a generic impl" means here.

Reading from a file and deserializing.


crates/blockifier/src/versioned_constants.rs line 12 at r3 (raw file):

const DEFAULT_CONSTANTS_JSON: &str = include_str!("../resources/versioned_constants.json");
static DEFAULT_CONSTANTS: Lazy<VersionedConstants> =
    Lazy::new(|| serde_json::from_str(DEFAULT_CONSTANTS_JSON).expect("malformed config.json"));

Please capitalize and phrase as a sentence.

Code quote:

malformed config.json

crates/blockifier/src/execution/entry_point.rs line 73 at r3 (raw file):

        let mut decrement_when_dropped = RecursionDepthGuard::new(
            context.current_recursion_depth.clone(),
            context.block_context.versioned_constants.max_recursion_depth,

In your feature branch (that contains the whole feature): how many a.b.c.ds do you have? Should we consider encapsulating some of them with getters?

Code quote:

context.block_context.versioned_constants

crates/native_blockifier/src/py_block_executor.rs line 291 at r3 (raw file):

    general_config: &PyGeneralConfig,
    block_info: PyBlockInfo,
) -> NativeBlockifierResult<BlockContext> {

Why so many clone()s? :(


crates/native_blockifier/src/py_validator.rs line 25 at r3 (raw file):

#[pyclass]
pub struct PyValidator {
    pub general_config: PyGeneralConfig,

How come this is not needed?

Code quote:

pub general_config: PyGeneralConfig,

Copy link
Collaborator

@elintul elintul left a 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, 9 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/resources/versioned_constants.json line 16 at r2 (raw file):

    "invoke_tx_max_n_steps": 3000000,
    "validate_max_n_steps": 1000000,
    "max_recursion_depth": 50

Not sure we want it here; it's not really versioned, but should stay configurable.

Code quote:

"max_recursion_depth": 50

Copy link
Collaborator Author

@giladchase giladchase left a 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, 6 unresolved discussions (waiting on @elintul and @noaov1)


crates/blockifier/resources/versioned_constants.json line 16 at r2 (raw file):

Previously, elintul (Elin) wrote…

Not sure we want it here; it's not really versioned, but should stay configurable.

Talked offline, keeping this here and overriding in native blockifier


crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

Previously, elintul (Elin) wrote…

And these?

Done.


crates/blockifier/src/versioned_constants.rs line 34 at r2 (raw file):

Previously, elintul (Elin) wrote…

Reading from a file and deserializing.

Ohhh good point.
I found this, which i think just might be better than wrangling strings, WDYT?

        // let raw_constants = std::fs::read_to_string(path)?;
        // Ok(serde_json::from_str(&raw_constants)?)

        Ok(serde_json::from_reader(std::fs::File::open(path)?)?)

This skips the unnecessary string generation.


crates/blockifier/src/execution/entry_point.rs line 73 at r3 (raw file):

Previously, elintul (Elin) wrote…

In your feature branch (that contains the whole feature): how many a.b.c.ds do you have? Should we consider encapsulating some of them with getters?

Not a lot, most of the times at least part of the chain is accessed twice in the same scope, so i define a variable like

let version_constants = context.block_context.versioned_constant

and use that instead of these chains.

A lot of accesses are removed in the next PRs (the refactor PRs).
Whatever's left after the PRs should be handled with getters, like you suggested.


crates/native_blockifier/src/py_block_executor.rs line 291 at r3 (raw file):

Previously, elintul (Elin) wrote…

Why so many clone()s? :(

First one (starknet_os_config): is because ChainInfo wants to own its fields; it's fields are small enough to not warrent an Arc, and adding a reference to this struct will require a lifetime, which, for a context struct, will pollute a lot of the methods with lifetimes.

Second one: (chain_info: chain_info.clone()) that's a bug, unnecessary clone, removed. Thanks for spotting 🙏

Thid one (versioned_constants): BlockContext wants to own this instance.
We can make this Arc: I don't think this is warrent, because the instance is almost a Copy; it has 3 Copy fields and one Arc field, which is cheapish to clone (but not Copy-cheap).
I moved the instance to the py_block_executor because of the override support i added for max_recursion, the copy is still necessary though, for the same reason.


crates/native_blockifier/src/py_validator.rs line 25 at r3 (raw file):

Previously, elintul (Elin) wrote…

How come this is not needed?

Was unused even before this change 😅

This is residue from the port from block_executor: the block executor recreates the TransactionExecutor for every block, so it needs the general_config on hand to recreate it (instead of passing it in setup_block_execution every time).

The validator has a single tx_executor all the time, so it doesn't need to save the general config, because the tx executor isn't reinitialized all the time.

@giladchase giladchase force-pushed the gilad/add-blockifier-config branch 2 times, most recently from 1f59fa5 to 81ade96 Compare January 24, 2024 09:54
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 5 files at r4, 5 of 6 files at r5, all commit messages.
Reviewable status: 21 of 24 files reviewed, 5 unresolved discussions (waiting on @giladchase and @noaov1)


crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

Previously, giladchase wrote…

Done.

Where did they come from?


crates/blockifier/src/versioned_constants.rs line 13 at r4 (raw file):

static DEFAULT_CONSTANTS: Lazy<VersionedConstants> = Lazy::new(|| {
    serde_json::from_str(DEFAULT_CONSTANTS_JSON)
        .expect("Versioned constants json file is malformed")

Suggestion:

JSON

crates/blockifier/src/execution/entry_point.rs line 73 at r3 (raw file):

Previously, giladchase wrote…

Not a lot, most of the times at least part of the chain is accessed twice in the same scope, so i define a variable like

let version_constants = context.block_context.versioned_constant

and use that instead of these chains.

A lot of accesses are removed in the next PRs (the refactor PRs).
Whatever's left after the PRs should be handled with getters, like you suggested.

Can you please add this as a task on Monday so we won't forget? Not urgent for 0.13.1, obviously.

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 21 of 24 files reviewed, 6 unresolved discussions (waiting on @elintul and @noaov1)


crates/blockifier/src/versioned_constants.rs line 19 at r5 (raw file):

pub struct VersionedConstants {
    // Fee related.
    pub vm_resource_fee_cost: Arc<HashMap<String, f64>>,

Should this be pub vm_resource_fee_cost: ResourceCosts (where ResourceCosts contains the 8 values in the json)?
I think it might be better, because the HashMap implies that this has a dynamic structure, even though (IIUC) the keys in this map change very rarely and we won't have too many of them.

Code quote:

    pub vm_resource_fee_cost: Arc<HashMap<String, f64>>,

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 13 of 13 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1)

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 24 files at r1, 2 of 3 files at r2, 1 of 5 files at r4, 1 of 6 files at r5, 2 of 4 files at r6, 13 of 13 files at r7, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @elintul and @giladchase)


crates/blockifier/BUILD line 7 at r7 (raw file):

    name="blockifier",
    srcs=glob(["src/**/*.rs"]),
    data=glob(["resources/**/*"]),

Can you please explain this change? :)

Code quote:

    data=glob(["resources/**/*"]),

crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

Previously, giladchase wrote…

Yep, and as far as i can find also the values for other chains, but i didn't do a thorough check

Why do here we set invoke_tx_max_n_steps: 1000000?


crates/blockifier/resources/versioned_constants.json line 1 at r7 (raw file):

{

Q- Why is this file located here and not under the versioned_constants module?


crates/blockifier/src/execution/entry_point.rs line 225 at r7 (raw file):

            ExecutionMode::Execute => min(
                constants.invoke_tx_max_n_steps,
                default_versioned_constants.invoke_tx_max_n_steps,

Why is the min needed? Isn't it the same value? Will we allow to override this value?
Wouldn't that be problematic for a full node re-executing transactions from older versions?

Code quote:

            ExecutionMode::Execute => min(
                constants.invoke_tx_max_n_steps,
                default_versioned_constants.invoke_tx_max_n_steps,

crates/native_blockifier/src/py_validator.rs line 48 at r7 (raw file):

            &general_config,
            // TODO(Gilad): add max_validate_n_steps override argument and override here.
            &versioned_constants,

Can you please explain why? :)

Code quote:

            // TODO(Gilad): add max_validate_n_steps override argument and override here.
            &versioned_constants,

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 23 of 25 files reviewed, 5 unresolved discussions (waiting on @elintul and @noaov1)


Cargo.toml line 55 at r7 (raw file):

tempfile = "3.7.0"
test-case = "2.2.2"
thiserror = "1.0.37"

alphabetized + added once_cell for Lazy

Code quote:

once_cell = "1.19.0"
papyrus_storage = "0.3.0-dev.0"
phf = { version = "0.11", features = ["macros"] }
pretty_assertions = "1.2.1"
pyo3 = "0.19.1"
pyo3-log = "0.8.1"
rstest = "0.17.0"
serde = "1.0.184"
serde_json = "1.0.81"
sha3 = "0.10.6"
starknet-crypto = "0.5.1"
starknet_api = "0.7.0-dev.0"
strum = "0.24.1"
strum_macros = "0.24.3"
tempfile = "3.7.0"
test-case = "2.2.2"
thiserror = "1.0.37"

crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why do here we set invoke_tx_max_n_steps: 1000000?

Not sure really, the values might vary between chains, and some might be stale and always overwritten.
I used 3000000 because that seemed to be the default value for general_config, and the expected ymls from the pyconf test also seemed to agree.

There's a task to go over all chains and compare values, which i'm doing concurrently to this, which was agreed to be mainnet values.

I think mainnet uses the default value (3000000) because its preset doesn't seem to Replace the default value above.
I can't really check for sure cause i don't have mainnet access.


crates/blockifier/resources/versioned_constants.json line 1 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Q- Why is this file located here and not under the versioned_constants module?

The resources dir is typically where ppl put non-code stuff, that is intended to be consumed during execution.
Putting it anywhere inside src could be considered a bit inappropriate, since it isn't source code, it's static.

Moreover, since this file should be easily accessible for users, and even allow them to store more similar files in this directory in addition to this one for quick switching between constants, a central location like a dedicated dir at the root of the crate is a spot for this.
The alternative would be to send the users to dig into our code, which is less convenient (especially for product ppl who just want to know prices of stuff and don't want to dig around code).


crates/blockifier/src/versioned_constants.rs line 13 at r4 (raw file):

static DEFAULT_CONSTANTS: Lazy<VersionedConstants> = Lazy::new(|| {
    serde_json::from_str(DEFAULT_CONSTANTS_JSON)
        .expect("Versioned constants json file is malformed")

Done, srry about the delay, i fixed it and must have lost it in a rebase.


crates/blockifier/src/execution/entry_point.rs line 225 at r7 (raw file):

Why is the min needed? Isn't it the same value?

For us it will be the same value, ya.
But for a node, if they load up a different json file this means "take the minimum between the invoke_tx_mx_n_steps from the json file you loaded and the one that came with json file that was shipped with this blockifier version (aka "default versioned_constants" here).

This can happen if they want to preload new gas costs without bumping a blockifier version (which might not work properly if the blockifier version with the new gas costs had other logic changes but this feature is a best-effort kind of thing).

Will we allow to override this value?
Wouldn't that be problematic for a full node re-executing transactions from older versions?

We won't override it interanally, for the exact reason you mentioned.
This isn't the case for max_validate n_steps, since that is only relevant for the gateway, and the nodes have nothing to do with that bit.


crates/native_blockifier/src/py_validator.rs line 48 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please explain why? :)

Basically we want to retain override capability for any consts that won't mess up the nodes (for example max_invoke n steps, like you mentioned in one of the comments above).
Changing max validate on the fly won't mess up nodes (at most, they'll start only getting txs with simple validates, but that's about it).


crates/blockifier/BUILD line 7 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please explain this change? :)

Hmm that's not needed anymore actually 😱

Removed.

Remained from a previous implementation, when i was accessing the json file via native_blockifier.

This change makes these files accessible to bazel.
However, since now we're downloading the file directly from blockifier's github page, this isn't necessary.

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noaov1)


crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

Previously, giladchase wrote…

Not sure really, the values might vary between chains, and some might be stale and always overwritten.
I used 3000000 because that seemed to be the default value for general_config, and the expected ymls from the pyconf test also seemed to agree.

There's a task to go over all chains and compare values, which i'm doing concurrently to this, which was agreed to be mainnet values.

I think mainnet uses the default value (3000000) because its preset doesn't seem to Replace the default value above.
I can't really check for sure cause i don't have mainnet access.

You can run the deployment command with the mainnet preset (or any env., for this matter) with --dry_run flag and TAL at the created artifacts.

Copy link
Collaborator

@elintul elintul left a 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, 4 unresolved discussions (waiting on @noaov1)

elintul
elintul previously approved these changes Jan 30, 2024
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @noaov1)

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @giladchase)


crates/blockifier/resources/versioned_constants.json line 1 at r7 (raw file):

Previously, giladchase wrote…

The resources dir is typically where ppl put non-code stuff, that is intended to be consumed during execution.
Putting it anywhere inside src could be considered a bit inappropriate, since it isn't source code, it's static.

Moreover, since this file should be easily accessible for users, and even allow them to store more similar files in this directory in addition to this one for quick switching between constants, a central location like a dedicated dir at the root of the crate is a spot for this.
The alternative would be to send the users to dig into our code, which is less convenient (especially for product ppl who just want to know prices of stuff and don't want to dig around code).

Got it. Thanks!


crates/blockifier/src/execution/entry_point.rs line 225 at r7 (raw file):

But for a node, if they load up a different json file this means "take the minimum between the invoke_tx_mx_n_steps from the json file you loaded and the one that came with json file that was shipped with this blockifier version (aka "default versioned_constants" here).

Why do we want this behaviour?
Isn't it enough to use only constants.invoke_tx_max_n_steps?


crates/native_blockifier/src/py_validator.rs line 48 at r7 (raw file):

Previously, giladchase wrote…

Basically we want to retain override capability for any consts that won't mess up the nodes (for example max_invoke n steps, like you mentioned in one of the comments above).
Changing max validate on the fly won't mess up nodes (at most, they'll start only getting txs with simple validates, but that's about it).

I don't think I fully understand :)
Isn't changing the max_validate_n_steps can affect the nodes when they re-execute transactions? (assume they are using the values from the file while we override it in the original execution)

Copy link
Collaborator Author

@giladchase giladchase left a 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, 2 unresolved discussions (waiting on @noaov1)


crates/blockifier/src/execution/entry_point.rs line 225 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

But for a node, if they load up a different json file this means "take the minimum between the invoke_tx_mx_n_steps from the json file you loaded and the one that came with json file that was shipped with this blockifier version (aka "default versioned_constants" here).

Why do we want this behaviour?
Isn't it enough to use only constants.invoke_tx_max_n_steps?

Good question, actually now that i think about it maybe constants::MAX_VALIDATE_STEPS_PER_TX was intended as global maximum 🤔
I'll ask Dori tomorrow and replace this const, i think it really was a global maximum.

Eitherway, good catch 🦅

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 23 of 25 files reviewed, 2 unresolved discussions (waiting on @elintul and @noaov1)


crates/native_blockifier/src/py_validator.rs line 48 at r7 (raw file):

Previously, noaov1 (Noa Oved) wrote…

I don't think I fully understand :)
Isn't changing the max_validate_n_steps can affect the nodes when they re-execute transactions? (assume they are using the values from the file while we override it in the original execution)

This is added up the stack, i'll add a TODO there to check it out, plz unblock for now? 😬

noaov1
noaov1 previously approved these changes Jan 31, 2024
Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase force-pushed the gilad/add-blockifier-config branch 2 times, most recently from f18366b to ef3526a Compare January 31, 2024 10:38
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r9, 16 of 16 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @giladchase)


Cargo.toml line 16 at r10 (raw file):

[workspace.dependencies]
anyhow = "1.0.0"

Why is it back?

Code quote:

anyhow = "1.0.0"

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)


Cargo.toml line 16 at r10 (raw file):

Previously, elintul (Elin) wrote…

Why is it back?

Rebased, someone added it main.
I think the reason had to do with the VM using it as a return type :(

Anyway not related to my work.

Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)


crates/blockifier/resources/versioned_constants.json line 15 at r3 (raw file):

Previously, elintul (Elin) wrote…

You can run the deployment command with the mainnet preset (or any env., for this matter) with --dry_run flag and TAL at the created artifacts.

Noa checked, the 1000000 is just for testing.
Elin: i'll run the command you said after we merge the stack? worst case i'll just have to change the json values again.

elintul
elintul previously approved these changes Jan 31, 2024
Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

- Currently only covers the constants previously contained in `BlockInfo`.
TODO: Add check if this covers all chain ids, which might have different
contstants
- Remove `BlockContextArgs`: no longer needed now that BlockContext can
  be initialized by 3 args of public types.
Copy link
Collaborator Author

@giladchase giladchase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 24 of 25 files reviewed, all discussions resolved (waiting on @elintul)


crates/blockifier/BUILD line 7 at r11 (raw file):

    name="blockifier",
    srcs=glob(["src/**/*.rs"]),
    data=glob(["resources/**/*"]),

OK this ended up being important for bazel, without this it won't have access to these files, they have to be whitelisted, which is basically what this does

Code quote:

    data=glob(["resources/**/*"]),

Copy link
Collaborator

@noaov1 noaov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r8, 16 of 16 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

Copy link
Collaborator

@elintul elintul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @giladchase)

@giladchase giladchase merged commit cb1637a into main-v0.13.1 Jan 31, 2024
8 checks passed
@giladchase giladchase deleted the gilad/add-blockifier-config branch January 31, 2024 15:13
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants