Skip to content

Fix flaky test_same_subnet_unsubscription() #8609

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.

Sign up for GitHub

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

Open
figtracer wants to merge 3 commits into
base: stable
Choose a base branch
from
+17 −25
Open

Fix flaky test_same_subnet_unsubscription() #8609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ecf2959
small fix
figtracer Dec 21, 2025
84c393a
fix
figtracer Dec 22, 2025
53640f4
new approach
figtracer Dec 22, 2025
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
7 changes: 0 additions & 7 deletions beacon_node/network/src/subnet_service/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,13 +198,6 @@ impl<T: BeaconChainTypes> SubnetService<T> {
self.permanent_attestation_subscriptions.iter()
}

/// Returns whether we are subscribed to a subnet for testing purposes.
#[cfg(test)]
pub(crate) fn is_subscribed(&self, subnet: &Subnet) -> bool {
self.subscriptions.contains_key(subnet)
|| self.permanent_attestation_subscriptions.contains(subnet)
}

/// Returns whether we are subscribed to a permanent subnet for testing purposes.
#[cfg(test)]
pub(crate) fn is_subscribed_permanent(&self, subnet: &Subnet) -> bool {
Expand Down
35 changes: 17 additions & 18 deletions beacon_node/network/src/subnet_service/tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,28 +335,27 @@ mod test {
// submit the subscriptions
subnet_service.validator_subscriptions(vec![sub1, sub2].into_iter());

// Unsubscription event should happen at slot 2 (since subnet id's are the same, unsubscription event should be at higher slot + 1)
let expected = SubnetServiceMessage::Subscribe(Subnet::Attestation(subnet_id1));
let subnet = Subnet::Attestation(subnet_id1);

if subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) {
// If we are permanently subscribed to this subnet, we won't see a subscribe message
let _ = get_events_until_num_slots(&mut subnet_service, None, 1).await;
// If permanently subscribed, no Subscribe/Unsubscribe events will be generated
if subnet_service.is_subscribed_permanent(&subnet) {
let _ = get_events_until_num_slots(&mut subnet_service, None, 3).await;
Copy link
Member

@ackintosh ackintosh Jan 2, 2026

Choose a reason for hiding this comment

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

To make the test more robust, I think we should check that the event returned from get_events_until_num_slots is empty.

} else {
let subscription = get_events_until_num_slots(&mut subnet_service, None, 1).await;
assert_eq!(subscription, [expected]);
}

// Get event for 1 more slot duration, we should get the unsubscribe event now.
let unsubscribe_event = get_events_until_num_slots(&mut subnet_service, None, 1).await;

// If the long lived and short lived subnets are different, we should get an unsubscription
// event.
let expected = SubnetServiceMessage::Unsubscribe(Subnet::Attestation(subnet_id1));
if !subnet_service.is_subscribed(&Subnet::Attestation(subnet_id1)) {
assert_eq!([expected], unsubscribe_event[..]);
// Wait for exactly 2 events (Subscribe + Unsubscribe) with a generous timeout
let expected = [
SubnetServiceMessage::Subscribe(subnet),
SubnetServiceMessage::Unsubscribe(subnet),
];
let events = get_events_until_num_slots(
&mut subnet_service,
Some(2),
(MainnetEthSpec::slots_per_epoch()) as u32,
)
.await;
assert_eq!(events, expected);
Comment on lines +344 to +355
Copy link
Member

@ackintosh ackintosh Jan 2, 2026

Choose a reason for hiding this comment

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

The original intent here (before this fix) was to check which event we get at each slot.
So I think it would be preferable to check the events slot by slot, for example like this:

let subscribe_event = get_events_until_num_slots(&mut subnet_service, Some(1), 1).await;
assert_eq!(subscribe_event, expected);

let unsubscribe_event = get_events_until_num_slots(&mut subnet_service, Some(1), 1).await;
assert_eq!(unsubscribe_event, expected);

}

// Should no longer be subscribed to any short lived subnets after unsubscription.
// Should no longer be subscribed to any short lived subnets after unsubscription.
assert_eq!(subnet_service.subscriptions().count(), 0);
}

Expand Down