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?

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.

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

@yerke26 this needs to be corrected

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

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

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

# 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

src/Nethermind/Nethermind.Consensus/Producers/PayloadAttributes.cs

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

src/Nethermind/Nethermind.Evm/BlobGasCalculator.cs

src/Nethermind/Nethermind.Evm/BlockExecutionContext.cs

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

src/Nethermind/Nethermind.TxPool/TxPool.cs

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

tanishqjasoria · 2024-11-05T04:29:58Z

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);


what happens when the spec is null?

if spec is null, then in TryCalculateFeePerBlobGas we assume that Eip7742 is diabled

# Conflicts: # src/Nethermind/Nethermind.AuRa.Test/Contract/ReportingValidatorContractTests.cs # src/Nethermind/Nethermind.Consensus/Requests/RequestProcessor.cs # src/Nethermind/Nethermind.Merge.AuRa.Test/AuRaMergeEngineModuleTests.cs # src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V4.cs # src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs # src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.Prague.cs # src/Nethermind/Nethermind.Merge.Plugin/Handlers/EngineRpcCapabilitiesProvider.cs # src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs # src/Nethermind/Nethermind.Merge.Plugin/IEngineRpcModule.Prague.cs # src/Nethermind/Nethermind.Serialization.Rlp/HeaderDecoder.cs # src/Nethermind/Nethermind.Shutter/ShutterBlockHandler.cs

LukaszRozmej

I havn't yet reviewed the whole thing, but I think the number of small changes to pass around spec provider suggest we are doing it wrong.

What I think we should do is some kind of refactor.

One thing we could do is change BlobGasCalculator from static class to an instance class (with interface for potential extensions). This would allow us to inject SpecProvider to its constructor and be done with it.

We could also make fee calculator part of the spec. This is similar to what was discussed (but not executed) in: #7761 (comment) in regards to BaseFeeCalculator.

We could even calculate BlobBaseFee in some transient BlockHeader property on start of processing. But that has it's own downsides.

Or as BLOBBASEFEE is the only place that uses it from BlockContext and it already has access to spec and header - lazy calculate it there and only then store it in block context.

Whichever thing we do I thing the current approach should be changed as this change seems to invasive for me, for such a small thing.

src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs

# Conflicts: # src/Nethermind/Nethermind.Consensus/Processing/BlockCachePreWarmer.cs

Co-authored-by: Lukasz Rozmej <[email protected]>

…ip-7742

tanishqjasoria · 2024-12-04T06:31:13Z

One thing we could do is change BlobGasCalculator from static class to an instance class (with interface for potential extensions). This would allow us to inject SpecProvider to its constructor and be done with it.

these were my initial thoughts as well, but then we might need to pass BlobGasCalculator around if we change it to an instance class.

LukaszRozmej · 2024-12-04T09:17:16Z

One thing we could do is change BlobGasCalculator from static class to an instance class (with interface for potential extensions). This would allow us to inject SpecProvider to its constructor and be done with it.

these were my initial thoughts as well, but then we might need to pass BlobGasCalculator around if we change it to an instance class.

Yeah the easiest workaround is to calculate BlobBaseFee lazily from BLOBBASEFEE opcode implementation

ak88 · 2024-12-03T13:38:05Z

src/Nethermind/Nethermind.Blockchain/Contracts/Contract.cs

@@ -36,18 +37,21 @@ public abstract partial class Contract
        protected IAbiEncoder AbiEncoder { get; }
        public AbiDefinition AbiDefinition { get; }
        public Address? ContractAddress { get; protected set; }
+        protected ISpecProvider SpecProvider { get; set; }


Why is this needed ?

ak88 · 2024-12-03T13:56:59Z

src/Nethermind/Nethermind.Core/Eip7742Constants.cs

+    /// Gets the <c>BLOB_GASPRICE_UPDATE_FRACTION</c> parameter.
+    /// </summary>
+    /// <remarks>Defaults to 3338477.</remarks>
+    public static UInt256 BlobGasPriceUpdateFraction { get; private set; } = 1112825;


can just be normal constant.

ak88 · 2024-12-03T21:29:29Z

src/Nethermind/Nethermind.Blockchain/Contracts/Contract.cs

        /// <param name="abiEncoder">Binary interface encoder/decoder.</param>
        /// <param name="contractAddress">Address where contract is deployed.</param>
        /// <param name="abiDefinition">Binary definition of contract.</param>
-        protected Contract(IAbiEncoder? abiEncoder = null, Address? contractAddress = null, AbiDefinition? abiDefinition = null)
+        protected Contract(ISpecProvider specProvider, IAbiEncoder? abiEncoder = null, Address? contractAddress = null, AbiDefinition? abiDefinition = null)


This isnt actually used and it causes a lot of changes.

ak88 · 2024-12-04T23:21:52Z

src/Nethermind/Nethermind.Evm/BlobGasCalculator.cs

@@ -85,10 +84,16 @@ static bool FakeExponentialOverflow(UInt256 factor, UInt256 num, UInt256 denomin
            return false;
        }

-        return !FakeExponentialOverflow(Eip4844Constants.MinBlobGasPrice, excessBlobGas, Eip4844Constants.BlobGasPriceUpdateFraction, out feePerBlobGas);
+        var denominator = spec?.IsEip7742Enabled ?? false
+            ? Eip7742Constants.BlobGasPriceUpdateFraction * targetBlobCount


Where does Eip7742Constants.BlobGasPriceUpdateFraction come from? The eip doesn't mention updating the rate of change?

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

resolve merge conflicts

2162240

tanishqjasoria reviewed Nov 5, 2024

View reviewed changes

yerke26 added 17 commits November 5, 2024 13:14

add MaxBlobCount in PayloadAttributes, refactoring, resolve comments

9d4ec74

fix tests

d8e088c

Merge branch 'master' into feature/eip-7742

793d085

fix Evm tool

a4fd1ef

fix Evm tool

d022c4f

resolve merge conflicts

020ceef

fix some tests

5699ad4

Merge branch 'master' into feature/eip-7742

81fb376

fix tests

332322d

fix AuraTests

91822aa

fix tests

b4790dd

fix JsonRpcTests

36464ad

fix TraceRpcModuleTests

459ebb7

fix tests

71963b7

fix tests

94a1e4d

fix whitespaces

35d29c5

yerke26 requested a review from tanishqjasoria December 2, 2024 01:35

LukaszRozmej self-requested a review December 2, 2024 11:11

LukaszRozmej requested changes Dec 3, 2024

View reviewed changes

src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs Outdated Show resolved Hide resolved

yerke26 and others added 4 commits December 3, 2024 16:03

Merge branch 'master' into feature/eip-7742

5fab184

# Conflicts: # src/Nethermind/Nethermind.Consensus/Processing/BlockCachePreWarmer.cs

Update src/Nethermind/Ethereum.Test.Base/GeneralTestBase.cs

f00d86a

Co-authored-by: Lukasz Rozmej <[email protected]>

Merge remote-tracking branch 'origin/feature/eip-7742' into feature/e…

1b9b367

…ip-7742

Merge branch 'master' into feature/eip-7742

f13d22f

ak88 reviewed Dec 4, 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

tanishqjasoria Nov 5, 2024

tanishqjasoria Nov 5, 2024

tanishqjasoria Dec 3, 2024

tanishqjasoria Nov 5, 2024

yerke26 Nov 5, 2024

LukaszRozmej left a comment

tanishqjasoria commented Dec 4, 2024 •

edited

Loading

LukaszRozmej commented Dec 4, 2024

ak88 Dec 3, 2024

ak88 Dec 3, 2024

ak88 Dec 3, 2024

ak88 Dec 4, 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

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

LukaszRozmej left a comment

Choose a reason for hiding this comment

tanishqjasoria commented Dec 4, 2024 • edited Loading

LukaszRozmej commented Dec 4, 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

yerke26 commented Sep 30, 2024 •

edited

Loading

tanishqjasoria commented Dec 4, 2024 •

edited

Loading