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.

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

chore(consensus): remove some fields from ConsensusConfig #3379

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

guy-starkware
Copy link
Contributor

@guy-starkware guy-starkware commented Jan 16, 2025

Remove some fields that were on ConsensusConfig that were no longer needed.

@reviewable-StarkWare
Copy link

This change is Reviewable

@guy-starkware guy-starkware marked this pull request as ready for review January 16, 2025 12:24
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from dc0dd26 to 14909c5 Compare January 16, 2025 13:55
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from eee25b4 to e07b167 Compare January 16, 2025 13:55
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 14909c5 to 28f6ed1 Compare January 16, 2025 14:59
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from e07b167 to fb11f3a Compare January 16, 2025 15:00
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 28f6ed1 to 296594f Compare January 16, 2025 15:20
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from fb11f3a to 0647983 Compare January 16, 2025 15:20
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 296594f to 8d996ae Compare January 22, 2025 14:03
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from 0647983 to 49d2b6a Compare January 22, 2025 14:03
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 8d996ae to 9ac69ae Compare January 23, 2025 08:51
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from 49d2b6a to cd98972 Compare January 23, 2025 08:51
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 9ac69ae to e864fff Compare January 23, 2025 09:39
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from cd98972 to 971b1a4 Compare January 23, 2025 09:39
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from e864fff to 59490b8 Compare January 23, 2025 10:05
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from 971b1a4 to 2aa4ba5 Compare January 23, 2025 10:05
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 59490b8 to 1aca5ea Compare January 23, 2025 12:52
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from 2aa4ba5 to ec22ff9 Compare January 23, 2025 12:53
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from 1aca5ea to 9902c7b Compare January 23, 2025 13:04
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from ec22ff9 to df3710f Compare January 23, 2025 13:04
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from df3710f to 4216dba Compare January 23, 2025 15:32
@guy-starkware guy-starkware force-pushed the guyn/config/add_papyrus_consensus_config branch from f4c4d38 to c533198 Compare January 30, 2025 08:34
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch 2 times, most recently from 761bdf6 to 6fbfb91 Compare February 2, 2025 09:33
@guy-starkware guy-starkware changed the base branch from guyn/config/add_papyrus_consensus_config to guyn/config/consensus_manager_config February 2, 2025 09:33
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 279df6d to 9859dd1 Compare February 2, 2025 09:54
@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch 2 times, most recently from 21218ae to c25a1d6 Compare February 2, 2025 12:06
@guy-starkware guy-starkware changed the base branch from guyn/config/consensus_manager_config to main February 2, 2025 12:06
Copy link
Contributor

@asmaastarkware asmaastarkware 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 10 files at r1, 27 of 27 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware and @matan-starkware)


crates/starknet_consensus/src/config.rs line 82 at r2 (raw file):

        Self {
            validator_id: ValidatorId::from(DEFAULT_VALIDATOR_ID),
            // start_height: BlockNumber::default(),

delete

@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from c25a1d6 to 172357a Compare February 3, 2025 09:06
Copy link
Contributor Author

@guy-starkware guy-starkware 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: 10 of 28 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @matan-starkware)


crates/starknet_consensus/src/config.rs line 82 at r2 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

delete

Done.

Copy link
Contributor

@asmaastarkware asmaastarkware left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@matan-starkware matan-starkware 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 @guy-starkware)


crates/starknet_consensus/src/config.rs line 36 at r3 (raw file):

    pub future_height_limit: u32,
    /// How many rounds in the future should we cache.
    pub future_round_limit: u32,

Adding these doesn't belong in "remove some fields from ConsensusConfig"

Code quote:

    /// How many heights in the future should we cache.
    pub future_height_limit: u32,
    /// How many rounds in the future should we cache.
    pub future_round_limit: u32,

Copy link
Contributor Author

@guy-starkware guy-starkware 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 @matan-starkware)


crates/starknet_consensus/src/config.rs line 36 at r3 (raw file):

Previously, matan-starkware wrote…

Adding these doesn't belong in "remove some fields from ConsensusConfig"

Should I break this into a new PR or rename this one?

@matan-starkware matan-starkware self-requested a review February 3, 2025 17:51
Copy link
Contributor

@matan-starkware matan-starkware 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 @guy-starkware)


crates/starknet_consensus/src/config.rs line 36 at r3 (raw file):

Previously, guy-starkware wrote…

Should I break this into a new PR or rename this one?

Adding these fields sould be a distinct PR from the removals.

Copy link
Contributor Author

@guy-starkware guy-starkware 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: 25 of 28 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware and @matan-starkware)


crates/starknet_consensus/src/config.rs line 36 at r3 (raw file):

Previously, matan-starkware wrote…

Adding these fields sould be a distinct PR from the removals.

I split it into a separate PR.

@guy-starkware guy-starkware force-pushed the guyn/config/remove_stuff_from_consensus_config branch from 445a40e to edf2e86 Compare February 4, 2025 08:41
@matan-starkware matan-starkware self-requested a review February 4, 2025 09:05
Copy link
Contributor

@matan-starkware matan-starkware 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 10 files at r1, 4 of 27 files at r2, 6 of 6 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 18 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

@matan-starkware matan-starkware self-requested a review February 4, 2025 11:16
@guy-starkware guy-starkware added this pull request to the merge queue Feb 4, 2025
Merged via the queue into main with commit 1c04f0e Feb 4, 2025
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2025
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