-
Notifications
You must be signed in to change notification settings - Fork 150
load da_committee from genesis file #3634
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
base: main
Are you sure you want to change the base?
Conversation
9c100c2
to
99589b4
Compare
crates/hotshot/types/src/lib.rs
Outdated
[(TYPES::Epoch::new(0), self.known_da_nodes.clone())].into() | ||
} else { | ||
if !self.known_da_nodes.is_empty() { | ||
tracing::warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be an error
or arguably even a panic -- I think it might be preferable to call this a misconfiguration and immediately panic vs. try to resolve the ambiguity
types/src/v0/impls/stake_table.rs
Outdated
let da_members = da_committees | ||
.get(&Epoch::new(0)) | ||
.cloned() | ||
.unwrap_or_default(); // For testing reasons, we want to support being given an empty map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or_default
means return an empty vec here right? I think it would be better to panic, since an empty DA committee is always a (fatal) error
sequencer/src/genesis.rs
Outdated
#[serde(default)] | ||
pub upgrades: BTreeMap<Version, Upgrade>, | ||
#[serde(default)] | ||
pub da_committees: Option<BTreeMap<TomlKeyU64, Vec<PeerConfigData>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A BTreeMap
can be null right? do we need to make this an Option
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also is there a reason it needs to be in both HotShotConfig
and Genesis
?
do we use it out of HotShotConfig
anywhere? (I think it's fine to leave it in both even if we only use it out of genesis, just wondering)
10bf649
to
ca9dcbd
Compare
types/src/v0/config.rs
Outdated
known_nodes_with_stake: Vec<PeerConfig<SeqTypes>>, | ||
known_da_nodes: Vec<PeerConfig<SeqTypes>>, | ||
#[serde(default)] | ||
da_committees: BTreeMap<Version, BTreeMap<u64, Vec<PeerConfig<SeqTypes>>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the inner BTreeMap
is unnecessary -- this should just be a BTreeMap<Version, (u64, PeerConfig<SeqTypes>)>
maybe @bfish713 can comment, but I think it's unlikely that we'd schedule an upgrade with multiple DA committees that take effect at different points in the future? and it definitely complicates the logic
ca9dcbd
to
a09bb19
Compare
This PR:
This PR does not:
Key places to review:
Example da_committees in data/genesis/X.toml: