OP Holocene - Proper EIP-1559 parameters handling #7926

emlautarom1 · 2024-12-17T13:46:32Z

Fixes #7903

Changes

Properly set up the Holocene timestamp.
Always store EIP-1559 parameters in ExtraData, even when dealing with defaults.
Ensure Block Header hash is recalculated when needed.
Use defaults from Chainspec rather than from constants.

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

Unit tests were extended to include cases where NoTxPool = true which were missing from the original PR.

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

Remarks

This PR fixes several issues that were found after running some integration tests leveraging Kurtosis and the optimism-package. More details will be provided as discussion comments.

- Makes code more readable

emlautarom1 · 2024-12-17T13:48:41Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecLoader.cs

@@ -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,


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.

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

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;

}

emlautarom1 · 2024-12-17T13:50:03Z

src/Nethermind/Nethermind.Optimism.Test/OptimismPayloadPreparationServiceTests.cs

+        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));
+        }


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.

emlautarom1 · 2024-12-17T13:55:57Z

src/Nethermind/Nethermind.Optimism/OptimismPayloadPreparationService.cs

+                // NOTE: Since we updated the `Header` we need to recalculate the hash.
+                currentBestBlock.Header.Hash = currentBestBlock.Header.CalculateHash();


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.

- There is no need since during validation we ensure the value is non-zero - Zero should never be written to `ExtraData`

- Add test for trailing bytes

emlautarom1 · 2024-12-17T15:07:49Z

src/Nethermind/Nethermind.Optimism.Test/OptimismHeaderValidatorTests.cs

@@ -35,7 +34,9 @@ public class OptimismHeaderValidatorTests
        yield return ("0xffffaaaa", false);
        yield return ("0x01ffffffff00000000", false);
        yield return ("0xff0000000100000001", false);
+        yield return ("0x000000000000000000", false);


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

emlautarom1 · 2024-12-17T15:09:06Z

src/Nethermind/Nethermind.Optimism/OptimismBaseFeeCalculator.cs

-                    ElasticityMultiplier = eip1559Params.Elasticity,
-                    BaseFeeMaxChangeDenominator = eip1559Params.Denominator
-                };
+            spec = new OverridableEip1559Spec(specFor1559)


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

deffrian · 2024-12-18T04:28:57Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecLoader.cs

@@ -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,


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

flcl42 · 2024-12-18T14:03:30Z

@deffrian won't it break fork info?

deffrian · 2024-12-19T02:41:29Z

Using OptimsimChainSpecEngineParameters is blocked by #7886. Let's merge it as is

emlautarom1 added 6 commits December 16, 2024 10:43

Include Holocene transition timestamp

6f682c5

Replace tests with encoded/decoded cases

72284e3

Unconditionally deal with EIP1559Parameters

477b96a

When zero use default parameters

11ca652

Recalculate hash when modifying ExtraData

3a12fd7

Inline local array

4051b6e

- Makes code more readable

emlautarom1 requested review from flcl42, LukaszRozmej, deffrian and jmederosalvarado December 17, 2024 13:46

emlautarom1 commented Dec 17, 2024

View reviewed changes

emlautarom1 added 2 commits December 17, 2024 11:15

Don't branch on IsZero

8cc38b6

- There is no need since during validation we ensure the value is non-zero - Zero should never be written to `ExtraData`

Streamline validation

fbf855b

- Add test for trailing bytes

emlautarom1 commented Dec 17, 2024

View reviewed changes

emlautarom1 mentioned this pull request Dec 17, 2024

OP Holocene (Kurtosis): Incorrect chain config, rejects holocene blocks. #7903

Closed

LukaszRozmej approved these changes Dec 17, 2024

View reviewed changes

deffrian requested changes Dec 18, 2024

View reviewed changes

deffrian approved these changes Dec 19, 2024

View reviewed changes

emlautarom1 merged commit 695420e into master Dec 19, 2024
79 checks passed

emlautarom1 deleted the fix/op-holocene-integration-tests branch December 19, 2024 13:10

brbrr pushed a commit that referenced this pull request Dec 23, 2024

OP Holocene - Proper EIP-1559 parameters handling (#7926)

924ab37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OP Holocene - Proper EIP-1559 parameters handling #7926

OP Holocene - Proper EIP-1559 parameters handling #7926

emlautarom1 commented Dec 17, 2024

emlautarom1 Dec 17, 2024

deffrian Dec 18, 2024

emlautarom1 Dec 18, 2024

emlautarom1 Dec 17, 2024

emlautarom1 Dec 17, 2024

emlautarom1 Dec 17, 2024

emlautarom1 Dec 17, 2024

deffrian Dec 18, 2024

flcl42 commented Dec 18, 2024 •

edited

Loading

deffrian commented Dec 19, 2024

	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;
	}

		// NOTE: Since we updated the `Header` we need to recalculate the hash.
		currentBestBlock.Header.Hash = currentBestBlock.Header.CalculateHash();

OP Holocene - Proper EIP-1559 parameters handling #7926

OP Holocene - Proper EIP-1559 parameters handling #7926

Conversation

emlautarom1 commented Dec 17, 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

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

flcl42 commented Dec 18, 2024 • edited Loading

deffrian commented Dec 19, 2024

flcl42 commented Dec 18, 2024 •

edited

Loading