AlexHentschel
commented
1 month ago Reminder to myself:
I think it would improve clarity if we moved the logic for ejection into a dedicated subcomponent. Initializing the lookups is done in several places without any need on the hot path and tracking that we really update it everywhere correctly is highly non-trivial.
Sketch of suggested approach:
introduce the auxiliary component ejector :
type trackedDynamicIdentityList struct {
dynamicIdentites flow.DynamicIdentityEntryList
identityLookup map[flow.Identifier]*flow.DynamicIdentityEntry // dynamicIdentites.Lookup() LAZY initialized
}
type ejector struct {
identityLists []trackedDynamicIdentityList
ejected []flow.Identifier
}
func (e *ejector) Eject(nodeID flow.Identifier) {
l := len(e.identityLists)
if len(e.ejected) == 0 { // if this is the first ejection sealed in this block, we have to populate the lookup first
for i:= 0; i < l; i++ {
e.identityLists[i].identityLookup = e.identityLists[i].dynamicIdentites.Lookup()
}
}
for i:= 0; i < l; i++ {
dynamicIdentity, found := e.identityLists[i].identityLookup[nodeID]
if found {
dynamicIdentity.Ejected = true
}
}
}
func (e *ejector) TrackDynamicIdentityList(list flow.DynamicIdentityEntryList) error {
tracker := trackedDynamicIdentityList{dynamicIdentites: list}
if len(e.ejected) > 0 {
// nodes were already ejected in this block, so their ejection should not be reverted by in the new `list`
tracker.identityLookup = list.Lookup()
for _, id := range e.ejected {
dynamicIdentity, found := tracker.identityLookup[id]
if found && !dynamicIdentity.Ejected {
return sentinelErrorf("node %v was previously ejected but next DynamicIdentityList reverts their ejection status", id)
}
}
}
e.identityLists = append(e.identityLists, tracker)
return nil
}thereby we can remove the lookups from the baseStateMachine, because the ejector now takes care of it
The ejector is more efficient (will never populate any lookups without ejection) and the ejector is much stronger encapsulated (none of the state machines need to worry about ejection and lookup).
Problem Definition
We have different state machines for orchestrating Epoch transitions in the package
state/protocol/protocol_state/epochs. The current implementation use theparentStatein their business logic in numerous places.In my opinion, this is extremely error prone. To the best of my understanding, the current code has at least one security-critical bug related to this here. Specifically,
buildNextEpochActiveParticipantshas a check to enforce that already ejected nodes are not re-admitted. For the current logic, two service eventseject node Xfollowed by aEpoch Setupin the same block could be a blind spot for this check. Because we always compare to the parent state (where ejection has not jet been applied). The current code would accept anEpoch Setup, where nodeXis still listed as a participant and override the prior ejection.In summary, when reviewing the state machines in the package
epochsthere are various places where I could not convince myself of the correctness of the code, because the parent state was just used without clear documentation why this is safe.Context
I think this design flaw is in part originating from our separation of
ProtocolStateEntryandRichProtocolStateEntry:RichProtocolStateEntry, the evolved protocol state is only maintained asProtocolStateEntry.CurrentEpochIdentityTableandNextEpochIdentityTablethroughout the epoch state evolution. As those fields are part of theRichProtocolStateEntry, we only work with theProtocolStateEntrywhen evolving the state.EpochSetupandEpochCommitevents. Whenever we need those, we need to go back to the parent state. And this is where the confusing mix of using the evolving state as well as the unevolved parents originates in my opinion.RichProtocolStateEntry, because they want the service events though don't care about the Identity tables. Thereby we have further instances, where we might erroneously use theparentStateinstead of the updatedstateGoal
The goal of this issue is three-fold:
parentStateas possible. In my opinion, the only place should be the state machine constructor, where we copy the parent.parentState, we must include a detailed comment explaining why this is safe even in the case of prior ejection events sealed in the current block.HappyPathStateMachineas well as theFallbackStateMachinethat confirm the correct behaviour in the following case:eject node Xfollowed by aEpoch SetupEpoch Setuplists nodeXparticipant, which would illegally override the prior ejection. As the smart contract emitted the ejection event before, it should account for this and not includeXin the setup.suggestion
I would suggest to add an interim struct between
ProtocolStateEntryandRichProtocolStateEntry. In more detail:ProtocolStateEntrytoEpochMinStateEntryadd struct
EpochStateEntry:rename
RichProtocolStateEntrytoEpochRichStateEntryand base it onEpochStateEntryEpochMinStateEntryandEpochRichStateEntry(as before)EpochMinStateEntryplus the actual service events would useEpochStateEntry. In particular, this would be the state machines. Thereby, we can cleanly avoid any usage of theparentStatein the logic that processes service events.