EIP-6110: Add deposits in EL #5055

ensi321 · 2023-02-05T06:54:16Z

PR description

Add deposits to the Execution Layer block structure: EIP-6110. The scope of this PR is to add Deposit related info into BlockHeader and BlockBody. The rest of the EIP including rpc api and validating Deposit with be included in the next PR.

Fixed Issue(s)

#5004

Documentation

I thought about documentation and added the doc-change-required label to this PR if
updates are required.

Changelog

I thought about the changelog and included a changelog update if required.

siladu · 2023-02-10T20:01:04Z

Excited to review this! Thanks for picking it up @naviechan 👍

siladu · 2023-02-18T22:31:33Z

Hey @naviechan check out this page about DCO https://wiki.hyperledger.org/display/BESU/DCO

All commits need to be signed off to pass this check. You can amend your commit messages and force push (or squash the commits and do it once).

I've just approved the CI to run the rest of the checks - you might need to prod us if you want to run these at any point (or you can just run gradle commands locally to check unit tests etc)

Spotless is failing, to fix run ./gradlew spotlessApply and commit the changes.

Javadoc is also failing, run ./gradlew javadoc locally and fix up the warnings, certain packages require javadoc to be added to public methods.

Finally, assemble is failing. I would recommend at least running ./gradlew assemble locally before pushing to CI.
Not a bad idea to always run spotlessApply too, you can script both this and the dco so you don't forget (I use a precommit hook)

Signed-off-by: Navie Chan <naviechan@gmail.com>

ensi321 · 2023-02-19T17:11:24Z

Hey @naviechan check out this page about DCO https://wiki.hyperledger.org/display/BESU/DCO

All commits need to be signed off to pass this check. You can amend your commit messages and force push (or squash the commits and do it once).

I've just approved the CI to run the rest of the checks - you might need to prod us if you want to run these at any point (or you can just run gradle commands locally to check unit tests etc)

Spotless is failing, to fix run ./gradlew spotlessApply and commit the changes.

Javadoc is also failing, run ./gradlew javadoc locally and fix up the warnings, certain packages require javadoc to be added to public methods.

Finally, assemble is failing. I would recommend at least running ./gradlew assemble locally before pushing to CI. Not a bad idea to always run spotlessApply too, you can script both this and the dco so you don't forget (I use a precommit hook)

Thanks so much this is very helpful! I think I ran into some kind of error for :acceptance-tests:tests:compileSolidity. The server returns 503 not sure what I can do on my end.

Also sorry for this huge PR. Was trying to limit the scope of this PR to just adding Deposit to block header and body. Turns out I had to modify a lot of the classes 😂

I will open another PR for json-rpc related changes

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/DepositsProcessor.java

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/Deposit.java

+import org.apache.tuweni.bytes.Bytes;
+import org.apache.tuweni.units.bigints.UInt64;
+
+public class Deposit implements org.hyperledger.besu.plugin.data.Deposit {


datatypes/src/main/java/org/hyperledger/besu/datatypes/WithdrawalCredential.java

Signed-off-by: Navie Chan <naviechan@gmail.com>

siladu · 2023-02-20T06:38:30Z

Thanks so much this is very helpful! I think I ran into some kind of error for :acceptance-tests:tests:compileSolidity. The server returns 503 not sure what I can do on my end.

This was a transient error, I gave it a kick and now you have a genuine test compilation error.

Also sorry for this huge PR. Was trying to limit the scope of this PR to just adding Deposit to block header and body. Turns out I had to modify a lot of the classes 😂

Large PRs are hard to review for sure, but not impossible. One approach that seems to work well is something like...

PR to add feature toggle
PR to add new types
PR per feature, behind the toggle where necessary (should be production grade, including tests)

You can always use one giant draft PR as a prototype before splitting into smaller ones, but I think getting some code onto main ASAP is the least painful approach.

By the way, I see you added eip6110time. That'll work fine and it may prove useful to have a separate flag, but note we already have experimentaleipstime for this purpose: #4873

siladu · 2023-02-20T06:40:58Z

@naviechan would you like someone to do an early review or would you prefer to wait until you mark it as ready?

ensi321 · 2023-02-20T07:31:19Z

@

Yes please do an early review! Thanks

Signed-off-by: Navie Chan <naviechan@gmail.com>

ensi321 · 2023-02-22T17:15:35Z

By the way, I see you added eip6110time. That'll work fine and it may prove useful to have a separate flag, but note we already have experimentaleipstime for this purpose: #4873

Thanks for the feedback. I will remove the EIP6110Time in favour of experimentaleipstime

Signed-off-by: navie <naviechan@gmail.com>

...reum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetBlockBodyValidator.java

@@ -304,4 +308,8 @@

    return true;
  }
+
+  private boolean validateDeposits(final Block unusedBlock) {


siladu

Thanks for your small commits and clear commit messages :)

I am pretty happy with this, only blocker is the BodyValidator test which I'll see if I can help you with.

Will give @jframe a chance to re-review as well.

siladu · 2023-03-21T05:25:14Z

datatypes/src/main/java/org/hyperledger/besu/datatypes/BLSPublicKey.java

+public class BLSPublicKey extends DelegatingBytes
+    implements org.hyperledger.besu.plugin.data.PublicKey {


I think BLSPublicKey and BLSSignature are OK as new types since they are quite a general concept and could imagine them being reused in the future.
Whereas DepositWithdrawalsCredential is quite specific Bytes32 is better IMO...it's not clear to me from the EIP what format this credential takes exactly, maybe there's an existing data type that is appropriate?

Possibly BLSPublicKey and BLSSignature shouldn't live in data types though since they are not broadly used. Maybe should live alongside Deposit.java for now?

jframe

Happy with the PR now too. Agree with @siladu the only thing missing is a test for the depositRoot in the BodyValidationTest

Signed-off-by: Navie Chan <naviechan@gmail.com>

Signed-off-by: navie <naviechan@gmail.com>

siladu · 2023-03-28T07:23:24Z

As a regression test, I span up 1x sepolia (snap sync), 1x goerli and 2x mainnet nodes which all got into sync and have stayed in sync for a number of days

siladu

LGTM 🎉

Add deposits to the Execution Layer block structure: EIP-6110. The scope of this commit is to add Deposit related info into BlockHeader and BlockBody. The rest of the EIP including RPC API and validating Deposit with be included in future PRs. --------- Signed-off-by: Navie Chan <naviechan@gmail.com> Signed-off-by: navie <naviechan@gmail.com> Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

non-fungible-nelson added the EIP Ethereum Improvement Proposal label Feb 16, 2023

siladu added the mainnet label Feb 18, 2023

ensi321 force-pushed the eip-6110 branch 5 times, most recently from f39bb57 to 8ebb906 Compare February 19, 2023 14:50

Initial commit

d57d70b

Signed-off-by: Navie Chan <naviechan@gmail.com>

ensi321 force-pushed the eip-6110 branch from 8ebb906 to d57d70b Compare February 19, 2023 14:53

Merge branch 'main' into eip-6110

1c8de5a

Signed-off-by: Navie Chan <naviechan@gmail.com>

ensi321 force-pushed the eip-6110 branch 2 times, most recently from aecb5c8 to 1c8de5a Compare February 19, 2023 15:24

ensi321 added 4 commits February 19, 2023 23:30

Added object classes for deposit fields

559b12a

Signed-off-by: Navie Chan <naviechan@gmail.com>

Spotless apply

22d4aa4

Signed-off-by: Navie Chan <naviechan@gmail.com>

Fix up compilation issue

97289b2

Signed-off-by: Navie Chan <naviechan@gmail.com>

Fix up compilation issue

1247429

Signed-off-by: Navie Chan <naviechan@gmail.com>

siladu added the TeamGroot GH issues worked on by Groot Team label Feb 20, 2023

github-advanced-security bot found potential problems Feb 20, 2023

View reviewed changes

Fix up compilation issue

2e68cca

Signed-off-by: Navie Chan <naviechan@gmail.com>

Update existed tests

91a851f

Signed-off-by: Navie Chan <naviechan@gmail.com>

ensi321 marked this pull request as ready for review February 20, 2023 15:39

Merge branch 'main' into eip-6110

e0eb573

ensi321 changed the title ~~EIP-6110: Handle validator deposits in EL~~ EIP-6110: Add deposits in EL Feb 20, 2023

ensi321 added 12 commits March 11, 2023 13:37

Add unstable annotation

15fd353

Signed-off-by: navie <naviechan@gmail.com>

Put deposits root after excess gas in BlockHeader

ba461ce

Signed-off-by: navie <naviechan@gmail.com>

Add deposits root to GetBodiesFromPeerTask.BodyIdentifier

8ae831f

Signed-off-by: navie <naviechan@gmail.com>

Update tests

3470caa

Signed-off-by: navie <naviechan@gmail.com>

Remove DepositWithdrawalCredential

7065425

Signed-off-by: navie <naviechan@gmail.com>

Update test

6995414

Signed-off-by: navie <naviechan@gmail.com>

Update test

7d3fc83

Signed-off-by: navie <naviechan@gmail.com>

Merge remote-tracking branch 'remote/main' into eip-6110

46fa21a

Signed-off-by: navie <naviechan@gmail.com>

Update test

ed6f99f

Signed-off-by: navie <naviechan@gmail.com>

Apply spotless

bce7b56

Signed-off-by: navie <naviechan@gmail.com>

Update hash

e8f5b96

Signed-off-by: navie <naviechan@gmail.com>

Fix up some test

33adc69

Signed-off-by: navie <naviechan@gmail.com>

github-advanced-security bot found potential problems Mar 20, 2023

View reviewed changes

...reum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/MainnetBlockBodyValidator.java

@@ -304,4 +308,8 @@

return true;

}

private boolean validateDeposits(final Block unusedBlock) {

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'unusedBlock' is never used.

siladu reviewed Mar 21, 2023

View reviewed changes

siladu requested a review from jframe March 21, 2023 05:38

jframe reviewed Mar 22, 2023

View reviewed changes

ensi321 added 3 commits March 27, 2023 01:19

Add DepositsRoot test and related block files

da838f6

Signed-off-by: Navie Chan <naviechan@gmail.com>

Merge remote-tracking branch 'remote/main' into eip-6110

4efac87

Signed-off-by: navie <naviechan@gmail.com>

Fix conflict

34453d4

Signed-off-by: navie <naviechan@gmail.com>

ensi321 requested a review from siladu March 28, 2023 06:24

siladu approved these changes Mar 28, 2023

View reviewed changes

siladu merged commit 3aef587 into hyperledger:main Mar 28, 2023

ensi321 deleted the eip-6110 branch April 3, 2023 04:20

ensi321 mentioned this pull request Apr 11, 2023

EIP-6110: Add deposit validation and apis #5295

Merged

3 tasks

mkalinin mentioned this pull request Jul 21, 2023

Propose Navie Chan protocolguild/documentation#93

Closed

philknows mentioned this pull request Jan 18, 2024

Add NC to Lodestar protocolguild/documentation#127

Merged

scorbajio mentioned this pull request Mar 5, 2024

[EIP-6110] Supply validator deposits on chain ethereumjs/ethereumjs-monorepo#3299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-6110: Add deposits in EL #5055

EIP-6110: Add deposits in EL #5055

ensi321 commented Feb 5, 2023 •

edited

Loading

siladu commented Feb 10, 2023

siladu commented Feb 18, 2023 •

edited

Loading

ensi321 commented Feb 19, 2023 •

edited

Loading

siladu commented Feb 20, 2023

siladu commented Feb 20, 2023

ensi321 commented Feb 20, 2023 •

edited

Loading

ensi321 commented Feb 22, 2023 •

edited

Loading

siladu left a comment

siladu Mar 21, 2023

jframe left a comment

siladu commented Mar 28, 2023

siladu left a comment

		public class BLSPublicKey extends DelegatingBytes
		implements org.hyperledger.besu.plugin.data.PublicKey {

EIP-6110: Add deposits in EL #5055

EIP-6110: Add deposits in EL #5055

Conversation

ensi321 commented Feb 5, 2023 • edited Loading

PR description

Fixed Issue(s)

Documentation

Changelog

siladu commented Feb 10, 2023

siladu commented Feb 18, 2023 • edited Loading

ensi321 commented Feb 19, 2023 • edited Loading

siladu commented Feb 20, 2023

siladu commented Feb 20, 2023

ensi321 commented Feb 20, 2023 • edited Loading

ensi321 commented Feb 22, 2023 • edited Loading

siladu left a comment

Choose a reason for hiding this comment

siladu Mar 21, 2023

Choose a reason for hiding this comment

jframe left a comment

Choose a reason for hiding this comment

siladu commented Mar 28, 2023

siladu left a comment

Choose a reason for hiding this comment

ensi321 commented Feb 5, 2023 •

edited

Loading

siladu commented Feb 18, 2023 •

edited

Loading

ensi321 commented Feb 19, 2023 •

edited

Loading

ensi321 commented Feb 20, 2023 •

edited

Loading

ensi321 commented Feb 22, 2023 •

edited

Loading