Fix subnet unsubscription time

[pawanjay176]

Issue Addressed

Hopefully fixes #6732

Proposed Changes

In our scheduled_subscriptions, we were setting unsubscription slot to be current_slot + 1. Given that we were subscribing to the subnet at duty.slot - 1, the unsubscription slot ended up being duty.slot. So we were unsubscribing to the subnet at the beginning of the duty slot which is insane.

Fixes the scheduled_subscriptions to unsubscribe at duty.slot + 1.

Additional Info

Running this on kurtosis made me realise that we do not send subscriptions for the first couple of slots because of https://github.com/sigp/lighthouse/blob/unstable/validator_client/validator_services/src/duties_service.rs#L79-L85

Not sure if we want to handle that.

[michaelsproul]

Nice find!

The fix makes sense to me.

The testnet case is interesting, but I think a fix for that can wait for another PR. I guess we might want to special-case the VC's/BN's handling of the first ~2 slots of a network (feels kind of dumb), or remove the loud warnings for late subscriptions from the BN and just track them with a metric.

[michaelsproul]

Looks like one of the tests is failing. It might actually be expecting the old behaviour 👀

[pawanjay176]

I fixed the failing test according to my understanding.
I think prior to #6146 , the test was checking if we bundled multiple unsubscriptions together into a single one (e.g. if we need to unsubscribe from subnet X at slot 9 and 12, then skip the unsubscribe at 9).

After the refactor, we decide to extend the unsubscription only in subscribe_to_subnet_immediately 1 slot before the subscription. So multiple unsubscriptions will get bundled together only in the case where there's overlap between an existing subscription and the new one (e.g. if we are subscribing at slot 11 and there's a planned unsubscription event at slot 12, then we just extend the subscription).

TL;DR: the test test_subscribe_same_subnet_several_slots_apart wasn't refactored with the logic change in #6146 , and started failing when we fixed a bug. cbf7dca refactors the test.

[jimmygchen]

Nice find! 👍

[dapplion]

You should add some comments referencing the issue and the reason for slot + 1 in the code. Useful if we ever refactor this and git blame breaks

[dapplion]

LGTM

[mergify]

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

• ☑️ test-suite-success

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

[michaelsproul]

Fix for CI failure:

• Cargo update for openssl vuln #6901

[AgeManning]

Yeah nice catch.

I wouldn't have imagined this would change the density of subscribed subnets between 5.3 and 6.0?

[jimmygchen]

@mergify requeue

[mergify]

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

[jimmygchen]

Yeah nice catch.

I wouldn't have imagined this would change the density of subscribed subnets between 5.3 and 6.0?

Looks like this was introduced in #6146 which was part of the 6.0 release (My bad, I missed this in the review!).

The old logic was good and same as the fix in this PR:
https://github.com/sigp/lighthouse/blob/08e8b92e5032044f253a7357aaf0a7c74f9f8b80/beacon_node/network/src/subnet_service/attestation_subnets.rs#L644-L647