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): add ContextConfig to SequencerConcensusContext #3338

Conversation

guy-starkware
Copy link
Contributor

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

This adds the ContextConfig struct from the previous PR unto the SequencerConsensusContext struct.

@reviewable-StarkWare
Copy link

This change is Reviewable

@guy-starkware guy-starkware marked this pull request as ready for review January 15, 2025 11:35
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 527bc74 to a57408c Compare January 15, 2025 13:04
@guy-starkware guy-starkware force-pushed the guyn/config/add_context_config branch from 14cc693 to 3010128 Compare January 15, 2025 13:07
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from a57408c to 30748e8 Compare January 15, 2025 13:08
@guy-starkware guy-starkware force-pushed the guyn/config/add_context_config branch from 3010128 to 4c6ccd6 Compare January 16, 2025 09:43
@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/add_context_config branch from 4c6ccd6 to 7878ee6 Compare January 16, 2025 13:55
@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/add_context_config branch from 7878ee6 to 41718f0 Compare January 16, 2025 14:59
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 4d20b96 to 75df68a Compare January 16, 2025 14:59
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 7 of 7 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @guy-starkware)

@matan-starkware matan-starkware self-requested a review January 20, 2025 08:09
@guy-starkware guy-starkware force-pushed the guyn/config/add_context_config branch 2 times, most recently from a871ad2 to cfad903 Compare January 22, 2025 14:03
@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 75df68a to 401c7aa Compare January 22, 2025 14:03
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 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guy-starkware)


a discussion (no related file):
When you move the struct, I'd like it to be in a file config.rs not inside of the sequencer context file.


crates/starknet_integration_tests/src/utils.rs line 150 at r2 (raw file):

            context_config: ContextConfig {
                num_validators,
                chain_id: papyrus_storage::test_utils::CHAIN_ID_FOR_TESTS.clone(),

Is this used elsewhere in testing code?

Code quote:

                chain_id: papyrus_storage::test_utils::CHAIN_ID_FOR_TESTS.clone(),

crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 112 at r2 (raw file):

// has a timeout as a defensive measure to make sure the proposal doesn't live forever if the
// Context crashes or has a bug.
const VALIDATE_PROPOSAL_MARGIN: Duration = Duration::from_secs(10);

These should enter the config. Can be a follow up PR.

Code quote:

const BUILD_PROPOSAL_MARGIN: Duration = Duration::from_millis(1000);
// When validating a proposal the Context is responsible for timeout handling. The Batcher though
// has a timeout as a defensive measure to make sure the proposal doesn't live forever if the
// Context crashes or has a bug.
const VALIDATE_PROPOSAL_MARGIN: Duration = Duration::from_secs(10);

@guy-starkware guy-starkware force-pushed the guyn/config/add_context_config branch from cfad903 to 69ee8c9 Compare January 22, 2025 14:23
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, 3 unresolved discussions (waiting on @matan-starkware)


crates/starknet_consensus_orchestrator/src/sequencer_consensus_context.rs line 112 at r2 (raw file):

Previously, matan-starkware wrote…

These should enter the config. Can be a follow up PR.

Added to Monday board.


crates/starknet_integration_tests/src/utils.rs line 150 at r2 (raw file):

Previously, matan-starkware wrote…

Is this used elsewhere in testing code?

In papyrus_storage test_utils and in end_to_end_flow_test.

@matan-starkware matan-starkware self-requested a review January 22, 2025 15:14
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 all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @matan-starkware)


crates/papyrus_node/src/run.rs line 201 at r2 (raw file):

    let consensus_config = consensus_config.clone();
    let context_config = context_config.clone();
    debug!("Consensus configuration: {consensus_config:?}");

add context_config

@guy-starkware guy-starkware force-pushed the guyn/config/add_context_config branch from 69ee8c9 to 4a8b4b4 Compare January 23, 2025 08:51
@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/sequencer_context_config branch from 41663c6 to 6baa38e Compare January 23, 2025 09:38
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 13 files reviewed, 2 unresolved discussions (waiting on @asmaastarkware and @matan-starkware)


crates/papyrus_node/src/run.rs line 201 at r2 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

add context_config

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.

:lgtm:

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware and @matan-starkware)

@guy-starkware guy-starkware force-pushed the guyn/config/sequencer_context_config branch from 6baa38e to d6dcb91 Compare January 23, 2025 10:05
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 13 files reviewed, 1 unresolved discussion (waiting on @matan-starkware)


a discussion (no related file):

Previously, matan-starkware wrote…

When you move the struct, I'd like it to be in a file config.rs not inside of the sequencer context file.

Done.

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 3 of 13 files at r2, 7 of 7 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)

@guy-starkware guy-starkware merged commit c2397fa into guyn/config/add_context_config Jan 23, 2025
14 of 15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 25, 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