diff --git a/neo/Consensus/ConsensusContext.cs b/neo/Consensus/ConsensusContext.cs index b7562898dc..622b71b621 100644 --- a/neo/Consensus/ConsensusContext.cs +++ b/neo/Consensus/ConsensusContext.cs @@ -61,7 +61,7 @@ public Block CreateBlock() ContractParametersContext sc = new ContractParametersContext(Block); for (int i = 0, j = 0; i < Validators.Length && j < this.M(); i++) { - if (CommitPayloads[i] == null) continue; + if (CommitPayloads[i]?.ConsensusMessage.ViewNumber != ViewNumber) continue; sc.AddSignature(contract, Validators[i], CommitPayloads[i].GetDeserializedMessage().Signature); j++; } @@ -146,12 +146,10 @@ public ConsensusPayload MakeChangeView(byte newViewNumber) 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; @@ -267,6 +265,7 @@ public void Reset(byte viewNumber) MyIndex = -1; ChangeViewPayloads = new ConsensusPayload[Validators.Length]; LastChangeViewPayloads = new ConsensusPayload[Validators.Length]; + CommitPayloads = new ConsensusPayload[Validators.Length]; keyPair = null; for (int i = 0; i < Validators.Length; i++) { @@ -290,7 +289,6 @@ public void Reset(byte viewNumber) Timestamp = 0; TransactionHashes = null; PreparationPayloads = new ConsensusPayload[Validators.Length]; - CommitPayloads = new ConsensusPayload[Validators.Length]; _header = null; } diff --git a/neo/Consensus/ConsensusService.cs b/neo/Consensus/ConsensusService.cs index ab4f6e26f0..130587e9a0 100644 --- a/neo/Consensus/ConsensusService.cs +++ b/neo/Consensus/ConsensusService.cs @@ -99,7 +99,7 @@ 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))) + if (context.CommitPayloads.Count(p => p?.ConsensusMessage.ViewNumber == context.ViewNumber) >= context.M() && context.TransactionHashes.All(p => context.Transactions.ContainsKey(p))) { Block block = context.CreateBlock(); Log($"relay block: {block.Hash}"); @@ -185,26 +185,28 @@ 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++) + if (!context.CommitSent()) { - 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; + 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; + } Log($"send recovery from view: {message.ViewNumber} to view: {context.ViewNumber}"); localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeRecoveryMessage() }); } @@ -222,18 +224,34 @@ private void OnChangeViewReceived(ConsensusPayload payload, ChangeView message) private void OnCommitReceived(ConsensusPayload payload, Commit commit) { - if (context.CommitPayloads[payload.ValidatorIndex] != null) return; - Log($"{nameof(OnCommitReceived)}: height={payload.BlockIndex} view={commit.ViewNumber} index={payload.ValidatorIndex}"); - byte[] hashData = context.MakeHeader()?.GetHashData(); - if (hashData == null) + ref ConsensusPayload existingCommitPayload = ref context.CommitPayloads[payload.ValidatorIndex]; + if (existingCommitPayload != null) { - context.CommitPayloads[payload.ValidatorIndex] = payload; + if (existingCommitPayload.Hash != payload.Hash) + Log($"{nameof(OnCommitReceived)}: different commit from validator! height={payload.BlockIndex} index={payload.ValidatorIndex} view={commit.ViewNumber} existingView={existingCommitPayload.ConsensusMessage.ViewNumber}", LogLevel.Warning); + return; } - else if (Crypto.Default.VerifySignature(hashData, commit.Signature, context.Validators[payload.ValidatorIndex].EncodePoint(false))) + + if (commit.ViewNumber == context.ViewNumber) { - context.CommitPayloads[payload.ValidatorIndex] = payload; - CheckCommits(); + Log($"{nameof(OnCommitReceived)}: height={payload.BlockIndex} view={commit.ViewNumber} index={payload.ValidatorIndex}"); + + byte[] hashData = context.MakeHeader()?.GetHashData(); + if (hashData == null) + { + existingCommitPayload = payload; + } + else if (Crypto.Default.VerifySignature(hashData, commit.Signature, + context.Validators[payload.ValidatorIndex].EncodePoint(false))) + { + existingCommitPayload = payload; + CheckCommits(); + } + return; } + // Receiving commit from another view + Log($"{nameof(OnCommitReceived)}: record commit for different view={commit.ViewNumber} index={payload.ValidatorIndex} height={payload.BlockIndex}"); + existingCommitPayload = payload; } private void OnConsensusPayload(ConsensusPayload payload) @@ -249,14 +267,10 @@ private void OnConsensusPayload(ConsensusPayload payload) return; } if (payload.ValidatorIndex >= context.Validators.Length) return; - ConsensusMessage message = payload.ConsensusMessage; - if (message.ViewNumber != context.ViewNumber && message.Type != ConsensusMessageType.ChangeView && - message.Type != ConsensusMessageType.RecoveryMessage) - return; foreach (IP2PPlugin plugin in Plugin.P2PPlugins) if (!plugin.OnConsensusMessage(payload)) return; - switch (message) + switch (payload.ConsensusMessage) { case ChangeView view: OnChangeViewReceived(payload, view); @@ -286,13 +300,12 @@ private void OnPersistCompleted(Block block) private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage message) { - if (message.ViewNumber < context.ViewNumber) return; - Log($"{nameof(OnRecoveryMessageReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex}"); // isRecovering is always set to false again after OnRecoveryMessageReceived isRecovering = true; int validChangeViews = 0, totalChangeViews = 0, validPrepReq = 0, totalPrepReq = 0; int validPrepResponses = 0, totalPrepResponses = 0, validCommits = 0, totalCommits = 0; + Log($"{nameof(OnRecoveryMessageReceived)}: height={payload.BlockIndex} view={message.ViewNumber} index={payload.ValidatorIndex}"); try { if (message.ViewNumber > context.ViewNumber) @@ -303,8 +316,7 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage foreach (ConsensusPayload changeViewPayload in changeViewPayloads) if (ReverifyAndProcessPayload(changeViewPayload)) validChangeViews++; } - if (message.ViewNumber != context.ViewNumber) return; - if (!context.CommitSent()) + if (message.ViewNumber == context.ViewNumber && !context.NotAcceptingPayloadsDueToViewChanging() && !context.CommitSent()) { if (!context.RequestSentOrReceived()) { @@ -322,10 +334,14 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage foreach (ConsensusPayload prepareResponsePayload in prepareResponsePayloads) if (ReverifyAndProcessPayload(prepareResponsePayload)) validPrepResponses++; } - ConsensusPayload[] commitPayloads = message.GetCommitPayloadsFromRecoveryMessage(context, payload); - totalCommits = commitPayloads.Length; - foreach (ConsensusPayload commitPayload in commitPayloads) - if (ReverifyAndProcessPayload(commitPayload)) validCommits++; + if (message.ViewNumber <= context.ViewNumber) + { + // Ensure we know about all commits from lower view numbers. + ConsensusPayload[] commitPayloads = message.GetCommitPayloadsFromRecoveryMessage(context, payload); + totalCommits = commitPayloads.Length; + foreach (ConsensusPayload commitPayload in commitPayloads) + if (ReverifyAndProcessPayload(commitPayload)) validCommits++; + } } finally { @@ -340,8 +356,8 @@ private void OnRecoveryMessageReceived(ConsensusPayload payload, RecoveryMessage private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest message) { - if (context.RequestSentOrReceived()) return; - if (payload.ValidatorIndex != context.PrimaryIndex) return; + if (context.RequestSentOrReceived() || context.NotAcceptingPayloadsDueToViewChanging()) return; + if (payload.ValidatorIndex != context.PrimaryIndex || message.ViewNumber != context.ViewNumber) 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()) { @@ -365,7 +381,7 @@ private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest m context.PreparationPayloads[payload.ValidatorIndex] = payload; byte[] hashData = context.MakeHeader().GetHashData(); for (int i = 0; i < context.CommitPayloads.Length; i++) - if (context.CommitPayloads[i] != null) + if (context.CommitPayloads[i]?.ConsensusMessage.ViewNumber == context.ViewNumber) if (!Crypto.Default.VerifySignature(hashData, context.CommitPayloads[i].GetDeserializedMessage().Signature, context.Validators[i].EncodePoint(false))) context.CommitPayloads[i] = null; Dictionary mempoolVerified = Blockchain.Singleton.MemPool.GetVerifiedTransactions().ToDictionary(p => p.Hash); @@ -399,7 +415,8 @@ private void OnPrepareRequestReceived(ConsensusPayload payload, PrepareRequest m private void OnPrepareResponseReceived(ConsensusPayload payload, PrepareResponse message) { - if (context.PreparationPayloads[payload.ValidatorIndex] != null) return; + if (message.ViewNumber != context.ViewNumber) return; + if (context.PreparationPayloads[payload.ValidatorIndex] != null || context.NotAcceptingPayloadsDueToViewChanging()) 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}"); @@ -493,12 +510,8 @@ private void OnTimer(Timer timer) private void OnTransaction(Transaction transaction) { if (transaction.Type == TransactionType.MinerTransaction) return; - if (!context.IsBackup() || !context.RequestSentOrReceived() || context.ResponseSent() || context.BlockSent()) + if (!context.IsBackup() || context.NotAcceptingPayloadsDueToViewChanging() || !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..40f55cecbe 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; @@ -28,9 +29,19 @@ internal static class Helper public static bool CommitSent(this IConsensusContext context) => !context.WatchOnly() && context.CommitPayloads[context.MyIndex] != null; [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().NewViewNumber > context.ViewNumber; + public static bool NotAcceptingPayloadsDueToViewChanging(this IConsensusContext context) => context.ViewChanging() && !context.MoreThanFNodesCommitted(); + + // 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 + // potentially splitting nodes among views and stalling the network. + 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.CommitPayloadCompact.cs b/neo/Consensus/RecoveryMessage.CommitPayloadCompact.cs index 58dd0e34e5..d9347b71bc 100644 --- a/neo/Consensus/RecoveryMessage.CommitPayloadCompact.cs +++ b/neo/Consensus/RecoveryMessage.CommitPayloadCompact.cs @@ -8,17 +8,20 @@ partial class RecoveryMessage { public class CommitPayloadCompact : ISerializable { + public byte ViewNumber; public ushort ValidatorIndex; public byte[] Signature; public byte[] InvocationScript; int ISerializable.Size => + sizeof(byte) + //ViewNumber sizeof(ushort) + //ValidatorIndex Signature.Length + //Signature InvocationScript.GetVarSize(); //InvocationScript void ISerializable.Deserialize(BinaryReader reader) { + ViewNumber = reader.ReadByte(); ValidatorIndex = reader.ReadUInt16(); Signature = reader.ReadBytes(64); InvocationScript = reader.ReadVarBytes(1024); @@ -29,6 +32,7 @@ public static CommitPayloadCompact FromPayload(ConsensusPayload payload) Commit message = payload.GetDeserializedMessage(); return new CommitPayloadCompact { + ViewNumber = message.ViewNumber, ValidatorIndex = payload.ValidatorIndex, Signature = message.Signature, InvocationScript = payload.Witness.InvocationScript @@ -37,6 +41,7 @@ public static CommitPayloadCompact FromPayload(ConsensusPayload payload) void ISerializable.Serialize(BinaryWriter writer) { + writer.Write(ViewNumber); writer.Write(ValidatorIndex); writer.Write(Signature); writer.WriteVarBytes(InvocationScript); diff --git a/neo/Consensus/RecoveryMessage.cs b/neo/Consensus/RecoveryMessage.cs index 64e05233fe..04bc7de407 100644 --- a/neo/Consensus/RecoveryMessage.cs +++ b/neo/Consensus/RecoveryMessage.cs @@ -71,7 +71,7 @@ public ConsensusPayload[] GetCommitPayloadsFromRecoveryMessage(IConsensusContext ValidatorIndex = p.ValidatorIndex, ConsensusMessage = new Commit { - ViewNumber = ViewNumber, + ViewNumber = p.ViewNumber, Signature = p.Signature }, Witness = new Witness