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