From 8d270e64690455e453e8fa8b71f076d2f572aded Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 8 Nov 2023 11:56:29 +0300 Subject: [PATCH] Improved scheme for Conflicts attribute storing and pricing (#2913) * Ledger: change conflict records storage scheme This commit is a part of #2907: it implements conflict records storage scheme discribed in https://github.com/neo-project/neo/issues/2907#issuecomment-1722506014. The short scheme description: Do not store the list of conflicting signers in the Ledger's conflict record value. Instead, put each conflicting signer in the conflict record key so that the reculting key is: {Prefix_Transaction, , }, and the value is {}. As an optimisation, for each conflict record store the dummy stub where the key is {Prefix_Transaction, } and the value is {}. This optimisation allows to reduce DB read requests during newly-pooled transaction verification for those transactions that do not have any on-chained conflicts. Also, IsTraceableBlock check is added for on-chain conflicts verification. It is needed to avoid situations when untraceable on-chained conflict affects the newly-pooled transaction verification. Signed-off-by: Anna Shaleva * UT_MemoryPool: remove unused test Malicious_OnChain_Conflict was constructed incorrectly (see the comment in the end of the test) and was replaced by the proper TestMaliciousOnChainConflict test in UT_Blockchain.cs way back ago in https://github.com/neo-project/neo/pull/2818/commits/0c06c915ef381cf146ea5314b6a9920f8fd07b26. Signed-off-by: Anna Shaleva * Policy: introduce fee for Conflicts attribute This commit implements Conflicts attribute policy described in https://github.com/neo-project/neo/issues/2907#issuecomment-1722506014. Signed-off-by: Anna Shaleva * *: remove remnants of ConflictsFee in native Policy ConflictsFee logic was replaced by the generic attribute fee mechanism implemented in #2916. Signed-off-by: Anna Shaleva * Native: do not remove malicious conflict records during OnPersist It's OK to keep them and save O(n) operations during OnPersist. The storage taken by these malicious conflict records is properly paid anyway. See the discussion under https://github.com/neo-project/neo/pull/2913#discussion_r1329176044. Signed-off-by: Anna Shaleva * Properly rewrite previously added malicious conflict if it's in the storage `engine.Snapshot.Add` doesn't allow to rewrite storage entity if it's already exist. Thus, we firstly need to remove it and afterwards to add the updated entity. Signed-off-by: Anna Shaleva * Throw proper exception if TestMaliciousOnChainConflict fails Signed-off-by: Anna Shaleva * Optimize conflicts records storing Use Snapshot.GetAndChange instead of subsequent calls to Delete and Add. Ref. https://github.com/neo-project/neo/pull/2913#discussion_r1384748139. Signed-off-by: Anna Shaleva --------- Signed-off-by: Anna Shaleva Co-authored-by: Shargon Co-authored-by: Jimmy --- src/Neo/Ledger/Blockchain.cs | 2 +- src/Neo/NeoSystem.cs | 2 +- .../SmartContract/Native/LedgerContract.cs | 33 +++++++-- .../SmartContract/Native/TransactionState.cs | 17 ++--- tests/Neo.UnitTests/Ledger/UT_Blockchain.cs | 23 +++++-- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 68 ------------------- .../Ledger/UT_TransactionState.cs | 9 +-- 7 files changed, 55 insertions(+), 99 deletions(-) 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); } } }