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

Jump to bottom

EIP 7742 implementation #7518

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

EIP 7742 implementation #7518

wants to merge 36 commits into from

Conversation

yerke26
Copy link
Contributor

@yerke26 yerke26 commented Sep 30, 2024 •
edited
Loading

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.

@yerke26 yerke26 requested a review from rubo as a code owner September 30, 2024 14:33
@yerke26 yerke26 marked this pull request as draft September 30, 2024 14:33
MarekM25
MarekM25 requested changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

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

@yerke26 yerke26 changed the title Get target_blob_count, max_blob_count from Engine Api EIP 7742 implementation Oct 1, 2024
@yerke26 yerke26 marked this pull request as ready for review October 2, 2024 10:15
@yerke26
add tests
cc02ce5
@yerke26
Merge branch 'master' into feature/eip-7742
206d514 
# Conflicts:
#	src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs
MarekM25
MarekM25 requested changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

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

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

update: here we can't skip this logic, it might need to be modified
now we recieve MaxBlobCount in payload attributes and we need to use that to construct the block.

Copy link
Contributor

Choose a reason for hiding this comment

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

so the idea with the EIP is to remove validation logic, but here it is for block construction and not validation

src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs Outdated Show resolved Hide resolved
@yerke26
add missing fields
9fbb9c1
@yerke26
fix PayloadAttributes
8ada4e2
@yerke26
add engine_forkchoiceUpdatedV4 to EngineRpcModule.Prague
3ae0fed
@yerke26
change to engine_forkchoiceUpdatedV4
372868a
@yerke26
Merge branch 'master' into feature/eip-7742
0bd75c6
@yerke26
add new fields to EngineModuleTests.V4
4b5fbfc
@yerke26
added new engine_forkchoiceUpdatedV4 to EngineRpcCapabilitiesProvider
5078b76
@yerke26
fix test cases
a92cdde
@yerke26
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
@yerke26
fix whitespace
4ab5fda
@yerke26
fix EngineRpcCapabilitiesProvider, revert ChainSpecBasedSpecProviderT…
0ba2006 
…ests
@yerke26
remove unnecessary comment
911d232
@yerke26
remove comments
f806f22
@yerke26 yerke26 requested a review from MarekM25 October 8, 2024 10:26
@tanishqjasoria
Copy link
Contributor

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

tanishqjasoria
tanishqjasoria reviewed Oct 22, 2024
View reviewed changes
src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Core/Block.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Core/BlockHeader.cs Outdated Show resolved Hide resolved
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)
Copy link
Contributor

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?

src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs Show resolved Hide resolved
@yerke26
Merge branch 'master' into feature/eip-7742
a993d59
@yerke26
Merge branch 'master' into feature/eip-7742
ba1a3de
@yerke26
add eip7742 logic to TryCalculateFeePerBlobGas
87e721a
@yerke26
remove MaxBlobCount, pass Spec to BlobGasCalculator
e52fc98
@yerke26
Merge branch 'master' into feature/eip-7742
49424d9 
# Conflicts:
#	src/Nethermind/Nethermind.Consensus.AuRa/InitializationSteps/InitializeBlockchainAuRa.cs
#	src/Nethermind/Nethermind.Consensus.AuRa/Rewards/AuRaRewardCalculator.cs
#	src/Nethermind/Nethermind.Merge.AuRa/AuRaMergeBlockProducerEnvFactory.cs
#	src/Nethermind/Nethermind.Merge.AuRa/InitializationSteps/InitializeBlockchainAuRaMerge.cs
#	src/Nethermind/Nethermind.Merge.AuRa/Withdrawals/WithdrawalContractFactory.cs
@yerke26
resolve merge conflicts
2162240
tanishqjasoria
tanishqjasoria reviewed Nov 5, 2024
View reviewed changes
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.TxPool/TxPool.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs
@@ -112,8 +112,9 @@ public ResultWrapper<Address> eth_coinbase()
{
return ResultWrapper<UInt256?>.Success(UInt256.Zero);
}
IReleaseSpec? spec = _blockFinder.Head?.Header is null ? null : specProvider.GetSpec(_blockFinder.Head.Header);
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens when the spec is null?

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 spec is null, then in TryCalculateFeePerBlobGas we assume that Eip7742 is diabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanishqjasoria tanishqjasoria tanishqjasoria left review comments

@rubo rubo Awaiting requested review from rubo rubo is a code owner

@MarekM25 MarekM25 Awaiting requested review from MarekM25

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement EIP-7442 Uncouple blob count between CL and EL
3 participants
@yerke26 @tanishqjasoria @MarekM25