Improved scheme for Conflicts attribute storing and pricing (#2913)

* Ledger: change conflict records storage scheme

This commit is a part of #2907: it implements conflict records storage scheme discribed
in #2907 (comment).

The short scheme description:

Do not store the list of conflicting signers in the Ledger's conflict record value.
Instead, put each conflicting signer in the conflict record key so that the
reculting key is: {Prefix_Transaction, <conflict hash>, <signer>}, and the value
is {<block index>}.

As an optimisation, for each conflict record store the dummy stub where the
key is {Prefix_Transaction, <conflict hash>} and the value is {<block index>}. This optimisation
allows to reduce DB read requests during newly-pooled transaction verification for
those transactions that do not have any on-chained conflicts.

Also, IsTraceableBlock check is added for on-chain conflicts verification. It is
needed to avoid situations when untraceable on-chained conflict affects the
newly-pooled transaction verification.

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

* UT_MemoryPool: remove unused test

Malicious_OnChain_Conflict was constructed incorrectly (see the comment in the end of
the test) and was replaced by the proper TestMaliciousOnChainConflict test in
UT_Blockchain.cs way back ago in
0c06c91.

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

* Policy: introduce fee for Conflicts attribute

This commit implements Conflicts attribute policy described in
#2907 (comment).

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

* *: remove remnants of ConflictsFee in native Policy

ConflictsFee logic was replaced by the generic attribute fee mechanism
implemented in #2916.

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

* Native: do not remove malicious conflict records during OnPersist

It's OK to keep them and save O(n) operations during OnPersist. The
storage taken by these malicious conflict records is properly paid
anyway. See the discussion under #2913 (comment).

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

* Properly rewrite previously added malicious conflict if it's in the storage

`engine.Snapshot.Add` doesn't allow to rewrite storage entity if it's already exist.
Thus, we firstly need to remove it and afterwards to add the updated entity.

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

* Throw proper exception if TestMaliciousOnChainConflict fails

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

* Optimize conflicts records storing

Use Snapshot.GetAndChange instead of subsequent calls to Delete and Add.
Ref. #2913 (comment).

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

---------

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Jimmy <jinghui@wayne.edu>

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, tx.Signers.Select(s => s.Account)))
+                if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash, tx.Signers.Select(s => s.Account), system.Settings.MaxTraceableBlocks))
                     continue;
                 // First remove the tx if it is unverified in the pool.
                 system.MemPool.TryRemoveUnVerified(tx.Hash, out _);

src/Neo/NeoSystem.cs

@@ -287,7 +287,7 @@ public bool ContainsTransaction(UInt256 hash)
         /// <returns><see langword="true"/> if the transaction conflicts with on-chain transaction; otherwise, <see langword="false"/>.</returns>
         public bool ContainsConflictHash(UInt256 hash, IEnumerable<UInt160> signers)
         {
-            return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers);
+            return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers, Settings.MaxTraceableBlocks);
         }
     }
 }

src/Neo/SmartContract/Native/LedgerContract.cs

@@ -47,13 +47,19 @@ internal override ContractTask OnPersist(ApplicationEngine engine)
             engine.Snapshot.Add(CreateStorageKey(Prefix_Block).Add(engine.PersistingBlock.Hash), new StorageItem(Trim(engine.PersistingBlock).ToArray()));
             foreach (TransactionState tx in transactions)
             {
-                engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx));
+                // It's possible that there are previously saved malicious conflict records for this transaction.
+                // If so, then remove it and store the relevant transaction itself.
+                engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(tx));
+
+                // Store transaction's conflicits.
                 var conflictingSigners = tx.Transaction.Signers.Select(s => s.Account);
                 foreach (var attr in tx.Transaction.GetAttributes<Conflicts>())
                 {
-                    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().ToArray();
+                    engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index }));
+                    foreach (var signer in conflictingSigners)
+                    {
+                        engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index }));
+                    }
                 }
             }
             engine.SetState(transactions);
@@ -145,11 +151,24 @@ public bool ContainsTransaction(DataCache snapshot, UInt256 hash)
         /// <param name="snapshot">The snapshot used to read data.</param>
         /// <param name="hash">The hash of the conflicting transaction.</param>
         /// <param name="signers">The list of signer accounts of the conflicting transaction.</param>
+        /// <param name="maxTraceableBlocks">MaxTraceableBlocks protocol setting.</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, IEnumerable<UInt160> signers)
+        public bool ContainsConflictHash(DataCache snapshot, UInt256 hash, IEnumerable<UInt160> signers, uint maxTraceableBlocks)
         {
-            var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable<TransactionState>();
-            return state is not null && state.Transaction is null && (signers is null || state.ConflictingSigners.Intersect(signers).Any());
+            // Check the dummy stub firstly to define whether there's exist at least one conflict record.
+            var stub = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable<TransactionState>();
+            if (stub is null || stub.Transaction is not null || !IsTraceableBlock(snapshot, stub.BlockIndex, maxTraceableBlocks))
+                return false;
+
+            // At least one conflict record is found, then need to check signers intersection.
+            foreach (var signer in signers)
+            {
+                var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash).Add(signer))?.GetInteroperable<TransactionState>();
+                if (state is not null && IsTraceableBlock(snapshot, state.BlockIndex, maxTraceableBlocks))
+                    return true;
+            }
+
+            return false;
         }
 
         /// <summary>

src/Neo/SmartContract/Native/TransactionState.cs

@@ -32,8 +32,6 @@ public class TransactionState : IInteroperable
         /// </summary>
         public Transaction Transaction;
 
-        public UInt160[] ConflictingSigners;
-
         /// <summary>
         /// The execution state
         /// </summary>
@@ -47,7 +45,6 @@ IInteroperable IInteroperable.Clone()
             {
                 BlockIndex = BlockIndex,
                 Transaction = Transaction,
-                ConflictingSigners = ConflictingSigners,
                 State = State,
                 _rawTransaction = _rawTransaction
             };
@@ -58,7 +55,6 @@ 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;
@@ -67,20 +63,21 @@ void IInteroperable.FromReplica(IInteroperable replica)
         void IInteroperable.FromStackItem(StackItem stackItem)
         {
             Struct @struct = (Struct)stackItem;
-            if (@struct.Count == 1)
-            {
-                ConflictingSigners = ((VM.Types.Array)@struct[0]).Select(u => new UInt160(u.GetSpan())).ToArray();
-                return;
-            }
             BlockIndex = (uint)@struct[0].GetInteger();
+
+            // Conflict record.
+            if (@struct.Count == 1) return;
+
+            // Fully-qualified transaction.
             _rawTransaction = ((ByteString)@struct[1]).Memory;
             Transaction = _rawTransaction.AsSerializable<Transaction>();
             State = (VMState)(byte)@struct[2].GetInteger();
         }
 
         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) { BlockIndex };
             if (_rawTransaction.IsEmpty)
                 _rawTransaction = Transaction.ToArray();
             return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State };

tests/Neo.UnitTests/Ledger/UT_Blockchain.cs

@@ -135,7 +135,7 @@ public void TestMaliciousOnChainConflict()
             {
                 Header = new Header()
                 {
-                    Index = 10000,
+                    Index = 5, // allow tx1, tx2 and tx3 to fit into MaxValidUntilBlockIncrement.
                     MerkleRoot = UInt256.Zero,
                     NextConsensus = UInt160.Zero,
                     PrevHash = UInt256.Zero,
@@ -149,13 +149,26 @@ public void TestMaliciousOnChainConflict()
                 sb.EmitSysCall(ApplicationEngine.System_Contract_NativeOnPersist);
                 onPersistScript = sb.ToArray();
             }
-            TransactionState[] transactionStates;
             using (ApplicationEngine engine2 = ApplicationEngine.Create(TriggerType.OnPersist, null, snapshot, block, TestBlockchain.TheNeoSystem.Settings, 0))
             {
                 engine2.LoadScript(onPersistScript);
-                if (engine2.Execute() != VMState.HALT) throw new InvalidOperationException();
-                Blockchain.ApplicationExecuted application_executed = new(engine2);
-                transactionStates = engine2.GetState<TransactionState[]>();
+                if (engine2.Execute() != VMState.HALT) throw engine2.FaultException;
+                engine2.Snapshot.Commit();
+            }
+            snapshot.Commit();
+
+            // Run PostPersist to update current block index in native Ledger.
+            // Relevant current block index is needed for conflict records checks.
+            byte[] postPersistScript;
+            using (ScriptBuilder sb = new())
+            {
+                sb.EmitSysCall(ApplicationEngine.System_Contract_NativePostPersist);
+                postPersistScript = sb.ToArray();
+            }
+            using (ApplicationEngine engine2 = ApplicationEngine.Create(TriggerType.PostPersist, null, snapshot, block, TestBlockchain.TheNeoSystem.Settings, 0))
+            {
+                engine2.LoadScript(postPersistScript);
+                if (engine2.Execute() != VMState.HALT) throw engine2.FaultException;
                 engine2.Snapshot.Commit();
             }
             snapshot.Commit();

tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs

@@ -736,74 +736,6 @@ public void TestUpdatePoolForBlockPersisted()
             _unit.VerifiedCount.Should().Be(0);
         }
 
-
-        [TestMethod]
-        public async Task Malicious_OnChain_Conflict()
-        {
-            // Arrange: prepare mempooled txs that have conflicts.
-            long txFee = 1;
-            var snapshot = GetSnapshot();
-            BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, senderAccount);
-            ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, settings: TestBlockchain.TheNeoSystem.Settings, gas: long.MaxValue);
-            engine.LoadScript(Array.Empty<byte>());
-
-            var tx1 = CreateTransactionWithFeeAndBalanceVerify(txFee);
-            var tx2 = CreateTransactionWithFeeAndBalanceVerify(txFee);
-
-            tx1.Signers[0].Account = UInt160.Parse("0x0001020304050607080900010203040506070809"); // Different sender
-
-            await NativeContract.GAS.Mint(engine, tx1.Sender, 100000, true); // balance enough for all mempooled txs
-            await NativeContract.GAS.Mint(engine, tx2.Sender, 100000, true); // balance enough for all mempooled txs
-
-            tx1.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = tx2.Hash } };
-
-            Assert.AreEqual(_unit.TryAdd(tx1, engine.Snapshot), VerifyResult.Succeed);
-
-            var block = new Block
-            {
-                Header = new Header()
-                {
-                    Index = 10000,
-                    MerkleRoot = UInt256.Zero,
-                    NextConsensus = UInt160.Zero,
-                    PrevHash = UInt256.Zero,
-                    Witness = new Witness() { InvocationScript = Array.Empty<byte>(), VerificationScript = Array.Empty<byte>() }
-                },
-                Transactions = new Transaction[] { tx1 },
-            };
-            _unit.UpdatePoolForBlockPersisted(block, engine.Snapshot);
-
-            _unit.SortedTxCount.Should().Be(0);
-
-            // Simulate persist tx1
-
-            Assert.AreEqual(_unit.TryAdd(tx2, engine.Snapshot), VerifyResult.Succeed);
-
-            byte[] onPersistScript;
-            using (ScriptBuilder sb = new())
-            {
-                sb.EmitSysCall(ApplicationEngine.System_Contract_NativeOnPersist);
-                onPersistScript = sb.ToArray();
-            }
-
-            TransactionState[] transactionStates;
-            using (ApplicationEngine engine2 = ApplicationEngine.Create(TriggerType.OnPersist, null, engine.Snapshot, block, TestBlockchain.TheNeoSystem.Settings, 0))
-            {
-                engine2.LoadScript(onPersistScript);
-                if (engine2.Execute() != VMState.HALT) throw new InvalidOperationException();
-                Blockchain.ApplicationExecuted application_executed = new(engine2);
-                transactionStates = engine2.GetState<TransactionState[]>();
-            }
-
-            // Test tx2 arrive
-
-            snapshot.Commit();
-
-            var senderProbe = CreateTestProbe();
-            senderProbe.Send(TestBlockchain.TheNeoSystem.Blockchain, tx2);
-            senderProbe.ExpectMsg<Blockchain.RelayResult>(p => p.Result == VerifyResult.InsufficientFunds); // should be Succedded.
-        }
-
         public static StorageKey CreateStorageKey(int id, byte prefix, byte[] key = null)
         {
             byte[] buffer = GC.AllocateUninitializedArray<byte>(sizeof(byte) + (key?.Length ?? 0));

tests/Neo.UnitTests/Ledger/UT_TransactionState.cs

@@ -36,11 +36,7 @@ public void Initialize()
             };
             originTrimmed = new TransactionState
             {
-                ConflictingSigners = new UInt160[]
-            {
-                new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })),
-                new UInt160(Crypto.Hash160(new byte[] { 4, 5, 6 }))
-            }
+                BlockIndex = 1,
             };
         }
 
@@ -67,10 +63,9 @@ public void TestDeserializeTrimmed()
             TransactionState dest = new();
             ((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null));
 
-            dest.BlockIndex.Should().Be(0);
+            dest.BlockIndex.Should().Be(originTrimmed.BlockIndex);
             dest.Transaction.Should().Be(null);
             dest.Transaction.Should().BeNull();
-            CollectionAssert.AreEqual(originTrimmed.ConflictingSigners, dest.ConflictingSigners);
         }
     }
 }