ssvlabs / ssv

Secret-Shared-Validator(SSV) for ethereum staking
https://ssv.network
GNU General Public License v3.0
179 stars 92 forks source link

fix: duty scheduler - indices change synchronization #1724

Open olegshmuelov opened 1 week ago

olegshmuelov commented 1 week ago

Describe the bug Indices change can cause a bug where duties are reseted while executing committee duties

To Reproduce Steps to reproduce the behavior:

  1. slot n: indices change triggered
  2. slot n+1: committee ticker event
  3. slot n+1: attester ticker event
  4. attester execute aggregator duties
  5. reset current epoch duties
  6. committee execute duties (no duties) - bug
y0sher commented 6 days ago

We pretty much know this bug from previously, well we didn't think of the bug, but we did talk about making the Committee handler in scheduler able to wait for a signal that the Duties container is ready. this can probably happen as well for reorg event.

Here's a short 2 ways of how this will be solved:

  1. Make sure committee handler is always run after attester and sync committee handlers in scheduler, if everything is run synchroniously and changes are updated on the same event. (not waiting to next epoch to get duities again but doing in inside the indiceChange event (without waiting for next slot like current code). Another issue is we're using Feed for that and feed adds every subscriber as a select case hence there's no order enforced between them.
  2. Make Duties containers LOCK whenever an event happens, and only release to readers after all updates have been done. this should work even if everything is asynchronously.
olegshmuelov commented 6 days ago

We pretty much know this bug from previously, well we didn't think of the bug, but we did talk about making the Committee handler in scheduler able to wait for a signal that the Duties container is ready. this can probably happen as well for reorg event.

Here's a short 2 ways of how this will be solved:

  1. Make sure committee handler is always run after attester and sync committee handlers in scheduler, if everything is run synchroniously and changes are updated on the same event. (not waiting to next epoch to get duities again but doing in inside the indiceChange event (without waiting for next slot like current code). Another issue is we're using Feed for that and feed adds every subscriber as a select case hence there's no order enforced between them.
  2. Make Duties containers LOCK whenever an event happens, and only release to readers after all updates have been done. this should work even if everything is asynchronously.

We prefer not to synchronize slot ticker events between the duty scheduler handlers, as it would add complexity and make the code harder to maintain. Instead, I propose triggering all the blocking functions of both the Attester and SyncCommittee handlers directly from the Committee handler, which should streamline the process without the added overhead