fix consensus #425

erikzhang · 2018-10-24T16:58:42Z

No description provided.

vncoelho · 2018-10-24T21:19:46Z

This fix is more related to avoiding communication after block is sent and verification of signatures, not so much related to the commit phase we have been working, right?
Good move.

igormcoelho

Nice changes! Seem to allow PrepareResponse to arrive prior to PrepareRequest, which happens a lot in practice. Usually timer used to finish together with block relay too, good fix... now with Akka I haven't seen anymore.

shargon · 2018-10-25T09:46:00Z

@erikzhang could you explain what happend here? why the necesity of review the signatures?

neo/neo/Consensus/ConsensusService.cs

Lines 252 to 257 in 92e55f1

    
           byte[] hashData = context.MakeHeader().GetHashData(); 
        
           if (!Crypto.Default.VerifySignature(hashData, message.Signature, context.Validators[payload.ValidatorIndex].EncodePoint(false))) return; 
        
           for (int i = 0; i < context.Signatures.Length; i++) 
        
               if (context.Signatures[i] != null) 
        
                   if (!Crypto.Default.VerifySignature(hashData, context.Signatures[i], context.Validators[i].EncodePoint(false))) 
        
                       context.Signatures[i] = null;

vncoelho · 2018-10-25T09:51:30Z

What if a normal RPC inject this, Shargon?
I think that this verification ensures that.

igormcoelho · 2018-10-25T11:29:07Z

I think its not reviewing signatures shargon, its allowing to.keep sigs from PrepReponse if PrepRequest arrives later.. the previous code was dropping useful signatures I guess.

erikzhang · 2018-10-25T11:59:05Z

I think its not reviewing signatures shargon, its allowing to.keep sigs from PrepReponse if PrepRequest arrives later.. the previous code was dropping useful signatures I guess.

Yes

shargon · 2018-10-25T12:29:30Z

It seems good for me, we will test it! :)

erikzhang · 2018-10-25T20:53:07Z

I updated it again and solved the consensus problem that is now facing. Please review it again. @shargon @vncoelho @igormcoelho

vncoelho · 2018-10-26T03:58:02Z

Double checked, @erikzhang. Great job and precise.

vncoelho · 2018-10-26T04:16:28Z

Erik, what if you verify the transactions OnPersist and, then, delete/remove the ones that fails the test along with these ones here

neo/neo/Ledger/Blockchain.cs

Line 382 in c2d7bd4

mem_pool.TryRemove(tx.Hash, out _);

?

igormcoelho · 2018-10-26T04:58:18Z

@erikzhang This change just feels redundant to me, because these tx are supposed to exist on mempool already... so perhaps it's something I didn't get. The most important is to get Neo safer and safer now, even if it costs performance temporarialy, so I agree with the changes.

shargon · 2018-10-26T08:13:04Z

@erikzhang if a CN doesn't have a TX, where he asking for it?

erikzhang · 2018-10-26T09:11:30Z

Erik, what if you verify the transactions OnPersist and, then, delete/remove the ones that fails the test along with these ones here

If there are a large number of transactions in the memory pool, it takes a lot of time to re-verify them all.

This change just feels redundant to me, because these tx are supposed to exist on mempool already...

After the block is persisted, all transactions in the memory pool must be re-verified.

if a CN doesn't have a TX, where he asking for it?

neo/neo/Consensus/ConsensusService.cs

Lines 325 to 330 in c2d7bd4

    
           SignAndRelay(context.MakePrepareRequest()); 
        
           if (context.TransactionHashes.Length > 1) 
        
           { 
        
               foreach (InvPayload payload in InvPayload.CreateGroup(InventoryType.TX, context.TransactionHashes.Skip(1).ToArray())) 
        
                   system.LocalNode.Tell(Message.Create("inv", payload)); 
        
           }

The primary will re-relay the transactions after sending the PrepareRequest.

vncoelho · 2018-10-26T15:37:07Z

Thanks, Erik.

If there are a large number of transactions in the memory pool, it takes a lot of time to re-verify them all.

Does this re-verifiying occurs naturally at the OnNewTransaction events (however, depends on P2P communication)?

One last question and summary, Erik. 🗡️

OnPersistCompleted Self.Tell(tx, ActorRefs.NoSender) send a message to All Connections with all current reimaining TXs in its local mempool, right?
Then, mem_pool is clear;
Block is distributed to everyone again;
Speaker FillContext only using its local memory pool with call .GetMemoryPool(). Since mem_pool was clear, Speaker memory pool just contains transactions that arrived though OnNewTransaction calls, correct?
Backups checks the Primary Context and get missing transactions from mem_pool_unverified for speed up instead of waiting for a transaction to arrive naturally as OnNewTransaction.

Thanks for the patient.

Following this reasoning, one could see this mem_pool_unverified as a speed up instead of a fix?

belane · 2018-10-26T16:48:41Z

Tested on private net and working fine.

==> /opt/node1/neo-cli/Logs/2018-10-26.log <==
[16:06:12.316] initialize: height=90 view=0 index=2 role=Primary
[16:06:27.323] timeout: height=90 view=0 state=Primary
[16:06:27.324] send prepare request: height=90 view=0
[16:06:27.345] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.380] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.385] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.390] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.394] relay block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a
[16:06:27.397] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.400] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.401] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.405] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.410] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.413] persist block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a

==> /opt/node2/neo-cli/Logs/2018-10-26.log <==
[16:06:12.322] initialize: height=90 view=0 index=0 role=Backup
[16:06:27.328] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.340] send prepare response
[16:06:27.341] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.345] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.346] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.352] relay block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a
[16:06:27.358] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.360] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.361] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.365] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.368] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.376] persist block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a

==> /opt/node3/neo-cli/Logs/2018-10-26.log <==
[16:06:12.337] initialize: height=90 view=0 index=3 role=Backup
[16:06:27.328] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.334] send prepare response
[16:06:27.335] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.335] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.342] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.364] relay block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a
[16:06:27.373] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.374] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.374] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.375] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.386] OnPrepareResponseReceived: height=90 view=0 index=1
[16:06:27.398] persist block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a

==> /opt/node4/neo-cli/Logs/2018-10-26.log <==
[16:06:12.325] initialize: height=90 view=0 index=1 role=Backup
[16:06:27.335] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.343] send prepare response
[16:06:27.347] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.347] OnPrepareRequestReceived: height=90 view=0 index=2 tx=1
[16:06:27.348] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.355] relay block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a
[16:06:27.369] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.371] OnPrepareResponseReceived: height=90 view=0 index=3
[16:06:27.372] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.380] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.383] OnPrepareResponseReceived: height=90 view=0 index=0
[16:06:27.388] persist block: 0x5440aad0193a621d92555f35b914b5e1f71743d1c94b8b94df64b69161308d4a

belane · 2018-10-26T17:02:18Z

I would prefer to re-verify the pending TXs in Blockchain.OnPersistCompleted insted to move them to mem_pool_unverified because they are now available only for backup nodes if they receive a PrepareRequest with one of these TX but are not available for the Primary in FillContext.

erikzhang · 2018-10-26T17:25:11Z

If there are a lot of transactions in the memory pool, it will take a long time to re-verify them. And it will block the consensus. That's why I move them into mem_pool_unverified.

shargon · 2018-10-26T17:30:02Z

This lock the state when a Block was sent, right? do you think that is needed the commit phase after this PR?

vncoelho · 2018-10-26T17:45:43Z

@erikzhang, I agree with you that we should not re-verify at OnPersist.

I think that this mem_pool_unverified is a speed up and not a fix, right?
I am trying to understand how the mempool is filled again.
Nowadays, all transaction that were cleared arrive now with Self.Tell(tx, ActorRefs.NoSender) as a OnNewTransaction event one by one?

With this PR the Backups will be able to speed up the process of getting a hash that they do not know.

erikzhang · 2018-10-26T18:06:03Z

I think that this mem_pool_unverified is a speed up and not a fix, right?

Right.

vncoelho · 2018-10-26T18:55:16Z

Lets go for it and then we can merge with the commit phase for finally tackling that.

vncoelho

@erikzhang, last time question... 🔢

vncoelho · 2018-10-27T13:32:47Z

neo/Consensus/ConsensusService.cs

+            byte[] hashData = context.MakeHeader()?.GetHashData();
+            if (hashData == null)
+            {
+                context.Signatures[payload.ValidatorIndex] = message.Signature;


@erikzhang, I am double checking here and just to be sure I am thinking why message.Signature is assigned here to the Signatures array.

Should not it be context.Signatures[payload.ValidatorIndex] = null?
If the hashData is null it means that the payload is kind of empty, right?

erikzhang · 2018-10-27T14:01:52Z

I think @igormcoelho has answered your question:

I think its not reviewing signatures shargon, its allowing to.keep sigs from PrepReponse if PrepRequest arrives later.. the previous code was dropping useful signatures I guess.

vncoelho · 2018-10-27T14:20:52Z

That part of reviewing signatures I understood (I think, eahueauea).

I mean this line

neo/neo/Consensus/ConsensusService.cs

Line 296 in 4407478

context.Signatures[payload.ValidatorIndex] = message.Signature;

Should not it be context.Signatures[payload.ValidatorIndex] = null;?

Because Node B could receive wrong empty hashdata of (C,D) and then proceed to CheckSignatures(); when a correct signature of node E arrives.
And CheckSignature could pass, relaying a block, because it does not verify again.

It it is null the verification will not count them context.Signatures.Count(p => p != null):

neo/neo/Consensus/ConsensusService.cs

Line 85 in 4407478

    
           if (context.Signatures.Count(p => p != null) >= context.M && context.TransactionHashes.All(p => context.Transactions.ContainsKey(p)))

fix consensus

92e55f1

erikzhang requested a review from shargon October 24, 2018 16:58

igormcoelho previously approved these changes Oct 25, 2018

View reviewed changes

cache unverified transactions

a142c92

erikzhang dismissed igormcoelho’s stale review via a142c92 October 25, 2018 20:50

Merge branch 'master' into fixes/consensus

4407478

vncoelho mentioned this pull request Oct 26, 2018

Question about Verify OnTransaction #429

Closed

shargon approved these changes Oct 27, 2018

View reviewed changes

erikzhang merged commit 2227e16 into master Oct 27, 2018

erikzhang deleted the fixes/consensus branch October 27, 2018 09:56

vncoelho reviewed Oct 27, 2018

View reviewed changes

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020

[EN] Better wording (neo-project#425)

e91cfab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix consensus #425

fix consensus #425

erikzhang commented Oct 24, 2018

vncoelho commented Oct 24, 2018 •

edited

Loading

igormcoelho left a comment

shargon commented Oct 25, 2018

vncoelho commented Oct 25, 2018 •

edited

Loading

igormcoelho commented Oct 25, 2018

erikzhang commented Oct 25, 2018

shargon commented Oct 25, 2018

erikzhang commented Oct 25, 2018

vncoelho commented Oct 26, 2018 •

edited

Loading

vncoelho commented Oct 26, 2018 •

edited

Loading

igormcoelho commented Oct 26, 2018 •

edited

Loading

shargon commented Oct 26, 2018

erikzhang commented Oct 26, 2018

vncoelho commented Oct 26, 2018 •

edited

Loading

belane commented Oct 26, 2018

belane commented Oct 26, 2018

erikzhang commented Oct 26, 2018

shargon commented Oct 26, 2018

vncoelho commented Oct 26, 2018 •

edited

Loading

erikzhang commented Oct 26, 2018

vncoelho commented Oct 26, 2018

vncoelho left a comment

vncoelho Oct 27, 2018 •

edited

Loading

erikzhang commented Oct 27, 2018

vncoelho commented Oct 27, 2018 •

edited

Loading

fix consensus #425

fix consensus #425

Conversation

erikzhang commented Oct 24, 2018

vncoelho commented Oct 24, 2018 • edited Loading

igormcoelho left a comment

Choose a reason for hiding this comment

shargon commented Oct 25, 2018

vncoelho commented Oct 25, 2018 • edited Loading

igormcoelho commented Oct 25, 2018

erikzhang commented Oct 25, 2018

shargon commented Oct 25, 2018

erikzhang commented Oct 25, 2018

vncoelho commented Oct 26, 2018 • edited Loading

vncoelho commented Oct 26, 2018 • edited Loading

igormcoelho commented Oct 26, 2018 • edited Loading

shargon commented Oct 26, 2018

erikzhang commented Oct 26, 2018

vncoelho commented Oct 26, 2018 • edited Loading

belane commented Oct 26, 2018

belane commented Oct 26, 2018

erikzhang commented Oct 26, 2018

shargon commented Oct 26, 2018

vncoelho commented Oct 26, 2018 • edited Loading

erikzhang commented Oct 26, 2018

vncoelho commented Oct 26, 2018

vncoelho left a comment

Choose a reason for hiding this comment

vncoelho Oct 27, 2018 • edited Loading

Choose a reason for hiding this comment

erikzhang commented Oct 27, 2018

vncoelho commented Oct 27, 2018 • edited Loading

vncoelho commented Oct 24, 2018 •

edited

Loading

vncoelho commented Oct 25, 2018 •

edited

Loading

vncoelho commented Oct 26, 2018 •

edited

Loading

vncoelho commented Oct 26, 2018 •

edited

Loading

igormcoelho commented Oct 26, 2018 •

edited

Loading

vncoelho commented Oct 26, 2018 •

edited

Loading

vncoelho commented Oct 26, 2018 •

edited

Loading

vncoelho Oct 27, 2018 •

edited

Loading

vncoelho commented Oct 27, 2018 •

edited

Loading