diff --git a/src/Neo/Ledger/Blockchain.cs b/src/Neo/Ledger/Blockchain.cs index 8acec8c974..b44595f722 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.Signers.Select(s => s.Account))) + if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash, tx.Signers.Select(s => s.Account), system.Settings.MaxTraceableBlocks)) continue; // First remove the tx if it is unverified in the pool. system.MemPool.TryRemoveUnVerified(tx.Hash, out _); diff --git a/src/Neo/NeoSystem.cs b/src/Neo/NeoSystem.cs index 2f66389672..11590377ad 100644 --- a/src/Neo/NeoSystem.cs +++ b/src/Neo/NeoSystem.cs @@ -287,7 +287,7 @@ public bool ContainsTransaction(UInt256 hash) /// if the transaction conflicts with on-chain transaction; otherwise, . public bool ContainsConflictHash(UInt256 hash, IEnumerable signers) { - return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers); + return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers, Settings.MaxTraceableBlocks); } } } diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index cd405c098b..b95c518518 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -47,13 +47,19 @@ internal override ContractTask OnPersist(ApplicationEngine engine) engine.Snapshot.Add(CreateStorageKey(Prefix_Block).Add(engine.PersistingBlock.Hash), new StorageItem(Trim(engine.PersistingBlock).ToArray())); foreach (TransactionState tx in transactions) { - engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx)); + // It's possible that there are previously saved malicious conflict records for this transaction. + // If so, then remove it and store the relevant transaction itself. + engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(tx)); + + // Store transaction's conflicits. var conflictingSigners = tx.Transaction.Signers.Select(s => s.Account); foreach (var attr in tx.Transaction.GetAttributes()) { - 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.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + foreach (var signer in conflictingSigners) + { + engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + } } } engine.SetState(transactions); @@ -145,11 +151,24 @@ public bool ContainsTransaction(DataCache snapshot, UInt256 hash) /// The snapshot used to read data. /// The hash of the conflicting transaction. /// The list of signer accounts of the conflicting transaction. + /// MaxTraceableBlocks protocol setting. /// if the blockchain contains the hash of the conflicting transaction; otherwise, . - public bool ContainsConflictHash(DataCache snapshot, UInt256 hash, IEnumerable signers) + public bool ContainsConflictHash(DataCache snapshot, UInt256 hash, IEnumerable signers, uint maxTraceableBlocks) { - 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()); + // Check the dummy stub firstly to define whether there's exist at least one conflict record. + var stub = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash))?.GetInteroperable(); + if (stub is null || stub.Transaction is not null || !IsTraceableBlock(snapshot, stub.BlockIndex, maxTraceableBlocks)) + return false; + + // At least one conflict record is found, then need to check signers intersection. + foreach (var signer in signers) + { + var state = snapshot.TryGet(CreateStorageKey(Prefix_Transaction).Add(hash).Add(signer))?.GetInteroperable(); + if (state is not null && IsTraceableBlock(snapshot, state.BlockIndex, maxTraceableBlocks)) + return true; + } + + return false; } /// diff --git a/src/Neo/SmartContract/Native/TransactionState.cs b/src/Neo/SmartContract/Native/TransactionState.cs index d3c7f644be..8994a7acd5 100644 --- a/src/Neo/SmartContract/Native/TransactionState.cs +++ b/src/Neo/SmartContract/Native/TransactionState.cs @@ -32,8 +32,6 @@ public class TransactionState : IInteroperable /// public Transaction Transaction; - public UInt160[] ConflictingSigners; - /// /// The execution state /// @@ -47,7 +45,6 @@ IInteroperable IInteroperable.Clone() { BlockIndex = BlockIndex, Transaction = Transaction, - ConflictingSigners = ConflictingSigners, State = State, _rawTransaction = _rawTransaction }; @@ -58,7 +55,6 @@ void IInteroperable.FromReplica(IInteroperable replica) TransactionState from = (TransactionState)replica; BlockIndex = from.BlockIndex; Transaction = from.Transaction; - ConflictingSigners = from.ConflictingSigners; State = from.State; if (_rawTransaction.IsEmpty) _rawTransaction = from._rawTransaction; @@ -67,12 +63,12 @@ void IInteroperable.FromReplica(IInteroperable replica) void IInteroperable.FromStackItem(StackItem stackItem) { Struct @struct = (Struct)stackItem; - if (@struct.Count == 1) - { - ConflictingSigners = ((VM.Types.Array)@struct[0]).Select(u => new UInt160(u.GetSpan())).ToArray(); - return; - } BlockIndex = (uint)@struct[0].GetInteger(); + + // Conflict record. + if (@struct.Count == 1) return; + + // Fully-qualified transaction. _rawTransaction = ((ByteString)@struct[1]).Memory; Transaction = _rawTransaction.AsSerializable(); State = (VMState)(byte)@struct[2].GetInteger(); @@ -80,7 +76,8 @@ void IInteroperable.FromStackItem(StackItem stackItem) StackItem IInteroperable.ToStackItem(ReferenceCounter referenceCounter) { - if (Transaction is null) return new Struct(referenceCounter) { new VM.Types.Array(referenceCounter, ConflictingSigners.Select(u => new ByteString(u.ToArray())).ToArray()) }; + if (Transaction is null) + return new Struct(referenceCounter) { BlockIndex }; 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 1cd8c42360..7ecb965ff5 100644 --- a/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs +++ b/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs @@ -135,7 +135,7 @@ public void TestMaliciousOnChainConflict() { Header = new Header() { - Index = 10000, + Index = 5, // allow tx1, tx2 and tx3 to fit into MaxValidUntilBlockIncrement. MerkleRoot = UInt256.Zero, NextConsensus = UInt160.Zero, PrevHash = UInt256.Zero, @@ -149,13 +149,26 @@ public void TestMaliciousOnChainConflict() sb.EmitSysCall(ApplicationEngine.System_Contract_NativeOnPersist); onPersistScript = sb.ToArray(); } - TransactionState[] transactionStates; using (ApplicationEngine engine2 = ApplicationEngine.Create(TriggerType.OnPersist, null, snapshot, block, TestBlockchain.TheNeoSystem.Settings, 0)) { engine2.LoadScript(onPersistScript); - if (engine2.Execute() != VMState.HALT) throw new InvalidOperationException(); - Blockchain.ApplicationExecuted application_executed = new(engine2); - transactionStates = engine2.GetState(); + if (engine2.Execute() != VMState.HALT) throw engine2.FaultException; + engine2.Snapshot.Commit(); + } + snapshot.Commit(); + + // Run PostPersist to update current block index in native Ledger. + // Relevant current block index is needed for conflict records checks. + byte[] postPersistScript; + using (ScriptBuilder sb = new()) + { + sb.EmitSysCall(ApplicationEngine.System_Contract_NativePostPersist); + postPersistScript = sb.ToArray(); + } + using (ApplicationEngine engine2 = ApplicationEngine.Create(TriggerType.PostPersist, null, snapshot, block, TestBlockchain.TheNeoSystem.Settings, 0)) + { + engine2.LoadScript(postPersistScript); + if (engine2.Execute() != VMState.HALT) throw engine2.FaultException; engine2.Snapshot.Commit(); } snapshot.Commit(); diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index de5bff6ba0..eb0aabf1f5 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -736,74 +736,6 @@ public void TestUpdatePoolForBlockPersisted() _unit.VerifiedCount.Should().Be(0); } - - [TestMethod] - public async Task Malicious_OnChain_Conflict() - { - // Arrange: prepare mempooled txs that have conflicts. - long txFee = 1; - var snapshot = GetSnapshot(); - BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, senderAccount); - ApplicationEngine engine = ApplicationEngine.Create(TriggerType.Application, null, snapshot, settings: TestBlockchain.TheNeoSystem.Settings, gas: long.MaxValue); - engine.LoadScript(Array.Empty()); - - 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 0270d97891..13629c4f63 100644 --- a/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs +++ b/tests/Neo.UnitTests/Ledger/UT_TransactionState.cs @@ -36,11 +36,7 @@ public void Initialize() }; originTrimmed = new TransactionState { - ConflictingSigners = new UInt160[] - { - new UInt160(Crypto.Hash160(new byte[] { 1, 2, 3 })), - new UInt160(Crypto.Hash160(new byte[] { 4, 5, 6 })) - } + BlockIndex = 1, }; } @@ -67,10 +63,9 @@ public void TestDeserializeTrimmed() TransactionState dest = new(); ((IInteroperable)dest).FromStackItem(BinarySerializer.Deserialize(ref reader, ExecutionEngineLimits.Default, null)); - dest.BlockIndex.Should().Be(0); + dest.BlockIndex.Should().Be(originTrimmed.BlockIndex); dest.Transaction.Should().Be(null); dest.Transaction.Should().BeNull(); - CollectionAssert.AreEqual(originTrimmed.ConflictingSigners, dest.ConflictingSigners); } } }