Implement beacon chain push withdrawals (EIP-4895) #4731

rubo · 2022-10-07T11:58:35Z

Closes #4580

Changes:

These changes are preliminary for prototyping purposes and are subject to change.

The new Withdrawal class has been added to the Block
The RLP serializer has been extended to handle withdrawals according to the spec
BlockProcessor has been modified to process withdrawals if any

Types of changes

What types of changes does your code introduce?

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation Update
Code style update (formatting, renaming)
Refactoring (no functional changes, no api changes)
Build related changes
Other (please describe):

Testing

Requires testing

Yes
No

In case you checked yes, did you write tests??

Yes
No

Further comments

src/Nethermind/Nethermind.Merge.Plugin/Data/V1/ExecutionPayloadV1.cs

src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs

src/Nethermind/Nethermind.Serialization.Rlp/WithdrawalDecoder.cs

MarekM25 · 2022-10-07T13:06:23Z

src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs

+
+            foreach (var withdrawal in block.Withdrawals)
+            {
+                if (_logger.IsTrace) _logger.Trace($"  {(BigInteger)withdrawal.Amount / (BigInteger)Unit.Ether:N3}{Unit.EthSymbol} to account {withdrawal.Recipient}");


what about traces around withdrawals?

MarekM25

Could you reach out Mikhail Kalinin and Alex Stokes about withdrawalRoot validation?

It should be added to BlockValidator for sure (even now), but the question is how we should validate them when we're receiving newPayload from CL client.

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

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

src/Nethermind/Nethermind.Merge.Plugin/IEngineRpcModule.cs

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

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

…stamp_activation_fix

…e_valid_responses

smartprogrammer93

LGTM

LukaszRozmej

Main issues:

~~Incomplete validation of index and amount could lead to consensus issue on our side~~
I don't like how engine versioning is being handled. IMO we should model it as it is in the spec with either composition or inheritance + generics for messages. This would help us have better separation between protocol versions similar to what we have for eth protocol in networking. Would make code changes tracked easier. Would make code more resemble the spec. We could potentially use similar strategy on tests. More details with example in NewPayloadHandler.cs comment.

src/Nethermind/Nethermind.Consensus/Withdrawals/ValidationWithdrawalProcessor.cs

src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs

src/Nethermind/Nethermind.Consensus/Withdrawals/ValidationWithdrawalProcessor.cs

src/Nethermind/Nethermind.Blockchain.Test/GenesisLoaderTests.cs

src/Nethermind/Nethermind.Core/BlockBody.cs

src/Nethermind/Nethermind.JsonRpc/IJsonRpcConfig.cs

src/Nethermind/Nethermind.JsonRpc/JsonRpcConfig.cs

src/Nethermind/Nethermind.Merge.AuRa.Test/AuRaMergePluginTests.cs

src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs

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

…b.com/nethermindeth/nethermind into feature/shanghai-eip-4895-withdrawals # Conflicts: # src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs

…into feature/shanghai-eip-4895-withdrawals

MarekM25 reviewed Oct 7, 2022

View reviewed changes

src/Nethermind/Nethermind.Merge.Plugin/Data/V1/ExecutionPayloadV1.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Oct 7, 2022

View reviewed changes

src/Nethermind/Nethermind.Consensus/Processing/BlockProcessor.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Oct 7, 2022

View reviewed changes

src/Nethermind/Nethermind.Serialization.Rlp/WithdrawalDecoder.cs Show resolved Hide resolved

MarekM25 reviewed Oct 7, 2022

View reviewed changes

MarekM25 requested changes Oct 7, 2022

View reviewed changes

MarekM25 mentioned this pull request Oct 10, 2022

Shanghai/Cancun tracking issue #4583

Closed

rubo force-pushed the feature/shanghai-eip-4895-withdrawals branch from 8d3db64 to 6d83a24 Compare October 13, 2022 20:36

rubo force-pushed the feature/shanghai-eip-4895-withdrawals branch from 2539d2f to 9e6ee93 Compare October 26, 2022 22:25

MarekM25 reviewed Oct 28, 2022

View reviewed changes

src/Nethermind/Nethermind.Serialization.Rlp/HeaderDecoder.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Oct 28, 2022

View reviewed changes

src/Nethermind/Nethermind.Serialization.Rlp/BlockDecoder.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Oct 28, 2022

View reviewed changes

src/Nethermind/Nethermind.Merge.Plugin/IEngineRpcModule.cs Show resolved Hide resolved

MarekM25 reviewed Oct 31, 2022

View reviewed changes

src/Nethermind/Nethermind.Serialization.Rlp/BlockDecoder.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Nov 1, 2022

View reviewed changes

src/Nethermind/Nethermind.Merge.Plugin/Data/ExecutionPayload.cs Outdated Show resolved Hide resolved

smartprogrammer93 and others added 17 commits November 2, 2022 18:19

More test fixes

f364bae

Merge branch 'master' into fix/forkId_calculation_is_wrong_after_time…

92f8394

…stamp_activation_fix

Fix for init and meter

b5f9883

Merge branch 'master' into feature/shanghai-eip-4895-withdrawals

b288926

Make withdrawals decoding optional

9781b77

include 4895 in chainspecParameters

7b4d460

introduce WithdrawalTimestamp

c793652

Fix PayloadAttributes handling when null

a783b04

Fix withdrawals length calculation

6c7d34f

Refactor and add checks for withdrawals for RPC methods of v1

4207577

Add engine_getPayloadV2

1f1a08f

Format whitespaces

5275510

Refactor and remove redundant "V1"

ba0219b

started adding Enginge V2 tests - V2_processing_block_should_serializ…

3d09efa

…e_valid_responses

formatting

10dcda2

formatting

94c17cb

Implement withdrawal root validation

45aae0a

smartprogrammer93 approved these changes Jan 3, 2023

View reviewed changes

Merge branch 'master' into feature/shanghai-eip-4895-withdrawals

7e804d8

rubo force-pushed the feature/shanghai-eip-4895-withdrawals branch from d506374 to 7e804d8 Compare January 4, 2023 20:41

Add missing file headers

b3cd6bc

LukaszRozmej requested changes Jan 5, 2023

View reviewed changes

MarekM25 and others added 7 commits January 5, 2023 10:22

resolved some review comments

ca1d670

more review comments

e8df266

more review related changes

ab6cf2e

fix Benchmarks build

b920306

Refactor tests

96b43d2

refactor tests - review comment

82707e4

cosmetic

9fe4cb2

MarekM25 requested a review from LukaszRozmej January 6, 2023 15:33

LukaszRozmej approved these changes Jan 6, 2023

View reviewed changes

rubo added 2 commits January 6, 2023 18:21

Revise suggested block validation messages

355be4f

Refactor patricia tries

8e7be8d

rubo force-pushed the feature/shanghai-eip-4895-withdrawals branch from b4bcf93 to 8e7be8d Compare January 6, 2023 21:16

Merge branch 'master' into feature/shanghai-eip-4895-withdrawals

a25e999

rubo force-pushed the feature/shanghai-eip-4895-withdrawals branch from dcb5a6a to a25e999 Compare January 6, 2023 22:01

MarekM25 added 4 commits January 9, 2023 11:46

fix InvalidBlockInterceptor for Withdrawals

8224f06

Merge branch 'feature/shanghai-eip-4895-withdrawals' of https://githu…

7cf5d72

…b.com/nethermindeth/nethermind into feature/shanghai-eip-4895-withdrawals # Conflicts: # src/Nethermind/Nethermind.Consensus/Validators/BlockValidator.cs

fix whitespaces

cf62d5e

fix tests

6aec41d

rubo marked this pull request as draft January 9, 2023 13:30

rubo and others added 2 commits January 9, 2023 18:48

Merge branch 'master' into feature/shanghai-eip-4895-withdrawals

89e1705

Merge branch 'master' of https://github.com/nethermindeth/nethermind …

c579ce8

…into feature/shanghai-eip-4895-withdrawals

MarekM25 marked this pull request as ready for review January 10, 2023 14:52

MarekM25 merged commit 193c1c3 into master Jan 11, 2023

MarekM25 deleted the feature/shanghai-eip-4895-withdrawals branch January 11, 2023 12:44

gakonst mentioned this pull request Feb 11, 2023

Tracking: Shanghai Support paradigmxyz/reth#1295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement beacon chain push withdrawals (EIP-4895) #4731

Implement beacon chain push withdrawals (EIP-4895) #4731

rubo commented Oct 7, 2022 •

edited

Loading

MarekM25 Oct 7, 2022

MarekM25 left a comment

smartprogrammer93 left a comment

LukaszRozmej left a comment •

edited by rubo

Loading

Implement beacon chain push withdrawals (EIP-4895) #4731

Implement beacon chain push withdrawals (EIP-4895) #4731

Conversation

rubo commented Oct 7, 2022 • edited Loading

Changes:

Types of changes

Testing

Further comments

MarekM25 Oct 7, 2022

Choose a reason for hiding this comment

MarekM25 left a comment

Choose a reason for hiding this comment

smartprogrammer93 left a comment

Choose a reason for hiding this comment

LukaszRozmej left a comment • edited by rubo Loading

Choose a reason for hiding this comment

rubo commented Oct 7, 2022 •

edited

Loading

LukaszRozmej left a comment •

edited by rubo

Loading