-
Notifications
You must be signed in to change notification settings - Fork 264
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
fix: use updateRoundStep to track consensus state changes #1464
Conversation
@@ -1518,6 +1515,7 @@ func (cs *State) enterPrecommitWait(height int64, round int32) { | |||
defer func() { | |||
// Done enterPrecommitWait: | |||
cs.TriggeredTimeoutPrecommit = true | |||
cs.updateRoundStep(round, cstypes.RoundStepPrecommitWait) | |||
cs.newStep() |
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.
We also trace round step changes in newStep(), wouldn't that cause duplicates?
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.
I misread the code, the change is fine! I think it causes duplicate entries in the traced data.
@@ -538,6 +538,7 @@ func (cs *State) updateRoundStep(round int32, step cstypes.RoundStepType) { | |||
} | |||
if cs.Step != step { | |||
cs.metrics.MarkStep(cs.Step) | |||
schema.WriteRoundState(cs.traceClient, cs.Height, round, uint8(step)) |
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.
I guess my previous question applies here: Isn't updateRoundStep
always followed by newStep
? Then we already trace changes in newStep
which means we will have duplicates?
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 is not always the case. For instance I notice the NewRound steo was never triggered and also the PrecommitWait step
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.
Oh and I removed the tracing from newStep
We're currently using
newStep
to indicate whether to write a new trace, however this actually doesn't track when the round and step change happens and thus misses certain steps (like NewRound). This PR fixes it