Skip to content

OP Holocene - Proper EIP-1559 parameters handling #7926

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

Merged
merged 8 commits into from
Dec 19, 2024
Merged

OP Holocene - Proper EIP-1559 parameters handling #7926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6f682c5
Include Holocene transition timestamp
emlautarom1 Dec 16, 2024
72284e3
Replace tests with encoded/decoded cases
emlautarom1 Dec 16, 2024
477b96a
Unconditionally deal with EIP1559Parameters
emlautarom1 Dec 16, 2024
11ca652
When zero use default parameters
emlautarom1 Dec 16, 2024
3a12fd7
Recalculate hash when modifying `ExtraData`
emlautarom1 Dec 16, 2024
4051b6e
Inline local array
emlautarom1 Dec 17, 2024
8cc38b6
Don't branch on `IsZero`
emlautarom1 Dec 17, 2024
fbf855b
Streamline validation
emlautarom1 Dec 17, 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: 2 additions & 1 deletion src/Nethermind/Nethermind.Optimism.Test/OptimismHeaderValidatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ public class OptimismHeaderValidatorTests
private static IEnumerable<(string, bool)> EIP1559ParametersExtraData()
{
// Valid
yield return ("0x000000000000000000", true);
yield return ("0x000000000100000000", true);
yield return ("0x0000000001000001bc", true);
yield return ("0x0000000001ffffffff", true);
Expand All @@ -35,7 +34,9 @@ public class OptimismHeaderValidatorTests
yield return ("0xffffaaaa", false);
yield return ("0x01ffffffff00000000", false);
yield return ("0xff0000000100000001", false);
yield return ("0x000000000000000000", false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In ExtraData, a 0 EIP-1559 parameters should not be valid, instead defaults should be used (which are non-zero)

yield return ("0x000000000000000001", false);
yield return ("0x00ffffffff000001bc00", false);
}

[TestCaseSource(nameof(EIP1559ParametersExtraData))]
Expand Down
63 changes: 34 additions & 29 deletions src/Nethermind/Nethermind.Optimism.Test/OptimismPayloadPreparationServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,54 @@
// SPDX-FileCopyrightText: 2024 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using Nethermind.Core.Test.Builders;
using NUnit.Framework;
using FluentAssertions;
using System.Threading.Tasks;
using System.Collections.Generic;
using System;
using NUnit.Framework;
using NSubstitute;
using Nethermind.Core.Specs;
using Nethermind.State;
using Nethermind.Optimism.Rpc;
using Nethermind.Merge.Plugin.BlockProduction;
using Nethermind.Core.Timers;
using Nethermind.Logging;
using Nethermind.Optimism.Rpc;
using Nethermind.Int256;
using Nethermind.Evm.Tracing;
using Nethermind.Core.Timers;
using Nethermind.Core.Test.Builders;
using Nethermind.Core.Specs;
using Nethermind.Core.Crypto;
using Nethermind.Core;
using Nethermind.Consensus.Transactions;
using Nethermind.Consensus.Processing;
using Nethermind.Blockchain;
using Nethermind.State;
using Nethermind.Consensus;
using Nethermind.Core;
using Nethermind.Config;
using Nethermind.Core.Crypto;
using Nethermind.Evm.Tracing;
using System.Buffers.Binary;
using System.Threading.Tasks;
using Nethermind.Blockchain;
using FluentAssertions;
using Nethermind.Crypto;

namespace Nethermind.Optimism.Test;

[Parallelizable(ParallelScope.All)]
public class OptimismPayloadPreparationServiceTests
{
[TestCase(8u, 2u)]
[TestCase(2u, 2u)]
[TestCase(2u, 10u)]
public async Task Writes_EIP1559Params_Into_HeaderExtraData(UInt32 denominator, UInt32 elasticity)
private static IEnumerable<(OptimismPayloadAttributes, EIP1559Parameters?)> TestCases()
{
foreach (var noTxPool in (bool[])[true, false])
{
yield return (new OptimismPayloadAttributes { EIP1559Params = [0, 0, 0, 8, 0, 0, 0, 2], NoTxPool = noTxPool }, new EIP1559Parameters(0, 8, 2));
yield return (new OptimismPayloadAttributes { EIP1559Params = [0, 0, 0, 2, 0, 0, 0, 2], NoTxPool = noTxPool }, new EIP1559Parameters(0, 2, 2));
yield return (new OptimismPayloadAttributes { EIP1559Params = [0, 0, 0, 2, 0, 0, 0, 10], NoTxPool = noTxPool }, new EIP1559Parameters(0, 2, 10));
yield return (new OptimismPayloadAttributes { EIP1559Params = [0, 0, 0, 0, 0, 0, 0, 0], NoTxPool = noTxPool }, new EIP1559Parameters(0, 250, 6));
}
Comment on lines +35 to +41
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added tests that set the NoTxPool = true, which showed that the PayloadPreparationService was not adjusting the BlockHeader.ExtraData with the EIP-1559 parameters even though this should always be done.

}
[TestCaseSource(nameof(TestCases))]
public async Task Writes_EIP1559Params_Into_HeaderExtraData((OptimismPayloadAttributes Attributes, EIP1559Parameters? ExpectedEIP1559Parameters) testCase)
{
var parent = Build.A.BlockHeader.TestObject;

var releaseSpec = Substitute.For<IReleaseSpec>();
releaseSpec.IsOpHoloceneEnabled.Returns(true);
releaseSpec.BaseFeeMaxChangeDenominator.Returns((UInt256)250);
releaseSpec.ElasticityMultiplier.Returns(6);
var specProvider = Substitute.For<ISpecProvider>();
specProvider.GetSpec(parent).Returns(releaseSpec);

Expand Down Expand Up @@ -69,23 +81,16 @@ public async Task Writes_EIP1559Params_Into_HeaderExtraData(UInt32 denominator,
logManager: TestLogManager.Instance
);

var eip1559Params = new byte[8];
BinaryPrimitives.WriteUInt32BigEndian(eip1559Params.AsSpan(0, 4), denominator);
BinaryPrimitives.WriteUInt32BigEndian(eip1559Params.AsSpan(4, 4), elasticity);

var attributes = new OptimismPayloadAttributes()
{
PrevRandao = Hash256.Zero,
SuggestedFeeRecipient = TestItem.AddressA,
EIP1559Params = eip1559Params,
};
testCase.Attributes.PrevRandao = Hash256.Zero;
testCase.Attributes.SuggestedFeeRecipient = TestItem.AddressA;

var payloadId = service.StartPreparingPayload(parent, attributes);
var payloadId = service.StartPreparingPayload(parent, testCase.Attributes);
var context = await service.GetPayload(payloadId);
var currentBestBlock = context?.CurrentBestBlock!;

currentBestBlock.Should().Be(block);
currentBestBlock.Header.TryDecodeEIP1559Parameters(out var parameters, out _).Should().BeTrue();
parameters.Should().BeEquivalentTo(new EIP1559Parameters(0, denominator, elasticity));
parameters.Should().BeEquivalentTo(testCase.ExpectedEIP1559Parameters);
currentBestBlock.Header.Hash.Should().BeEquivalentTo(currentBestBlock.Header.CalculateHash());
}
}
20 changes: 7 additions & 13 deletions src/Nethermind/Nethermind.Optimism/OptimismBaseFeeCalculator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,22 +23,16 @@ public UInt256 Calculate(BlockHeader parent, IEip1559Spec specFor1559)
if (parent.Timestamp >= holoceneTimestamp)
{
// NOTE: This operation should never fail since headers should be valid at this point.
if (!parent.TryDecodeEIP1559Parameters(out EIP1559Parameters eip1559Params, out _))
if (!parent.TryDecodeEIP1559Parameters(out EIP1559Parameters eip1559Params, out var error))
{
throw new InvalidOperationException($"{nameof(BlockHeader)} was not properly validated: missing {nameof(EIP1559Parameters)}");
throw new InvalidOperationException($"{nameof(BlockHeader)} was not properly validated: {error}");
}

spec = eip1559Params.IsZero()
? new OverridableEip1559Spec(specFor1559)
{
ElasticityMultiplier = Eip1559Constants.DefaultElasticityMultiplier,
BaseFeeMaxChangeDenominator = Eip1559Constants.DefaultBaseFeeMaxChangeDenominator
}
: new OverridableEip1559Spec(specFor1559)
{
ElasticityMultiplier = eip1559Params.Elasticity,
BaseFeeMaxChangeDenominator = eip1559Params.Denominator
};
spec = new OverridableEip1559Spec(specFor1559)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need to branch here since 0 EIP-1559 parameters should never get to ExtraData.

{
ElasticityMultiplier = eip1559Params.Elasticity,
BaseFeeMaxChangeDenominator = eip1559Params.Denominator
};
}

return baseFeeCalculator.Calculate(parent, spec);
Expand Down
8 changes: 7 additions & 1 deletion src/Nethermind/Nethermind.Optimism/OptimismHeaderValidator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,17 @@ public override bool Validate(BlockHeader header, BlockHeader? parent, bool isUn
IReleaseSpec spec = _specProvider.GetSpec(header);
if (spec.IsOpHoloceneEnabled)
{
if (!header.TryDecodeEIP1559Parameters(out _, out var decodeError))
if (!header.TryDecodeEIP1559Parameters(out var parameters, out var decodeError))
{
error = decodeError;
return false;
}

if (parameters.IsZero())
{
error = $"{nameof(EIP1559Parameters)} is zero";
return false;
}
}

return base.Validate(header, parent, isUncle, out error);
Expand Down
40 changes: 25 additions & 15 deletions src/Nethermind/Nethermind.Optimism/OptimismPayloadPreparationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Nethermind.Core;
using Nethermind.Core.Specs;
using Nethermind.Core.Timers;
using Nethermind.Crypto;
using Nethermind.Int256;
using Nethermind.Logging;
using Nethermind.Merge.Plugin.BlockProduction;
Expand Down Expand Up @@ -45,6 +46,30 @@ public OptimismPayloadPreparationService(
protected override void ImproveBlock(string payloadId, BlockHeader parentHeader,
PayloadAttributes payloadAttributes, Block currentBestBlock, DateTimeOffset startDateTime)
{
if (payloadAttributes is OptimismPayloadAttributes optimismPayload)
{
var spec = _specProvider.GetSpec(currentBestBlock.Header);
if (spec.IsOpHoloceneEnabled)
{
// NOTE: This operation should never fail since headers should be valid at this point.
if (!optimismPayload.TryDecodeEIP1559Parameters(out EIP1559Parameters eip1559Parameters, out var error))
{
throw new InvalidOperationException($"{nameof(BlockHeader)} was not properly validated: {error}");
}

if (eip1559Parameters.IsZero())
{
eip1559Parameters = new EIP1559Parameters(eip1559Parameters.Version, (UInt32)spec.BaseFeeMaxChangeDenominator, (UInt32)spec.ElasticityMultiplier);
}

currentBestBlock.Header.ExtraData = new byte[EIP1559Parameters.ByteLength];
eip1559Parameters.WriteTo(currentBestBlock.Header.ExtraData);

// NOTE: Since we updated the `Header` we need to recalculate the hash.
currentBestBlock.Header.Hash = currentBestBlock.Header.CalculateHash();
Comment on lines +68 to +69
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the comment highlights, since we're modifying the BlockHeader.ExtraData we need to recompute the Hash. This is again error prone and tedious since we can easily forget to update the hash.

It's not clear to me if this is the best place to perform this recalculation or maybe there is a different stage in the processing pipeline where it should be done.

}
}

if (payloadAttributes is OptimismPayloadAttributes { NoTxPool: true })
{
if (_logger.IsDebug)
Expand All @@ -56,21 +81,6 @@ protected override void ImproveBlock(string payloadId, BlockHeader parentHeader,
}
else
{
if (payloadAttributes is OptimismPayloadAttributes optimismPayload)
{
var spec = _specProvider.GetSpec(currentBestBlock.Header);
if (spec.IsOpHoloceneEnabled)
{
if (!optimismPayload.TryDecodeEIP1559Parameters(out EIP1559Parameters eip1559Parameters, out var error))
{
throw new InvalidOperationException($"{nameof(OptimismPayloadAttributes)} was not properly validated: invalid {nameof(OptimismPayloadAttributes.EIP1559Params)}");
}

currentBestBlock.Header.ExtraData = new byte[EIP1559Parameters.ByteLength];
eip1559Parameters.WriteTo(currentBestBlock.Header.ExtraData);
}
}

base.ImproveBlock(payloadId, parentHeader, payloadAttributes, currentBestBlock, startDateTime);
}
}
Expand Down
5 changes: 2 additions & 3 deletions src/Nethermind/Nethermind.Optimism/Rpc/OptimismPayloadAttributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public byte[][]? Transactions
/// See <see href="https://specs.optimism.io/protocol/holocene/exec-engine.html#eip-1559-parameters-in-payloadattributesv3"/>
/// </remarks>
public byte[]? EIP1559Params { get; set; }
private const int EIP1559ParamsLength = 8;

private int TransactionsLength => Transactions?.Length ?? 0;

Expand Down Expand Up @@ -121,9 +120,9 @@ public override PayloadAttributesValidationResult Validate(ISpecProvider specPro
error = $"{nameof(EIP1559Params)} should be null before Holocene";
return PayloadAttributesValidationResult.InvalidPayloadAttributes;
}
if (releaseSpec.IsOpHoloceneEnabled && EIP1559Params?.Length != EIP1559ParamsLength)
if (releaseSpec.IsOpHoloceneEnabled && !this.TryDecodeEIP1559Parameters(out _, out var decodeError))
{
error = $"{nameof(EIP1559Params)} should be {EIP1559ParamsLength} bytes long";
error = decodeError;
return PayloadAttributesValidationResult.InvalidPayloadAttributes;
}

Expand Down
1 change: 1 addition & 0 deletions src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecLoader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ bool GetForInnerPathExistence(KeyValuePair<string, JsonElement> o) =>
Eip6780TransitionTimestamp = chainSpecJson.Params.Eip6780TransitionTimestamp,
Rip7212TransitionTimestamp = chainSpecJson.Params.Rip7212TransitionTimestamp,
OpGraniteTransitionTimestamp = chainSpecJson.Params.OpGraniteTransitionTimestamp,
OpHoloceneTransitionTimestamp = chainSpecJson.Params.OpHoloceneTransitionTimestamp,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first issue was that we missed one of these copies which resulted in a IReleaseSpec where Holocene was never enabled. I think some parts of the system were working correctly since we were getting this timestamp from a different source.

Either way, this kind of work is repetitive and error prone and it would be great if we could come up with a better solution that does not require such manual work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove OpHoloceneTransitionTimestamp from ChainSpecLoader. We have OptimsimChainSpecEngineParameters for that. Also we should remove OpGraniteTransitionTimestamp, but not in this pr proabably

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, that sounds good, but I'm having a bit of issues getting an instance of OptimismChainSpecEngineParameters in certain places like in OptimismPayloadAttributes.Validate:

nethermind/src/Nethermind/Nethermind.Optimism/Rpc/OptimismPayloadAttributes.cs

Lines 117 to 127 in fbf855b

IReleaseSpec releaseSpec = specProvider.GetSpec(ForkActivation.TimestampOnly(Timestamp));
if (!releaseSpec.IsOpHoloceneEnabled && EIP1559Params is not null)
{
error = $"{nameof(EIP1559Params)} should be null before Holocene";
return PayloadAttributesValidationResult.InvalidPayloadAttributes;
}
if (releaseSpec.IsOpHoloceneEnabled && !this.TryDecodeEIP1559Parameters(out _, out var decodeError))
{
error = decodeError;
return PayloadAttributesValidationResult.InvalidPayloadAttributes;
}

Eip4788TransitionTimestamp = chainSpecJson.Params.Eip4788TransitionTimestamp,
Eip7702TransitionTimestamp = chainSpecJson.Params.Eip7702TransitionTimestamp,
Eip4788ContractAddress = chainSpecJson.Params.Eip4788ContractAddress ?? Eip4788Constants.BeaconRootsAddress,
Expand Down
Loading