Panic when own checkpoint differs from weak cert

Change-Id: I4ce07658b1ee63756fdd113c1713d9cf411180ff
Signed-off-by: Kostas Christidis <kostas@christidis.io>

consensus/pbft/pbft-core.go

@@ -1170,7 +1170,14 @@ func (instance *pbftCore) recvCheckpoint(chkpt *Checkpoint) events.Event {
 		instance.id, matching, chkpt.SequenceNumber, chkpt.Id)
 
 	if matching == instance.f+1 {
-		// We do have a weak cert
+		// We have a weak cert
+		// If we have generated a checkpoint for this seqNo, make sure we have a match
+		if ownChkptID, ok := instance.chkpts[chkpt.SequenceNumber]; ok {
+			if ownChkptID != chkpt.Id {
+				logger.Panicf("Own checkpoint for seqNo %d (%s) different from weak checkpoint certificate (%s)",
+					chkpt.SequenceNumber, ownChkptID, chkpt.Id)
+			}
+		}
 		instance.witnessCheckpointWeakCert(chkpt)
 	}
 
@@ -1187,8 +1194,7 @@ 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
-	chkptID, ok := instance.chkpts[chkpt.SequenceNumber]
-	if !ok {
+	if _, ok := instance.chkpts[chkpt.SequenceNumber]; !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 {
@@ -1206,12 +1212,6 @@ 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/pbft/pbft-core_test.go

@@ -1751,42 +1751,33 @@ func TestStateNetworkMovesOnDuringSlowStateTransfer(t *testing.T) {
 	}
 }
 
-// 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{
-		invalidateStateImpl: func() { invalidated = true },
-		skipToImpl:          func(s uint64, id []byte, replicas []uint64) { skipped = true },
-	}, &inertTimerFactory{})
+// This test is designed to ensure the peer panics if the value of the weak cert is different from its own checkpoint
+func TestCheckpointDiffersFromWeakCert(t *testing.T) {
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("Weak checkpoint certificate different from own, should have panicked.")
+		}
+	}()
 
-	seqNo := uint64(10)
+	instance := newPbftCore(3, loadConfig(), &omniProto{}, &inertTimerFactory{})
 
 	badChkpt := &Checkpoint{
 		SequenceNumber: 10,
 		Id:             base64.StdEncoding.EncodeToString([]byte("WRONG")),
-		ReplicaId:      0,
+		ReplicaId:      3,
 	}
-	instance.chkpts[seqNo] = badChkpt.Id // This is done via the exec path, shortcut it here
+	instance.chkpts[10] = badChkpt.Id // This is done via the exec path, shortcut it here
 	events.SendEvent(instance, badChkpt)
 
-	for i := uint64(1); i <= 3; i++ {
+	for i := uint64(0); i < 2; 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.skipInProgress {
-		t.Fatalf("Replica should be attempting state transfer")
-	}
-
-	if !invalidated || !skipped {
-		t.Fatalf("Replica should have invalidated its state and skipped")
+	if instance.highStateTarget != nil {
+		t.Fatalf("State target should not have been updated")
 	}
 }