Separate recent hashed values cache into separate types (#2902)

* Separate recent hashed values cache into separate types

* Get rid of unnecessary clones

* Replace call to contains with pulling cert directly

* In create_final_vote, set validated_vote to None

* make-X methods in tests work with new certificate types

* Fix tests

* Fix comments

* RM todo comment

* Bring back rm'd comment

linera-chain/src/block.rs

@@ -38,12 +38,32 @@ impl ValidatedBlock {
 
 impl BcsHashable for ValidatedBlock {}
 
+impl Has<ChainId> for ValidatedBlock {
+    fn get(&self) -> &ChainId {
+        &self.executed_block.block.chain_id
+    }
+}
+
 impl From<ValidatedBlock> for HashedCertificateValue {
     fn from(value: ValidatedBlock) -> Self {
         HashedCertificateValue::new_validated(value.executed_block)
     }
 }
 
+impl TryFrom<HashedCertificateValue> for Hashed<ValidatedBlock> {
+    type Error = &'static str;
+
+    fn try_from(value: HashedCertificateValue) -> Result<Self, Self::Error> {
+        let hash = value.hash();
+        match value.into_inner() {
+            CertificateValue::ValidatedBlock(validated) => {
+                Ok(Hashed::unchecked_new(validated, hash))
+            }
+            _ => Err("Expected a ValidatedBlock value"),
+        }
+    }
+}
+
 /// Wrapper around an `ExecutedBlock` that has been confirmed.
 #[derive(Debug, PartialEq, Eq, Hash, Clone, Deserialize, Serialize)]
 pub struct ConfirmedBlock {
@@ -57,6 +77,20 @@ impl From<ConfirmedBlock> for HashedCertificateValue {
     }
 }
 
+impl TryFrom<HashedCertificateValue> for Hashed<ConfirmedBlock> {
+    type Error = &'static str;
+
+    fn try_from(value: HashedCertificateValue) -> Result<Self, Self::Error> {
+        let hash = value.hash();
+        match value.into_inner() {
+            CertificateValue::ConfirmedBlock(confirmed) => {
+                Ok(Hashed::unchecked_new(confirmed, hash))
+            }
+            _ => Err("Expected a ConfirmedBlock value"),
+        }
+    }
+}
+
 impl BcsHashable for ConfirmedBlock {}
 
 impl ConfirmedBlock {
@@ -90,6 +124,12 @@ impl ConfirmedBlock {
     }
 }
 
+impl Has<ChainId> for ConfirmedBlock {
+    fn get(&self) -> &ChainId {
+        &self.executed_block.block.chain_id
+    }
+}
+
 #[derive(Debug, PartialEq, Eq, Hash, Clone, Deserialize, Serialize)]
 pub struct Timeout {
     pub chain_id: ChainId,

linera-chain/src/certificate/generic.rs

@@ -35,10 +35,16 @@ impl<T> GenericCertificate<T> {
         }
     }
 
+    /// Returns a reference to the `Hashed` value contained in this certificate.
     pub fn value(&self) -> &Hashed<T> {
         &self.value
     }
 
+    /// Consumes this certificate, returning the value it contains.
+    pub fn into_value(self) -> Hashed<T> {
+        self.value
+    }
+
     /// Returns reference to the value contained in this certificate.
     pub fn inner(&self) -> &T {
         self.value.inner()

linera-chain/src/certificate/lite.rs

@@ -4,11 +4,11 @@
 
 use std::borrow::Cow;
 
-use linera_base::{crypto::Signature, data_types::Round};
+use linera_base::{crypto::Signature, data_types::Round, identifiers::ChainId};
 use linera_execution::committee::{Committee, ValidatorName};
 use serde::{Deserialize, Serialize};
 
-use super::{Certificate, HashedCertificateValue};
+use super::{GenericCertificate, Has, Hashed};
 use crate::{
     data_types::{check_signatures, LiteValue, LiteVote},
     ChainError,
@@ -42,7 +42,7 @@ impl<'a> LiteCertificate<'a> {
         }
     }
 
-    /// Creates a `LiteCertificate` from a list of votes, without cryptographically checking the
+    /// Creates a [`LiteCertificate`] from a list of votes, without cryptographically checking the
     /// signatures. Returns `None` if the votes are empty or don't have matching values and rounds.
     pub fn try_from_votes(votes: impl IntoIterator<Item = LiteVote>) -> Option<Self> {
         let mut votes = votes.into_iter();
@@ -73,20 +73,19 @@ impl<'a> LiteCertificate<'a> {
         Ok(&self.value)
     }
 
-    /// Returns the `Certificate` with the specified value, if it matches.
-    pub fn with_value(self, value: HashedCertificateValue) -> Option<Certificate> {
-        if self.value.chain_id != value.inner().chain_id() || self.value.value_hash != value.hash()
-        {
+    /// Returns the [`GenericCertificate`] with the specified value, if it matches.
+    pub fn with_value<T: Has<ChainId>>(self, value: Hashed<T>) -> Option<GenericCertificate<T>> {
+        if &self.value.chain_id != value.inner().get() || self.value.value_hash != value.hash() {
             return None;
         }
-        Some(Certificate::new(
+        Some(GenericCertificate::new(
             value,
             self.round,
             self.signatures.into_owned(),
         ))
     }
 
-    /// Returns a `LiteCertificate` that owns the list of signatures.
+    /// Returns a [`LiteCertificate`] that owns the list of signatures.
     pub fn cloned(&self) -> LiteCertificate<'static> {
         LiteCertificate {
             value: self.value.clone(),

linera-chain/src/manager.rs

@@ -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,9 +225,24 @@ 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.
@@ -361,24 +380,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.
             }
         }
+
+        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 +412,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 +439,28 @@ 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"),
+                    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));
         }
+
+        (&self.validated_vote, &self.confirmed_vote)
     }
 
     /// Signs a vote to confirm the validated block.
@@ -455,11 +481,10 @@ 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);
+            self.validated_vote = None;
         }
     }
 
@@ -599,7 +624,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 +645,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()));
         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 +673,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();

linera-chain/src/test.rs

@@ -15,21 +15,22 @@ use linera_execution::{
 };
 
 use crate::{
+    block::ConfirmedBlock,
     data_types::{Block, BlockProposal, IncomingBundle, PostedMessage, SignatureAggregator, Vote},
-    types::{GenericCertificate, HashedCertificateValue},
+    types::{GenericCertificate, Hashed},
 };
 
 /// Creates a new child of the given block, with the same timestamp.
-pub fn make_child_block(parent: &HashedCertificateValue) -> Block {
+pub fn make_child_block(parent: &Hashed<ConfirmedBlock>) -> Block {
     let parent_value = parent.inner();
-    let parent_block = parent_value.block().unwrap();
+    let parent_block = &parent_value.inner().block;
     Block {
-        epoch: parent_value.epoch(),
-        chain_id: parent_value.chain_id(),
+        epoch: parent_block.epoch,
+        chain_id: parent_block.chain_id,
         incoming_bundles: vec![],
         operations: vec![],
         previous_block_hash: Some(parent.hash()),
-        height: parent_value.height().try_add_one().unwrap(),
+        height: parent_block.height.try_add_one().unwrap(),
         authenticated_signer: parent_block.authenticated_signer,
         timestamp: parent_block.timestamp,
     }

linera-chain/src/unit_tests/chain_tests.rs

@@ -250,7 +250,7 @@ async fn test_application_permissions() {
     let value = HashedCertificateValue::new_confirmed(outcome.with(valid_block));
 
     // In the second block, other operations are still not allowed.
-    let invalid_block = make_child_block(&value)
+    let invalid_block = make_child_block(&value.clone().try_into().unwrap())
         .with_simple_transfer(chain_id, Amount::ONE)
         .with_operation(app_operation.clone());
     let result = chain.execute_block(&invalid_block, time, None).await;
@@ -259,7 +259,7 @@ async fn test_application_permissions() {
     );
 
     // Also, blocks without an application operation or incoming message are forbidden.
-    let invalid_block = make_child_block(&value);
+    let invalid_block = make_child_block(&value.clone().try_into().unwrap());
     let result = chain.execute_block(&invalid_block, time, None).await;
     assert_matches!(result, Err(ChainError::MissingMandatoryApplications(app_ids))
         if app_ids == vec![application_id]
@@ -268,6 +268,6 @@ async fn test_application_permissions() {
     // But app operations continue to work.
     application.expect_call(ExpectedCall::execute_operation(|_, _, _| Ok(vec![])));
     application.expect_call(ExpectedCall::default_finalize());
-    let valid_block = make_child_block(&value).with_operation(app_operation);
+    let valid_block = make_child_block(&value.try_into().unwrap()).with_operation(app_operation);
     chain.execute_block(&valid_block, time, None).await.unwrap();
 }