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

feat: add more constants toVersionedConstants #1369

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

giladchase
Copy link
Collaborator

@giladchase giladchase commented Jan 24, 2024

  • Add gateway constants, these are only for transparency, and aren't used directly by the Blockifier whatsoever.
  • Pass validate_max_n_steps into the blockifier via native_blockifier, to override versioned constants defaults.

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


This change is Reviewable

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 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @giladchase and @noaov1)


-- commits line 5 at r1:
Why add them then?

Code quote:

  - Add gateway constants, these are only for transparency, and aren't
    used directly by the Blockifier whatsoever.

crates/blockifier/src/versioned_constants.rs line 16 at r1 (raw file):

});

/// `VersionedConstants` contains constants for the Blockifier that may vary between versions.

Suggestion:

Contains

crates/native_blockifier/src/py_block_executor.rs line 41 at r1 (raw file):

    pub fn create(
        general_config: PyGeneralConfig,
        validate_max_n_steps: u32,

What about max number of steps for execute? Also, why not inside general config?

Code quote:

validate_max_n_steps

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 @elintul and @noaov1)


-- commits line 5 at r1:

Previously, elintul (Elin) wrote…

Why add them then?

Lior asked for it during the design review, for transparency, and to make the blockifier a single source of truth for constants relating to its operation.


crates/native_blockifier/src/py_block_executor.rs line 41 at r1 (raw file):

Previously, elintul (Elin) wrote…

What about max number of steps for execute? Also, why not inside general config?

It came up in the design review as well, Lior wanted this one to be configurable but not the execute, we didn't have enough time in the meeting to enquire why.

@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2024

Codecov Report

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

Comparison is base (38af968) 70.51% compared to head (4aaa9d1) 70.42%.

Files Patch % Lines
crates/native_blockifier/src/py_utils.rs 0.00% 9 Missing ⚠️
crates/native_blockifier/src/py_block_executor.rs 0.00% 3 Missing ⚠️
crates/native_blockifier/src/py_validator.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                    @@
##           gilad/account-tx-info    #1369      +/-   ##
=========================================================
- Coverage                  70.51%   70.42%   -0.10%     
=========================================================
  Files                         60       60              
  Lines                       7759     7769      +10     
  Branches                    7759     7769      +10     
=========================================================
  Hits                        5471     5471              
- Misses                      1861     1871      +10     
  Partials                     427      427              

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

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 r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase and @noaov1)


-- commits line 5 at r1:

Previously, giladchase wrote…

Lior asked for it during the design review, for transparency, and to make the blockifier a single source of truth for constants relating to its operation.

How is it a single source of truth if they aren't used? :(


crates/native_blockifier/src/py_block_executor.rs line 41 at r1 (raw file):

Previously, giladchase wrote…

It came up in the design review as well, Lior wanted this one to be configurable but not the execute, we didn't have enough time in the meeting to enquire why.

Let's try to understand, maybe in Slack?
Also understand where did the invoke max steps go. 😬

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 @elintul and @noaov1)


-- commits line 5 at r1:

Previously, elintul (Elin) wrote…

How is it a single source of truth if they aren't used? :(

Single source of truth for both python and blockifier.
Python downloads the json file via bazel (downloads it from the blockifier repo) and extracts these constants from there.

@giladchase giladchase force-pushed the gilad/account-tx-info branch 11 times, most recently from 005cf5a to fe0dad0 Compare January 29, 2024 06:51
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:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

@giladchase giladchase force-pushed the gilad/account-tx-info branch 5 times, most recently from ea76ec4 to a23cb93 Compare January 31, 2024 15:54
@giladchase giladchase force-pushed the gilad/account-tx-info branch 2 times, most recently from 09d12f8 to 51afda5 Compare January 31, 2024 16:53
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.

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

- Add gateway constants, these are only for transparency, and aren't
  used directly by the Blockifier whatsoever.
- Pass `validate_max_n_steps` into the blockifier via native_blockifier,
  to override versioned constants defaults.
- Sort json
Base automatically changed from gilad/account-tx-info to main-v0.13.1 February 4, 2024 10:41
@giladchase giladchase dismissed elintul’s stale review February 4, 2024 10:41

The base branch was changed.

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 @noaov1)


crates/native_blockifier/src/py_block_executor.rs line 41 at r1 (raw file):

Previously, elintul (Elin) wrote…

Let's try to understand, maybe in Slack?
Also understand where did the invoke max steps go. 😬

Resolved f2f. Closing.

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 1 of 4 files at r1, 1 of 4 files at r3, 1 of 26 files at r4, 2 of 26 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @giladchase)


crates/native_blockifier/src/py_block_executor.rs line 42 at r5 (raw file):

        general_config: PyGeneralConfig,
        validate_max_n_steps: u32,
        max_recursion_depth: usize,

Out of curiosity- Do we always want to pass it? Why not making it optional?

Code quote:

        validate_max_n_steps: u32,
        max_recursion_depth: usize,

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.

Note that the gateway constants will also be overriden in the native blockifier just as the validate constant is at this PR.

We'll handle this separately from this PR in an upcoming gateway.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @noaov1)


crates/native_blockifier/src/py_block_executor.rs line 42 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Out of curiosity- Do we always want to pass it? Why not making it optional?

Just to simplify the implementation.
As an optional, it'll require the python config to also hold this optionally, and we'll have to match on it here.

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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @giladchase)


crates/native_blockifier/src/py_block_executor.rs line 42 at r5 (raw file):

Previously, giladchase wrote…

Just to simplify the implementation.
As an optional, it'll require the python config to also hold this optionally, and we'll have to match on it here.

Okay. So we plan to have a copy of this constants in the python repository that will be "fed" to the blockifier?

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:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @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: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @noaov1)


crates/native_blockifier/src/py_block_executor.rs line 42 at r5 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Okay. So we plan to have a copy of this constants in the python repository that will be "fed" to the blockifier?

Xactly, see the python PR for how it is passed.
In the upcoming Prs, once the infra is ready, python will no longer have copy of the default value from versioned_constants.json, it'll take it directly from the blockifier, so that the only case where they'll be different is if they are explicitly replaced at runtime or if someone overrides the defaults in one of the python presets.

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.

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

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 24 of 26 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)

@giladchase giladchase merged commit 29d2d5f into main-v0.13.1 Feb 4, 2024
13 checks passed
@giladchase giladchase deleted the gilad/add-constants-to-config branch February 4, 2024 12:44
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
…-libs#1719)

ref starkware-libs#1369

- re-export [reth-metrics-derive](https://github.com/paradigmxyz/reth/tree/v0.2.0-beta.4/crates/metrics/metrics-derive) crate from dojo-metrics
- rename metrics -> dojo-metrics crate
- re-export core metrics stuff from dojo-metrics

---

this PR relies on starkware-libs#1720 bcs reth's MSRV is 1.76.0, otherwise can't build reth-metrics-derive.
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
ref starkware-libs#1369 

add metrics for rpc server connections and methods calls
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
ref starkware-libs#1369 

Add metrics on katana executor; tracking the total L1 **gas** and Cairo **steps** used.

There were two approaches that i thought of; 
1. record the metrics on every tx execution, or
2. on every block

~Decided to go with (1) as it would allow to measure it in realtime (as the tx is being executed), instead of having to wait until the block is finished being processed.~

Thought im not exactly sure which one is the ideal one.

Doing (1) might be less performant bcs we have to acquire the lock to the metrics recorder more frequently (ie every tx), as opposed to only updating the metrics once every block.

another thing to note, currently doing (1) would require all executor implementations to define the metrics in their own implmentations, meaning have to duplicate code. If do (2) can just define it under `block_producer` scope and be executor agnostic.

EDIT: doing (2). metrics are collected upon completion of block production

---

some changes are made to gather the value after block production:
- simplify params on `backend::do_mine_block`, now only accept two args; `BlockEnv` and `ExecutionOutput`
- add a new type `ExecutionStats` under `katana-executor`, this is where executor would store the gas and steps value
gswirski pushed a commit to reilabs/blockifier that referenced this pull request Jun 26, 2024
…bs#1818)

ref starkware-libs#1791 starkware-libs#1369

<img width="1420" alt="Screenshot 2024-04-12 at 2 36 56 AM" src="https://github.com/dojoengine/dojo/assets/26515232/b97b49df-c5fc-429a-9cb0-bf66138a00b6">

showing total gas and steps using a simple line charts, tracking its growth over time
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