From 8668510f5a5b335c3c2a6219736d42693372d929 Mon Sep 17 00:00:00 2001 From: George Danezis Date: Tue, 7 Jun 2022 12:01:08 +0100 Subject: [PATCH] [checkpoints] Added a previous hash to the checkpoint (#2433) * Added a previous hash to the checkpoint * Fix comment Co-authored-by: George Danezis --- .../authority_active/checkpoint_driver/mod.rs | 10 ++-- crates/sui-core/src/checkpoints/mod.rs | 54 +++++++++++++++---- .../src/checkpoints/tests/checkpoint_tests.rs | 12 +++-- crates/sui-types/src/messages_checkpoint.rs | 26 +++++---- 4 files changed, 76 insertions(+), 26 deletions(-) diff --git a/crates/sui-core/src/authority_active/checkpoint_driver/mod.rs b/crates/sui-core/src/authority_active/checkpoint_driver/mod.rs index 72d4b9f6409ff..6e79b1059d724 100644 --- a/crates/sui-core/src/authority_active/checkpoint_driver/mod.rs +++ b/crates/sui-core/src/authority_active/checkpoint_driver/mod.rs @@ -15,8 +15,8 @@ use sui_types::{ messages::{CertifiedTransaction, ConfirmationTransaction, TransactionInfoRequest}, messages_checkpoint::{ AuthenticatedCheckpoint, AuthorityCheckpointInfo, CertifiedCheckpoint, CheckpointContents, - CheckpointFragment, CheckpointRequest, CheckpointResponse, CheckpointSequenceNumber, - SignedCheckpoint, SignedCheckpointProposal, + CheckpointDigest, CheckpointFragment, CheckpointRequest, CheckpointResponse, + CheckpointSequenceNumber, SignedCheckpoint, SignedCheckpointProposal, }, }; use tokio::time::timeout; @@ -329,7 +329,7 @@ where // Attempt to construct a newer checkpoint from signed summaries. #[allow(clippy::type_complexity)] let mut partial_checkpoints: BTreeMap< - (CheckpointSequenceNumber, [u8; 32]), + (CheckpointSequenceNumber, CheckpointDigest), Vec<(AuthorityName, SignedCheckpoint)>, > = BTreeMap::new(); final_state @@ -553,8 +553,8 @@ where info: _info, detail: Some(contents), }) => { - // TODO: check here that the digest of contents matches - if contents.digest() != checkpoint.checkpoint.digest { + // Check here that the digest of contents matches + if contents.digest() != checkpoint.checkpoint.content_digest { // A byzantine authority! // TODO: Report Byzantine authority warn!("Sync Error: Incorrect contents returned"); diff --git a/crates/sui-core/src/checkpoints/mod.rs b/crates/sui-core/src/checkpoints/mod.rs index 7a06f444afa39..3e43a552e2f04 100644 --- a/crates/sui-core/src/checkpoints/mod.rs +++ b/crates/sui-core/src/checkpoints/mod.rs @@ -22,8 +22,8 @@ use sui_types::{ messages::CertifiedTransaction, messages_checkpoint::{ AuthenticatedCheckpoint, AuthorityCheckpointInfo, CertifiedCheckpoint, CheckpointContents, - CheckpointFragment, CheckpointRequest, CheckpointResponse, CheckpointSequenceNumber, - CheckpointSummary, SignedCheckpoint, SignedCheckpointProposal, + CheckpointDigest, CheckpointFragment, CheckpointRequest, CheckpointResponse, + CheckpointSequenceNumber, CheckpointSummary, SignedCheckpoint, SignedCheckpointProposal, }, }; use typed_store::{ @@ -138,6 +138,26 @@ pub struct CheckpointStore { } impl CheckpointStore { + fn get_prev_checkpoint_digest( + &mut self, + checkpoint_sequence: CheckpointSequenceNumber, + ) -> Result, SuiError> { + // Extract the previous checkpoint digest if there is one. + Ok(if checkpoint_sequence > 0 { + self.checkpoints + .get(&(checkpoint_sequence - 1))? + .map(|prev_checkpoint| match prev_checkpoint { + AuthenticatedCheckpoint::Certified(cert) => cert.checkpoint.digest(), + AuthenticatedCheckpoint::Signed(signed) => signed.checkpoint.digest(), + _ => { + unreachable!(); + } + }) + } else { + None + }) + } + // Manage persistent local variables /// Loads the locals from the store -- do this at init @@ -154,19 +174,20 @@ impl CheckpointStore { // Recreate the proposal if locals.proposal_next_transaction.is_some() { - let checkpoint = locals.next_checkpoint; + let checkpoint_sequence = locals.next_checkpoint; let transactions = self .extra_transactions .iter() .filter(|(_, seq)| seq < locals.proposal_next_transaction.as_ref().unwrap()) .map(|(digest, _)| digest); - let transactions = CheckpointContents::new(transactions); + let previous_digest = self.get_prev_checkpoint_digest(checkpoint_sequence)?; let proposal = SignedCheckpointProposal(SignedCheckpoint::new( - checkpoint, + checkpoint_sequence, self.name, &*self.secret, &transactions, + previous_digest, )); let proposal_and_transactions = CheckpointProposal::new(proposal, transactions); @@ -644,7 +665,11 @@ impl CheckpointStore { .map_err(|e| FragmentInternalError::Error(e.into()))?; // Now create the new checkpoint and move all locals forward. - let summary = CheckpointSummary::new(next_sequence_number, &contents); + let previous_digest = self + .get_prev_checkpoint_digest(next_sequence_number) + .map_err(FragmentInternalError::Error)?; + let summary = + CheckpointSummary::new(next_sequence_number, &contents, previous_digest); self.handle_internal_set_checkpoint(summary, &contents) .map_err(FragmentInternalError::Error)?; return Ok(true); @@ -693,7 +718,14 @@ impl CheckpointStore { .map_err(|e| FragmentInternalError::Error(e.into()))?; let contents = CheckpointContents::new(contents.into_iter()); - let summary = CheckpointSummary::new(next_sequence_number, &contents); + let previous_digest = self + .get_prev_checkpoint_digest(next_sequence_number) + .map_err(FragmentInternalError::Error)?; + let summary = CheckpointSummary::new( + next_sequence_number, + &contents, + previous_digest, + ); self.handle_internal_set_checkpoint(summary, &contents) .map_err(FragmentInternalError::Error)?; return Ok(true); @@ -880,15 +912,19 @@ impl CheckpointStore { // Include the sequence number of all extra transactions not already in a // checkpoint. And make a list of the transactions. - let sequence_number = self.next_checkpoint(); + let checkpoint_sequence = self.next_checkpoint(); let next_local_tx_sequence = self.extra_transactions.values().max().unwrap() + 1; + // Extract the previous checkpoint digest if there is one. + let previous_digest = self.get_prev_checkpoint_digest(checkpoint_sequence)?; + let transactions = CheckpointContents::new(self.extra_transactions.keys()); let proposal = SignedCheckpointProposal(SignedCheckpoint::new( - sequence_number, + checkpoint_sequence, self.name, &*self.secret, &transactions, + previous_digest, )); let proposal_and_transactions = CheckpointProposal::new(proposal, transactions); diff --git a/crates/sui-core/src/checkpoints/tests/checkpoint_tests.rs b/crates/sui-core/src/checkpoints/tests/checkpoint_tests.rs index a4b1c5c8871a7..ddf54427c6be4 100644 --- a/crates/sui-core/src/checkpoints/tests/checkpoint_tests.rs +++ b/crates/sui-core/src/checkpoints/tests/checkpoint_tests.rs @@ -390,7 +390,7 @@ fn latest_proposal() { .collect(); let transactions = CheckpointContents::new(ckp_items.clone().into_iter()); - let summary = CheckpointSummary::new(0, &transactions); + let summary = CheckpointSummary::new(0, &transactions, None); cps1.handle_internal_set_checkpoint(summary.clone(), &transactions) .unwrap(); @@ -529,7 +529,7 @@ fn set_get_checkpoint() { .cloned(); let transactions = CheckpointContents::new(ckp_items); - let summary = CheckpointSummary::new(0, &transactions); + let summary = CheckpointSummary::new(0, &transactions, None); cps1.handle_internal_set_checkpoint(summary.clone(), &transactions) .unwrap(); @@ -673,7 +673,13 @@ fn checkpoint_integration() { .chain(some_fresh_transactions.iter().cloned().map(|(_, d)| d)) .collect(); let transactions = CheckpointContents::new(unprocessed.clone().into_iter()); - let summary = CheckpointSummary::new(cps.get_locals().next_checkpoint, &transactions); + let next_checkpoint = cps.get_locals().next_checkpoint; + let summary = CheckpointSummary::new( + next_checkpoint, + &transactions, + cps.get_prev_checkpoint_digest(next_checkpoint) + .expect("previous checkpoint should exist"), + ); cps.handle_internal_set_checkpoint(summary.clone(), &transactions) .unwrap(); diff --git a/crates/sui-types/src/messages_checkpoint.rs b/crates/sui-types/src/messages_checkpoint.rs index ee2c0c12e5693..bb0a5b9b7ad8b 100644 --- a/crates/sui-types/src/messages_checkpoint.rs +++ b/crates/sui-types/src/messages_checkpoint.rs @@ -183,7 +183,8 @@ pub type CheckpointDigest = [u8; 32]; pub struct CheckpointSummary { pub sequence_number: CheckpointSequenceNumber, pub waypoint: Box, // Bigger structure, can live on heap. - pub digest: CheckpointDigest, + pub content_digest: CheckpointDigest, + pub previous_digest: Option, // TODO: add digest of previous checkpoint summary } @@ -191,18 +192,20 @@ impl CheckpointSummary { pub fn new( sequence_number: CheckpointSequenceNumber, transactions: &CheckpointContents, + previous_digest: Option, ) -> CheckpointSummary { let mut waypoint = Box::new(Waypoint::default()); transactions.transactions.iter().for_each(|tx| { waypoint.insert(tx); }); - let proposal_digest = transactions.digest(); + let content_digest = transactions.digest(); Self { sequence_number, waypoint, - digest: proposal_digest, + content_digest, + previous_digest, } } @@ -232,8 +235,9 @@ impl SignedCheckpoint { authority: AuthorityName, signer: &dyn signature::Signer, transactions: &CheckpointContents, + previous_digest: Option, ) -> SignedCheckpoint { - let checkpoint = CheckpointSummary::new(sequence_number, transactions); + let checkpoint = CheckpointSummary::new(sequence_number, transactions, previous_digest); SignedCheckpoint::new_from_summary(checkpoint, authority, signer) } @@ -260,7 +264,11 @@ impl SignedCheckpoint { // Check that the digest and transactions are correctly signed pub fn verify_with_transactions(&self, contents: &CheckpointContents) -> Result<(), SuiError> { self.verify()?; - let recomputed = CheckpointSummary::new(*self.checkpoint.sequence_number(), contents); + let recomputed = CheckpointSummary::new( + *self.checkpoint.sequence_number(), + contents, + self.checkpoint.previous_digest, + ); fp_ensure!( recomputed == self.checkpoint, @@ -382,7 +390,7 @@ impl CertifiedCheckpoint { ) -> Result<(), SuiError> { self.verify(committee)?; fp_ensure!( - contents.digest() == self.checkpoint.digest, + contents.digest() == self.checkpoint.content_digest, SuiError::from("Transaction digest mismatch") ); Ok(()) @@ -482,7 +490,7 @@ mod tests { let set = [ExecutionDigests::random()]; let set = CheckpointContents::new(set.iter().cloned()); - let mut proposal = SignedCheckpoint::new(1, *name, &authority_key[0], &set); + let mut proposal = SignedCheckpoint::new(1, *name, &authority_key[0], &set, None); // Signature is correct on proposal, and with same transactions assert!(proposal.verify().is_ok()); @@ -512,7 +520,7 @@ mod tests { .map(|k| { let name = k.public_key_bytes(); - SignedCheckpoint::new(1, *name, k, &set) + SignedCheckpoint::new(1, *name, k, &set, None) }) .collect(); @@ -532,7 +540,7 @@ mod tests { let set: BTreeSet<_> = [ExecutionDigests::random()].into_iter().collect(); let set = CheckpointContents::new(set.iter().cloned()); - SignedCheckpoint::new(1, *name, k, &set) + SignedCheckpoint::new(1, *name, k, &set, None) }) .collect();