aggregator committee: improve pre-consensus completeness#2691
aggregator committee: improve pre-consensus completeness#2691nkryuchkov wants to merge 12 commits intoboole-forkfrom
Conversation
Greptile SummaryUpdated Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A[Receive Pre-Consensus Message] --> B{Has Consensus<br/>Started?}
B -->|Yes| C[Return Error:<br/>Ignore Message]
B -->|No| D[Mark Signer as Seen]
D --> E[Process Message<br/>basePreConsensusMsgProcessing]
E --> F{Has New<br/>Quorum?}
F -->|No| G{Seen All<br/>Signers?}
G -->|Yes| H[Finish Duty]
G -->|No| I[Wait for More Messages]
F -->|Yes| J{Any Aggregator<br/>Selected?}
J -->|No| K{All Duties Checked<br/>OR All Signers Seen?}
K -->|Yes| L[Finish Duty]
K -->|No| M[Wait for More Messages]
J -->|Yes| N[Mark Duties as Checked]
N --> O[Fetch Aggregates/<br/>Contributions]
O --> P[Start Consensus]
Last reviewed commit: ac78bee |
# Conflicts: # go.mod # go.sum # ssvsigner/go.mod # ssvsigner/go.sum
iurii-ssv
left a comment
There was a problem hiding this comment.
LGTM, just minor suggestions
iurii-ssv
left a comment
There was a problem hiding this comment.
LGTM, with minor comment
| logger.Warn("failed to process sync committee selection proof", | ||
| zap.Error(err), | ||
| fields.Slot(vDuty.Slot), | ||
| fields.ValidatorIndex(vDuty.ValidatorIndex), | ||
| fields.Root(root)) |
There was a problem hiding this comment.
I believe we don't need to add any of: fields.Slot(vDuty.Slot), fields.ValidatorIndex(vDuty.ValidatorIndex), fields.Root(root) here:
- the
fields.Slot(vDuty.Slot)is already propagated with theloggeritself (true for every p2p-message-handling func such asProcessPreConsensus, true for every runner) - the
fields.ValidatorIndex(vDuty.ValidatorIndex),fields.Root(root)are already added to the logger in the code above
| r.state().Finished = true | ||
| r.measurements.EndDutyFlow() | ||
| recordTotalDutyDuration(ctx, r.measurements.TotalDutyTime(), spectypes.RoleAggregatorCommittee, 0) | ||
| return anyErr |
There was a problem hiding this comment.
We should also log & record all the data here too, similar to how it's done in other places with .Finished = true:
r.state().Finished = true
r.measurements.EndDutyFlow()
recordTotalDutyDuration(ctx, r.measurements.TotalDutyTime(), spectypes.RoleAggregatorCommittee, 0)
const dutyFinishedNoAggregators = "✔️successfully finished duty processing (no validator is aggregator or sync committee contributor)"
logger.Info(dutyFinishedNoAggregators,
fields.PreConsensusTime(r.measurements.PreConsensusTime()),
fields.TotalDutyTime(r.measurements.TotalDutyTime()),
)
span.AddEvent(dutyFinishedNoAggregators)
return anyErr
| logger.Info(dutyFinishedNoMessages, | ||
| fields.PreConsensusTime(r.measurements.PreConsensusTime()), | ||
| fields.TotalConsensusTime(r.measurements.TotalConsensusTime()), | ||
| fields.TotalDutyTime(r.measurements.TotalDutyTime()), | ||
| ) |
There was a problem hiding this comment.
Since we couldn't have had the consensus phase (if we ended up here), we shouldn't be logging fields.TotalConsensusTime(r.measurements.TotalConsensusTime()), at all (otherwise its value will be 0 which is confusing) ... so I would remove this field from this log-line.
| r.BaseRunner.State.Finished = true | ||
| r.measurements.EndDutyFlow() |
There was a problem hiding this comment.
We should also add these here right before the r.measurements.EndDutyFlow() call, to make sure we log the correct value (and actually record the metric) when terminating duty like that:
r.measurements.EndPreConsensus()
recordPreConsensusDuration(ctx, r.measurements.PreConsensusTime(), spectypes.RoleAggregatorCommittee)
ssvlabs/ssv-spec#604