Restrict maximum number of conflicting transactions per Ledger's conf…

…lict record (#2911)

* Revert "Sort and limit size"

This reverts commit 2a69047.

As it was said in #2909 (comment),
we can't save only a part of on-chained conflicting signers. Instead, we
need to construct new blocks in such way so that there were no
conflicting signers limit overflow. It's not hard to implement, see the
further commits.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

* Ledger: restrict max number of conflicting txs per conflict record

Instead of restricting the maximum number of conflicting signers per conflict
record we can restrict the number of transactions that make their contribution
to this conflict record. The constraint nature is the same, it doesn't allow to
spam the chain with a large set of transactions with the same Conflicts attribute.
However, this approach has several benefits over restricting the number of
signers for these conflicting transactions:
1. It significantly simplifies and accelerates a newly-pooled transactions verification.
   We don't need to perform this `Concat(signers).Distinct().Count()` operation anymore
   on new transaction addition.
2. It has a space for improvement which is not presented in this commit, but have to be
   implemented if the current solution will be accepted by the core developers. A space
   for improvement is the following: during Conflicts attribute verification we don't
   actually need to retrieve the whole list of conflicting signers for the conflict
   record. What we need instead is to check the `ConflictingTransactionsCount` field only.
   Thus, we may safely omit the `ConflictingSigners` deserialisation from stack item.
3. It allows to easily port the same constraint to the mempool (see the further commit)
   which, in turn, roughly restrict the overall possible number of transactions per
   conflict record in Ledger contract up to 2*MaxAllowedConflictingTransactions. It means,
   that the maximum number of signers per conflict record in the worst case equals to
   2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480.
At the same time, the overhead the current commit brings in the `TransactionState` is
quite insignificant (it's only a single extra Integer field).

Note 1: this PR changes the native Ledger scheme, thus, requires the DB resync on
upgrade. States will differ from the current 3.6.0 T5.

Note 2: this PR (as far as #2908) doesn't help with those transactions that already
were included into blocks 2690038-2690040 of the current T5 due to the
#2907 (comment). We need to have a
separate discussion on these "malicious" blocks and decide how to handle them.
However, this PR doesn't ever prevent the node from the proper processing of these
blocks on resync, the blocks will be processed successfully (with HALT state, the same
as they were processed by the 3.6.0 release).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

* MemoryPool: restrict max number of conflicting txs per conflict record

Port the Ledger conflicting tx restriction logic to the MemoryPool: the maximum
number of mempooled transactions that has the same Conflicts attribute (i.e.
those that conflict with the same transaction) is restricted by the
LedgerContract.MaxAllowedConflictingTransactions constant. This restriction,
combined with the existing Ledger's storage restriction, roughly restricts the
overall possible number of transactions per conflict record in Ledger contract
up to 2*MaxAllowedConflictingTransactions. It means, that the maximum number
of signers per conflict record in the worst case equals to
2*MaxAllowedConflictingTransactions*(MaxTransactionAttributes-1)=480.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

---------

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

src/Neo/Ledger/MemoryPool.cs

@@ -367,6 +367,12 @@ private bool CheckConflicts(Transaction tx, out List<PoolItem> conflictsList)
             // Step 2: check if unsorted transactions were in `tx`'s Conflicts attributes.
             foreach (var hash in tx.GetAttributes<Conflicts>().Select(p => p.Hash))
             {
+                // Firstly, check whether there's a free space in the list of transactions that has the same
+                // conflicts as `tx` has (in case of successfull verification, `tx` has to be added to this list).
+                if (_conflicts.TryGetValue(hash, out var pooled))
+                    if (pooled.Count() >= Neo.SmartContract.Native.LedgerContract.MaxAllowedConflictingTransactions) return false;
+
+                // After that, check if unsorted transactions are in the `tx`'s Conflicts attributes.
                 if (_unsortedTransactions.TryGetValue(hash, out PoolItem unsortedTx))
                 {
                     if (!tx.Signers.Select(p => p.Account).Intersect(unsortedTx.Tx.Signers.Select(p => p.Account)).Any()) return false;

src/Neo/Network/P2P/Payloads/Conflicts.cs

@@ -40,21 +40,10 @@ public override JObject ToJson()
 
         public override bool Verify(DataCache snapshot, Transaction tx)
         {
-            // Check already stored signers
-
-            HashSet<UInt256> hashes = new();
-            var conflictingSigners = tx.Signers.Select(s => s.Account).ToArray();
-
-            foreach (var attr in tx.GetAttributes<Conflicts>())
-            {
-                // Can't fit MaxAllowedConflictingSigners
-                if (!NativeContract.Ledger.CanFitConflictingSigners(snapshot, tx.Hash, conflictingSigners))
-                    return false;
-
-                // Hash duplicated on different attributes
-                if (!hashes.Add(attr.Hash))
-                    return false;
-            }
+            // Check whether conflicting record has enough space to fit the signers of
+            // yet another conflicting transaction.
+            if (!NativeContract.Ledger.CanFitConflictingTransaction(snapshot, tx))
+                return false;
 
             // Only check if conflicting transaction is on chain. It's OK if the
             // conflicting transaction was in the Conflicts attribute of some other

src/Neo/SmartContract/Native/LedgerContract.cs

@@ -30,7 +30,15 @@ public sealed class LedgerContract : NativeContract
         private const byte Prefix_CurrentBlock = 12;
         private const byte Prefix_Block = 5;
         private const byte Prefix_Transaction = 11;
-        private const int MaxAllowedConflictingSigners = 100;
+        // MaxAllowedConflictingTransactions is the maximum number of transactions that make their
+        // contribution to a single conflicting signers record. This setting affects native Ledger
+        // storage in the following way: after MaxAllowedConflictingTransactions is stored in the
+        // Ledger's storage for a certain confclit record, mempool stops accepting new conflicting
+        // transactions corresponding to this record. At the same time, maximum number of
+        // conflicting records in mempool is limited by the same constant. Thus, the overall maximum
+        // number of transactions per a single conflict record that can ever exist in the Ledger's
+        // storage is 2*MaxAllowedConflictingTransactions.
+        internal const int MaxAllowedConflictingTransactions = 16;
 
         internal LedgerContract()
         {
@@ -54,24 +62,33 @@ internal override ContractTask OnPersist(ApplicationEngine engine)
                 {
                     var conflictRecord = engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash),
                         () => new StorageItem(new TransactionState { ConflictingSigners = Array.Empty<UInt160>() })).GetInteroperable<TransactionState>();
-                    conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners)
-                        .Distinct().OrderBy(s => s).Take(MaxAllowedConflictingSigners).ToArray();
+                    conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners).Distinct().ToArray();
                 }
             }
             engine.SetState(transactions);
             return ContractTask.CompletedTask;
         }
 
-        internal bool CanFitConflictingSigners(DataCache snapshot, UInt256 hash, UInt160[] signers)
+        internal bool CanFitConflictingTransaction(DataCache snapshot, Transaction tx)
         {
-            var conflictRecord = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?
+            HashSet<UInt256> hashes = new();
+
+            foreach (var attr in tx.GetAttributes<Conflicts>())
+            {
+                // Filter out already handled hashes.
+                if (!hashes.Add(attr.Hash))
+                    continue;
+
+                var conflictRecord = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(attr.Hash))?
                     .GetInteroperable<TransactionState>();
 
-            if (conflictRecord is null || conflictRecord.Transaction is not null) return true;
+                // Filter out fully-qualified (simple) transactions and check only conflicing records.
+                if (conflictRecord is null || conflictRecord.Transaction is not null)
+                    continue;
+
+                if (conflictRecord.ConflictingTransactionsCount > MaxAllowedConflictingTransactions)
+                    return false;
 
-            if (conflictRecord.ConflictingSigners.Concat(signers).Distinct().Count() > MaxAllowedConflictingSigners)
-            {
-                return false;
             }
 
             return true;

src/Neo/SmartContract/Native/TransactionState.cs

@@ -32,6 +32,8 @@ public class TransactionState : IInteroperable
         /// </summary>
         public Transaction Transaction;
 
+        public uint ConflictingTransactionsCount;
+
         public UInt160[] ConflictingSigners;
 
         /// <summary>
@@ -47,6 +49,7 @@ IInteroperable IInteroperable.Clone()
             {
                 BlockIndex = BlockIndex,
                 Transaction = Transaction,
+                ConflictingTransactionsCount = ConflictingTransactionsCount,
                 ConflictingSigners = ConflictingSigners,
                 State = State,
                 _rawTransaction = _rawTransaction
@@ -59,6 +62,7 @@ void IInteroperable.FromReplica(IInteroperable replica)
             BlockIndex = from.BlockIndex;
             Transaction = from.Transaction;
             ConflictingSigners = from.ConflictingSigners;
+            ConflictingTransactionsCount = from.ConflictingTransactionsCount;
             State = from.State;
             if (_rawTransaction.IsEmpty)
                 _rawTransaction = from._rawTransaction;
@@ -67,9 +71,10 @@ void IInteroperable.FromReplica(IInteroperable replica)
         void IInteroperable.FromStackItem(StackItem stackItem)
         {
             Struct @struct = (Struct)stackItem;
-            if (@struct.Count == 1)
+            if (@struct.Count == 2)
             {
-                ConflictingSigners = ((VM.Types.Array)@struct[0]).Select(u => new UInt160(u.GetSpan())).ToArray();
+                ConflictingTransactionsCount = (uint)@struct[0].GetInteger();
+                ConflictingSigners = ((VM.Types.Array)@struct[1]).Select(u => new UInt160(u.GetSpan())).ToArray();
                 return;
             }
             BlockIndex = (uint)@struct[0].GetInteger();
@@ -80,7 +85,7 @@ void IInteroperable.FromStackItem(StackItem stackItem)
 
         StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter)
         {
-            if (Transaction is null) return new Struct(referenceCounter) { new VM.Types.Array(referenceCounter, ConflictingSigners.Select(u => new ByteString(u.ToArray())).ToArray()) };
+            if (Transaction is null) return new Struct(referenceCounter) { ConflictingTransactionsCount, 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 };