From 77d823cabfdd6e9b9d97701192b0785289b4e89a Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 23 Sep 2022 16:07:31 +0300 Subject: [PATCH 01/19] Payloads: implement Conflicts attribute Closes #1991. --- src/Neo/Ledger/MemoryPool.cs | 157 ++++++++++++++-- src/Neo/Ledger/TransactionRemovalReason.cs | 9 +- .../Ledger/TransactionVerificationContext.cs | 7 +- src/Neo/Ledger/VerifyResult.cs | 5 + src/Neo/Network/P2P/Payloads/Conflicts.cs | 44 +++++ src/Neo/Network/P2P/Payloads/Transaction.cs | 10 +- .../P2P/Payloads/TransactionAttributeType.cs | 8 +- .../SmartContract/Native/LedgerContract.cs | 9 +- .../SmartContract/Native/TransactionState.cs | 18 +- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 167 +++++++++++++++++- .../Ledger/UT_TransactionState.cs | 21 +++ .../UT_TransactionVerificationContext.cs | 43 ++++- .../Network/P2P/Payloads/UT_Conflicts.cs | 74 ++++++++ .../Network/P2P/Payloads/UT_Transaction.cs | 10 +- 14 files changed, 541 insertions(+), 41 deletions(-) create mode 100644 src/Neo/Network/P2P/Payloads/Conflicts.cs create mode 100644 tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 6ade372ee1..6ac64fde68 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -55,6 +55,10 @@ public class MemoryPool : IReadOnlyCollection /// private readonly Dictionary _unsortedTransactions = new(); /// + /// Store transaction hashes that conflict with verified mempooled transactions. + /// + private readonly Dictionary> _conflicts = new(); + /// /// Stores the verified sorted transactions currently in the pool. /// private readonly SortedSet _sortedTransactions = new(); @@ -275,7 +279,8 @@ private PoolItem GetLowestFeeTransaction(out Dictionary unsor } } - // Note: this must only be called from a single thread (the Blockchain actor) + // Note: this must only be called from a single thread (the Blockchain actor) and + // doesn't take into account conflicting transactions. internal bool CanTransactionFitInPool(Transaction tx) { if (Count < Capacity) return true; @@ -293,15 +298,31 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) _txRwLock.EnterWriteLock(); try { - VerifyResult result = tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext); + if (!CheckConflicts(tx, out List conflictsToBeRemoved)) return VerifyResult.HasConflicts; + VerifyResult result = tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext, conflictsToBeRemoved.Select(c => c.Tx)); if (result != VerifyResult.Succeed) return result; _unsortedTransactions.Add(tx.Hash, poolItem); VerificationContext.AddTransaction(tx); _sortedTransactions.Add(poolItem); + foreach (var conflict in conflictsToBeRemoved) + { + if (TryRemoveVerified(conflict.Tx.Hash, out var _)) + VerificationContext.RemoveTransaction(conflict.Tx); + } + removedTransactions = conflictsToBeRemoved.Select(itm => itm.Tx).ToList(); + foreach (var attr in tx.GetAttributes()) + { + if (!_conflicts.TryGetValue(attr.Hash, out var pooled)) + { + pooled = new List(); + } + pooled.Add(tx.Hash); + _conflicts.AddOrSet(attr.Hash, pooled); + } if (Count > Capacity) - removedTransactions = RemoveOverCapacity(); + removedTransactions.AddRange(RemoveOverCapacity()); } finally { @@ -309,7 +330,7 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) } TransactionAdded?.Invoke(this, poolItem.Tx); - if (removedTransactions != null) + if (removedTransactions.Count() > 0) TransactionRemoved?.Invoke(this, new() { Transactions = removedTransactions, @@ -320,6 +341,44 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) return VerifyResult.Succeed; } + /// + /// Checks whether transaction conflicts with mempooled verified transactions + /// and can be added to the pool. If true, then transactions from conflictsList + /// should be removed from the verified structures (_unsortedTransactions and + /// _sortedTransactions). + /// + /// The that needs to be checked. + /// The list of conflicting verified transactions that should be removed from the pool if tx fits the pool. + /// True if transaction fits the pool, otherwise false. + private bool CheckConflicts(Transaction tx, out List conflictsList) + { + conflictsList = new(); + // Step 1: check if `tx` was in attributes of mempooled transactions. + if (_conflicts.TryGetValue(tx.Hash, out var conlicting)) + { + foreach (var hash in conlicting) + { + var existingTx = _unsortedTransactions[hash]; + if (existingTx.Tx.Signers.Select(s => s.Account).Contains(tx.Sender) && existingTx.Tx.NetworkFee > tx.NetworkFee) return false; + conflictsList.Add(existingTx); + } + } + // Step 2: check if mempooled transactions were in `tx`'s attributes. + foreach (var hash in tx.GetAttributes().Select(p => p.Hash)) + { + if (_unsortedTransactions.TryGetValue(hash, out PoolItem existingTx)) + { + if (!tx.Signers.Select(p => p.Account).Contains(existingTx.Tx.Sender)) return false; + if (existingTx.Tx.NetworkFee >= tx.NetworkFee) return false; + conflictsList.Add(existingTx); + } + } + // Step 3: take into account sender's conflicting transactions while balance check, + // this will be done in VerifyStateDependant. + + return true; + } + private List RemoveOverCapacity() { List removedTransactions = new(); @@ -332,7 +391,10 @@ private List RemoveOverCapacity() removedTransactions.Add(minItem.Tx); if (ReferenceEquals(sortedPool, _sortedTransactions)) + { + RemoveConflictsOfVerified(minItem); VerificationContext.RemoveTransaction(minItem.Tx); + } } while (Count > Capacity); return removedTransactions; @@ -347,9 +409,27 @@ private bool TryRemoveVerified(UInt256 hash, out PoolItem item) _unsortedTransactions.Remove(hash); _sortedTransactions.Remove(item); + RemoveConflictsOfVerified(item); + return true; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void RemoveConflictsOfVerified(PoolItem item) + { + foreach (var h in item.Tx.GetAttributes().Select(attr => attr.Hash)) + { + if (_conflicts.TryGetValue(h, out List conflicts)) + { + conflicts.Remove(item.Tx.Hash); + if (conflicts.Count() == 0) + { + _conflicts.Remove(h); + } + } + } + } + [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool TryRemoveUnVerified(UInt256 hash, out PoolItem item) { @@ -374,28 +454,60 @@ internal void InvalidateVerifiedTransactions() _unsortedTransactions.Clear(); VerificationContext = new TransactionVerificationContext(); _sortedTransactions.Clear(); + _conflicts.Clear(); } // Note: this must only be called from a single thread (the Blockchain actor) internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) { + var conflictingItems = new List(); _txRwLock.EnterWriteLock(); try { + HashSet conflicts = new HashSet(); // 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 _)) continue; - TryRemoveUnVerified(tx.Hash, out _); + if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerified(tx.Hash, out _); + foreach (var h in tx.GetAttributes().Select(a => a.Hash)) + { + conflicts.Add(h); + } } - // Add all the previously verified transactions back to the unverified transactions + // Then remove the transactions conflicting with the accepted ones. + // No need to modify VerificationContext as it will be reset afterwards. + var persisted = block.Transactions.Select(t => t.Hash); + var stale = new List(); + foreach (var item in _sortedTransactions) + { + if (conflicts.Contains(item.Tx.Hash) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Count() > 0) + { + stale.Add(item.Tx.Hash); + conflictingItems.Add(item.Tx); + } + } + foreach (var h in stale) + { + if (!TryRemoveVerified(h, out _)) TryRemoveUnVerified(h, out _); + } + + // Add all the previously verified transactions back to the unverified transactions and clear mempool conflicts list. InvalidateVerifiedTransactions(); } finally { _txRwLock.ExitWriteLock(); } + if (conflictingItems.Count() > 0) + { + TransactionRemoved?.Invoke(this, new() + { + Transactions = conflictingItems.ToArray(), + Reason = TransactionRemovalReason.Conflict, + }); + } // If we know about headers of future blocks, no point in verifying transactions from the unverified tx pool // until we get caught up. @@ -431,10 +543,31 @@ private int ReverifyTransactions(SortedSet verifiedSortedTxPool, // Since unverifiedSortedTxPool is ordered in an ascending manner, we take from the end. foreach (PoolItem item in unverifiedSortedTxPool.Reverse().Take(count)) { - if (item.Tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext) == VerifyResult.Succeed) + if (CheckConflicts(item.Tx, out List conflictsToBeRemoved) && + item.Tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext, conflictsToBeRemoved.Select(c => c.Tx)) == VerifyResult.Succeed) { reverifiedItems.Add(item); - VerificationContext.AddTransaction(item.Tx); + if (_unsortedTransactions.TryAdd(item.Tx.Hash, item)) + { + verifiedSortedTxPool.Add(item); + foreach (var attr in item.Tx.GetAttributes()) + { + if (!_conflicts.TryGetValue(attr.Hash, out var pooled)) + { + pooled = new List(); + } + pooled.Add(item.Tx.Hash); + _conflicts.AddOrSet(attr.Hash, pooled); + } + VerificationContext.AddTransaction(item.Tx); + foreach (var conflict in conflictsToBeRemoved) + { + if (TryRemoveVerified(conflict.Tx.Hash, out var _)) + VerificationContext.RemoveTransaction(conflict.Tx); + invalidItems.Add(conflict); + } + + } } else // Transaction no longer valid -- it will be removed from unverifiedTxPool. invalidItems.Add(item); @@ -450,18 +583,14 @@ private int ReverifyTransactions(SortedSet verifiedSortedTxPool, var rebroadcastCutOffTime = TimeProvider.Current.UtcNow.AddMilliseconds(-_system.Settings.MillisecondsPerBlock * blocksTillRebroadcast); foreach (PoolItem item in reverifiedItems) { - if (_unsortedTransactions.TryAdd(item.Tx.Hash, item)) + if (_unsortedTransactions.ContainsKey(item.Tx.Hash)) { - verifiedSortedTxPool.Add(item); - if (item.LastBroadcastTimestamp < rebroadcastCutOffTime) { _system.LocalNode.Tell(new LocalNode.RelayDirectly { Inventory = item.Tx }, _system.Blockchain); item.LastBroadcastTimestamp = TimeProvider.Current.UtcNow; } } - else - VerificationContext.RemoveTransaction(item.Tx); _unverifiedTransactions.Remove(item.Tx.Hash); unverifiedSortedTxPool.Remove(item); diff --git a/src/Neo/Ledger/TransactionRemovalReason.cs b/src/Neo/Ledger/TransactionRemovalReason.cs index 9767fb919f..fa66b60981 100644 --- a/src/Neo/Ledger/TransactionRemovalReason.cs +++ b/src/Neo/Ledger/TransactionRemovalReason.cs @@ -16,13 +16,18 @@ namespace Neo.Ledger public enum TransactionRemovalReason : byte { /// - /// The transaction was ejected since it was the lowest priority transaction and the memory pool capacity was exceeded. + /// The transaction was rejected since it was the lowest priority transaction and the memory pool capacity was exceeded. /// CapacityExceeded, /// - /// The transaction was ejected due to failing re-validation after a block was persisted. + /// The transaction was rejected due to failing re-validation after a block was persisted. /// NoLongerValid, + + /// + /// The transaction was rejected due to conflict with higher priority transactions with Conflicts attribute. + /// + Conflict, } } diff --git a/src/Neo/Ledger/TransactionVerificationContext.cs b/src/Neo/Ledger/TransactionVerificationContext.cs index b77adfe9c7..021f71ad9c 100644 --- a/src/Neo/Ledger/TransactionVerificationContext.cs +++ b/src/Neo/Ledger/TransactionVerificationContext.cs @@ -50,14 +50,19 @@ public void AddTransaction(Transaction tx) /// Determine whether the specified conflicts with other transactions. /// /// The specified . + /// The list of that conflicts with the specified one and are to be removed from the pool. /// The snapshot used to verify the . /// if the passes the check; otherwise, . - public bool CheckTransaction(Transaction tx, DataCache snapshot) + public bool CheckTransaction(Transaction tx, IEnumerable conflictingTxs, DataCache snapshot) { BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, tx.Sender); senderFee.TryGetValue(tx.Sender, out var totalSenderFeeFromPool); BigInteger fee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool; + foreach (var conflictTx in conflictingTxs) + { + fee -= conflictTx.NetworkFee + conflictTx.SystemFee; + } if (balance < fee) return false; var oracle = tx.GetAttribute(); diff --git a/src/Neo/Ledger/VerifyResult.cs b/src/Neo/Ledger/VerifyResult.cs index 7858903ad2..94c9e78bf8 100644 --- a/src/Neo/Ledger/VerifyResult.cs +++ b/src/Neo/Ledger/VerifyResult.cs @@ -77,6 +77,11 @@ public enum VerifyResult : byte /// PolicyFail, + /// + /// Indicates that the failed to verify because it conflicts with on-chain or mempooled transactions. + /// + HasConflicts, + /// /// Indicates that the failed to verify due to other reasons. /// diff --git a/src/Neo/Network/P2P/Payloads/Conflicts.cs b/src/Neo/Network/P2P/Payloads/Conflicts.cs new file mode 100644 index 0000000000..9ef42e8500 --- /dev/null +++ b/src/Neo/Network/P2P/Payloads/Conflicts.cs @@ -0,0 +1,44 @@ +using Neo.IO; +using Neo.Json; +using Neo.Persistence; +using Neo.SmartContract.Native; +using System.IO; + +namespace Neo.Network.P2P.Payloads +{ + public class Conflicts : TransactionAttribute + { + /// + /// Indicates the conflict transaction hash. + /// + public UInt256 Hash; + + public override TransactionAttributeType Type => TransactionAttributeType.Conflicts; + + public override bool AllowMultiple => true; + + public override int Size => base.Size + Hash.Size; + + protected override void DeserializeWithoutType(ref MemoryReader reader) + { + Hash = reader.ReadSerializable(); + } + + protected override void SerializeWithoutType(BinaryWriter writer) + { + writer.Write(Hash); + } + + public override JObject ToJson() + { + JObject json = base.ToJson(); + json["hash"] = Hash.ToString(); + return json; + } + + public override bool Verify(DataCache snapshot, Transaction tx) + { + return !NativeContract.Ledger.ContainsTransaction(snapshot, Hash); + } + } +} diff --git a/src/Neo/Network/P2P/Payloads/Transaction.cs b/src/Neo/Network/P2P/Payloads/Transaction.cs index b99449ac48..5babf5b822 100644 --- a/src/Neo/Network/P2P/Payloads/Transaction.cs +++ b/src/Neo/Network/P2P/Payloads/Transaction.cs @@ -338,12 +338,13 @@ public JObject ToJson(ProtocolSettings settings) /// The used to verify the transaction. /// The snapshot used to verify the transaction. /// The used to verify the transaction. + /// The list of conflicting those fee should be excluded from sender's overall fee during -based verification. /// The result of the verification. - public VerifyResult Verify(ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context) + public VerifyResult Verify(ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context, IEnumerable conflictsList) { VerifyResult result = VerifyStateIndependent(settings); if (result != VerifyResult.Succeed) return result; - return VerifyStateDependent(settings, snapshot, context); + return VerifyStateDependent(settings, snapshot, context, conflictsList); } /// @@ -352,8 +353,9 @@ public VerifyResult Verify(ProtocolSettings settings, DataCache snapshot, Transa /// The used to verify the transaction. /// The snapshot used to verify the transaction. /// The used to verify the transaction. + /// The list of conflicting those fee should be excluded from sender's overall fee during -based verification. /// The result of the verification. - public virtual VerifyResult VerifyStateDependent(ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context) + public virtual VerifyResult VerifyStateDependent(ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context, IEnumerable conflictsList) { uint height = NativeContract.Ledger.CurrentIndex(snapshot); if (ValidUntilBlock <= height || ValidUntilBlock > height + settings.MaxValidUntilBlockIncrement) @@ -362,7 +364,7 @@ public virtual VerifyResult VerifyStateDependent(ProtocolSettings settings, Data foreach (UInt160 hash in hashes) if (NativeContract.Policy.IsBlocked(snapshot, hash)) return VerifyResult.PolicyFail; - if (!(context?.CheckTransaction(this, snapshot) ?? true)) return VerifyResult.InsufficientFunds; + if (!(context?.CheckTransaction(this, conflictsList, snapshot) ?? true)) return VerifyResult.InsufficientFunds; foreach (TransactionAttribute attribute in Attributes) if (!attribute.Verify(snapshot, this)) return VerifyResult.InvalidAttribute; diff --git a/src/Neo/Network/P2P/Payloads/TransactionAttributeType.cs b/src/Neo/Network/P2P/Payloads/TransactionAttributeType.cs index 41c69b507d..66dccb340b 100644 --- a/src/Neo/Network/P2P/Payloads/TransactionAttributeType.cs +++ b/src/Neo/Network/P2P/Payloads/TransactionAttributeType.cs @@ -33,6 +33,12 @@ public enum TransactionAttributeType : byte /// Indicates that the transaction is not valid before . /// [ReflectionCache(typeof(NotValidBefore))] - NotValidBefore = 0x20 + NotValidBefore = 0x20, + + /// + /// Indicates that the transaction conflicts with . + /// + [ReflectionCache(typeof(Conflicts))] + Conflicts = 0x21 } } diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index eec49da99a..d686d4f3b2 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -38,6 +38,7 @@ internal override ContractTask OnPersist(ApplicationEngine engine) { TransactionState[] transactions = engine.PersistingBlock.Transactions.Select(p => new TransactionState { + Trimmed = false, BlockIndex = engine.PersistingBlock.Index, Transaction = p, State = VMState.NONE @@ -47,6 +48,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)); + foreach (var attr in tx.Transaction.GetAttributes()) + { + engine.Snapshot.GetOrAdd(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState { Trimmed = true })); + } } engine.SetState(transactions); return ContractTask.CompletedTask; @@ -220,7 +225,9 @@ public Header GetHeader(DataCache snapshot, uint index) /// The with the specified hash. public TransactionState GetTransactionState(DataCache snapshot, UInt256 hash) { - return snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); + var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); + if (state is not null && state.Trimmed) return null; + return state; } /// diff --git a/src/Neo/SmartContract/Native/TransactionState.cs b/src/Neo/SmartContract/Native/TransactionState.cs index 5c7380f1b8..538b474a7e 100644 --- a/src/Neo/SmartContract/Native/TransactionState.cs +++ b/src/Neo/SmartContract/Native/TransactionState.cs @@ -21,6 +21,11 @@ namespace Neo.SmartContract.Native /// public class TransactionState : IInteroperable { + /// + /// The transaction is trimmed. + /// To indicate this state is a placeholder for a conflict transaction. + /// + public bool Trimmed; /// /// The block containing this transaction. /// @@ -42,6 +47,7 @@ IInteroperable IInteroperable.Clone() { return new TransactionState { + Trimmed = Trimmed, BlockIndex = BlockIndex, Transaction = Transaction, State = State, @@ -52,6 +58,7 @@ IInteroperable IInteroperable.Clone() void IInteroperable.FromReplica(IInteroperable replica) { TransactionState from = (TransactionState)replica; + Trimmed = from.Trimmed; BlockIndex = from.BlockIndex; Transaction = from.Transaction; State = from.State; @@ -62,17 +69,20 @@ void IInteroperable.FromReplica(IInteroperable replica) void IInteroperable.FromStackItem(StackItem stackItem) { Struct @struct = (Struct)stackItem; - BlockIndex = (uint)@struct[0].GetInteger(); - _rawTransaction = ((ByteString)@struct[1]).Memory; + Trimmed = @struct[0].GetBoolean(); + if (Trimmed) return; + BlockIndex = (uint)@struct[1].GetInteger(); + _rawTransaction = ((ByteString)@struct[2]).Memory; Transaction = _rawTransaction.AsSerializable(); - State = (VMState)(byte)@struct[2].GetInteger(); + State = (VMState)(byte)@struct[3].GetInteger(); } StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter) { + if (Trimmed) return new Struct(referenceCounter) { Trimmed }; if (_rawTransaction.IsEmpty) _rawTransaction = Transaction.ToArray(); - return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State }; + return new Struct(referenceCounter) { Trimmed, BlockIndex, _rawTransaction, (byte)State }; } } } diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index 98f7b009fd..1a649f82a5 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -2,6 +2,7 @@ using FluentAssertions; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; +using Neo.Cryptography; using Neo.IO; using Neo.Ledger; using Neo.Network.P2P.Payloads; @@ -68,7 +69,7 @@ private Transaction CreateTransactionWithFee(long fee) var randomBytes = new byte[16]; random.NextBytes(randomBytes); Mock mock = new(); - mock.Setup(p => p.VerifyStateDependent(It.IsAny(), It.IsAny(), It.IsAny())).Returns(VerifyResult.Succeed); + mock.Setup(p => p.VerifyStateDependent(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny>())).Returns(VerifyResult.Succeed); mock.Setup(p => p.VerifyStateIndependent(It.IsAny())).Returns(VerifyResult.Succeed); mock.Object.Script = randomBytes; mock.Object.NetworkFee = fee; @@ -92,7 +93,7 @@ private Transaction CreateTransactionWithFeeAndBalanceVerify(long fee) random.NextBytes(randomBytes); Mock mock = new(); UInt160 sender = senderAccount; - mock.Setup(p => p.VerifyStateDependent(It.IsAny(), It.IsAny(), It.IsAny())).Returns((ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context) => context.CheckTransaction(mock.Object, snapshot) ? VerifyResult.Succeed : VerifyResult.InsufficientFunds); + mock.Setup(p => p.VerifyStateDependent(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny>())).Returns((ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context, IEnumerable conflictsList) => context.CheckTransaction(mock.Object, conflictsList, snapshot) ? VerifyResult.Succeed : VerifyResult.InsufficientFunds); mock.Setup(p => p.VerifyStateIndependent(It.IsAny())).Returns(VerifyResult.Succeed); mock.Object.Script = randomBytes; mock.Object.NetworkFee = fee; @@ -241,6 +242,168 @@ public async Task BlockPersistAndReverificationWillAbandonTxAsBalanceTransfered( _ = NativeContract.GAS.Mint(applicationEngine, sender, balance, true); } + [TestMethod] + public async Task UpdatePoolForBlockPersisted_RemoveBlockConflicts() + { + // Arrange: prepare mempooled and in-bock txs conflicting with each other. + 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()); + await NativeContract.GAS.Burn(engine, UInt160.Zero, balance); + _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 6, true); // balance enough for 6 mempooled txs + + var mp1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp1 doesn't conflict with anyone + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); + var tx1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // but in-block tx1 conflicts with mempooled mp1 => mp1 should be removed from pool after persist + tx1.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; + + var mp2 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp1 and mp2 don't conflict with anyone + _unit.TryAdd(mp2, engine.Snapshot); + var mp3 = CreateTransactionWithFeeAndBalanceVerify(txFee); + _unit.TryAdd(mp3, engine.Snapshot); + var tx2 = CreateTransactionWithFeeAndBalanceVerify(txFee); // in-block tx2 conflicts with mempooled mp2 and mp3 => mp2 and mp3 should be removed from pool after persist + tx2.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp2.Hash }, new Conflicts() { Hash = mp3.Hash } }; + + var tx3 = CreateTransactionWithFeeAndBalanceVerify(txFee); // in-block tx3 doesn't conflict with anyone + var mp4 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp4 conflicts with in-block tx3 => mp4 should be removed from pool after persist + mp4.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = tx3.Hash } }; + _unit.TryAdd(mp4, engine.Snapshot); + + var tx4 = CreateTransactionWithFeeAndBalanceVerify(txFee); // in-block tx4 and tx5 don't conflict with anyone + var tx5 = CreateTransactionWithFeeAndBalanceVerify(txFee); + var mp5 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp5 conflicts with in-block tx4 and tx5 => mp5 should be removed from pool after persist + mp5.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = tx4.Hash }, new Conflicts() { Hash = tx5.Hash } }; + _unit.TryAdd(mp5, engine.Snapshot); + + var mp6 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp6 doesn't conflict with anyone and noone conflicts with mp6 => mp6 should be left in the pool after persist + _unit.TryAdd(mp6, engine.Snapshot); + + _unit.SortedTxCount.Should().Be(6); + _unit.UnverifiedSortedTxCount.Should().Be(0); + + // Act: persist block and reverify all mempooled txs. + var block = new Block + { + Header = new Header(), + Transactions = new Transaction[] { tx1, tx2, tx3, tx4, tx5 }, + }; + _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.GetSortedVerifiedTransactions().Select(tx => tx.Hash).Should().Contain(mp6.Hash); + _unit.UnverifiedSortedTxCount.Should().Be(0); + + // Cleanup: revert the balance. + await NativeContract.GAS.Burn(engine, UInt160.Zero, txFee * 6); + _ = NativeContract.GAS.Mint(engine, UInt160.Zero, balance, true); + } + + [TestMethod] + public async Task TryAdd_AddRangeOfConflictingTransactions() + { + // 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()); + await NativeContract.GAS.Burn(engine, UInt160.Zero, balance); + _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 100, true); // balance enough for all mempooled txs + + var mp1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp1 doesn't conflict with anyone and not in the pool yet + + var mp2 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp2 conflicts with mp1 and has the same network fee + mp2.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; + _unit.TryAdd(mp2, engine.Snapshot); + + var mp3 = CreateTransactionWithFeeAndBalanceVerify(2 * txFee); // mp3 conflicts with mp1 and has larger network fee + mp3.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; + _unit.TryAdd(mp3, engine.Snapshot); + + var mp4 = CreateTransactionWithFeeAndBalanceVerify(3 * txFee); // mp4 conflicts with mp3 and has larger network fee + mp4.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp3.Hash } }; + + var malicious = CreateTransactionWithFeeAndBalanceVerify(3 * txFee); // malicious conflicts with mp3 and has larger network fee, but different sender + malicious.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp3.Hash } }; + malicious.Signers = new Signer[] { new Signer() { Account = new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })), Scopes = WitnessScope.None } }; + + var mp5 = CreateTransactionWithFeeAndBalanceVerify(2 * txFee); // mp5 conflicts with mp4 and has smaller network fee + mp5.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp4.Hash } }; + + _unit.SortedTxCount.Should().Be(2); + _unit.UnverifiedSortedTxCount.Should().Be(0); + + // Act & Assert: try to add conlflicting transactions to the pool. + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2 and mp3 but has lower network fee than mp3 => mp1 fails to be added + _unit.SortedTxCount.Should().Be(2); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp3 }); + + _unit.TryAdd(malicious, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // malicious conflicts with mp3, has larger network fee but malicious (different) sender => mp3 shoould be left in pool + _unit.SortedTxCount.Should().Be(2); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp3 }); + + _unit.TryAdd(mp4, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp4 conflicts with mp3 and has larger network fee => mp3 shoould be removed from pool + _unit.SortedTxCount.Should().Be(2); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp4 }); + + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2 and has same network fee => mp2 shoould be removed from pool + _unit.SortedTxCount.Should().Be(2); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); + + _unit.TryAdd(mp5, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2 and has same network fee => mp2 shoould be removed from pool + _unit.SortedTxCount.Should().Be(2); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); + + // Cleanup: revert the balance. + await NativeContract.GAS.Burn(engine, UInt160.Zero, 100); + _ = NativeContract.GAS.Mint(engine, UInt160.Zero, balance, true); + } + + [TestMethod] + public async Task TryRemoveVerified_RemoveVerifiedTxWithConflicts() + { + // 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()); + await NativeContract.GAS.Burn(engine, UInt160.Zero, balance); + _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 100, true); // balance enough for all mempooled txs + + var mp1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp1 doesn't conflict with anyone and not in the pool yet + + var mp2 = CreateTransactionWithFeeAndBalanceVerify(2 * txFee); // mp2 conflicts with mp1 and has larger same network fee + mp2.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; + _unit.TryAdd(mp2, engine.Snapshot); + + _unit.SortedTxCount.Should().Be(1); + _unit.UnverifiedSortedTxCount.Should().Be(0); + + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2 but has lower network fee + _unit.SortedTxCount.Should().Be(1); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2 }); + + // Act & Assert: try to invalidate verified transactions and push conflicting one. + _unit.InvalidateVerifiedTransactions(); + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2 but mp2 is not verified anymore + _unit.SortedTxCount.Should().Be(1); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1 }); + + var tx1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // in-block tx1 doesn't conflict with anyone and is aimed to trigger reverification + var block = new Block + { + Header = new Header(), + Transactions = new Transaction[] { tx1 }, + }; + _unit.UpdatePoolForBlockPersisted(block, engine.Snapshot); + _unit.SortedTxCount.Should().Be(1); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2 }); // after reverificaion mp2 should be back at verified list; mp1 should be completely kicked off + } + private static void VerifyTransactionsSortedDescending(IEnumerable transactions) { Transaction lastTransaction = null; diff --git a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs index db023d3eb8..1e3f765dd7 100644 --- a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs +++ b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs @@ -14,6 +14,8 @@ public class UT_TransactionState { TransactionState origin; + TransactionState originTrimmed; + [TestInitialize] public void Initialize() { @@ -31,6 +33,10 @@ public void Initialize() } } } }; + originTrimmed = new TransactionState() + { + Trimmed = true, + }; } [TestMethod] @@ -44,6 +50,21 @@ public void TestDeserialize() dest.BlockIndex.Should().Be(origin.BlockIndex); dest.Transaction.Hash.Should().Be(origin.Transaction.Hash); + dest.Trimmed.Should().Be(false); + } + + [TestMethod] + public void TestDeserializeTrimmed() + { + var data = BinarySerializer.Serialize(((IInteroperable)originTrimmed).ToStackItem(null), 1024); + var reader = new MemoryReader(data); + + TransactionState dest = new TransactionState(); + ((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null)); + + dest.BlockIndex.Should().Be(0); + dest.Transaction.Should().Be(null); + dest.Trimmed.Should().Be(true); } } } diff --git a/tests/Neo.UnitTests/Ledger/UT_TransactionVerificationContext.cs b/tests/Neo.UnitTests/Ledger/UT_TransactionVerificationContext.cs index 27d584dca9..522b964ff9 100644 --- a/tests/Neo.UnitTests/Ledger/UT_TransactionVerificationContext.cs +++ b/tests/Neo.UnitTests/Ledger/UT_TransactionVerificationContext.cs @@ -7,6 +7,7 @@ using Neo.SmartContract; using Neo.SmartContract.Native; using System; +using System.Collections.Generic; using System.Numerics; using System.Threading.Tasks; @@ -27,7 +28,7 @@ private Transaction CreateTransactionWithFee(long networkFee, long systemFee) var randomBytes = new byte[16]; random.NextBytes(randomBytes); Mock mock = new(); - mock.Setup(p => p.VerifyStateDependent(It.IsAny(), It.IsAny(), It.IsAny())).Returns(VerifyResult.Succeed); + mock.Setup(p => p.VerifyStateDependent(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny>())).Returns(VerifyResult.Succeed); mock.Setup(p => p.VerifyStateIndependent(It.IsAny())).Returns(VerifyResult.Succeed); mock.Object.Script = randomBytes; mock.Object.NetworkFee = networkFee; @@ -60,12 +61,13 @@ public async Task TestDuplicateOracle() TransactionVerificationContext verificationContext = new(); var tx = CreateTransactionWithFee(1, 2); tx.Attributes = new TransactionAttribute[] { new OracleResponse() { Code = OracleResponseCode.ConsensusUnreachable, Id = 1, Result = Array.Empty() } }; - verificationContext.CheckTransaction(tx, snapshot).Should().BeTrue(); + var conflicts = new List(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); verificationContext.AddTransaction(tx); tx = CreateTransactionWithFee(2, 1); tx.Attributes = new TransactionAttribute[] { new OracleResponse() { Code = OracleResponseCode.ConsensusUnreachable, Id = 1, Result = Array.Empty() } }; - verificationContext.CheckTransaction(tx, snapshot).Should().BeFalse(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeFalse(); } [TestMethod] @@ -79,15 +81,40 @@ public async Task TestTransactionSenderFee() TransactionVerificationContext verificationContext = new(); var tx = CreateTransactionWithFee(1, 2); - verificationContext.CheckTransaction(tx, snapshot).Should().BeTrue(); + var conflicts = new List(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); verificationContext.AddTransaction(tx); - verificationContext.CheckTransaction(tx, snapshot).Should().BeTrue(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); verificationContext.AddTransaction(tx); - verificationContext.CheckTransaction(tx, snapshot).Should().BeFalse(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeFalse(); verificationContext.RemoveTransaction(tx); - verificationContext.CheckTransaction(tx, snapshot).Should().BeTrue(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); verificationContext.AddTransaction(tx); - verificationContext.CheckTransaction(tx, snapshot).Should().BeFalse(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeFalse(); + } + + [TestMethod] + public async Task TestTransactionSenderFeeWithConflicts() + { + var snapshot = TestBlockchain.GetTestSnapshot(); + ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, settings: TestBlockchain.TheNeoSystem.Settings, gas: long.MaxValue); + BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, UInt160.Zero); + await NativeContract.GAS.Burn(engine, UInt160.Zero, balance); + _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 3 + 3 + 1, true); // balance is enough for 2 transactions and 1 GAS is left. + + TransactionVerificationContext verificationContext = new(); + var tx = CreateTransactionWithFee(1, 2); + var conflictingTx = CreateTransactionWithFee(1, 1); // costs 2 GAS + + var conflicts = new List(); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); + verificationContext.AddTransaction(tx); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); + verificationContext.AddTransaction(tx); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeFalse(); + + conflicts.Add(conflictingTx); + verificationContext.CheckTransaction(tx, conflicts, snapshot).Should().BeTrue(); // 1 GAS is left on the balance + 2 GAS is free after conflicts removal => enough for one more trasnaction. } } } diff --git a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs new file mode 100644 index 0000000000..1e5e4ddad8 --- /dev/null +++ b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs @@ -0,0 +1,74 @@ +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Neo.IO; +using Neo.Network.P2P.Payloads; +using Neo.SmartContract.Native; +using Neo.SmartContract; +using System; + +namespace Neo.UnitTests.Network.P2P.Payloads +{ + [TestClass] + public class UT_Conflicts + { + private const byte Prefix_Transaction = 11; + private static UInt256 _u = new UInt256(new byte[32] { + 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, + 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, + 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, + 0x01, 0x01 + }); + + [TestMethod] + public void Size_Get() + { + var test = new Conflicts() { Hash = _u }; + test.Size.Should().Be(1 + 32); + } + + [TestMethod] + public void ToJson() + { + var test = new Conflicts() { Hash = _u }; + var json = test.ToJson().ToString(); + Assert.AreEqual(@"{""type"":""Conflicts"",""hash"":""0x0101010101010101010101010101010101010101010101010101010101010101""}", json); + } + + [TestMethod] + public void DeserializeAndSerialize() + { + var test = new Conflicts() { Hash = _u }; + + var clone = test.ToArray().AsSerializable(); + Assert.AreEqual(clone.Type, test.Type); + + // As transactionAttribute + byte[] buffer = test.ToArray(); + var reader = new MemoryReader(buffer); + clone = TransactionAttribute.DeserializeFrom(ref reader) as Conflicts; + Assert.AreEqual(clone.Type, test.Type); + + // Wrong type + buffer[0] = 0xff; + Assert.ThrowsException(() => + { + var reader = new MemoryReader(buffer); + TransactionAttribute.DeserializeFrom(ref reader); + }); + } + + [TestMethod] + public void Verify() + { + var test = new Conflicts() { Hash = _u }; + var snapshot = TestBlockchain.GetTestSnapshot(); + var key = Ledger.UT_MemoryPool.CreateStorageKey(NativeContract.Ledger.Id, Prefix_Transaction, _u.ToArray()); + + snapshot.Add(key, new StorageItem()); + Assert.IsFalse(test.Verify(snapshot, new Transaction())); + + snapshot.Delete(key); + Assert.IsTrue(test.Verify(snapshot, new Transaction())); + } + } +} diff --git a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs index 98c49f31c4..1df1340012 100644 --- a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs +++ b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Transaction.cs @@ -10,6 +10,7 @@ using Neo.VM; using Neo.Wallets; using System; +using System.Collections.Generic; using System.Linq; using System.Numerics; @@ -773,7 +774,7 @@ public void Transaction_Reverify_Hashes_Length_Unequal_To_Witnesses_Length() }; UInt160[] hashes = txSimple.GetScriptHashesForVerifying(snapshot); Assert.AreEqual(1, hashes.Length); - Assert.AreNotEqual(VerifyResult.Succeed, txSimple.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext())); + Assert.AreNotEqual(VerifyResult.Succeed, txSimple.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext(), new List())); } [TestMethod] @@ -1204,11 +1205,12 @@ public void Test_VerifyStateDependent() var key = NativeContract.GAS.CreateStorageKey(20, tx.Sender); var balance = snapshot.GetAndChange(key, () => new StorageItem(new AccountState())); balance.GetInteroperable().Balance = tx.NetworkFee; + var conflicts = new List(); - tx.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext()).Should().Be(VerifyResult.Invalid); + tx.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext(), conflicts).Should().Be(VerifyResult.Invalid); balance.GetInteroperable().Balance = 0; tx.SystemFee = 10; - tx.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext()).Should().Be(VerifyResult.InsufficientFunds); + tx.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext(), conflicts).Should().Be(VerifyResult.InsufficientFunds); var walletA = TestUtils.GenerateTestWallet("123"); var walletB = TestUtils.GenerateTestWallet("123"); @@ -1254,7 +1256,7 @@ public void Test_VerifyStateDependent() Assert.IsTrue(data.Completed); tx.Witnesses = data.GetWitnesses(); - tx.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext()).Should().Be(VerifyResult.Succeed); + tx.VerifyStateDependent(ProtocolSettings.Default, snapshot, new TransactionVerificationContext(), new List()).Should().Be(VerifyResult.Succeed); } [TestMethod] From e42dd05e855a6c5cb1f0bd736aa285868242c089 Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 7 Mar 2023 22:21:09 +0100 Subject: [PATCH 02/19] Update src/Neo/Ledger/MemoryPool.cs --- src/Neo/Ledger/MemoryPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 6ac64fde68..0c6f6cbbcd 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -482,7 +482,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) var stale = new List(); foreach (var item in _sortedTransactions) { - if (conflicts.Contains(item.Tx.Hash) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Count() > 0) + if (conflicts.Contains(item.Tx.Hash) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) { stale.Add(item.Tx.Hash); conflictingItems.Add(item.Tx); From 9c4bedf233240085776152858903a1a0cb3bd62c Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 9 Mar 2023 14:11:10 +0300 Subject: [PATCH 03/19] Fix conflicting tx fees accounting Fix the comment https://github.com/neo-project/neo/pull/2818#discussion_r1130790669. --- src/Neo/Ledger/TransactionVerificationContext.cs | 3 ++- src/Neo/Network/P2P/Payloads/Transaction.cs | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Neo/Ledger/TransactionVerificationContext.cs b/src/Neo/Ledger/TransactionVerificationContext.cs index 021f71ad9c..03ae0ca6ec 100644 --- a/src/Neo/Ledger/TransactionVerificationContext.cs +++ b/src/Neo/Ledger/TransactionVerificationContext.cs @@ -12,6 +12,7 @@ using Neo.Persistence; using Neo.SmartContract.Native; using System.Collections.Generic; +using System.Linq; using System.Numerics; namespace Neo.Ledger @@ -59,7 +60,7 @@ public bool CheckTransaction(Transaction tx, IEnumerable conflictin senderFee.TryGetValue(tx.Sender, out var totalSenderFeeFromPool); BigInteger fee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool; - foreach (var conflictTx in conflictingTxs) + foreach (var conflictTx in conflictingTxs.Where(c => c.Sender.Equals(tx.Sender))) { fee -= conflictTx.NetworkFee + conflictTx.SystemFee; } diff --git a/src/Neo/Network/P2P/Payloads/Transaction.cs b/src/Neo/Network/P2P/Payloads/Transaction.cs index 5babf5b822..24581e2b18 100644 --- a/src/Neo/Network/P2P/Payloads/Transaction.cs +++ b/src/Neo/Network/P2P/Payloads/Transaction.cs @@ -338,7 +338,7 @@ public JObject ToJson(ProtocolSettings settings) /// The used to verify the transaction. /// The snapshot used to verify the transaction. /// The used to verify the transaction. - /// The list of conflicting those fee should be excluded from sender's overall fee during -based verification. + /// The list of conflicting those fee should be excluded from sender's overall fee during -based verification in case of sender's match. /// The result of the verification. public VerifyResult Verify(ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context, IEnumerable conflictsList) { @@ -353,7 +353,7 @@ public VerifyResult Verify(ProtocolSettings settings, DataCache snapshot, Transa /// The used to verify the transaction. /// The snapshot used to verify the transaction. /// The used to verify the transaction. - /// The list of conflicting those fee should be excluded from sender's overall fee during -based verification. + /// The list of conflicting those fee should be excluded from sender's overall fee during -based verification in case of sender's match. /// The result of the verification. public virtual VerifyResult VerifyStateDependent(ProtocolSettings settings, DataCache snapshot, TransactionVerificationContext context, IEnumerable conflictsList) { From 4d54d6018c8f04d3c6c7d1f9e8837fbe179739a9 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 6 Apr 2023 17:28:35 +0300 Subject: [PATCH 04/19] Reformat code and improve variables naming Fix Vitor's comments. --- src/Neo/Ledger/TransactionVerificationContext.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Neo/Ledger/TransactionVerificationContext.cs b/src/Neo/Ledger/TransactionVerificationContext.cs index 03ae0ca6ec..3a1a91b71a 100644 --- a/src/Neo/Ledger/TransactionVerificationContext.cs +++ b/src/Neo/Ledger/TransactionVerificationContext.cs @@ -59,12 +59,12 @@ public bool CheckTransaction(Transaction tx, IEnumerable conflictin BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, tx.Sender); senderFee.TryGetValue(tx.Sender, out var totalSenderFeeFromPool); - BigInteger fee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool; + BigInteger expectedFee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool; foreach (var conflictTx in conflictingTxs.Where(c => c.Sender.Equals(tx.Sender))) { - fee -= conflictTx.NetworkFee + conflictTx.SystemFee; + expectedFee -= (conflictTx.NetworkFee + conflictTx.SystemFee); } - if (balance < fee) return false; + if (balance < expectedFee) return false; var oracle = tx.GetAttribute(); if (oracle != null && oracleResponses.ContainsKey(oracle.Id)) From 1ac2913bcc48a34df1e895f4374c6e155c975207 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 22 May 2023 13:58:43 +0300 Subject: [PATCH 05/19] Make comments more clear and adjust variables naming Fix Owen's feedback. --- src/Neo/Ledger/MemoryPool.cs | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 0c6f6cbbcd..6fc9a87be0 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -342,35 +342,34 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) } /// - /// Checks whether transaction conflicts with mempooled verified transactions - /// and can be added to the pool. If true, then transactions from conflictsList - /// should be removed from the verified structures (_unsortedTransactions and - /// _sortedTransactions). + /// Checks whether there is no mismatch in Conflicts attributes between the current transaction + /// and mempooled unsorted transactions. If true, then these unsorted transactions will be added + /// into conflictsList. /// - /// The that needs to be checked. + /// The current transaction needs to be checked. /// The list of conflicting verified transactions that should be removed from the pool if tx fits the pool. /// True if transaction fits the pool, otherwise false. private bool CheckConflicts(Transaction tx, out List conflictsList) { conflictsList = new(); - // Step 1: check if `tx` was in attributes of mempooled transactions. - if (_conflicts.TryGetValue(tx.Hash, out var conlicting)) + // Step 1: check if `tx` was in Conflicts attributes of unsorted transactions. + if (_conflicts.TryGetValue(tx.Hash, out var conflicting)) { - foreach (var hash in conlicting) + foreach (var hash in conflicting) { - var existingTx = _unsortedTransactions[hash]; - if (existingTx.Tx.Signers.Select(s => s.Account).Contains(tx.Sender) && existingTx.Tx.NetworkFee > tx.NetworkFee) return false; - conflictsList.Add(existingTx); + var unsortedTx = _unsortedTransactions[hash]; + if (unsortedTx.Tx.Signers.Select(s => s.Account).Contains(tx.Sender) && unsortedTx.Tx.NetworkFee > tx.NetworkFee) return false; + conflictsList.Add(unsortedTx); } } - // Step 2: check if mempooled transactions were in `tx`'s attributes. + // Step 2: check if unsorted transactions were in `tx`'s Conflicts attributes. foreach (var hash in tx.GetAttributes().Select(p => p.Hash)) { - if (_unsortedTransactions.TryGetValue(hash, out PoolItem existingTx)) + if (_unsortedTransactions.TryGetValue(hash, out PoolItem unsortedTx)) { - if (!tx.Signers.Select(p => p.Account).Contains(existingTx.Tx.Sender)) return false; - if (existingTx.Tx.NetworkFee >= tx.NetworkFee) return false; - conflictsList.Add(existingTx); + if (!tx.Signers.Select(p => p.Account).Contains(unsortedTx.Tx.Sender)) return false; + if (unsortedTx.Tx.NetworkFee >= tx.NetworkFee) return false; + conflictsList.Add(unsortedTx); } } // Step 3: take into account sender's conflicting transactions while balance check, From 5798354e390ac1694f8cc1c030e7448de6e51b79 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 25 May 2023 11:51:04 +0300 Subject: [PATCH 06/19] Fix Conflicts attribute verification Consider the case when transaction B has the hash of transaction A in the Conflicts attribute and transaction B is on-chain. Let the transaction C has the hash of transaction A in the Conflicts attribute. Then transaction C still should be able to pass verification and should be accepted to the subsequent block, because transaction A isn't on chain and transaction A will never be accepted. Thanks to Owen for testing, this case is described at the https://github.com/neo-project/neo/pull/2818#pullrequestreview-1443230800. The expected behaviour in the described case is that TXID-D and TXID-E will be successfully accepted to the mempool and to chain, because the conflicting TXID-A is not on chain (and it's OK that the hash of TXID-A is on-chain as the conflicting hash). --- src/Neo/Ledger/Blockchain.cs | 8 ++++++-- src/Neo/NeoSystem.cs | 10 ++++++++++ src/Neo/Network/P2P/Payloads/Conflicts.cs | 3 +++ .../Network/P2P/RemoteNode.ProtocolHandler.cs | 4 ++-- src/Neo/SmartContract/Native/LedgerContract.cs | 17 ++++++++++++++++- .../Network/P2P/Payloads/UT_Conflicts.cs | 18 +++++++++++++++++- 6 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/Neo/Ledger/Blockchain.cs b/src/Neo/Ledger/Blockchain.cs index 52b3c37ce5..9f4c33c3af 100644 --- a/src/Neo/Ledger/Blockchain.cs +++ b/src/Neo/Ledger/Blockchain.cs @@ -206,6 +206,8 @@ private void OnFillMemoryPool(IEnumerable transactions) { if (NativeContract.Ledger.ContainsTransaction(snapshot, tx.Hash)) continue; + if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash)) + continue; // First remove the tx if it is unverified in the pool. system.MemPool.TryRemoveUnVerified(tx.Hash, out _); // Add to the memory pool @@ -337,6 +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; return system.MemPool.TryAdd(transaction, system.StoreView); } @@ -391,8 +394,9 @@ private void OnTransaction(Transaction tx) { if (system.ContainsTransaction(tx.Hash)) SendRelayResult(tx, VerifyResult.AlreadyExists); - else - system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true)); + else if (system.ContainsConflictHash(tx.Hash)) + SendRelayResult(tx, VerifyResult.HasConflicts); + else system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true)); } private void Persist(Block block) diff --git a/src/Neo/NeoSystem.cs b/src/Neo/NeoSystem.cs index c87b9e2630..b715f98382 100644 --- a/src/Neo/NeoSystem.cs +++ b/src/Neo/NeoSystem.cs @@ -278,5 +278,15 @@ public bool ContainsTransaction(UInt256 hash) if (MemPool.ContainsKey(hash)) return true; return NativeContract.Ledger.ContainsTransaction(StoreView, hash); } + + /// + /// Determines whether the specified transaction conflicts with some on-chain transaction. + /// + /// The hash of the transaction + /// if the transaction conflicts with on-chain transaction; otherwise, . + public bool ContainsConflictHash(UInt256 hash) + { + return NativeContract.Ledger.ContainsConflictHash(StoreView, hash); + } } } diff --git a/src/Neo/Network/P2P/Payloads/Conflicts.cs b/src/Neo/Network/P2P/Payloads/Conflicts.cs index 9ef42e8500..db00b03259 100644 --- a/src/Neo/Network/P2P/Payloads/Conflicts.cs +++ b/src/Neo/Network/P2P/Payloads/Conflicts.cs @@ -38,6 +38,9 @@ public override JObject ToJson() public override bool Verify(DataCache snapshot, Transaction tx) { + // Only check if conflicting transaction is on chain. It's OK if the + // conflicting transaction was in the Conflicts attribute of some other + // on-chain transaction. return !NativeContract.Ledger.ContainsTransaction(snapshot, Hash); } } diff --git a/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs b/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs index dfd21c63b8..37c12b38dc 100644 --- a/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs +++ b/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)) + if (!(system.ContainsTransaction(transaction.Hash) || system.ContainsConflictHash(transaction.Hash))) 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)).ToArray(); + hashes = hashes.Where(p => !(NativeContract.Ledger.ContainsTransaction(snapshot, p) || NativeContract.Ledger.ContainsConflictHash(snapshot, p))).ToArray(); } break; } diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index d686d4f3b2..9dbb67aa74 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -131,7 +131,22 @@ public bool ContainsBlock(DataCache snapshot, UInt256 hash) /// if the blockchain contains the transaction; otherwise, . public bool ContainsTransaction(DataCache snapshot, UInt256 hash) { - return snapshot.Contains(CreateStorageKey(Prefix_Transaction).Add(hash)); + var txState = GetTransactionState(snapshot, hash); + return txState != null; + } + + /// + /// Determine whether the specified transaction hash is contained in the blockchain + /// as the hash of conflicting transaction. + /// + /// The snapshot used to read data. + /// The hash of the conflicting transaction. + /// if the blockchain contains the hash of the conflicting transaction; otherwise, . + public bool ContainsConflictHash(DataCache snapshot, UInt256 hash) + { + var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); + if (state is not null && state.Trimmed) return true; + return false; } /// diff --git a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs index 1e5e4ddad8..c955995bb1 100644 --- a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs +++ b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs @@ -4,6 +4,7 @@ using Neo.Network.P2P.Payloads; using Neo.SmartContract.Native; using Neo.SmartContract; +using Neo.VM; using System; namespace Neo.UnitTests.Network.P2P.Payloads @@ -64,9 +65,24 @@ public void Verify() var snapshot = TestBlockchain.GetTestSnapshot(); var key = Ledger.UT_MemoryPool.CreateStorageKey(NativeContract.Ledger.Id, Prefix_Transaction, _u.ToArray()); - snapshot.Add(key, new StorageItem()); + // Conflicting transaction is in the Conflicts attribute of some other on-chain transaction. + var conflict = new TransactionState { Trimmed = true }; + snapshot.Add(key, new StorageItem(conflict)); + Assert.IsTrue(test.Verify(snapshot, new Transaction())); + + // Conflicting transaction is on-chain. + snapshot.Delete(key); + conflict = new TransactionState + { + Trimmed = false, + BlockIndex = 123, + Transaction = new Transaction(), + State = VMState.NONE + }; + snapshot.Add(key, new StorageItem(conflict)); Assert.IsFalse(test.Verify(snapshot, new Transaction())); + // There's no conflicting transaction at all. snapshot.Delete(key); Assert.IsTrue(test.Verify(snapshot, new Transaction())); } From 658aafe9138d8b059eae1818215371483d481a29 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 26 May 2023 15:21:13 +0300 Subject: [PATCH 07/19] Fix formatting, comments and add a testcase for conflicting mempool txs --- src/Neo/Ledger/TransactionVerificationContext.cs | 2 -- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 8 ++++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/Neo/Ledger/TransactionVerificationContext.cs b/src/Neo/Ledger/TransactionVerificationContext.cs index 3a1a91b71a..5b6cc6cc9c 100644 --- a/src/Neo/Ledger/TransactionVerificationContext.cs +++ b/src/Neo/Ledger/TransactionVerificationContext.cs @@ -61,9 +61,7 @@ public bool CheckTransaction(Transaction tx, IEnumerable conflictin BigInteger expectedFee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool; foreach (var conflictTx in conflictingTxs.Where(c => c.Sender.Equals(tx.Sender))) - { expectedFee -= (conflictTx.NetworkFee + conflictTx.SystemFee); - } if (balance < expectedFee) return false; var oracle = tx.GetAttribute(); diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index 1a649f82a5..8cd64eed24 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -349,11 +349,15 @@ public async Task TryAdd_AddRangeOfConflictingTransactions() _unit.SortedTxCount.Should().Be(2); _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp4 }); - _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2 and has same network fee => mp2 shoould be removed from pool + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2 and has same network fee, mp2 has Conflicts attribute => mp2 should be removed from pool _unit.SortedTxCount.Should().Be(2); _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); - _unit.TryAdd(mp5, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2 and has same network fee => mp2 shoould be removed from pool + _unit.TryAdd(mp2, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp2 conflicts with mp1 and has same network fee, mp2 has Conflicts attribute => mp2 shoouldn't be added to the pool + _unit.SortedTxCount.Should().Be(2); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); + + _unit.TryAdd(mp5, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp5 conflicts with mp4 and has smaller network fee => mp5 fails to be added _unit.SortedTxCount.Should().Be(2); _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); From 1dbf96507470041e68be97d939e144b163817f8c Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 29 May 2023 16:55:03 +0300 Subject: [PATCH 08/19] Add one more testcase for conflicting transactions Testcase provided by Vitor in https://github.com/neo-project/neo/pull/2818#discussion_r1209313799. --- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 29 ++++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index 8cd64eed24..2d8d378b16 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -315,9 +315,12 @@ public async Task TryAdd_AddRangeOfConflictingTransactions() var mp1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp1 doesn't conflict with anyone and not in the pool yet - var mp2 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp2 conflicts with mp1 and has the same network fee - mp2.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; - _unit.TryAdd(mp2, engine.Snapshot); + var mp2_1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp2_1 conflicts with mp1 and has the same network fee + mp2_1.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; + _unit.TryAdd(mp2_1, engine.Snapshot); + var mp2_2 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp2_2 also conflicts with mp1 and has the same network fee as mp1 and mp2_1 + mp2_2.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; + _unit.TryAdd(mp2_2, engine.Snapshot); var mp3 = CreateTransactionWithFeeAndBalanceVerify(2 * txFee); // mp3 conflicts with mp1 and has larger network fee mp3.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp1.Hash } }; @@ -333,27 +336,27 @@ public async Task TryAdd_AddRangeOfConflictingTransactions() var mp5 = CreateTransactionWithFeeAndBalanceVerify(2 * txFee); // mp5 conflicts with mp4 and has smaller network fee mp5.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp4.Hash } }; - _unit.SortedTxCount.Should().Be(2); + _unit.SortedTxCount.Should().Be(3); _unit.UnverifiedSortedTxCount.Should().Be(0); // Act & Assert: try to add conlflicting transactions to the pool. - _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2 and mp3 but has lower network fee than mp3 => mp1 fails to be added - _unit.SortedTxCount.Should().Be(2); - _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp3 }); + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2_1, mp2_2 and mp3 but has lower network fee than mp3 => mp1 fails to be added + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2_1, mp2_2, mp3 }); _unit.TryAdd(malicious, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // malicious conflicts with mp3, has larger network fee but malicious (different) sender => mp3 shoould be left in pool - _unit.SortedTxCount.Should().Be(2); - _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp3 }); + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2_1, mp2_2, mp3 }); _unit.TryAdd(mp4, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp4 conflicts with mp3 and has larger network fee => mp3 shoould be removed from pool - _unit.SortedTxCount.Should().Be(2); - _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2, mp4 }); + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2_1, mp2_2, mp4 }); - _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2 and has same network fee, mp2 has Conflicts attribute => mp2 should be removed from pool + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2_1 and mp2_2 and has same network fee, mp2_1 and mp2_2 have Conflicts attribute => mp2_1 and mp2_2 should be removed from pool _unit.SortedTxCount.Should().Be(2); _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); - _unit.TryAdd(mp2, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp2 conflicts with mp1 and has same network fee, mp2 has Conflicts attribute => mp2 shoouldn't be added to the pool + _unit.TryAdd(mp2_1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp2_1 conflicts with mp1 and has same network fee, mp2_1 has Conflicts attribute => mp2_1 shoouldn't be added to the pool _unit.SortedTxCount.Should().Be(2); _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); From 6c9b0f577edc93684ff34c8af8e2ec2fa83fc65d Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 1 Jun 2023 13:04:13 +0300 Subject: [PATCH 09/19] Implement economic adjustments for Conflicts attribute Transaction that conflicts with mempooled transactions have to pay larger network fee than the sum of all conflicting transactions in the pool that have the same sender as the newcomer. Port the https://github.com/nspcc-dev/neo-go/pull/3031. --- src/Neo/Ledger/MemoryPool.cs | 10 ++++- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 49 +++++++++++++++++---- 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 6fc9a87be0..5769d4181a 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -352,13 +352,15 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) private bool CheckConflicts(Transaction tx, out List conflictsList) { conflictsList = new(); + long conflictsFeeSum = 0; // Step 1: check if `tx` was in Conflicts attributes of unsorted transactions. if (_conflicts.TryGetValue(tx.Hash, out var conflicting)) { foreach (var hash in conflicting) { var unsortedTx = _unsortedTransactions[hash]; - if (unsortedTx.Tx.Signers.Select(s => s.Account).Contains(tx.Sender) && unsortedTx.Tx.NetworkFee > tx.NetworkFee) return false; + if (unsortedTx.Tx.Signers.Select(s => s.Account).Contains(tx.Sender)) + conflictsFeeSum += unsortedTx.Tx.NetworkFee; conflictsList.Add(unsortedTx); } } @@ -368,10 +370,14 @@ private bool CheckConflicts(Transaction tx, out List conflictsList) if (_unsortedTransactions.TryGetValue(hash, out PoolItem unsortedTx)) { if (!tx.Signers.Select(p => p.Account).Contains(unsortedTx.Tx.Sender)) return false; - if (unsortedTx.Tx.NetworkFee >= tx.NetworkFee) return false; + conflictsFeeSum += unsortedTx.Tx.NetworkFee; conflictsList.Add(unsortedTx); } } + // Network fee of tx have to be larger than the sum of conflicting txs network fees. + if (conflictsFeeSum != 0 && conflictsFeeSum >= tx.NetworkFee) + return false; + // Step 3: take into account sender's conflicting transactions while balance check, // this will be done in VerifyStateDependant. diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index 2d8d378b16..da07d148ee 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -306,12 +306,14 @@ public async Task TryAdd_AddRangeOfConflictingTransactions() { // Arrange: prepare mempooled txs that have conflicts. long txFee = 1; + var maliciousSender = new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })); 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()); await NativeContract.GAS.Burn(engine, UInt160.Zero, balance); _ = NativeContract.GAS.Mint(engine, UInt160.Zero, 100, true); // balance enough for all mempooled txs + _ = NativeContract.GAS.Mint(engine, maliciousSender, 100, true); // balance enough for all mempooled txs var mp1 = CreateTransactionWithFeeAndBalanceVerify(txFee); // mp1 doesn't conflict with anyone and not in the pool yet @@ -336,6 +338,18 @@ public async Task TryAdd_AddRangeOfConflictingTransactions() var mp5 = CreateTransactionWithFeeAndBalanceVerify(2 * txFee); // mp5 conflicts with mp4 and has smaller network fee mp5.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp4.Hash } }; + var mp6 = CreateTransactionWithFeeAndBalanceVerify(mp2_1.NetworkFee + mp2_2.NetworkFee + 1); // mp6 conflicts with mp2_1 and mp2_2 and has larger network fee. + mp6.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp2_1.Hash }, new Conflicts() { Hash = mp2_2.Hash } }; + + var mp7 = CreateTransactionWithFeeAndBalanceVerify(txFee * 2 + 1); // mp7 doesn't conflicts with anyone, but mp8, mp9 and mp10malicious has smaller sum network fee and conflict with mp7. + var mp8 = CreateTransactionWithFeeAndBalanceVerify(txFee); + mp8.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp7.Hash } }; + var mp9 = CreateTransactionWithFeeAndBalanceVerify(txFee); + mp9.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp7.Hash } }; + var mp10malicious = CreateTransactionWithFeeAndBalanceVerify(txFee); + mp10malicious.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = mp7.Hash } }; + mp10malicious.Signers = new Signer[] { new Signer() { Account = maliciousSender, Scopes = WitnessScope.None } }; + _unit.SortedTxCount.Should().Be(3); _unit.UnverifiedSortedTxCount.Should().Be(0); @@ -352,21 +366,40 @@ public async Task TryAdd_AddRangeOfConflictingTransactions() _unit.SortedTxCount.Should().Be(3); _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2_1, mp2_2, mp4 }); - _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2_1 and mp2_2 and has same network fee, mp2_1 and mp2_2 have Conflicts attribute => mp2_1 and mp2_2 should be removed from pool - _unit.SortedTxCount.Should().Be(2); - _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp1 conflicts with mp2_1 and mp2_2 and has same network fee => mp2_1 and mp2_2 should be left in pool. + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2_1, mp2_2, mp4 }); - _unit.TryAdd(mp2_1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp2_1 conflicts with mp1 and has same network fee, mp2_1 has Conflicts attribute => mp2_1 shoouldn't be added to the pool + _unit.TryAdd(mp6, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp6 conflicts with mp2_1 and mp2_2 and has larger network fee than the sum of mp2_1 and mp2_2 fees => mp6 should be added. _unit.SortedTxCount.Should().Be(2); - _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp6, mp4 }); - _unit.TryAdd(mp5, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp5 conflicts with mp4 and has smaller network fee => mp5 fails to be added - _unit.SortedTxCount.Should().Be(2); - _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp4 }); + _unit.TryAdd(mp1, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp1 conflicts with mp2_1 and mp2_2, but they are not in the pool now => mp1 should be added. + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp6, mp4 }); + + _unit.TryAdd(mp2_1, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp2_1 conflicts with mp1 and has same network fee => mp2_1 shouldn't be added to the pool. + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp6, mp4 }); + + _unit.TryAdd(mp5, engine.Snapshot).Should().Be(VerifyResult.HasConflicts); // mp5 conflicts with mp4 and has smaller network fee => mp5 fails to be added. + _unit.SortedTxCount.Should().Be(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp6, mp4 }); + + _unit.TryAdd(mp8, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp8, mp9 and mp10malicious conflict with mp7, but mo7 is not in the pool yet. + _unit.TryAdd(mp9, engine.Snapshot).Should().Be(VerifyResult.Succeed); + _unit.TryAdd(mp10malicious, engine.Snapshot).Should().Be(VerifyResult.Succeed); + _unit.SortedTxCount.Should().Be(6); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp6, mp4, mp8, mp9, mp10malicious }); + _unit.TryAdd(mp7, engine.Snapshot).Should().Be(VerifyResult.Succeed); // mp7 has larger network fee than the sum of mp8 and mp9 fees => should be added to the pool. + _unit.SortedTxCount.Should().Be(4); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp1, mp6, mp4, mp7 }); // Cleanup: revert the balance. await NativeContract.GAS.Burn(engine, UInt160.Zero, 100); _ = NativeContract.GAS.Mint(engine, UInt160.Zero, balance, true); + await NativeContract.GAS.Burn(engine, maliciousSender, 100); + _ = NativeContract.GAS.Mint(engine, maliciousSender, balance, true); } [TestMethod] From 8f53e90f708069af4a6ca887b36d1efcd4eff0c1 Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Wed, 5 Jul 2023 20:10:37 +0200 Subject: [PATCH 10/19] Remove Trimmed value --- .../SmartContract/Native/LedgerContract.cs | 7 +++--- .../SmartContract/Native/TransactionState.cs | 22 ++++++------------- .../Ledger/UT_TransactionState.cs | 13 +++++------ .../Network/P2P/Payloads/UT_Conflicts.cs | 3 +-- 4 files changed, 16 insertions(+), 29 deletions(-) diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 9dbb67aa74..648739f77b 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -38,7 +38,6 @@ internal override ContractTask OnPersist(ApplicationEngine engine) { TransactionState[] transactions = engine.PersistingBlock.Transactions.Select(p => new TransactionState { - Trimmed = false, BlockIndex = engine.PersistingBlock.Index, Transaction = p, State = VMState.NONE @@ -50,7 +49,7 @@ internal override ContractTask OnPersist(ApplicationEngine engine) engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx)); foreach (var attr in tx.Transaction.GetAttributes()) { - engine.Snapshot.GetOrAdd(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState { Trimmed = true })); + engine.Snapshot.GetOrAdd(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState())); } } engine.SetState(transactions); @@ -145,7 +144,7 @@ public bool ContainsTransaction(DataCache snapshot, UInt256 hash) public bool ContainsConflictHash(DataCache snapshot, UInt256 hash) { var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); - if (state is not null && state.Trimmed) return true; + if (state is not null && state.Transaction is null) return true; return false; } @@ -241,7 +240,7 @@ public Header GetHeader(DataCache snapshot, uint index) public TransactionState GetTransactionState(DataCache snapshot, UInt256 hash) { var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); - if (state is not null && state.Trimmed) return null; + if (state?.Transaction is null) return null; return state; } diff --git a/src/Neo/SmartContract/Native/TransactionState.cs b/src/Neo/SmartContract/Native/TransactionState.cs index 538b474a7e..c0f5f8334d 100644 --- a/src/Neo/SmartContract/Native/TransactionState.cs +++ b/src/Neo/SmartContract/Native/TransactionState.cs @@ -21,18 +21,13 @@ namespace Neo.SmartContract.Native /// public class TransactionState : IInteroperable { - /// - /// The transaction is trimmed. - /// To indicate this state is a placeholder for a conflict transaction. - /// - public bool Trimmed; /// /// The block containing this transaction. /// public uint BlockIndex; /// - /// The transaction. + /// The transaction, if the transaction is trimmed this value will be null /// public Transaction Transaction; @@ -47,7 +42,6 @@ IInteroperable IInteroperable.Clone() { return new TransactionState { - Trimmed = Trimmed, BlockIndex = BlockIndex, Transaction = Transaction, State = State, @@ -58,7 +52,6 @@ IInteroperable IInteroperable.Clone() void IInteroperable.FromReplica(IInteroperable replica) { TransactionState from = (TransactionState)replica; - Trimmed = from.Trimmed; BlockIndex = from.BlockIndex; Transaction = from.Transaction; State = from.State; @@ -69,20 +62,19 @@ void IInteroperable.FromReplica(IInteroperable replica) void IInteroperable.FromStackItem(StackItem stackItem) { Struct @struct = (Struct)stackItem; - Trimmed = @struct[0].GetBoolean(); - if (Trimmed) return; - BlockIndex = (uint)@struct[1].GetInteger(); - _rawTransaction = ((ByteString)@struct[2]).Memory; + if (@struct.Count == 0) return; + BlockIndex = (uint)@struct[0].GetInteger(); + _rawTransaction = ((ByteString)@struct[1]).Memory; Transaction = _rawTransaction.AsSerializable(); - State = (VMState)(byte)@struct[3].GetInteger(); + State = (VMState)(byte)@struct[2].GetInteger(); } StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter) { - if (Trimmed) return new Struct(referenceCounter) { Trimmed }; + if (Transaction is null) return new Struct(referenceCounter); if (_rawTransaction.IsEmpty) _rawTransaction = Transaction.ToArray(); - return new Struct(referenceCounter) { Trimmed, BlockIndex, _rawTransaction, (byte)State }; + return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State }; } } } diff --git a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs index 1e3f765dd7..fc56d7a24d 100644 --- a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs +++ b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs @@ -33,10 +33,7 @@ public void Initialize() } } } }; - originTrimmed = new TransactionState() - { - Trimmed = true, - }; + originTrimmed = new TransactionState(); } [TestMethod] @@ -45,12 +42,12 @@ public void TestDeserialize() var data = BinarySerializer.Serialize(((IInteroperable)origin).ToStackItem(null), 1024); var reader = new MemoryReader(data); - TransactionState dest = new TransactionState(); + TransactionState dest = new(); ((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null)); dest.BlockIndex.Should().Be(origin.BlockIndex); dest.Transaction.Hash.Should().Be(origin.Transaction.Hash); - dest.Trimmed.Should().Be(false); + dest.Transaction.Should().NotBeNull(); } [TestMethod] @@ -59,12 +56,12 @@ public void TestDeserializeTrimmed() var data = BinarySerializer.Serialize(((IInteroperable)originTrimmed).ToStackItem(null), 1024); var reader = new MemoryReader(data); - TransactionState dest = new TransactionState(); + TransactionState dest = new(); ((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null)); dest.BlockIndex.Should().Be(0); dest.Transaction.Should().Be(null); - dest.Trimmed.Should().Be(true); + dest.Transaction.Should().BeNull(); } } } diff --git a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs index c955995bb1..bbf46920aa 100644 --- a/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs +++ b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs @@ -66,7 +66,7 @@ public void Verify() var key = Ledger.UT_MemoryPool.CreateStorageKey(NativeContract.Ledger.Id, Prefix_Transaction, _u.ToArray()); // Conflicting transaction is in the Conflicts attribute of some other on-chain transaction. - var conflict = new TransactionState { Trimmed = true }; + var conflict = new TransactionState(); snapshot.Add(key, new StorageItem(conflict)); Assert.IsTrue(test.Verify(snapshot, new Transaction())); @@ -74,7 +74,6 @@ public void Verify() snapshot.Delete(key); conflict = new TransactionState { - Trimmed = false, BlockIndex = 123, Transaction = new Transaction(), State = VMState.NONE From fd1748d9ed296d3755434f4bdec8645b2ecd48ec Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 17 Jul 2023 19:55:33 +0300 Subject: [PATCH 11/19] Check signers of on-chained conflict during new tx verification Fix the problem described in https://github.com/neo-project/neo/pull/2818#pullrequestreview-1526521347. 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. --- src/Neo/Ledger/Blockchain.cs | 6 +++--- src/Neo/Ledger/MemoryPool.cs | 7 ++++--- src/Neo/NeoSystem.cs | 5 +++-- src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs | 4 ++-- src/Neo/SmartContract/Native/LedgerContract.cs | 8 +++++--- src/Neo/SmartContract/Native/TransactionState.cs | 13 +++++++++++-- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 15 +++++++++++---- tests/Neo.UnitTests/Ledger/UT_TransactionState.cs | 11 ++++++++++- 8 files changed, 49 insertions(+), 20 deletions(-) diff --git a/src/Neo/Ledger/Blockchain.cs b/src/Neo/Ledger/Blockchain.cs index 9f4c33c3af..68b4333c03 100644 --- a/src/Neo/Ledger/Blockchain.cs +++ b/src/Neo/Ledger/Blockchain.cs @@ -206,7 +206,7 @@ private void OnFillMemoryPool(IEnumerable 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)); } diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 5769d4181a..47eaf344fe 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -469,15 +469,16 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) _txRwLock.EnterWriteLock(); try { - HashSet conflicts = new HashSet(); + Dictionary conflicts = new Dictionary(); // 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().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(); foreach (var item in _sortedTransactions) { - if (conflicts.Contains(item.Tx.Hash) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) + if ((conflicts.TryGetValue(item.Tx.Hash, out var signers) && signers.Contains(item.Tx.Sender)) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) { stale.Add(item.Tx.Hash); conflictingItems.Add(item.Tx); diff --git a/src/Neo/NeoSystem.cs b/src/Neo/NeoSystem.cs index b715f98382..1e4dbb61bc 100644 --- a/src/Neo/NeoSystem.cs +++ b/src/Neo/NeoSystem.cs @@ -283,10 +283,11 @@ public bool ContainsTransaction(UInt256 hash) /// Determines whether the specified transaction conflicts with some on-chain transaction. /// /// The hash of the transaction + /// The sender of the transaction /// if the transaction conflicts with on-chain transaction; otherwise, . - 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); } } } diff --git a/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs b/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs index 37c12b38dc..06aa51b325 100644 --- a/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs +++ b/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; } diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 648739f77b..745823823c 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/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()) { - 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) /// /// The snapshot used to read data. /// The hash of the conflicting transaction. + /// The sender of the conflicting transaction. /// if the blockchain contains the hash of the conflicting transaction; otherwise, . - 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(); - 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; } diff --git a/src/Neo/SmartContract/Native/TransactionState.cs b/src/Neo/SmartContract/Native/TransactionState.cs index c0f5f8334d..d3c7f644be 100644 --- a/src/Neo/SmartContract/Native/TransactionState.cs +++ b/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 /// public Transaction Transaction; + public UInt160[] ConflictingSigners; + /// /// The execution state /// @@ -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(); @@ -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 }; diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index da07d148ee..664f74a442 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/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()); 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); } diff --git a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs index fc56d7a24d..0270d97891 100644 --- a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs +++ b/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); } } } From 431eda4eb859ba26db78e23bc38b9da6552deb6a Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 18 Jul 2023 13:45:56 +0300 Subject: [PATCH 12/19] Properly handle duplicating Conflicts hashes from the persisted transactions --- src/Neo/Ledger/MemoryPool.cs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 47eaf344fe..d2641dab84 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -469,7 +469,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) _txRwLock.EnterWriteLock(); try { - Dictionary conflicts = new Dictionary(); + Dictionary> conflicts = new Dictionary>(); // 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) @@ -478,7 +478,13 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) UInt160[] conflictingSigners = tx.Signers.Select(s => s.Account).ToArray(); foreach (var h in tx.GetAttributes().Select(a => a.Hash)) { - conflicts.Add(h, conflictingSigners); + if (conflicts.TryGetValue(h, out var signersList)) + { + signersList.Add(conflictingSigners); + continue; + } + signersList = new List { conflictingSigners }; + conflicts.Add(h, signersList); } } @@ -488,7 +494,22 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) var stale = new List(); foreach (var item in _sortedTransactions) { - if ((conflicts.TryGetValue(item.Tx.Hash, out var signers) && signers.Contains(item.Tx.Sender)) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) + var shouldRemove = false; + if (conflicts.TryGetValue(item.Tx.Hash, out var signersList)) + { + foreach (var signers in signersList) + { + if (signers.Contains(item.Tx.Sender)) + { + shouldRemove = true; + break; + } + } + } + if (!shouldRemove && item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) + shouldRemove = true; + + if (shouldRemove) { stale.Add(item.Tx.Hash); conflictingItems.Add(item.Tx); From 3de091d3a7d4d10d153f64118a81d48f083b1444 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 19 Jul 2023 11:42:34 +0300 Subject: [PATCH 13/19] Store signers of all conflicting on-chain transactions with the same Conflicts attribute Store not only signers of the latest conflicting on-chain transaction, but signers of all conflicting transactions with the same attribute. --- src/Neo/Ledger/Blockchain.cs | 6 ++--- src/Neo/Ledger/MemoryPool.cs | 25 ++++--------------- src/Neo/NeoSystem.cs | 6 ++--- .../Network/P2P/RemoteNode.ProtocolHandler.cs | 4 +-- .../SmartContract/Native/LedgerContract.cs | 13 +++++----- 5 files changed, 20 insertions(+), 34 deletions(-) diff --git a/src/Neo/Ledger/Blockchain.cs b/src/Neo/Ledger/Blockchain.cs index 68b4333c03..8acec8c974 100644 --- a/src/Neo/Ledger/Blockchain.cs +++ b/src/Neo/Ledger/Blockchain.cs @@ -206,7 +206,7 @@ private void OnFillMemoryPool(IEnumerable transactions) { if (NativeContract.Ledger.ContainsTransaction(snapshot, tx.Hash)) continue; - if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash, tx.Sender)) + if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash, tx.Signers.Select(s => s.Account))) 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, transaction.Sender)) return VerifyResult.HasConflicts; + if (system.ContainsConflictHash(transaction.Hash, transaction.Signers.Select(s => s.Account))) 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, tx.Sender)) + else if (system.ContainsConflictHash(tx.Hash, tx.Signers.Select(s => s.Account))) SendRelayResult(tx, VerifyResult.HasConflicts); else system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true)); } diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index d2641dab84..497ecdcf2b 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -469,21 +469,21 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) _txRwLock.EnterWriteLock(); try { - Dictionary> conflicts = new Dictionary>(); + Dictionary> conflicts = new Dictionary>(); // 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(); + var conflictingSigners = tx.Signers.Select(s => s.Account); foreach (var h in tx.GetAttributes().Select(a => a.Hash)) { if (conflicts.TryGetValue(h, out var signersList)) { - signersList.Add(conflictingSigners); + signersList.AddRange(conflictingSigners); continue; } - signersList = new List { conflictingSigners }; + signersList = conflictingSigners.ToList(); conflicts.Add(h, signersList); } } @@ -494,22 +494,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) var stale = new List(); foreach (var item in _sortedTransactions) { - var shouldRemove = false; - if (conflicts.TryGetValue(item.Tx.Hash, out var signersList)) - { - foreach (var signers in signersList) - { - if (signers.Contains(item.Tx.Sender)) - { - shouldRemove = true; - break; - } - } - } - if (!shouldRemove && item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) - shouldRemove = true; - - if (shouldRemove) + if ((conflicts.TryGetValue(item.Tx.Hash, out var signersList) && signersList.Intersect(item.Tx.Signers.Select(s => s.Account)).Any()) || item.Tx.GetAttributes().Select(a => a.Hash).Intersect(persisted).Any()) { stale.Add(item.Tx.Hash); conflictingItems.Add(item.Tx); diff --git a/src/Neo/NeoSystem.cs b/src/Neo/NeoSystem.cs index 1e4dbb61bc..2f66389672 100644 --- a/src/Neo/NeoSystem.cs +++ b/src/Neo/NeoSystem.cs @@ -283,11 +283,11 @@ public bool ContainsTransaction(UInt256 hash) /// Determines whether the specified transaction conflicts with some on-chain transaction. /// /// The hash of the transaction - /// The sender of the transaction + /// The list of signer accounts of the transaction /// if the transaction conflicts with on-chain transaction; otherwise, . - public bool ContainsConflictHash(UInt256 hash, UInt160 sender) + public bool ContainsConflictHash(UInt256 hash, IEnumerable signers) { - return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, sender); + return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers); } } } diff --git a/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs b/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs index 06aa51b325..ffbc53c1cb 100644 --- a/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs +++ b/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, transaction.Sender))) + if (!(system.ContainsTransaction(transaction.Hash) || system.ContainsConflictHash(transaction.Hash, transaction.Signers.Select(s => s.Account)))) 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, null))).ToArray(); + hashes = hashes.Where(p => !NativeContract.Ledger.ContainsTransaction(snapshot, p)).ToArray(); } break; } diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 745823823c..5032a52ede 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -15,6 +15,7 @@ using Neo.Persistence; using Neo.VM; using System; +using System.Collections.Generic; using System.Linq; using System.Numerics; @@ -47,10 +48,11 @@ 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(); + var conflictingSigners = tx.Transaction.Signers.Select(s => s.Account); foreach (var attr in tx.Transaction.GetAttributes()) { - engine.Snapshot.GetOrAdd(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState { ConflictingSigners = conflictingSigners })); + var conflictRecord = engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState { ConflictingSigners = new UInt160[] { } })).GetInteroperable(); + conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners).ToArray(); } } engine.SetState(transactions); @@ -141,13 +143,12 @@ public bool ContainsTransaction(DataCache snapshot, UInt256 hash) /// /// The snapshot used to read data. /// The hash of the conflicting transaction. - /// The sender of the conflicting transaction. + /// The list of signer accounts of the conflicting transaction. /// if the blockchain contains the hash of the conflicting transaction; otherwise, . - public bool ContainsConflictHash(DataCache snapshot, UInt256 hash, UInt160 sender) + public bool ContainsConflictHash(DataCache snapshot, UInt256 hash, IEnumerable signers) { var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); - if (state is not null && state.Transaction is null && (sender is null || state.ConflictingSigners.Contains(sender))) return true; - return false; + return state is not null && state.Transaction is null && (signers is null || state.ConflictingSigners.Intersect(signers).Any()); } /// From efa8f57019d4c8aa3e828cb1de35eb1a3b91e2fc Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 24 Aug 2023 15:21:40 +0300 Subject: [PATCH 14/19] MemoryPool: consider signers intersection on Conflicts check This patch should be a part of fd1748d9ed296d3755434f4bdec8645b2ecd48ec, but was accidentally skipped. This patch matches exactly the NeoGo behaviour that was added in https://github.com/nspcc-dev/neo-go/pull/3061 and that was discussed earlier in https://github.com/neo-project/neo/pull/2818#issuecomment-1632972055. Fix the issue described in https://github.com/neo-project/neo/pull/2818#issuecomment-1691293260. Signed-off-by: Anna Shaleva --- src/Neo/Ledger/MemoryPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 497ecdcf2b..2e1ba046f2 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -369,7 +369,7 @@ private bool CheckConflicts(Transaction tx, out List conflictsList) { if (_unsortedTransactions.TryGetValue(hash, out PoolItem unsortedTx)) { - if (!tx.Signers.Select(p => p.Account).Contains(unsortedTx.Tx.Sender)) return false; + if (!tx.Signers.Select(p => p.Account).Intersect(unsortedTx.Tx.Signers.Select(p => p.Account)).Any()) return false; conflictsFeeSum += unsortedTx.Tx.NetworkFee; conflictsList.Add(unsortedTx); } From b6856ff4b9ee4ed14da2c2e79265d4b7db720f49 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 24 Aug 2023 16:06:32 +0300 Subject: [PATCH 15/19] MemoryPool: add test for on-chain conflict Signed-off-by: Anna Shaleva --- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 83 +++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index 664f74a442..c658d4bd49 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -5,15 +5,30 @@ using Neo.Cryptography; using Neo.IO; using Neo.Ledger; +using Neo.Network.P2P; using Neo.Network.P2P.Payloads; using Neo.Persistence; using Neo.SmartContract; using Neo.SmartContract.Native; +using Neo.VM; using System; using System.Collections; using System.Collections.Generic; using System.Linq; using System.Numerics; +using Akka.TestKit; +using Akka.TestKit.Xunit2; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Neo.Ledger; +using Neo.Network.P2P.Payloads; +using Neo.Persistence; +using Neo.SmartContract; +using Neo.SmartContract.Native; +using Neo.Wallets; +using Neo.Wallets.NEP6; +using System; +using System.Linq; +using System.Numerics; using System.Threading.Tasks; namespace Neo.UnitTests.Ledger @@ -735,6 +750,74 @@ 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()); + + 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(), VerificationScript = Array.Empty() } + }, + 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(); + } + + // Test tx2 arrive + + snapshot.Commit(); + + var senderProbe = CreateTestProbe(); + senderProbe.Send(TestBlockchain.TheNeoSystem.Blockchain, tx2); + senderProbe.ExpectMsg(p => p.Result == VerifyResult.InsufficientFunds); // should be Succedded. + } + public static StorageKey CreateStorageKey(int id, byte prefix, byte[] key = null) { byte[] buffer = GC.AllocateUninitializedArray(sizeof(byte) + (key?.Length ?? 0)); From 9460db6c1c524ada46271549bac04321169c6a0e Mon Sep 17 00:00:00 2001 From: Fernando Diaz Toledano Date: Sat, 26 Aug 2023 11:11:12 +0200 Subject: [PATCH 16/19] Clean using --- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 26 +++++---------------- 1 file changed, 6 insertions(+), 20 deletions(-) diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index c658d4bd49..de5bff6ba0 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -1,35 +1,21 @@ -using Akka.TestKit.Xunit2; -using FluentAssertions; -using Microsoft.VisualStudio.TestTools.UnitTesting; -using Moq; -using Neo.Cryptography; -using Neo.IO; -using Neo.Ledger; -using Neo.Network.P2P; -using Neo.Network.P2P.Payloads; -using Neo.Persistence; -using Neo.SmartContract; -using Neo.SmartContract.Native; -using Neo.VM; using System; using System.Collections; using System.Collections.Generic; using System.Linq; using System.Numerics; -using Akka.TestKit; +using System.Threading.Tasks; using Akka.TestKit.Xunit2; +using FluentAssertions; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using Neo.Cryptography; +using Neo.IO; using Neo.Ledger; using Neo.Network.P2P.Payloads; using Neo.Persistence; using Neo.SmartContract; using Neo.SmartContract.Native; -using Neo.Wallets; -using Neo.Wallets.NEP6; -using System; -using System.Linq; -using System.Numerics; -using System.Threading.Tasks; +using Neo.VM; namespace Neo.UnitTests.Ledger { From 0c06c915ef381cf146ea5314b6a9920f8fd07b26 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 28 Aug 2023 19:04:52 +0300 Subject: [PATCH 17/19] Refactor Malicious_OnChain_Conflict test Move it to the UT_Blockchain.cs file and refactor it a bit to make it actually pass as expected. Signed-off-by: Anna Shaleva --- tests/Neo.UnitTests/Ledger/UT_Blockchain.cs | 71 +++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs b/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs index 130df54e60..1cd8c42360 100644 --- a/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs +++ b/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs @@ -6,6 +6,7 @@ using Neo.Persistence; using Neo.SmartContract; using Neo.SmartContract.Native; +using Neo.VM; using Neo.Wallets; using Neo.Wallets.NEP6; using System; @@ -97,5 +98,75 @@ private static Transaction CreateValidTx(DataCache snapshot, NEP6Wallet wallet, tx.Witnesses = data.GetWitnesses(); return tx; } + + [TestMethod] + public void TestMaliciousOnChainConflict() + { + var snapshot = TestBlockchain.TheNeoSystem.GetSnapshot(); + var walletA = TestUtils.GenerateTestWallet("123"); + var accA = walletA.CreateAccount(); + var walletB = TestUtils.GenerateTestWallet("456"); + var accB = walletB.CreateAccount(); + ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, settings: TestBlockchain.TheNeoSystem.Settings, gas: long.MaxValue); + engine.LoadScript(Array.Empty()); + + // Fake balance for accounts A and B. + var key = new KeyBuilder(NativeContract.GAS.Id, 20).Add(accA.ScriptHash); + var entry = snapshot.GetAndChange(key, () => new StorageItem(new AccountState())); + entry.GetInteroperable().Balance = 100_000_000 * NativeContract.GAS.Factor; + snapshot.Commit(); + + key = new KeyBuilder(NativeContract.GAS.Id, 20).Add(accB.ScriptHash); + entry = snapshot.GetAndChange(key, () => new StorageItem(new AccountState())); + entry.GetInteroperable().Balance = 100_000_000 * NativeContract.GAS.Factor; + snapshot.Commit(); + + // Create transactions: + // tx1 conflicts with tx2 and has the same sender (thus, it's a valid conflict and must prevent tx2 from entering the chain); + // tx2 conflicts with tx3 and has different sender (thus, this conflict is invalid and must not prevent tx3 from entering the chain). + var tx1 = CreateValidTx(snapshot, walletA, accA.ScriptHash, 0); + var tx2 = CreateValidTx(snapshot, walletA, accA.ScriptHash, 1); + var tx3 = CreateValidTx(snapshot, walletB, accB.ScriptHash, 2); + + tx1.Attributes = new TransactionAttribute[] { new Conflicts() { Hash = tx2.Hash }, new Conflicts() { Hash = tx3.Hash } }; + + // Persist tx1. + var block = new Block + { + Header = new Header() + { + Index = 10000, + MerkleRoot = UInt256.Zero, + NextConsensus = UInt160.Zero, + PrevHash = UInt256.Zero, + Witness = new Witness() { InvocationScript = Array.Empty(), VerificationScript = Array.Empty() } + }, + Transactions = new Transaction[] { tx1 }, + }; + 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, 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(); + engine2.Snapshot.Commit(); + } + snapshot.Commit(); + + // Add tx2: must fail because valid conflict is alredy on chain (tx1). + senderProbe.Send(TestBlockchain.TheNeoSystem.Blockchain, tx2); + senderProbe.ExpectMsg(p => p.Result == VerifyResult.HasConflicts); + + // Add tx3: must succeed because on-chain conflict is invalid (doesn't have proper signer). + senderProbe.Send(TestBlockchain.TheNeoSystem.Blockchain, tx3); + senderProbe.ExpectMsg(p => p.Result == VerifyResult.Succeed); + } } } From f7c3acb3f4de20d7104f385b2cb8caa16a42b2d5 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 12:56:34 +0300 Subject: [PATCH 18/19] Use Distinct to filter out duplicating conflicting on-chain signers Co-authored-by: Shargon --- src/Neo/SmartContract/Native/LedgerContract.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 5032a52ede..cd405c098b 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -51,8 +51,9 @@ internal override ContractTask OnPersist(ApplicationEngine engine) var conflictingSigners = tx.Transaction.Signers.Select(s => s.Account); foreach (var attr in tx.Transaction.GetAttributes()) { - var conflictRecord = engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState { ConflictingSigners = new UInt160[] { } })).GetInteroperable(); - conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners).ToArray(); + var conflictRecord = engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), + () => new StorageItem(new TransactionState { ConflictingSigners = Array.Empty() })).GetInteroperable(); + conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners).Distinct().ToArray(); } } engine.SetState(transactions); From 2efa3b2e57eeccca3996667bf027ddd636e6b68f Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 1 Sep 2023 14:18:35 +0300 Subject: [PATCH 19/19] Use HashSet as a container of conflicting hashes in the mempool Fix https://github.com/neo-project/neo/pull/2818#discussion_r1312873470. Signed-off-by: Anna Shaleva --- src/Neo/Ledger/MemoryPool.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 2e1ba046f2..a966835fc3 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -57,7 +57,7 @@ public class MemoryPool : IReadOnlyCollection /// /// Store transaction hashes that conflict with verified mempooled transactions. /// - private readonly Dictionary> _conflicts = new(); + private readonly Dictionary> _conflicts = new(); /// /// Stores the verified sorted transactions currently in the pool. /// @@ -315,7 +315,7 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) { if (!_conflicts.TryGetValue(attr.Hash, out var pooled)) { - pooled = new List(); + pooled = new HashSet(); } pooled.Add(tx.Hash); _conflicts.AddOrSet(attr.Hash, pooled); @@ -424,7 +424,7 @@ private void RemoveConflictsOfVerified(PoolItem item) { foreach (var h in item.Tx.GetAttributes().Select(attr => attr.Hash)) { - if (_conflicts.TryGetValue(h, out List conflicts)) + if (_conflicts.TryGetValue(h, out HashSet conflicts)) { conflicts.Remove(item.Tx.Hash); if (conflicts.Count() == 0) @@ -566,7 +566,7 @@ private int ReverifyTransactions(SortedSet verifiedSortedTxPool, { if (!_conflicts.TryGetValue(attr.Hash, out var pooled)) { - pooled = new List(); + pooled = new HashSet(); } pooled.Add(item.Tx.Hash); _conflicts.AddOrSet(attr.Hash, pooled);