Skip to content
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

[ZKS-04] Properly set the gc_round on Storage initialization #3145

Merged
merged 3 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions node/bft/src/bft.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1226,4 +1226,71 @@ mod tests {
assert_eq!(result.unwrap_err().to_string(), error_msg);
Ok(())
}

#[tokio::test]
#[tracing_test::traced_test]
async fn test_bft_gc_on_commit() -> Result<()> {
let rng = &mut TestRng::default();

// Initialize the round parameters.
let max_gc_rounds = 1;
let committee_round = 0;
let commit_round = 2;
let current_round = commit_round + 1;

// Sample the certificates.
let (_, certificates) = snarkvm::ledger::narwhal::batch_certificate::test_helpers::sample_batch_certificate_with_previous_certificates(
current_round,
rng,
);

// Initialize the committee.
let committee = snarkvm::ledger::committee::test_helpers::sample_committee_for_round_and_members(
committee_round,
vec![
certificates[0].author(),
certificates[1].author(),
certificates[2].author(),
certificates[3].author(),
],
rng,
);

// Initialize the ledger.
let ledger = Arc::new(MockLedgerService::new(committee.clone()));

// Initialize the storage.
let transmissions = Arc::new(BFTMemoryService::new());
let storage = Storage::new(ledger.clone(), transmissions, max_gc_rounds);
// Insert the certificates into the storage.
for certificate in certificates.iter() {
storage.testing_only_insert_certificate_testing_only(certificate.clone());
}

// Get the leader certificate.
let leader = committee.get_leader(commit_round).unwrap();
let leader_certificate = storage.get_certificate_for_round_with_author(commit_round, leader).unwrap();

// Initialize the BFT.
let account = Account::new(rng)?;
let bft = BFT::new(account, storage.clone(), ledger, None, &[], None)?;
// Insert a mock DAG in the BFT.
*bft.dag.write() = crate::helpers::dag::test_helpers::mock_dag_with_modified_last_committed_round(commit_round);

// Ensure that the `gc_round` has not been updated yet.
assert_eq!(bft.storage().gc_round(), committee_round.saturating_sub(max_gc_rounds));

// Insert the certificates into the BFT.
for certificate in certificates {
assert!(bft.update_dag::<false>(certificate).await.is_ok());
}

// Commit the leader certificate.
bft.commit_leader_certificate::<false, false>(leader_certificate).await.unwrap();

// Ensure that the `gc_round` has been updated.
assert_eq!(bft.storage().gc_round(), commit_round - max_gc_rounds);

Ok(())
}
}
2 changes: 2 additions & 0 deletions node/bft/src/helpers/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ impl<N: Network> Storage<N> {
}));
// Update the storage to the current round.
storage.update_current_round(current_round);
// Perform GC on the current round.
storage.garbage_collect_certificates(current_round);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what is happening here. Shouldn't you garbage collect based on the last committed round instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case here is called in the constructor.

On bootup, we load the current_round which is technically the last committed round from the ledger's perspective.

I believe this is equivalent to what your intention is as well.

// Return the storage.
storage
}
Expand Down
2 changes: 2 additions & 0 deletions node/bft/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ impl<N: Network> Sync<N> {
self.storage.sync_height_with_block(latest_block.height());
// Sync the round with the block.
self.storage.sync_round_with_block(latest_block.round());
// Perform GC on the latest block round.
self.storage.garbage_collect_certificates(latest_block.round());
// Iterate over the blocks.
for block in &blocks {
// If the block authority is a subdag, then sync the batch certificates with the block.
Expand Down
30 changes: 30 additions & 0 deletions node/bft/src/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,8 @@ mod tests {

type CurrentNetwork = snarkvm::prelude::MainnetV0;

const ITERATIONS: usize = 100;

mock! {
Gateway<N: Network> {}
#[async_trait]
Expand Down Expand Up @@ -868,6 +870,34 @@ mod tests {
assert!(!worker.pending.contains(transmission_id));
assert!(worker.ready.contains(transmission_id));
}

#[tokio::test]
async fn test_storage_gc_on_initialization() {
let rng = &mut TestRng::default();

for _ in 0..ITERATIONS {
// Mock the ledger round.
let max_gc_rounds = rng.gen_range(50..=100);
let latest_ledger_round = rng.gen_range((max_gc_rounds + 1)..1000);
let expected_gc_round = latest_ledger_round - max_gc_rounds;

// Sample a committee.
let committee =
snarkvm::ledger::committee::test_helpers::sample_committee_for_round(latest_ledger_round, rng);

// Setup the mock gateway and ledger.
let mut mock_ledger = MockLedger::default();
mock_ledger.expect_current_committee().returning(move || Ok(committee.clone()));

let ledger: Arc<dyn LedgerService<CurrentNetwork>> = Arc::new(mock_ledger);
// Initialize the storage.
let storage =
Storage::<CurrentNetwork>::new(ledger.clone(), Arc::new(BFTMemoryService::new()), max_gc_rounds);

// Ensure that the storage GC round is correct.
assert_eq!(storage.gc_round(), expected_gc_round);
}
}
}

#[cfg(test)]
Expand Down