Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(consensus): add versioned constants to Consensus #3223

Merged
merged 1 commit into from
Jan 16, 2025
Merged

feat(consensus): add versioned constants to Consensus #3223

merged 1 commit into from
Jan 16, 2025

Conversation

ayeletstarkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@ayeletstarkware ayeletstarkware marked this pull request as ready for review January 9, 2025 14:28
@ayeletstarkware Graphite App
Copy link
Contributor Author

ayeletstarkware commented Jan 9, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ayeletstarkware ayeletstarkware mentioned this pull request Jan 9, 2025
Merged
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025 •
edited
Loading

Artifacts upload workflows:

@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/add-versioned-constants-to-concensus branch from e9d58e3 to 01b8533 Compare January 9, 2025 14:34
@ayeletstarkware ayeletstarkware mentioned this pull request Jan 9, 2025
Merged
@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/move-from-blockifier-to-snapi branch from f6f7f30 to 55013d2 Compare January 9, 2025 16:00
@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/add-versioned-constants-to-concensus branch from 01b8533 to a454f2c Compare January 9, 2025 16:00
@ayeletstarkware ayeletstarkware changed the title feat(concensus): add versioned constants to Concensus feat(consensus): add versioned constants to Consensus Jan 9, 2025
@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/move-from-blockifier-to-snapi branch from 55013d2 to 1499408 Compare January 12, 2025 11:25
@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/add-versioned-constants-to-concensus branch from a454f2c to 6e52ae0 Compare January 12, 2025 11:25
@github-actions GitHub Actions
Copy link

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [35.051 ms 35.133 ms 35.225 ms]
change: [-4.0199% -2.4204% -1.0158%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
4 (4.00%) high mild
1 (1.00%) high severe

@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/move-from-blockifier-to-snapi branch 2 times, most recently from 0981189 to cd1a325 Compare January 14, 2025 12:41
@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/add-versioned-constants-to-concensus branch from 6e52ae0 to 8859ae8 Compare January 14, 2025 12:42
@ayeletstarkware ayeletstarkware force-pushed the ayelet/versioned-constants/add-versioned-constants-to-concensus branch from 8859ae8 to d3ad325 Compare January 15, 2025 12:30
@ayeletstarkware ayeletstarkware changed the base branch from ayelet/versioned-constants/move-from-blockifier-to-snapi to main January 15, 2025 12:30
alonh5
alonh5 requested changes Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@alonh5 alonh5 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 r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ayeletstarkware)

crates/sequencing/papyrus_consensus_orchestrator/src/versioned_constants.rs line 16 at r2 (raw file):

    pub min_gas_price: u64,
    /// The maximum block size in gas units.
    pub max_block_size: u64,

Was this also discussed to be a version constant? I remember only the other 3.

Code quote:

max_block_size

ayeletstarkware
ayeletstarkware commented Jan 16, 2025
View reviewed changes
Copy link
Contributor Author

@ayeletstarkware ayeletstarkware 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 @alonh5)

crates/sequencing/papyrus_consensus_orchestrator/src/versioned_constants.rs line 16 at r2 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Was this also discussed to be a version constant? I remember only the other 3.

I think so. That's what I wrote on Monday after the design review.

alonh5
alonh5 approved these changes Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@alonh5 alonh5 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 @ayeletstarkware)

alonh5
alonh5 approved these changes Jan 16, 2025
View reviewed changes
Copy link
Collaborator

@alonh5 alonh5 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @ayeletstarkware)

@ayeletstarkware ayeletstarkware added this pull request to the merge queue Jan 16, 2025
Merged via the queue into main with commit 453e3f1 Jan 16, 2025
21 of 39 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alonh5 alonh5 alonh5 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ayeletstarkware @reviewable-StarkWare @alonh5