Check signers of on-chained conflict during new tx verification

Fix the problem described in
neo-project#2818 (review).

During new transaction verification if there's an on-chain conflicting
transaction, we should check the signers of this conflicting transaction.
If the signers contain payer of the incoming transaction, then the conflict
is treated as valid and verification for new incoming transaction should
fail. Otherwise, the conflict is treated as the malicious attack attempt
and will not be taken into account; verification for the new incoming
transaction should continue in this case.

Author: AnnaShaleva

src/Neo/Ledger/Blockchain.cs

@@ -206,7 +206,7 @@ private void OnFillMemoryPool(IEnumerable<Transaction> transactions)
             {
                 if (NativeContract.Ledger.ContainsTransaction(snapshot, tx.Hash))
                     continue;
-                if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash))
+                if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash, tx.Sender))
                     continue;
                 // First remove the tx if it is unverified in the pool.
                 system.MemPool.TryRemoveUnVerified(tx.Hash, out _);
@@ -339,7 +339,7 @@ private VerifyResult OnNewExtensiblePayload(ExtensiblePayload payload)
         private VerifyResult OnNewTransaction(Transaction transaction)
         {
             if (system.ContainsTransaction(transaction.Hash)) return VerifyResult.AlreadyExists;
-            if (system.ContainsConflictHash(transaction.Hash)) return VerifyResult.HasConflicts;
+            if (system.ContainsConflictHash(transaction.Hash, transaction.Sender)) return VerifyResult.HasConflicts;
             return system.MemPool.TryAdd(transaction, system.StoreView);
         }
 
@@ -394,7 +394,7 @@ private void OnTransaction(Transaction tx)
         {
             if (system.ContainsTransaction(tx.Hash))
                 SendRelayResult(tx, VerifyResult.AlreadyExists);
-            else if (system.ContainsConflictHash(tx.Hash))
+            else if (system.ContainsConflictHash(tx.Hash, tx.Sender))
                 SendRelayResult(tx, VerifyResult.HasConflicts);
             else system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true));
         }

src/Neo/Ledger/MemoryPool.cs

@@ -469,15 +469,16 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot)
             _txRwLock.EnterWriteLock();
             try
             {
-                HashSet<UInt256> conflicts = new HashSet<UInt256>();
+                Dictionary<UInt256, UInt160[]> conflicts = new Dictionary<UInt256, UInt160[]>();
                 // First remove the transactions verified in the block.
                 // No need to modify VerificationContext as it will be reset afterwards.
                 foreach (Transaction tx in block.Transactions)
                 {
                     if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerified(tx.Hash, out _);
+                    UInt160[] conflictingSigners = tx.Signers.Select(s => s.Account).ToArray();
                     foreach (var h in tx.GetAttributes<Conflicts>().Select(a => a.Hash))
                     {
-                        conflicts.Add(h);
+                        conflicts.Add(h, conflictingSigners);
                     }
                 }
 
@@ -487,7 +488,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot)
                 var stale = new List<UInt256>();
                 foreach (var item in _sortedTransactions)
                 {
-                    if (conflicts.Contains(item.Tx.Hash) || item.Tx.GetAttributes<Conflicts>().Select(a => a.Hash).Intersect(persisted).Any())
+                    if ((conflicts.TryGetValue(item.Tx.Hash, out var signers) && signers.Contains(item.Tx.Sender)) || item.Tx.GetAttributes<Conflicts>().Select(a => a.Hash).Intersect(persisted).Any())
                     {
                         stale.Add(item.Tx.Hash);
                         conflictingItems.Add(item.Tx);

src/Neo/NeoSystem.cs

@@ -283,10 +283,11 @@ public bool ContainsTransaction(UInt256 hash)
         /// Determines whether the specified transaction conflicts with some on-chain transaction.
         /// </summary>
         /// <param name="hash">The hash of the transaction</param>
+        /// <param name="sender">The sender of the transaction</param>
         /// <returns><see langword="true"/> if the transaction conflicts with on-chain transaction; otherwise, <see langword="false"/>.</returns>
-        public bool ContainsConflictHash(UInt256 hash)
+        public bool ContainsConflictHash(UInt256 hash, UInt160 sender)
         {
-            return NativeContract.Ledger.ContainsConflictHash(StoreView, hash);
+            return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, sender);
         }
     }
 }

src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs

@@ -318,7 +318,7 @@ private void OnInventoryReceived(IInventory inventory)
             switch (inventory)
             {
                 case Transaction transaction:
-                    if (!(system.ContainsTransaction(transaction.Hash) || system.ContainsConflictHash(transaction.Hash)))
+                    if (!(system.ContainsTransaction(transaction.Hash) || system.ContainsConflictHash(transaction.Hash, transaction.Sender)))
                         system.TxRouter.Tell(new TransactionRouter.Preverify(transaction, true));
                     break;
                 case Block block:
@@ -347,7 +347,7 @@ private void OnInvMessageReceived(InvPayload payload)
                 case InventoryType.TX:
                     {
                         DataCache snapshot = system.StoreView;
-                        hashes = hashes.Where(p => !(NativeContract.Ledger.ContainsTransaction(snapshot, p) || NativeContract.Ledger.ContainsConflictHash(snapshot, p))).ToArray();
+                        hashes = hashes.Where(p => !(NativeContract.Ledger.ContainsTransaction(snapshot, p) || NativeContract.Ledger.ContainsConflictHash(snapshot, p, null))).ToArray();
                     }
                     break;
             }

src/Neo/SmartContract/Native/LedgerContract.cs

@@ -47,9 +47,10 @@ internal override ContractTask OnPersist(ApplicationEngine engine)
             foreach (TransactionState tx in transactions)
             {
                 engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx));
+                UInt160[] conflictingSigners = tx.Transaction.Signers.Select(s => s.Account).ToArray();
                 foreach (var attr in tx.Transaction.GetAttributes<Conflicts>())
                 {
-                    engine.Snapshot.GetOrAdd(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState()));
+                    engine.Snapshot.GetOrAdd(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState { ConflictingSigners = conflictingSigners }));
                 }
             }
             engine.SetState(transactions);
@@ -140,11 +141,12 @@ public bool ContainsTransaction(DataCache snapshot, UInt256 hash)
         /// </summary>
         /// <param name="snapshot">The snapshot used to read data.</param>
         /// <param name="hash">The hash of the conflicting transaction.</param>
+        /// <param name="sender">The sender of the conflicting transaction.</param>
         /// <returns><see langword="true"/> if the blockchain contains the hash of the conflicting transaction; otherwise, <see langword="false"/>.</returns>
-        public bool ContainsConflictHash(DataCache snapshot, UInt256 hash)
+        public bool ContainsConflictHash(DataCache snapshot, UInt256 hash, UInt160 sender)
         {
             var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable<TransactionState>();
-            if (state is not null && state.Transaction is null) return true;
+            if (state is not null && state.Transaction is null && (sender is null || state.ConflictingSigners.Contains(sender))) return true;
             return false;
         }
 

src/Neo/SmartContract/Native/TransactionState.cs

@@ -13,6 +13,7 @@
 using Neo.VM;
 using Neo.VM.Types;
 using System;
+using System.Linq;
 
 namespace Neo.SmartContract.Native
 {
@@ -31,6 +32,8 @@ public class TransactionState : IInteroperable
         /// </summary>
         public Transaction Transaction;
 
+        public UInt160[] ConflictingSigners;
+
         /// <summary>
         /// The execution state
         /// </summary>
@@ -44,6 +47,7 @@ IInteroperable IInteroperable.Clone()
             {
                 BlockIndex = BlockIndex,
                 Transaction = Transaction,
+                ConflictingSigners = ConflictingSigners,
                 State = State,
                 _rawTransaction = _rawTransaction
             };
@@ -54,6 +58,7 @@ void IInteroperable.FromReplica(IInteroperable replica)
             TransactionState from = (TransactionState)replica;
             BlockIndex = from.BlockIndex;
             Transaction = from.Transaction;
+            ConflictingSigners = from.ConflictingSigners;
             State = from.State;
             if (_rawTransaction.IsEmpty)
                 _rawTransaction = from._rawTransaction;
@@ -62,7 +67,11 @@ void IInteroperable.FromReplica(IInteroperable replica)
         void IInteroperable.FromStackItem(StackItem stackItem)
         {
             Struct @struct = (Struct)stackItem;
-            if (@struct.Count == 0) return;
+            if (@struct.Count == 1)
+            {
+                ConflictingSigners = ((VM.Types.Array)@struct[0]).Select(u => new UInt160(u.GetSpan())).ToArray();
+                return;
+            }
             BlockIndex = (uint)@struct[0].GetInteger();
             _rawTransaction = ((ByteString)@struct[1]).Memory;
             Transaction = _rawTransaction.AsSerializable<Transaction>();
@@ -71,7 +80,7 @@ void IInteroperable.FromStackItem(StackItem stackItem)
 
         StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter)
         {
-            if (Transaction is null) return new Struct(referenceCounter);
+            if (Transaction is null) return new Struct(referenceCounter) { new VM.Types.Array(referenceCounter, ConflictingSigners.Select(u => new ByteString(u.ToArray())).ToArray()) };
             if (_rawTransaction.IsEmpty)
                 _rawTransaction = Transaction.ToArray();
             return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State };

tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs

@@ -252,7 +252,7 @@ public async Task UpdatePoolForBlockPersisted_RemoveBlockConflicts()
             ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, settings: TestBlockchain.TheNeoSystem.Settings, gas: long.MaxValue);
             engine.LoadScript(Array.Empty<byte>());
             await NativeContract.GAS.Burn(engine, UInt160.Zero, balance);
-            _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 6, true); // balance enough for 6 mempooled txs
+            _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 7, true); // balance enough for 7 mempooled txs
 
             var mp1 = CreateTransactionWithFeeAndBalanceVerify(txFee);  // mp1 doesn't conflict with anyone
             _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed);
@@ -283,21 +283,28 @@ public async Task UpdatePoolForBlockPersisted_RemoveBlockConflicts()
             _unit.SortedTxCount.Should().Be(6);
             _unit.UnverifiedSortedTxCount.Should().Be(0);
 
+            var mp7 = CreateTransactionWithFeeAndBalanceVerify(txFee);  // mp7 doesn't conflict with anyone
+            var tx6 = CreateTransactionWithFeeAndBalanceVerify(txFee);  // in-block tx6 conflicts with mp7, but doesn't include sender of mp7 into signers list => even if tx6 is included into block, mp7 shouldn't be removed from the pool
+            tx6.Signers = new Signer[] { new Signer() { Account = new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })) }, new Signer() { Account = new UInt160(Crypto.Hash160(new byte[] { 4, 5, 6 })) } };
+            tx6.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp7.Hash } };
+            _unit.TryAdd(mp7, engine.Snapshot);
+
             // Act: persist block and reverify all mempooled txs.
             var block = new Block
             {
                 Header = new Header(),
-                Transactions = new Transaction[] { tx1, tx2, tx3, tx4, tx5 },
+                Transactions = new Transaction[] { tx1, tx2, tx3, tx4, tx5, tx6 },
             };
             _unit.UpdatePoolForBlockPersisted(block, engine.Snapshot);
 
             // Assert: conflicting txs should be removed from the pool; the only mp6 that doesn't conflict with anyone should be left.
-            _unit.SortedTxCount.Should().Be(1);
+            _unit.SortedTxCount.Should().Be(2);
             _unit.GetSortedVerifiedTransactions().Select(tx => tx.Hash).Should().Contain(mp6.Hash);
+            _unit.GetSortedVerifiedTransactions().Select(tx => tx.Hash).Should().Contain(mp7.Hash);
             _unit.UnverifiedSortedTxCount.Should().Be(0);
 
             // Cleanup: revert the balance.
-            await NativeContract.GAS.Burn(engine, UInt160.Zero, txFee * 6);
+            await NativeContract.GAS.Burn(engine, UInt160.Zero, txFee * 7);
             _ = NativeContract.GAS.Mint(engine, UInt160.Zero, balance, true);
         }
 

tests/Neo.UnitTests/Ledger/UT_TransactionState.cs

@@ -1,5 +1,6 @@
 using FluentAssertions;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Neo.Cryptography;
 using Neo.IO;
 using Neo.Network.P2P.Payloads;
 using Neo.SmartContract;
@@ -33,7 +34,14 @@ public void Initialize()
                     } }
                 }
             };
-            originTrimmed = new TransactionState();
+            originTrimmed = new TransactionState
+            {
+                ConflictingSigners = new UInt160[]
+            {
+                new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })),
+                new UInt160(Crypto.Hash160(new byte[] { 4, 5, 6 }))
+            }
+            };
         }
 
         [TestMethod]
@@ -62,6 +70,7 @@ public void TestDeserializeTrimmed()
             dest.BlockIndex.Should().Be(0);
             dest.Transaction.Should().Be(null);
             dest.Transaction.Should().BeNull();
+            CollectionAssert.AreEqual(originTrimmed.ConflictingSigners, dest.ConflictingSigners);
         }
     }
 }