runner: clarify queue state management (with respect to slot) - #2778
runner: clarify queue state management (with respect to slot)#2778iurii-ssv wants to merge 15 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
iurii-ssv
changed the title
Greptile SummaryThis PR fixes message prioritization in Validator and Committee queues by ensuring Key changes:
Confidence Score: 4/5Safe to merge; the slot fix is targeted and correct, with one minor nil-safety concern in HasRunningQBFTInstance. The queue-state slot fix is correct and well-scoped. The mutex removal is justified by the single-goroutine queue consumer model. The minor concern is HasRunningQBFTInstance() directly accessing RunningInstance.State.Decided without guarding against nil, unlike the old IsDecided() method. This relies on a documented by-construction invariant and is unlikely to cause issues in practice. protocol/v2/ssv/runner/runner.go (HasRunningQBFTInstance nil-safety) Important Files Changed
Sequence DiagramsequenceDiagram
participant Timer as Round Timer Goroutine
participant Q as queue.Queue
participant Consumer as Queue Consumer Goroutine
participant R as Runner (Committee / Validator)
participant QBFT as QBFTController
Note over Consumer: Init: rState.Slot = r.GetCurrentDutySlot()
loop Each message
Consumer->>R: HasRunningQBFTInstance()
R-->>Consumer: bool
Consumer->>R: GetLastHeight() / GetLastRound()
R-->>Consumer: height, round
Consumer->>R: GetCurrentDutySlot() [validator: each iteration]
R-->>Consumer: slot
Consumer->>Q: Pop(ctx, prioritizer(rState), filter)
Q-->>Consumer: SSVMessage
Consumer->>R: ProcessMessage(msg)
R-->>Consumer: error / nil
end
Timer->>Q: TryPush(timeout event)
Note over Consumer: Dequeues timeout event
Consumer->>R: OnTimeoutQBFT(timeoutData)
Note over R: Guard 1: hasDutyRunning() check
Note over R: Guard 2: timeoutData.Height == GetCurrentDutySlot()
R->>QBFT: OnTimeout(ctx, timeoutData)
Reviews (1): Last reviewed commit: "runner: clarify queue state management (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (b *BaseRunner) HasRunningQBFTInstance() bool { | ||
| var runningInstance *instance.Instance | ||
| if b.HasRunningDuty() { | ||
| runningInstance = b.State.RunningInstance | ||
| if runningInstance != nil { | ||
| decided, _ := runningInstance.IsDecided() | ||
| return !decided | ||
| } | ||
| } | ||
| return false | ||
| // Note: RunningInstance.State cannot be nil for existing RunningInstance by construction. | ||
| return b.hasDutyRunning() && b.HasStartedQBFTInstance() && !b.State.RunningInstance.State.Decided |
There was a problem hiding this comment.
RunningInstance.State no longer guarded
The previous implementation delegated to runningInstance.IsDecided(), which guards against a nil inner State:
func (i *Instance) IsDecided() (bool, []byte) {
if state := i.State; state != nil {
return state.Decided, state.DecidedValue
}
return false, nil
}The new code accesses b.State.RunningInstance.State.Decided directly. If RunningInstance.State is ever nil (e.g., after deserializing a malformed or legacy snapshot), this will panic instead of returning false gracefully. The existing comment documents the by-construction invariant, but it may be worth keeping the nil guard defensively:
| func (b *BaseRunner) HasRunningQBFTInstance() bool { | |
| var runningInstance *instance.Instance | |
| if b.HasRunningDuty() { | |
| runningInstance = b.State.RunningInstance | |
| if runningInstance != nil { | |
| decided, _ := runningInstance.IsDecided() | |
| return !decided | |
| } | |
| } | |
| return false | |
| // Note: RunningInstance.State cannot be nil for existing RunningInstance by construction. | |
| return b.hasDutyRunning() && b.HasStartedQBFTInstance() && !b.State.RunningInstance.State.Decided | |
| func (b *BaseRunner) HasRunningQBFTInstance() bool { | |
| // Note: RunningInstance.State cannot be nil for existing RunningInstance by construction. | |
| return b.hasDutyRunning() && b.HasStartedQBFTInstance() && | |
| b.State.RunningInstance.State != nil && !b.State.RunningInstance.State.Decided | |
| } |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 30 days if there are no updates. Please comment if you would like to keep it open. |
1 participant
Based on #2777 (the actual change is in the latest commit).
While working on #2777 it became apparent that Validator/Committee queues (the Queues that hold all duty-related messages, Runners process those messages sequentially) are not using
rState.Slotto prioritize messages (specifically this affects partial-sig messages, when older "irrelevant" message can unnecessarily delay the processing of an up-to-date "relevant" one).This PR should fix that.
Related:
F-ssv-186F-ssv-054https://github.com/ssvlabs/ssv-node-board/issues/764