[Dynamic Protocol State] Refactoring to support orthogonal state machine operating on sub-states #5616
Conversation
…tocol State refactoring. Fixed tests. WIP on fixing last TODOs
…s built and DB operations are handled
There was a problem hiding this comment.
The scope of the problem is getting more clear now. Unfortunately, it is bigger than I had hoped. See specifically this comment
Co-authored-by: Alexander Hentschel <[email protected]>
…causality issue on block production. Implemented an alternative approach with deferred DB operations
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/protocol-state-kvstore #5616 +/- ##
==================================================================
- Coverage 55.96% 53.58% -2.39%
==================================================================
Files 1019 1017 -2
Lines 98142 97935 -207
==================================================================
- Hits 54926 52477 -2449
- Misses 39103 41525 +2422
+ Partials 4113 3933 -180
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // ApplyServiceEventsFromValidatedSeals applies the state changes that are delivered via | ||
| // sealed service events: |
There was a problem hiding this comment.
| // ApplyServiceEventsFromValidatedSeals applies the state changes that are delivered via | |
| // sealed service events: | |
| // ApplyServiceEventsFromValidatedSeals applies state changes associated with a candidate block. | |
| // State changes are often triggered by a service event sealed by the candidate block, however they | |
| // may also be triggered by entering a particular view. | |
| // CAUTION: this method MUST be called for all candidates, even if `seals` is empty. | |
| // |
| // only exceptions should be propagated | ||
| err := stateMachine.ProcessUpdate(orderedUpdates) |
There was a problem hiding this comment.
| // only exceptions should be propagated | |
| err := stateMachine.ProcessUpdate(orderedUpdates) | |
| // CAUTION: `ProcessUpdate` must be called, even if `orderedUpdates` is empty. | |
| // only exceptions should be propagated | |
| err := stateMachine.ProcessUpdate(orderedUpdates) |
There was a problem hiding this comment.
The more warning messages I add, the more I feel we should change the interface instead 😅
| return dbUpdates | ||
| } | ||
|
|
||
| // ProcessUpdate applies the state changes that are delivered via sealed service events. |
There was a problem hiding this comment.
Change of opinion 😅
I think the patter of "this function must be called even if there is no input" is a vastly unsafe pattern, because it creates this additional edge case across multiple abstraction layers. I think we would need CAUTION statements at this layer for example. I think we can simplify the usage patterns of our API in this regards. I another words, I think we need to fix it (TODO - not in this PR).
My opinion: we solve this problem at the lowest level. We want to guarantee to state machines that their OrthogonalStoreStateMachine.ProcessUpdate(..) method is called? I would suggest that we implement this guarantee at the StateMutator level:
- The
StateMutatoradd a stateful wrapper at the lowest level, right after creating the list of state machines through their factory methods. Thereby,StateMutatorhandles the most general set of cases (call patterns as well as state machine needs).
durkmurder
requested review from
jordanschalm
and removed request for
peterargue
state/protocol/protocol_state/epochs/happy_path_statemachine.go
Outdated
Show resolved
Hide resolved
state/protocol/protocol_state/kvstore/upgrade_statemachine_test.go
Outdated
Show resolved
Hide resolved
state/protocol/protocol_state/kvstore/upgrade_statemachine_test.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexander Hentschel <[email protected]> Co-authored-by: Jordan Schalm <[email protected]>
…//github.com/onflow/flow-go into yurii/5556-epochs-hierarchical-state-machine
Co-authored-by: Alexander Hentschel <[email protected]> Co-authored-by: Jordan Schalm <[email protected]>
…//github.com/onflow/flow-go into yurii/5556-epochs-hierarchical-state-machine
|
@AlexHentschel @jordanschalm FYI, I have commented out a much needed check for integrity of the sealing segment(which I have previously added as suggest from Alex) because we cannot support it in current state, due to different protocol state IDs at root block and the very next one. |
durkmurder
merged commit 27a79f1
into
feature/protocol-state-kvstore
#5556
Context
This PR implements a huge refactoring for addressing a few outstanding design issues summarized in linked issue. The answer to it was to change the design to one that better fits out needs.
In proposed implementation the design is centered around the KV Store. KV Store is used to store all data related to the Dynamic Protocol State. Processing pipeline was restructured to use orthogonal state machines where they operate on piece of the overall state(a sub-state) and modify only it.
The Epoch state machine was remolded into a hierarchical state machine and it's treated as another orthogonal state machine which is part of the Dynamic Protocol State.
Support for version upgrades/replication has been added as part of this PR as well.
Conceptually the biggest change is that after this refactoring we are moving from the idea of previous protocol state(
ProtocolStateEntry,RichProtocolStateEntry, etc) will get replaced with KV Store and the previous protocol state becomes part of the KV Store.Snapshotto provide KV Store, this is not implemented yet. Tests will be enabled again after implementing: #5316