-
Notifications
You must be signed in to change notification settings - Fork 103
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
2652 Membership::set_first_epoch #2654
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ use committable::Committable; | |
use hotshot_types::{ | ||
consensus::OuterConsensus, | ||
data::{Leaf2, QuorumProposalWrapper, VidDisperseShare}, | ||
drb::{compute_drb_result, DrbResult}, | ||
drb::{compute_drb_result, DrbResult, INITIAL_DRB_RESULT}, | ||
event::{Event, EventType}, | ||
message::{Proposal, UpgradeLock}, | ||
simple_vote::{HasEpoch, QuorumData2, QuorumVote2}, | ||
|
@@ -51,6 +51,7 @@ async fn notify_membership_of_drb_result<TYPES: NodeType>( | |
epoch: <TYPES as NodeType>::Epoch, | ||
drb_result: DrbResult, | ||
) { | ||
tracing::debug!("Calling add_drb_result for epoch {:?}", epoch); | ||
membership.write().await.add_drb_result(epoch, drb_result); | ||
} | ||
|
||
|
@@ -379,7 +380,7 @@ pub(crate) async fn handle_quorum_proposal_validated< | |
) | ||
.await | ||
} else { | ||
decide_from_proposal( | ||
decide_from_proposal::<TYPES, V>( | ||
proposal, | ||
OuterConsensus::new(Arc::clone(&task_state.consensus.inner_consensus)), | ||
Arc::clone(&task_state.upgrade_lock.decided_upgrade_certificate), | ||
|
@@ -390,21 +391,58 @@ pub(crate) async fn handle_quorum_proposal_validated< | |
.await | ||
}; | ||
|
||
if let Some(cert) = &task_state.staged_epoch_upgrade_certificate { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clear this option after we apply the upgrade so we don't hit this on every proposal |
||
if leaf_views.last().unwrap().leaf.height() >= task_state.epoch_upgrade_block_height { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here we are waiting to apply the upgrade until the configured height, but we already set the first epoch, I think that needs to happen here |
||
let mut decided_certificate_lock = task_state | ||
.upgrade_lock | ||
.decided_upgrade_certificate | ||
.write() | ||
.await; | ||
*decided_certificate_lock = Some(cert.clone()); | ||
drop(decided_certificate_lock); | ||
|
||
let _ = task_state | ||
.storage | ||
.write() | ||
.await | ||
.update_decided_upgrade_certificate(Some(cert.clone())) | ||
.await; | ||
|
||
task_state.staged_epoch_upgrade_certificate = None; | ||
} | ||
}; | ||
|
||
if let Some(cert) = decided_upgrade_cert.clone() { | ||
let mut decided_certificate_lock = task_state | ||
.upgrade_lock | ||
.decided_upgrade_certificate | ||
.write() | ||
.await; | ||
*decided_certificate_lock = Some(cert.clone()); | ||
drop(decided_certificate_lock); | ||
if cert.data.new_version == V::Epochs::VERSION { | ||
task_state.staged_epoch_upgrade_certificate = Some(cert); | ||
|
||
let _ = task_state | ||
.storage | ||
.write() | ||
.await | ||
.update_decided_upgrade_certificate(Some(cert.clone())) | ||
.await; | ||
let epoch_height = task_state.consensus.read().await.epoch_height; | ||
let first_epoch_number = TYPES::Epoch::new(epoch_from_block_number( | ||
task_state.epoch_upgrade_block_height, | ||
epoch_height, | ||
)); | ||
tracing::debug!("Calling set_first_epoch for epoch {:?}", first_epoch_number); | ||
task_state | ||
.membership | ||
.write() | ||
.await | ||
.set_first_epoch(first_epoch_number, INITIAL_DRB_RESULT); | ||
} else { | ||
let mut decided_certificate_lock = task_state | ||
.upgrade_lock | ||
.decided_upgrade_certificate | ||
.write() | ||
.await; | ||
*decided_certificate_lock = Some(cert.clone()); | ||
drop(decided_certificate_lock); | ||
|
||
let _ = task_state | ||
.storage | ||
.write() | ||
.await | ||
.update_decided_upgrade_certificate(Some(cert.clone())) | ||
.await; | ||
} | ||
} | ||
|
||
let mut consensus_writer = task_state.consensus.write().await; | ||
|
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.
FYI this function stops being called when we upgrade, decide_from_proposal_2 gets activated, any logic specific to epochs would need to be in there too
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.
my understanding here was that once we're in epochs we don't want to set_first_epoch any more anyway. Either way this logic moved due to the next comment
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.
you are correct once we in epochs we don't have to worry about epoch upgrade stuff