[WIP]Add Eip6110 implementation #6849

Demuirgos · 2024-03-20T12:30:39Z

Fixes Closes Resolves #6274

Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.

Changes

implements Eip6110 as per spec : https://eips.ethereum.org/EIPS/eip-6110

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.

+ add missing checks

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

MarekM25 · 2024-03-27T14:29:52Z

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

+            {
+                foreach (var log in receipts[i].Logs)
+                {
+                    if (log.LoggersAddress == spec.Eip6110ContractAddress)


Please find the address for various networks (holesky, sepolia, mainnet, chiado, gnosis) and hardcode them in ChainSpecBasedSpecProviderTests

MarekM25 · 2024-03-27T14:32:47Z

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

+                    {
+                        var depositDecoder = new DepositDecoder();
+                        Deposit? deposit = depositDecoder.Decode(new RlpStream(log.Data));
+                        depositList[depositCount++] = deposit;


this will throw exception if processedBlock has more deposits than suggested block

I am afriad that such exception won't be handled (perhaps, it should) and will destroy BlockProcessingQueue.

It means that you would be able to break Nethermind nodes by posting invalid block

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

src/Nethermind/Nethermind.Consensus/Validators/IBlockValidator.cs

src/Nethermind/Nethermind.Core.Test/Builders/BlockValidatorBuilder.cs

MarekM25 · 2024-03-27T15:32:24Z

src/Nethermind/Nethermind.Core/Block.cs

@@ -24,15 +24,16 @@ public Block(BlockHeader header, BlockBody body)
        BlockHeader header,
        IEnumerable<Transaction> transactions,
        IEnumerable<BlockHeader> uncles,
-        IEnumerable<Withdrawal>? withdrawals = null)
+        IEnumerable<Withdrawal>? withdrawals = null,
+        IEnumerable<Deposit>? deposits = null)
    {


Check where this constructor is being used and you will find a few obvious bugs

MarekM25 · 2024-03-27T15:41:38Z

src/Nethermind/Nethermind.Core/Deposit.cs

+/// </summary>
+public class Deposit
+{
+    public byte[]? PublicKey { get; set; }


Is there Engine API spec for this EIP? Names of fields matter for json serialization, here: https://eips.ethereum.org/EIPS/eip-6110 PubKey was used, but the correct names should come from Engine API

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

src/Nethermind/Nethermind.Core/Deposit.cs

src/Nethermind/Nethermind.Serialization.Rlp/BlockDecoder.cs

src/Nethermind/Nethermind.Serialization.Rlp/HeaderDecoder.cs

MarekM25 · 2024-03-27T15:58:00Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs

@@ -252,6 +252,9 @@ private static ReleaseSpec CreateReleaseSpec(ChainSpec chainSpec, long releaseSt
            releaseSpec.IsEip4788Enabled = (chainSpec.Parameters.Eip4788TransitionTimestamp ?? ulong.MaxValue) <= releaseStartTimestamp;
            releaseSpec.Eip4788ContractAddress = chainSpec.Parameters.Eip4788ContractAddress;

+            releaseSpec.IsEip6110Enabled = (chainSpec.Parameters.Eip6110TransitionTimestamp ?? ulong.MaxValue) <= releaseStartTimestamp;
+            releaseSpec.Eip6110ContractAddress = chainSpec.Parameters.Eip6110ContractAddress;


What should we do if this EIP is specified in config, but no addres? should we stop application with info?

I suppose so, as both inputs are required for correct config

src/Nethermind/Nethermind.Specs/Forks/17_Prague.cs

MarekM25

Extra things that requires thoughts/changes:

GenesisLoader - how deposits should be loaded by defaulf from chainspec if we start from Prague in genesis, I guess it should be empty deposit
-EngineModuleTests - we need a few tests
BlockForRpc? with question mark not sure
exchangeCapabilitesHandler

MarekM25

I think I see one more potential red flag:
I don't see changes in BlockProcessor. For example, for withdrawals which are very similar we have them here: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs#L250

For EIP 7002 we have it here:
https://github.com/NethermindEth/nethermind/blob/feature/eip-7002/src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs#L256

Additionally, check how withdrawals handled root creation for block production, check those two classes:
WithdrawalProcessor
BlockProductionWithdrawalProcessor

It seems that 7002 started with different approach, we have to think about and make it consistent for withdrawals/6110/7002

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

src/Nethermind/Nethermind.Core/Block.cs

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecLoader.cs

- moving code from BlockValidator to BlockProcessor - fixing error discards

ak88 · 2024-04-11T09:56:22Z

src/Nethermind/Nethermind.Consensus/Deposits/DepositsProcessor.cs

+        _logger = logManager.GetClassLogger();
+    }
+
+    public void ProcessDeposits(Block block, TxReceipt[] receipts, IReleaseSpec spec)


Consider moving IReleaseSpec to ctor instead, or alternatively remove it and make the if check before consumers call ProcessDeposits.

I think it is not possible, you could pass specProvider only. To me this is okay

src/Nethermind/Nethermind.Consensus/Messages/BlockErrorMessages.cs

Co-authored-by: ak88 <anders.holmbjerg@hotmail.com>

MarekM25

CI is still red

MarekM25 · 2024-04-16T10:16:40Z

src/Nethermind/Nethermind.Consensus/Messages/BlockErrorMessages.cs

@@ -116,4 +117,7 @@ public static class BlockErrorMessages

    public const string NegativeGasUsed =
        "NegativeGasUsed: Cannot be negative.";
+    public static string MissingDeposits => "MissingDeposits: Deposits cannot be null in block when EIP-6110 activated.";


Why do we have such messages for deposits in BlockErrorMessages but we don't for withdrawals and 7002?

we have MissingWithdrawals Error, I am not sure abt Eip7002 tho

src/Nethermind/Nethermind.Consensus/Withdrawals/WithdrawalProcessor.cs

MarekM25 · 2024-04-16T10:21:01Z

src/Nethermind/Nethermind.Consensus/Processing/BlockExtensions.cs

@@ -19,7 +19,7 @@ internal static class BlockExtensions
        public static Block CreateCopy(this Block block, BlockHeader header) =>
            block is BlockToProduce blockToProduce
                ? new BlockToProduce(header, blockToProduce.Transactions, blockToProduce.Uncles, blockToProduce.Withdrawals)


@Demuirgos @rjnrohit check this line carefully:
? new BlockToProduce(header, blockToProduce.Transactions, blockToProduce.Uncles, blockToProduce.Withdrawals)

analyze all usages of BlockToProduce constructor

MarekM25 · 2024-04-16T10:23:21Z

src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V3.cs

@@ -355,6 +355,7 @@ async Task<(MergeTestBlockchain blockchain, IEngineRpcModule engineRpcModule)> M
                 Substitute.For<IAsyncHandler<byte[], ExecutionPayload?>>(),


I can't find EngineModuleTests.V4

src/Nethermind/Nethermind.Consensus/Deposits/DepositsProcessor.cs

MarekM25 · 2024-04-16T11:10:42Z

src/Nethermind/Nethermind.Consensus/Deposits/DepositsProcessor.cs

+                if (log.LoggersAddress == spec.Eip6110ContractAddress)
+                {
+                    var depositDecoder = new DepositDecoder();
+                    Deposit? deposit = depositDecoder.Decode(new RlpStream(log.Data));


Open question, probably to other teams as well: is it possible to get incorrect data here?

MarekM25 · 2024-04-16T11:12:34Z

src/Nethermind/Nethermind.Consensus/Deposits/DepositsProcessor.cs

+        CalculateDepositsRoot(block, depositList, spec);
+    }
+
+    private void CalculateDepositsRoot(Block block, IEnumerable<Deposit> deposits, IReleaseSpec spec)


Cosmetic a bit, but worth considering: Withdrawals only calculates root during the block production while exits and deposits always calculate the root.
cc: @rjnrohit @Demuirgos

MarekM25 · 2024-04-16T12:17:07Z

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs

@@ -252,6 +252,9 @@ private static ReleaseSpec CreateReleaseSpec(ChainSpec chainSpec, long releaseSt
            releaseSpec.IsEip4788Enabled = (chainSpec.Parameters.Eip4788TransitionTimestamp ?? ulong.MaxValue) <= releaseStartTimestamp;
            releaseSpec.Eip4788ContractAddress = chainSpec.Parameters.Eip4788ContractAddress;

+            releaseSpec.IsEip6110Enabled = (chainSpec.Parameters.Eip6110TransitionTimestamp ?? ulong.MaxValue) <= releaseStartTimestamp;
+            releaseSpec.Eip6110ContractAddress = chainSpec.Parameters.Eip6110ContractAddress;


(Cosmetic) I think it would be nice to rename it to DepositContractAddress. This address could be used in the future in other places

…ethermind into feature/eip/6110

rjnrohit · 2024-04-17T17:33:15Z

src/Nethermind/Nethermind.Merge.Plugin/EngineRpcModule.Prague.cs

+    private readonly IAsyncHandler<byte[], GetPayloadV4Result?> _getPayloadHandlerV4;
+
+    public Task<ResultWrapper<ForkchoiceUpdatedV1Result>> engine_forkchoiceUpdatedV4(ForkchoiceStateV1 forkchoiceState, PayloadAttributes? payloadAttributes = null)
+        => ForkchoiceUpdated(forkchoiceState, payloadAttributes, EngineApiVersions.Cancun);


change from EngineApiVersions.Cancun to EngineApiVersions.Prague

Demuirgos added 11 commits February 5, 2024 00:34

+ Draft implementation of Eip6110

70a3e5c

Merge remote-tracking branch 'origin/master' into Feature/Eip/6110

02974cc

fix merge issue

c541cb1

- fix NullPtrException in BlockProcessing

b9d03c4

add missing fields to ChainSpecJson

dfc79d6

- added new fields to Builder api

45e0c44

- revert ReleaseSpec creeping out

c336a0f

+ add missing checks

Ammended PayloadAttribute and ExecutionPayload with new Deposits field

f3271b2

Merge remote-tracking branch 'origin/master' into Feature/Eip/6110

81466dd

fix erroneous change

e5c7ed0

merge master and resolve conflicts

4e91720

MarekM25 mentioned this pull request Mar 27, 2024

Nethermind Pectra Tracker #6867

Open

fix

7ee1fef

MarekM25 reviewed Mar 27, 2024

View reviewed changes

MarekM25 added 3 commits March 27, 2024 16:38

Typo credentials

d700629

cosmetic comment fix

3d9ab43

more fixes

758ffa5

MarekM25 reviewed Mar 27, 2024

View reviewed changes

merge master, resolve errors, apply suggestions

a518336

MarekM25 requested changes Mar 27, 2024

View reviewed changes

Demuirgos added 4 commits March 28, 2024 01:18

Added roundtrip tests

130593c

Merge remote-tracking branch 'origin/master' into feature/eip/6110

1353354

add v4 capabilities

852b4b7

add back executionParloadV3 params to executionParloadV4

ec51f7f

MarekM25 requested changes Apr 3, 2024

View reviewed changes

MarekM25 mentioned this pull request Apr 3, 2024

[WIP]EIP-7002 implementation #6648

Draft

16 tasks

Demuirgos and others added 2 commits April 3, 2024 12:04

minor fixes

04f881b

Merge branch 'master' into feature/eip/6110

56559ba

MarekM25 reviewed Apr 5, 2024

View reviewed changes

src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayloadV4.cs Show resolved Hide resolved

src/Nethermind/Nethermind.Core/Block.cs Show resolved Hide resolved

revert change

b0a809f

rjnrohit reviewed Apr 9, 2024

View reviewed changes

src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecLoader.cs Show resolved Hide resolved

rjnrohit and others added 4 commits April 10, 2024 14:22

add deposists in BlockForRPC

3ada282

- minor chainspecloader fixes

c9be5af

- moving code from BlockValidator to BlockProcessor - fixing error discards

merge pulled commits

9429e49

fix build issues

4018745

Demuirgos requested review from ak88 and MarekM25 April 11, 2024 07:11

ak88 reviewed Apr 11, 2024

View reviewed changes

Demuirgos and others added 2 commits April 14, 2024 03:21

minor fix

685617d

Merge branch 'master' into feature/eip/6110

e6f0e33

ak88 reviewed Apr 15, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Messages/BlockErrorMessages.cs Outdated Show resolved Hide resolved

ak88 reviewed Apr 15, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Messages/BlockErrorMessages.cs Outdated Show resolved Hide resolved

ak88 reviewed Apr 15, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Messages/BlockErrorMessages.cs Outdated Show resolved Hide resolved

Demuirgos and others added 3 commits April 15, 2024 09:48

Update BlockErrorMessages.cs

4cc7255

Co-authored-by: ak88 <anders.holmbjerg@hotmail.com>

Update BlockErrorMessages.cs

b020b87

Co-authored-by: ak88 <anders.holmbjerg@hotmail.com>

Update BlockErrorMessages.cs

c7dcf00

Co-authored-by: ak88 <anders.holmbjerg@hotmail.com>

MarekM25 requested changes Apr 16, 2024

View reviewed changes

MarekM25 reviewed Apr 16, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Withdrawals/WithdrawalProcessor.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Apr 16, 2024

View reviewed changes

src/Nethermind/Nethermind.Consensus/Deposits/DepositsProcessor.cs Outdated Show resolved Hide resolved

Cosmetic cleanup

e30be14

MarekM25 reviewed Apr 16, 2024

View reviewed changes

Demuirgos added 3 commits April 17, 2024 06:15

merge changes

931d1dd

Merge branch 'feature/eip/6110' of https://github.com/NethermindEth/n…

a86ac19

…ethermind into feature/eip/6110

rename EipPropertyName, fix BlockToBuild ctpr call

066af5a

rjnrohit reviewed Apr 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Add Eip6110 implementation #6849

[WIP]Add Eip6110 implementation #6849

Demuirgos commented Mar 20, 2024

MarekM25 Mar 27, 2024

MarekM25 Mar 27, 2024

MarekM25 Mar 27, 2024

MarekM25 Mar 27, 2024

MarekM25 Mar 27, 2024

Demuirgos Mar 27, 2024

MarekM25 left a comment

MarekM25 left a comment

ak88 Apr 11, 2024

MarekM25 Apr 16, 2024

MarekM25 left a comment

MarekM25 Apr 16, 2024

Demuirgos Apr 17, 2024

MarekM25 Apr 16, 2024

MarekM25 Apr 16, 2024

MarekM25 Apr 16, 2024

MarekM25 Apr 16, 2024

MarekM25 Apr 16, 2024

rjnrohit Apr 17, 2024

		@@ -355,6 +355,7 @@ async Task<(MergeTestBlockchain blockchain, IEngineRpcModule engineRpcModule)> M
		Substitute.For<IAsyncHandler<byte[], ExecutionPayload?>>(),

[WIP]Add Eip6110 implementation #6849

Are you sure you want to change the base?

[WIP]Add Eip6110 implementation #6849

Conversation

Demuirgos commented Mar 20, 2024

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

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

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

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