beacon-kit icon indicating copy to clipboard operation
beacon-kit copied to clipboard

[WIP] feat(deposits): Introduce Electra1 fork for EIP-6110 style processing

Open calbera opened this issue 7 months ago • 5 comments

Super simple implementation of EIP-6110 style deposits

3 requirements for the implementation to be so simple:

  1. 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.

    • This is guaranteed with the current gas limit of 30 million.
    • 1 deposit on Berachain costs at least 45,000 gas (sample deposit)
    • 8192 deposits would cost 45,000 * 8192 = 368,640.000 gas
    • As long as gas per block stays under 368 million gas, we are good ✅
      • If we do increase gas, we can also increase the maximum amount of 8192 accordingly.
  2. 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.

    • Currently the way the system works:
      • in finalize_block N, we store any new EL deposits from EL block N-1
      • in build_block N+1, we include these new EL deposits from EL block N-1
    • With 6110:
      • in build_block N, the EL will now include the deposits events that are in EL block N itself
    • So if Electra1 activates at block N+1 (previously for this block N+1 we would have included deposits from EL block N-1). Now with new fork, we will just include for block N+1 the deposits from EL block N+1. This means EL deposits from blocks N-1 & N will be skipped 🚨
    • ⚠️ Potential workarounds include:
      • asking people to not deposit for the 5 seconds before the hard fork lol
      • adding a special case around the fork activation that retrieves the previous 2 EL blocks' deposits to specifically include them
  3. Forking on the EL to change the deposit event signature to Berachain's: 0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46 (sample deposit)

    • 1 line change in geth here
    • Change(s) in In reth here

TODOs:

  • [ ] Handle edge case around fork activation. Approach TBD
  • [ ] Unit tests for deposit requests.
  • [ ] Simulation tests for deposit requests.

calbera avatar May 28 '25 22:05 calbera

Codecov Report

Attention: Patch coverage is 41.48148% with 79 lines in your changes missing coverage. Please review.

Project coverage is 60.54%. Comparing base (87d043a) to head (fdac98b).

Files with missing lines Patch % Lines
state-transition/core/state_processor_forks.go 2.43% 40 Missing :warning:
state-transition/core/state_processor_staking.go 61.22% 14 Missing and 5 partials :warning:
beacon/validator/block_builder.go 45.45% 14 Missing and 4 partials :warning:
chain/spec.go 33.33% 2 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
beacon/blockchain/finalize_block.go 69.84% <100.00%> (-0.24%) :arrow_down:
config/spec/mainnet.go 100.00% <100.00%> (ø)
state-transition/core/validation_deposits.go 50.00% <ø> (ø)
chain/spec.go 73.77% <33.33%> (-1.02%) :arrow_down:
beacon/validator/block_builder.go 62.06% <45.45%> (-1.47%) :arrow_down:
state-transition/core/state_processor_staking.go 61.49% <61.22%> (-0.22%) :arrow_down:
state-transition/core/state_processor_forks.go 64.67% <2.43%> (-18.41%) :arrow_down:

... and 1 file with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar May 28 '25 22:05 codecov[bot]

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 upgradeToElectra1

rezbera avatar May 29 '25 22:05 rezbera

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).

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.

calbera avatar Jun 02 '25 20:06 calbera

I don't think the risk of loosing deposits is acceptable, nor like the idea of adding code to patch it if it happens. I like this proposal as an end state, but I believe the right way to activate it is to make deposits fail at the EL level for the blocks around fork activation (which then can be timestamp based). This way there are no loss of founds possible (just some gas), no code to be added to CL and possibly the deposits revert may be even remove post fork activation if no deposits are done in that window.

abi87 avatar Jun 05 '25 15:06 abi87

I don't think the risk of loosing deposits is acceptable, nor like the idea of adding code to patch it if it happens. I like this proposal as an end state, but I believe the right way to activate it is to make deposits fail at the EL level for the blocks around fork activation (which then can be timestamp based). This way there are no loss of founds possible (just some gas), no code to be added to CL and possibly the deposits revert may be even remove post fork activation if no deposits are done in that window.

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

calbera avatar Aug 31 '25 08:08 calbera