From 9bd2a0f8695e14a7809475d05654d62b4e37c627 Mon Sep 17 00:00:00 2001 From: Sergey Fedorov Date: Mon, 13 Jan 2020 23:41:27 +0100 Subject: [PATCH] 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 --- core/message-handling.go | 7 ++++--- core/message-handling_test.go | 8 +++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/core/message-handling.go b/core/message-handling.go index 65240131..7d031a7a 100644 --- a/core/message-handling.go +++ b/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,7 +384,7 @@ 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 @@ -392,7 +392,8 @@ func makeReplicaMessageProcessor(id uint32, process messageProcessor, processUIM 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) } } diff --git a/core/message-handling_test.go b/core/message-handling_test.go index 553c01fb..55966475 100644 --- a/core/message-handling_test.go +++ b/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()