Improve Consensus Liveness: Count failed nodes to inhibit changing vi…

…ew and request recovery (#665)

neo.UnitTests/UT_Consensus.cs

@@ -239,13 +239,13 @@ public void TestSerializeAndDeserializeConsensusContext()
             consensusContext.Timestamp = TimeProvider.Current.UtcNow.ToTimestamp();
 
             consensusContext.ChangeViewPayloads = new ConsensusPayload[consensusContext.Validators.Length];
-            consensusContext.ChangeViewPayloads[0] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, NewViewNumber = 2, Timestamp = 6 }, 0, new[] { (byte)'A' });
-            consensusContext.ChangeViewPayloads[1] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, NewViewNumber = 2, Timestamp = 5 }, 1, new[] { (byte)'B' });
+            consensusContext.ChangeViewPayloads[0] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, Timestamp = 6 }, 0, new[] { (byte)'A' });
+            consensusContext.ChangeViewPayloads[1] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, Timestamp = 5 }, 1, new[] { (byte)'B' });
             consensusContext.ChangeViewPayloads[2] = null;
-            consensusContext.ChangeViewPayloads[3] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, NewViewNumber = 2, Timestamp = uint.MaxValue }, 3, new[] { (byte)'C' });
+            consensusContext.ChangeViewPayloads[3] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, Timestamp = uint.MaxValue }, 3, new[] { (byte)'C' });
             consensusContext.ChangeViewPayloads[4] = null;
             consensusContext.ChangeViewPayloads[5] = null;
-            consensusContext.ChangeViewPayloads[6] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, NewViewNumber = 2, Timestamp = 1 }, 6, new[] { (byte)'D' });
+            consensusContext.ChangeViewPayloads[6] = MakeSignedPayload(consensusContext, new ChangeView { ViewNumber = 1, Timestamp = 1 }, 6, new[] { (byte)'D' });
 
             consensusContext.LastChangeViewPayloads = new ConsensusPayload[consensusContext.Validators.Length];
 

neo/Consensus/ChangeView.cs

@@ -4,34 +4,31 @@ namespace Neo.Consensus
 {
     public class ChangeView : ConsensusMessage
     {
-        public byte NewViewNumber;
+        /// <summary>
+        /// NewViewNumber is always set to the current ViewNumber asking changeview + 1
+        /// </summary>
+        public byte NewViewNumber => (byte)(ViewNumber + 1);
         /// <summary>
         /// Timestamp of when the ChangeView message was created. This allows receiving nodes to ensure
-        // they only respond once to a specific ChangeView request (it thus prevents replay of the ChangeView
-        // message from repeatedly broadcasting RecoveryMessages).
+        /// they only respond once to a specific ChangeView request (it thus prevents replay of the ChangeView
+        /// message from repeatedly broadcasting RecoveryMessages).
         /// </summary>
         public uint Timestamp;
 
         public override int Size => base.Size
-            + sizeof(byte)  //NewViewNumber
             + sizeof(uint); //Timestamp
 
-        public ChangeView()
-            : base(ConsensusMessageType.ChangeView)
-        {
-        }
+        public ChangeView() : base(ConsensusMessageType.ChangeView) { }
 
         public override void Deserialize(BinaryReader reader)
         {
             base.Deserialize(reader);
-            NewViewNumber = reader.ReadByte();
             Timestamp = reader.ReadUInt32();
         }
 
         public override void Serialize(BinaryWriter writer)
         {
             base.Serialize(writer);
-            writer.Write(NewViewNumber);
             writer.Write(Timestamp);
         }
     }

neo/Consensus/ConsensusContext.cs

@@ -37,6 +37,9 @@ internal class ConsensusContext : IConsensusContext
         public ConsensusPayload[] CommitPayloads { get; set; }
         public ConsensusPayload[] ChangeViewPayloads { get; set; }
         public ConsensusPayload[] LastChangeViewPayloads { get; set; }
+        // LastSeenMessage array stores the height of the last seen message, for each validator.
+        // if this node never heard from validator i, LastSeenMessage[i] will be -1.
+        public int[] LastSeenMessage { get; set; }
         public Block Block { get; set; }
         public Snapshot Snapshot { get; private set; }
         private KeyPair keyPair;
@@ -139,7 +142,6 @@ public ConsensusPayload MakeChangeView(byte newViewNumber)
         {
             return ChangeViewPayloads[MyIndex] = MakeSignedPayload(new ChangeView
             {
-                NewViewNumber = newViewNumber,
                 Timestamp = TimeProvider.Current.UtcNow.ToTimestamp()
             });
         }
@@ -266,6 +268,12 @@ public void Reset(byte viewNumber)
                 ChangeViewPayloads = new ConsensusPayload[Validators.Length];
                 LastChangeViewPayloads = new ConsensusPayload[Validators.Length];
                 CommitPayloads = new ConsensusPayload[Validators.Length];
+                if (LastSeenMessage == null)
+                {
+                    LastSeenMessage = new int[Validators.Length];
+                    for (int i = 0; i < Validators.Length; i++)
+                        LastSeenMessage[i] = -1;
+                }
                 keyPair = null;
                 for (int i = 0; i < Validators.Length; i++)
                 {
@@ -279,7 +287,7 @@ public void Reset(byte viewNumber)
             else
             {
                 for (int i = 0; i < LastChangeViewPayloads.Length; i++)
-                    if (ChangeViewPayloads[i]?.GetDeserializedMessage<ChangeView>().NewViewNumber == viewNumber)
+                    if (ChangeViewPayloads[i]?.GetDeserializedMessage<ChangeView>().NewViewNumber >= viewNumber)
                         LastChangeViewPayloads[i] = ChangeViewPayloads[i];
                     else
                         LastChangeViewPayloads[i] = null;
@@ -289,6 +297,7 @@ public void Reset(byte viewNumber)
             Timestamp = 0;
             TransactionHashes = null;
             PreparationPayloads = new ConsensusPayload[Validators.Length];
+            if (MyIndex >= 0) LastSeenMessage[MyIndex] = (int) BlockIndex;
             _header = null;
         }
 

neo/Consensus/ConsensusService.cs

@@ -110,11 +110,14 @@ private void CheckCommits()
         private void CheckExpectedView(byte viewNumber)
         {
             if (context.ViewNumber == viewNumber) return;
-            if (context.ChangeViewPayloads.Count(p => p != null && p.GetDeserializedMessage<ChangeView>().NewViewNumber == viewNumber) >= context.M())
+            // if there are `M` change view payloads with NewViewNumber greater than viewNumber, then, it is safe to move
+            if (context.ChangeViewPayloads.Count(p => p != null && p.GetDeserializedMessage<ChangeView>().NewViewNumber >= viewNumber) >= context.M())
             {
                 if (!context.WatchOnly())
                 {
                     ChangeView message = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
+                    // Communicate the network about my agreement to move to `viewNumber`
+                    // if my last change view payload, `message`, has NewViewNumber lower than current view to change
                     if (message is null || message.NewViewNumber < viewNumber)
                         localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
                 }
@@ -136,15 +139,6 @@ private void CheckPreparations()
             }
         }
 
-        private byte GetLastExpectedView(int validatorIndex)
-        {
-            var lastPreparationPayload = context.PreparationPayloads[validatorIndex];
-            if (lastPreparationPayload != null)
-                return lastPreparationPayload.GetDeserializedMessage<ConsensusMessage>().ViewNumber;
-
-            return context.ChangeViewPayloads[validatorIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber ?? (byte)0;
-        }
-
         private void InitializeConsensus(byte viewNumber)
         {
             context.Reset(viewNumber);
@@ -193,7 +187,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 if (!context.CommitSent())
                 {
                     bool shouldSendRecovery = false;
-                    // Limit recovery to sending from `f` nodes when the request is from a lower view number.
+                    // Limit recovery to be sent from, at least, `f` nodes when the request is from a lower view number.
                     int allowedRecoveryNodeCount = context.F();
                     for (int i = 0; i < allowedRecoveryNodeCount; i++)
                     {
@@ -213,7 +207,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
 
             if (context.CommitSent()) return;
 
-            var expectedView = GetLastExpectedView(payload.ValidatorIndex);
+            var expectedView = context.ChangeViewPayloads[payload.ValidatorIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber ?? (byte)0;
             if (message.NewViewNumber <= expectedView)
                 return;
 
@@ -234,7 +228,7 @@ private void OnCommitReceived(ConsensusPayload payload, Commit commit)
 
             if (commit.ViewNumber == context.ViewNumber)
             {
-                Log($"{nameof(OnCommitReceived)}: height={payload.BlockIndex} view={commit.ViewNumber} index={payload.ValidatorIndex}");
+                Log($"{nameof(OnCommitReceived)}: height={payload.BlockIndex} view={commit.ViewNumber} index={payload.ValidatorIndex} nc={context.CountCommitted()} nf={context.CountFailed()}");
 
                 byte[] hashData = context.MakeHeader()?.GetHashData();
                 if (hashData == null)
@@ -267,6 +261,7 @@ private void OnConsensusPayload(ConsensusPayload payload)
                 return;
             }
             if (payload.ValidatorIndex >= context.Validators.Length) return;
+            context.LastSeenMessage[payload.ValidatorIndex] = (int) payload.BlockIndex;
             foreach (IP2PPlugin plugin in Plugin.P2PPlugins)
                 if (!plugin.OnConsensusMessage(payload))
                     return;
@@ -457,6 +452,12 @@ protected override void OnReceive(object message)
             }
         }
 
+        private void SendChangeViewToRequestRecovery(byte viewNumber)
+        {
+            if (context.BlockIndex == Blockchain.Singleton.HeaderHeight + 1)
+                localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
+        }
+
         private void OnStart(Start options)
         {
             Log("OnStart");
@@ -478,8 +479,8 @@ private void OnStart(Start options)
             }
             InitializeConsensus(0);
             // Issue a ChangeView with NewViewNumber of 0 to request recovery messages on start-up.
-            if (context.BlockIndex == Blockchain.Singleton.HeaderHeight + 1 && !context.WatchOnly())
-                localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(0) });
+            if (!context.WatchOnly())
+                SendChangeViewToRequestRecovery(0);
         }
 
         private void OnTimer(Timer timer)
@@ -534,11 +535,19 @@ public static Props Props(IActorRef localNode, IActorRef taskManager, Store stor
         private void RequestChangeView()
         {
             if (context.WatchOnly()) return;
-            byte expectedView = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber ?? 0;
-            if (expectedView < context.ViewNumber) expectedView = context.ViewNumber;
+            // Request for next view is always one view more than the current context.ViewNumber
+            // Nodes will not contribute for changing to a view higher than (context.ViewNumber+1), unless they are recovered
+            // The latter may happen by nodes in higher views with, at least, `M` proofs
+            byte expectedView = context.ViewNumber;
             expectedView++;
-            Log($"request change view: height={context.BlockIndex} view={context.ViewNumber} nv={expectedView}");
             ChangeTimer(TimeSpan.FromSeconds(Blockchain.SecondsPerBlock << (expectedView + 1)));
+            if ((context.CountCommitted() + context.CountFailed()) > context.F())
+            {
+                Log($"Skip requesting change view to nv={expectedView} because nc={context.CountCommitted()} nf={context.CountFailed()}");
+                SendChangeViewToRequestRecovery(context.ViewNumber);
+                return;
+            }
+            Log($"request change view: height={context.BlockIndex} view={context.ViewNumber} nv={expectedView} nc={context.CountCommitted()} nf={context.CountFailed()}");
             localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(expectedView) });
             CheckExpectedView(expectedView);
         }

neo/Consensus/Helper.cs

@@ -19,6 +19,10 @@ internal static class Helper
         public static bool WatchOnly(this IConsensusContext context) => context.MyIndex < 0;
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static Header PrevHeader(this IConsensusContext context) => context.Snapshot.GetHeader(context.PrevHash);
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static int CountCommitted(this IConsensusContext context) => context.CommitPayloads.Count(p => p != null);
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static int CountFailed(this IConsensusContext context) => context.LastSeenMessage.Count(p => p < (((int) context.BlockIndex) - 1));
 
         // Consensus States
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
@@ -33,14 +37,14 @@ internal static class Helper
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static bool ViewChanging(this IConsensusContext context) => !context.WatchOnly() && context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber > context.ViewNumber;
 
-        public static bool NotAcceptingPayloadsDueToViewChanging(this IConsensusContext context) => context.ViewChanging() && !context.MoreThanFNodesCommitted();
+        public static bool NotAcceptingPayloadsDueToViewChanging(this IConsensusContext context) => context.ViewChanging() && !context.MoreThanFNodesCommittedOrLost();
 
         // A possible attack can happen if the last node to commit is malicious and either sends change view after his
         // commit to stall nodes in a higher view, or if he refuses to send recovery messages. In addition, if a node
         // asking change views loses network or crashes and comes back when nodes are committed in more than one higher
-        // numbered view, it is possible for the node accepting recovery and commit in any of the higher views, thus
+        // numbered view, it is possible for the node accepting recovery to commit in any of the higher views, thus
         // potentially splitting nodes among views and stalling the network.
-        public static bool MoreThanFNodesCommitted(this IConsensusContext context) => context.CommitPayloads.Count(p => p != null) > context.F();
+        public static bool MoreThanFNodesCommittedOrLost(this IConsensusContext context) => (context.CountCommitted() + context.CountFailed()) > context.F();
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static uint GetPrimaryIndex(this IConsensusContext context, byte viewNumber)

neo/Consensus/IConsensusContext.cs

@@ -24,6 +24,7 @@ public interface IConsensusContext : IDisposable, ISerializable
         ConsensusPayload[] PreparationPayloads { get; set; }
         ConsensusPayload[] CommitPayloads { get; set; }
         ConsensusPayload[] ChangeViewPayloads { get; set; }
+        int[] LastSeenMessage { get; set; }
         Block Block { get; set; }
         Snapshot Snapshot { get; }
 

neo/Consensus/RecoveryMessage.cs

@@ -57,7 +57,6 @@ public ConsensusPayload[] GetChangeViewPayloads(IConsensusContext context, Conse
                 ConsensusMessage = new ChangeView
                 {
                     ViewNumber = p.OriginalViewNumber,
-                    NewViewNumber = ViewNumber,
                     Timestamp = p.Timestamp
                 },
                 Witness = new Witness