Skip to content

Commit

Permalink
Merge branch 'CON-1127' into 'master'
Browse files Browse the repository at this point in the history 
feat(backup, CON-1127): only backup notarizations of finalized chain

With this change, the replicas will only backup notarizations belonging to the finalized chain. 

See merge request dfinity-lab/public/ic!14658
  • Loading branch information
@chmllr
chmllr committed Sep 13, 2023
2 parents db40584 + 26849b1 commit 0a3c17d
Showing 1 changed file with 61 additions and 36 deletions.
97 changes: 61 additions & 36 deletions rs/artifact_pool/src/consensus_pool.rs
View file
Expand Up @@ -501,9 +501,9 @@ impl ConsensusPoolImpl {
}
}

// Checks if the artifacts to be backed up contain a new finalization, and if it is the case, we simply traverse the blockchain starting from the hash
// of the new finalization back to the previous finalization and add to the list of artifacts
// all block proposals that are now provably belong to the finalized chain.
// Persists consensus artifacts required for backup validation. If the provided artifacts contain a new finalization,
// the function traverses the blockchain backwards to the last available finalization and additionally persists all
// block proposals with their notarizations that are now provably belong to the finalized chain.
fn backup_artifacts(
&self,
backup: &Backup,
Expand Down Expand Up @@ -535,18 +535,28 @@ impl ConsensusPoolImpl {
})
});

if let Some(proposal) = finalized_proposal {
artifacts_for_backup.extend(
self.as_cache()
.chain_iterator(self, proposal.content.into_inner())
.take_while(|block| block.height > latest_finalization_height)
.filter_map(|block| {
find_proposal_by(block.height, &|proposal: &BlockProposal| {
proposal.content.as_ref() == &block
})
if let Some(finalized_proposal) = finalized_proposal {
for proposal in self
.as_cache()
.chain_iterator(self, finalized_proposal.content.into_inner())
.take_while(|block| block.height > latest_finalization_height)
.filter_map(|block| {
find_proposal_by(block.height, &|proposal: &BlockProposal| {
proposal.content.as_ref() == &block
})
.map(ConsensusMessage::BlockProposal),
);
})
{
if let Some(notarization) = self
.validated()
.notarization()
.get_by_height(proposal.content.height())
.find(|notarization| proposal.content.get_hash() == &notarization.content.block)
{
artifacts_for_backup.push(ConsensusMessage::Notarization(notarization))
}

artifacts_for_backup.push(ConsensusMessage::BlockProposal(proposal));
}
}

backup.store(time_source, artifacts_for_backup);
Expand Down Expand Up @@ -652,9 +662,9 @@ impl MutablePool<ConsensusArtifact, ChangeSet> for ConsensusPoolImpl {
.filter_map(|op| match op {
PoolSectionOp::Insert(artifact)
// When we prepare a list of artifacts for a backup, we first remove all
// block proposals. We need to do this to avoid "polluting" the backup partition with non-
// finalized blocks, which are the largest artifacts.
if !matches!(&artifact.msg, &ConsensusMessage::BlockProposal(_)) =>
// block proposals and notarizations. We need to do this to avoid "polluting" the backup
// partition with non-finalized blocks, which are the largest artifacts.
if !matches!(&artifact.msg, &ConsensusMessage::BlockProposal(_)) && !matches!(&artifact.msg, &ConsensusMessage::Notarization(_)) =>
{
Some(artifact.msg.clone())
}
Expand Down Expand Up @@ -1373,6 +1383,10 @@ mod tests {
},
);
let proposal3_final = BlockProposal::fake(block.clone(), node_test_id(333));
let notarization3 = Notarization::fake(NotarizationContent::new(
Height::from(3),
ic_types::crypto::crypto_hash(&block),
));

let block = Block::new(
ic_types::crypto::crypto_hash(&block),
Expand Down Expand Up @@ -1430,6 +1444,7 @@ mod tests {
proposal.clone().into_message(),
proposal_non_final.clone().into_message(),
proposal3.clone().into_message(),
notarization3.clone().into_message(),
proposal3_final.clone().into_message(),
cup.clone().into_message(),
]
Expand Down Expand Up @@ -1497,14 +1512,13 @@ mod tests {
path.join("2").join("random_tape.bin").exists(),
"random tape at height 2 was backed up"
);
assert!(
notarization_path.exists(),
"notarization at height 2 was backed up"
);
// notarization at height 2 was not backed up becasue this is height is not
// finalized
assert!(!notarization_path.exists());
assert_eq!(
fs::read_dir(path.join("2")).unwrap().count(),
2,
"only two artifacts for height 2 were backed up"
1,
"only one artifact for height 2 was backed up"
);
let mut file = fs::File::open(path.join("2").join("random_tape.bin")).unwrap();
let mut buffer = Vec::new();
Expand All @@ -1515,17 +1529,6 @@ mod tests {
random_tape, restored,
"restored random tape is identical with the original one"
);
let mut file = fs::File::open(notarization_path).unwrap();
let mut buffer = Vec::new();
file.read_to_end(&mut buffer).unwrap();
let restored =
Notarization::try_from(pb::Notarization::decode(buffer.as_slice()).unwrap())
.unwrap();
assert_eq!(
notarization, restored,
"restored notarization is identical with the original one"
);

// Check backup for height 3
let finalization_path = path.join("3").join(format!(
"finalization_{}_{}.bin",
Expand All @@ -1538,8 +1541,8 @@ mod tests {
);
assert_eq!(
fs::read_dir(path.join("3")).unwrap().count(),
2,
"only two artifact for height 3 was backed up"
3,
"only three artifact for height 3 were backed up"
);
let mut file = fs::File::open(path.join("3").join(finalization_path)).unwrap();
let mut buffer = Vec::new();
Expand Down Expand Up @@ -1568,6 +1571,26 @@ mod tests {
));
assert!(proposal_path.exists(), "final proposal was backed up");

let notarization_path = path.join("3").join(format!(
"notarization_{}_{}.bin",
bytes_to_hex_str(&notarization3.content.block),
bytes_to_hex_str(&ic_types::crypto::crypto_hash(&notarization3)),
));
assert!(
notarization_path.exists(),
"notarization at height 3 was backed up",
);
let mut file = fs::File::open(notarization_path).unwrap();
let mut buffer = Vec::new();
file.read_to_end(&mut buffer).unwrap();
let restored =
Notarization::try_from(pb::Notarization::decode(buffer.as_slice()).unwrap())
.unwrap();
assert_eq!(
notarization3, restored,
"restored notarization is identical with the original one"
);

// Check backup for height 4
assert!(
path.join("4").join("catch_up_package.bin").exists(),
Expand Down Expand Up @@ -1713,6 +1736,7 @@ mod tests {
CryptoHashOf::from(CryptoHash(Vec::new())),
));
let random_tape = RandomTape::fake(RandomTapeContent::new(Height::from(2)));
let random_tape3 = RandomTape::fake(RandomTapeContent::new(Height::from(3)));
let notarization = Notarization::fake(NotarizationContent::new(
Height::from(3),
CryptoHashOf::from(CryptoHash(vec![1, 2, 3])),
Expand Down Expand Up @@ -1763,6 +1787,7 @@ mod tests {
// Now add new artifacts
let changeset = vec![
notarization.into_message(),
random_tape3.into_message(),
proposal.into_message(),
finalization.into_message(),
]
Expand Down

0 comments on commit 0a3c17d

Please sign in to comment.