Eip 4788/beacon parent block root in evm by Demuirgos · Pull Request #5476 · NethermindEth/nethermind

Demuirgos · 2023-03-21T20:44:02Z

Changes

Implements Eip4788
Adds BeaconStateRoot stateful precompile
Modifies BlockProcessor
Refactor access modifier of Precompile.Instance.Address field

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

smartprogrammer93

Would advice to keep refactoring changes in separate PR.

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

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

src/Nethermind/Nethermind.Evm/Precompiles/Statefull/BeaconStateRootPrecompile.cs

smartprogrammer93

Great, Still would put the refactoring changes in a separate PR <3

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

src/Nethermind/Nethermind.Evm/Precompiles/Stateful/BeaconStateRootPrecompile.cs

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

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

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

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

src/Nethermind/Nethermind.Evm/Precompiles/Stateful/BeaconStateRootPrecompile.cs

LukaszRozmej · 2023-07-07T09:21:18Z

src/Nethermind/Nethermind.Evm/Precompiles/Stateful/BeaconStateRootPrecompile.cs

+        Span<byte> inputDataSpan = stackalloc byte[32];
+        inputData.PrepareEthInput(inputDataSpan);
+
+        UInt256 timestamp = new UInt256(inputDataSpan, true);


Maybe we can avoid it with Unsafe? @benaadams

src/Nethermind/Nethermind.Evm/Precompiles/Stateful/BeaconStateRootPrecompile.cs

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

MarekM25

wrong naming (BeaconStateRoot -> ParentBeaconBlockRoot)?

MarekM25

Added a few comments.
Do you know about any hive tets/execution spec tests for this EIP?

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

MarekM25 · 2023-07-12T10:06:28Z

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

            Is.EqualTo(input.IsExcessDataGasSet));
    }

+    [TestCaseSource(nameof(BeacondStateRootGetPayloadV3ForDifferentSpecTestSource))]


no tests that verify if we can produce blocks with ParentBeaconBlockRoot and send them later through engine API?

no tests that we can't pass null ParentBeaconBlockRoot to payload attributes

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

src/Nethermind/Nethermind.Evm/Metrics.cs

src/Nethermind/Nethermind.Evm/Precompiles/Stateful/BeaconBlockRootPrecompile.cs

- Added Eip4788 wiring to ChainSpecs

- Added Eip4788 stateful precompile

LukaszRozmej · 2023-08-07T09:54:38Z

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs

+        stateProvider.CreateAccountIfNotExists(BeaconBlockRootPrecompile.Address, 1);
+
+        UInt256.Mod(timestamp, HISTORICAL_ROOTS_LENGTH, out UInt256 timestampReduced);
+        UInt256 rootIndex = timestampReduced + HISTORICAL_ROOTS_LENGTH;
+
+        StorageCell tsStorageCell = new(BeaconBlockRootPrecompile.Address, timestampReduced);
+        StorageCell brStorageCell = new(BeaconBlockRootPrecompile.Address, rootIndex);
+
+        stateProvider.Set(tsStorageCell, timestamp.ToBigEndian());
+        stateProvider.Set(brStorageCell, parentBeaconBlockRoot.Bytes.ToArray());


Wouldn't it be better if this code would live in some static method in BeaconBlockRootPrecompile?
You are touching its address, you are duplicating the logic of calculating indexes, doesn't make sense to be anywhere else.

funnily tho it was in a static method on IBeaconBlockRootPrecompile, but I dont think it's worth modifying now cause chances are it will be yeeeted out completely one the v2 comes arround

LukaszRozmej · 2023-08-07T09:58:58Z

src/Nethermind/Nethermind.Blockchain/BeaconBlockRoot/BeaconBlockRootHandler.cs

+{
+    public void HandleBeaconBlockRoot(Block block, IReleaseSpec spec, IWorldState stateProvider)
+    {
+        if (!spec.IsBeaconBlockRootAvailable) return;


Lets:

Rename it to InitStatefulPrecompiles (Handler?)

Create it in the BlockProcessor instead of injecting it, contrary to withdrawals which are consensus specific, this is directly controlled by spec, so doesn't make sense to inject different implementations - sorry if I confused you earlier.

perfect ig (it was painful to make it injectable xD)

…V3' into EIP-4788/BeaconStateRoot-in-EVM

…choiceUpdatedV3

…nStateRoot-in-EVM

…eRoot-in-EVM

Demuirgos requested a review from MarekM25 March 30, 2023 14:14

Demuirgos force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch from aa05d72 to 68c4fce Compare May 4, 2023 12:43

Demuirgos self-assigned this May 18, 2023

Demuirgos force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch from 9d4aadf to 7c35693 Compare June 13, 2023 16:30

Demuirgos marked this pull request as ready for review June 13, 2023 19:15

smartprogrammer93 requested changes Jun 15, 2023

View reviewed changes

Demuirgos requested a review from smartprogrammer93 June 15, 2023 15:25

smartprogrammer93 approved these changes Jun 15, 2023

View reviewed changes

MarekM25 mentioned this pull request Jun 16, 2023

Nethermind Cancun Tracker #5460

Closed

Demuirgos added the eip label Jun 22, 2023

Demuirgos force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch from af267e2 to d95d388 Compare June 23, 2023 11:37

Demuirgos requested review from a team and rubo as code owners June 23, 2023 11:37

Demuirgos requested review from benaadams and removed request for rubo July 4, 2023 13:51

benaadams approved these changes Jul 4, 2023

View reviewed changes

MarekM25 requested a review from smartprogrammer93 July 7, 2023 08:17

LukaszRozmej reviewed Jul 7, 2023

View reviewed changes

LukaszRozmej mentioned this pull request Jul 7, 2023

Make Engine *V3s handle V3 requests only #5845

Merged

12 tasks

benaadams reviewed Jul 7, 2023

View reviewed changes

src/Nethermind/Nethermind.Evm/Precompiles/Stateful/BeaconStateRootPrecompile.cs Outdated Show resolved Hide resolved

MarekM25 reviewed Jul 7, 2023

View reviewed changes

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

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

LukaszRozmej reviewed Jul 10, 2023

View reviewed changes

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

LukaszRozmej mentioned this pull request Jul 11, 2023

Made precompile address property static #5920

Merged

16 tasks

MarekM25 requested changes Jul 12, 2023

View reviewed changes

MarekM25 reviewed Jul 12, 2023

View reviewed changes

Demuirgos force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch 2 times, most recently from 9aabf7c to 224a831 Compare July 14, 2023 12:39

Demuirgos added 3 commits July 19, 2023 15:36

- Added Eip4788 to ReleaseSpecs

962de19

- Added Eip4788 wiring to ChainSpecs

- Added passing state to Precompiles

ec00aec

- Added Eip4788 stateful precompile

- Adds consensus logic of Eip4788

30f15e0

Demuirgos and others added 2 commits August 4, 2023 12:35

- ws fix

2a822bb

Init beacon root precompile state for genesis (#5982)

3d975ef

LukaszRozmej reviewed Aug 7, 2023

View reviewed changes

- Applied suggested changes

467d74c

flcl42 force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch from aea2755 to 3d975ef Compare August 7, 2023 13:53

flcl42 and others added 10 commits August 7, 2023 16:55

Merge remote-tracking branch 'origin/Engine/Rpc/Add-forkchoiceUpdated…

06d9491

…V3' into EIP-4788/BeaconStateRoot-in-EVM

Fix merge

2e3c632

- Added a engine_fcuV3 test

763d032

- Add simple BeaconParentBlockRoot test for NewPayloadV3

c4c4b12

- Assert precompile creation at cancun block execution

1447648

- Applied flcl's fixes

3a5379a

Merge remote-tracking branch 'origin/master' into Engine/Rpc/Add-fork…

7ed2ab4

…choiceUpdatedV3

- naming fix

a9a7e36

Merge branch 'Engine/Rpc/Add-forkchoiceUpdatedV3' into EIP-4788/Beaco…

f8c8b9b

…nStateRoot-in-EVM

Merge remote-tracking branch 'origin/master' into EIP-4788/BeaconStat…

895669d

…eRoot-in-EVM

Demuirgos force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch from 81eeece to 895669d Compare August 9, 2023 12:48

Demuirgos added 2 commits August 9, 2023 14:27

- ignore 0x0b from precompile checks

ddb0262

- applied some Alexey's fixes

1edb452

Demuirgos changed the base branch from master to cancun August 17, 2023 09:06

Demuirgos added 2 commits August 17, 2023 10:07

- merged master

4877dd6

- removed reference of Precompile from code

4da6ccd

Demuirgos changed the base branch from cancun to master August 17, 2023 09:28

Demuirgos added 4 commits August 17, 2023 10:34

removed unnecessary refs of BlockRootClass

dc1b94e

- removing more unnecessary refs

1a718f3

- added more of Alexey's fixes

706c8dc

revert master merge

26ddb49

Demuirgos force-pushed the EIP-4788/BeaconStateRoot-in-EVM branch from 3142e87 to 26ddb49 Compare August 17, 2023 09:54

- merged master (not definitive)

e0f109a

Demuirgos closed this Aug 17, 2023

rubo deleted the EIP-4788/BeaconStateRoot-in-EVM branch December 20, 2023 21:33

Conversation

Demuirgos commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Uh oh!

smartprogrammer93 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smartprogrammer93 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukaszRozmej Jul 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MarekM25 left a comment

Choose a reason for hiding this comment

Uh oh!

MarekM25 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarekM25 Jul 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukaszRozmej Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Demuirgos Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

LukaszRozmej Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Demuirgos Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

Demuirgos commented Mar 21, 2023 •

edited

Loading

smartprogrammer93 left a comment •

edited

Loading

LukaszRozmej Aug 7, 2023 •

edited

Loading