-
Notifications
You must be signed in to change notification settings - Fork 434
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.
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
EIP 7742 implementation #7518
base: master
Are you sure you want to change the base?
EIP 7742 implementation #7518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very quick check: no HeaderDecoder changes?
# Conflicts: # src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Red flag, don't see the change in PrepareBlockForProcessing in BlockchainProcessor. Perhaps, check how other fields from header are used
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm is this handled correcty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing here, need to skip the logic related to MaxBlobGasPerBlock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but if EIP enabled than we do what? I think it is not handled correctly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?
ulong totalDataGas = BlobGasCalculator.CalculateBlobGas(blobCount); | ||
return totalDataGas > Eip4844Constants.MaxBlobGasPerTransaction ? TxErrorMessages.BlobTxGasLimitExceeded | ||
: blobCount < Eip4844Constants.MinBlobsPerTransaction ? TxErrorMessages.BlobTxMissingBlobs | ||
if (!releaseSpec.IsEip7742Enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing here, need to skip the logic related to MaxBlobGasPerBlock
which is equal to MaxBlobGasPerTransaction
/// </summary> | ||
[JsonRequired] | ||
public sealed override WithdrawalRequest[]? WithdrawalRequests { get; set; } | ||
// [JsonRequired] TODO: should be on ExecutionPayloadV4? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ask other core devs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
More discussions going around it: |
ParentBeaconBlockRoot = bh.ParentBeaconBlockRoot | ||
ParentBeaconBlockRoot = bh.ParentBeaconBlockRoot, | ||
TargetBlobCount = bh.TargetBlobCount, | ||
MaxBlobCount = bh.MaxBlobCount, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think MaxBlobCount should be here - EIP just says TargetBlobCount
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per the EIP there is no MaxBlobCount in ELHeader
@@ -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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per the EIP there is no MaxBlobCount in ELHeader
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think its assumed that the CL will do the verification regarding the MaxBlockGas/MaxBlobCount because that value is provided for blockProduction?
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?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Optional. Remove if not applicable.
Documentation
Requires documentation update
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
If yes, fill in the details here. Remove if not applicable.
Remarks
Optional. Remove if not applicable.