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 84 commits into
base: master
Choose a base branch
from
Open

EIP 7742 implementation #7518

wants to merge 84 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 added 2 commits October 2, 2024 17:21
@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 Outdated Show resolved Hide resolved
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 added 13 commits October 4, 2024 15:41
@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
LukaszRozmej
LukaszRozmej reviewed Dec 9, 2024
View reviewed changes
src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.FeeHistory.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Evm/TransactionExtensions.cs Outdated Show resolved Hide resolved
LukaszRozmej
LukaszRozmej approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Apart of MaxBlobCount confusion I am fine with the PR

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs Outdated Show resolved Hide resolved
ak88
ak88 reviewed Dec 11, 2024
View reviewed changes
Copy link
Contributor

@ak88 ak88 left a comment

Choose a reason for hiding this comment

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

what about the new way to calculate excess blob gas that the EIP mentions?
https://eips.ethereum.org/EIPS/eip-7742#block-processing

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

if (!spec.IsEip7742Enabled || payloadAttributes?.MaxBlobCount is not null)
{
SelectBlobTransactions(blobTransactions, parent, spec, selectedBlobTxs);
Copy link
Contributor

Choose a reason for hiding this comment

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

You dont pass the PayloadAttributes

src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs
@@ -119,7 +123,7 @@ private static IEnumerable<Transaction> PickBlobTxsBetterThanCurrentTx(ArrayPool
}
}

private void SelectBlobTransactions(IEnumerable<Transaction> blobTransactions, BlockHeader parent, IReleaseSpec spec, ArrayPoolList<Transaction> selectedBlobTxs)
private void SelectBlobTransactions(IEnumerable<Transaction> blobTransactions, BlockHeader parent, IReleaseSpec spec, ArrayPoolList<Transaction> selectedBlobTxs, PayloadAttributes? payloadAttributes = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is PayloadAttributes optional?

@yerke26
Merge branch 'master' into feature/eip-7742
e231959 
# Conflicts:
#	src/Nethermind/Nethermind.Merge.AuRa.Test/AuRaMergeEngineModuleTests.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V4.cs
@yerke26
resolve merge conflicts
5c6dc2c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@ak88 ak88 ak88 left review comments

@tanishqjasoria tanishqjasoria tanishqjasoria approved these changes

@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
5 participants
@yerke26 @tanishqjasoria @LukaszRozmej @ak88 @MarekM25