Merge pull request #2008 from jyellick/clear-batch-on-viewchange-master

Clear batch store on view change

consensus/obcpbft/config.yaml

@@ -45,9 +45,9 @@ general:
 
         # Send a pre-prepare if there are pending requests, batchsize isn't reached yet,
         # and this much time has elapsed since the current batch was formed
-        batch: 2s
+        batch: 1s
 
-        # How long may a request take between reception and execution
+        # How long may a request take between reception and execution, must be greater than the batch timeout
         request: 2s
 
         # How long may a view change take
@@ -56,7 +56,7 @@ general:
         # How long to wait for a view change quorum before resending (the same) view change
         resendviewchange: 2s
 
-        # Interval to send "keep-alive" null requests.  Set to 0 to disable.
+        # Interval to send "keep-alive" null requests.  Set to 0 to disable. If enabled, must be greater than request timeout
         nullrequest: 0s
 
 ################################################################################

consensus/obcpbft/obc-batch.go

@@ -101,6 +101,16 @@ func newObcBatch(id uint64, config *viper.Viper, stack consensus.Stack) *obcBatc
 	logger.Infof("PBFT Batch size = %d", op.batchSize)
 	logger.Infof("PBFT Batch timeout = %v", op.batchTimeout)
 
+	if op.batchTimeout >= op.pbft.requestTimeout {
+		op.pbft.requestTimeout = 3 * op.batchTimeout / 2
+		logger.Warningf("Configured request timeout must be greater than batch timeout, setting to %v", op.pbft.requestTimeout)
+	}
+
+	if op.pbft.requestTimeout >= op.pbft.nullRequestTimeout && op.pbft.nullRequestTimeout != 0 {
+		op.pbft.nullRequestTimeout = 3 * op.pbft.requestTimeout / 2
+		logger.Warningf("Configured null request timeout must be greater than request timeout, setting to %v", op.pbft.nullRequestTimeout)
+	}
+
 	op.incomingChan = make(chan *batchMessage)
 
 	op.batchTimer = etf.CreateTimer()
@@ -208,8 +218,7 @@ func (op *obcBatch) execute(seqNo uint64, raw []byte) {
 			continue
 		}
 
-		// TODO, this is a really and inefficient way to do this, but because reqs aren't comparable, they cannot be retrieved from the map directly
-		logger.Debugf("Batch replica %d executing request with transaction %s from outstandingReqs", op.pbft.id, tx.Uuid)
+		logger.Debugf("Batch replica %d executing request with transaction %s from outstandingReqs, seqNo=%d", op.pbft.id, tx.Uuid, seqNo)
 
 		if outstanding, pending := op.reqStore.remove(req); !outstanding || !pending {
 			logger.Debugf("Batch replica %d missing transaction %s outstanding=%v, pending=%v", op.pbft.id, tx.Uuid, outstanding, pending)
@@ -413,6 +422,7 @@ func (op *obcBatch) ProcessEvent(event events.Event) events.Event {
 		op.startTimerIfOutstandingRequests()
 		return res
 	case viewChangedEvent:
+		op.batchStore = nil
 		// Outstanding reqs doesn't make sense for batch, as all the requests in a batch may be processed
 		// in a different batch, but PBFT core can't see through the opaque structure to see this
 		// so, on view change, clear it out
@@ -505,9 +515,9 @@ func (op *obcBatch) getManager() events.Manager {
 }
 
 func (op *obcBatch) startTimerIfOutstandingRequests() {
-	if op.pbft.skipInProgress || op.pbft.currentExec != nil {
+	if op.pbft.skipInProgress || op.pbft.currentExec != nil || !op.pbft.activeView {
 		// Do not start view change timer if some background event is in progress
-		logger.Debugf("Replica %d not starting timer because skip in progress or current exec", op.pbft.id)
+		logger.Debugf("Replica %d not starting timer because skip in progress or current exec or in view change", op.pbft.id)
 		return
 	}
 

consensus/obcpbft/obc-batch_test.go

@@ -354,3 +354,18 @@ func TestClassicBackToBackStateTransfer(t *testing.T) {
 		}
 	}
 }
+
+func TestClearBatchStoreOnViewChange(t *testing.T) {
+	b := newObcBatch(1, loadConfig(), &omniProto{})
+	defer b.Close()
+
+	b.batchStore = []*Request{&Request{}}
+
+	// Send a request, which will be ignored, triggering view change
+	b.manager.Queue() <- viewChangedEvent{}
+	b.manager.Queue() <- nil
+
+	if len(b.batchStore) != 0 {
+		t.Fatalf("Should have cleared the batch store on view change")
+	}
+}