Commit phase - AKKA Model by shargon · Pull Request #422 · neo-project/neo

shargon · 2018-10-23T11:56:43Z

Is the same implementation as #320

TODO:

Ported to AKKA model
We need to lock the commit phase
Tested in private environment
Define if we must send the signature in commit message or not
We need to recover the consensus state when a CN is rebooted

Fixes #193
Fixes neo-project/neo-node#219

shargon · 2018-10-23T12:29:24Z

At this moment, the unique different line is this, the lock:

https://github.com/neo-project/neo/pull/422/files#diff-0285c1c12d1d492897a99ffe07f9fed9R77

I am working on recover the consensus state

vncoelho

@shargon brodas,

I saw that change view is now being blocked with if (context.State.HasFlag(ConsensusState.CommitSent)) return at CheckExpectedView method.

If the comments I mentioned are applied, then, we would send a partial signature and we would need Two Counters now (context.SignaturesPartial and context.Signatures).

After veryfing that context.SignaturesPartial is fulfilled at line

neo/neo/Consensus/ConsensusService.cs

Line 87 in 064ebc9

if (!context.State.HasFlag(ConsensusState.CommitSent) &&

, we should block change view (as you implemented).

Then, the CN would enter in a loop waiting for the real signatures context.Signatures.
For me, the point is: The same problem of the "fork" can happen here, some CN entering in the BlockChangeView (commit phase) and others not.
However, there should be a mechnism in which Good Nodes would be able to also enter commit phase, for this purpose, the nodes in the Commit Phase (Blocking the view) should Re-Send their Partial Signatures in order that good nodes (that possible entered in a loop due to connections looses) will occasionally join the commit phase waiting for the Real Signatures.

vncoelho · 2018-10-23T12:37:33Z

neo/Consensus/ConsensusService.cs


        private void CheckSignatures()
        {
+            if (!context.State.HasFlag(ConsensusState.CommitSent) &&


@shargon,
I believe that here we should check the partial signatures instead if the complete and, then, at lines 353 (

neo/neo/Consensus/ConsensusService.cs

Line 353 in 064ebc9

context.Signatures[context.MyIndex] = context.MakeHeader().Sign(context.KeyPair);

) and 52 (

neo/neo/Consensus/ConsensusService.cs

Line 52 in 064ebc9

context.Signatures[context.MyIndex] = context.MakeHeader().Sign(context.KeyPair);

) they should not be send.

shargon · 2018-10-23T12:52:22Z

I think that the priority should be fix the network errors. Obiously we need to find the right way to fix the malicious CN who can produce a fork, but i think ... if we send the complete signatures on the commit, we are doing the same as without this patch, i need to think about it 🤔

vncoelho · 2018-10-23T13:02:15Z

I also agree that the priority is to fix the network errors.
But if yes, we should remove the block of the Change View after Commit phase, this block only makes sense if we implement the partial signature (which is a feature that additionally tries to tackle the Malicious Nodes behavior)

belane · 2018-10-23T13:11:36Z

good job @shargon, it's working on private net.

==> node1/neo-cli/Logs/2018-10-23.log <==
[12:54:19.743] initialize: height=27 view=0 index=2 role=Backup
[12:54:34.755] OnPrepareRequestReceived: height=27 view=0 index=3 tx=1
[12:54:34.756] send prepare response
[12:54:36.606] OnPrepareResponseReceived: height=27 view=0 index=0
[12:54:36.607] Commit sent: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 state=Backup, RequestReceived, SignatureSent, CommitSent
[12:54:36.610] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=3
[12:54:36.612] OnPrepareResponseReceived: height=27 view=0 index=1
[12:54:36.614] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=1
[12:54:36.618] relay block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:36.618] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=0
[12:54:36.620] OnPrepareResponseReceived: height=27 view=0 index=1
[12:54:36.624] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=3
[12:54:36.626] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=1
[12:54:36.630] persist block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:36.631] initialize: height=28 view=0 index=2 role=Backup

==> node2/neo-cli/Logs/2018-10-23.log <==
[12:54:19.750] initialize: height=27 view=0 index=0 role=Backup
[12:54:34.755] OnPrepareRequestReceived: height=27 view=0 index=3 tx=1
[12:54:34.755] send prepare response
[12:54:34.759] OnPrepareResponseReceived: height=27 view=0 index=2
[12:54:34.761] Commit sent: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 state=Backup, RequestReceived, SignatureSent, CommitSent
[12:54:34.763] OnPrepareResponseReceived: height=27 view=0 index=1
[12:54:34.770] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=3
[12:54:34.779] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=1
[12:54:34.782] relay block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.787] persist block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.789] initialize: height=28 view=0 index=0 role=Primary

==> node3/neo-cli/Logs/2018-10-23.log <==
[12:54:19.748] initialize: height=27 view=0 index=3 role=Primary
[12:54:34.751] timeout: height=27 view=0 state=Primary
[12:54:34.751] send prepare request: height=27 view=0
[12:54:34.759] OnPrepareResponseReceived: height=27 view=0 index=0
[12:54:34.764] OnPrepareResponseReceived: height=27 view=0 index=1
[12:54:34.767] Commit sent: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 state=Primary, RequestSent, CommitSent
[12:54:34.768] OnPrepareResponseReceived: height=27 view=0 index=2
[12:54:34.770] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=0
[12:54:34.787] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=1
[12:54:34.791] relay block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.796] persist block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.797] initialize: height=28 view=0 index=3 role=Backup

==> node4/neo-cli/Logs/2018-10-23.log <==
[12:54:19.755] initialize: height=27 view=0 index=1 role=Backup
[12:54:34.755] OnPrepareRequestReceived: height=27 view=0 index=3 tx=1
[12:54:34.756] send prepare response
[12:54:34.775] OnPrepareResponseReceived: height=27 view=0 index=2
[12:54:34.776] Commit sent: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 state=Backup, RequestReceived, SignatureSent, CommitSent
[12:54:34.777] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=3
[12:54:34.780] OnPrepareResponseReceived: height=27 view=0 index=0
[12:54:34.789] OnPrepareResponseReceived: height=27 view=0 index=0
[12:54:34.790] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=0
[12:54:34.794] relay block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.794] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=0
[12:54:34.802] persist block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.803] initialize: height=28 view=0 index=1 role=Backup

Now, we have to decide if is better to lock change view on commit phase.
There are two possible attacks if someone control two CN (and has the ability to arbitrary distribute consensus messages to each CN).

Without lock can produce a fork. Stage 3 of dBFT (Commit) #320 (comment)
With lock can stop block generation. Stage 3 of dBFT (Commit) #320 (comment)

Also, as @vncoelho said, partial signature is important here.

vncoelho · 2018-10-23T13:22:48Z

Hey, @belane,
Thanks for mentioning and congrats to You and Shargonn.

Jaime, I prefer 2. instead of 1.
In particular, because that "stop block generation" is natural of the dBFT if f nodes are MN. Otherwise, the stop would just be a delay until everyone enter commit phase (this could be reinforced by a re-sending mechanism:

which should make changeview of some nodes return in some cases (decrease until the point where agreement was previously reached).
I mean, in some cases, nodes in a higher changeview would return to the view in which a node is locked because the node would see that an agreement was already obtained, thus could be verified when the node in a Loop on the Commit resent his state with more/equal than M valid context.SignaturesPartial)

aheuaheuahuea

Let's evaluate and think about this with careful, maybe there also can be a problem...Crazy and Smart minds everywhere, we never know where they gonna find a possible entrance 📦

vncoelho · 2018-10-23T13:33:18Z

But another discussion is: merge the critical part and later think about this or try to add this additional important change now?
I believe that if we do not do it now we could all be lazy and delay this notorious innovative and important feature for the Neo Blockchain.

shargon · 2018-10-24T10:10:26Z

In 5f3e99a and f0c2ada i locked the view change on the timeout, and for every timeout, the CN resend his signature.

What do you think?

vncoelho

@shargon, fast as a bolt!

The first commit looks precise to me.
The second one (f0c2ada) I do not know, because I thought that the CheckExpectedView was correctly implemented for blocking the ChangeView of those nodes with commit already sent. I think that you do not need to re-send here, just keep that same return for blocking changeview.

Now, the challenges are:

Create a mechanism for accepting the incoming re-send (at OnPrepareResponse). Perhaps, including an exception here:

neo/neo/Consensus/ConsensusService.cs

Line 248 in f0c2ada

if (message.ViewNumber != context.ViewNumber && message.Type != ConsensusMessageType.ChangeView)
Change the complete signature at line

neo/neo/Consensus/ConsensusService.cs

Line 52 in f0c2ada

context.Signatures[context.MyIndex] = context.MakeHeader().Sign(context.KeyPair);

. Maybe we do not need to change the one of the Primary. Perhaps, for the Primary we could send both together:

context.Signatures[context.MyIndex] = context.MakeHeader().Sign(context.KeyPair);
context.SignaturesPartial[context.MyIndex] = context.SOMETHINGTOTHINGABOUT.Sign(context.KeyPair);

vncoelho · 2018-10-24T11:59:37Z

neo/Consensus/ConsensusService.cs

+                    // If signature was sent, we send again
+
+                    SignAndRelay(context.MakePrepareResponse(context.Signatures[context.MyIndex]));
+                    CheckSignatures();


Maybe remove this CheckSignatures here because the node is just re-sending. It should keep Checking at OnPrepareResponseReceived which also filters messages from nodes with wrong view and etc...

igormcoelho · 2018-10-24T13:12:21Z

Still some repeated logs.. we must see that:

[12:54:34.780] OnPrepareResponseReceived: height=27 view=0 index=0
[12:54:34.789] OnPrepareResponseReceived: height=27 view=0 index=0
[12:54:34.790] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=0
[12:54:34.794] relay block: 0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92
[12:54:34.794] OnCommitAgreement: height=27 hash=0x12313680c434188bdc316d641958694efff7b0f656a9ec4b84880ae89ceb6e92 view=0 index=0

igormcoelho · 2018-10-24T13:46:11Z

neo/Consensus/ConsensusState.cs

        SignatureSent = 0x10,
        BlockSent = 0x20,
        ViewChanging = 0x40,
+        CommitSent = 0x80,


Correct 0x80.

neo/Consensus/ConsensusMessageType.cs

igormcoelho · 2018-10-24T13:58:16Z

neo/Consensus/ConsensusService.cs

+
+        private void OnCommitAgreement(ConsensusPayload payload, CommitAgreement message)
+        {
+            Log($"{nameof(OnCommitAgreement)}: height={payload.BlockIndex} hash={message.BlockHash.ToString()} view={message.ViewNumber} index={payload.ValidatorIndex}");


Can you put this message Log($"{nameof(OnCommitAgreement)}: ... after the if? So it won't be repeated.
Please, do that to Log($"{nameof(OnPrepareResponseReceived)}: ... too, because it's also repeated.

@shargon This is why messages are repeated, easy to fix.

shargon · 2018-10-24T14:17:26Z

@igormcoelho try again with 14a31e6 please :)

@vncoelho i removed the CheckSignatures like you said on 6b23061

neo/Consensus/ConsensusMessageType.cs

igormcoelho · 2018-10-24T14:31:36Z

neo/Consensus/ConsensusService.cs


        private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest message)
        {
+            if (context.State.HasFlag(ConsensusState.RequestReceived))


Very good move! Avoiding this repeated message.

igormcoelho · 2018-10-24T14:31:48Z

neo/Consensus/ConsensusService.cs

            if (context.State.HasFlag(ConsensusState.BlockSent)) return;
            if (context.Signatures[payload.ValidatorIndex] != null) return;
+
+            Log($"{nameof(OnPrepareResponseReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex}");


vncoelho

Try to filter before RequestView

vncoelho · 2018-10-24T14:56:24Z

neo/Consensus/ConsensusService.cs


        private void RequestChangeView()
        {
+            if (context.State.HasFlag(ConsensusState.CommitSent))


let's remove this and filter before, @shargon.

else if ((context.State.HasFlag(ConsensusState.Primary) && context.State.HasFlag(ConsensusState.RequestSent)) || (context.State.HasFlag(ConsensusState.Backup) && !context.State.HasFlag(ConsensusState.CommitSent)) )

at line

neo/neo/Consensus/ConsensusService.cs

Line 384 in 6b23061

else if ((context.State.HasFlag(ConsensusState.Primary) && context.State.HasFlag(ConsensusState.RequestSent)) || context.State.HasFlag(ConsensusState.Backup))

igormcoelho · 2018-10-24T14:59:22Z

neo/Consensus/ConsensusService.cs

+                {
+                    // If signature was sent, we send again
+
+                    SignAndRelay(context.MakePrepareResponse(context.Signatures[context.MyIndex]));


Perhaps you need to Relay your CommitAgreement again too. Not sure. Or is it implicit by PrepareResponse message?

igormcoelho · 2018-10-25T01:27:44Z

Shargon, it's looking good for me! I'll try to simulate forking scenarios again, to make sure it's 100%... but congratulations in advance ;)

longfeiWan9 · 2018-10-29T10:01:51Z

neo/Consensus/ConsensusService.cs

+                {
+                    // If signature was sent, we send again
+
+                    SignAndRelay(context.MakePrepareResponse(context.Signatures[context.MyIndex]));


@shargon ，if a node (in commit phase) receives change view request, and re-send prepareResponse. From current code, the node (who have sent the changeView request ) will not accept prepareResponse because the viewNumber does not match. We may need to find a way to accept the re-send prepareResponse.

Is just for dropped packets (network errors), maybe there are a disconnection, and this packet never arrive to your CN.

yes. Imagine one CN(A) sent ChangeView request to other CN if it did not collect enough PrepareResponse on time because the network error. Then A increases its view number. But when other CN(B) receives the ChangeView message, it resends prepareResponse because B is in commit phase now.

In this situation, even if A re-connect with B, A will not accept B's resent prepareResponse message because view numbers do not match anymore. But we need to let A accepts this message and move on to commit phase as B, right ?

longfeiWan9 · 2018-10-30T09:50:19Z

neo/Consensus/ConsensusService.cs

+            if (context.State.HasFlag(ConsensusState.BlockSent) ||
+                !context.TryToCommit(payload, message)) return;
+
+            Log($"{nameof(OnCommitAgreement)}: height={payload.BlockIndex} hash={message.BlockHash.ToString()} view={message.ViewNumber} index={payload.ValidatorIndex}");


Could you please move log() before if()? From the log, it looks like CN send out block when it receives only one commitAgreement message. This is very confusing.

shargon · 2018-11-21T07:19:06Z

I close this for focus on #426

erikzhang · 2018-11-21T08:05:26Z

I would prefer a smaller pr instead of a big pr with many changes.

vncoelho · 2018-11-21T08:25:57Z

Hey @erikzhang,
You are right about smaller changes. However, this is a special case and occasion.

I think that NGD is already testing the other PR and we also have been testing it for a couple of days.

Most of the changes there are quite solid and required minor redesigns;
However, since changes require split of Full Signature before the commit phase (as we discussed here and in previous conversation) we needed to make that adjustments;
The new itself design proportionate new things that would be hard to implement separately because are all interlaced;
Currently, all changes are quite documented in the code and we gonna clean after everyone agrees in every little aspect;
There are just minor improvements that could be separated and it would be hard to decide if they are improvements and not lack of efficiency in implementing the new design;
I do not believe it is really 100% correct but we are finally reaching close to it. In the beginning that PR was kind of an exercise. It is already something that contains the contributions of many of us, because every change was previous discussed and detected in group in our previous Issues and PR's.

The PR is a sum of several contributions that all of us have been discussing in the past months.
I think that we should really focus there.

Commit phase

63847fc

shargon added Enhancement Type - Changes that may affect performance, usability or add new features to existing modules. Critical Issues (bugs) that need to be fixed ASAP labels Oct 23, 2018

shargon requested review from PeterLinX, belane, erikzhang and fabwa October 23, 2018 11:56

shargon mentioned this pull request Oct 23, 2018

Stage 3 of dBFT (Commit) #320

Closed

3 tasks

Lock change view if commit was sent

064ebc9

vncoelho reviewed Oct 23, 2018

View reviewed changes

Lock View change and resend the signature

5f3e99a

If someone sent view change and is locked, relay the signature again

f0c2ada

vncoelho reviewed Oct 24, 2018

View reviewed changes

igormcoelho reviewed Oct 24, 2018

View reviewed changes

Move logs to avoid repetitions

14a31e6

Just relay again

6b23061

igormcoelho reviewed Oct 24, 2018

View reviewed changes

vncoelho reviewed Oct 24, 2018

View reviewed changes

igormcoelho reviewed Oct 24, 2018

View reviewed changes

Update ConsensusMessageType.cs

c8eb77b

longfeiWan9 reviewed Oct 29, 2018

View reviewed changes

vncoelho mentioned this pull request Oct 29, 2018

Minnor fix on consensus OnPrepareResponseReceived #434

Closed

longfeiWan9 reviewed Oct 30, 2018

View reviewed changes

shargon added 3 commits October 30, 2018 11:29

Update ConsensusService.cs

bdb9d96

Merge branch 'master' into commit-phase

91dcaad

Update ConsensusService.cs

d1d950d

shargon closed this Nov 21, 2018

erikzhang reopened this Nov 21, 2018

edgedlt mentioned this pull request Dec 24, 2018

[Question] Stuck at 2623809/2623809/2623809 neo-project/neo-node#219

Closed

vncoelho mentioned this pull request Jan 14, 2019

dBFT 2.0 #547

Merged

erikzhang closed this Jan 24, 2019

shargon deleted the commit-phase branch January 24, 2019 16:27

erikzhang added this to the NEO 3.0 milestone Jan 25, 2019

Conversation

shargon commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Oct 23, 2018

Uh oh!

vncoelho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon commented Oct 23, 2018

Uh oh!

vncoelho commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

belane commented Oct 23, 2018

Uh oh!

vncoelho commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vncoelho commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

vncoelho Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igormcoelho commented Oct 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vncoelho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igormcoelho Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igormcoelho commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shargon commented Nov 21, 2018

Uh oh!

erikzhang commented Nov 21, 2018

Uh oh!

vncoelho commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

shargon commented Oct 23, 2018 •

edited

Loading

vncoelho left a comment •

edited

Loading

vncoelho commented Oct 23, 2018 •

edited

Loading

vncoelho commented Oct 23, 2018 •

edited

Loading

vncoelho commented Oct 23, 2018 •

edited

Loading

shargon commented Oct 24, 2018 •

edited

Loading

vncoelho Oct 24, 2018 •

edited

Loading

shargon commented Oct 24, 2018 •

edited

Loading

igormcoelho Oct 24, 2018 •

edited

Loading

vncoelho commented Nov 21, 2018 •

edited

Loading