Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor TxValidator #7386

Merged
merged 28 commits into from
Sep 10, 2024
Merged

Refactor TxValidator #7386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
f045e23
Add default interface implementation
emlautarom1 Sep 3, 2024
6eeec81
Separate all validators
emlautarom1 Sep 3, 2024
6cef6ef
Merge signature validation
emlautarom1 Sep 3, 2024
a5758b8
Move to primary constructor
emlautarom1 Sep 3, 2024
cb523bd
Update `OptimismTxValidator`
emlautarom1 Sep 3, 2024
f6f0c92
Reorder imports (avoid diff)
emlautarom1 Sep 3, 2024
7d85f1e
`IntrinsicGasTxValidator`: Use singleton
emlautarom1 Sep 3, 2024
17ff03f
`ContractSizeTxValidator`: Use singleton
emlautarom1 Sep 3, 2024
37f626d
`NonBlobFieldsTxValidator`: Use singleton
emlautarom1 Sep 3, 2024
53f184c
`SignatureTxValidator`: Use singleton
emlautarom1 Sep 3, 2024
5f5dce2
`GasFieldsTxValidator`: Use singleton
emlautarom1 Sep 3, 2024
dd15a56
Use singleton for BlobTxValidator internals
emlautarom1 Sep 3, 2024
b416151
Merge branch 'master' into refactor/tx-validator
emlautarom1 Sep 4, 2024
1a95ca6
Move everything to the same file
emlautarom1 Sep 4, 2024
1430dc5
Rename `AddValidator` to `WithValidator`
emlautarom1 Sep 6, 2024
750e3fa
Use `<remarks>` instead of `<summary>`
emlautarom1 Sep 6, 2024
84dce88
Remove unnecessary nullability annotation
emlautarom1 Sep 6, 2024
86bc383
Rename `AdditonalCheck` to `ValidateChainId`
emlautarom1 Sep 6, 2024
1691e20
Use `static` modifier in lambdas when possible
emlautarom1 Sep 6, 2024
644ac9a
Remove `IsWellFormed` with no error message
emlautarom1 Sep 6, 2024
d4091c9
Add Blob EIP4844 no EIP1559 validation test
emlautarom1 Sep 6, 2024
fff2f90
Change `<summary>` to `<remark>`
emlautarom1 Sep 6, 2024
5e14c71
Add `bad gas fields` test with EIP1559 disabled
emlautarom1 Sep 6, 2024
04d7fff
Add remarks to tests
emlautarom1 Sep 6, 2024
840d3a3
Use small array instead of `Dictionary`
emlautarom1 Sep 6, 2024
9437715
Merge branch 'master' into refactor/tx-validator
emlautarom1 Sep 6, 2024
ec0d5a2
Refactor 'TxValidator' further (#7402)
LukaszRozmej Sep 9, 2024
0b5d838
Merge remote-tracking branch 'origin/master' into refactor/tx-validator
LukaszRozmej Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Nethermind/Nethermind.Api/IApiWithBlockchain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required due to the change on NethermindApi


/// <summary>
/// Manager of block finalization
Expand Down
16 changes: 16 additions & 0 deletions src/Nethermind/Nethermind.Api/INethermindApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
// SPDX-License-Identifier: LGPL-3.0-only

#nullable enable
using System;
using Nethermind.Config;
using Nethermind.Core;
using Nethermind.Serialization.Rlp;
using Nethermind.Serialization.Rlp.TxDecoders;
using Nethermind.TxPool;

namespace Nethermind.Api
{
Expand All @@ -15,4 +20,15 @@ public T Config<T>() where T : IConfig

(IApiWithNetwork GetFromApi, INethermindApi SetInApi) ForRpc => (this, this);
}

public static class NethermindApiExtensions
{
public static void RegisterTxType(this INethermindApi api, TxType type, ITxDecoder decoder, ITxValidator validator)
{
ArgumentNullException.ThrowIfNull(api.TxValidator);

api.TxValidator.RegisterValidator(type, validator);
TxDecoder.Instance.RegisterDecoder(decoder);
}
}
}
2 changes: 1 addition & 1 deletion src/Nethermind/Nethermind.Api/NethermindApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()),

public IBlockFinalizationManager? FinalizationManager { get; set; }
public IGasLimitCalculator? GasLimitCalculator { get; set; }

Expand Down
40 changes: 0 additions & 40 deletions src/Nethermind/Nethermind.Blockchain.Test/Validators/TestTransactionValidator.cs
View file
Open in desktop

This file was deleted.

42 changes: 17 additions & 25 deletions src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public void Zero_r_is_not_valid()
Transaction tx = Build.A.Transaction.WithSignature(signature).TestObject;

TxValidator txValidator = new(TestBlockchainIds.ChainId);
txValidator.IsWellFormed(tx, MuirGlacier.Instance).Should().BeFalse();
txValidator.IsWellFormed(tx, MuirGlacier.Instance).AsBool().Should().BeFalse();
}

private static byte CalculateV() => (byte)EthereumEcdsa.CalculateV(TestBlockchainIds.ChainId);
Expand All @@ -72,7 +72,7 @@ public void Zero_s_is_not_valid()
Transaction tx = Build.A.Transaction.WithSignature(signature).TestObject;

TxValidator txValidator = new(TestBlockchainIds.ChainId);
txValidator.IsWellFormed(tx, MuirGlacier.Instance).Should().BeFalse();
txValidator.IsWellFormed(tx, MuirGlacier.Instance).AsBool().Should().BeFalse();
}

[Test, Timeout(Timeout.MaxTestTime)]
Expand All @@ -86,7 +86,7 @@ public void Bad_chain_id_is_not_valid()
Transaction tx = Build.A.Transaction.WithSignature(signature).TestObject;

TxValidator txValidator = new(TestBlockchainIds.ChainId);
txValidator.IsWellFormed(tx, MuirGlacier.Instance).Should().BeFalse();
txValidator.IsWellFormed(tx, MuirGlacier.Instance).AsBool().Should().BeFalse();
}

[Test, Timeout(Timeout.MaxTestTime)]
Expand All @@ -100,7 +100,7 @@ public void No_chain_id_legacy_tx_is_valid()
Transaction tx = Build.A.Transaction.WithSignature(signature).TestObject;

TxValidator txValidator = new(TestBlockchainIds.ChainId);
txValidator.IsWellFormed(tx, MuirGlacier.Instance).Should().BeTrue();
txValidator.IsWellFormed(tx, MuirGlacier.Instance).AsBool().Should().BeTrue();
}

[Test, Timeout(Timeout.MaxTestTime)]
Expand All @@ -114,7 +114,7 @@ public void Is_valid_with_valid_chain_id()
Transaction tx = Build.A.Transaction.WithSignature(signature).TestObject;

TxValidator txValidator = new(TestBlockchainIds.ChainId);
txValidator.IsWellFormed(tx, MuirGlacier.Instance).Should().BeTrue();
txValidator.IsWellFormed(tx, MuirGlacier.Instance).AsBool().Should().BeTrue();
}

[Timeout(Timeout.MaxTestTime)]
Expand All @@ -134,7 +134,7 @@ public void Before_eip_155_has_to_have_valid_chain_id_unless_overridden(bool val
releaseSpec.ValidateChainId.Returns(validateChainId);

TxValidator txValidator = new(TestBlockchainIds.ChainId);
txValidator.IsWellFormed(tx, releaseSpec).Should().Be(!validateChainId);
txValidator.IsWellFormed(tx, releaseSpec).AsBool().Should().Be(!validateChainId);
}

[Timeout(Timeout.MaxTestTime)]
Expand Down Expand Up @@ -274,7 +274,7 @@ public void Transaction_with_init_code_above_max_value_is_rejected_when_eip3860E
.WithData(initCode).TestObject;

TxValidator txValidator = new(1);
txValidator.IsWellFormed(tx, releaseSpec).Should().Be(expectedResult);
txValidator.IsWellFormed(tx, releaseSpec).AsBool().Should().Be(expectedResult);
}

//leading zeros in AccessList - expected to pass (real mainnet tx)
Expand Down Expand Up @@ -345,8 +345,8 @@ public void ShardBlobTransactions_should_have_destination_set()
.WithChainId(TestBlockchainIds.ChainId)
.SignedAndResolved().TestObject;

Assert.That(txValidator.IsWellFormed(txWithoutTo, Cancun.Instance), Is.False);
Assert.That(txValidator.IsWellFormed(txWithTo, Cancun.Instance));
Assert.That(txValidator.IsWellFormed(txWithoutTo, Cancun.Instance).AsBool(), Is.False);
Assert.That(txValidator.IsWellFormed(txWithTo, Cancun.Instance).AsBool());
}

[Timeout(Timeout.MaxTestTime)]
Expand Down Expand Up @@ -404,9 +404,8 @@ public void IsWellFormed_NotBlobTxButMaxFeePerBlobGasIsSet_ReturnFalse()

Transaction tx = txBuilder.TestObject;
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

[Test]
Expand All @@ -419,9 +418,8 @@ public void IsWellFormed_NotBlobTxButBlobVersionedHashesIsSet_ReturnFalse()

Transaction tx = txBuilder.TestObject;
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

[Test]
Expand All @@ -437,9 +435,8 @@ public void IsWellFormed_BlobTxToIsNull_ReturnFalse()

Transaction tx = txBuilder.TestObject;
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}
[Test]
public void IsWellFormed_BlobTxHasMoreDataGasThanAllowed_ReturnFalse()
Expand All @@ -454,9 +451,8 @@ public void IsWellFormed_BlobTxHasMoreDataGasThanAllowed_ReturnFalse()

Transaction tx = txBuilder.TestObject;
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

[Test]
Expand All @@ -472,9 +468,8 @@ public void IsWellFormed_BlobTxHasNoBlobs_ReturnFalse()

Transaction tx = txBuilder.TestObject;
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

[Test]
Expand All @@ -490,9 +485,8 @@ public void IsWellFormed_BlobTxHasBlobOverTheSizeLimit_ReturnFalse()
Transaction tx = txBuilder.TestObject;
((ShardBlobNetworkWrapper)tx.NetworkWrapper!).Blobs[0] = new byte[Ckzg.Ckzg.BytesPerBlob + 1];
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

[Test]
Expand All @@ -508,9 +502,8 @@ public void IsWellFormed_BlobTxHasCommitmentOverTheSizeLimit_ReturnFalse()
Transaction tx = txBuilder.TestObject;
((ShardBlobNetworkWrapper)tx.NetworkWrapper!).Commitments[0] = new byte[Ckzg.Ckzg.BytesPerCommitment + 1];
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

[Test]
Expand All @@ -526,9 +519,8 @@ public void IsWellFormed_BlobTxHasProofOverTheSizeLimit_ReturnFalse()
Transaction tx = txBuilder.TestObject;
((ShardBlobNetworkWrapper)tx.NetworkWrapper!).Proofs[0] = new byte[Ckzg.Ckzg.BytesPerProof + 1];
TxValidator txValidator = new(TestBlockchainIds.ChainId);
string? error;

Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance, out error), Is.False);
Assert.That(txValidator.IsWellFormed(tx, Cancun.Instance).AsBool(), Is.False);
}

private static byte[] MakeArray(int count, params byte[] elements) =>
Expand Down
12 changes: 4 additions & 8 deletions src/Nethermind/Nethermind.Consensus/Validators/AlwaysValid.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@ namespace Nethermind.Consensus.Validators;
public class Always : IBlockValidator, ISealValidator, IUnclesValidator, ITxValidator
{
private readonly bool _result;
private readonly ValidationResult _validationResult;

private Always(bool result)
{
_validationResult = result ? ValidationResult.Success : "Always invalid.";
_result = result;
}

Expand Down Expand Up @@ -79,16 +81,10 @@ public bool Validate(BlockHeader header, BlockHeader[] uncles)
return _result;
}

public bool IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec)
public ValidationResult IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec)
{
return _result;
return _validationResult;
}
public bool IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec, out string? errorMessage)
{
errorMessage = null;
return _result;
}

public bool ValidateWithdrawals(Block block, out string? error)
{
error = null;
Expand Down
6 changes: 4 additions & 2 deletions src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,11 @@ private bool ValidateTransactions(Block block, IReleaseSpec spec, out string? er
{
Transaction transaction = transactions[txIndex];

if (!_txValidator.IsWellFormed(transaction, spec, out errorMessage))
ValidationResult isWellFormed = _txValidator.IsWellFormed(transaction, spec);
if (!isWellFormed)
{
if (_logger.IsDebug) _logger.Debug($"{Invalid(block)} Invalid transaction: {errorMessage}");
if (_logger.IsDebug) _logger.Debug($"{Invalid(block)} Invalid transaction: {isWellFormed}");
errorMessage = isWellFormed.Error;
return false;
}
}
Expand Down
Loading