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): update ConsensusManagerConfig with new parameters #3340

Merged
merged 1 commit into from
Feb 2, 2025

Conversation

guy-starkware
Copy link
Contributor

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

This adds a few missing fields to the ConsensusManagerConfig struct.
One of them is network_config, which is also removed from ConsensusConfig.

The ConsensusConfig needs a few other modifications, but I'll get to them in one of the next PRs.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

guy-starkware commented Jan 15, 2025

@guy-starkware guy-starkware marked this pull request as ready for review January 15, 2025 12:46
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch 2 times, most recently from a57408c to 30748e8 Compare January 15, 2025 13:08
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 8e1a680 to 4c3caf1 Compare January 15, 2025 13:08
Copy link

github-actions bot commented Jan 15, 2025

Artifacts upload workflows:

@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 30748e8 to 407201c Compare January 16, 2025 09:43
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 4c3caf1 to f1cfc9e Compare January 16, 2025 09:43
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 407201c to 4d20b96 Compare January 16, 2025 13:55
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from f1cfc9e to 2c57fef Compare January 16, 2025 13:55
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 4d20b96 to 75df68a Compare January 16, 2025 14:59
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 2c57fef to 298f522 Compare January 16, 2025 14:59
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 75df68a to 401c7aa Compare January 22, 2025 14:03
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 298f522 to 3f08f67 Compare January 22, 2025 14:03
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 401c7aa to 41663c6 Compare January 23, 2025 08:51
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 3f08f67 to 764afbb Compare January 23, 2025 08:51
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 41663c6 to 6baa38e Compare January 23, 2025 09:38
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 764afbb to 1760a5b Compare January 23, 2025 09:39
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 6baa38e to d6dcb91 Compare January 23, 2025 10:05
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 1760a5b to 7fa81cf Compare January 23, 2025 10:05
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 8 of 10 files at r1.
Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @matan-starkware)


crates/starknet_consensus_manager/src/config.rs line 23 at r2 (raw file):

    pub votes_topic: String,
    pub proposals_topic: String,
    pub immediate_active_height: u64,

Do you mean the start_height in ConsensusConfig? if so, I would prefer using start_height instead


crates/starknet_consensus_manager/src/config.rs line 57 at r2 (raw file):

        ]);
        output_tree.extend(additional_parameters);
        output_tree

Suggestion:

        let mut config = BTreeMap::from_iter([
            ser_param(
                "votes_topic",
                &self.votes_topic,
                "The topic for consensus votes.",
                ParamPrivacyInput::Public,
            ),
            ser_param(
                "proposals_topic",
                &self.proposals_topic,
                "The topic for consensus proposals.",
                ParamPrivacyInput::Public,
            ),
            ser_param(
                "immediate_active_height",
                &self.immediate_active_height,
                "The height at which the consensus manager becomes active.",
                ParamPrivacyInput::Public,
            ),
        ]);
        config.extend(append_sub_config_name(self.consensus_config.dump(), "consensus_config"));
        config.extend(append_sub_config_name(self.context_config.dump(), "context_config"));
        config.extend(append_sub_config_name(self.cende_config.dump(), "cende_config"));
        config.extend(append_sub_config_name(self.network_config.dump(), "network_confi)"));
        config

@guy-starkware guy-starkware changed the base branch from guyn/config/sequencer_context_config to guyn/config/sequencer_context_config2 January 23, 2025 12:52
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: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)


crates/starknet_consensus_manager/src/config.rs line 23 at r2 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

Do you mean the start_height in ConsensusConfig? if so, I would prefer using start_height instead

I'm not sure. @matan-starkware WDYT?


crates/starknet_consensus_manager/src/config.rs line 57 at r2 (raw file):

        ]);
        output_tree.extend(additional_parameters);
        output_tree

Done.

@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch 2 times, most recently from b7e9378 to 3454b9e Compare January 23, 2025 15:31
@guy-starkware guy-starkware changed the base branch from guyn/config/sequencer_context_config2 to main January 23, 2025 15:31
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 3454b9e to 06d06b3 Compare January 30, 2025 08:34
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 5 of 10 files at r1, 1 of 3 files at r5, 4 of 4 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)


crates/starknet_consensus_manager/src/config.rs line 23 at r2 (raw file):

Previously, guy-starkware wrote…

I'm not sure. @matan-starkware WDYT?

I think immediate_active_height is preferable. The point is that we are overriding observer mode, not setting the start height, as that is still derived from that batcher.


crates/starknet_consensus_manager/src/config.rs line 46 at r6 (raw file):

                "immediate_active_height",
                &self.immediate_active_height,
                "The height at which the consensus manager becomes active.",

Suggestion:

                "The height at which the node may actively participate in consensus.",

crates/starknet_integration_tests/src/utils.rs line 137 at r6 (raw file):

            consensus_config: ConsensusConfig {
                start_height: BlockNumber(1),
		// TODO(Matan, Dan): Set the right amount

fix spacing

Code quote:

		// TODO(Matan, Dan): Set the right amount

@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 06d06b3 to 1dc5be3 Compare January 30, 2025 09:49
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: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)


crates/starknet_consensus_manager/src/config.rs line 23 at r2 (raw file):

Previously, matan-starkware wrote…

I think immediate_active_height is preferable. The point is that we are overriding observer mode, not setting the start height, as that is still derived from that batcher.

Done.


crates/starknet_integration_tests/src/utils.rs line 137 at r6 (raw file):

Previously, matan-starkware wrote…

fix spacing

Done.


crates/starknet_consensus_manager/src/config.rs line 46 at r6 (raw file):

                "immediate_active_height",
                &self.immediate_active_height,
                "The height at which the consensus manager becomes active.",

Done.

@matan-starkware matan-starkware self-requested a review January 30, 2025 10:08
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 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware)


crates/starknet_integration_tests/src/utils.rs line 137 at r6 (raw file):

Previously, guy-starkware wrote…

Done.

Still shows tabs for me on reviewable. Can you double check they are spaces?

@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 1dc5be3 to f449128 Compare January 30, 2025 12:43
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: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)


crates/starknet_integration_tests/src/utils.rs line 137 at r6 (raw file):

Previously, matan-starkware wrote…

Still shows tabs for me on reviewable. Can you double check they are spaces?

OK I didn't notice those were tabs. It is very strange because I didn't even know there were any tabs in VSCode... I fixed it now.

@matan-starkware matan-starkware self-requested a review January 30, 2025 12:46
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.

Reviewed 5 of 10 files at r1, 2 of 4 files at r6, 2 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @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.

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

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.

:lgtm:

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

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 2 of 4 files at r6, 1 of 3 files at r7, 1 of 1 files at r8.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)

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.

Dismissed @asmaastarkware from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from f449128 to 279df6d Compare February 2, 2025 09:32
@guy-starkware guy-starkware force-pushed the guyn/config/consensus_manager_config branch from 279df6d to 9859dd1 Compare February 2, 2025 09:54
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.

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

@guy-starkware guy-starkware added this pull request to the merge queue Feb 2, 2025
Merged via the queue into main with commit 733948e Feb 2, 2025
19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 4, 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