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): move network parameters into consensus config #3130

Conversation

guy-starkware
Copy link
Contributor

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

Moved a few parameters used in consensus from constants into config fields.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

guy-starkware commented Jan 6, 2025

@guy-starkware guy-starkware marked this pull request as ready for review January 6, 2025 11:16
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 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware)


crates/starknet_consensus_manager/src/consensus_manager.rs line 22 at r1 (raw file):

// TODO(Dan, Guy): move to config.
// pub const BROADCAST_BUFFER_SIZE: usize = 100;

dont forget to remove :)

@guy-starkware guy-starkware force-pushed the guyn/config/remove_some_defaults branch from ee9fa2d to bcabfe7 Compare January 8, 2025 14:25
@guy-starkware guy-starkware force-pushed the guyn/config/add_consensus_items branch 2 times, most recently from dec918d to c1414f4 Compare January 8, 2025 14:27
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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @asmaastarkware)


crates/starknet_consensus_manager/src/consensus_manager.rs line 22 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

dont forget to remove :)

Thanks!

@guy-starkware guy-starkware force-pushed the guyn/config/remove_some_defaults branch from bcabfe7 to 7078ff6 Compare January 9, 2025 07:37
@guy-starkware guy-starkware force-pushed the guyn/config/add_consensus_items branch from c1414f4 to 17326cc Compare January 9, 2025 07:37
@guy-starkware guy-starkware force-pushed the guyn/config/remove_some_defaults branch from 7078ff6 to 9f1b01c Compare January 12, 2025 11:09
@guy-starkware guy-starkware force-pushed the guyn/config/add_consensus_items branch from 17326cc to 0a7bb60 Compare January 12, 2025 11:09
Copy link

Artifacts upload workflows:

@guy-starkware guy-starkware force-pushed the guyn/config/remove_some_defaults branch from 9f1b01c to 8d4f147 Compare January 12, 2025 15:23
@guy-starkware guy-starkware force-pushed the guyn/config/add_consensus_items branch from 0a7bb60 to 22c3068 Compare January 12, 2025 15:23
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: 3 of 7 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @guy-starkware)


crates/sequencing/papyrus_consensus/src/config.rs line 55 at r2 (raw file):

    pub proposals_topic: String,
    /// Name of the topic for vote messages.
    pub votes_topic: String,

Why are we putting these into the consensus config? The plan is to put them into a new config struct for the node ConsensusManagerConfig/PapyrusConsensusConfig

Code quote:

    /// The buffer size of each channel, for votes channel and proposals channel.
    pub broadcast_buffer_size: usize,
    /// Name of the topic for streaming proposals.
    pub proposals_topic: String,
    /// Name of the topic for vote messages.
    pub votes_topic: String,

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