Refactor `TxValidator` #7386

emlautarom1 · 2024-09-03T19:33:21Z

Partially solves #7313

Changes

Refactor TxValidator to dispatch transaction validation to appropriate sub validators.

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Notes on testing

All unit tests are passing.

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

Remarks

Please see the existing discussion.

- Reduce burden on implementators

- Construct per TxType validator

- Avoid code duplication through inheritance

- By default this validator does not check anything (existing behavior)

emlautarom1 · 2024-09-03T19:33:37Z

src/Nethermind/Nethermind.Blockchain.Test/Validators/TestTransactionValidator.cs

-
-namespace Nethermind.Blockchain.Test.Validators
-{
-    public class TestTxValidator : ITxValidator


Unused class

emlautarom1 · 2024-09-03T19:37:54Z

src/Nethermind/Nethermind.Optimism/OptimismTxValidator.cs

@@ -8,21 +8,11 @@

 namespace Nethermind.Optimism;

-public class OptimismTxValidator : ITxValidator
+public sealed class OptimismTxValidator : ITxValidator


The existing OptimismTxValidator always validates transactions that are of TxType.Deposit without performing any checks. This does not look correct since there are validations that are performed on all existing TxTypes like IntrinsicGasTxValidator but I might be missing some context.

maybe @jmederosalvarado knows more about this?

Deposit transactions are special types, I think there should only be one per block on first step. Ping @jmederosalvarado , @deffrian or @flcl42 about it.

DepositTxs are mostly validated on L1 smart contract, and later by the CL client. Whatever DepositTx the CL feeds to the EL client is already valid.

- Avoid allocations

- No need to separate things

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs

ak88 · 2024-09-05T12:08:05Z

src/Nethermind/Nethermind.Optimism/OptimismTxValidator.cs

@@ -8,21 +8,11 @@

 namespace Nethermind.Optimism;

-public class OptimismTxValidator : ITxValidator
+public sealed class OptimismTxValidator : ITxValidator


maybe @jmederosalvarado knows more about this?

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs

LukaszRozmej

Overall I like it, just some details

src/Nethermind/Nethermind.TxPool/ITxValidator.cs

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs

LukaszRozmej · 2024-09-06T08:23:31Z

src/Nethermind/Nethermind.Optimism/OptimismTxValidator.cs

@@ -8,21 +8,11 @@

 namespace Nethermind.Optimism;

-public class OptimismTxValidator : ITxValidator
+public sealed class OptimismTxValidator : ITxValidator


Deposit transactions are special types, I think there should only be one per block on first step. Ping @jmederosalvarado , @deffrian or @flcl42 about it.

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs

jmederosalvarado · 2024-09-09T10:41:22Z

src/Nethermind/Nethermind.Api/NethermindApi.cs

@@ -208,7 +208,7 @@ public ISealEngine SealEngine
        public ITxPoolInfoProvider? TxPoolInfoProvider { get; set; }
        public IHealthHintService? HealthHintService { get; set; }
        public IRpcCapabilitiesProvider? RpcCapabilitiesProvider { get; set; }
-        public ITxValidator? TxValidator { get; set; }
+        public TxValidator? TxValidator { get; set; }


I would like this to remain an interface. Any reason not to?

I had to change it to a concrete class since during initialization we might want to register validators for new TxTypes:

nethermind/src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

Line 77 in 9437715

_api.TxValidator.WithValidator(TxType.DepositTx, new OptimismTxValidator()),

jmederosalvarado · 2024-09-09T10:41:34Z

src/Nethermind/Nethermind.Api/IApiWithBlockchain.cs

@@ -74,7 +74,7 @@ public interface IApiWithBlockchain : IApiWithStores, IBlockchainBridgeFactory
        IHealthHintService? HealthHintService { get; set; }
        IRpcCapabilitiesProvider? RpcCapabilitiesProvider { get; set; }
        ITransactionComparerProvider? TransactionComparerProvider { get; set; }
-        ITxValidator? TxValidator { get; set; }
+        TxValidator? TxValidator { get; set; }


I would like this to remain an interface. Any reason not to?

Required due to the change on NethermindApi

jmederosalvarado · 2024-09-09T10:50:44Z

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs


-        public TxValidator(ulong chainId)
+    public TxValidator WithValidator(TxType type, ITxValidator validator)


Having something like WithValidator mutate the object itself looks weird. I'm not sure if this is how we do it in other parts of the code. I know we do it for object builders (e.g. TransactionBuilder) but not for the object itself

I would implement WithValidator as a clone, that is, return a new TxValidator with the added validator, but I thought that it made little sense to pay for that due to our use case. Do you have any specific suggestion?

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs

Co-authored-by: Jorge Mederos <46798594+jmederosalvarado@users.noreply.github.com>

# Conflicts: # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

emlautarom1 · 2024-09-09T19:25:51Z

src/Nethermind/Nethermind.Specs/ReleaseSpec.cs

+        public bool IsEip1559Enabled
+        {
+            get => _isEip1559Enabled || IsEip4844Enabled;
+            set => _isEip1559Enabled = value;
+        }


NOTE: we might want to revisit this later since EIPs can require other EIPs, and those can have their own requirements and so forth.

This approach is kind of fragile, for example, EIP 4844 requires 2930 but we're not overriding IsEip2930Enabled like we're doing with IsEip1559Enabled.

Yeah to be honest we should make sure that our release spec is following the spec, so have those cascades.

emlautarom1 · 2024-09-09T19:29:53Z

src/Nethermind/Nethermind.Core/ValidationResult.cs

+public record struct ValidationResult(string? Error)
+{
+    public static ValidationResult Success => new(null);
+    public static implicit operator bool(ValidationResult result) => result.AsBool();
+    public static implicit operator ValidationResult(string error) => new(error);
+    public override string ToString() => Error ?? "Success";
+    public bool AsBool() => Error is null;
+}


NOTE: We have several *Result types that could be replaced with a generic Result<E, T> (ex. ValidationResult, SearchResult, RpcResult, etc.)

Maybe but not sure, here we are using syntactic sugar with implicit conversion to bool, which allows us to treat it as a boolean value. Not sure if we can leverage that with generics easily and if it would be worth it - would have to see it.

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com> Co-authored-by: Jorge Mederos <46798594+jmederosalvarado@users.noreply.github.com>

emlautarom1 added 6 commits September 3, 2024 13:23

Add default interface implementation

f045e23

- Reduce burden on implementators

Separate all validators

6eeec81

- Construct per TxType validator

Merge signature validation

6cef6ef

- Avoid code duplication through inheritance

Move to primary constructor

a5758b8

Update OptimismTxValidator

cb523bd

- By default this validator does not check anything (existing behavior)

Reorder imports (avoid diff)

f6f0c92

emlautarom1 requested review from flcl42, ak88, LukaszRozmej, deffrian and jmederosalvarado September 3, 2024 19:33

emlautarom1 commented Sep 3, 2024

View reviewed changes

emlautarom1 and others added 8 commits September 3, 2024 16:39

IntrinsicGasTxValidator: Use singleton

7d85f1e

- Avoid allocations

ContractSizeTxValidator: Use singleton

17ff03f

- Avoid allocations

NonBlobFieldsTxValidator: Use singleton

37f626d

- Avoid allocations

SignatureTxValidator: Use singleton

53f184c

- Avoid allocations

GasFieldsTxValidator: Use singleton

5f5dce2

- Avoid allocations

Use singleton for BlobTxValidator internals

dd15a56

Merge branch 'master' into refactor/tx-validator

b416151

Move everything to the same file

1a95ca6

- No need to separate things

ak88 approved these changes Sep 5, 2024

View reviewed changes

smartprogrammer93 approved these changes Sep 6, 2024

View reviewed changes

LukaszRozmej reviewed Sep 6, 2024

View reviewed changes

emlautarom1 added 6 commits September 6, 2024 11:48

Rename AddValidator to WithValidator

1430dc5

Use <remarks> instead of <summary>

750e3fa

Remove unnecessary nullability annotation

84dce88

Rename AdditonalCheck to ValidateChainId

86bc383

Use static modifier in lambdas when possible

1691e20

Remove IsWellFormed with no error message

644ac9a

emlautarom1 and others added 6 commits September 6, 2024 12:29

Add Blob EIP4844 no EIP1559 validation test

d4091c9

Change <summary> to <remark>

fff2f90

Add bad gas fields test with EIP1559 disabled

5e14c71

Add remarks to tests

04d7fff

Use small array instead of Dictionary

840d3a3

Merge branch 'master' into refactor/tx-validator

9437715

LukaszRozmej reviewed Sep 8, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Outdated Show resolved Hide resolved

LukaszRozmej mentioned this pull request Sep 8, 2024

Refactor 'TxValidator' further #7402

Merged

12 tasks

jmederosalvarado reviewed Sep 9, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Outdated Show resolved Hide resolved

LukaszRozmej and others added 2 commits September 9, 2024 19:46

Refactor 'TxValidator' further (#7402)

ec0d5a2

Co-authored-by: Jorge Mederos <46798594+jmederosalvarado@users.noreply.github.com>

Merge remote-tracking branch 'origin/master' into refactor/tx-validator

0b5d838

# Conflicts: # src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs

LukaszRozmej requested review from LukaszRozmej and jmederosalvarado September 9, 2024 17:51

LukaszRozmej approved these changes Sep 9, 2024

View reviewed changes

emlautarom1 commented Sep 9, 2024

View reviewed changes

jmederosalvarado approved these changes Sep 10, 2024

View reviewed changes

emlautarom1 merged commit 030b32f into master Sep 10, 2024
66 checks passed

emlautarom1 deleted the refactor/tx-validator branch September 10, 2024 16:11

rjnrohit pushed a commit that referenced this pull request Sep 12, 2024

Refactor TxValidator (#7386)

69e36dd

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com> Co-authored-by: Jorge Mederos <46798594+jmederosalvarado@users.noreply.github.com>

emlautarom1 mentioned this pull request Sep 18, 2024

[WIP] Pectra eip7702 consolidations #7449

Closed

16 tasks

deffrian pushed a commit that referenced this pull request Oct 28, 2024

Refactor TxValidator (#7386)

deaa395

Co-authored-by: Lukasz Rozmej <lukasz.rozmej@gmail.com> Co-authored-by: Jorge Mederos <46798594+jmederosalvarado@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `TxValidator` #7386

Refactor `TxValidator` #7386

emlautarom1 commented Sep 3, 2024

emlautarom1 Sep 3, 2024

emlautarom1 Sep 3, 2024 •

edited

Loading

ak88 Sep 5, 2024

LukaszRozmej Sep 6, 2024

jmederosalvarado Sep 9, 2024

ak88 Sep 5, 2024

LukaszRozmej left a comment

LukaszRozmej Sep 6, 2024

jmederosalvarado Sep 9, 2024

emlautarom1 Sep 9, 2024

jmederosalvarado Sep 9, 2024

emlautarom1 Sep 9, 2024

jmederosalvarado Sep 9, 2024

emlautarom1 Sep 9, 2024

emlautarom1 Sep 9, 2024 •

edited

Loading

LukaszRozmej Sep 9, 2024

emlautarom1 Sep 9, 2024 •

edited

Loading

LukaszRozmej Sep 9, 2024


		public TxValidator(ulong chainId)
		public TxValidator WithValidator(TxType type, ITxValidator validator)

Refactor TxValidator #7386

Refactor TxValidator #7386

Conversation

emlautarom1 commented Sep 3, 2024

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Notes on testing

Documentation

Requires documentation update

Requires explanation in Release Notes

Remarks

Choose a reason for hiding this comment

emlautarom1 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaszRozmej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `TxValidator` #7386

Refactor `TxValidator` #7386

emlautarom1 Sep 3, 2024 •

edited

Loading

emlautarom1 Sep 9, 2024 •

edited

Loading

emlautarom1 Sep 9, 2024 •

edited

Loading