Merge "Panic when the network can't get stable checkpoint cert"

consensus/pbft/pbft-core.go

@@ -1160,15 +1160,31 @@ func (instance *pbftCore) recvCheckpoint(chkpt *Checkpoint) events.Event {
 
 	instance.checkpointStore[*chkpt] = true
 
+	// Track how many different checkpoint values we have for the seqNo in question
+	diffValues := make(map[string]struct{})
+	diffValues[chkpt.Id] = struct{}{}
+
 	matching := 0
 	for testChkpt := range instance.checkpointStore {
-		if testChkpt.SequenceNumber == chkpt.SequenceNumber && testChkpt.Id == chkpt.Id {
-			matching++
+		if testChkpt.SequenceNumber == chkpt.SequenceNumber {
+			if testChkpt.Id == chkpt.Id {
+				matching++
+			} else {
+				if _, ok := diffValues[testChkpt.Id]; !ok {
+					diffValues[testChkpt.Id] = struct{}{}
+				}
+			}
 		}
 	}
 	logger.Debugf("Replica %d found %d matching checkpoints for seqNo %d, digest %s",
 		instance.id, matching, chkpt.SequenceNumber, chkpt.Id)
 
+	// If f+2 different values have been observed, we'll never be able to get a stable cert for this seqNo
+	if count := len(diffValues); count > instance.f+1 {
+		logger.Panicf("Network unable to find stable certificate for seqNo %d (%d different values observed already)",
+			chkpt.SequenceNumber, count)
+	}
+
 	if matching == instance.f+1 {
 		// We have a weak cert
 		// If we have generated a checkpoint for this seqNo, make sure we have a match

consensus/pbft/pbft-core_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"os"
 	"reflect"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
@@ -1763,7 +1764,7 @@ func TestCheckpointDiffersFromWeakCert(t *testing.T) {
 
 	badChkpt := &Checkpoint{
 		SequenceNumber: 10,
-		Id:             base64.StdEncoding.EncodeToString([]byte("WRONG")),
+		Id:             "WRONG",
 		ReplicaId:      3,
 	}
 	instance.chkpts[10] = badChkpt.Id // This is done via the exec path, shortcut it here
@@ -1772,7 +1773,7 @@ func TestCheckpointDiffersFromWeakCert(t *testing.T) {
 	for i := uint64(0); i < 2; i++ {
 		events.SendEvent(instance, &Checkpoint{
 			SequenceNumber: 10,
-			Id:             base64.StdEncoding.EncodeToString([]byte("CORRECT")),
+			Id:             "CORRECT",
 			ReplicaId:      i,
 		})
 	}
@@ -1781,3 +1782,23 @@ func TestCheckpointDiffersFromWeakCert(t *testing.T) {
 		t.Fatalf("State target should not have been updated")
 	}
 }
+
+// This test is designed to ensure the peer panics if it observes > f+1 different checkpoint values for the same seqNo
+// This indicates a network that will be unable to move its watermarks and thus progress
+func TestNoCheckpointQuorum(t *testing.T) {
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("More than f+1 different checkpoint values found, should have panicked.")
+		}
+	}()
+
+	instance := newPbftCore(3, loadConfig(), &omniProto{}, &inertTimerFactory{})
+
+	for i := uint64(0); i < 3; i++ {
+		events.SendEvent(instance, &Checkpoint{
+			SequenceNumber: 10,
+			Id:             strconv.FormatUint(i, 10),
+			ReplicaId:      i,
+		})
+	}
+}