From a1ae1639edafea48698cdf0e9a5bd7b4e0af5411 Mon Sep 17 00:00:00 2001 From: Shargon Date: Wed, 16 Sep 2020 15:22:09 +0200 Subject: [PATCH 1/9] Policy contract's SetMaxTransactionsPerBlock() doesn't check for Block.MaxTransactionsPerBlock --- src/neo/Consensus/ConsensusContext.cs | 2 +- src/neo/SmartContract/Native/PolicyContract.cs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/neo/Consensus/ConsensusContext.cs b/src/neo/Consensus/ConsensusContext.cs index f711268fea..583f63a99c 100644 --- a/src/neo/Consensus/ConsensusContext.cs +++ b/src/neo/Consensus/ConsensusContext.cs @@ -275,7 +275,7 @@ internal int GetExpectedBlockSizeWithoutTransactions(int expectedTransactions) /// Ordered transactions internal void EnsureMaxBlockLimitation(IEnumerable txs) { - uint maxBlockSize = NativeContract.Policy.GetMaxBlockSize(Snapshot); + uint maxBlockSize = Math.Min(Network.P2P.Message.PayloadMaxSize, NativeContract.Policy.GetMaxBlockSize(Snapshot)); long maxBlockSystemFee = NativeContract.Policy.GetMaxBlockSystemFee(Snapshot); uint maxTransactionsPerBlock = NativeContract.Policy.GetMaxTransactionsPerBlock(Snapshot); diff --git a/src/neo/SmartContract/Native/PolicyContract.cs b/src/neo/SmartContract/Native/PolicyContract.cs index 32f90ba89d..a9eadd18c4 100644 --- a/src/neo/SmartContract/Native/PolicyContract.cs +++ b/src/neo/SmartContract/Native/PolicyContract.cs @@ -1,6 +1,8 @@ #pragma warning disable IDE0051 using Neo.Ledger; +using Neo.Network.P2P; +using Neo.Network.P2P.Payloads; using Neo.Persistence; using Neo.SmartContract.Manifest; using System; @@ -87,8 +89,8 @@ public bool IsAnyAccountBlocked(StoreView snapshot, params UInt160[] hashes) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSize(ApplicationEngine engine, uint value) { + if (Message.PayloadMaxSize <= value) return false; if (!CheckCommittee(engine)) return false; - if (Network.P2P.Message.PayloadMaxSize <= value) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSize), () => new StorageItem()); storage.Set(value); return true; @@ -97,6 +99,7 @@ private bool SetMaxBlockSize(ApplicationEngine engine, uint value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxTransactionsPerBlock(ApplicationEngine engine, uint value) { + if (value > Block.MaxTransactionsPerBlock) return false; if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxTransactionsPerBlock), () => new StorageItem()); storage.Set(value); From 5bbeb3835de4761026fccefc230b334dc3f882b5 Mon Sep 17 00:00:00 2001 From: Shargon Date: Wed, 16 Sep 2020 15:24:57 +0200 Subject: [PATCH 2/9] add more --- src/neo/SmartContract/Native/PolicyContract.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/neo/SmartContract/Native/PolicyContract.cs b/src/neo/SmartContract/Native/PolicyContract.cs index a9eadd18c4..338f96cb06 100644 --- a/src/neo/SmartContract/Native/PolicyContract.cs +++ b/src/neo/SmartContract/Native/PolicyContract.cs @@ -89,7 +89,7 @@ public bool IsAnyAccountBlocked(StoreView snapshot, params UInt160[] hashes) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSize(ApplicationEngine engine, uint value) { - if (Message.PayloadMaxSize <= value) return false; + if (value > Message.PayloadMaxSize) return false; if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSize), () => new StorageItem()); storage.Set(value); @@ -109,8 +109,8 @@ private bool SetMaxTransactionsPerBlock(ApplicationEngine engine, uint value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) { - if (!CheckCommittee(engine)) return false; if (value <= 4007600) return false; + if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSystemFee), () => new StorageItem()); storage.Set(value); return true; @@ -119,6 +119,7 @@ private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetFeePerByte(ApplicationEngine engine, long value) { + if (value < 0) return false; if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_FeePerByte), () => new StorageItem()); storage.Set(value); From ec174a2e75cc26f1645a74760b3e15d77983ce4e Mon Sep 17 00:00:00 2001 From: Shargon Date: Wed, 16 Sep 2020 15:25:40 +0200 Subject: [PATCH 3/9] Clean change --- src/neo/Consensus/ConsensusContext.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/neo/Consensus/ConsensusContext.cs b/src/neo/Consensus/ConsensusContext.cs index 583f63a99c..f711268fea 100644 --- a/src/neo/Consensus/ConsensusContext.cs +++ b/src/neo/Consensus/ConsensusContext.cs @@ -275,7 +275,7 @@ internal int GetExpectedBlockSizeWithoutTransactions(int expectedTransactions) /// Ordered transactions internal void EnsureMaxBlockLimitation(IEnumerable txs) { - uint maxBlockSize = Math.Min(Network.P2P.Message.PayloadMaxSize, NativeContract.Policy.GetMaxBlockSize(Snapshot)); + uint maxBlockSize = NativeContract.Policy.GetMaxBlockSize(Snapshot); long maxBlockSystemFee = NativeContract.Policy.GetMaxBlockSystemFee(Snapshot); uint maxTransactionsPerBlock = NativeContract.Policy.GetMaxTransactionsPerBlock(Snapshot); From e42a31e836d0395b62535ba67160e59a807dcc20 Mon Sep 17 00:00:00 2001 From: Shargon Date: Wed, 16 Sep 2020 15:49:50 +0200 Subject: [PATCH 4/9] Fix ut --- tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs b/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs index 9cf22fae78..2c8a629207 100644 --- a/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs +++ b/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs @@ -73,7 +73,7 @@ public void Check_SetMaxBlockSize() // More than expected ret = NativeContract.Policy.Call(snapshot, new Nep5NativeContractExtensions.ManualWitness(committeeMultiSigAddr), - "setMaxBlockSize", new ContractParameter(ContractParameterType.Integer) { Value = Neo.Network.P2P.Message.PayloadMaxSize }); + "setMaxBlockSize", new ContractParameter(ContractParameterType.Integer) { Value = Neo.Network.P2P.Message.PayloadMaxSize + 1 }); ret.Should().BeOfType(); ret.GetBoolean().Should().BeFalse(); From f0206b06d6b9d9d3ceeda82962c5f7e98ea66274 Mon Sep 17 00:00:00 2001 From: Shargon Date: Thu, 17 Sep 2020 10:22:21 +0200 Subject: [PATCH 5/9] throw exceptions --- src/neo/SmartContract/Native/PolicyContract.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/neo/SmartContract/Native/PolicyContract.cs b/src/neo/SmartContract/Native/PolicyContract.cs index 338f96cb06..174439d155 100644 --- a/src/neo/SmartContract/Native/PolicyContract.cs +++ b/src/neo/SmartContract/Native/PolicyContract.cs @@ -89,7 +89,7 @@ public bool IsAnyAccountBlocked(StoreView snapshot, params UInt160[] hashes) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSize(ApplicationEngine engine, uint value) { - if (value > Message.PayloadMaxSize) return false; + if (value > Message.PayloadMaxSize) throw new Exception("value cannot be bigger than PayloadMaxSize"); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSize), () => new StorageItem()); storage.Set(value); @@ -99,7 +99,7 @@ private bool SetMaxBlockSize(ApplicationEngine engine, uint value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxTransactionsPerBlock(ApplicationEngine engine, uint value) { - if (value > Block.MaxTransactionsPerBlock) return false; + if (value > Block.MaxTransactionsPerBlock) throw new Exception("value cannot be bigger than MaxTransactionsPerBlock"); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxTransactionsPerBlock), () => new StorageItem()); storage.Set(value); @@ -109,7 +109,7 @@ private bool SetMaxTransactionsPerBlock(ApplicationEngine engine, uint value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) { - if (value <= 4007600) return false; + if (value <= 4007600) throw new Exception("value cannot be lower or equal to 4007600"); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSystemFee), () => new StorageItem()); storage.Set(value); @@ -119,7 +119,7 @@ private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetFeePerByte(ApplicationEngine engine, long value) { - if (value < 0) return false; + if (value < 0) throw new Exception("value cannot be lower than 0"); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_FeePerByte), () => new StorageItem()); storage.Set(value); From ca81be0865d55bb47ab2a27f8ec5c2552b639a4d Mon Sep 17 00:00:00 2001 From: erikzhang Date: Thu, 17 Sep 2020 22:07:40 +0800 Subject: [PATCH 6/9] ArgumentOutOfRangeException --- src/neo/SmartContract/Native/PolicyContract.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/neo/SmartContract/Native/PolicyContract.cs b/src/neo/SmartContract/Native/PolicyContract.cs index 174439d155..4fd82d5e3f 100644 --- a/src/neo/SmartContract/Native/PolicyContract.cs +++ b/src/neo/SmartContract/Native/PolicyContract.cs @@ -89,7 +89,7 @@ public bool IsAnyAccountBlocked(StoreView snapshot, params UInt160[] hashes) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSize(ApplicationEngine engine, uint value) { - if (value > Message.PayloadMaxSize) throw new Exception("value cannot be bigger than PayloadMaxSize"); + if (value > Message.PayloadMaxSize) throw new ArgumentOutOfRangeException(nameof(value)); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSize), () => new StorageItem()); storage.Set(value); @@ -99,7 +99,7 @@ private bool SetMaxBlockSize(ApplicationEngine engine, uint value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxTransactionsPerBlock(ApplicationEngine engine, uint value) { - if (value > Block.MaxTransactionsPerBlock) throw new Exception("value cannot be bigger than MaxTransactionsPerBlock"); + if (value > Block.MaxTransactionsPerBlock) throw new ArgumentOutOfRangeException(nameof(value)); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxTransactionsPerBlock), () => new StorageItem()); storage.Set(value); @@ -109,7 +109,7 @@ private bool SetMaxTransactionsPerBlock(ApplicationEngine engine, uint value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) { - if (value <= 4007600) throw new Exception("value cannot be lower or equal to 4007600"); + if (value <= 4007600) throw new ArgumentOutOfRangeException(nameof(value)); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_MaxBlockSystemFee), () => new StorageItem()); storage.Set(value); @@ -119,7 +119,7 @@ private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetFeePerByte(ApplicationEngine engine, long value) { - if (value < 0) throw new Exception("value cannot be lower than 0"); + if (value < 0) throw new ArgumentOutOfRangeException(nameof(value)); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_FeePerByte), () => new StorageItem()); storage.Set(value); From 601486c996aa8ccb035ea8afce2b6d60db431d4e Mon Sep 17 00:00:00 2001 From: Shargon Date: Thu, 17 Sep 2020 16:55:17 +0200 Subject: [PATCH 7/9] Update src/neo/SmartContract/Native/PolicyContract.cs Co-authored-by: Erik Zhang --- src/neo/SmartContract/Native/PolicyContract.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/neo/SmartContract/Native/PolicyContract.cs b/src/neo/SmartContract/Native/PolicyContract.cs index 4fd82d5e3f..2376b06439 100644 --- a/src/neo/SmartContract/Native/PolicyContract.cs +++ b/src/neo/SmartContract/Native/PolicyContract.cs @@ -119,7 +119,7 @@ private bool SetMaxBlockSystemFee(ApplicationEngine engine, long value) [ContractMethod(0_03000000, CallFlags.AllowModifyStates)] private bool SetFeePerByte(ApplicationEngine engine, long value) { - if (value < 0) throw new ArgumentOutOfRangeException(nameof(value)); + if (value < 0 || value > 1_00000000) throw new ArgumentOutOfRangeException(nameof(value)); if (!CheckCommittee(engine)) return false; StorageItem storage = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_FeePerByte), () => new StorageItem()); storage.Set(value); From 8f707119cc82b77750d17f79bb38040d69e06ac5 Mon Sep 17 00:00:00 2001 From: Shargon Date: Fri, 18 Sep 2020 11:14:56 +0200 Subject: [PATCH 8/9] Fix UT --- .../Extensions/NativeContractExtensions.cs | 5 ++++- .../SmartContract/Native/UT_PolicyContract.cs | 15 +++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs b/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs index 688e428ec0..5e9dddf0f1 100644 --- a/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs +++ b/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs @@ -33,7 +33,10 @@ public static StackItem Call(this NativeContract contract, StoreView snapshot, I if (engine.Execute() != VMState.HALT) { - throw new InvalidOperationException(); + Exception exception = engine.FaultException; + while (exception?.InnerException != null) exception = exception.InnerException; + + throw exception ?? new InvalidOperationException(); } return engine.ResultStack.Pop(); diff --git a/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs b/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs index 2c8a629207..8d0ea3d1b2 100644 --- a/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs +++ b/tests/neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs @@ -6,6 +6,7 @@ using Neo.SmartContract; using Neo.SmartContract.Native; using Neo.UnitTests.Extensions; +using System; using System.Linq; namespace Neo.UnitTests.SmartContract.Native @@ -72,10 +73,11 @@ public void Check_SetMaxBlockSize() // More than expected - ret = NativeContract.Policy.Call(snapshot, new Nep5NativeContractExtensions.ManualWitness(committeeMultiSigAddr), + Assert.ThrowsException(() => + { + NativeContract.Policy.Call(snapshot, new Nep5NativeContractExtensions.ManualWitness(committeeMultiSigAddr), "setMaxBlockSize", new ContractParameter(ContractParameterType.Integer) { Value = Neo.Network.P2P.Message.PayloadMaxSize + 1 }); - ret.Should().BeOfType(); - ret.GetBoolean().Should().BeFalse(); + }); ret = NativeContract.Policy.Call(snapshot, "getMaxBlockSize"); ret.Should().BeOfType(); @@ -120,10 +122,11 @@ public void Check_SetMaxBlockSystemFee() // Less than expected - ret = NativeContract.Policy.Call(snapshot, new Nep5NativeContractExtensions.ManualWitness(committeeMultiSigAddr), + Assert.ThrowsException(() => + { + NativeContract.Policy.Call(snapshot, new Nep5NativeContractExtensions.ManualWitness(committeeMultiSigAddr), "setMaxBlockSystemFee", new ContractParameter(ContractParameterType.Integer) { Value = -1000 }); - ret.Should().BeOfType(); - ret.GetBoolean().Should().BeFalse(); + }); ret = NativeContract.Policy.Call(snapshot, "getMaxBlockSystemFee"); ret.Should().BeOfType(); From 4887d8c9a29c9d086d975b8cee7dd031f19b9ff9 Mon Sep 17 00:00:00 2001 From: Shargon Date: Fri, 18 Sep 2020 11:15:24 +0200 Subject: [PATCH 9/9] Update NativeContractExtensions.cs --- tests/neo.UnitTests/Extensions/NativeContractExtensions.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs b/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs index 5e9dddf0f1..1981d03c9f 100644 --- a/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs +++ b/tests/neo.UnitTests/Extensions/NativeContractExtensions.cs @@ -35,7 +35,6 @@ public static StackItem Call(this NativeContract contract, StoreView snapshot, I { Exception exception = engine.FaultException; while (exception?.InnerException != null) exception = exception.InnerException; - throw exception ?? new InvalidOperationException(); }