-
Notifications
You must be signed in to change notification settings - Fork 257
[WIP] feat(deposits): Introduce Electra1 fork for EIP-6110 style processing #2794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2794 +/- ##
==========================================
- Coverage 60.71% 60.54% -0.17%
==========================================
Files 349 349
Lines 16117 16183 +66
Branches 22 22
==========================================
+ Hits 9785 9798 +13
- Misses 5586 5638 +52
- Partials 746 747 +1
🚀 New features to boost your workflow:
|
| requests, err := blk.GetBody().GetExecutionRequests() | ||
| if err != nil { | ||
| // EIP-6110 Deposits. | ||
| for _, dep := range requests.Deposits { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the biggest change is here: Post this fork, we will just accept the deposits from here as we have instant finality. No deposits queue or epoch processing is necessary here; real-time instant deposits very easily done
|
Supportive of this approach. Especially since its <200 line diff and gets rid of the CL querying the EL which isn't a great pattern. Agreed that the Block Gas Limit is a good enough protection from hitting the deposit queue. It's unlikely we'll be x10'ing the gas limit arbitrarily in the coming months as that's not a bottleneck. As you noted, the EL change is trivial. There is a point to be noted that we deviate from the spec by removing the deposit queue altogether, which is present in the spec. In the spec, deposit processing is done per epoch, whereas we do it per block. This allows us to avoid any BeaconState changes. IMO the processing overhead of deposits is low enough that it doesn't need to be batched into an epoch (don't have empirical data to support this tho). Unsure on the solution for the overlap period between old system and new. Perhaps it's just a custom state transition as part of |
It does "deviate from spec" but not any more than we already are "deviated from the spec". Currently also deposits are processed block by block and the state transition operation for each is not too expensive (signatures are not verified on repeat -- most -- deposits). This change only affects the "lag" at which deposits are processed but not the frequency of processing itself. |
|
I don't think the risk of loosing deposits is acceptable, nor like the idea of adding code to patch it if it happens. |
Making deposits at the EL level will require specific opinionated code, potentially in the EVM. This would defeat the purpose of not adding code to the CL as we'd have to manage this regardless. Will work on adding the special state transition to catch up the 2 blocks deposits |
Super simple implementation of EIP-6110 style deposits
3 requirements for the implementation to be so simple:
Given an 8192 limit of deposits per block, we need to guarantee that an EL doesn't include more than 8192 deposits in 1 block.
Given the way deposits are taken from EL deposit contract events and then processed in CL, as is we will lose deposits from the 2 blocks right before the Electra1 fork (which starts using 6110) is activated.
Forking on the EL to change the deposit event signature to Berachain's:
0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46(sample deposit)TODOs: