From 3f56cebc7cd3b9113e8ff05e6bdb5e0c484616ab Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 09:08:41 -0700
Subject: [PATCH 01/21] Restore Liveness: Prevent Commit after ChangeView.
 Require locking agreement to ChangeView.

---
 neo/Consensus/ChangeView.cs       |  16 +++-
 neo/Consensus/ConsensusContext.cs |  23 ++++--
 neo/Consensus/ConsensusService.cs | 132 ++++++++++++++++++------------
 neo/Consensus/Helper.cs           |  18 +++-
 neo/Consensus/RecoveryMessage.cs  |  19 ++++-
 5 files changed, 140 insertions(+), 68 deletions(-)

diff --git a/neo/Consensus/ChangeView.cs b/neo/Consensus/ChangeView.cs
index f2ec0ccac3..9f0881ea65 100644
--- a/neo/Consensus/ChangeView.cs
+++ b/neo/Consensus/ChangeView.cs
@@ -7,14 +7,22 @@ public class ChangeView : ConsensusMessage
         public byte NewViewNumber;
         /// <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;
+        /// <summary>
+        /// Flag whether the node is locked/committed to change view from it's current view.
+        /// If not set, this indicates this is a request to change view, but not a commitment, and therefore it may
+        /// still accept further preparations and commit to generate a block in the current view.
+        /// If set, this node is locked to change view, and will not accept further preparations in the current view.
+        /// </summary>
+        public bool Locked;
 
         public override int Size => base.Size
             + sizeof(byte)  //NewViewNumber
-            + sizeof(uint); //Timestamp
+            + sizeof(uint)  //Timestamp
+            + sizeof(bool); //Committed
 
         public ChangeView()
             : base(ConsensusMessageType.ChangeView)
@@ -26,6 +34,7 @@ public override void Deserialize(BinaryReader reader)
             base.Deserialize(reader);
             NewViewNumber = reader.ReadByte();
             Timestamp = reader.ReadUInt32();
+            Locked = reader.ReadBoolean();
         }
 
         public override void Serialize(BinaryWriter writer)
@@ -33,6 +42,7 @@ public override void Serialize(BinaryWriter writer)
             base.Serialize(writer);
             writer.Write(NewViewNumber);
             writer.Write(Timestamp);
+            writer.Write(Locked);
         }
     }
 }
diff --git a/neo/Consensus/ConsensusContext.cs b/neo/Consensus/ConsensusContext.cs
index b7562898dc..ccf578c1af 100644
--- a/neo/Consensus/ConsensusContext.cs
+++ b/neo/Consensus/ConsensusContext.cs
@@ -137,21 +137,23 @@ public bool Load()
 
         public ConsensusPayload MakeChangeView(byte newViewNumber)
         {
+            var changeViewLockedCount = ChangeViewPayloads.Count(p => p != null && p.GetDeserializedMessage<ChangeView>().NewViewNumber > ViewNumber);
+            if (ChangeViewPayloads[MyIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber <= ViewNumber && newViewNumber > ViewNumber)
+                changeViewLockedCount++;
             return ChangeViewPayloads[MyIndex] = MakeSignedPayload(new ChangeView
             {
                 NewViewNumber = newViewNumber,
-                Timestamp = TimeProvider.Current.UtcNow.ToTimestamp()
+                Timestamp = TimeProvider.Current.UtcNow.ToTimestamp(),
+                Locked = changeViewLockedCount >= this.M()
             });
         }
 
         public ConsensusPayload MakeCommit()
         {
-            if (CommitPayloads[MyIndex] == null)
-                CommitPayloads[MyIndex] = MakeSignedPayload(new Commit
-                {
-                    Signature = MakeHeader()?.Sign(keyPair)
-                });
-            return CommitPayloads[MyIndex];
+            return CommitPayloads[MyIndex] ?? (CommitPayloads[MyIndex] = MakeSignedPayload(new Commit
+            {
+                Signature = MakeHeader()?.Sign(keyPair)
+            }));
         }
 
         private Block _header = null;
@@ -235,6 +237,7 @@ public ConsensusPayload MakeRecoveryMessage()
             }
             return MakeSignedPayload(new RecoveryMessage()
             {
+                Timestamp = TimeProvider.Current.UtcNow.ToTimestamp(),
                 ChangeViewMessages = LastChangeViewPayloads.Where(p => p != null).Select(p => RecoveryMessage.ChangeViewPayloadCompact.FromPayload(p)).Take(this.M()).ToDictionary(p => (int)p.ValidatorIndex),
                 PrepareRequestMessage = prepareRequestMessage,
                 // We only need a PreparationHash set if we don't have the PrepareRequest information.
@@ -280,10 +283,14 @@ public void Reset(byte viewNumber)
             else
             {
                 for (int i = 0; i < LastChangeViewPayloads.Length; i++)
-                    if (ChangeViewPayloads[i]?.GetDeserializedMessage<ChangeView>().NewViewNumber == viewNumber)
+                {
+                    var changeViewMessage = ChangeViewPayloads[i]?.GetDeserializedMessage<ChangeView>();
+                    if (changeViewMessage?.NewViewNumber == viewNumber && changeViewMessage.Locked)
                         LastChangeViewPayloads[i] = ChangeViewPayloads[i];
                     else
                         LastChangeViewPayloads[i] = null;
+                }
+
             }
             ViewNumber = viewNumber;
             PrimaryIndex = this.GetPrimaryIndex(viewNumber);
diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index ab4f6e26f0..f5ed0336b0 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -51,18 +51,27 @@ public ConsensusService(IActorRef localNode, IActorRef taskManager, IConsensusCo
             Context.System.EventStream.Subscribe(Self, typeof(Blockchain.PersistCompleted));
         }
 
+        private void RequestChangeViewDueToInvalidRequest()
+        {
+            byte expectedView = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber ?? 0;
+            // requesting view change due to an invalid request should only count the expected view once, additional
+            // increments of the expected view have to be by timeouts until the view actually changes.
+            if (context.ViewNumber < expectedView) return;
+            RequestChangeView();
+        }
+
         private bool AddTransaction(Transaction tx, bool verify)
         {
             if (verify && !tx.Verify(context.Snapshot, context.Transactions.Values))
             {
                 Log($"Invalid transaction: {tx.Hash}{Environment.NewLine}{tx.ToArray().ToHexString()}", LogLevel.Warning);
-                RequestChangeView();
+                RequestChangeViewDueToInvalidRequest();
                 return false;
             }
             if (!Plugin.CheckPolicy(tx))
             {
                 Log($"reject tx: {tx.Hash}{Environment.NewLine}{tx.ToArray().ToHexString()}", LogLevel.Warning);
-                RequestChangeView();
+                RequestChangeViewDueToInvalidRequest();
                 return false;
             }
             context.Transactions[tx.Hash] = tx;
@@ -70,6 +79,10 @@ private bool AddTransaction(Transaction tx, bool verify)
             {
                 if (VerifyRequest())
                 {
+                    // If our view is already changing we cannot send a prepare response, since we can never send both
+                    // a change view and a commit message for the same view without potentially forking the network.
+                    if (context.ViewChanging()) return true;
+
                     // if we are the primary for this view, but acting as a backup because we recovered our own
                     // previously sent prepare request, then we don't want to send a prepare response.
                     if (context.IsPrimary() || context.WatchOnly()) return true;
@@ -80,7 +93,7 @@ private bool AddTransaction(Transaction tx, bool verify)
                 }
                 else
                 {
-                    RequestChangeView();
+                    RequestChangeViewDueToInvalidRequest();
                     return false;
                 }
             }
@@ -99,27 +112,30 @@ private void ChangeTimer(TimeSpan delay)
 
         private void CheckCommits()
         {
-            if (context.CommitPayloads.Count(p => p != null) >= context.M() && context.TransactionHashes.All(p => context.Transactions.ContainsKey(p)))
-            {
-                Block block = context.CreateBlock();
-                Log($"relay block: {block.Hash}");
-                localNode.Tell(new LocalNode.Relay { Inventory = block });
-            }
+            if (context.CommitPayloads.Count(p => p != null) < context.M() || !context.TransactionHashes.All(p => context.Transactions.ContainsKey(p))) return;
+            Block block = context.CreateBlock();
+            Log($"relay block: {block.Hash}");
+            localNode.Tell(new LocalNode.Relay { Inventory = block });
         }
 
         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 (context.ChangeViewPayloads.Count(p => p != null && p.GetDeserializedMessage<ChangeView>().NewViewNumber == viewNumber) < context.M()) return;
+            if (!context.WatchOnly())
             {
-                if (!context.WatchOnly())
-                {
-                    ChangeView message = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
-                    if (message is null || message.NewViewNumber < viewNumber)
-                        localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
-                }
-                InitializeConsensus(viewNumber);
+                ChangeView message = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
+                if (!context.CommitSent() && (message is null || message.NewViewNumber < viewNumber || !message.Locked))
+                    localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
             }
+
+            if (context.ChangeViewPayloads
+                .Count(p =>
+                    {
+                        ChangeView message = p?.GetDeserializedMessage<ChangeView>();
+                        return message != null && message.NewViewNumber == viewNumber && message.Locked;
+                    }) >= context.M())
+                InitializeConsensus(viewNumber);
         }
 
         private void CheckPreparations()
@@ -136,15 +152,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);
@@ -178,6 +185,19 @@ private void Log(string message, LogLevel level = LogLevel.Info)
             Plugin.Log(nameof(ConsensusService), level, message);
         }
 
+        private bool IsRecoveryAllowed(ushort validatorIndex, byte startingValidatorOffset, int allowedRecoveryNodeCount)
+        {
+            for (int i = 0; i < allowedRecoveryNodeCount; i++)
+            {
+                var eligibleResponders = context.Validators.Length - 1;
+                var chosenIndex = (validatorIndex + i + startingValidatorOffset) % eligibleResponders;
+                if (chosenIndex >= validatorIndex) chosenIndex++;
+                if (chosenIndex != context.MyIndex) continue;
+                return true;
+            }
+            return false;
+        }
+
         private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
         {
             // We keep track of the payload hashes received in this block, and don't respond with recovery
@@ -185,37 +205,36 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // ChangeView messages include a Timestamp when the change view is sent, thus if a node restarts
             // and issues a change view for the same view, it will have a different hash and will correctly respond
             // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
-            // additonal recovery message response.
+            // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
             if (message.NewViewNumber <= context.ViewNumber)
             {
                 if (context.WatchOnly()) return;
-                bool shouldSendRecovery = false;
-                // Limit recovery to sending from `f` nodes when the request is from a lower view number.
-                int allowedRecoveryNodeCount = context.F();
-                for (int i = 0; i < allowedRecoveryNodeCount; i++)
-                {
-                    var eligibleResponders = context.Validators.Length - 1;
-                    var chosenIndex = (payload.ValidatorIndex + i + message.NewViewNumber) % eligibleResponders;
-                    if (chosenIndex >= payload.ValidatorIndex) chosenIndex++;
-                    if (chosenIndex != context.MyIndex) continue;
-                    shouldSendRecovery = true;
-                    break;
-                }
 
-                if (!shouldSendRecovery) return;
+                // Limit recovery to sending from `f` nodes when the request is from a lower view number.
+                if (!IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });
             }
 
-            if (context.CommitSent()) return;
+            var lastChangeViewMessage = context.ChangeViewPayloads[payload.ValidatorIndex]?.GetDeserializedMessage<ChangeView>();
+            // If we have had a change view message from this validator after our current view.
+            if (lastChangeViewMessage != null && lastChangeViewMessage.NewViewNumber > context.ViewNumber)
+            {
+                if (message.NewViewNumber < context.ViewNumber) return;
 
-            var expectedView = GetLastExpectedView(payload.ValidatorIndex);
-            if (message.NewViewNumber <= expectedView)
-                return;
+                if (lastChangeViewMessage.Locked && !message.Locked) return;
+                if (lastChangeViewMessage.Locked == message.Locked && message.NewViewNumber <= lastChangeViewMessage.NewViewNumber)
+                    return;
+            }
+            else
+            {
+                var expectedView = lastChangeViewMessage?.NewViewNumber ?? (byte)0;
+                if (message.NewViewNumber < expectedView) return;
+            }
 
-            Log($"{nameof(OnChangeViewReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex} nv={message.NewViewNumber}");
+            Log($"{nameof(OnChangeViewReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex} nv={message.NewViewNumber} committed={message.Locked}");
             context.ChangeViewPayloads[payload.ValidatorIndex] = payload;
             CheckExpectedView(message.NewViewNumber);
         }
@@ -286,7 +305,17 @@ private void OnPersistCompleted(Block block)
 
         private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage message)
         {
-            if (message.ViewNumber < context.ViewNumber) return;
+            if (message.ViewNumber < context.ViewNumber)
+            {
+                if (!knownHashes.Add(payload.Hash)) return;
+                // If we see a recovery message from a lower view we should recover them to our higher view; they may
+                // be stuck in commit.
+                if (!IsRecoveryAllowed(payload.ValidatorIndex, 1, context.F() + 1 ))
+                    return;
+                Log($"{nameof(OnRecoveryMessageReceived)} send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}");
+                localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });
+                return;
+            }
             Log($"{nameof(OnRecoveryMessageReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex}");
             // isRecovering is always set to false again after OnRecoveryMessageReceived
             isRecovering = true;
@@ -297,14 +326,13 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage
             {
                 if (message.ViewNumber > context.ViewNumber)
                 {
-                    if (context.CommitSent()) return;
                     ConsensusPayload[] changeViewPayloads = message.GetChangeViewPayloads(context, payload);
                     totalChangeViews = changeViewPayloads.Length;
                     foreach (ConsensusPayload changeViewPayload in changeViewPayloads)
                         if (ReverifyAndProcessPayload(changeViewPayload)) validChangeViews++;
                 }
                 if (message.ViewNumber != context.ViewNumber) return;
-                if (!context.CommitSent())
+                if (!context.ViewChanging() && !context.CommitSent())
                 {
                     if (!context.RequestSentOrReceived())
                     {
@@ -340,7 +368,7 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage
 
         private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest message)
         {
-            if (context.RequestSentOrReceived()) return;
+            if (context.RequestSentOrReceived() || context.ViewChanging()) return;
             if (payload.ValidatorIndex != context.PrimaryIndex) return;
             Log($"{nameof(OnPrepareRequestReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex} tx={message.TransactionHashes.Length}");
             if (message.Timestamp <= context.PrevHeader().Timestamp || message.Timestamp > TimeProvider.Current.UtcNow.AddMinutes(10).ToTimestamp())
@@ -399,7 +427,7 @@ private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest m
 
         private void OnPrepareResponseReceived(ConsensusPayload payload, PrepareResponse message)
         {
-            if (context.PreparationPayloads[payload.ValidatorIndex] != null) return;
+            if (context.PreparationPayloads[payload.ValidatorIndex] != null || context.ViewChanging()) return;
             if (context.PreparationPayloads[context.PrimaryIndex] != null && !message.PreparationHash.Equals(context.PreparationPayloads[context.PrimaryIndex].Hash))
                 return;
             Log($"{nameof(OnPrepareResponseReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex}");
@@ -495,10 +523,6 @@ private void OnTransaction(Transaction transaction)
             if (transaction.Type == TransactionType.MinerTransaction) return;
             if (!context.IsBackup() || !context.RequestSentOrReceived() || context.ResponseSent() || context.BlockSent())
                 return;
-            // If we are changing view but we already have enough preparation payloads to commit in the current view,
-            // we must keep on accepting transactions in the current view to be able to create the block.
-            if (context.ViewChanging() &&
-                context.PreparationPayloads.Count(p => p != null) < context.M()) return;
             if (context.Transactions.ContainsKey(transaction.Hash)) return;
             if (!context.TransactionHashes.Contains(transaction.Hash)) return;
             AddTransaction(transaction, true);
diff --git a/neo/Consensus/Helper.cs b/neo/Consensus/Helper.cs
index a5bc5b62e4..1e34573806 100644
--- a/neo/Consensus/Helper.cs
+++ b/neo/Consensus/Helper.cs
@@ -1,3 +1,4 @@
+using System.Linq;
 using Neo.Network.P2P.Payloads;
 using Neo.Persistence;
 using System.Runtime.CompilerServices;
@@ -29,7 +30,22 @@ internal static class Helper
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static bool BlockSent(this IConsensusContext context) => context.Block != null;
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public static bool ViewChanging(this IConsensusContext context) => !context.WatchOnly() && context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>().NewViewNumber > context.ViewNumber;
+        public static bool ViewChanging(this IConsensusContext context)
+        {
+            if (context.WatchOnly() || context.MoreThanFNodesCommitted()) return false;
+            var myChangeViewMessage = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
+            if (myChangeViewMessage == null) return false;
+
+            return myChangeViewMessage.Locked && myChangeViewMessage.NewViewNumber > context.ViewNumber;
+        }
+
+        /// <summary>
+        /// More than F nodes committed in current view.
+        /// </summary>
+        /// <param name="context">consensus context</param>
+        /// <returns>true if more than F nodes have committed in the current view.</returns>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static bool MoreThanFNodesCommitted(this IConsensusContext context) => context.CommitPayloads.Count(p => p != null) > context.F();
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static uint GetPrimaryIndex(this IConsensusContext context, byte viewNumber)
diff --git a/neo/Consensus/RecoveryMessage.cs b/neo/Consensus/RecoveryMessage.cs
index 64e05233fe..c1d41827aa 100644
--- a/neo/Consensus/RecoveryMessage.cs
+++ b/neo/Consensus/RecoveryMessage.cs
@@ -17,14 +17,28 @@ public partial class RecoveryMessage : ConsensusMessage
         public UInt256 PreparationHash;
         public Dictionary<int, PreparationPayloadCompact> PreparationMessages;
         public Dictionary<int, CommitPayloadCompact> CommitMessages;
+        /// <summary>
+        /// Timestamp of when the Recovery message was created. This allows receiving nodes to ensure
+        /// they only respond once to a specific RecoveryMessage request from a lower view.
+        /// </summary>
+        public uint Timestamp;
 
         public RecoveryMessage() : base(ConsensusMessageType.RecoveryMessage)
         {
         }
 
+        public override int Size => base.Size
+            + /* Timestamp */ sizeof(uint)
+            + /* ChangeViewMessages */ IO.Helper.GetVarSize(ChangeViewMessages?.Count ?? 0) + ChangeViewMessages?.Values.Sum(p => ((ISerializable)p).Size) ?? 0
+            + /* PrepareRequestMessage */ 1 + ((ISerializable) PrepareRequestMessage)?.Size ?? 0
+            + /* PreparationHash */ PreparationHash?.Size ?? 0
+            + /* PreparationMessages */IO.Helper.GetVarSize(PreparationMessages?.Count ?? 0) + PreparationMessages?.Values.Sum(p => ((ISerializable)p).Size) ?? 0
+            + /* CommitMessages */IO.Helper.GetVarSize(CommitMessages?.Count ?? 0) + CommitMessages?.Values.Sum(p => ((ISerializable)p).Size) ?? 0;
+
         public override void Deserialize(BinaryReader reader)
         {
             base.Deserialize(reader);
+            Timestamp = reader.ReadUInt32();
             ChangeViewMessages = reader.ReadSerializableArray<ChangeViewPayloadCompact>(Blockchain.MaxValidators).ToDictionary(p => (int)p.ValidatorIndex);
             if (reader.ReadBoolean())
                 PrepareRequestMessage = reader.ReadSerializable<PrepareRequest>();
@@ -51,7 +65,8 @@ public ConsensusPayload[] GetChangeViewPayloads(IConsensusContext context, Conse
                 {
                     ViewNumber = p.OriginalViewNumber,
                     NewViewNumber = ViewNumber,
-                    Timestamp = p.Timestamp
+                    Timestamp = p.Timestamp,
+                    Locked = true
                 },
                 Witness = new Witness
                 {
@@ -128,6 +143,7 @@ public ConsensusPayload[] GetPrepareResponsePayloads(IConsensusContext context,
         public override void Serialize(BinaryWriter writer)
         {
             base.Serialize(writer);
+            writer.Write(Timestamp);
             writer.Write(ChangeViewMessages.Values.ToArray());
             bool hasPrepareRequestMessage = PrepareRequestMessage != null;
             writer.Write(hasPrepareRequestMessage);
@@ -140,7 +156,6 @@ public override void Serialize(BinaryWriter writer)
                 else
                     writer.WriteVarBytes(PreparationHash.ToArray());
             }
-
             writer.Write(PreparationMessages.Values.ToArray());
             writer.Write(CommitMessages.Values.ToArray());
         }

From 4edf61f636f2afc79d642beff41fd421f5b9a5b6 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 10:21:22 -0700
Subject: [PATCH 02/21] To prevent duplicate blocks, since we allow commits to
 move, we cannot lock changing view if we have sent prepare response.

---
 neo/Consensus/ConsensusContext.cs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusContext.cs b/neo/Consensus/ConsensusContext.cs
index ccf578c1af..05c79f454e 100644
--- a/neo/Consensus/ConsensusContext.cs
+++ b/neo/Consensus/ConsensusContext.cs
@@ -144,7 +144,7 @@ public ConsensusPayload MakeChangeView(byte newViewNumber)
             {
                 NewViewNumber = newViewNumber,
                 Timestamp = TimeProvider.Current.UtcNow.ToTimestamp(),
-                Locked = changeViewLockedCount >= this.M()
+                Locked = changeViewLockedCount >= this.M() && !this.ResponseSent() && !(this.IsPrimary() && this.RequestSentOrReceived())
             });
         }
 

From 609954748c579f202aa169058a65233b88dee03e Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 10:34:14 -0700
Subject: [PATCH 03/21] Fix log message.

---
 neo/Consensus/ConsensusService.cs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index f5ed0336b0..64f4dbeec6 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -234,7 +234,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 if (message.NewViewNumber < expectedView) return;
             }
 
-            Log($"{nameof(OnChangeViewReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex} nv={message.NewViewNumber} committed={message.Locked}");
+            Log($"{nameof(OnChangeViewReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex} nv={message.NewViewNumber} locked={message.Locked}");
             context.ChangeViewPayloads[payload.ValidatorIndex] = payload;
             CheckExpectedView(message.NewViewNumber);
         }

From 01be5aaa7642660fe0635a28c73573eb3d376e93 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 11:02:47 -0700
Subject: [PATCH 04/21] Add additional criteria for sending recovery since we
 must recover nodes that missed prepare response.

---
 neo/Consensus/ConsensusService.cs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 64f4dbeec6..8a96132fbd 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -207,7 +207,8 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
             // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
-            if (message.NewViewNumber <= context.ViewNumber)
+            if (message.NewViewNumber <= context.ViewNumber || message.ViewNumber < context.ViewNumber ||
+                (message.ViewNumber == context.ViewNumber && (context.ResponseSent() || context.IsPrimary()) && !message.Locked && context.PreparationPayloads[payload.ValidatorIndex] == null))
             {
                 if (context.WatchOnly()) return;
 

From 87d61cb108ea277369b3310aeb4664ac0eb13502 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 11:22:53 -0700
Subject: [PATCH 05/21] Fix logic for inhibiting locking commit.

---
 neo/Consensus/ConsensusContext.cs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusContext.cs b/neo/Consensus/ConsensusContext.cs
index 05c79f454e..26085691e4 100644
--- a/neo/Consensus/ConsensusContext.cs
+++ b/neo/Consensus/ConsensusContext.cs
@@ -144,7 +144,7 @@ public ConsensusPayload MakeChangeView(byte newViewNumber)
             {
                 NewViewNumber = newViewNumber,
                 Timestamp = TimeProvider.Current.UtcNow.ToTimestamp(),
-                Locked = changeViewLockedCount >= this.M() && !this.ResponseSent() && !(this.IsPrimary() && this.RequestSentOrReceived())
+                Locked = changeViewLockedCount >= this.M() && !(this.ResponseSent() || this.IsPrimary())
             });
         }
 

From f795cf899dd62c48bab5f7dea90272db659447de Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 11:51:10 -0700
Subject: [PATCH 06/21] Primary should always help recover nodes missing
 prepare response.

---
 neo/Consensus/ConsensusService.cs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 8a96132fbd..7000174460 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -207,13 +207,15 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
             // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
+            bool shouldRecoverPrepareResponseInSameView = message.ViewNumber == context.ViewNumber
+                && (context.ResponseSent() || context.IsPrimary()) && !message.Locked && context.PreparationPayloads[payload.ValidatorIndex] == null;
             if (message.NewViewNumber <= context.ViewNumber || message.ViewNumber < context.ViewNumber ||
-                (message.ViewNumber == context.ViewNumber && (context.ResponseSent() || context.IsPrimary()) && !message.Locked && context.PreparationPayloads[payload.ValidatorIndex] == null))
+                shouldRecoverPrepareResponseInSameView)
             {
                 if (context.WatchOnly()) return;
 
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
-                if (!IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
+                if (!(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });

From 51fbbf7fbb573222b57c7391b5556056b68fcea0 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 12:15:21 -0700
Subject: [PATCH 07/21] Need to be able to recover commit in the same view.

---
 neo/Consensus/ConsensusService.cs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 7000174460..bd2882a850 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -207,15 +207,18 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
             // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
-            bool shouldRecoverPrepareResponseInSameView = message.ViewNumber == context.ViewNumber
-                && (context.ResponseSent() || context.IsPrimary()) && !message.Locked && context.PreparationPayloads[payload.ValidatorIndex] == null;
+            bool inSameViewAndUnlocked = message.ViewNumber == context.ViewNumber && !message.Locked;
+            bool shouldRecoverPrepareResponseInSameView = inSameViewAndUnlocked
+                 && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
+            bool shouldRecoverCommitInSameView = inSameViewAndUnlocked
+                && context.CommitSent() && context.CommitPayloads[payload.ValidatorIndex] == null;
             if (message.NewViewNumber <= context.ViewNumber || message.ViewNumber < context.ViewNumber ||
-                shouldRecoverPrepareResponseInSameView)
+                shouldRecoverPrepareResponseInSameView || shouldRecoverCommitInSameView)
             {
                 if (context.WatchOnly()) return;
 
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
-                if (!(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
+                if (!shouldRecoverCommitInSameView || !(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });

From 770ca579108080d3354154df85d9c01556a1416f Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 12:31:03 -0700
Subject: [PATCH 08/21] Fix bug in sending recovery logic.

---
 neo/Consensus/ConsensusService.cs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index bd2882a850..32e9e354dd 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -218,7 +218,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 if (context.WatchOnly()) return;
 
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
-                if (!shouldRecoverCommitInSameView || !(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
+                if (!shouldRecoverCommitInSameView && !(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });

From 00a5e0aaa32158117251827f3fa527994fc35e37 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 14:31:33 -0700
Subject: [PATCH 09/21] Fix recovery response logic.

---
 neo/Consensus/ConsensusService.cs | 2 +-
 neo/Consensus/Helper.cs           | 2 ++
 neo/Consensus/RecoveryMessage.cs  | 2 +-
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 32e9e354dd..aacdd7bf8b 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -207,7 +207,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
             // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
-            bool inSameViewAndUnlocked = message.ViewNumber == context.ViewNumber && !message.Locked;
+            bool inSameViewAndUnlocked = message.ViewNumber == context.ViewNumber && (!message.Locked || context.MoreThanFNodesPrepared());
             bool shouldRecoverPrepareResponseInSameView = inSameViewAndUnlocked
                  && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
             bool shouldRecoverCommitInSameView = inSameViewAndUnlocked
diff --git a/neo/Consensus/Helper.cs b/neo/Consensus/Helper.cs
index 1e34573806..bbe1aa440d 100644
--- a/neo/Consensus/Helper.cs
+++ b/neo/Consensus/Helper.cs
@@ -47,6 +47,8 @@ public static bool ViewChanging(this IConsensusContext context)
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static bool MoreThanFNodesCommitted(this IConsensusContext context) => context.CommitPayloads.Count(p => p != null) > context.F();
 
+        public static bool MoreThanFNodesPrepared(this IConsensusContext context) => context.PreparationPayloads.Count(p => p != null) > context.F();
+
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static uint GetPrimaryIndex(this IConsensusContext context, byte viewNumber)
         {
diff --git a/neo/Consensus/RecoveryMessage.cs b/neo/Consensus/RecoveryMessage.cs
index c1d41827aa..801a21a578 100644
--- a/neo/Consensus/RecoveryMessage.cs
+++ b/neo/Consensus/RecoveryMessage.cs
@@ -100,7 +100,7 @@ public ConsensusPayload[] GetCommitPayloadsFromRecoveryMessage(IConsensusContext
         public ConsensusPayload GetPrepareRequestPayload(IConsensusContext context, ConsensusPayload payload)
         {
             if (PrepareRequestMessage == null) return null;
-            if (!PreparationMessages.TryGetValue((int)context.PrimaryIndex, out RecoveryMessage.PreparationPayloadCompact compact))
+            if (!PreparationMessages.TryGetValue((int)context.PrimaryIndex, out PreparationPayloadCompact compact))
                 return null;
             return new ConsensusPayload
             {

From b4ded679175050c515aee39f4daeaed41797f9fd Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 15:07:33 -0700
Subject: [PATCH 10/21] Adjust logic for inhibiting view changing since now
 locking view change is inhibitted if a validator has sent its preparation.

---
 neo/Consensus/ConsensusService.cs |  4 ++--
 neo/Consensus/Helper.cs           | 10 +---------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index aacdd7bf8b..2ee972cdda 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -125,7 +125,7 @@ private void CheckExpectedView(byte viewNumber)
             if (!context.WatchOnly())
             {
                 ChangeView message = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
-                if (!context.CommitSent() && (message is null || message.NewViewNumber < viewNumber || !message.Locked))
+                if (!context.CommitSent() && !context.ResponseSent() && !context.IsPrimary() && (message is null || message.NewViewNumber < viewNumber || !message.Locked))
                     localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
             }
 
@@ -220,7 +220,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
                 if (!shouldRecoverCommitInSameView && !(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
-                Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}");
+                Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber} for index={payload.ValidatorIndex}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });
             }
 
diff --git a/neo/Consensus/Helper.cs b/neo/Consensus/Helper.cs
index bbe1aa440d..cb1ef7eba7 100644
--- a/neo/Consensus/Helper.cs
+++ b/neo/Consensus/Helper.cs
@@ -32,21 +32,13 @@ internal static class Helper
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static bool ViewChanging(this IConsensusContext context)
         {
-            if (context.WatchOnly() || context.MoreThanFNodesCommitted()) return false;
+            if (context.WatchOnly() || context.MoreThanFNodesPrepared()) return false;
             var myChangeViewMessage = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
             if (myChangeViewMessage == null) return false;
 
             return myChangeViewMessage.Locked && myChangeViewMessage.NewViewNumber > context.ViewNumber;
         }
 
-        /// <summary>
-        /// More than F nodes committed in current view.
-        /// </summary>
-        /// <param name="context">consensus context</param>
-        /// <returns>true if more than F nodes have committed in the current view.</returns>
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public static bool MoreThanFNodesCommitted(this IConsensusContext context) => context.CommitPayloads.Count(p => p != null) > context.F();
-
         public static bool MoreThanFNodesPrepared(this IConsensusContext context) => context.PreparationPayloads.Count(p => p != null) > context.F();
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]

From 900baacc4be8250bd1aab5e0101ad3650b9d75e0 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 15:11:26 -0700
Subject: [PATCH 11/21] Simplify.

---
 neo/Consensus/ConsensusService.cs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 2ee972cdda..d3acd933d9 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -125,7 +125,7 @@ private void CheckExpectedView(byte viewNumber)
             if (!context.WatchOnly())
             {
                 ChangeView message = context.ChangeViewPayloads[context.MyIndex]?.GetDeserializedMessage<ChangeView>();
-                if (!context.CommitSent() && !context.ResponseSent() && !context.IsPrimary() && (message is null || message.NewViewNumber < viewNumber || !message.Locked))
+                if (!context.ResponseSent() && !context.IsPrimary() && (message is null || message.NewViewNumber < viewNumber || !message.Locked))
                     localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
             }
 

From 9b59e5c7db18f67d1ddf508840e29ee13a486a1f Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 16:00:24 -0700
Subject: [PATCH 12/21] Make all respond for now for
 shouldRecoverPrepareResponseInSameView.

---
 neo/Consensus/ConsensusService.cs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index d3acd933d9..420fbb93c2 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -218,7 +218,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 if (context.WatchOnly()) return;
 
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
-                if (!shouldRecoverCommitInSameView && !(context.IsPrimary() && shouldRecoverPrepareResponseInSameView) && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
+                if (!shouldRecoverCommitInSameView && !shouldRecoverPrepareResponseInSameView && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber} for index={payload.ValidatorIndex}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });

From da3369345c7c014769b7aad080a1fbc67b77d4af Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 16:31:18 -0700
Subject: [PATCH 13/21] Minor naming refactor.

---
 neo/Consensus/ConsensusService.cs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 420fbb93c2..33894350bd 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -208,17 +208,17 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
             bool inSameViewAndUnlocked = message.ViewNumber == context.ViewNumber && (!message.Locked || context.MoreThanFNodesPrepared());
-            bool shouldRecoverPrepareResponseInSameView = inSameViewAndUnlocked
+            bool shouldRecoverPreparationsInSameView = inSameViewAndUnlocked
                  && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
-            bool shouldRecoverCommitInSameView = inSameViewAndUnlocked
+            bool shouldRecoverCommitsInSameView = inSameViewAndUnlocked
                 && context.CommitSent() && context.CommitPayloads[payload.ValidatorIndex] == null;
             if (message.NewViewNumber <= context.ViewNumber || message.ViewNumber < context.ViewNumber ||
-                shouldRecoverPrepareResponseInSameView || shouldRecoverCommitInSameView)
+                shouldRecoverPreparationsInSameView || shouldRecoverCommitsInSameView)
             {
                 if (context.WatchOnly()) return;
 
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
-                if (!shouldRecoverCommitInSameView && !shouldRecoverPrepareResponseInSameView && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
+                if (!shouldRecoverCommitsInSameView && !shouldRecoverPreparationsInSameView && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber} for index={payload.ValidatorIndex}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });

From e54638c97b170a18417511fedcdb305c59cf54bc Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 16:36:38 -0700
Subject: [PATCH 14/21] Move logic for whether to send recovery from
 OnChangeView into a method for clarity.

---
 neo/Consensus/ConsensusService.cs | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 33894350bd..369355e2fa 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -198,20 +198,13 @@ private bool IsRecoveryAllowed(ushort validatorIndex, byte startingValidatorOffs
             return false;
         }
 
-        private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
+        private void SendRecoveryIfNecessary(ConsensusPayload payload, ChangeView message)
         {
-            // We keep track of the payload hashes received in this block, and don't respond with recovery
-            // in response to the same payload that we already responded to previously.
-            // ChangeView messages include a Timestamp when the change view is sent, thus if a node restarts
-            // and issues a change view for the same view, it will have a different hash and will correctly respond
-            // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
-            // additional recovery message response.
-            if (!knownHashes.Add(payload.Hash)) return;
             bool inSameViewAndUnlocked = message.ViewNumber == context.ViewNumber && (!message.Locked || context.MoreThanFNodesPrepared());
             bool shouldRecoverPreparationsInSameView = inSameViewAndUnlocked
-                 && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
+                                                       && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
             bool shouldRecoverCommitsInSameView = inSameViewAndUnlocked
-                && context.CommitSent() && context.CommitPayloads[payload.ValidatorIndex] == null;
+                                                  && context.CommitSent() && context.CommitPayloads[payload.ValidatorIndex] == null;
             if (message.NewViewNumber <= context.ViewNumber || message.ViewNumber < context.ViewNumber ||
                 shouldRecoverPreparationsInSameView || shouldRecoverCommitsInSameView)
             {
@@ -223,6 +216,18 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber} for index={payload.ValidatorIndex}");
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() });
             }
+        }
+
+        private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
+        {
+            // We keep track of the payload hashes received in this block, and don't respond with recovery
+            // in response to the same payload that we already responded to previously.
+            // ChangeView messages include a Timestamp when the change view is sent, thus if a node restarts
+            // and issues a change view for the same view, it will have a different hash and will correctly respond
+            // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
+            // additional recovery message response.
+            if (!knownHashes.Add(payload.Hash)) return;
+            SendRecoveryIfNecessary(payload, message);
 
             var lastChangeViewMessage = context.ChangeViewPayloads[payload.ValidatorIndex]?.GetDeserializedMessage<ChangeView>();
             // If we have had a change view message from this validator after our current view.

From 38f15507c7484f477c75d497b4e1bb30d6bd42df Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 16:49:21 -0700
Subject: [PATCH 15/21] Clean-up.

---
 neo/Consensus/ConsensusService.cs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 369355e2fa..fd1c10c31f 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -205,11 +205,10 @@ private void SendRecoveryIfNecessary(ConsensusPayload payload, ChangeView messag
                                                        && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
             bool shouldRecoverCommitsInSameView = inSameViewAndUnlocked
                                                   && context.CommitSent() && context.CommitPayloads[payload.ValidatorIndex] == null;
-            if (message.NewViewNumber <= context.ViewNumber || message.ViewNumber < context.ViewNumber ||
-                shouldRecoverPreparationsInSameView || shouldRecoverCommitsInSameView)
+            bool requestingFromLowerView = message.ViewNumber < context.ViewNumber;
+            bool requestingTheCurrentView = message.NewViewNumber == context.ViewNumber;
+            if ( requestingFromLowerView || requestingTheCurrentView || shouldRecoverPreparationsInSameView || shouldRecoverCommitsInSameView)
             {
-                if (context.WatchOnly()) return;
-
                 // Limit recovery to sending from `f` nodes when the request is from a lower view number.
                 if (!shouldRecoverCommitsInSameView && !shouldRecoverPreparationsInSameView && !IsRecoveryAllowed(payload.ValidatorIndex, message.NewViewNumber, context.F())) return;
 
@@ -227,7 +226,8 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
             // again; however replay attacks of the ChangeView message from arbitrary nodes will not trigger an
             // additional recovery message response.
             if (!knownHashes.Add(payload.Hash)) return;
-            SendRecoveryIfNecessary(payload, message);
+            if (!context.WatchOnly())
+                SendRecoveryIfNecessary(payload, message);
 
             var lastChangeViewMessage = context.ChangeViewPayloads[payload.ValidatorIndex]?.GetDeserializedMessage<ChangeView>();
             // If we have had a change view message from this validator after our current view.

From 42a5832d0d1b10b9e0dabb0ea5951a61d13bcc4c Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 17:49:46 -0700
Subject: [PATCH 16/21] Added needed special logic to handle recovering while
 the view is changing.

---
 neo/Consensus/ConsensusService.cs | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index fd1c10c31f..547f35ceb9 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -343,7 +343,31 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage
                         if (ReverifyAndProcessPayload(changeViewPayload)) validChangeViews++;
                 }
                 if (message.ViewNumber != context.ViewNumber) return;
-                if (!context.ViewChanging() && !context.CommitSent())
+
+                bool overrideViewChangingCheckDueToMoreThanFNodesPrepared = false;
+                if (context.ViewChanging() && !context.CommitSent())
+                {
+                    int validPreparations = context.PreparationPayloads.Count(p => p != null);
+                    // If we don't already have the prepare request, we need to potentially accept it
+                    if (!context.RequestSentOrReceived())
+                    {
+                        ConsensusPayload prepareRequestPayload = message.GetPrepareRequestPayload(context, payload);
+                        if (prepareRequestPayload != null && prepareRequestPayload.Verify(context.Snapshot))
+                            validPreparations++; // NOTE: may want to add more validation that PrepReq here is valid.
+                    }
+                    ConsensusPayload[] prepareResponsePayloads = message.GetPrepareResponsePayloads(context, payload);
+                    totalPrepResponses = prepareResponsePayloads.Length;
+                    foreach (ConsensusPayload prepareResponsePayload in prepareResponsePayloads)
+                    {
+                        if (context.PreparationPayloads[prepareResponsePayload.ValidatorIndex] != null) continue;
+                        if (prepareResponsePayload.Verify(context.Snapshot))
+                            validPreparations++;
+                    }
+
+                    if (validPreparations >= context.F())
+                        overrideViewChangingCheckDueToMoreThanFNodesPrepared = true;
+                }
+                if (!context.CommitSent() && (overrideViewChangingCheckDueToMoreThanFNodesPrepared || !context.ViewChanging()) )
                 {
                     if (!context.RequestSentOrReceived())
                     {
@@ -379,7 +403,7 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage
 
         private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest message)
         {
-            if (context.RequestSentOrReceived() || context.ViewChanging()) return;
+            if (context.RequestSentOrReceived() || (!isRecovering && context.ViewChanging())) return;
             if (payload.ValidatorIndex != context.PrimaryIndex) return;
             Log($"{nameof(OnPrepareRequestReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex} tx={message.TransactionHashes.Length}");
             if (message.Timestamp <= context.PrevHeader().Timestamp || message.Timestamp > TimeProvider.Current.UtcNow.AddMinutes(10).ToTimestamp())
@@ -438,7 +462,7 @@ private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest m
 
         private void OnPrepareResponseReceived(ConsensusPayload payload, PrepareResponse message)
         {
-            if (context.PreparationPayloads[payload.ValidatorIndex] != null || context.ViewChanging()) return;
+            if (context.PreparationPayloads[payload.ValidatorIndex] != null || (!isRecovering && context.ViewChanging())) return;
             if (context.PreparationPayloads[context.PrimaryIndex] != null && !message.PreparationHash.Equals(context.PreparationPayloads[context.PrimaryIndex].Hash))
                 return;
             Log($"{nameof(OnPrepareResponseReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex}");

From e7e67d5bb49ee65cb55ada85615dac9a0ebd62c4 Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 19:15:28 -0700
Subject: [PATCH 17/21] Fix threshold for valid preparations and sending
 prepare response from recovery.

---
 neo/Consensus/ConsensusService.cs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 547f35ceb9..b94a2d5b78 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -81,7 +81,9 @@ private bool AddTransaction(Transaction tx, bool verify)
                 {
                     // If our view is already changing we cannot send a prepare response, since we can never send both
                     // a change view and a commit message for the same view without potentially forking the network.
-                    if (context.ViewChanging()) return true;
+                    // When recovering though, in the case we have a block containing only the miner transaction,
+                    // this check is made during the recovery code that handles receiving the prepare request.
+                    if (context.ViewChanging() && !isRecovering) return true;
 
                     // if we are the primary for this view, but acting as a backup because we recovered our own
                     // previously sent prepare request, then we don't want to send a prepare response.
@@ -364,7 +366,7 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage
                             validPreparations++;
                     }
 
-                    if (validPreparations >= context.F())
+                    if (validPreparations > context.F())
                         overrideViewChangingCheckDueToMoreThanFNodesPrepared = true;
                 }
                 if (!context.CommitSent() && (overrideViewChangingCheckDueToMoreThanFNodesPrepared || !context.ViewChanging()) )

From 0c194e983d03f578078121520cb381ed36dd943e Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Sun, 24 Mar 2019 21:02:30 -0700
Subject: [PATCH 18/21] Fix calculating valid preparations during recovery
 while changing view.

---
 neo/Consensus/ConsensusService.cs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index b94a2d5b78..c6b9255721 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -355,8 +355,14 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage
                     {
                         ConsensusPayload prepareRequestPayload = message.GetPrepareRequestPayload(context, payload);
                         if (prepareRequestPayload != null && prepareRequestPayload.Verify(context.Snapshot))
+                        {
                             validPreparations++; // NOTE: may want to add more validation that PrepReq here is valid.
+                            // Must set the preparation hash to be able ot get the prepare response payloads, since we are
+                            // not actually processing the message here.
+                            message.PreparationHash = prepareRequestPayload.Hash;
+                        }
                     }
+
                     ConsensusPayload[] prepareResponsePayloads = message.GetPrepareResponsePayloads(context, payload);
                     totalPrepResponses = prepareResponsePayloads.Length;
                     foreach (ConsensusPayload prepareResponsePayload in prepareResponsePayloads)

From f75e037f1ae912bfa74be6a1fd096b669605881b Mon Sep 17 00:00:00 2001
From: jsolman <jsolinsky@gmail.com>
Date: Mon, 25 Mar 2019 05:22:54 -0700
Subject: [PATCH 19/21] Primary doesn't send requests to change view.

---
 neo/Consensus/ConsensusService.cs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index c6b9255721..4c3aaec473 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -593,7 +593,8 @@ private void RequestChangeView()
             expectedView++;
             Log($"request change view: height={context.BlockIndex} view={context.ViewNumber} nv={expectedView}");
             ChangeTimer(TimeSpan.FromSeconds(Blockchain.SecondsPerBlock << (expectedView + 1)));
-            localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(expectedView) });
+            if (!context.IsPrimary())
+                localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(expectedView) });
             CheckExpectedView(expectedView);
         }
 

From 23586db6c3086dc7a5ac9a75d9d8bc83e390b44d Mon Sep 17 00:00:00 2001
From: Vitor <vncoelho@gmail.com>
Date: Mon, 25 Mar 2019 10:02:02 -0300
Subject: [PATCH 20/21] Adding some comments and notes

---
 neo/Consensus/ConsensusContext.cs | 2 ++
 neo/Consensus/ConsensusService.cs | 9 +++++++++
 2 files changed, 11 insertions(+)

diff --git a/neo/Consensus/ConsensusContext.cs b/neo/Consensus/ConsensusContext.cs
index 26085691e4..468e5a6a1e 100644
--- a/neo/Consensus/ConsensusContext.cs
+++ b/neo/Consensus/ConsensusContext.cs
@@ -144,6 +144,8 @@ public ConsensusPayload MakeChangeView(byte newViewNumber)
             {
                 NewViewNumber = newViewNumber,
                 Timestamp = TimeProvider.Current.UtcNow.ToTimestamp(),
+                // Primary will just change view after seing M guys locked to change view, as well as those who agreed with the current PrepReq
+                // NOTE: This may reduce liveness of dBFT, because the remainder locked nodes will be responsible for changing view by themselves
                 Locked = changeViewLockedCount >= this.M() && !(this.ResponseSent() || this.IsPrimary())
             });
         }
diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs
index 4c3aaec473..a7f31e4c6f 100644
--- a/neo/Consensus/ConsensusService.cs
+++ b/neo/Consensus/ConsensusService.cs
@@ -131,6 +131,8 @@ private void CheckExpectedView(byte viewNumber)
                     localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(viewNumber) });
             }
 
+            // We need, at least, M locked nodes for changing view
+            // Primary and all the have `ResponseSent` will not contribute
             if (context.ChangeViewPayloads
                 .Count(p =>
                     {
@@ -202,12 +204,16 @@ private bool IsRecoveryAllowed(ushort validatorIndex, byte startingValidatorOffs
 
         private void SendRecoveryIfNecessary(ConsensusPayload payload, ChangeView message)
         {
+            // Primary is always unlocked, as well as those who agreed with Primary PrepReq
+            // A node can be locked, but if it sees that, at least, `F+1` nodes are Prepared, it will accept the possibility of entering commit phase
             bool inSameViewAndUnlocked = message.ViewNumber == context.ViewNumber && (!message.Locked || context.MoreThanFNodesPrepared());
             bool shouldRecoverPreparationsInSameView = inSameViewAndUnlocked
                                                        && (context.ResponseSent() || context.IsPrimary()) && context.PreparationPayloads[payload.ValidatorIndex] == null;
             bool shouldRecoverCommitsInSameView = inSameViewAndUnlocked
                                                   && context.CommitSent() && context.CommitPayloads[payload.ValidatorIndex] == null;
+            // The node is asking to move to a view that we are already known
             bool requestingFromLowerView = message.ViewNumber < context.ViewNumber;
+            // Node is requesting from a lower view that you already have conditions to proof why you are in
             bool requestingTheCurrentView = message.NewViewNumber == context.ViewNumber;
             if ( requestingFromLowerView || requestingTheCurrentView || shouldRecoverPreparationsInSameView || shouldRecoverCommitsInSameView)
             {
@@ -238,6 +244,7 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message)
                 if (message.NewViewNumber < context.ViewNumber) return;
 
                 if (lastChangeViewMessage.Locked && !message.Locked) return;
+                // If both are locked, you just accept the other that moves you to a higher view
                 if (lastChangeViewMessage.Locked == message.Locked && message.NewViewNumber <= lastChangeViewMessage.NewViewNumber)
                     return;
             }
@@ -593,6 +600,8 @@ private void RequestChangeView()
             expectedView++;
             Log($"request change view: height={context.BlockIndex} view={context.ViewNumber} nv={expectedView}");
             ChangeTimer(TimeSpan.FromSeconds(Blockchain.SecondsPerBlock << (expectedView + 1)));
+            // Primary will not contribute to changeview, but just accept a viewchanging conditions
+            // NOTE: This may reduce liveness of dBFT, because the remainder will be responsible for changing view by themselves
             if (!context.IsPrimary())
                 localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(expectedView) });
             CheckExpectedView(expectedView);

From 67d53f08fda94f6243537a92a0f86ba51310aa87 Mon Sep 17 00:00:00 2001
From: Vitor <vncoelho@gmail.com>
Date: Mon, 25 Mar 2019 10:54:42 -0300
Subject: [PATCH 21/21] Updating notes

---
 neo/Consensus/ConsensusContext.cs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/neo/Consensus/ConsensusContext.cs b/neo/Consensus/ConsensusContext.cs
index 468e5a6a1e..588ed31782 100644
--- a/neo/Consensus/ConsensusContext.cs
+++ b/neo/Consensus/ConsensusContext.cs
@@ -145,7 +145,6 @@ public ConsensusPayload MakeChangeView(byte newViewNumber)
                 NewViewNumber = newViewNumber,
                 Timestamp = TimeProvider.Current.UtcNow.ToTimestamp(),
                 // Primary will just change view after seing M guys locked to change view, as well as those who agreed with the current PrepReq
-                // NOTE: This may reduce liveness of dBFT, because the remainder locked nodes will be responsible for changing view by themselves
                 Locked = changeViewLockedCount >= this.M() && !(this.ResponseSent() || this.IsPrimary())
             });
         }