-
Notifications
You must be signed in to change notification settings - Fork 180
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
Separate recent hashed values cache into separate types #2902
Changes from 3 commits
e40c1a3
4114648
b13db05
32587e2
3ac3326
a527744
98202c9
0e6baef
2cfad93
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 |
---|---|---|
|
@@ -84,10 +84,10 @@ use rand_distr::{Distribution, WeightedAliasIndex}; | |
use serde::{Deserialize, Serialize}; | ||
|
||
use crate::{ | ||
block::Timeout, | ||
block::{ConfirmedBlock, Timeout, ValidatedBlock}, | ||
data_types::{Block, BlockExecutionOutcome, BlockProposal, LiteVote, ProposalContent, Vote}, | ||
types::{ | ||
CertificateValue, ConfirmedBlockCertificate, HashedCertificateValue, TimeoutCertificate, | ||
ConfirmedBlockCertificate, Hashed, HashedCertificateValue, TimeoutCertificate, | ||
ValidatedBlockCertificate, | ||
}, | ||
ChainError, | ||
|
@@ -124,9 +124,12 @@ pub struct ChainManager { | |
/// Latest leader timeout certificate we have received. | ||
#[debug(skip_if = Option::is_none)] | ||
pub timeout: Option<TimeoutCertificate>, | ||
/// Latest vote we have cast, to validate or confirm. | ||
/// Latest vote we cast to confirm a block. | ||
#[debug(skip_if = Option::is_none)] | ||
pub pending: Option<Vote<CertificateValue>>, | ||
pub confirmed_vote: Option<Vote<ConfirmedBlock>>, | ||
/// Latest vote we cast to validate a block. | ||
#[debug(skip_if = Option::is_none)] | ||
pub validated_vote: Option<Vote<ValidatedBlock>>, | ||
/// Latest timeout vote we cast. | ||
#[debug(skip_if = Option::is_none)] | ||
pub timeout_vote: Option<Vote<Timeout>>, | ||
|
@@ -211,7 +214,8 @@ impl ChainManager { | |
proposed: None, | ||
locked: None, | ||
timeout: None, | ||
pending: None, | ||
confirmed_vote: None, | ||
validated_vote: None, | ||
timeout_vote: None, | ||
fallback_vote: None, | ||
round_timeout, | ||
|
@@ -221,12 +225,26 @@ impl ChainManager { | |
}) | ||
} | ||
|
||
/// Returns the most recent vote we cast. | ||
pub fn pending(&self) -> Option<&Vote<CertificateValue>> { | ||
self.pending.as_ref() | ||
/// Returns the most recent confirmed vote we cast. | ||
pub fn confirmed_vote(&self) -> Option<&Vote<ConfirmedBlock>> { | ||
self.confirmed_vote.as_ref() | ||
} | ||
|
||
/// Returns the most recent validated vote we cast. | ||
pub fn validated_vote(&self) -> Option<&Vote<ValidatedBlock>> { | ||
self.validated_vote.as_ref() | ||
} | ||
|
||
/// Returns the most recent timeout vote we cast. | ||
pub fn timeout_vote(&self) -> Option<&Vote<Timeout>> { | ||
self.timeout_vote.as_ref() | ||
} | ||
|
||
/// Returns the most recent fallback vote we cast. | ||
pub fn fallback_vote(&self) -> Option<&Vote<Timeout>> { | ||
self.fallback_vote.as_ref() | ||
} | ||
|
||
/// Verifies the safety of a proposed block with respect to voting rules. | ||
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. Why not? 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. You mean why I removed the comment? probably by mistake. 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. Done in 2cfad93 |
||
pub fn check_proposed_block(&self, proposal: &BlockProposal) -> Result<Outcome, ChainError> { | ||
let new_round = proposal.content.round; | ||
let new_block = &proposal.content.block; | ||
|
@@ -361,24 +379,16 @@ impl ChainManager { | |
) -> Result<Outcome, ChainError> { | ||
let new_block = &certificate.executed_block().block; | ||
let new_round = certificate.round; | ||
if let Some(Vote { value, round, .. }) = &self.pending { | ||
match value.inner() { | ||
CertificateValue::ConfirmedBlock(confirmed) => { | ||
if &confirmed.inner().block == new_block && *round == new_round { | ||
return Ok(Outcome::Skip); // We already voted to confirm this block. | ||
} | ||
} | ||
CertificateValue::ValidatedBlock(_) => { | ||
ensure!(new_round >= *round, ChainError::InsufficientRound(*round)) | ||
} | ||
CertificateValue::Timeout(_) => { | ||
// Unreachable: We only put validated or confirmed blocks in pending. | ||
return Err(ChainError::InternalError( | ||
"pending can only be validated or confirmed block".to_string(), | ||
)); | ||
} | ||
if let Some(Vote { value, round, .. }) = &self.confirmed_vote { | ||
if value.inner().inner().block == *new_block && *round == new_round { | ||
return Ok(Outcome::Skip); // We already voted to confirm this block. | ||
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. Now I'm wondering if we're missing a check in this function that, regardless of locked block or votes, the certificate is from at least the current round. I created #2914 for it. |
||
} | ||
} | ||
|
||
if let Some(Vote { round, .. }) = &self.validated_vote { | ||
ensure!(new_round >= *round, ChainError::InsufficientRound(*round)) | ||
} | ||
|
||
// We don't compare to `current_round` here: Non-validators must update their locked block | ||
// even if it is older than the current round. Validators will only sign in the current | ||
// round, though. (See `create_final_vote` below.) | ||
|
@@ -401,7 +411,7 @@ impl ChainManager { | |
outcome: BlockExecutionOutcome, | ||
key_pair: Option<&KeyPair>, | ||
local_time: Timestamp, | ||
) { | ||
) -> (&Option<Vote<ValidatedBlock>>, &Option<Vote<ConfirmedBlock>>) { | ||
// Record the proposed block, so it can be supplied to clients that request it. | ||
self.proposed = Some(proposal.clone()); | ||
self.update_current_round(local_time); | ||
|
@@ -428,13 +438,29 @@ impl ChainManager { | |
|
||
if let Some(key_pair) = key_pair { | ||
// If this is a fast block, vote to confirm. Otherwise vote to validate. | ||
let value = if round.is_fast() { | ||
HashedCertificateValue::new_confirmed(executed_block) | ||
if round.is_fast() { | ||
self.confirmed_vote = Some(Vote::new( | ||
HashedCertificateValue::new_confirmed(executed_block) | ||
.try_into() | ||
.expect("ConfirmedBlock"), | ||
Comment on lines
+444
to
+446
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. We could have a more direct constructor for that now. |
||
round, | ||
key_pair, | ||
)); | ||
self.validated_vote = None | ||
} else { | ||
HashedCertificateValue::new_validated(executed_block) | ||
self.validated_vote = Some(Vote::new( | ||
HashedCertificateValue::new_validated(executed_block) | ||
.try_into() | ||
.expect("ValidatedBlock"), | ||
round, | ||
key_pair, | ||
)); | ||
self.confirmed_vote = None; | ||
}; | ||
self.pending = Some(Vote::new(value, round, key_pair)); | ||
} | ||
|
||
// TODO: Is it guaranteed that if key_pair = None, then the vote is not signed ever? | ||
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. I want to bring reviewers' attention to this - i checked but want to confirm 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. Yes, these should only ever contain votes we signed with |
||
(&self.validated_vote, &self.confirmed_vote) | ||
} | ||
|
||
/// Signs a vote to confirm the validated block. | ||
|
@@ -455,11 +481,9 @@ impl ChainManager { | |
self.update_current_round(local_time); | ||
if let Some(key_pair) = key_pair { | ||
// Vote to confirm. | ||
// NOTE: For backwards compatibility, we need to turn `ValidatedBlockCertificate` | ||
// back into `Certificate` type so that the vote is cast over hash of the old type. | ||
let vote = Vote::new(confirmed.into_inner().into(), round, key_pair); | ||
let vote = Vote::new(confirmed.value().clone(), round, key_pair); | ||
// Ok to overwrite validation votes with confirmation votes at equal or higher round. | ||
self.pending = Some(vote); | ||
self.confirmed_vote = Some(vote); | ||
} | ||
} | ||
|
||
|
@@ -599,7 +623,10 @@ pub struct ChainManagerInfo { | |
pub fallback_vote: Option<LiteVote>, | ||
/// The value we voted for, if requested. | ||
#[debug(skip_if = Option::is_none)] | ||
pub requested_pending_value: Option<Box<HashedCertificateValue>>, | ||
pub requested_confirmed: Option<Box<Hashed<ConfirmedBlock>>>, | ||
/// The value we voted for, if requested. | ||
#[debug(skip_if = Option::is_none)] | ||
pub requested_validated: Option<Box<Hashed<ValidatedBlock>>>, | ||
/// The current round, i.e. the lowest round where we can still vote to validate a block. | ||
pub current_round: Round, | ||
/// The current leader, who is allowed to propose the next block. | ||
|
@@ -617,15 +644,21 @@ pub struct ChainManagerInfo { | |
impl From<&ChainManager> for ChainManagerInfo { | ||
fn from(manager: &ChainManager) -> Self { | ||
let current_round = manager.current_round; | ||
let pending = manager | ||
.confirmed_vote | ||
.as_ref() | ||
.map(|vote| vote.lite()) | ||
.or_else(move || manager.validated_vote.as_ref().map(|vote| vote.lite())); | ||
Comment on lines
+648
to
+652
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. Also this - it shouldn't happen that both 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. It looks like you made sure of that everywhere. Alternatively, we could avoid explicitly setting them to |
||
ChainManagerInfo { | ||
ownership: manager.ownership.clone(), | ||
requested_proposed: None, | ||
requested_locked: None, | ||
timeout: manager.timeout.clone().map(Box::new), | ||
pending: manager.pending.as_ref().map(|vote| vote.lite()), | ||
pending, | ||
timeout_vote: manager.timeout_vote.as_ref().map(Vote::lite), | ||
fallback_vote: manager.fallback_vote.as_ref().map(Vote::lite), | ||
requested_pending_value: None, | ||
requested_confirmed: None, | ||
requested_validated: None, | ||
current_round, | ||
leader: manager.round_leader(current_round).cloned(), | ||
round_timeout: manager.round_timeout, | ||
|
@@ -639,8 +672,12 @@ impl ChainManagerInfo { | |
pub fn add_values(&mut self, manager: &ChainManager) { | ||
self.requested_proposed = manager.proposed.clone().map(Box::new); | ||
self.requested_locked = manager.locked.clone().map(Box::new); | ||
self.requested_pending_value = manager | ||
.pending | ||
self.requested_confirmed = manager | ||
.confirmed_vote | ||
.as_ref() | ||
.map(|vote| Box::new(vote.value.clone())); | ||
self.requested_validated = manager | ||
.validated_vote | ||
.as_ref() | ||
.map(|vote| Box::new(vote.value.clone())); | ||
self.pending_blobs = manager.pending_blobs.clone(); | ||
|
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.
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.
Done in Fix comments