From 6398801629a8bd11cc72d09d6c563308748ad522 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 4 Sep 2023 10:19:17 +0300 Subject: [PATCH] Payloads: implement Conflicts attribute (#2818) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Payloads: implement Conflicts attribute Closes #1991. * Update src/Neo/Ledger/MemoryPool.cs * Fix conflicting tx fees accounting Fix the comment https://github.com/neo-project/neo/pull/2818#discussion_r1130790669. * Reformat code and improve variables naming Fix Vitor's comments. * Make comments more clear and adjust variables naming Fix Owen's feedback. * 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). * Fix formatting, comments and add a testcase for conflicting mempool txs * Add one more testcase for conflicting transactions Testcase provided by Vitor in https://github.com/neo-project/neo/pull/2818#discussion_r1209313799. * 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. * Remove Trimmed value * 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. * Properly handle duplicating Conflicts hashes from the persisted transactions * 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. * 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 * MemoryPool: add test for on-chain conflict Signed-off-by: Anna Shaleva * Clean using * 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 * Use Distinct to filter out duplicating conflicting on-chain signers Co-authored-by: Shargon * 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 --------- Signed-off-by: Anna Shaleva Co-authored-by: Shargon Co-authored-by: Vitor Nazário Coelho Co-authored-by: Jimmy --- src/Neo/Ledger/Blockchain.cs | 8 +- src/Neo/Ledger/MemoryPool.cs | 169 +++++++++- src/Neo/Ledger/TransactionRemovalReason.cs | 9 +- .../Ledger/TransactionVerificationContext.cs | 10 +- src/Neo/Ledger/VerifyResult.cs | 5 + src/Neo/NeoSystem.cs | 11 + src/Neo/Network/P2P/Payloads/Conflicts.cs | 47 +++ src/Neo/Network/P2P/Payloads/Transaction.cs | 10 +- .../P2P/Payloads/TransactionAttributeType.cs | 8 +- .../Network/P2P/RemoteNode.ProtocolHandler.cs | 2 +- .../SmartContract/Native/LedgerContract.cs | 29 +- .../SmartContract/Native/TransactionState.cs | 13 +- tests/Neo.UnitTests/Ledger/UT_Blockchain.cs | 71 +++++ tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 295 +++++++++++++++++- .../Ledger/UT_TransactionState.cs | 29 +- .../UT_TransactionVerificationContext.cs | 43 ++- .../Network/P2P/Payloads/UT_Conflicts.cs | 89 ++++++ .../Network/P2P/Payloads/UT_Transaction.cs | 10 +- 18 files changed, 807 insertions(+), 51 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/Blockchain.cs b/src/Neo/Ledger/Blockchain.cs index 52b3c37ce5..8acec8c974 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, tx.Signers.Select(s => s.Account))) + 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, transaction.Signers.Select(s => s.Account))) 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, tx.Signers.Select(s => s.Account))) + SendRelayResult(tx, VerifyResult.HasConflicts); + else system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true)); } private void Persist(Block block) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 6ade372ee1..a966835fc3 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 HashSet(); + } + 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,49 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot) return VerifyResult.Succeed; } + /// + /// 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 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(); + 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)) + conflictsFeeSum += unsortedTx.Tx.NetworkFee; + conflictsList.Add(unsortedTx); + } + } + // 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 unsortedTx)) + { + 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); + } + } + // 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. + + return true; + } + private List RemoveOverCapacity() { List removedTransactions = new(); @@ -332,7 +396,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 +414,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 HashSet 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 +459,67 @@ 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 { + 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 _)) continue; - TryRemoveUnVerified(tx.Hash, out _); + if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerified(tx.Hash, out _); + 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.AddRange(conflictingSigners); + continue; + } + signersList = conflictingSigners.ToList(); + conflicts.Add(h, signersList); + } } - // 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.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); + } + } + 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 +555,31 @@ internal void InvalidateAllTransactions() // 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 HashSet(); + } + 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 +595,14 @@ internal void InvalidateAllTransactions() 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..5b6cc6cc9c 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 @@ -50,15 +51,18 @@ 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; - if (balance < fee) return false; + 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(); if (oracle != null && oracleResponses.ContainsKey(oracle.Id)) 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/NeoSystem.cs b/src/Neo/NeoSystem.cs index c87b9e2630..2f66389672 100644 --- a/src/Neo/NeoSystem.cs +++ b/src/Neo/NeoSystem.cs @@ -278,5 +278,16 @@ 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 + /// The list of signer accounts of the transaction + /// if the transaction conflicts with on-chain transaction; otherwise, . + public bool ContainsConflictHash(UInt256 hash, IEnumerable signers) + { + return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers); + } } } diff --git a/src/Neo/Network/P2P/Payloads/Conflicts.cs b/src/Neo/Network/P2P/Payloads/Conflicts.cs new file mode 100644 index 0000000000..db00b03259 --- /dev/null +++ b/src/Neo/Network/P2P/Payloads/Conflicts.cs @@ -0,0 +1,47 @@ +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) + { + // 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/Payloads/Transaction.cs b/src/Neo/Network/P2P/Payloads/Transaction.cs index b99449ac48..24581e2b18 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 in case of sender's match. /// 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 in case of sender's match. /// 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/Network/P2P/RemoteNode.ProtocolHandler.cs b/src/Neo/Network/P2P/RemoteNode.ProtocolHandler.cs index dfd21c63b8..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)) + 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: diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index eec49da99a..cd405c098b 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,6 +48,13 @@ internal override ContractTask OnPersist(ApplicationEngine engine) foreach (TransactionState tx in transactions) { engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx)); + 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 = Array.Empty() })).GetInteroperable(); + conflictRecord.ConflictingSigners = conflictRecord.ConflictingSigners.Concat(conflictingSigners).Distinct().ToArray(); + } } engine.SetState(transactions); return ContractTask.CompletedTask; @@ -126,7 +134,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. + /// 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, IEnumerable signers) + { + var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); + return state is not null && state.Transaction is null && (signers is null || state.ConflictingSigners.Intersect(signers).Any()); } /// @@ -220,7 +243,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?.Transaction is null) return null; + return state; } /// diff --git a/src/Neo/SmartContract/Native/TransactionState.cs b/src/Neo/SmartContract/Native/TransactionState.cs index 5c7380f1b8..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 { @@ -27,10 +28,12 @@ public class TransactionState : IInteroperable public uint BlockIndex; /// - /// The transaction. + /// The transaction, if the transaction is trimmed this value will be null /// 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,6 +67,11 @@ void IInteroperable.FromReplica(IInteroperable replica) void IInteroperable.FromStackItem(StackItem stackItem) { Struct @struct = (Struct)stackItem; + if (@struct.Count == 1) + { + ConflictingSigners = ((VM.Types.Array)@struct[0]).Select(u => new UInt160(u.GetSpan())).ToArray(); + return; + } BlockIndex = (uint)@struct[0].GetInteger(); _rawTransaction = ((ByteString)@struct[1]).Memory; Transaction = _rawTransaction.AsSerializable(); @@ -70,6 +80,7 @@ void IInteroperable.FromStackItem(StackItem stackItem) StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter) { + if (Transaction is null) return new Struct(referenceCounter) { new VM.Types.Array(referenceCounter, ConflictingSigners.Select(u => new ByteString(u.ToArray())).ToArray()) }; if (_rawTransaction.IsEmpty) _rawTransaction = Transaction.ToArray(); return new Struct(referenceCounter) { BlockIndex, _rawTransaction, (byte)State }; 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); + } } } diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index 98f7b009fd..de5bff6ba0 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -1,19 +1,21 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Linq; +using System.Numerics; +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 System; -using System.Collections; -using System.Collections.Generic; -using System.Linq; -using System.Numerics; -using System.Threading.Tasks; +using Neo.VM; namespace Neo.UnitTests.Ledger { @@ -68,7 +70,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 +94,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 +243,215 @@ 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, 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); + 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); + + 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, 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(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 * 7); + _ = 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 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 + + 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 } }; + _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 } }; + + 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); + + // Act & Assert: try to add conlflicting transactions to the pool. + _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(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(3); + _unit.GetVerifiedTransactions().Should().Contain(new List() { mp2_1, mp2_2, 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(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() { mp6, 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] + 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; @@ -525,6 +736,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)); diff --git a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs index db023d3eb8..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; @@ -14,6 +15,8 @@ public class UT_TransactionState { TransactionState origin; + TransactionState originTrimmed; + [TestInitialize] public void Initialize() { @@ -31,6 +34,14 @@ public void Initialize() } } } }; + originTrimmed = new TransactionState + { + ConflictingSigners = new UInt160[] + { + new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })), + new UInt160(Crypto.Hash160(new byte[] { 4, 5, 6 })) + } + }; } [TestMethod] @@ -39,11 +50,27 @@ 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.Transaction.Should().NotBeNull(); + } + + [TestMethod] + public void TestDeserializeTrimmed() + { + var data = BinarySerializer.Serialize(((IInteroperable)originTrimmed).ToStackItem(null), 1024); + var reader = new MemoryReader(data); + + TransactionState dest = new(); + ((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null)); + + dest.BlockIndex.Should().Be(0); + dest.Transaction.Should().Be(null); + dest.Transaction.Should().BeNull(); + CollectionAssert.AreEqual(originTrimmed.ConflictingSigners, dest.ConflictingSigners); } } } 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..bbf46920aa --- /dev/null +++ b/tests/Neo.UnitTests/Network/P2P/Payloads/UT_Conflicts.cs @@ -0,0 +1,89 @@ +using FluentAssertions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Neo.IO; +using Neo.Network.P2P.Payloads; +using Neo.SmartContract.Native; +using Neo.SmartContract; +using Neo.VM; +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()); + + // Conflicting transaction is in the Conflicts attribute of some other on-chain transaction. + var conflict = new TransactionState(); + snapshot.Add(key, new StorageItem(conflict)); + Assert.IsTrue(test.Verify(snapshot, new Transaction())); + + // Conflicting transaction is on-chain. + snapshot.Delete(key); + conflict = new TransactionState + { + 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())); + } + } +} 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]