Merge pull request #1973 from jyellick/fix-checkpoint-garbage-collect…

…ion-master

Invalidate state on checkpoint quorum peer disagrees with

consensus/obcpbft/pbft-core.go

@@ -1222,7 +1222,8 @@ func (instance *pbftCore) recvCheckpoint(chkpt *Checkpoint) events.Event {
 	// we have reached this checkpoint
 	// Note, this is not divergent from the paper, as the paper requires that
 	// the quorum certificate must contain 2f+1 messages, including its own
-	if _, ok := instance.chkpts[chkpt.SequenceNumber]; !ok {
+	chkptID, ok := instance.chkpts[chkpt.SequenceNumber]
+	if !ok {
 		logger.Debugf("Replica %d found checkpoint quorum for seqNo %d, digest %s, but it has not reached this checkpoint itself yet",
 			instance.id, chkpt.SequenceNumber, chkpt.Id)
 		if instance.skipInProgress {
@@ -1240,6 +1241,11 @@ func (instance *pbftCore) recvCheckpoint(chkpt *Checkpoint) events.Event {
 	logger.Debugf("Replica %d found checkpoint quorum for seqNo %d, digest %s",
 		instance.id, chkpt.SequenceNumber, chkpt.Id)
 
+	if chkptID != chkpt.Id {
+		logger.Criticalf("Replica %d generated a checkpoint of %s, but a quorum of the network agrees on %s.  This is almost definitely non-deterministic chaincode.", instance.id, chkptID, chkpt.Id)
+		instance.stateTransfer(nil)
+	}
+
 	instance.moveWatermarks(chkpt.SequenceNumber)
 
 	return instance.processNewView()

consensus/obcpbft/pbft-core_test.go

@@ -1770,31 +1770,45 @@ func TestStateNetworkMovesOnDuringSlowStateTransfer(t *testing.T) {
 	}
 }
 
-func TestViewChangeResend(t *testing.T) {
-	viewChangeSent := false
+// This test is designed to ensure state transfer occurs if our checkpoint does not match a quorum cert
+func TestCheckpointDiffersFromQuorum(t *testing.T) {
+	invalidated := false
+	skipped := false
 	instance := newPbftCore(3, loadConfig(), &omniProto{
-		broadcastImpl: func(b []byte) { viewChangeSent = true },
-		signImpl:      func(b []byte) ([]byte, error) { return b, nil },
-		verifyImpl:    func(senderID uint64, signature []byte, message []byte) error { return nil },
+		//broadcastImpl:       func(b []byte) { viewChangeSent = true },
+		//signImpl:            func(b []byte) ([]byte, error) { return b, nil },
+		//verifyImpl:          func(senderID uint64, signature []byte, message []byte) error { return nil },
+		invalidateStateImpl: func() { invalidated = true },
+		skipToImpl:          func(s uint64, id []byte, replicas []uint64) { skipped = true },
 	}, &inertTimerFactory{})
-	instance.activeView = true
 
-	events.SendEvent(instance, viewChangeResendTimerEvent{})
+	seqNo := uint64(10)
 
-	if !instance.activeView {
-		t.Fatalf("Should not have resent view change resent timer event while in an active view")
+	badChkpt := &Checkpoint{
+		SequenceNumber: 10,
+		Id:             base64.StdEncoding.EncodeToString([]byte("WRONG")),
+		ReplicaId:      0,
 	}
+	instance.chkpts[seqNo] = badChkpt.Id // This is done via the exec path, shortcut it here
+	events.SendEvent(instance, badChkpt)
 
-	oldView := uint64(2)
-	instance.activeView = false
-	instance.view = oldView
-	events.SendEvent(instance, viewChangeResendTimerEvent{})
+	for i := uint64(1); i <= 3; i++ {
+		events.SendEvent(instance, &Checkpoint{
+			SequenceNumber: 10,
+			Id:             base64.StdEncoding.EncodeToString([]byte("CORRECT")),
+			ReplicaId:      i,
+		})
+	}
+
+	if instance.h != 10 {
+		t.Fatalf("Replica should have moved its watermarks but did not")
+	}
 
-	if instance.activeView {
-		t.Fatalf("Should still be inactive in our view")
+	if !instance.skipInProgress {
+		t.Fatalf("Replica should be attempting state transfer")
 	}
 
-	if instance.view != oldView {
-		t.Fatalf("Should still be waiting for the same view (%d) got %d", oldView, instance.view)
+	if !invalidated || !skipped {
+		t.Fatalf("Replica should have invalidated its state and skipped")
 	}
 }