core: Ignore errors when processing valid embedded messages

Correct replicas must always agree on message validity, i.e. the
result of checking for message's internal consistency and
authenticity. However, it is practically hard, if not even impossible,
to guarantee the same regarding message's correctness with respect to
other messages, i.e. guarantee accurate detection of any protocol
violation. For example, Commit messages referring to already executed
requests are simply ignored as excessive, even if they were actually
generated with protocol violation.

In current implementation, a replica does not process a received
message itself if it detects protocol violation when processing
messages embedded into the received message. That does not create any
problem, as long as replicas do not embed potentially incorrect
messages into their own generated messages. This currently holds for
normal-case protocol messages.

However, this way of treating incorrect embedded messages might
compromise liveness guarantee that is to be ensured by view change
operation. That is because a new primary needs to prove correctness of
transition into the new view by presenting a new-view certificate
which includes message logs received by the new primary from other
replicas. Due to the aforementioned inaccuracy in detection of
protocol violations, even a correct new primary might pick some
message logs containing valid but incorrect messages. Should some
other correct replicas keep rejecting such new-view certificates, view
change operation may never finish.

Nevertheless, a correct primary will only use well-formed message logs
consisting of all valid messages from a sufficient quorum of replicas.
This will be validated by other correct replicas and must be enough to
ensure safe transition into a new view, even if some messages included
into the new-view certificate are incorrect. Incorrect messages are
either ignored or rejected by correct replicas, thus they cannot cause
request execution; whereas the purpose of the new-view certificate is
to ensure all requests that were prepared and potentially accepted for
execution are propagated to the new view.

Without compromising safety, ignore errors encountered when processing
embedded messages. This will allow to use the existing code to process
messages embedded into ViewChange and NewView messages, i.e. messages
of the message log included into ViewChange message, and ViewChange
messages of the new-view certificate included into NewView message.

Signed-off-by: Sergey Fedorov <sergey.fedorov@neclab.eu>

core/message-handling.go

@@ -217,7 +217,7 @@ func defaultIncomingMessageHandler(id uint32, log messagelog.MessageLog, config
 	processRequest := makeRequestProcessor(captureSeq, pendingReq, viewState, applyRequest)
 	processViewMessage := makeViewMessageProcessor(viewState, applyReplicaMessage)
 	processUIMessage := makeUIMessageProcessor(captureUI, processViewMessage)
-	processReplicaMessage := makeReplicaMessageProcessor(id, processMessageThunk, processUIMessage)
+	processReplicaMessage := makeReplicaMessageProcessor(id, processMessageThunk, processUIMessage, logger)
 	processMessage = makeMessageProcessor(processRequest, processReplicaMessage)
 
 	replyRequest := makeRequestReplier(clientStates)
@@ -384,15 +384,16 @@ func makeMessageProcessor(processRequest requestProcessor, processReplicaMessage
 	}
 }
 
-func makeReplicaMessageProcessor(id uint32, process messageProcessor, processUIMessage uiMessageProcessor) replicaMessageProcessor {
+func makeReplicaMessageProcessor(id uint32, process messageProcessor, processUIMessage uiMessageProcessor, logger *logging.Logger) replicaMessageProcessor {
 	return func(msg messages.ReplicaMessage) (new bool, err error) {
 		if msg.ReplicaID() == id {
 			return false, nil
 		}
 
 		for _, m := range messages.EmbeddedMessages(msg) {
 			if _, err := process(m); err != nil {
-				return false, fmt.Errorf("Failed to process embedded message: %s", err)
+				logger.Warningf("Failed to process %s extracted from %s: %s",
+					messageString(m), messageString(msg), err)
 			}
 		}
 

core/message-handling_test.go

@@ -256,7 +256,7 @@ func TestMakeReplicaMessageProcessor(t *testing.T) {
 		args := mock.MethodCalled("uiMessageProcessor", msg)
 		return args.Bool(0), args.Error(1)
 	}
-	process := makeReplicaMessageProcessor(id, processMessage, processUIMessage)
+	process := makeReplicaMessageProcessor(id, processMessage, processUIMessage, logging.MustGetLogger(module))
 
 	request := messageImpl.NewRequest(0, rand.Uint64(), nil)
 
@@ -275,8 +275,9 @@ func TestMakeReplicaMessageProcessor(t *testing.T) {
 		prepare := messageImpl.NewPrepare(primary, viewForPrimary(n, primary), request)
 
 		mock.On("messageProcessor", request).Return(false, fmt.Errorf("Error")).Once()
+		mock.On("uiMessageProcessor", prepare).Return(true, nil).Once()
 		_, err = process(prepare)
-		assert.Error(t, err, "Failed to process embedded Request")
+		assert.NoError(t, err)
 
 		mock.On("messageProcessor", request).Return(true, nil).Once()
 		mock.On("uiMessageProcessor", prepare).Return(false, fmt.Errorf("Error")).Once()
@@ -307,8 +308,9 @@ func TestMakeReplicaMessageProcessor(t *testing.T) {
 		commit := messageImpl.NewCommit(randOtherReplicaID(id, n), prepare)
 
 		mock.On("messageProcessor", prepare).Return(false, fmt.Errorf("Error")).Once()
+		mock.On("uiMessageProcessor", commit).Return(true, nil).Once()
 		_, err = process(commit)
-		assert.Error(t, err, "Failed to process embedded Prepare")
+		assert.NoError(t, err)
 
 		mock.On("messageProcessor", prepare).Return(true, nil).Once()
 		mock.On("uiMessageProcessor", commit).Return(false, fmt.Errorf("Error")).Once()