Fix - Future decided no instance nil check #342
Conversation
| return ssvMessage | ||
| } | ||
|
|
||
| multiSpecTest := &tests.MultiMsgProcessingSpecTest{ |
There was a problem hiding this comment.
we don't need this test for proposer?
There was a problem hiding this comment.
The proposer and blinded proposer tests are in the end of the file
ssv/runner.go
Outdated
| @@ -196,7 +196,7 @@ func (b *BaseRunner) basePartialSigMsgProcessing( | |||
| // didDecideCorrectly returns true if the expected consensus instance decided correctly | |||
| func (b *BaseRunner) didDecideCorrectly(prevDecided bool, decidedMsg *qbft.SignedMessage) (bool, error) { | |||
| decided := decidedMsg != nil | |||
There was a problem hiding this comment.
I suggest to write it differently, this is how we write stuff lie this in go and I also think is more readable and easier to reason about
if decidedMsg == nil {
return false, nil
}
if b.State.RunningInstance == nil {
return false, errors.New("decided wrong instance")
}
if decidedMsg.Message.Height != b.State.RunningInstance.GetHeight() {
return false, errors.New("decided wrong instance")
}There was a problem hiding this comment.
let me know what you guys think, I know its not a part of this PR but I couldn't help but see this
@MatheusFranco99 , @GalRogozinski , @moshe-blox . its ok also to decide not to do it here.
There was a problem hiding this comment.
I 100% agree. @GalRogozinski what do you think?
MatheusFranco99
force-pushed
the
future-decided-no-instance-panic
branch
from
491ccef to
a775ca0
Compare
| if !decidedRunningInstance { | ||
|
|
||
| if b.State.RunningInstance == nil { | ||
| return false, errors.New("decided wrong instance") |
There was a problem hiding this comment.
we should return different err message here
Fix #235 by adding a
nilcheck.New test: