Move cosigners to transaction attributes (neo-project#1620)

* First draft * Remove dictonary * Move json methods * format * Fix UT * Fix Size * clean code * Fix UT * Clean code * Remove const * Remove blank line * Pluralize * Allow multiple attributes of the same type * Update src/neo/Wallets/Wallet.cs * Fix some UT * Fix UT * Fix UT * Update src/neo/Network/P2P/Payloads/CosignerAttribute.cs Co-authored-by: Erik Zhang <[email protected]> * Singular * Move Cosigner to TransactionCollection * Optimize empty * Use ReflectionCache * Change json format * Rename * Remove FromJson * Optimize TransactionAttribute.ToJson() * Rename * Refactor * Change the value of TransactionAttributeType.Cosigner * Fix UTs * Fix attribute order * Remove TransactionAttributeCollection * Reorder * Revert some changes * Revert some changes Co-authored-by: Erik Zhang <[email protected]>
Tommo-L · Jun 22, 2020 · 7b0adda · 7b0adda
1 parent 4050822
commit 7b0adda
Show file tree

Hide file tree

Showing 23 changed files with 138 additions and 156 deletions.
diff --git a/src/neo/Ledger/Blockchain.cs b/src/neo/Ledger/Blockchain.cs
@@ -167,8 +167,7 @@ private static Transaction DeployNativeContracts()
                 Script = script,
                 Sender = (new[] { (byte)OpCode.PUSH1 }).ToScriptHash(),
                 SystemFee = 0,
-                Attributes = new TransactionAttribute[0],
-                Cosigners = new Cosigner[0],
+                Attributes = Array.Empty<TransactionAttribute>(),
                 Witnesses = new[]
                 {
                     new Witness

diff --git a/src/neo/Network/P2P/Payloads/Cosigner.cs b/src/neo/Network/P2P/Payloads/Cosigner.cs
@@ -6,28 +6,30 @@
 
 namespace Neo.Network.P2P.Payloads
 {
-    public class Cosigner : ISerializable
+    public class Cosigner : TransactionAttribute
     {
+        // This limits maximum number of AllowedContracts or AllowedGroups here
+        private const int MaxSubitems = 16;
+
         public UInt160 Account;
         public WitnessScope Scopes;
         public UInt160[] AllowedContracts;
         public ECPoint[] AllowedGroups;
 
+        public override TransactionAttributeType Type => TransactionAttributeType.Cosigner;
+
         public Cosigner()
         {
             this.Scopes = WitnessScope.Global;
         }
 
-        // This limits maximum number of AllowedContracts or AllowedGroups here
-        private int MaxSubitems = 16;
-
-        public int Size =>
+        public override int Size => base.Size +
             /*Account*/             UInt160.Length +
             /*Scopes*/              sizeof(WitnessScope) +
             /*AllowedContracts*/    (Scopes.HasFlag(WitnessScope.CustomContracts) ? AllowedContracts.GetVarSize() : 0) +
             /*AllowedGroups*/       (Scopes.HasFlag(WitnessScope.CustomGroups) ? AllowedGroups.GetVarSize() : 0);
 
-        void ISerializable.Deserialize(BinaryReader reader)
+        protected override void DeserializeWithoutType(BinaryReader reader)
         {
             Account = reader.ReadSerializable<UInt160>();
             Scopes = (WitnessScope)reader.ReadByte();
@@ -39,7 +41,7 @@ void ISerializable.Deserialize(BinaryReader reader)
                 : new ECPoint[0];
         }
 
-        void ISerializable.Serialize(BinaryWriter writer)
+        protected override void SerializeWithoutType(BinaryWriter writer)
         {
             writer.Write(Account);
             writer.Write((byte)Scopes);
@@ -49,9 +51,9 @@ void ISerializable.Serialize(BinaryWriter writer)
                 writer.Write(AllowedGroups);
         }
 
-        public JObject ToJson()
+        public override JObject ToJson()
         {
-            JObject json = new JObject();
+            JObject json = base.ToJson();
             json["account"] = Account.ToString();
             json["scopes"] = Scopes;
             if (Scopes.HasFlag(WitnessScope.CustomContracts))

diff --git a/src/neo/Network/P2P/Payloads/Transaction.cs b/src/neo/Network/P2P/Payloads/Transaction.cs
@@ -24,11 +24,7 @@ public class Transaction : IEquatable<Transaction>, IInventory, IInteroperable
         /// <summary>
         /// Maximum number of attributes that can be contained within a transaction
         /// </summary>
-        private const int MaxTransactionAttributes = 16;
-        /// <summary>
-        /// Maximum number of cosigners that can be contained within a transaction
-        /// </summary>
-        private const int MaxCosigners = 16;
+        public const int MaxTransactionAttributes = 16;
 
         private byte version;
         private uint nonce;
@@ -37,7 +33,6 @@ public class Transaction : IEquatable<Transaction>, IInventory, IInteroperable
         private long netfee;
         private uint validUntilBlock;
         private TransactionAttribute[] attributes;
-        private Cosigner[] cosigners;
         private byte[] script;
         private Witness[] witnesses;
 
@@ -52,14 +47,11 @@ public class Transaction : IEquatable<Transaction>, IInventory, IInteroperable
         public TransactionAttribute[] Attributes
         {
             get => attributes;
-            set { attributes = value; _hash = null; _size = 0; }
+            set { attributes = value; _cosigners = null; _hash = null; _size = 0; }
         }
 
-        public Cosigner[] Cosigners
-        {
-            get => cosigners;
-            set { cosigners = value; _hash = null; _size = 0; }
-        }
+        private Cosigner[] _cosigners;
+        public Cosigner[] Cosigners => _cosigners ??= attributes.OfType<Cosigner>().ToArray();
 
         /// <summary>
         /// The <c>NetworkFee</c> for the transaction divided by its <c>Size</c>.
@@ -117,10 +109,9 @@ public int Size
                 if (_size == 0)
                 {
                     _size = HeaderSize +
-                        Attributes.GetVarSize() +   //Attributes
-                        Cosigners.GetVarSize() +    //Cosigners
-                        Script.GetVarSize() +       //Script
-                        Witnesses.GetVarSize();     //Witnesses
+                        Attributes.GetVarSize() +   // Attributes
+                        Script.GetVarSize() +       // Script
+                        Witnesses.GetVarSize();     // Witnesses
                 }
                 return _size;
             }
@@ -176,8 +167,9 @@ public void DeserializeUnsigned(BinaryReader reader)
             if (NetworkFee < 0) throw new FormatException();
             if (SystemFee + NetworkFee < SystemFee) throw new FormatException();
             ValidUntilBlock = reader.ReadUInt32();
-            Attributes = reader.ReadSerializableArray<TransactionAttribute>(MaxTransactionAttributes);
-            Cosigners = reader.ReadSerializableArray<Cosigner>(MaxCosigners);
+            Attributes = new TransactionAttribute[reader.ReadVarInt(MaxTransactionAttributes)];
+            for (int i = 0; i < Attributes.Length; i++)
+                Attributes[i] = TransactionAttribute.DeserializeFrom(reader);
             if (Cosigners.Select(u => u.Account).Distinct().Count() != Cosigners.Length) throw new FormatException();
             Script = reader.ReadVarBytes(ushort.MaxValue);
             if (Script.Length == 0) throw new FormatException();
@@ -227,7 +219,6 @@ void IVerifiable.SerializeUnsigned(BinaryWriter writer)
             writer.Write(NetworkFee);
             writer.Write(ValidUntilBlock);
             writer.Write(Attributes);
-            writer.Write(Cosigners);
             writer.WriteVarBytes(Script);
         }
 
@@ -243,7 +234,6 @@ public JObject ToJson()
             json["net_fee"] = NetworkFee.ToString();
             json["valid_until_block"] = ValidUntilBlock;
             json["attributes"] = Attributes.Select(p => p.ToJson()).ToArray();
-            json["cosigners"] = Cosigners.Select(p => p.ToJson()).ToArray();
             json["script"] = Convert.ToBase64String(Script);
             json["witnesses"] = Witnesses.Select(p => p.ToJson()).ToArray();
             return json;

diff --git a/src/neo/Network/P2P/Payloads/TransactionAttribute.cs b/src/neo/Network/P2P/Payloads/TransactionAttribute.cs
@@ -1,37 +1,48 @@
 using Neo.IO;
+using Neo.IO.Caching;
 using Neo.IO.Json;
 using System;
 using System.IO;
 
 namespace Neo.Network.P2P.Payloads
 {
-    public class TransactionAttribute : ISerializable
+    public abstract class TransactionAttribute : ISerializable
     {
-        public TransactionAttributeUsage Usage;
-        public byte[] Data;
+        public abstract TransactionAttributeType Type { get; }
+        public virtual int Size => sizeof(TransactionAttributeType);
 
-        public int Size => sizeof(TransactionAttributeUsage) + Data.GetVarSize();
+        public void Deserialize(BinaryReader reader)
+        {
+            if (reader.ReadByte() != (byte)Type)
+                throw new FormatException();
+            DeserializeWithoutType(reader);
+        }
 
-        void ISerializable.Deserialize(BinaryReader reader)
+        public static TransactionAttribute DeserializeFrom(BinaryReader reader)
         {
-            Usage = (TransactionAttributeUsage)reader.ReadByte();
-            if (!Enum.IsDefined(typeof(TransactionAttributeUsage), Usage))
+            TransactionAttributeType type = (TransactionAttributeType)reader.ReadByte();
+            if (!(ReflectionCache<TransactionAttributeType>.CreateInstance(type) is TransactionAttribute attribute))
                 throw new FormatException();
-            Data = reader.ReadVarBytes(252);
+            attribute.DeserializeWithoutType(reader);
+            return attribute;
         }
 
-        void ISerializable.Serialize(BinaryWriter writer)
+        protected abstract void DeserializeWithoutType(BinaryReader reader);
+
+        public virtual JObject ToJson()
         {
-            writer.Write((byte)Usage);
-            writer.WriteVarBytes(Data);
+            return new JObject
+            {
+                ["type"] = Type
+            };
         }
 
-        public JObject ToJson()
+        public void Serialize(BinaryWriter writer)
         {
-            JObject json = new JObject();
-            json["usage"] = Usage;
-            json["data"] = Convert.ToBase64String(Data);
-            return json;
+            writer.Write((byte)Type);
+            SerializeWithoutType(writer);
         }
+
+        protected abstract void SerializeWithoutType(BinaryWriter writer);
     }
 }
diff --git a/src/neo/Network/P2P/Payloads/TransactionAttributeType.cs b/src/neo/Network/P2P/Payloads/TransactionAttributeType.cs
@@ -0,0 +1,10 @@
+using Neo.IO.Caching;
+
+namespace Neo.Network.P2P.Payloads
+{
+    public enum TransactionAttributeType : byte
+    {
+        [ReflectionCache(typeof(Cosigner))]
+        Cosigner = 0x01
+    }
+}
diff --git a/src/neo/Network/P2P/Payloads/TransactionAttributeUsage.cs b/src/neo/Network/P2P/Payloads/TransactionAttributeUsage.cs
diff --git a/src/neo/SmartContract/InteropService.Runtime.cs b/src/neo/SmartContract/InteropService.Runtime.cs
@@ -81,24 +81,24 @@ internal static bool CheckWitnessInternal(ApplicationEngine engine, UInt160 hash
             {
                 if (engine.ScriptContainer is Transaction tx)
                 {
-                    Cosigner usage = tx.Cosigners.FirstOrDefault(p => p.Account.Equals(hash));
-                    if (usage is null) return false;
-                    if (usage.Scopes == WitnessScope.Global) return true;
-                    if (usage.Scopes.HasFlag(WitnessScope.CalledByEntry))
+                    Cosigner cosigner = tx.Cosigners.FirstOrDefault(p => p.Account.Equals(hash));
+                    if (cosigner is null) return false;
+                    if (cosigner.Scopes == WitnessScope.Global) return true;
+                    if (cosigner.Scopes.HasFlag(WitnessScope.CalledByEntry))
                     {
                         if (engine.CallingScriptHash == engine.EntryScriptHash)
                             return true;
                     }
-                    if (usage.Scopes.HasFlag(WitnessScope.CustomContracts))
+                    if (cosigner.Scopes.HasFlag(WitnessScope.CustomContracts))
                     {
-                        if (usage.AllowedContracts.Contains(engine.CurrentScriptHash))
+                        if (cosigner.AllowedContracts.Contains(engine.CurrentScriptHash))
                             return true;
                     }
-                    if (usage.Scopes.HasFlag(WitnessScope.CustomGroups))
+                    if (cosigner.Scopes.HasFlag(WitnessScope.CustomGroups))
                     {
                         var contract = engine.Snapshot.Contracts[engine.CallingScriptHash];
                         // check if current group is the required one
-                        if (contract.Manifest.Groups.Select(p => p.PubKey).Intersect(usage.AllowedGroups).Any())
+                        if (contract.Manifest.Groups.Select(p => p.PubKey).Intersect(cosigner.AllowedGroups).Any())
                             return true;
                     }
                     return false;

diff --git a/src/neo/Wallets/Wallet.cs b/src/neo/Wallets/Wallet.cs
@@ -285,11 +285,11 @@ public Transaction MakeTransaction(TransferOutput[] outputs, UInt160 from = null
                              Account = new UInt160(p.ToArray())
                          }).ToArray();
 
-                return MakeTransaction(snapshot, script, new TransactionAttribute[0], cosigners, balances_gas);
+                return MakeTransaction(snapshot, script, cosigners, balances_gas);
             }
         }
 
-        public Transaction MakeTransaction(byte[] script, UInt160 sender = null, TransactionAttribute[] attributes = null, Cosigner[] cosigners = null)
+        public Transaction MakeTransaction(byte[] script, UInt160 sender = null, TransactionAttribute[] attributes = null)
         {
             UInt160[] accounts;
             if (sender is null)
@@ -305,11 +305,11 @@ public Transaction MakeTransaction(byte[] script, UInt160 sender = null, Transac
             using (SnapshotView snapshot = Blockchain.Singleton.GetSnapshot())
             {
                 var balances_gas = accounts.Select(p => (Account: p, Value: NativeContract.GAS.BalanceOf(snapshot, p))).Where(p => p.Value.Sign > 0).ToList();
-                return MakeTransaction(snapshot, script, attributes ?? new TransactionAttribute[0], cosigners ?? new Cosigner[0], balances_gas);
+                return MakeTransaction(snapshot, script, attributes ?? new TransactionAttribute[0], balances_gas);
             }
         }
 
-        private Transaction MakeTransaction(StoreView snapshot, byte[] script, TransactionAttribute[] attributes, Cosigner[] cosigners, List<(UInt160 Account, BigInteger Value)> balances_gas)
+        private Transaction MakeTransaction(StoreView snapshot, byte[] script, TransactionAttribute[] attributes, List<(UInt160 Account, BigInteger Value)> balances_gas)
         {
             Random rand = new Random();
             foreach (var (account, value) in balances_gas)
@@ -322,8 +322,8 @@ private Transaction MakeTransaction(StoreView snapshot, byte[] script, Transacti
                     Sender = account,
                     ValidUntilBlock = snapshot.Height + Transaction.MaxValidUntilBlockIncrement,
                     Attributes = attributes,
-                    Cosigners = cosigners
                 };
+
                 // will try to execute 'transfer' script to check if it works
                 using (ApplicationEngine engine = ApplicationEngine.Run(script, snapshot.Clone(), tx, testMode: true))
                 {
@@ -334,8 +334,8 @@ private Transaction MakeTransaction(StoreView snapshot, byte[] script, Transacti
 
                 UInt160[] hashes = tx.GetScriptHashesForVerifying(snapshot);
 
-                // base size for transaction: includes const_header + attributes + cosigners with scopes + script + hashes
-                int size = Transaction.HeaderSize + attributes.GetVarSize() + cosigners.GetVarSize() + script.GetVarSize() + IO.Helper.GetVarSize(hashes.Length);
+                // base size for transaction: includes const_header + attributes + script + hashes
+                int size = Transaction.HeaderSize + tx.Attributes.GetVarSize() + script.GetVarSize() + IO.Helper.GetVarSize(hashes.Length);
 
                 foreach (UInt160 hash in hashes)
                 {

diff --git a/tests/neo.UnitTests/Consensus/UT_ConsensusContext.cs b/tests/neo.UnitTests/Consensus/UT_ConsensusContext.cs
@@ -98,8 +98,7 @@ private Transaction CreateTransactionWithSize(int v)
             var r = new Random();
             var tx = new Transaction()
             {
-                Cosigners = new Cosigner[0],
-                Attributes = new TransactionAttribute[0],
+                Attributes = Array.Empty<TransactionAttribute>(),
                 NetworkFee = 0,
                 Nonce = (uint)Environment.TickCount,
                 Script = new byte[0],

diff --git a/tests/neo.UnitTests/Cryptography/UT_Cryptography_Helper.cs b/tests/neo.UnitTests/Cryptography/UT_Cryptography_Helper.cs
@@ -145,8 +145,7 @@ public void TestTest()
                 Script = TestUtils.GetByteArray(32, 0x42),
                 Sender = UInt160.Zero,
                 SystemFee = 4200000000,
-                Attributes = new TransactionAttribute[0],
-                Cosigners = new Cosigner[0],
+                Attributes = Array.Empty<TransactionAttribute>(),
                 Witnesses = new[]
                 {
                     new Witness

diff --git a/tests/neo.UnitTests/IO/Caching/UT_RelayCache.cs b/tests/neo.UnitTests/IO/Caching/UT_RelayCache.cs
@@ -2,6 +2,7 @@
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Neo.IO.Caching;
 using Neo.Network.P2P.Payloads;
+using System;
 
 namespace Neo.UnitTests.IO.Caching
 {
@@ -27,8 +28,7 @@ public void TestGetKeyForItem()
                 SystemFee = 0,
                 NetworkFee = 0,
                 ValidUntilBlock = 100,
-                Cosigners = new Cosigner[0],
-                Attributes = new TransactionAttribute[0],
+                Attributes = Array.Empty<TransactionAttribute>(),
                 Script = new byte[] { 0x00, 0x01, 0x02, 0x03, 0x04 },
                 Witnesses = new Witness[0]
             };

diff --git a/tests/neo.UnitTests/Ledger/UT_Blockchain.cs b/tests/neo.UnitTests/Ledger/UT_Blockchain.cs
@@ -70,13 +70,13 @@ public void TestContainsTransaction()
         [TestMethod]
         public void TestGetCurrentBlockHash()
         {
-            Blockchain.Singleton.CurrentBlockHash.Should().Be(UInt256.Parse("0xc9387b803c8b4c6c1f69f6c876ed7848482c414b0225eb2a3a5395af39425455"));
+            Blockchain.Singleton.CurrentBlockHash.Should().Be(UInt256.Parse("0x557f5c9d0c865a211a749899681e5b4fbf745b3bcf0c395e6d6a7f1edb0d86f1"));
         }
 
         [TestMethod]
         public void TestGetCurrentHeaderHash()
         {
-            Blockchain.Singleton.CurrentHeaderHash.Should().Be(UInt256.Parse("0xc9387b803c8b4c6c1f69f6c876ed7848482c414b0225eb2a3a5395af39425455"));
+            Blockchain.Singleton.CurrentHeaderHash.Should().Be(UInt256.Parse("0x557f5c9d0c865a211a749899681e5b4fbf745b3bcf0c395e6d6a7f1edb0d86f1"));
         }
 
         [TestMethod]
@@ -88,7 +88,7 @@ public void TestGetBlock()
         [TestMethod]
         public void TestGetBlockHash()
         {
-            Blockchain.Singleton.GetBlockHash(0).Should().Be(UInt256.Parse("0xc9387b803c8b4c6c1f69f6c876ed7848482c414b0225eb2a3a5395af39425455"));
+            Blockchain.Singleton.GetBlockHash(0).Should().Be(UInt256.Parse("0x557f5c9d0c865a211a749899681e5b4fbf745b3bcf0c395e6d6a7f1edb0d86f1"));
             Blockchain.Singleton.GetBlockHash(10).Should().BeNull();
         }
 

diff --git a/tests/neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/neo.UnitTests/Ledger/UT_MemoryPool.cs
@@ -79,8 +79,7 @@ private Transaction CreateTransactionWithFee(long fee)
             mock.Object.Script = randomBytes;
             mock.Object.Sender = UInt160.Zero;
             mock.Object.NetworkFee = fee;
-            mock.Object.Attributes = new TransactionAttribute[0];
-            mock.Object.Cosigners = new Cosigner[0];
+            mock.Object.Attributes = Array.Empty<TransactionAttribute>();
             mock.Object.Witnesses = new[]
             {
                 new Witness
@@ -104,8 +103,7 @@ private Transaction CreateTransactionWithFeeAndBalanceVerify(long fee)
             mock.Object.Script = randomBytes;
             mock.Object.Sender = sender;
             mock.Object.NetworkFee = fee;
-            mock.Object.Attributes = new TransactionAttribute[0];
-            mock.Object.Cosigners = new Cosigner[0];
+            mock.Object.Attributes = Array.Empty<TransactionAttribute>();
             mock.Object.Witnesses = new[]
             {
                 new Witness