From fd1748d9ed296d3755434f4bdec8645b2ecd48ec Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 17 Jul 2023 19:55:33 +0300 Subject: [PATCH] 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); } } }