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

Fix op-mainnet snapshot #7881

Merged
merged 9 commits into from
Dec 10, 2024
Merged

Fix op-mainnet snapshot #7881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ad54d84
No signature in legacy transactions
deffrian Dec 10, 2024
2ebd925
set fallback to null
deffrian Dec 10, 2024
0483d2f
Fix build
deffrian Dec 10, 2024
b0e5339
Fix tools
deffrian Dec 10, 2024
0b8adf4
Cleanup
deffrian Dec 10, 2024
f9c088c
Fix suggestions
deffrian Dec 10, 2024
63c6a16
Remove block header
deffrian Dec 10, 2024
b5656e6
Fix
deffrian Dec 10, 2024
78349f5
Merge branch 'master' into fix/optimism-snapshot
deffrian Dec 10, 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
3 changes: 3 additions & 0 deletions src/Nethermind/Nethermind.Optimism/InitializeBlockchainOptimism.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
using Nethermind.Core;
using Nethermind.Evm;
using Nethermind.Evm.TransactionProcessing;
using Nethermind.Facade.Eth.RpcTransaction;
using Nethermind.Init.Steps;
using Nethermind.Merge.Plugin.InvalidChainTracker;
using Nethermind.Optimism.Rpc;
using Nethermind.Serialization.Rlp.TxDecoders;
using Nethermind.TxPool;

namespace Nethermind.Optimism;
Expand All @@ -36,6 +38,7 @@ protected override async Task InitBlockchain()
await base.InitBlockchain();

api.RegisterTxType<OptimismTransactionForRpc>(new OptimismTxDecoder<Transaction>(), Always.Valid);
api.RegisterTxType<LegacyTransactionForRpc>(new OptimismLegacyTxDecoder(), new OptimismLegacyTxValidator());
}

protected override ITransactionProcessor CreateTransactionProcessor(CodeInfoRepository codeInfoRepository, VirtualMachine virtualMachine)
Expand Down
39 changes: 39 additions & 0 deletions src/Nethermind/Nethermind.Optimism/OptimismLegacyTxDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// SPDX-FileCopyrightText: 2024 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using System;
using Nethermind.Core;
using Nethermind.Core.Crypto;
using Nethermind.Core.Specs;
using Nethermind.Serialization.Rlp;
using Nethermind.Serialization.Rlp.TxDecoders;
using Nethermind.TxPool;

namespace Nethermind.Optimism;

public class OptimismLegacyTxDecoder : LegacyTxDecoder<Transaction>
{
protected override Signature? DecodeSignature(ulong v, ReadOnlySpan<byte> rBytes, ReadOnlySpan<byte> sBytes, Signature? fallbackSignature = null,
RlpBehaviors rlpBehaviors = RlpBehaviors.None)
{
if (v == 0 && rBytes.IsEmpty && sBytes.IsEmpty)
{
return null;
}
return base.DecodeSignature(v, rBytes, sBytes, fallbackSignature, rlpBehaviors);
}
}

public class OptimismLegacyTxValidator : ITxValidator
{
public ValidationResult IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec)
{
// In op bedrock eip1559 activated with bedrock
if (releaseSpec.IsEip1559Enabled)
{
return transaction.Signature is null ? new ValidationResult("Empty signature") : ValidationResult.Success;
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

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

Lines 24 to 30 in d00410f

RegisterValidator(TxType.Legacy, new CompositeTxValidator([
IntrinsicGasTxValidator.Instance,
new LegacySignatureTxValidator(chainId),
ContractSizeTxValidator.Instance,
NonBlobFieldsTxValidator.Instance,
NonSetCodeFieldsTxValidator.Instance
]));

With these changes, Legacy transactions that are processed in Optimism are not validated at all, when previously they went through several checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that means I made a mistake. But why we don't check DepositTransaction?
api.RegisterTxType<OptimismTransactionForRpc>(new OptimismTxDecoder<Transaction>(), Always.Valid);

Copy link
Contributor

Choose a reason for hiding this comment

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

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

public bool IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec) =>
IsWellFormed(transaction, releaseSpec, out _);
public bool IsWellFormed(Transaction transaction, IReleaseSpec releaseSpec, [NotNullWhen(false)] out string? error)
{
error = null;
return transaction.Type == TxType.DepositTx || _txValidator.IsWellFormed(transaction, releaseSpec, out error);
}

We didn't change the logic in the Transaction refactoring, so the current validator for Deposit transactions always succeeds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed in #7887.

}

return ValidationResult.Success;
}
}
3 changes: 3 additions & 0 deletions src/Nethermind/Nethermind.Optimism/OptimismPlugin.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@
using Nethermind.Consensus.Rewards;
using Nethermind.Consensus.Validators;
using Nethermind.Core;
using Nethermind.Facade.Eth.RpcTransaction;
using Nethermind.Merge.Plugin.Synchronization;
using Nethermind.HealthChecks;
using Nethermind.Serialization.Json;
using Nethermind.Specs.ChainSpecStyle;
using Nethermind.Serialization.Rlp;
using Nethermind.Optimism.Rpc;
using Nethermind.Serialization.Rlp.TxDecoders;
using Nethermind.Synchronization;

namespace Nethermind.Optimism;
Expand Down Expand Up @@ -84,6 +86,7 @@ public void InitTxTypesAndRlpDecoders(INethermindApi api)
if (ShouldRunSteps(api))
{
api.RegisterTxType<OptimismTransactionForRpc>(new OptimismTxDecoder<Transaction>(), Always.Valid);
api.RegisterTxType<LegacyTransactionForRpc>(new OptimismLegacyTxDecoder(), new OptimismLegacyTxValidator());
Rlp.RegisterDecoders(typeof(OptimismReceiptMessageDecoder).Assembly, true);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Nethermind/Nethermind.Serialization.Rlp/TxDecoders/LegacyTxDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

namespace Nethermind.Serialization.Rlp.TxDecoders;

public sealed class LegacyTxDecoder<T>(Func<T>? transactionFactory = null) : BaseTxDecoder<T>(TxType.Legacy, transactionFactory) where T : Transaction, new()
public class LegacyTxDecoder<T>(Func<T>? transactionFactory = null) : BaseTxDecoder<T>(TxType.Legacy, transactionFactory) where T : Transaction, new()
{
private static bool IncludeSigChainIdHack(bool isEip155Enabled, ulong chainId) => isEip155Enabled && chainId != 0;

Expand Down
Loading