-
Notifications
You must be signed in to change notification settings - Fork 179
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.
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
[Dynamic Protocol State] Dedicated protocol state machine for epoch fallback #4931
[Dynamic Protocol State] Dedicated protocol state machine for epoch fallback #4931
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the logic much easier to follow - nice improvement.
My main feedback is to ensure that, for the time being, we do not allow invalid UpdateIdentity
events to trigger EFM or halt the chain.
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
…/flow-go into yurii/4649-epoch-fallback-statemachine-attempt-1
…ateMachine to return if event was applied
… fallback state machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Personally, I would still like to change the name of the flag for epoch fallback, as mentioned here. Not a blocker, though.
…/flow-go into yurii/4649-epoch-fallback-statemachine-attempt-1
…nflow/flow-go into yurii/4649-epoch-fallback-statemachine-attempt-1
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/dynamic-protocol-state #4931 +/- ##
==================================================================
+ Coverage 56.14% 56.18% +0.03%
==================================================================
Files 974 967 -7
Lines 90574 90142 -432
==================================================================
- Hits 50853 50646 -207
+ Misses 35934 35712 -222
+ Partials 3787 3784 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work. My comments are largely suggestions for goDoc revisions, extensions of tests and revisions for code clarity and modularity. All are only minor revision (I hope).
state/protocol/protocol_state/epoch_fallback_statemachine_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Context
This PR addresses high priority item from #4649 to consolidate logic of handling service events when invalid state transition has been detected.
In this PR all state machine modifications are treated as one atomic state transition. It either succeeds in its entirety or in case of a failure, there is no update to the protocol state. If invalid state transition has been detected we a special version of protocol state machine for epoch fallback mode. Whenever we have entered epoch fallback mode we will re-process all the events in that block and continue using epoch fallback state machine for subsequent blocks. EFM state machine ignores epoch events and doesn't perform transitions to next epoch but it still handles ejection events.
Most of the implementation is following Alex's comment