From 530a03b0a98665f2049eb62565c3d63cb8873fa6 Mon Sep 17 00:00:00 2001 From: Hagar Meir Date: Sun, 18 Oct 2026 14:58:41 +0300 Subject: [PATCH] Read the in-flight proposal and its prepared flag together for ViewData prepareViewDataMsg read the in-flight proposal and whether it is prepared with two separate calls, each taking the InFlightData lock on its own. If a new proposal was stored between the two calls, the view data paired one proposal with the prepared flag of another. Add InFlightData.InFlightProposalAndPrepared, which returns both under one lock, and use it when building the view data. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: Hagar Meir --- internal/bft/util.go | 13 +++++++++ internal/bft/util_test.go | 57 +++++++++++++++++++++++++++++++++++++ internal/bft/viewchanger.go | 7 ++--- 3 files changed, 73 insertions(+), 4 deletions(-) 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