fixing view change #427
fixing view change #427
Conversation
Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir <[email protected]>
| return false | ||
| } | ||
| v.Logger.Debugf("Node %d got %s from %d, the in flight proposal is valid", v.SelfID, viewDataToString(vd), sender) |
There was a problem hiding this comment.
This line doesn't exist in the new version, and is pretty valuable
internal/bft/viewchanger.go
Outdated
| return true, lastDecisionMD.LatestSequence | ||
| } | ||
|
|
||
| if lastDecisionMD.LatestSequence == mySequence+1 { |
There was a problem hiding this comment.
perhaps check the complement condition and fail fast (return false) instead of indenting this whole block
internal/bft/viewchanger.go
Outdated
| Payload: vd.LastDecision.Payload, | ||
| VerificationSequence: int64(vd.LastDecision.VerificationSequence), | ||
| } | ||
| signatures := make([]types.Signature, 0) |
There was a problem hiding this comment.
var signatures []types.Signature
internal/bft/viewchanger.go
Outdated
| } | ||
| v.Logger.Debugf("Node %d checked the in flight and it was valid", v.SelfID) | ||
| // create the new view message | ||
| signedMsgs := make([]*protos.SignedViewData, 0) |
There was a problem hiding this comment.
var signedMsgs []*protos.SignedViewData
| myMsg := v.prepareViewDataMsg() // since it might have changed by now | ||
| signedMsgs = append(signedMsgs, myMsg.GetViewData()) // leader's message will always be the first | ||
| close(v.viewDataMsgs.votes) | ||
| for vote := range v.viewDataMsgs.votes { |
There was a problem hiding this comment.
I think we should never have "processed" our own view data message in the first place, but let's take care of it in a subsequent PR
internal/bft/viewchanger.go
Outdated
| @@ -714,189 +827,231 @@ func CheckInFlight(messages []*protos.ViewData, f int, quorum int, N uint64, ver | |||
| func maxLastDecisionSequence(messages []*protos.ViewData, quorum int, N uint64, verifier api.Verifier) uint64 { | |||
There was a problem hiding this comment.
these quorum, N, verifier are not used anymore. please remove them?
| @@ -714,189 +827,231 @@ func CheckInFlight(messages []*protos.ViewData, f int, quorum int, N uint64, ver | |||
| func maxLastDecisionSequence(messages []*protos.ViewData, quorum int, N uint64, verifier api.Verifier) uint64 { | |||
| max := uint64(0) | |||
| for _, vd := range messages { | |||
There was a problem hiding this comment.
I might be paranoid but I think we should add a parameter to this method that is the current sequence, and then just check if there is a sequence that is our seq + 1 and return it
There was a problem hiding this comment.
added a comment to clarify
| if vd.LastDecision.Metadata == nil { // this is a genesis proposal | ||
| if mySequence > 0 { | ||
| // can't validate the signature since I am ahead | ||
| if err := ValidateInFlight(vd.InFlightProposal, 0); err != nil { |
There was a problem hiding this comment.
if we're ahead then why don't we also check that the content of the in flight proposal is equal to our latest decision?
There was a problem hiding this comment.
We check the in flight proposals later
We don't compare it now since the in flight proposal might be different then the last decision and the view data message is still considered valid
|
|
||
| if lastDecisionMD.LatestSequence < mySequence { // this is a decision in the past | ||
| // can't validate the signature since I am ahead | ||
| if err := ValidateInFlight(vd.InFlightProposal, lastDecisionMD.LatestSequence); err != nil { |
There was a problem hiding this comment.
why don't we also check the content of the in flight?
There was a problem hiding this comment.
same as the last comment
| } | ||
| signatures = append(signatures, signature) | ||
| } | ||
| v.deliverDecision(proposal, signatures) |
There was a problem hiding this comment.
Why do we blindly deliver this without calling v.Verifier.VerifyProposal() beforehand?
There was a problem hiding this comment.
we don't...we deliver only after calling ValidateLastDecision
internal/bft/viewchanger.go
Outdated
| case change := <-v.startChangeChan: | ||
| v.startViewChange(change) | ||
| if !noInFlight { | ||
| if !v.commitInFlightProposal(inFlightProposal) { |
There was a problem hiding this comment.
this can be collapsed into a single if condition
| @@ -1187,6 +1217,69 @@ func TestGradualStart(t *testing.T) { | |||
|
|
|||
| } | |||
|
|
|||
| func TestReconfigAndViewChange(t *testing.T) { | |||
There was a problem hiding this comment.
can you please add some comments to explain what is going on in this test?
Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir <[email protected]>
* fix the ending of view change (#424) Signed-off-by: Hagar Meir <[email protected]> * sync avoid write to view change channel (#425) Signed-off-by: Hagar Meir <[email protected]> * fixing view change (#427) * fixing view change Signed-off-by: Hagar Meir <[email protected]> * test view data Signed-off-by: Hagar Meir <[email protected]> * test new view Signed-off-by: Hagar Meir <[email protected]> * after review Signed-off-by: Hagar Meir <[email protected]> * update protos Signed-off-by: Hagar Meir <[email protected]> * change comment * add return reconfig in test Signed-off-by: Hagar Meir <[email protected]>
Signed-off-by: Hagar Meir [email protected]