feat(consensus): add config to Context

[guy-starkware]

Adds a ContextConfig struct that is used by the two context structs.

[reviewable-StarkWare]

This change is 

[guy-starkware]

• chore(consensus): reorder config item #3430
• chore(consensus): move some network parameter consts into config #3380
• chore(consensus): remove some fields from ConsensusConfig #3379
• chore(consensus): add PapyrusConsensusConfig struct #3377
• chore(consensus): update ConsensusManagerConfig with new parameters #3340
• chore(consensus): add ContextConfig to SequencerConcensusContext #3338
• feat(consensus): add config to Context #3268 👈 (View in Graphite)
• chore(consensus): remove default value for chain_id config parameter #2898 : 1 other dependent PR (#3130 )
• main

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

[github-actions]

Artifacts upload workflows:

• Started at 2025-01-16T09:43:14Z
• Started at 2025-01-16T09:43:14Z

[asmaastarkware]

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dan-starkware, @giladchase, and @guy-starkware)

---

crates/papyrus_node/src/config/mod.rs line 12 at r1 (raw file):

use std::ops::IndexMut;
use std::path::{Path, PathBuf};
use std::task::Context;

unused right?

---

crates/papyrus_node/src/config/mod.rs line 69 at r1 (raw file):

    pub p2p_sync: Option<P2pSyncClientConfig>,
    pub consensus: Option<ConsensusConfig>,
    pub context: ContextConfig,

make it optional, and apply the relevant changes

Suggestion:

pub context: Option<ContextConfig>,

---

crates/sequencing/papyrus_consensus/src/types.rs line 35 at r1 (raw file):

/// Configuration for the Context struct.
#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Validate)]
pub struct ContextConfig {

why not adding it to the context dir?

---

crates/sequencing/papyrus_consensus/src/types.rs line 41 at r1 (raw file):

    pub num_validators: u64,
    /// The chain id of the Starknet chain.
    pub chain_id: ChainId,

both num_validators and chain_id are part of the consensus config and are passed to the context. I suggest either removing them now or adding a TODO to complete the concept within the PR.
also to add batcher_build_buffer when its used (if we still want to add it to the config since the papyrus context doesn't interact with the batcher)

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs line 145 at r1 (raw file):

    let papyrus_context = PapyrusConsensusContext::new(
        ContextConfig::default(),

by default its 1 not 4.. tests are safe :)

[guy-starkware]

Reviewable status: 3 of 9 files reviewed, 4 unresolved discussions (waiting on @asmaastarkware, @dan-starkware, and @giladchase)

---

crates/papyrus_node/src/config/mod.rs line 12 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

unused right?

yes! I wonder why clippy didn't see that...

---

crates/papyrus_node/src/config/mod.rs line 69 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

make it optional, and apply the relevant changes

Done.

---

crates/sequencing/papyrus_consensus/src/types.rs line 35 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

why not adding it to the context dir?

I'm not really happy about having it in types.rs either.

I think I should do a PR at the end of the stack where I move the ContextConfig to another place. I'm not sure where it will be, if you have a place you think is right, let me know.

---

crates/sequencing/papyrus_consensus/src/types.rs line 41 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

both num_validators and chain_id are part of the consensus config and are passed to the context. I suggest either removing them now or adding a TODO to complete the concept within the PR.
also to add batcher_build_buffer when its used (if we still want to add it to the config since the papyrus context doesn't interact with the batcher)

I added a TODO. This happens in another PR on this stack.

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context_test.rs line 145 at r1 (raw file):

Previously, asmaastarkware (asmaa-starkware) wrote…

by default its 1 not 4.. tests are safe :)

I think I had an issue with the number of validators in one of the tests in the next PRs. I don't remember exactly.

[asmaastarkware]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware and @giladchase)

[matan-starkware]

Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @guy-starkware)

---

crates/sequencing/papyrus_consensus/src/types.rs line 33 at r2 (raw file):

pub type ProposalContentId = BlockHash;

// TODO(guyn): move this to another file.

Why not just create this struct from the start in the other crate?

Code quote:

// TODO(guyn): move this to another file.

---

crates/sequencing/papyrus_consensus/src/types.rs line 36 at r2 (raw file):

/// Configuration for the Context struct.
#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Validate)]
pub struct ContextConfig {

When you move this to the node orchestrator crate rename it to SequencerContextConfig. I want it to be clear that is our use case, and if the papyrus context uses it then that's fine but we do not design around the needs of papyrus.

Code quote:

// TODO(guyn): move this to another file.
/// Configuration for the Context struct.
#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Validate)]
pub struct ContextConfig {

---

crates/sequencing/papyrus_consensus/src/types.rs line 38 at r2 (raw file):

pub struct ContextConfig {
    /// The buffer size for the batcher when building proposals.
    pub batcher_build_buffer: usize,

Is this meant to replace the const CHANNEL_SIZE? If so, it shouldn't mention the batcher, as this is the buffer size between the context and streaming. Maybe

• build_proposal_buffer_size
• proposal_streaming_buffer_size

Or another option is that this is just a pointer to the channel size which we define in the streaming config (ie the outbound streams have the same buffer len as the inbound streams). This feels simplest to me since there is only 1 value to config.

Suggestion:

    /// The buffer size for streaming outbound proposals.
    pub batcher_build_buffer: usize,

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs line 49 at r2 (raw file):

const CHANNEL_SIZE: usize = 100;

pub struct PapyrusConsensusContext {

Why are we only updating the papyrus context? The sequencer context is the one which is actually driving the design of the config and should be the lead example

Code quote:

pub struct PapyrusConsensusContext {

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs line 50 at r2 (raw file):

pub struct PapyrusConsensusContext {
    _config: ContextConfig,

If it's not used, no need to store it

Code quote:

    _config: ContextConfig,

[guy-starkware]

Reviewable status: 8 of 9 files reviewed, 5 unresolved discussions (waiting on @asmaastarkware, @dan-starkware, and @matan-starkware)

---

crates/sequencing/papyrus_consensus/src/types.rs line 33 at r2 (raw file):

Previously, matan-starkware wrote…

Why not just create this struct from the start in the other crate?

I didn't move it because I couldn't decide on a better place for it. I have a suspicion that if I move it in the first PR it will cause lots of chaos across the stack, so I'd prefer to move it in one PR at the end of the stack. Will be much faster to review that PR, as well.

---

crates/sequencing/papyrus_consensus/src/types.rs line 36 at r2 (raw file):

Previously, matan-starkware wrote…

When you move this to the node orchestrator crate rename it to SequencerContextConfig. I want it to be clear that is our use case, and if the papyrus context uses it then that's fine but we do not design around the needs of papyrus.

Good idea.

---

crates/sequencing/papyrus_consensus/src/types.rs line 38 at r2 (raw file):

Previously, matan-starkware wrote…

Is this meant to replace the const CHANNEL_SIZE? If so, it shouldn't mention the batcher, as this is the buffer size between the context and streaming. Maybe

• build_proposal_buffer_size
• proposal_streaming_buffer_size

Or another option is that this is just a pointer to the channel size which we define in the streaming config (ie the outbound streams have the same buffer len as the inbound streams). This feels simplest to me since there is only 1 value to config.

To be honest I took this field from your slack message and I had no idea what it was supposed to do...

Connecting it to the streaming config is a good idea.

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs line 49 at r2 (raw file):

Previously, matan-starkware wrote…

Why are we only updating the papyrus context? The sequencer context is the one which is actually driving the design of the config and should be the lead example

Yes, that makes sense. I arbitrarily started with one of them. I'll keep this in mind next time we have things that affect both sequencer and papyrus.

---

crates/sequencing/papyrus_consensus_orchestrator/src/papyrus_consensus_context.rs line 50 at r2 (raw file):

Previously, matan-starkware wrote…

If it's not used, no need to store it

Done.

[matan-starkware]

Reviewed 2 of 8 files at r1, 6 of 6 files at r2, 1 of 1 files at r3, 10 of 10 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @guy-starkware)

---

crates/sequencing/papyrus_consensus/src/types.rs line 38 at r2 (raw file):

Previously, guy-starkware wrote…

To be honest I took this field from your slack message and I had no idea what it was supposed to do...

Connecting it to the streaming config is a good idea.

My suggestion is that we should create a StreamHandlerConfig and then have this config point to that one.

---

crates/papyrus_node/src/run.rs line 225 at r3 (raw file):

        Ok(papyrus_consensus::run_consensus(
            context,
            consensus_config.start_height,

One of the goals of this refactor is for the consensus config to only include fields which run_consensus receives. Then run_consensus can just take ConsensusConfig as a parameter instead of this whole long list. This may not be entirely feasible, but but something to consider.

Code quote:

            consensus_config.start_height,

[guy-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @matan-starkware)

---

crates/papyrus_node/src/run.rs line 225 at r3 (raw file):

Previously, matan-starkware wrote…

One of the goals of this refactor is for the consensus config to only include fields which run_consensus receives. Then run_consensus can just take ConsensusConfig as a parameter instead of this whole long list. This may not be entirely feasible, but but something to consider.

Should I put this in another PR?

---

crates/sequencing/papyrus_consensus/src/types.rs line 38 at r2 (raw file):

Previously, matan-starkware wrote…

My suggestion is that we should create a StreamHandlerConfig and then have this config point to that one.

I'm adding an item to do add a StreamHandler config.

[matan-starkware]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @guy-starkware)

---

crates/papyrus_node/src/run.rs line 225 at r3 (raw file):

Previously, guy-starkware wrote…

Should I put this in another PR?

Analyze if this is easy. My fear is a mismatch between the config this function wants and the config the node needs on startup.

For instance the observe and active heights. We want the height to be derived from the batcher's storage, not to be passed in by the config. The node should get the special override flag to be immediately active.

[guy-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @matan-starkware)

---

crates/papyrus_node/src/run.rs line 225 at r3 (raw file):

Previously, matan-starkware wrote…

Analyze if this is easy. My fear is a mismatch between the config this function wants and the config the node needs on startup.

For instance the observe and active heights. We want the height to be derived from the batcher's storage, not to be passed in by the config. The node should get the special override flag to be immediately active.

I'll try to add it into this one.

I don't really understand your example, maybe we should discuss tomorrow?

[guy-starkware]

Reviewable status: 5 of 13 files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @matan-starkware)

---

crates/papyrus_node/src/run.rs line 225 at r3 (raw file):

Previously, guy-starkware wrote…

I'll try to add it into this one.

I don't really understand your example, maybe we should discuss tomorrow?

Indeed this is a little more complicated than I thought. I'm leaving this to the end of the stack.

[guy-starkware]

Reviewed 1 of 8 files at r1, 2 of 6 files at r2, 1 of 10 files at r4, 1 of 1 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @matan-starkware)