Introduce prepare timer

[nhoriguchi]

This pull request suggests to introduce prepare timer based on the discussion #5. The outline is described mainly in the 1st patch. But I admit that I'm still on the way and there're some points to discuss/solve:

• Using generatedMessageConsumer to forward REQUEST messages: I just used existing code for the current purpose, but it seems not comply with the current abstraction rule. I guess that we need have prepareTimeoutHandler, but that's not done in this version.
• Adding some "implement" method on type messages.Request: this is to make the type implement interface messages.ReplicaMessage. I feel that this change has some impact and might need more adjustment on messages/messages.go.
• Fault simulator: I do some testing by modifying the sample application to do some faulty behavior, I think this might be helpful to merge it into mainline code as "faulty mode", but maybe that need more consideration on design.
• To refactor duplicated code between request timer and prepare timer: I copied most of prepare timer from request timer, so there's lines of duplicate code, I tried to define common type/interface and defining two timers in hierarchal manner, but I still don't make it.

I'm glad if I get some feedbacks.

CircleCI status is here.

[sergefdrv]

Thanks for taking the initiative to solve the issue. This doesn't look an easy one. Perhaps the most difficult part is the actual mechanism to relay REQUEST message. Therefore, I would suggest to split the work. Maybe first focus on handling the timeout itself, and just report the timeout event in the diagnostic message log. Then we could tackle message relaying as a separate issue.

I was not sure about changing the request timeout semantics. Originally, it was the maximum expected time for a received request to become ready for execution. This PR delays starting the request timer until the PREPARE message is received. However, when a correct replica has accepted a PREPARE message, it will effectively multicast it to other replicas in COMMIT message. This ensures eventual request execution unless view change is triggered. In other words, once a single correct replica has accepted a PREPARE message, reaching the consensus does not depend on the primary anymore. Thus, the request timer started at that point would not really serve for faulty primary detection.

I suggest we start both prepare and request timer upon reception of a new REQUEST message. If the replica does not accept the corresponding PREPARE message for longer than the prepare timeout, it would first assume the client was faulty. Then it will relay the REQUEST message to the primary and restart the request timer. I am currently discussing this scheme with experts, but I am not sure how soon I can get their feedback.

• Using generatedMessageConsumer to forward REQUEST messages: I just used existing code for the current purpose, but it seems not comply with the current abstraction rule. I guess that we need have prepareTimeoutHandler, but that's not done in this version.

• Adding some "implement" method on type messages.Request: this is to make the type implement interface messages.ReplicaMessage. I feel that this change has some impact and might need more adjustment on messages/messages.go.

Those points need more consideration and there is clearly something to be changed. It would not be appropriate to treat REQUEST as replica message. When a prepare timeout expires, the REQUEST message should only be forwarded to the primary, there is no need to multicast it to all replicas. Maybe we should open a dedicated message stream to the primary for REQUEST message forwarding.

Another consideration is cancelling of the transmission. If a new REQUEST is accepted from the same client, forwarding of the old REQUEST should be cancelled. At least, there should be a clear limit for REQUEST messages being forwarded, they should not pile up indefinitely.

• Fault simulator: I do some testing by modifying the sample application to do some faulty behavior, I think this might be helpful to merge it into mainline code as "faulty mode", but maybe that need more consideration on design.

As opposed to incorporating faulty behavior into normal code, we can create some kind of tool for testing. The tool would act as a client or replica following some predefined scenario, including Byzantine behavior. What do you think?

I think we could continue reviewing and merge the commit 2006f59 in a separate PR.

[nhoriguchi]

... Therefore, I would suggest to split the work. Maybe first focus on handling the timeout itself, and just report the timeout event in the diagnostic message log. Then we could tackle message relaying as a separate issue.

OK.

I was not sure about changing the request timeout semantics. Originally, it was the maximum expected time for a received request to become ready for execution. This PR delays starting the request timer until the PREPARE message is received. However, when a correct replica has accepted a PREPARE message, it will effectively multicast it to other replicas in COMMIT message. This ensures eventual request execution unless view change is triggered. In other words, once a single correct replica has accepted a PREPARE message, reaching the consensus does not depend on the primary anymore. Thus, the request timer started at that point would not really serve for faulty primary detection.

I suggest we start both prepare and request timer upon reception of a new REQUEST message. If the replica does not accept the corresponding PREPARE message for longer than the prepare timeout, it would first assume the client was faulty. Then it will relay the REQUEST message to the primary and restart the request timer. I am currently discussing this scheme with experts, but I am not sure how soon I can get their feedback.

Thanks for detailed explanation, this note is very important for me. I'll change request timer behavior as you stated above.

...

Another consideration is cancelling of the transmission. If a new REQUEST is accepted from the same client, forwarding of the old REQUEST should be cancelled. At least, there should be a clear limit for REQUEST messages being forwarded, they should not pile up indefinitely.

In the current version, a backup replica "forwards" (i.e. multicasts) a REQUEST message only once when the prepare timer expires, so maybe no worry about request's pile-up. And if a replica receives the old REQUEST message after the new REQUEST messages is accepted, the old one should be properly rejected by CaptureRequestSeq mechanism.

• Fault simulator: I do some testing by modifying the sample application to do some faulty behavior, I think this might be helpful to merge it into mainline code as "faulty mode", but maybe that need more consideration on design.

As opposed to incorporating faulty behavior into normal code, we can create some kind of tool for testing. The tool would act as a client or replica following some predefined scenario, including Byzantine behavior. What do you think?

Any faulty behaviors on client side can be mimicked by testing tools. As for replica side, some external attacks (like replay attack) can be simulated by tools, but we might need some internal module for testing inside attacks. The internal module should be switched on or off on compile time to ensure that it's never used on production setting.

I think we could continue reviewing and merge the commit 2006f59 in a separate PR.

OK, I'll separate the suggestion for this commit too.

[sergefdrv]

In the current version, a backup replica "forwards" (i.e. multicasts) a REQUEST message only once when the prepare timer expires, so maybe no worry about request's pile-up.

If I understood correctly, the prepare timer is started for each new REQUEST message. When it expires, the message is appended to the message log. My concern was that there is no limit for the number of REQUEST messages from the same client that can be appended to the message log.

And if a replica receives the old REQUEST message after the new REQUEST messages is accepted, the old one should be properly rejected by CaptureRequestSeq mechanism.

That is correct, I was just thinking of wasted network bandwidth.

[...] we might need some internal module for testing inside attacks. The internal module should be switched on or off on compile time to ensure that it's never used on production setting.

I was not quite clear about what you mean by "inside attacks". Do you mean simulation of intrusion attacks on replica's untrusted code? Such compromised replica can be simply considered faulty. And the fraction of faulty replicas is assumed limited. What is your perspective on this?

[sergefdrv]

The internal module should be switched on or off on compile time to ensure that it's never used on production setting.

My biggest concern with this approach is code clarity. I'm afraid embedding faulty logic for testing can make the code hard to follow.

[nhoriguchi]

[...] we might need some internal module for testing inside attacks. The internal module should be switched on or off on compile time to ensure that it's never used on production setting.

I was not quite clear about what you mean by "inside attacks". Do you mean simulation of intrusion attacks on replica's untrusted code?

Yes, I have that in mind.

Such compromised replica can be simply considered faulty. And the fraction of faulty replicas is assumed limited. What is your perspective on this?

What I thought was that MinBFT code is public, so attackers (note that some legitimate member could be an attacker on trustless network) can learn it and easily embed any compromised behavior (which might be hard for correct replicas to find). So I thought that we will be better to provide working demonstration code of typical scenarios with byzantine faulty replicas for anyone to verify.

The internal module should be switched on or off on compile time to ensure that it's never used on production setting.

My biggest concern with this approach is code clarity. I'm afraid embedding faulty logic for testing can make the code hard to follow.

Yes, I understand that, so I'm ok to start with separate cod/branch.

Anyway, thank you for the idea.

[sergefdrv]

What I thought was that MinBFT code is public, so attackers (note that some legitimate member could be an attacker on trustless network) can learn it and easily embed any compromised behavior (which might be hard for correct replicas to find). So I thought that we will be better to provide working demonstration code of typical scenarios with byzantine faulty replicas for anyone to verify.

So you consider simulating a tolerated fraction of Byzantine replicas? If so, I thought we might better have some dummy replica nodes that behave Byzantine, following some predefined scenario.

What you were probably concerned with was that the replica behavior we would like to test might be quite complicated. In that case, we might need some smarter tool, which behaves almost the same as correct replica.

I think we can start exploring this issue with a scenario-based tool and see how it goes.

I can think of two different approaches with the tool: (1) the tool mimics an individual replica's behavior, and (2) the tool mimics the behavior of all replicas in the network, except the tested one. In (1), it would be easier to define the Byzantine scenario for the simulated faulty replica. In (2), it would be easier to define the scenario, taking asynchronous message exchange into account. Maybe there are more possibilities, or it might make sense to use a hybrid approach.

[nhoriguchi]

So you consider simulating a tolerated fraction of Byzantine replicas?

Yes, in most severe cases, f replicas are in conspiracy to disturb consensus process.

If so, I thought we might better have some dummy replica nodes that behave Byzantine, following some predefined scenario.

What you were probably concerned with was that the replica behavior we would like to test might be quite complicated. In that case, we might need some smarter tool, which behaves almost the same as correct replica.

I think we can start exploring this issue with a scenario-based tool and see how it goes.

I can think of two different approaches with the tool: (1) the tool mimics an individual replica's behavior, and (2) the tool mimics the behavior of all replicas in the network, except the tested one. In (1), it would be easier to define the Byzantine scenario for the simulated faulty replica. In (2), it would be easier to define the scenario, taking asynchronous message exchange into account. Maybe there are more possibilities, or it might make sense to use a hybrid approach.

The hybrid approach sounds nice to me, because these approaches complement each other: (1) is for integration testing which should define expected behaviors as a system for given scenarios, so that's close to what I imaged, but (2) is also important because it tests whether a correct replica behave as expected for given scenarios.

[ynamiki]

As Sergey has wrote, I think it is better to have timerState struct. Sorry for my late response.

[nhoriguchi]

Summary of changes, the latest version:

• introduces timerState struct,
• proposes only prepare timer implementation, and message forwarding is not included (Relay request messages to primary replica #140), and
• postpones to make timeout parameter mandatory (Mandatory parameters for timeout values #139)

[nhoriguchi]

I updated the branch again. In this version I tried two things:

• timeout duration is passed to client-state provider as a mandatory parameter, and
• more code generalization (so that there's no longer requestTimerState)

[sergefdrv]

Thanks for your effort, Naoya, but I think the code needs some more adjustment.

[sergefdrv]

Maybe also extend core/internal/clientstate/timeout_test.go so that it tests prepare timer methods, or just test timerState and assume its methods are invoked correctly from the methods of clientState?

[nhoriguchi]

Maybe also extend core/internal/clientstate/timeout_test.go so that it tests prepare timer methods, or just test timerState and assume its methods are invoked correctly from the methods of clientState?

Pushed one additional commit, thank you.

[sergefdrv]

@Naoya-Horiguchi Looks good. Thanks!