From d6d3d475acb8c768a11e29ef49f24f278ea4ea53 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 18 Sep 2023 19:39:32 +0300 Subject: [PATCH 1/8] 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 --- 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 | 21 +++++++++--- .../Ledger/UT_TransactionState.cs | 9 ++--- 6 files changed, 55 insertions(+), 29 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..3a490e204c 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -47,13 +47,21 @@ 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) { + // Remove possible previously saved malicious conflict records for the transaction (if any). + foreach (var (key, _) in engine.Snapshot.Find(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash).ToArray())) + engine.Snapshot.Delete(key); + engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), 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.Add(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + foreach (var signer in conflictingSigners) + { + engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer), new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + } } } engine.SetState(transactions); @@ -145,11 +153,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..0a2d0eb013 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(); + 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 new InvalidOperationException(); engine2.Snapshot.Commit(); } snapshot.Commit(); 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); } } } From 002e7bf19c0ff7ecfce9fa036d6ba2018642f293 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 18 Sep 2023 21:06:22 +0300 Subject: [PATCH 2/8] 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 --- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 68 --------------------- 1 file changed, 68 deletions(-) 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)); From cba97f144afe0c81677f0202db3fb0a5f4e55d99 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 18 Sep 2023 20:31:56 +0300 Subject: [PATCH 3/8] 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 --- src/Neo/Network/P2P/Payloads/Transaction.cs | 4 +- .../SmartContract/Native/PolicyContract.cs | 31 ++++++++++ src/Neo/Wallets/Wallet.cs | 1 + .../SmartContract/Native/UT_PolicyContract.cs | 61 +++++++++++++++++++ 4 files changed, 96 insertions(+), 1 deletion(-) diff --git a/src/Neo/Network/P2P/Payloads/Transaction.cs b/src/Neo/Network/P2P/Payloads/Transaction.cs index 24581e2b18..574f8954d7 100644 --- a/src/Neo/Network/P2P/Payloads/Transaction.cs +++ b/src/Neo/Network/P2P/Payloads/Transaction.cs @@ -368,7 +368,9 @@ public virtual VerifyResult VerifyStateDependent(ProtocolSettings settings, Data foreach (TransactionAttribute attribute in Attributes) if (!attribute.Verify(snapshot, this)) return VerifyResult.InvalidAttribute; - long net_fee = NetworkFee - Size * NativeContract.Policy.GetFeePerByte(snapshot); + + long conflictsFee = GetAttributes().Count() * Signers.Count() * NativeContract.Policy.GetConflictsFee(snapshot); + long net_fee = NetworkFee - Size * NativeContract.Policy.GetFeePerByte(snapshot) - conflictsFee; if (net_fee < 0) return VerifyResult.InsufficientFunds; if (net_fee > MaxVerificationGas) net_fee = MaxVerificationGas; diff --git a/src/Neo/SmartContract/Native/PolicyContract.cs b/src/Neo/SmartContract/Native/PolicyContract.cs index 536bc65691..f565cc9fbc 100644 --- a/src/Neo/SmartContract/Native/PolicyContract.cs +++ b/src/Neo/SmartContract/Native/PolicyContract.cs @@ -31,6 +31,11 @@ public sealed class PolicyContract : NativeContract /// public const uint DefaultStoragePrice = 100000; + /// + /// The default fee for Conflicts attribute per signer. + /// + public const uint DefaultConflictsFee = 0; + /// /// The default network fee per byte of transactions. /// @@ -46,10 +51,16 @@ public sealed class PolicyContract : NativeContract /// public const uint MaxStoragePrice = 10000000; + /// + /// The maximum fee for Conflicts attribute per signer that the committee can set. + /// + public const uint MaxConflictsFee = 10_0000_0000; + private const byte Prefix_BlockedAccount = 15; private const byte Prefix_FeePerByte = 10; private const byte Prefix_ExecFeeFactor = 18; private const byte Prefix_StoragePrice = 19; + private const byte Prefix_ConflictsFee = 20; internal PolicyContract() { @@ -60,6 +71,7 @@ internal override ContractTask Initialize(ApplicationEngine engine) engine.Snapshot.Add(CreateStorageKey(Prefix_FeePerByte), new StorageItem(DefaultFeePerByte)); engine.Snapshot.Add(CreateStorageKey(Prefix_ExecFeeFactor), new StorageItem(DefaultExecFeeFactor)); engine.Snapshot.Add(CreateStorageKey(Prefix_StoragePrice), new StorageItem(DefaultStoragePrice)); + engine.Snapshot.Add(CreateStorageKey(Prefix_ConflictsFee), new StorageItem(DefaultConflictsFee)); return ContractTask.CompletedTask; } @@ -96,6 +108,17 @@ public uint GetStoragePrice(DataCache snapshot) return (uint)(BigInteger)snapshot[CreateStorageKey(Prefix_StoragePrice)]; } + /// + /// Gets the fee for Conflicts attribute per signer. + /// + /// The snapshot used to read data. + /// The fee for Conflicts attribute per signer. + [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.ReadStates)] + public uint GetConflictsFee(DataCache snapshot) + { + return (uint)(BigInteger)snapshot[CreateStorageKey(Prefix_ConflictsFee)]; + } + /// /// Determines whether the specified account is blocked. /// @@ -132,6 +155,14 @@ private void SetStoragePrice(ApplicationEngine engine, uint value) engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_StoragePrice)).Set(value); } + [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States)] + private void SetConflictsFee(ApplicationEngine engine, uint value) + { + if (value > MaxConflictsFee) throw new ArgumentOutOfRangeException(nameof(value)); + if (!CheckCommittee(engine)) throw new InvalidOperationException(); + engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_ConflictsFee)).Set(value); + } + [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States)] private bool BlockAccount(ApplicationEngine engine, UInt160 account) { diff --git a/src/Neo/Wallets/Wallet.cs b/src/Neo/Wallets/Wallet.cs index 60a3b60891..0a5a0dfc55 100644 --- a/src/Neo/Wallets/Wallet.cs +++ b/src/Neo/Wallets/Wallet.cs @@ -661,6 +661,7 @@ public long CalculateNetworkFee(DataCache snapshot, Transaction tx) } } networkFee += size * NativeContract.Policy.GetFeePerByte(snapshot); + networkFee += tx.GetAttributes().Count() * tx.Signers.Count() * NativeContract.Policy.GetConflictsFee(snapshot); return networkFee; } diff --git a/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs b/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs index 6730dd8825..64a44ae7be 100644 --- a/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs +++ b/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs @@ -33,6 +33,10 @@ public void Check_Default() var ret = NativeContract.Policy.Call(snapshot, "getFeePerByte"); ret.Should().BeOfType(); ret.GetInteger().Should().Be(1000); + + ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); + ret.Should().BeOfType(); + ret.GetInteger().Should().Be(0); } [TestMethod] @@ -174,6 +178,63 @@ public void Check_SetStoragePrice() ret.GetInteger().Should().Be(300300); } + [TestMethod] + public void Check_SetConflictsFee() + { + var snapshot = _snapshot.CreateSnapshot(); + + // Fake blockchain + Block block = new() + { + Header = new Header + { + Index = 1000, + PrevHash = UInt256.Zero + } + }; + + // Without signature + Assert.ThrowsException(() => + { + NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(), block, + "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 100500 }); + }); + + var ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); + ret.Should().BeOfType(); + ret.GetInteger().Should().Be(0); + + // With signature, wrong value + UInt160 committeeMultiSigAddr = NativeContract.NEO.GetCommitteeAddress(snapshot); + Assert.ThrowsException(() => + { + NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(committeeMultiSigAddr), block, + "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 11_0000_0000 }); + }); + + ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); + ret.Should().BeOfType(); + ret.GetInteger().Should().Be(0); + + // Proper set + ret = NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(committeeMultiSigAddr), block, + "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 300300 }); + ret.IsNull.Should().BeTrue(); + + ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); + ret.Should().BeOfType(); + ret.GetInteger().Should().Be(300300); + + // Set to zero + ret = NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(committeeMultiSigAddr), block, + "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 0 }); + ret.IsNull.Should().BeTrue(); + + ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); + ret.Should().BeOfType(); + ret.GetInteger().Should().Be(0); + } + [TestMethod] public void Check_BlockAccount() { From 330cbea762948e1b2c2f4c446a810a455606e2b7 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 10 Oct 2023 17:06:50 +0300 Subject: [PATCH 4/8] *: 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 --- .../SmartContract/Native/PolicyContract.cs | 19 ------- .../SmartContract/Native/UT_PolicyContract.cs | 57 ------------------- 2 files changed, 76 deletions(-) diff --git a/src/Neo/SmartContract/Native/PolicyContract.cs b/src/Neo/SmartContract/Native/PolicyContract.cs index 9b48e9d15e..c754a15a93 100644 --- a/src/Neo/SmartContract/Native/PolicyContract.cs +++ b/src/Neo/SmartContract/Native/PolicyContract.cs @@ -32,11 +32,6 @@ public sealed class PolicyContract : NativeContract /// public const uint DefaultStoragePrice = 100000; - /// - /// The default fee for Conflicts attribute per signer. - /// - public const uint DefaultConflictsFee = 0; - /// /// The default network fee per byte of transactions. /// @@ -62,11 +57,6 @@ public sealed class PolicyContract : NativeContract /// public const uint MaxStoragePrice = 10000000; - /// - /// The maximum fee for Conflicts attribute per signer that the committee can set. - /// - public const uint MaxConflictsFee = 10_0000_0000; - private const byte Prefix_BlockedAccount = 15; private const byte Prefix_FeePerByte = 10; private const byte Prefix_ExecFeeFactor = 18; @@ -82,7 +72,6 @@ internal override ContractTask Initialize(ApplicationEngine engine) engine.Snapshot.Add(CreateStorageKey(Prefix_FeePerByte), new StorageItem(DefaultFeePerByte)); engine.Snapshot.Add(CreateStorageKey(Prefix_ExecFeeFactor), new StorageItem(DefaultExecFeeFactor)); engine.Snapshot.Add(CreateStorageKey(Prefix_StoragePrice), new StorageItem(DefaultStoragePrice)); - engine.Snapshot.Add(CreateStorageKey(Prefix_ConflictsFee), new StorageItem(DefaultConflictsFee)); return ContractTask.CompletedTask; } @@ -181,14 +170,6 @@ private void SetStoragePrice(ApplicationEngine engine, uint value) engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_StoragePrice)).Set(value); } - [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States)] - private void SetConflictsFee(ApplicationEngine engine, uint value) - { - if (value > MaxConflictsFee) throw new ArgumentOutOfRangeException(nameof(value)); - if (!CheckCommittee(engine)) throw new InvalidOperationException(); - engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_ConflictsFee)).Set(value); - } - [ContractMethod(CpuFee = 1 << 15, RequiredCallFlags = CallFlags.States)] private bool BlockAccount(ApplicationEngine engine, UInt160 account) { diff --git a/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs b/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs index 4f652cbeef..6793c27614 100644 --- a/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs +++ b/tests/Neo.UnitTests/SmartContract/Native/UT_PolicyContract.cs @@ -240,63 +240,6 @@ public void Check_SetStoragePrice() ret.GetInteger().Should().Be(300300); } - [TestMethod] - public void Check_SetConflictsFee() - { - var snapshot = _snapshot.CreateSnapshot(); - - // Fake blockchain - Block block = new() - { - Header = new Header - { - Index = 1000, - PrevHash = UInt256.Zero - } - }; - - // Without signature - Assert.ThrowsException(() => - { - NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(), block, - "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 100500 }); - }); - - var ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); - ret.Should().BeOfType(); - ret.GetInteger().Should().Be(0); - - // With signature, wrong value - UInt160 committeeMultiSigAddr = NativeContract.NEO.GetCommitteeAddress(snapshot); - Assert.ThrowsException(() => - { - NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(committeeMultiSigAddr), block, - "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 11_0000_0000 }); - }); - - ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); - ret.Should().BeOfType(); - ret.GetInteger().Should().Be(0); - - // Proper set - ret = NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(committeeMultiSigAddr), block, - "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 300300 }); - ret.IsNull.Should().BeTrue(); - - ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); - ret.Should().BeOfType(); - ret.GetInteger().Should().Be(300300); - - // Set to zero - ret = NativeContract.Policy.Call(snapshot, new Nep17NativeContractExtensions.ManualWitness(committeeMultiSigAddr), block, - "setConflictsFee", new ContractParameter(ContractParameterType.Integer) { Value = 0 }); - ret.IsNull.Should().BeTrue(); - - ret = NativeContract.Policy.Call(snapshot, "getConflictsFee"); - ret.Should().BeOfType(); - ret.GetInteger().Should().Be(0); - } - [TestMethod] public void Check_BlockAccount() { From 024a82a3e40e2d827f60991bc5089588e6d49cd3 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 10 Oct 2023 17:15:20 +0300 Subject: [PATCH 5/8] 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 --- src/Neo/SmartContract/Native/LedgerContract.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 3a490e204c..0f48c5e99f 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -47,9 +47,9 @@ 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) { - // Remove possible previously saved malicious conflict records for the transaction (if any). - foreach (var (key, _) in engine.Snapshot.Find(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash).ToArray())) - engine.Snapshot.Delete(key); + // It's possible that there are previously saved malicious conflict records for this transaction. + // These records are garbage in fact and can be removed in a separate flow, but at the same time + // it won't hurt to keep them. engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx)); From c6105c0d09c63667a44e17f846adb79b98af357d Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 7 Nov 2023 13:52:01 +0300 Subject: [PATCH 6/8] 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 --- src/Neo/SmartContract/Native/LedgerContract.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 0f48c5e99f..598464dd23 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -48,19 +48,23 @@ internal override ContractTask OnPersist(ApplicationEngine engine) foreach (TransactionState tx in transactions) { // It's possible that there are previously saved malicious conflict records for this transaction. - // These records are garbage in fact and can be removed in a separate flow, but at the same time - // it won't hurt to keep them. - - engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash), new StorageItem(tx)); + // If so, then remove it and store the relevant transaction itself. + var txKey = CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash); + engine.Snapshot.Delete(txKey); + engine.Snapshot.Add(txKey, new StorageItem(tx)); // Store transaction's conflicits. var conflictingSigners = tx.Transaction.Signers.Select(s => s.Account); foreach (var attr in tx.Transaction.GetAttributes()) { - engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(attr.Hash), new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + var recordKey = CreateStorageKey(Prefix_Transaction).Add(attr.Hash); + engine.Snapshot.Delete(recordKey); + engine.Snapshot.Add(recordKey, new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); foreach (var signer in conflictingSigners) { - engine.Snapshot.Add(CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer), new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + var conflictKey = CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer); + engine.Snapshot.Delete(conflictKey); + engine.Snapshot.Add(conflictKey, new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); } } } From 32d13415b2327aa23b74d402ed27811d10e48a26 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 7 Nov 2023 13:55:14 +0300 Subject: [PATCH 7/8] Throw proper exception if TestMaliciousOnChainConflict fails Signed-off-by: Anna Shaleva --- tests/Neo.UnitTests/Ledger/UT_Blockchain.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs b/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs index 0a2d0eb013..7ecb965ff5 100644 --- a/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs +++ b/tests/Neo.UnitTests/Ledger/UT_Blockchain.cs @@ -152,7 +152,7 @@ public void TestMaliciousOnChainConflict() 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(); + if (engine2.Execute() != VMState.HALT) throw engine2.FaultException; engine2.Snapshot.Commit(); } snapshot.Commit(); @@ -168,7 +168,7 @@ public void TestMaliciousOnChainConflict() using (ApplicationEngine engine2 = ApplicationEngine.Create(TriggerType.PostPersist, null, snapshot, block, TestBlockchain.TheNeoSystem.Settings, 0)) { engine2.LoadScript(postPersistScript); - if (engine2.Execute() != VMState.HALT) throw new InvalidOperationException(); + if (engine2.Execute() != VMState.HALT) throw engine2.FaultException; engine2.Snapshot.Commit(); } snapshot.Commit(); From 98456099d0eac9ef8427fa476d4b2f79d7b95fed Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Tue, 7 Nov 2023 16:15:30 +0300 Subject: [PATCH 8/8] 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 --- src/Neo/SmartContract/Native/LedgerContract.cs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/Neo/SmartContract/Native/LedgerContract.cs b/src/Neo/SmartContract/Native/LedgerContract.cs index 598464dd23..b95c518518 100644 --- a/src/Neo/SmartContract/Native/LedgerContract.cs +++ b/src/Neo/SmartContract/Native/LedgerContract.cs @@ -49,22 +49,16 @@ internal override ContractTask OnPersist(ApplicationEngine engine) { // 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. - var txKey = CreateStorageKey(Prefix_Transaction).Add(tx.Transaction.Hash); - engine.Snapshot.Delete(txKey); - engine.Snapshot.Add(txKey, new StorageItem(tx)); + 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 recordKey = CreateStorageKey(Prefix_Transaction).Add(attr.Hash); - engine.Snapshot.Delete(recordKey); - engine.Snapshot.Add(recordKey, new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + 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) { - var conflictKey = CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer); - engine.Snapshot.Delete(conflictKey); - engine.Snapshot.Add(conflictKey, new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); + engine.Snapshot.GetAndChange(CreateStorageKey(Prefix_Transaction).Add(attr.Hash).Add(signer), () => new StorageItem(new TransactionState())).FromReplica(new StorageItem(new TransactionState() { BlockIndex = engine.PersistingBlock.Index })); } } }