Refactor tx filters (#3626)

* change ITxFilter to be like IIncomingTxFilter * introduce struct AcceptTxResult * fixes * fix baseline? * fix baseline? * refactor * cosmetic * move AuRa code * cosmetic * added more WithMessage * Equals() and GetHashCode() overriden * add implicit bool cast, refactor comparisons * remove redundant ToString() * nameof * fix tests * refactor in one more place * changes in TxPoolSender * refactor few last places * override value equality operators * fix * requested changes
NethermindEth · Dec 6, 2021 · 9b68ec0 · 9b68ec0
1 parent ddf3727
commit 9b68ec0
Show file tree

Hide file tree

Showing 50 changed files with 295 additions and 227 deletions.
diff --git a/src/Nethermind/Nethermind.AuRa.Test/AuraBlockProcessorTests.cs b/src/Nethermind/Nethermind.AuRa.Test/AuraBlockProcessorTests.cs
@@ -35,6 +35,7 @@
 using Nethermind.Specs;
 using Nethermind.State;
 using Nethermind.Trie.Pruning;
+using Nethermind.TxPool;
 using NSubstitute;
 using NUnit.Framework;
 
@@ -64,7 +65,7 @@ public void For_not_empty_block_tx_filter_should_be_called()
             ITxFilter txFilter = Substitute.For<ITxFilter>();
             txFilter
                 .IsAllowed(Arg.Any<Transaction>(), Arg.Any<BlockHeader>())
-                .Returns((true, string.Empty));
+                .Returns(AcceptTxResult.Accepted);
             AuRaBlockProcessor processor = CreateProcessor(txFilter);
 
             BlockHeader header = Build.A.BlockHeader.WithAuthor(TestItem.AddressD).WithNumber(3).TestObject;

diff --git a/src/Nethermind/Nethermind.AuRa.Test/Transactions/MinGasPriceContractTxFilterTests.cs b/src/Nethermind/Nethermind.AuRa.Test/Transactions/MinGasPriceContractTxFilterTests.cs
@@ -26,6 +26,7 @@
 using Nethermind.Core.Specs;
 using Nethermind.Core.Test.Builders;
 using Nethermind.Int256;
+using Nethermind.TxPool;
 using NSubstitute;
 using NUnit.Framework;
 
@@ -62,7 +63,7 @@ public bool is_allowed_returns_correct(Address address, ulong gasLimit)
             MinGasPriceContractTxFilter txFilter = new(minGasPriceFilter, dictionaryContractDataStore);
             Transaction tx = Build.A.Transaction.WithTo(address).WithGasPrice(gasLimit).WithData(null).TestObject;
 
-            return txFilter.IsAllowed(tx, Build.A.BlockHeader.TestObject).Allowed;
+            return txFilter.IsAllowed(tx, Build.A.BlockHeader.TestObject);
         }
     }
 }
diff --git a/src/Nethermind/Nethermind.AuRa.Test/Transactions/TxCertifierFilterTests.cs b/src/Nethermind/Nethermind.AuRa.Test/Transactions/TxCertifierFilterTests.cs
@@ -32,6 +32,7 @@
 using Nethermind.Core.Test.Builders;
 using Nethermind.Logging;
 using Nethermind.Trie.Pruning;
+using Nethermind.TxPool;
 using NSubstitute;
 using NSubstitute.ExceptionExtensions;
 using NUnit.Framework;
@@ -53,7 +54,7 @@ public void SetUp()
             _specProvider = Substitute.For<ISpecProvider>();
 
             _notCertifiedFilter.IsAllowed(Arg.Any<Transaction>(), Arg.Any<BlockHeader>())
-                .Returns((false, string.Empty));
+                .Returns(AcceptTxResult.Invalid);
 
             _certifierContract.Certified(Arg.Any<BlockHeader>(), 
                 Arg.Is<Address>(a => TestItem.Addresses.Take(3).Contains(a)))
@@ -90,7 +91,7 @@ public void should_not_allow_addresses_on_contract_error()
         public void should_default_to_inner_contract_on_non_zero_transactions(bool expected)
         {
             _notCertifiedFilter.IsAllowed(Arg.Any<Transaction>(), Arg.Any<BlockHeader>())
-                .Returns((expected, string.Empty));
+                .Returns(expected ? AcceptTxResult.Accepted : AcceptTxResult.Invalid);
 
             ShouldAllowAddress(TestItem.Addresses.First(), 1ul, expected);
         }
@@ -99,7 +100,7 @@ private void ShouldAllowAddress(Address address, ulong gasPrice = 0ul, bool expe
         {
             _filter.IsAllowed(
                 Build.A.Transaction.WithGasPrice(gasPrice).WithSenderAddress(address).TestObject,
-                Build.A.BlockHeader.TestObject).Allowed.Should().Be(expected);
+                Build.A.BlockHeader.TestObject).Equals(AcceptTxResult.Accepted).Should().Be(expected);
         }
 
         [Test]

diff --git a/src/Nethermind/Nethermind.AuRa.Test/Transactions/TxPermissionFilterTest.cs b/src/Nethermind/Nethermind.AuRa.Test/Transactions/TxPermissionFilterTest.cs
@@ -42,6 +42,7 @@
 using Nethermind.Specs.ChainSpecStyle;
 using Nethermind.State;
 using Nethermind.Trie.Pruning;
+using Nethermind.TxPool;
 using NSubstitute;
 using NUnit.Framework;
 
@@ -216,9 +217,9 @@ public static IEnumerable<TestCaseData> V4Tests()
         {
             using TestTxPermissionsBlockchain chain = await chainFactory();
             Block? head = chain.BlockTree.Head;
-            (bool Allowed, string Reason) isAllowed = chain.PermissionBasedTxFilter.IsAllowed(tx, head.Header);
+            AcceptTxResult isAllowed = chain.PermissionBasedTxFilter.IsAllowed(tx, head.Header);
             chain.TransactionPermissionContractVersions.Get(head.Header.Hash).Should().Be(version);
-            return (isAllowed.Allowed, chain.TxPermissionFilterCache.Permissions.Contains((head.Hash, tx.SenderAddress)));
+            return (isAllowed, chain.TxPermissionFilterCache.Permissions.Contains((head.Hash, tx.SenderAddress)));
         }
 
         private static IEnumerable<TestCaseData> GetTestCases(IEnumerable<Test> tests, string testsName, Func<Test, ITransactionPermissionContract.TxPermissions, TransactionBuilder<Transaction>> transactionBuilder)
@@ -267,7 +268,7 @@ public bool allows_transactions_before_transitions(long blockNumber)
                 Substitute.For<ISpecProvider>());
 
             PermissionBasedTxFilter filter = new(transactionPermissionContract, new PermissionBasedTxFilter.Cache(), LimboLogs.Instance);
-            return filter.IsAllowed(Build.A.Transaction.WithSenderAddress(TestItem.AddressB).TestObject, Build.A.BlockHeader.WithNumber(blockNumber).TestObject).Allowed;
+            return filter.IsAllowed(Build.A.Transaction.WithSenderAddress(TestItem.AddressB).TestObject, Build.A.BlockHeader.WithNumber(blockNumber).TestObject);
         }
 
         public class TestTxPermissionsBlockchain : TestContractBlockchain

diff --git a/src/Nethermind/Nethermind.Baseline.Test/BaselineTreeTracker_ReorganizationTests.cs b/src/Nethermind/Nethermind.Baseline.Test/BaselineTreeTracker_ReorganizationTests.cs
@@ -90,10 +90,10 @@ private void InsertLeafFromArray(Keccak[] transactions, TestRpcBlockchain testRp
                 transaction.SenderAddress = key.Address;
                 ecdsa.Sign(key, transaction, true);
                 transaction.Hash = transaction.CalculateHash();
-                AddTxResult result = testRpc.TxPool.SubmitTx(transaction, TxHandlingOptions.None);
-                if (result != AddTxResult.Added)
+                AcceptTxResult isAllowed = testRpc.TxPool.SubmitTx(transaction, TxHandlingOptions.None);
+                if (!isAllowed)
                 {
-                    throw new Exception("failed to add " + result);
+                    throw new Exception("failed to add " + isAllowed);
                 }
             }
         }
@@ -111,10 +111,10 @@ private void InsertLeavesFromArray(Keccak[][] transactions, TestRpcBlockchain te
                 transaction.SenderAddress = key.Address;
                 ecdsa.Sign(key, transaction, true);
                 transaction.Hash = transaction.CalculateHash();
-                AddTxResult result = testRpc.TxPool.SubmitTx(transaction, TxHandlingOptions.None);
-                if (result != AddTxResult.Added)
+                AcceptTxResult isAllowed = testRpc.TxPool.SubmitTx(transaction, TxHandlingOptions.None);
+                if (!isAllowed)
                 {
-                    throw new Exception("failed to add " + result);
+                    throw new Exception("failed to add " + isAllowed);
                 }
             }
         }

diff --git a/src/Nethermind/Nethermind.Blockchain.Test/Producers/BlockProducerBaseTests.BaseFee.cs b/src/Nethermind/Nethermind.Blockchain.Test/Producers/BlockProducerBaseTests.BaseFee.cs
@@ -134,7 +134,7 @@ private async Task<ScenarioBuilder> SendTransactionAsync(long gasLimit, UInt256
                     };
 
                     var (_, result) = await _testRpcBlockchain.TxSender.SendTransaction(tx, TxHandlingOptions.None);
-                    Assert.AreEqual(AddTxResult.Added, result);
+                    Assert.AreEqual(AcceptTxResult.Accepted, result);
                     return this;
                 }
 

diff --git a/src/Nethermind/Nethermind.Blockchain/TxFilterAdapter.cs b/src/Nethermind/Nethermind.Blockchain/TxFilterAdapter.cs
@@ -37,23 +37,23 @@ public TxFilterAdapter(IBlockTree blockTree, ITxFilter txFilter, ILogManager log
             _blockTree = blockTree ?? throw new ArgumentNullException(nameof(blockTree));
         }
 
-        public (bool Accepted, AddTxResult? Reason) Accept(Transaction tx, TxHandlingOptions txHandlingOptions)
+        public AcceptTxResult Accept(Transaction tx, TxHandlingOptions txHandlingOptions)
         {
             if (tx is not GeneratedTransaction)
             {
                 BlockHeader parentHeader = _blockTree.Head?.Header;
-                if (parentHeader == null) return (true, null);
+                if (parentHeader == null) return AcceptTxResult.Accepted;
 
-                (bool accepted, string? reason) = _txFilter.IsAllowed(tx, parentHeader);
-                if (reason is not null)
+                AcceptTxResult isAllowed = _txFilter.IsAllowed(tx, parentHeader);
+                if (!isAllowed)
                 {
-                    if (_logger.IsTrace) _logger.Trace($"Skipped adding transaction {tx.ToString("  ")}, filtered ({reason}).");
+                    if (_logger.IsTrace) _logger.Trace($"Skipped adding transaction {tx.ToString("  ")}, filtered ({isAllowed}).");
                 }
 
-                return (accepted, reason is null ? null : AddTxResult.Filtered);
+                return isAllowed;
             }
 
-            return (true, null);
+            return AcceptTxResult.Accepted;
         }
     }
 }
diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/AuRaBlockProcessor.cs b/src/Nethermind/Nethermind.Consensus.AuRa/AuRaBlockProcessor.cs
@@ -31,6 +31,7 @@
 using Nethermind.Evm.Tracing;
 using Nethermind.Logging;
 using Nethermind.State;
+using Nethermind.TxPool;
 
 namespace Nethermind.Consensus.AuRa
 {
@@ -138,7 +139,7 @@ private void OnAddingTransaction(object? sender, AddingTxEventArgs e)
 
         private AddingTxEventArgs CheckTxPosdaoRules(AddingTxEventArgs args)
         {
-            (bool Allowed, string Reason)? TryRecoverSenderAddress(Transaction tx, BlockHeader header)
+            AcceptTxResult? TryRecoverSenderAddress(Transaction tx, BlockHeader header)
             {
                 if (tx.Signature != null)
                 {
@@ -157,15 +158,15 @@ private AddingTxEventArgs CheckTxPosdaoRules(AddingTxEventArgs args)
             }
 
             BlockHeader parentHeader = GetParentHeader(args.Block);
-            (bool Allowed, string Reason) txFilterResult = _txFilter.IsAllowed(args.Transaction, parentHeader);
-            if (!txFilterResult.Allowed)
+            AcceptTxResult isAllowed = _txFilter.IsAllowed(args.Transaction, parentHeader);
+            if (!isAllowed)
             {
-                txFilterResult = TryRecoverSenderAddress(args.Transaction, parentHeader) ?? txFilterResult;
+                isAllowed = TryRecoverSenderAddress(args.Transaction, parentHeader) ?? isAllowed;
             }
 
-            if (!txFilterResult.Allowed)
+            if (!isAllowed)
             {
-                args.Set(TxAction.Skip, txFilterResult.Reason);
+                args.Set(TxAction.Skip, isAllowed.ToString());
             }
 
             return args;

diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/AcceptTxResultAuRa.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/AcceptTxResultAuRa.cs
@@ -0,0 +1,29 @@
+//  Copyright (c) 2021 Demerzel Solutions Limited
+//  This file is part of the Nethermind library.
+// 
+//  The Nethermind library is free software: you can redistribute it and/or modify
+//  it under the terms of the GNU Lesser General Public License as published by
+//  the Free Software Foundation, either version 3 of the License, or
+//  (at your option) any later version.
+// 
+//  The Nethermind library is distributed in the hope that it will be useful,
+//  but WITHOUT ANY WARRANTY; without even the implied warranty of
+//  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+//  GNU Lesser General Public License for more details.
+// 
+//  You should have received a copy of the GNU Lesser General Public License
+//  along with the Nethermind. If not, see <http://www.gnu.org/licenses/>.
+// 
+
+using Nethermind.TxPool;
+
+namespace Nethermind.Consensus.AuRa.Transactions
+{
+    public readonly struct AcceptTxResultAuRa
+    {
+        /// <summary>
+        /// Permission denied for this tx type.
+        /// </summary>
+        public static readonly AcceptTxResult PermissionDenied = new(100, nameof(PermissionDenied));
+    }
+}
diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/LocalTxFilter.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/LocalTxFilter.cs
@@ -18,6 +18,7 @@
 using System;
 using Nethermind.Consensus.Transactions;
 using Nethermind.Core;
+using Nethermind.TxPool;
 
 namespace Nethermind.Consensus.AuRa.Transactions
 {
@@ -30,14 +31,14 @@ public LocalTxFilter(ISigner signer)
             _signer = signer;
         }
 
-        public (bool Allowed, string Reason) IsAllowed(Transaction tx, BlockHeader parentHeader)
+        public AcceptTxResult IsAllowed(Transaction tx, BlockHeader parentHeader)
         {
             if (tx.SenderAddress == _signer.Address)
             {
                 tx.IsServiceTransaction = true;
             }
 
-            return (true, string.Empty);
+            return AcceptTxResult.Accepted;
         }
     }
 }
diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/MinGasPriceContractTxFilter.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/MinGasPriceContractTxFilter.cs
@@ -20,6 +20,7 @@
 using Nethermind.Consensus.AuRa.Contracts.DataStore;
 using Nethermind.Consensus.Transactions;
 using Nethermind.Core;
+using Nethermind.TxPool;
 
 namespace Nethermind.Consensus.AuRa.Transactions
 {
@@ -35,19 +36,19 @@ public MinGasPriceContractTxFilter(IMinGasPriceTxFilter minGasPriceFilter, IDict
         }
 
 
-        public (bool Allowed, string Reason) IsAllowed(Transaction tx, BlockHeader parentHeader)
+        public AcceptTxResult IsAllowed(Transaction tx, BlockHeader parentHeader)
         {
-            (bool Allowed, string Reason) result = _minGasPriceFilter.IsAllowed(tx, parentHeader);
-            if (!result.Allowed)
+            AcceptTxResult isAllowed = _minGasPriceFilter.IsAllowed(tx, parentHeader);
+            if (!isAllowed)
             {
-                return result;
+                return isAllowed;
             }
             else if (_minGasPrices.TryGetValue(parentHeader, tx, out TxPriorityContract.Destination @override))
             {
                 return _minGasPriceFilter.IsAllowed(tx, parentHeader, @override.Value);
             }
 
-            return (true, string.Empty);
+            return AcceptTxResult.Accepted;
         }
     }
 }
diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/ServiceTxFilter.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/ServiceTxFilter.cs
@@ -19,6 +19,7 @@
 using Nethermind.Consensus.Transactions;
 using Nethermind.Core;
 using Nethermind.Core.Specs;
+using Nethermind.TxPool;
 
 namespace Nethermind.Consensus.AuRa.Transactions
 {
@@ -31,14 +32,14 @@ public ServiceTxFilter(ISpecProvider specProvider)
             _specProvider = specProvider;
         }
 
-        public (bool Allowed, string Reason) IsAllowed(Transaction tx, BlockHeader parentHeader)
+        public AcceptTxResult IsAllowed(Transaction tx, BlockHeader parentHeader)
         {
             if (tx.IsZeroGasPrice(parentHeader, _specProvider))
             {
                 tx.IsServiceTransaction = true;
             }
 
-            return (true, string.Empty);
+            return AcceptTxResult.Accepted;
         }
     }
 }
diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/TxCertifierFilter.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/TxCertifierFilter.cs
@@ -27,6 +27,7 @@
 using Nethermind.Core.Resettables;
 using Nethermind.Core.Specs;
 using Nethermind.Logging;
+using Nethermind.TxPool;
 
 namespace Nethermind.Consensus.AuRa.Transactions
 {
@@ -47,8 +48,8 @@ public TxCertifierFilter(ICertifierContract certifierContract, ITxFilter notCert
             _logger = logManager?.GetClassLogger<TxCertifierFilter>() ?? throw new ArgumentNullException(nameof(logManager));
         }
 
-        public (bool Allowed, string Reason) IsAllowed(Transaction tx, BlockHeader parentHeader) => 
-            IsCertified(tx, parentHeader) ? (true, string.Empty) : _notCertifiedFilter.IsAllowed(tx, parentHeader);
+        public AcceptTxResult IsAllowed(Transaction tx, BlockHeader parentHeader) => 
+            IsCertified(tx, parentHeader) ? AcceptTxResult.Accepted : _notCertifiedFilter.IsAllowed(tx, parentHeader);
 
         private bool IsCertified(Transaction tx, BlockHeader parentHeader)
         {

diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/TxGasPriceSender.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/TxGasPriceSender.cs
@@ -42,7 +42,7 @@ public TxGasPriceSender(
             _percentDelta = percentDelta;
         }
 
-        public ValueTask<(Keccak, AddTxResult?)> SendTransaction(Transaction tx, TxHandlingOptions txHandlingOptions)
+        public ValueTask<(Keccak, AcceptTxResult?)> SendTransaction(Transaction tx, TxHandlingOptions txHandlingOptions)
         {
             UInt256 gasPriceEstimated = _gasPriceOracle.GetGasPriceEstimate() * _percentDelta / 100;
             tx.DecodedMaxFeePerGas = gasPriceEstimated;

diff --git a/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/TxPermissionFilter.cs b/src/Nethermind/Nethermind.Consensus.AuRa/Transactions/TxPermissionFilter.cs
@@ -25,6 +25,7 @@
 using Nethermind.Core.Crypto;
 using Nethermind.Logging;
 using Nethermind.State;
+using Nethermind.TxPool;
 
 namespace Nethermind.Consensus.AuRa.Transactions
 {
@@ -44,18 +45,18 @@ public PermissionBasedTxFilter(
             _logger = logManager?.GetClassLogger<PermissionBasedTxFilter>() ?? throw new ArgumentNullException(nameof(logManager));
         }
 
-        public (bool Allowed, string Reason) IsAllowed(Transaction tx, BlockHeader parentHeader)
+        public AcceptTxResult IsAllowed(Transaction tx, BlockHeader parentHeader)
         {
             if (parentHeader.Number + 1 < _contract.Activation)
             {
-                return (true, string.Empty);
+                return AcceptTxResult.Accepted;
             }
 
             var txPermissions = GetPermissions(tx, parentHeader);
             var txType = GetTxType(tx, txPermissions.ContractExists);
             if (_logger.IsTrace) _logger.Trace($"Given transaction: {tx.Hash} sender: {tx.SenderAddress} to: {tx.To} value: {tx.Value}, gas_price: {tx.GasPrice}. " +
                                                $"Permissions required: {txType}, got: {txPermissions}.");
-            return (txPermissions.Permissions & txType) == txType ? (true, string.Empty) : (false, $"permission denied for tx type: {txType}, actual permissions: {txPermissions.Permissions}");
+            return (txPermissions.Permissions & txType) == txType ? AcceptTxResult.Accepted : AcceptTxResultAuRa.PermissionDenied.WithMessage($"permission denied for tx type: {txType}, actual permissions: {txPermissions.Permissions}");
         }
 
         private (ITransactionPermissionContract.TxPermissions Permissions, bool ContractExists) GetPermissions(Transaction tx, BlockHeader parentHeader)