Skip to content

Commit

Permalink
[Consensus] fix GC missing blocks reported via the try_fetch_blocks m…
Browse files Browse the repository at this point in the history
…ethod (#20992)

## Description 

Blocks reported as missing via the `block_manager.try_fetch_blocks`
method because they are not added in the `missing_ancestors` map they
are not picked up for cleaning via the Garbage Collection mechanism.
Those blocks are then picked up by the periodic synchronizer to fetch -
which is unnecessary - and even worse, since the majority of the network
might have already rejected them there aren't many many authorities
around which actually have the blocks available to serve. That leads to
the periodic synchronizer constantly working and not able to fetch any
of those blocks.

This PR is fixing this behaviour by adding the blocks in the
`missing_ancestors` map so they are properly picked up by GC for clean
up.

## Test plan 

Wrote a test which confirmed the existing behaviour (blocks are stuck in
the block manager and not cleaned up). Then when applied the fix test
turned into Green.

CI/PT

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] gRPC:
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
  • Loading branch information
akichidis authored Jan 28, 2025
1 parent 90a0d61 commit 65fced6
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions consensus/core/src/block_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ impl BlockManager {
// Fetches the block if it is not in dag state or suspended.
missing_blocks.insert(*block_ref);
if self.missing_blocks.insert(*block_ref) {
// We want to report this as a missing ancestor even if there is no block that is actually references it right now. That will allow us
// to seamlessly GC the block later if needed.
self.missing_ancestors.entry(*block_ref).or_default();

let block_ref_hostname =
&self.context.committee.authority(block_ref.author).hostname;
self.context
Expand Down Expand Up @@ -994,6 +998,14 @@ mod tests {
}
assert!(!block_manager.is_empty());

// AND also call the try_to_find method with some non existing block refs. Those should be cleaned up as well once GC kicks in.
let non_existing_refs = (1..=3)
.map(|round| {
BlockRef::new(round, AuthorityIndex::new_for_test(0), BlockDigest::MIN)
})
.collect::<Vec<_>>();
assert_eq!(block_manager.try_find_blocks(non_existing_refs).len(), 3);

// AND
// Trigger a commit which will advance GC round
let last_commit = TrustedCommit::new_for_test(
Expand Down

0 comments on commit 65fced6

Please sign in to comment.