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 7840 implementation #7964

Merged
merged 105 commits into from
Jan 24, 2025
Merged

eip 7840 implementation #7964

merged 105 commits into from
Jan 24, 2025

Conversation

yerke26
Copy link
Contributor

@yerke26 yerke26 commented Dec 23, 2024

Closes #9129

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.

smartprogrammer93
smartprogrammer93 requested changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

looks good, but please add tests.

smartprogrammer93
smartprogrammer93 reviewed Dec 23, 2024
View reviewed changes
Copy link
Contributor

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

Also, as discussed. i dont think we need the EIP7840 enabled. we can just replace the constants with whatever we push in the chainspec for the cancun through the same mechanism

@yerke26 yerke26 requested a review from rubo as a code owner December 30, 2024 12:02
tanishqjasoria
tanishqjasoria requested changes Jan 7, 2025
View reviewed changes
src/Nethermind/Nethermind.Blockchain.Test/TransactionSelectorTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain.Test/TransactionSelectorTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain.Test/Validators/ShardBlobBlockValidatorTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Blockchain.Test/Validators/TxValidatorTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Core/Specs/ReleaseSpecDecorator.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Core/Specs/ReleaseSpecDecorator.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Evm.Test/TransactionProcessorEip4844Tests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs Outdated Show resolved Hide resolved
LukaszRozmej
LukaszRozmej reviewed Jan 22, 2025
View reviewed changes
src/Nethermind/Chains/chiado.json
"terminalTotalDifficulty": "231707791542740786049188744689299064356246512"
"terminalTotalDifficulty": "231707791542740786049188744689299064356246512",
"blobSchedule": {
"cancun": {
Copy link
Member

Choose a reason for hiding this comment

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

Don't like it to be honest it is inconsistent with our previous chainspecs. EIP's shouldn't define internal stuff. Should we consider going to timestamps @MarekM25 ?

src/Nethermind/Nethermind.Consensus/Messages/TxErrorMessages.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Producers/TxPoolTxSource.cs Outdated
{
if (_logger.IsTrace) _logger.Trace($"Declining {blobTx.ToShortString()}, no more blob space. Block already have {blobGasCounter} blob gas which is max value allowed.");
break;
}

checkedBlobTransactions++;

ulong txBlobGas = (ulong)(blobTx.BlobVersionedHashes?.Length ?? 0) * _eip4844Config.GasPerBlob;
if (txBlobGas > _eip4844Config.MaxBlobGasPerBlock - blobGasCounter)
ulong txBlobGas = (ulong)(blobTx.BlobVersionedHashes?.Length ?? 0) * Eip4844Constants.GasPerBlob;
Copy link
Member

Choose a reason for hiding this comment

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

This looks to me like it should be an extension method on Transaction object

src/Nethermind/Nethermind.Core.Test/Encoding/BlockDecoderTests.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Evm/VirtualMachine.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Modules/Eth/FeeHistory/FeeHistoryOracle.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs Outdated Show resolved Hide resolved
tools/SendBlobs/SendBlobs.csproj Show resolved Hide resolved
@yerke26 yerke26 mentioned this pull request Jan 23, 2025
Merged
16 tasks
yerke26 and others added 5 commits January 23, 2025 21:28
@yerke26 @MarekM25
Eip 7840 - add IReleaseSpec to BlockExecutionContext constructor (#8096)
e3b52cd 
Co-authored-by: Marek Moraczyński <[email protected]>
@yerke26
Merge branch 'master' into eip-7840
06157a4 
# Conflicts:
#	src/Nethermind/Nethermind.Specs.Test/ChainSpecStyle/ChainSpecBasedSpecProviderTests.cs
#	src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs
@yerke26
resolve merge conflicts
37b5079
@yerke26
mark ChainSpecBasedSpecProviderTests sequential
7d0ea7d
@yerke26
add todo for ChainSpecBasedSpecProviderTests
a25830c
@MarekM25 MarekM25 merged commit a453048 into master Jan 24, 2025
79 checks passed
@MarekM25 MarekM25 deleted the eip-7840 branch January 24, 2025 14:08
brbrr pushed a commit that referenced this pull request Jan 27, 2025
@yerke26 @MarekM25
@tanishqjasoria @brbrr
eip 7840 implementation (#7964)
ce7cb98 
Co-authored-by: MarekM25 <[email protected]>
Co-authored-by: Tanishq Jasoria <[email protected]>
@brbrr brbrr added this to the 1.31.0 milestone Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smartprogrammer93 smartprogrammer93 smartprogrammer93 approved these changes

@marcindsobczak marcindsobczak marcindsobczak approved these changes

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@MarekM25 MarekM25 MarekM25 approved these changes

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

@tanishqjasoria tanishqjasoria Awaiting requested review from tanishqjasoria

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

Successfully merging this pull request may close these issues.
8 participants
@yerke26 @MarekM25 @marcindsobczak @rubo @tanishqjasoria @LukaszRozmej @smartprogrammer93 @brbrr