diff --git a/internal/bft/util.go b/internal/bft/util.go index 4306b00a..5437a32b 100644 --- a/internal/bft/util.go +++ b/internal/bft/util.go @@ -215,6 +215,19 @@ func (ifp *InFlightData) IsInFlightPrepared() bool { return ifp.v.prepared } +// InFlightProposalAndPrepared returns the in-flight proposal, or nil if there is no such, +// together with whether it is prepared, so that the two are never taken from different proposals. +func (ifp *InFlightData) InFlightProposalAndPrepared() (*types.Proposal, bool) { + ifp.lock.RLock() + defer ifp.lock.RUnlock() + + if ifp.v == nil { + return nil, false + } + + return ifp.v.proposal, ifp.v.prepared +} + // StoreProposal stores an in-flight proposal. func (ifp *InFlightData) StoreProposal(prop types.Proposal) { p := prop diff --git a/internal/bft/util_test.go b/internal/bft/util_test.go index 1bb6780b..fe61d1c7 100644 --- a/internal/bft/util_test.go +++ b/internal/bft/util_test.go @@ -7,6 +7,7 @@ package bft import ( "fmt" + "sync" "testing" "github.com/hyperledger/SmartBFT/pkg/api" @@ -15,6 +16,7 @@ import ( protos "github.com/hyperledger/SmartBFT/smartbftprotos" "github.com/stretchr/testify/assert" "go.uber.org/zap" + "google.golang.org/protobuf/proto" ) func TestBlacklist(t *testing.T) { @@ -132,6 +134,61 @@ func TestInFlightProposalLatest(t *testing.T) { assert.Equal(t, prop, *ifp.InFlightProposal()) } +type noopSigner struct{} + +func (noopSigner) Sign([]byte) []byte { return nil } + +func (noopSigner) SignProposal(types.Proposal, []byte) *types.Signature { return nil } + +// TestViewDataInFlightWhileStored makes sure that the in flight proposal and whether it is prepared, +// as reported in a view data message, are taken together, even while the view stores a new in flight proposal. +// The view stores proposals with an even payload and then their prepares, and proposals with an odd payload +// without ever preparing them, so a view data message must never report an odd proposal as prepared. +func TestViewDataInFlightWhileStored(t *testing.T) { + basicLog, err := zap.NewDevelopment() + assert.NoError(t, err) + checkpoint := &types.Checkpoint{} + checkpoint.Set(types.Proposal{}, nil) // the genesis proposal, so any in flight proposal is reported + vc := &ViewChanger{ + SelfID: 1, + Logger: basicLog.Sugar(), + Signer: noopSigner{}, + Checkpoint: checkpoint, + InFlight: &InFlightData{}, + } + + done := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; ; i++ { + select { + case <-done: + return + default: + } + vc.InFlight.StoreProposal(types.Proposal{Payload: []byte{byte(i)}, Metadata: []byte{}}) + if i%2 == 0 { + vc.InFlight.StorePrepares(0, 0) + } + } + }() + + inconsistent := 0 + for range 100000 { + viewData := &protos.ViewData{} + assert.NoError(t, proto.Unmarshal(vc.prepareViewDataMsg().GetViewData().RawViewData, viewData)) + if viewData.InFlightPrepared && viewData.InFlightProposal.Payload[0]%2 == 1 { + inconsistent++ + } + } + close(done) + wg.Wait() + + assert.Zero(t, inconsistent, "view data reported an in flight proposal as prepared although it was never prepared") +} + func TestQuorum(t *testing.T) { // Ensure that quorum size is as expected. diff --git a/internal/bft/viewchanger.go b/internal/bft/viewchanger.go index 2b1e2539..58d9c9da 100644 --- a/internal/bft/viewchanger.go +++ b/internal/bft/viewchanger.go @@ -443,8 +443,8 @@ func (v *ViewChanger) processViewChangeMsg(restore bool) { func (v *ViewChanger) prepareViewDataMsg() *protos.Message { lastDecision, lastDecisionSignatures := v.Checkpoint.Get() - inFlight := v.getInFlight(lastDecision) - prepared := v.InFlight.IsInFlightPrepared() + inFlightProposal, prepared := v.InFlight.InFlightProposalAndPrepared() + inFlight := v.getInFlight(lastDecision, inFlightProposal) vd := &protos.ViewData{ NextView: v.currView, LastDecision: lastDecision, @@ -466,8 +466,7 @@ func (v *ViewChanger) prepareViewDataMsg() *protos.Message { return msg } -func (v *ViewChanger) getInFlight(lastDecision *protos.Proposal) *protos.Proposal { - inFlight := v.InFlight.InFlightProposal() +func (v *ViewChanger) getInFlight(lastDecision *protos.Proposal, inFlight *types.Proposal) *protos.Proposal { if inFlight == nil { v.Logger.Debugf("Node %d's in flight proposal is not set", v.SelfID) return nil