feat(deposits): Migration to EIP-6110 style processing#2794
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2794 +/- ##
==========================================
+ Coverage 60.73% 61.21% +0.48%
==========================================
Files 369 369
Lines 18847 18922 +75
==========================================
+ Hits 11446 11584 +138
+ Misses 6456 6380 -76
- Partials 945 958 +13
🚀 New features to boost your workflow:
|
|
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. |
fridrik01
left a comment
There was a problem hiding this comment.
did a quick pass, overall looks good but will need to do another
| blockToFetch := blockNum - eth1FollowDistance | ||
| deposits, err := depositContract.ReadDeposits(ctx, blockToFetch) | ||
| if err != nil { | ||
| logger.Error("Failed to read deposits", "block", blockNum, "error", err) |
There was a problem hiding this comment.
is it safe to drop the error and only log, we had a retry mechanism before in depositCatchupFetcher?
There was a problem hiding this comment.
yes, this doesnt change any behavior. That retry mechanism was non-effectual, long overdue cleaning of tech debt
| // If we already fetched deposits for this block, we don't need to do it again. | ||
| if depositContract.LastBlockNumber() == lph.GetNumber() { |
There was a problem hiding this comment.
is this to guard against calling this several times from different places if isFirstFuluBlock is true?
There was a problem hiding this comment.
Yes this guards only to prevent catchup. We will return from this function early if we are not in the first fulu block. If we are in first fulu block we need to run this fetch once amongst prepare/processProposal and finalizeBlock
|
@calbera we should test this soon on devnet with load testing deposits |
| // LastBlockNumber returns the last block number that was successfully | ||
| // consumed from the deposit contract. | ||
| func (dc *WrappedDepositContract) LastBlockNumber() math.U64 { | ||
| return math.U64(dc.lastBlockNumber.Load()) |
There was a problem hiding this comment.
@calbera something I noticed: lastBlockNumber zero-value doesn't distinguish "fetched at block 0" from "never fetched." Should be harmless in practice since IIUC genesis deposits bypass contract events, but wanted to call it out anyway.
There was a problem hiding this comment.
yeah thats true, but for the purpose of deposit contract, its functionally equivalent to say that we "never fetched" deposits if the last block fetched was 0 since 0 block will not have any deposits. We start at 1 onwards.
|
|
||
| // If on the first block of Fulu, catchup the previous block's deposits. Between | ||
| // Prepare/ProcessProposal and FinalizeBlock, this only needs to be done once. | ||
| func CatchupFuluDeposits( |
There was a problem hiding this comment.
nit: unit tests for these would be nice (though they should be covered in simulation/integration tests)
There was a problem hiding this comment.
I didn't add unit tests for this file since most of the logic already existed in our processproposal / finalizeblock flows, so refactoring it to here should change no behavior, which can be enforced by syncing mainnet, and other e2e tests
Signed-off-by: Cal Bera <calbera@berachain.com>
Replace the standard EIP-6110 DepositEvent signature with Berachain's custom Deposit event: Deposit(bytes,bytes,uint64,bytes,uint64) → 0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46. The Berachain event differs in two ways: the event name is Deposit (not DepositEvent) and amount/index are uint64 rather than bytes, requiring little-endian conversion when constructing deposit request bytestrings. Enabled in the CL [here](berachain/beacon-kit#2794) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * EIP-6110 deposit request handling is enabled for the Prague hardfork, using the configured deposit contract and deposit event topic. * Added support for a custom deposit event format and exposed deposit parsing so deposit requests are included in block payloads. * **Tests** * Added end-to-end tests validating deposit request creation, payload contents when active, and absence when no deposit transactions occur. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Cal Bera <calbera@berachain.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: bar-bera <bearbaresco@berachain.com>
EIP-6110 style deposits
3 requirements
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 Fulu fork (which starts using 6110) is activated.
Forking on the EL to change the deposit event signature to Berachain's:
0x68af751683498a9f9be59fe8b0d52a64dd155255d85cdb29fea30b1e3f891d46(sample deposit)TODOs:
Remaining Testing
On a live devnet/testnet, ensure that timeouts do not occur when building first Fulu block as the build and verification procedures for this block now include an extra JSON RPC call to the EL. Look for PrepareProposal/ProcessProposal/FinalizeBlock timeouts.