Fix op-mainnet snapshot #7881
Fix op-mainnet snapshot #7881
Conversation
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| namespace Nethermind.Init.Steps | |||
| { | |||
| [RunnerStepDependencies(typeof(InitTxTypesAndRlp), typeof(InitDatabase), typeof(MigrateConfigs), typeof(SetupKeyStore))] | |||
| [RunnerStepDependencies(typeof(InitTxTypesAndRlp), typeof(InitDatabase), typeof(MigrateConfigs), typeof(SetupKeyStore), typeof(InitTxTypesAndRlp))] | |||
There was a problem hiding this comment.
InitTxTypesAndRlp is twice there
| @@ -8,6 +8,6 @@ namespace Nethermind.TxPool | |||
| { | |||
| public interface ITxValidator | |||
| { | |||
| public ValidationResult IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec); | |||
| public ValidationResult IsWellFormed(Transaction transaction, Block? block, IReleaseSpec releaseSpec); | |||
There was a problem hiding this comment.
Maybe? Would reduce those unnecessary nulls all over the place
| public ValidationResult IsWellFormed(Transaction transaction, Block? block, IReleaseSpec releaseSpec); | |
| public ValidationResult IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec, Block? block = null); |
| { | ||
| public ValidationResult IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec, BlockHeader? header = null) | ||
| { | ||
| if (header is null || specHelper.IsBedrock(header)) |
There was a problem hiding this comment.
Could IsBedrock be in IReleaseSpec, then we don't have to pass header?
IReleaseSpec was created to have all fork information.
There was a problem hiding this comment.
we can check if eip1559 is enabled. Not sure it's good idea. But as I know ReleaseSpec is not aware of optimism forks
There was a problem hiding this comment.
Would be good to make IReleaseSpec extendable by plugins then
| // In op bedrock eip1559 activated with bedrock | ||
| if (releaseSpec.IsEip1559Enabled) | ||
| { | ||
| return transaction.Signature is null ? new ValidationResult("Empty signature") : ValidationResult.Success; |
There was a problem hiding this comment.
Before we were not performing any validations on Deposit transactions, but now we require that post-Bedrock transactions contain a non-null signature. Is this correct? If so, shouldn't we also validate the signature (using SignatureTxValidator)?
There was a problem hiding this comment.
if signature is null here it means that while decoding signature in DecodeSignature we got inside if (v == 0 && rBytes.IsEmpty && sBytes.IsEmpty). Without this pr we would crash inside DecodeSignature.
There was a problem hiding this comment.
That makes sense, what I'm missing is why we don't validate the signature of a Legacy transaction post bedrock. More so, why are we not performing the standard Legacy validations:
With these changes, Legacy transactions that are processed in Optimism are not validated at all, when previously they went through several checks.
There was a problem hiding this comment.
Oh, that means I made a mistake. But why we don't check DepositTransaction?
api.RegisterTxType<OptimismTransactionForRpc>(new OptimismTxDecoder<Transaction>(), Always.Valid);
There was a problem hiding this comment.
It dates back to the original Optimism plugin PR where the OptimismTxValidator performed no checks on Deposit transactions
nethermind/src/Nethermind/Nethermind.Optimism/OptimismTxValidator.cs
Lines 20 to 26 in 6edd397
We didn't change the logic in the Transaction refactoring, so the current validator for Deposit transactions always succeeds.
Fixes Closes Resolves #
Pre-bedrock legacy transactions might not have signature
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Documentation
Requires documentation update
Requires explanation in Release Notes