EIP 7742 implementation #7518

yerke26 · 2024-09-30T14:33:31Z

Resolves #7526

Changes

removed MAX_BLOB_GAS_PER_BLOCK, TARGET_BLOB_GAS_PER_BLOCK
Get target_blob_count, max_blob_count from Engine Api
added target_blob_count, max_blob_count to ExecutionPayload
added target_blob_count, max_blob_count to BlockHeader
removed verifications related to MAX_BLOB_GAS_PER_BLOCK
used target_blob_count * Eip4844Constants.GasPerBlob instead of TARGET_BLOB_GAS_PER_BLOCK

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

Optional. Remove if not applicable.

Documentation

Requires documentation update

Yes
No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

Yes
No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

MarekM25

very quick check: no HeaderDecoder changes?

# Conflicts: # src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs

MarekM25

Red flag, don't see the change in PrepareBlockForProcessing in BlockchainProcessor. Perhaps, check how other fields from header are used

MarekM25 · 2024-10-03T12:15:06Z

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

@@ -137,7 +137,7 @@ private void SelectBlobTransactions(IEnumerable<Transaction> blobTransactions, B
                checkedBlobTransactions++;

                ulong txBlobGas = (ulong)(blobTx.BlobVersionedHashes?.Length ?? 0) * _eip4844Config.GasPerBlob;
-                if (txBlobGas > _eip4844Config.MaxBlobGasPerBlock - blobGasCounter)
+                if (!spec.IsEip7742Enabled && txBlobGas > _eip4844Config.MaxBlobGasPerBlock - blobGasCounter)


hmmm is this handled correcty?

same thing here, need to skip the logic related to MaxBlobGasPerBlock

i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?

MarekM25 · 2024-10-03T12:15:58Z

src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs

@@ -405,7 +405,7 @@ private bool ValidateEip4844Fields(Block block, IReleaseSpec spec, out string? e

        ulong blobGasUsed = BlobGasCalculator.CalculateBlobGas(blobsInBlock);

-        if (blobGasUsed > Eip4844Constants.MaxBlobGasPerBlock)
+        if (!spec.IsEip7742Enabled && blobGasUsed > Eip4844Constants.MaxBlobGasPerBlock)


but if EIP enabled than we do what? I think it is not handled correctly

Upon activating this EIP (i.e. before processing any transactions), the verification of the blob maximum as given in EIP-4844 can be skipped. Concretely, this means any logic relating to MAX_BLOB_GAS_PER_BLOCK as given in EIP-4844 can be deprecated

This is written in EIP specification, I think if IsEip7742Enabled = true we need to skip this logic

i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?

MarekM25 · 2024-10-03T12:16:19Z

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

-        ulong totalDataGas = BlobGasCalculator.CalculateBlobGas(blobCount);
-        return totalDataGas > Eip4844Constants.MaxBlobGasPerTransaction ? TxErrorMessages.BlobTxGasLimitExceeded
-            : blobCount < Eip4844Constants.MinBlobsPerTransaction ? TxErrorMessages.BlobTxMissingBlobs
+        if (!releaseSpec.IsEip7742Enabled


same issue?

same thing here, need to skip the logic related to MaxBlobGasPerBlock which is equal to MaxBlobGasPerTransaction

MarekM25 · 2024-10-03T12:20:18Z

src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs

    /// </summary>
-    [JsonRequired]
-    public sealed override WithdrawalRequest[]? WithdrawalRequests { get; set; }
+    // [JsonRequired] TODO: should be on ExecutionPayloadV4?


Should we ask other core devs?

Added to ExecutionPayloadV4

# Conflicts: # src/Nethermind/Nethermind.Core/Specs/IReleaseSpec.cs # src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs # src/Nethermind/Nethermind.Merge.Plugin/Handlers/EngineRpcCapabilitiesProvider.cs

…ests

tanishqjasoria · 2024-10-22T09:40:49Z

More discussions going around it:
ethereum/EIPs#8979
ethereum/execution-apis#574

tanishqjasoria · 2024-10-21T10:51:44Z

src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs

-            ParentBeaconBlockRoot = bh.ParentBeaconBlockRoot
+            ParentBeaconBlockRoot = bh.ParentBeaconBlockRoot,
+            TargetBlobCount = bh.TargetBlobCount,
+            MaxBlobCount = bh.MaxBlobCount,


i dont think MaxBlobCount should be here - EIP just says TargetBlobCount

tanishqjasoria · 2024-10-22T10:05:47Z

src/Nethermind/Nethermind.Core/Block.cs

@@ -122,6 +122,9 @@ public Transaction[] Transactions

    public Hash256? RequestsRoot => Header.RequestsRoot; // do not add setter here

+    public ulong? TargetBlobCount => Header.TargetBlobCount; // do not add setter here
+
+    public ulong? MaxBlobCount => Header.MaxBlobCount; // do not add setter here


as per the EIP there is no MaxBlobCount in ELHeader

tanishqjasoria · 2024-10-22T10:06:01Z

src/Nethermind/Nethermind.Core/BlockHeader.cs

@@ -84,6 +84,8 @@ public BlockHeader(

    public string SealEngineType { get; set; } = Core.SealEngineType.Ethash;
    public bool IsPostMerge { get; set; }
+    public ulong? TargetBlobCount { get; set; }
+    public ulong? MaxBlobCount { get; set; }


as per the EIP there is no MaxBlobCount in ELHeader

tanishqjasoria · 2024-10-22T10:09:26Z

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs

@@ -137,7 +137,7 @@ private void SelectBlobTransactions(IEnumerable<Transaction> blobTransactions, B
                checkedBlobTransactions++;

                ulong txBlobGas = (ulong)(blobTx.BlobVersionedHashes?.Length ?? 0) * _eip4844Config.GasPerBlob;
-                if (txBlobGas > _eip4844Config.MaxBlobGasPerBlock - blobGasCounter)
+                if (!spec.IsEip7742Enabled && txBlobGas > _eip4844Config.MaxBlobGasPerBlock - blobGasCounter)


i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?

tanishqjasoria · 2024-10-22T10:09:49Z

src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs

@@ -405,7 +405,7 @@ private bool ValidateEip4844Fields(Block block, IReleaseSpec spec, out string? e

        ulong blobGasUsed = BlobGasCalculator.CalculateBlobGas(blobsInBlock);

-        if (blobGasUsed > Eip4844Constants.MaxBlobGasPerBlock)
+        if (!spec.IsEip7742Enabled && blobGasUsed > Eip4844Constants.MaxBlobGasPerBlock)


i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?

Get target_blob_count, max_blob_count from Engine Api

de19765

yerke26 requested a review from rubo as a code owner September 30, 2024 14:33

yerke26 marked this pull request as draft September 30, 2024 14:33

MarekM25 requested changes Sep 30, 2024

View reviewed changes

yerke26 changed the title ~~Get target_blob_count, max_blob_count from Engine Api~~ EIP 7742 implementation Oct 1, 2024

yerke26 added 8 commits October 1, 2024 17:02

fix HeaderDecoder, restore Eip4844 changes, Add Eip7742

eca6690

Merge branch 'master' into feature/eip-7742

7ac90df

fix HeaderDecoder

9be8a53

move new params from ExecutionPayload to ExecutionPayloadV4

c592412

revert minor changes

f6ae01c

added new params to PayloadAttributes

f803097

fix tests

7043124

fix whitespaces

fef9cb2

yerke26 marked this pull request as ready for review October 2, 2024 10:15

yerke26 added 2 commits October 2, 2024 17:21

add tests

cc02ce5

Merge branch 'master' into feature/eip-7742

206d514

# Conflicts: # src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs

MarekM25 requested changes Oct 3, 2024

View reviewed changes

yerke26 added 13 commits October 4, 2024 15:41

add missing fields

9fbb9c1

fix PayloadAttributes

8ada4e2

add engine_forkchoiceUpdatedV4 to EngineRpcModule.Prague

3ae0fed

change to engine_forkchoiceUpdatedV4

372868a

Merge branch 'master' into feature/eip-7742

0bd75c6

add new fields to EngineModuleTests.V4

4b5fbfc

added new engine_forkchoiceUpdatedV4 to EngineRpcCapabilitiesProvider

5078b76

fix test cases

a92cdde

Merge branch 'master' into feature/eip-7742

c032480

# Conflicts: # src/Nethermind/Nethermind.Core/Specs/IReleaseSpec.cs # src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs # src/Nethermind/Nethermind.Merge.Plugin/Handlers/EngineRpcCapabilitiesProvider.cs

fix whitespace

4ab5fda

fix EngineRpcCapabilitiesProvider, revert ChainSpecBasedSpecProviderT…

0ba2006

…ests

remove unnecessary comment

911d232

remove comments

f806f22

yerke26 requested a review from MarekM25 October 8, 2024 10:26

Merge branch 'master' into feature/eip-7742

8dc3d6a

tanishqjasoria self-requested a review October 22, 2024 09:48

tanishqjasoria reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP 7742 implementation #7518

EIP 7742 implementation #7518

yerke26 commented Sep 30, 2024 •

edited

Loading

MarekM25 left a comment

MarekM25 left a comment

MarekM25 Oct 3, 2024

yerke26 Oct 4, 2024

tanishqjasoria Oct 22, 2024

MarekM25 Oct 3, 2024

yerke26 Oct 4, 2024 •

edited

Loading

tanishqjasoria Oct 22, 2024

MarekM25 Oct 3, 2024

yerke26 Oct 4, 2024

MarekM25 Oct 3, 2024

yerke26 Oct 14, 2024

tanishqjasoria commented Oct 22, 2024

tanishqjasoria Oct 21, 2024

tanishqjasoria Oct 22, 2024

tanishqjasoria Oct 22, 2024

tanishqjasoria Oct 22, 2024

tanishqjasoria Oct 22, 2024

EIP 7742 implementation #7518

Are you sure you want to change the base?

EIP 7742 implementation #7518

Conversation

yerke26 commented Sep 30, 2024 • edited Loading

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

MarekM25 left a comment

Choose a reason for hiding this comment

MarekM25 left a 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

yerke26 Oct 4, 2024 • edited Loading

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

tanishqjasoria commented Oct 22, 2024

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

yerke26 commented Sep 30, 2024 •

edited

Loading

yerke26 Oct 4, 2024 •

edited

Loading